Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add the Pimoroni Blinkt device binding #2370

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add the Pimoroni Blinkt device binding #2370

wants to merge 9 commits into from
+391 −0

Conversation

jimbobbennett
Copy link

@jimbobbennett jimbobbennett commented Dec 28, 2024 •
edited by dotnet-policy-service bot
Loading

Fixes #2369

This adds a device binding for the Pimoroni Blinkt.

Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Dec 28, 2024
@jimbobbennett jimbobbennett changed the title Add the Pimoroni Blinkt device Add the Pimoroni Blinkt device binding Dec 28, 2024
pgrawehr
pgrawehr requested changes Dec 30, 2024
View reviewed changes
Copy link
Contributor

@pgrawehr pgrawehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid start, thank you. Added some improvement suggestions to make the binding more versatile.

src/devices/Blinkt/Blinkt.cs Outdated Show resolved Hide resolved
src/devices/Blinkt/Blinkt.cs Outdated Show resolved Hide resolved
src/devices/Blinkt/Blinkt.cs Outdated Show resolved Hide resolved
src/devices/Blinkt/Blinkt.cs Outdated Show resolved Hide resolved
src/devices/Blinkt/samples/README.md Outdated Show resolved Hide resolved
@dotnet-policy-service dotnet-policy-service bot added the Needs: Author Feedback We are waiting for author to react to feedback (action required) label Dec 30, 2024
@pgrawehr
Copy link
Contributor

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet-policy-service dotnet-policy-service bot removed the Needs: Author Feedback We are waiting for author to react to feedback (action required) label Jan 3, 2025
@jimbobbennett jimbobbennett requested a review from pgrawehr January 3, 2025 22:02
Ellerbach
Ellerbach reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good all up! Thanks!

src/devices/Blinkt/Blinkt.cs Outdated
/// <summary>
/// The number of pixels in the Blinkt strip
/// </summary>
public const int NUMBER_OF_PIXELS = 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For constants, we're using like for variables Pascal Case. So, this one should be NumberOfPixels. We do exceptions for internal registers for examples to be closer to the binding documentation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

src/devices/Blinkt/README.md Outdated Show resolved Hide resolved
src/devices/Device-Index.md Outdated
@@ -20,6 +20,7 @@
* [AXP192 - Enhanced single Cell Li-Battery and Power System Management IC](Axp192/README.md)
* [Bh1745 - RGB Sensor](Bh1745/README.md)
* [BH1750FVI - Ambient Light Sensor](Bh1750fvi/README.md)
* [Blinkt - 8-LED indicator strip](Blinkt/README.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not necessary and it's done automatically at merge time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

src/devices/README.md Outdated
@@ -203,6 +203,7 @@ Our vision: the majority of .NET bindings are written completely in .NET languag

* [Adafruit Seesaw - extension board (ADC, PWM, GPIO expander)](Seesaw/README.md)
* [APA102 - Double line transmission integrated control LED](Apa102/README.md)
* [Blinkt - 8-LED indicator strip](Blinkt/README.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not necessary and it's done automatically at merge time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@Ellerbach
Copy link
Member

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Ellerbach
Ellerbach approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Make the linter happy and good to merge :-)

@jimbobbennett
Copy link
Author

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 2370 in repo dotnet/iot

@jimbobbennett
Copy link
Author

Looks good! Make the linter happy and good to merge :-)

Thanks. Removed the only issue I could see from the linter, but don't have rights to run this myself to test.

@jimbobbennett
Copy link
Author

I see the Arduino build has failed some unit tests. I can't see where these have happened as the output doesn't show any failures. Is this a flaky test that can be re-run, or is there more information I can use to resolve this?

@raffaeler
Copy link
Contributor

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@raffaeler
Copy link
Contributor

@pgrawehr Could you please take a look at the failures? They are unrelated to this PR

@pgrawehr
Copy link
Contributor

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgrawehr pgrawehr pgrawehr approved these changes

@Ellerbach Ellerbach Ellerbach approved these changes

Assignees
No one assigned
Labels
area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add device binding for the Pimoroni Blinkt LEDs
4 participants
@jimbobbennett @pgrawehr @Ellerbach @raffaeler