drivers/tm1637: add driver and tests for tm1637 7-segment display #21112
Conversation
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?
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.
| /** | ||
| * @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.
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. |
9264efb to
5c78bb7
Compare
|
Please add a (your test doesn't fit there as the CI reports) |
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