-
Notifications
You must be signed in to change notification settings - Fork 582
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 binding for IS31FL3730 LED driver #1930
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.
I see the draft, but hope you're OK with early feedback.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
|
||
if (CurrentSetting > 0) | ||
{ | ||
_i2cDevice.Write(new byte[] { LIGHTING_EFFECT_REGISTER, (byte)CurrentSetting }); |
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.
I2CDevice
has an overload that takes a ROS
public abstract void Write(ReadOnlySpan<byte> buffer); |
_i2cDevice.Write(new byte[] { LIGHTING_EFFECT_REGISTER, (byte)CurrentSetting }); | |
_i2cDevice.Write(stackalloc byte[] { LIGHTING_EFFECT_REGISTER, (byte)CurrentSetting }); |
Same below.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
if (value > 0) | ||
{ | ||
buffer.AsSpan().Fill(255); | ||
} | ||
else | ||
{ | ||
buffer.AsSpan().Fill(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.
if (value > 0) | |
{ | |
buffer.AsSpan().Fill(255); | |
} | |
else | |
{ | |
buffer.AsSpan().Fill(0); | |
} | |
buffer.AsSpan().Fill(value > 0 ? 255 : 0); |
More a matter of taste, but IL-size is also smaller (which I don't believe is a criteria here).
Or
if (value > 0) | |
{ | |
buffer.AsSpan().Fill(255); | |
} | |
else | |
{ | |
buffer.AsSpan().Fill(0); | |
} | |
if (value > 0) | |
{ | |
buffer.AsSpan().Fill(255); | |
} | |
else | |
{ | |
buffer.AsSpan().Clear(); | |
} |
Clear
is (in general) better than Fill(0)
(better optimization in the implementation).
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.
just FYI we prefer cleaner code to microoptimizations in this code-base except where it really matters. Reducing allocations - yes, microoptimization - not quite (except maybe direct GPIO operations). Here I2C transmission itself will likely take more than 99% of the time and this won't make any observable difference. I'd rather write this code as:
buffer.AsSpan().Fill(value > 0 ? 255 : 0);
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
private void Write(byte address, byte[] value) | ||
{ | ||
byte[] data = new byte[value.Length + 1]; | ||
data[0] = address; | ||
value.CopyTo(data.AsSpan(1)); | ||
_i2cDevice.Write(data); | ||
} |
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 void Write(byte address, byte[] value) | |
{ | |
byte[] data = new byte[value.Length + 1]; | |
data[0] = address; | |
value.CopyTo(data.AsSpan(1)); | |
_i2cDevice.Write(data); | |
} | |
private void Write(byte address, ReadOnlySpan<byte> value) | |
{ | |
Debug.Assert(value.Length < 256); | |
Span<byte> data = stackalloc byte[value.Length + ]; | |
data[0] = address; | |
value.CopyTo(data.Slice(1)); | |
_i2cDevice.Write(data); | |
} |
Avoids the temp. allocation.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
|
||
private void Write(byte address, byte value) | ||
{ | ||
_i2cDevice.Write(new byte[] { address, 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.
_i2cDevice.Write(new byte[] { address, value }); | |
_i2cDevice.Write(stackalloc byte[] { address, value }); |
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
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 for this very nice addition :-)
/// <param name="i2cDevice">The <see cref="System.Device.I2c.I2cDevice"/> to create with.</param> | ||
public BreakoutPair5x7(I2cDevice? i2cDevice = null) | ||
{ | ||
_i2cDevice = i2cDevice is not null ? i2cDevice : I2cDevice.Create(new(1, Is31fl3730.DefaultI2cAddress)); |
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.
We're not using this pattern and always leave the creation of the I2cDevice to the caller. Reason is because bus may be different, ownership as well and finally adaptors like FT4222 or Arduino. So don't pass the null as default and raise and exception if null.
{ | ||
_is31fl3730?.Dispose(); | ||
_is31fl3730 = null!; | ||
_i2cDevice?.Dispose(); |
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 the pattern we are using, as we don't create I2cDevice in the bindings, we don't dispose them. It's to the caller to take care of this.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
{ | ||
// Function register | ||
// table 2 in datasheet | ||
private const byte CONFIGURATION_REGISTER = 0x0; |
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 using constants, use a Register.cs file with enums in there being internal. Like this, you will be able to keep the register names like they are (all uppercase). And it's much better than constants.
src/devices/Is31fl3730/MatrixMode.cs
Outdated
/// <summary> | ||
/// Represents a 8x8 LED matrix. | ||
/// </summary> | ||
Size8x8, |
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.
As you are using explicit check of MatrixMode > 0 earlier, make the first element explicit to 0
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
/// </summary> | ||
public void UpdateDecimalPoint(int matrix, int value) | ||
{ | ||
int matrixOneMask = 128; |
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.
const for those 2 would be better as you are using them like this
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
{ | ||
Span<byte> data = stackalloc byte[value.Length + 1]; | ||
data[0] = address; | ||
value.CopyTo(data[1..]); |
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 guess you have to adjust the imports to add the new range (I love them btw :-))
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.
Is there guidance on how to make this work for .NET Standard 2.0? Is this the right pattern? https://www.meziantou.net/how-to-use-csharp-8-indices-and-ranges-in-dotnet-standard-2-0-and-dotn.htm
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 went with this: d82f7ff
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
/// <param name="x">The x dimension for the LED.</param> | ||
/// <param name="y">The y dimension for the LED.</param> | ||
/// <param name="value">The value to write.</param> | ||
public void WriteLed(int matrix, int x, int y, int 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.
Something to consider as you only have 2 matrix: use an enum rather than an int?
/// <param name="i2cDevice">The <see cref="System.Device.I2c.I2cDevice"/> to create with.</param> | ||
public BreakoutPair5x7(I2cDevice? i2cDevice = null) | ||
{ | ||
_i2cDevice = i2cDevice is not null ? i2cDevice : I2cDevice.Create(new(1, Is31fl3730.DefaultI2cAddress)); |
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 remark as for the other one, we don't create I2cDevice from the bindings.
{ | ||
_is31fl3730?.Dispose(); | ||
_is31fl3730 = null!; | ||
_i2cDevice?.Dispose(); |
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.
And similar as my comment for the other one, no need to dispose as we don't create it in the binding
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 followed that guidance, however, when I came to supporting the Micro Dot pHAT, I found that there was a need to specify three I2C addresses. That's a bit of a pain. What do you think?
Here are two constructors: 0d1e95f#diff-c1aa6bb26153d700cceaa57064762e4c6dce51672088c47a5f883ded6f76c12eR28-R47
I'm happy to do whatever you think best. I wanted to demonstrate the UX of the two approaches.
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 resolved this.
is this very different from #1926? Should the two be completely independent? |
Yes. They are very different. The two drivers do not work similarly. If you look at the datasheets, you'll see that there very little similarity w/no opportunity for code sharing. Still, good 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.
Looks mostly good. Some general comments:
- Documentation of some classes could be improved (was apparently just copy-pasted in some places)
- Review the visibility of enums and constants. Ideally, bindings should not expose register numbers and other low-level constants.
src/devices/Is31fl3730/Current.cs
Outdated
/// <summary> | ||
/// Represents an IS31FL3731 LED Matrix driver | ||
/// </summary> | ||
public enum Current |
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.
Documentation is apparently wrong. Should be something about (maximum?) LED current.
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 copied the text from the datasheet.
/// <summary> | ||
/// Represents an IS31FL3731 LED Matrix driver | ||
/// </summary> | ||
public enum DisplayMode |
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.
Again: This documentation doesn't describe the enum
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
public DisplayMode DisplayMode { get; set; } | ||
|
||
/// <summary> | ||
/// Enables or disables auto-buggering. |
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.
Typo in comment
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
/// <param name="x">The x dimension for the LED.</param> | ||
/// <param name="y">The y dimension for the LED.</param> | ||
/// <param name="value">The value to write.</param> | ||
public void Write(int matrix, int x, int y, int 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.
Since apparently, value
is either 0 or 1, wouldn't it be better to use a bool or a PinValue
type? Or is this designed to support displays with colors, too? (The method isn't virtual or from an interface, though)
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/Is31fl3730/Is31fl3730.cs
Outdated
/// <summary> | ||
/// Brightness of LED matrix (override default value (128; max brightness); set before calling Initialize method). | ||
/// </summary> | ||
public int Brightness { get; set; } | ||
|
||
/// <summary> | ||
/// Full current setting for each row output of LED matrix (override default value (40 mA)); set before calling Initialize method). | ||
/// </summary> | ||
public Current Current { get; set; } |
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.
Is there a reason why we don't allow setting Current and Brightness after Initialization? At the moment, setting these members after initialisation has no effect (not even an exception), but it looks like writing the associated registers should always be possible.
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.
+1, the interest of having such properties is for them to be dynamic, so you can adjust them on the fly without having to call the initialize function
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. Good call.
/// <summary> | ||
/// Matrix Values. | ||
/// </summary> | ||
public static class MatrixValues |
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.
Does this need to be public?
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. Fixed.
src/devices/Is31fl3730/Registers.cs
Outdated
/// <summary> | ||
/// Register addresses for the Function Register. | ||
/// </summary> | ||
public static class FunctionRegister |
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.
There's probably no need to make this public.
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. Fixed.
src/devices/Is31fl3730/Registers.cs
Outdated
/// <summary> | ||
/// Register addresses for the Configuration Register. | ||
/// </summary> | ||
public static class ConfigurationRegister |
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.
and this, too.
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.
and each enum should be in its own file
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.
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 more comments.
src/devices/Is31fl3730/Current.cs
Outdated
/// <summary> | ||
/// 40 mA | ||
/// </summary> | ||
CmA40 = 0b0, |
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.
even if those are just 0, maybe better to align with 4 0 like the rest?
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
/// <summary> | ||
/// Brightness of LED matrix (override default value (128; max brightness); set before calling Initialize method). | ||
/// </summary> | ||
public int Brightness { get; set; } | ||
|
||
/// <summary> | ||
/// Full current setting for each row output of LED matrix (override default value (40 mA)); set before calling Initialize method). | ||
/// </summary> | ||
public Current Current { get; set; } |
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.
+1, the interest of having such properties is for them to be dynamic, so you can adjust them on the fly without having to call the initialize function
src/devices/Is31fl3730/Registers.cs
Outdated
/// <summary> | ||
/// Register addresses for the Configuration Register. | ||
/// </summary> | ||
public static class ConfigurationRegister |
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.
and each enum should be in its own file
Co-authored-by: Laurent Ellerbach <laurelle@microsoft.com>
I'm having some namespace problems, as you can see with 0921724. Any advice on that would be appreciated. |
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.
Thanks, looks great to me now!
Thanks for all the help on these bindings. I learned a lot. |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
LGTM. You might wish to take a look at input validation, though.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
// Set high bit for the 128th item | ||
// Set high bit low for the remaining 127 items | ||
// And then set lower bits as appropriate, to represent 0-127 | ||
public void UpdateBrightness(byte value) => Write(Is31fl3730FunctionRegister.Pwm, 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.
According to the documentation, not all values are valid (only 0-128). If that's correct, this method should contain some input validation.
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.
Interesting. That's a good point. Fair.
src/devices/Is31fl3730/Is31fl3730.cs
Outdated
/// <summary> | ||
/// Update current setting for each row output of LED matrix (default value is 40 mA). | ||
/// </summary> | ||
public void UpdateCurrent(Current value) => Write(Is31fl3730FunctionRegister.LightingEffect, (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.
While we're not doing that everywhere correctly, it would be good to check for valid values here as well. The user could pass any value that is not part of the enum.
Used by:
Port of https://github.com/pimoroni/microdot-phat.
Microsoft Reviewers: Open in CodeFlow