-
Notifications
You must be signed in to change notification settings - Fork 585
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 Matrix binding for HT16K33 #1916
Conversation
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.
Nice additions, few minor comments
src/devices/Display/Matrix16x8.cs
Outdated
/// <summary> | ||
/// Writes bytes to matrix, row by row | ||
/// </summary> | ||
public void Write(ReadOnlySpan<byte> 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.
why you need this function while you have the previous one?
src/devices/Display/Matrix16x8.cs
Outdated
rowIndex++; | ||
} | ||
|
||
if (BufferingEnabled) |
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 have those 4 lines in many places, does it make sense to replace it by a simple function?
somthing like this?
private void FlushIfNeed() => BufferEnabled ? Flush(): return;
@@ -0,0 +1,128 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. | |||
// The .NET Foundation licenses this file to you under the MIT license. | |||
|
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 update the main README.md to add some of those elements?
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 idea. Will do that after the Bicolor PR merges.
@richlander looking at the PR, don't you want to add the nice animated gif as well in the README? At least it shows how things looks like. |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
@joperezr Any idea what's wrong with the MacOS build? This is confusing:
|
Yes, this failed due to a failing unit test in Button.tests.csproj. There is a bug in VSTest that when test projects are run in parallel, not all of them will log out the failures into the console, but if you take the build's binlog and create a diagnostic log out of it, you'll see that:
|
This PR should be ready to go now. I resolved conflicts with #1915, updated the README, and added a TPN entry. |
Thanks. We do need to find out what's wrong with the macos builds, though (certainly not related to the changes, but still weird) |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
I merged I broke compat with this change since I wanted to align this binding with the one I made just before this. Both have a bi-color capability, and they should share the same enum. How I break compat? Changes in question are in On that note, should I be using The markdown link check also failed, however, I can reach the link that failed. I'm guessing that's a one-off issue. It didn't fail in |
@richlander We don't really care much about such changes in Iot.Device.Bindings. If the unification makes sense (and it seems to me it does) the breaking change is typically ok. |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
Hey @richlander, took the liberty to push the breaking changes suppressions as well as the broken link issue (which wasn't related to your PR). Hope that's ok with you. |
86c5bb2
to
d27d8f0
Compare
I just re-tested all the devices. I found and fixed one (off by one) sample bug. Looks like the linter bug is the last one on this PR so I think we can merge this one. |
This PR adds i2c-based bindings for the Adafruit matrices. It uses the same HT16K33 chip as is used in the Display folder.
Products:
I didn't implement any animation capabilities for this binding. Same with fonts. That should be implemented a layer higher, IMO. We can discuss that.
There is an open question on how to implement all the different capabilities:
The Adafruit implementation uses one type for matrix type. That seems like the way to go.
Note: I didn't use the Adafruit implementation as the basis for my implementation. I just used an empirical approach to determine which bytes affected which parts of the matrix and then designed the implementation to do same.
This will have merge conflicts with #1915. I'll resolve those once one of them gets merged.
Microsoft Reviewers: Open in CodeFlow