Skip to content

Conversation

@xingrz
Copy link
Member

@xingrz xingrz commented Feb 3, 2024

This PR introduces a new driver for a common GPIO-driven 7-segment display. supporting both common anode and common cathode configurations.

It also includes a new sample demonstrates counting from 0 to 1000, with an interval of 100ms between each increment.

506611706963670_ pic

@xingrz xingrz force-pushed the auxdisplay-7seg/pr branch from 68be124 to 682a00d Compare February 3, 2024 12:39
@xingrz xingrz marked this pull request as ready for review February 3, 2024 12:49
@xingrz xingrz requested a review from thedjnK as a code owner February 3, 2024 12:49
@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples area: Aux display labels Feb 3, 2024
@zephyrbot zephyrbot requested review from galak, kartben and nashif February 3, 2024 12:50
Copy link
Contributor

Choose a reason for hiding this comment

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

For the other drivers, this always writes from the last position, whilst for a 7-segment display you might want (or expect) that, it deviates from the API:

 * @brief	Write data to auxiliary display screen at current position

What are your thoughts on adding position get/set and having write update the current character and advancing the position (also thinking this can be used on hex segment displays also)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually a consideration for handling decimal points. For example, if you want to display 10.3, the decimal point is the third character in the data. However, on a 7-segment display, it is part of the second digit (0). Therefore, I need to do some processing in the write function, so that when the current character is identified as '.', the decimal point of the previous digit is lit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good if this sample adapted to the number of characters in the display

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not check that a dot segment is present

Copy link
Member Author

Choose a reason for hiding this comment

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

It's okay here since it's just setting the buffer bits and not actually updating the GPIOs directly. The DP bit will be ignored if only 7 GPIOs are present, as handled in the auxdisplay_gpio_7seg_timer_expiry_fn function below.

@xingrz xingrz force-pushed the auxdisplay-7seg/pr branch from 682a00d to 9a37357 Compare February 9, 2024 04:36
@xingrz
Copy link
Member Author

xingrz commented Mar 19, 2024

Oh, wow! I have a new idea. Maybe I should separate this into an LED strip (or LED controller) driver, probably named "common-electrode-leds", and build the segment display driver on top of it. So, in theory, we could then drive any any-segment display with any form of multi-channel LED controller.

@thedjnK
Copy link
Contributor

thedjnK commented Apr 9, 2024

Oh, wow! I have a new idea. Maybe I should separate this into an LED strip (or LED controller) driver, probably named "common-electrode-leds", and build the segment display driver on top of it. So, in theory, we could then drive any any-segment display with any form of multi-channel LED controller.

It's an option. For LED displays they are usually numeric, alphanumeric or hex, so auxdisplay can work with all of those.

@github-actions
Copy link

github-actions bot commented Jun 9, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 9, 2024
@github-actions github-actions bot closed this Jun 24, 2024
@cfriedt
Copy link
Member

cfriedt commented Aug 12, 2024

@xingrz - is there any chance that this could be reopened? I guess that this fell off the radar during the last release cycle.

I would be happy to re-review 😄

@xingrz
Copy link
Member Author

xingrz commented Aug 13, 2024

@cfriedt Well, I’m not sure. I’m uncertain if this idea is fully developed. I’ve noticed that in actual projects, people often use dedicated LED driver ICs or GPIO expanders instead of the MCU’s GPIOs to drive the 7-segment displays. This approach provides enough driving power and minimizes IO usage. I think we should consider these scenarios, but I don’t have any concrete ideas yet. Let’s keep this PR closed for now. Perhaps in the future, someone interested might discover it and offer some new insights.

@simonguinot simonguinot self-requested a review August 13, 2024 08:45
@thedjnK
Copy link
Contributor

thedjnK commented Aug 13, 2024

Port expanders are natively supported in zephyr through the GPIO interface so should be fine

@cfriedt
Copy link
Member

cfriedt commented Aug 13, 2024

I have a board in front of me that just uses GPIO as well without a driver. Depending on the brightness level required, I think it's reasonable in some circumstances.

@xingrz xingrz reopened this Aug 14, 2024
@github-actions github-actions bot removed the Stale label Aug 15, 2024
Copy link
Contributor

