New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BiColorBargraph using Ht16k33 #1915
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments but all up, looks good!
src/devices/Display/BarColor.cs
Outdated
/// <summary> | ||
/// Disable LED. | ||
/// </summary> | ||
OFF = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With our naming convention, those public enums should be like Off
, Red
, Green
, Yellow
.
// Product: https://www.adafruit.com/product/1721 | ||
public class BiColorBarGraph : Ht16k33 | ||
{ | ||
private byte[] _displayBuffer = new byte[7]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private readonly
maybe for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of those are being assigned to in the code. That means they cannot be readonly
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use _displayBuffer.AsSpan().Clear();
on L47 then it can be readonly
, as nothing -- except the ctor -- assigns a new reference to the fields.
Writing to an array element of a readonly array-field doesn't violate "readonly" if that was kind of the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Repeated what you proposed on the other PR. Thanks for showing me!
{ | ||
Flush(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation of this bracket seems missing some spaces
byte red = _displayBuffer[bufferIndex]; | ||
byte green = _displayBuffer[bufferIndex + 1]; | ||
|
||
if (value is BarColor.OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not a switch rather than the series of if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :-) Thanks for the addition :-)
This PR adds a binding for the Adafruit 24-bar bicolor bargraph. It uses the same HT16K33 chip as is used in the
Display
folder.I've also written a binding for 8x8 Matrices, which I'll add in another PR. This and bicolor bargraph are two additional bindings implemented at https://github.com/adafruit/Adafruit_CircuitPython_HT16K33 for this same chip.
I created a separate folder for HT16K33 in this PR. I had previously looked for it and not found it due to the
Display
folder naming, which I consider poorly chosen. We could rename theDisplay
if that's decided to be a good thing.Microsoft Reviewers: Open in CodeFlow