-
Notifications
You must be signed in to change notification settings - Fork 580
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 LED matrix bindings for IS31FL3731 driver #1926
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.
Couple of comments. Also, we should have a discussion regarding simple things like Color. With changes in the graphic libs, we need to come with something simple we can use anywhere for consistency
/// <param name="r">Red brightness value 0->255.</param> | ||
/// <param name="g">Green brightness value 0->255.</param> | ||
/// <param name="b">Blue brightness value 0->255.</param> | ||
public void WritePixelRgb(int x, int y, int r, int g, int b) |
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.
I know we are switching our graphic lib but we've been using Color in other bindings to pass colors. I would like to see that because of the predefined colors for example
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.
Fixed.
src/devices/Is31fl3731/Is31Fl3731.cs
Outdated
// Command register | ||
// Points pages one to nine | ||
// table 2 in datasheet | ||
private const byte COMMAND_REGISTER = 0xFD; |
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.
I'm a big fan of having all those in a Registers.cs file as an internal enum. It's much cleaner than using constants
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.
Not sure about that. It's gonna make the code look pretty ugly without target typed enums.
Let's take a look.
This line:
Write(FUNCTION_REGISTER, DISPLAY_REGISTER, (byte)value);
Will turn into:
Write(Registers.FUNCTION_REGISTER, Registers.DISPLAY_REGISTER, (byte)value);
Right? I think that makes the C# look worse as compared to the Python code I read. Seems like this is trading cleanup at the top of the file for worse readability within the core logic.
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.
I really prefer to use enums because they avoid problems. You can group them. And in this case have 2 different enums, one for the register and one for the command.
Short term const seems super nice. Then when it comes to maintenance, they are not good at all. Enums forces you to use the right one. Here on the Write function, I can use the wrong one very easilly.
So most likely your write will turn out like with 2 different enums. And when you have possibility to mix elements, it's much easier for maintenance and readability with enums:
Write(Function.FUNCTION_REGISTER, Register.DISPLAY_REGISTER, (byte)value);
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.
I get it now. Thanks for explanation. Basically "type safety".
(21, 0) => 15, | ||
(_, 0) => x - 14, | ||
// y == 1 | ||
(<2, 1) => 69 - x, |
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.
you're missing some spaces in the following lines
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.
Fixed.
/// <param name="r">Red brightness value 0->255.</param> | ||
/// <param name="g">Green brightness value 0->255.</param> | ||
/// <param name="b">Blue brightness value 0->255.</param> | ||
public void WritePixelRgb(int x, int r, int g, int b) |
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.
Same feedback as for the other one: usage of Color
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.
Fixed.
I need to re-test all the devices and then I'll switch this PR to "Ready for review". |
src/devices/Is31fl3731/Is31fl3731.cs
Outdated
// Command register | ||
// Points pages one to nine | ||
// table 2 in datasheet | ||
private const byte COMMAND_REGISTER = 0xFD; |
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.
rather than private const, a nice enum is always preferable especially in that case where you have quite a lot of them.
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.
I raised a concern on that. What are your thoughts? This is the official Python implementation: https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731/blob/main/adafruit_is31fl3731/__init__.py
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.
I commented then on the other comment.
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.
I took your advice.
I first tried enums. That looked pretty rough to me since I had to add a cast at every callsite: f3c7b63
I then switched to static classes. That resolved the issue: ecd652f
I left the API signatures as byte
. The reason is that there are multiple registers and their values overlap. Also, one of the registers takes values 0-7 for 8 pages of data. Forcing the use of an enum or static class to iterate from 0..7
doesn't make much sense.
Seem better?
/// </summary> | ||
// Enables pages one to nine | ||
// Table 2 in datasheet | ||
public static class CommandRegister |
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 public cstatic lasses and not enums? you don't seem to override them or anything like this.
// https://www.adafruit.com/product/2974 | ||
using I2cDevice i2cDevice = I2cDevice.Create(new I2cConnectionSettings(busId: 1, Backpack16x9.DefaultI2cAddress)); | ||
Backpack16x9 matrix = new(i2cDevice); | ||
// For 16x8 matrix Charlieplex bonet |
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.
maybe itroduce an empty line between each block of matrix? you can as well use 4 slash or dashes or anything to make it more redable
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.
Good call. Done.
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.
Very minor comments, all look very nice :-) mpatient to buy few of those and find time to use them :-)
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 that (I think) explain the build failures.
@@ -0,0 +1,11 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
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.
Please put all sample projects into separate folder (samples\sample1, samples\sample2, etc.). Having multiple .csproj files in the same folder causes problems on macos due to a compiler bug.
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.
I believe that there are multiple bindings that have that problem. Is this our new policy?
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFrameworks>net60</TargetFrameworks> |
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.
TFM must be net6.0
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFrameworks>net60</TargetFrameworks> |
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.
TFM must be net6.0
.
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. |
I just looked through the logs. None of the errors seem related to this PR. |
This needs a conflict resolution. |
Used by:
Port of https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731.
Microsoft Reviewers: Open in CodeFlow