@thedjnK thedjnK left a comment

Choose a reason for hiding this comment

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

Wasn't aware this got updated, minor change request for the binding compatible then looks good to go!

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Nov 11, 2024
@cfriedt cfriedt removed the Stale label Nov 12, 2024
@cfriedt
Copy link
Member

cfriedt commented Nov 12, 2024

This is so close to being done @xingrz

@xingrz xingrz force-pushed the auxdisplay-7seg/pr branch from 9a37357 to 74228b0 Compare November 12, 2024 04:58
@decsny decsny added this to the v4.1.0 milestone Dec 20, 2024
@xingrz
Copy link
Member Author

xingrz commented Dec 21, 2024

Hi @cfriedt . No rush to merge right now. I didn't realize the Linux binding existed before, and I’m thinking about how to handle compatibility. Once merged, it might be harder to deal with that.

@kartben kartben modified the milestones: v4.1.0, v4.2.0 Feb 15, 2025
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 17, 2025
@github-actions github-actions bot closed this May 1, 2025
@decsny
Copy link
Member

decsny commented May 5, 2025

Hi @cfriedt . No rush to merge right now. I didn't realize the Linux binding existed before, and I’m thinking about how to handle compatibility. Once merged, it might be harder to deal with that.

My point was that the binding seemed almost exactly the same as the linux one, so all that should need to be done is to remove the zephyr, prefix from the compatible string. I am not even sure why it had the zephyr, prefix because it's not a zephyr specific binding, that prefix is only for things where we break the "rule" of having no project specific software description in DT. Not sure why the PR became stalled for half a year over this.

@cfriedt cfriedt reopened this May 5, 2025
@github-actions github-actions bot requested a review from rruuaanng May 5, 2025 18:18
@cfriedt
Copy link
Member

cfriedt commented May 5, 2025

I agree about maintaining binding compatibility.

PR's grow stale for lots of reasons, but a common one is that contributors are doing the work in spare time, which.. has some costs.

@xingrz - would you agree to allowing a maintainer to update this PR on your behalf, or perhaps fork it and add the needed changes?

@github-actions github-actions bot removed the Stale label May 6, 2025
@xingrz
Copy link
Member Author

xingrz commented May 6, 2025

@cfriedt @decsny

Sorry for the long silence and thanks a lot for your patience. I’ll address the immediate issue first — I agree with removing the zephyr, prefix to align with the Linux binding, and I’ll make that change shortly to get things moving.

As for the extra digit control logic in my implementation (where additional GPIOs and timer-based refreshing are used to select which digit is active), I think it would make sense to make that optional — so if digit-gpios and refresh-period-ms properties are omitted from DTS, the driver will simply drive segment outputs directly without any refresh logic. This could be considered as a future improvement to make the driver closer to the Linux model while still supporting advanced setups.

@xingrz xingrz force-pushed the auxdisplay-7seg/pr branch from 92a721d to 7bbcbe1 Compare May 6, 2025 16:02
xingrz added 2 commits May 7, 2025 00:04
This commit introduces a new driver for a common GPIO-driven 7-segment
display. supporting both common anode and common cathode configurations.

Signed-off-by: Chen Xingyu <[email protected]>
This commit introduces a new sample for the Auxiliary display driver.

The sample demonstrates counting from 0 to 1000, with an interval of 100ms
between each increment.

This sample primarily serves to demonstrate the capabilities of segment
displays.

Signed-off-by: Chen Xingyu <[email protected]>
@xingrz xingrz dismissed stale reviews from cfriedt, thedjnK, and kartben via 4cc0e82 May 6, 2025 16:08
@xingrz xingrz force-pushed the auxdisplay-7seg/pr branch from 7bbcbe1 to 4cc0e82 Compare May 6, 2025 16:08
@xingrz xingrz requested a review from decsny May 6, 2025 16:09
@kartben kartben requested review from nordicjm and thedjnK May 8, 2025 05:52
@nordicjm nordicjm removed their request for review May 9, 2025 11:09
@kartben kartben merged commit dc4e259 into zephyrproject-rtos:main May 9, 2025
24 checks passed
@xingrz xingrz deleted the auxdisplay-7seg/pr branch November 24, 2025 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants