TM1637 7-segment display driver #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.
drivers/include/tm1637.h
Outdated
| * @brief Configuration parameters | ||
| * | ||
| */ | ||
| tm1637_params_t params; |
There was a problem hiding this comment.
The driver should hold a pointer to tm1637_params_t. Not a copy.
There was a problem hiding this comment.
I based my works on the hd44780 driver. Why should my driver hold a pointer while the other keeps a copy of the parameters? Wouldn't the pointer method lead to problems with the lifetime of the parameter struct because it is managed by the user?
There was a problem hiding this comment.
No need to repeat mistakes of the past ;)
Having a copy in RAM results having to spent precious RAM on it in addition to having to spent precious flash on it. With a pointer you only need to pay sizeof(void *) in RAM in addition to the ROM price.
| TM1637_PW_12_16 = 0x05, | ||
| TM1637_PW_13_16 = 0x06, | ||
| TM1637_PW_14_16 = 0x07 | ||
| } tm1637_brightness_t; |
There was a problem hiding this comment.
The meaning of these values is unclear to me. I assume they are fractions of full brightness, but the doc does not say this. If they are fractions, why are some values skipped?
There was a problem hiding this comment.
This should be addressed in the newest commit.
| /** | ||
| * @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
| #define DATA_COMMAND 0x40 | ||
|
|
||
| /** | ||
| * @brief Sets the brightness and display state to on/off | ||
| */ | ||
| #define DISPLAY_AND_CONTROL_COMMAND 0x80 | ||
|
|
||
| /** | ||
| * @brief Sets the address where data is written | ||
| */ | ||
| #define ADDRESS_COMMAND 0xC0 |
There was a problem hiding this comment.
| #define DATA_COMMAND 0x40 | |
| /** | |
| * @brief Sets the brightness and display state to on/off | |
| */ | |
| #define DISPLAY_AND_CONTROL_COMMAND 0x80 | |
| /** | |
| * @brief Sets the address where data is written | |
| */ | |
| #define ADDRESS_COMMAND 0xC0 | |
| #define COMMAND_DATA 0x40 | |
| /** | |
| * @brief Sets the brightness and display state to on/off | |
| */ | |
| #define COMMAND_DISPLAY_AND_CONTROL 0x80 | |
| /** | |
| * @brief Sets the address where data is written | |
| */ | |
| #define COMMAND_ADDRESS 0xC0 |
would keep them logically grouped (similar below for the bit mask). Not a must though.
| /** | ||
| * @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.
drivers/tm1637/tm1637.c
Outdated
| * @param[in] segments array of length 4 encoding the display's segments | ||
| */ | ||
| static void transmit_segments(const tm1637_t *dev, | ||
| const uint8_t *segments, |
There was a problem hiding this comment.
| const uint8_t *segments, | |
| const uint8_t segments[4], |
or even
| const uint8_t *segments, | |
| const uint8_t segments[DIGIT_COUNT], |
drivers/tm1637/tm1637.c
Outdated
| * | ||
| * @param[in] dev device descriptor of the display | ||
| */ | ||
| static void stop(const tm1637_t *dev) |
There was a problem hiding this comment.
| static void stop(const tm1637_t *dev) | |
| static void _stop(const tm1637_t *dev) |
prefixing internal function with underscore is a common convention, I leave it to you to decide whether you want to adopt it or not
tests/drivers/tm1637/Makefile
Outdated
| CFLAGS += -DTM1637_PARAM_CLK=GPIO_PIN\(0\,\ 0\) | ||
| CFLAGS += -DTM1637_PARAM_DIO=GPIO_PIN\(0\,\ 1\) |
There was a problem hiding this comment.
| CFLAGS += -DTM1637_PARAM_CLK=GPIO_PIN\(0\,\ 0\) | |
| CFLAGS += -DTM1637_PARAM_DIO=GPIO_PIN\(0\,\ 1\) | |
| CFLAGS += "-DTM1637_PARAM_CLK=GPIO_PIN(0, 0)" | |
| CFLAGS += "-DTM1637_PARAM_DIO=GPIO_PIN(0, 1)" |
I think ™️ that should also work without the backslash escapes.
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