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.

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

New Device: LED segment display 5641AS #2350

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

Conversation

jabberhams
Copy link
Contributor

@jabberhams jabberhams commented Aug 27, 2024

New device LedSegmentDisplay5641AS added. The device has fewer display characteristics (no colon or degrees) and while other devices in the Display category were driven by backpacks over I2C, this code is full GPIO only. In addition to the device, a dedicated pin scheme LedSegmentDisplay5641ASPinScheme is for mapping board pins to device pins, and a helper class translates Fonts to segment assignments. Readme also updated.

My first contribution so looking for constructive feedback where necessary.

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 Aug 27, 2024
@jabberhams
Copy link
Contributor Author

@dotnet-policy-service agree

Copy link
Contributor Author

@jabberhams jabberhams left a comment

Choose a reason for hiding this comment

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

Fixed markdown syntax for local image

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.

Thanks, couple of questions and remarks. It looks good all up.

/// <summary>
/// Opens GPIO pins defined in <see cref="LedSegmentDisplay5641ASPinScheme"/> as <see cref="PinMode.Output"/>
/// </summary>
public void Open()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have the Open and Close functions? Can Open be done in the constructor and Close in the Dispose? So it's really fully transparent for the user?
You can keep the function and use them but then change the visibility in private in that case.
I may have miss something why you need an Open and Close as public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, just carryover from testing. I will make the changes.

public int NumberOfDigits => _pinScheme.DigitCount;

/// <inheritdoc />
public Segment this[int address] { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
Copy link
Member

Choose a reason for hiding this comment

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

is there any way to recreate a Segment? So that it's make easy for the user and kind of easier?

@@ -1,10 +1,14 @@
# HT16K33 - LED Matrix Display Driver
# LED Displays
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# LED Displays
# LED Displays - 5641AS, HT16K33 and LED Matrix Display Driver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants