drivers/tm1637: add driver and tests for tm1637 7-segment display#21112
drivers/tm1637: add driver and tests for tm1637 7-segment display#21112mguetschow merged 2 commits intoRIOT-OS:masterfrom
Conversation
N11cc00
requested review from
MichelRottleuthner,
aabadie and
leandrolanzieri
as code owners
github-actions
bot
added
Area: doc
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
thx, this looks quite good to me.
The CI has a few comments (check in the code view here):
- One line is longer than 100 chars, please add a line break
- A few files have no terminating newline. (Btw: What editor are you using? Maybe we can add a config to our repo that your editor will pick up. IMO it is better to automate such tedious style things and concentrate.)
Btw: I think negative numbers and leading zeros will result in the minus sign to be overwritten by a zero, or did I read the code incorrectly?
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Thanks, great code quality! I have mostly nits below.
You've rightfully already mentioned #20981. IIRC, the idea was there to provide a unified driver for different kinds of 7-segment displays. How does yours differ and could it be integrated somehow? Otherwise, we should rename the driver in the other PR back to something more specific.
mguetschow
added
the
CI: ready for build
Enoch247
left a comment
Enoch247
left a comment
There was a problem hiding this comment.
I mostly reviewed the API only so far. I had a few suggestions that I noted. Thank you for the great contribution!
| /** | ||
| * @brief see @ref tm1637_params_t | ||
| */ | ||
| #define TM1637_PARAM_CLK GPIO_PIN(0, 0) |
There was a problem hiding this comment.
I would suggest setting the default to GPIO_UNDEF. Then add an assert to the init function to check that the pins are defined. There is no sane default you could ever define. Better to fail loudly then quietly do the wrong thing.
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
This is almost ready I'd say. If you bring the hardware one of those days I can test locally and confirm its working :)
drivers/tm1637/tm1637.c
Outdated
| /** | ||
| * @brief Delay between bits in milliseconds | ||
| */ | ||
| #define BIT_TIME_MS 1 |
There was a problem hiding this comment.
Are there any maximum times? If not, then I'd say leave as is.
|
LGTM now, thanks for bearing with us and thanks for your contribution! @Enoch247 do you want to have a second look? |
|
Oh, and please squash the commits together with |
|
I don't currently have the time to do a full review, but it looks like all my previous concerns have been addressed. |
N11cc00
changed the title
N11cc00
force-pushed
the
tm1637
branch
3 times, most recently
from
9264efb to
5c78bb7
Compare
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Looks good and tested locally to work as expected. Please squash and rename the commits to follow the convention.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
mguetschow
added
CI: full build
|
Please add a (your test doesn't fit there as the CI reports) |
mguetschow
added
CI: ready for build
5 participants
Contribution description
This is a 4 digit display driver for the TM1637 7-segment display. It can display base 10 integers on a range from 9999 to -999, with or without leading zeros and variable brightness. The display can be cleared, too.
The driver was implemented according to a subset of all functionality and adheres to the specification .
Testing procedure
A test can be found in tests/drivers/tm1637/. The test covers all possible configurations for the driver and contains default pin configuration in tm1637_params.h.
Issues/PRs references
A similar display driver is still in review at PR #20981, but isn't compatible with this 4 pin display.
Images