TM1637 7-segment display driver #21112

KSKNico · 2024-12-27T17:47:22Z

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

maribu

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

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 · 2025-01-02T14:23:48Z

drivers/include/tm1637.h

+ * @brief Brightness level enum for the display
+ *
+ * @note The brightness is defined as a fraction of
+ * the pulse width and must be on of the given values.


Suggested change

* the pulse width and must be on of the given values.

* the pulse width and must be one of the given values.

also, I don't fully understand the sentence here. The brightness being a fraction of the pulse width is an implementation detail not really interesting to the user, right?

mguetschow · 2025-01-02T14:25:56Z

drivers/include/tm1637.h

+ *
+ * @note The integer can't be bigger than 9999 or smaller than
+ * -999 as only 4 digits can be displayed at a time.
+ * With the leading zeros enabled, the display is padded with zeros.


Suggested change

* With the leading zeros enabled, the display is padded with zeros.

* When leading zeros are enabled, the display is padded with zeros.

mguetschow · 2025-01-02T14:29:04Z

drivers/include/tm1637.h

+ * @param[out] dev device descriptor of the display
+ * @param[in] params configuration parameters
+ * @return 0 on success, error otherwise


would be slightly easier to read if the params are aligned, as in

Suggested change

* @param[out] dev device descriptor of the display

* @param[in] params configuration parameters

* @return 0 on success, error otherwise

* @param[out] dev device descriptor of the display

* @param[in] params configuration parameters

* @return 0 on success, error otherwise

It is apparently not explicitly mandated by the coding convention, but that's how I've seen it everywhere across RIOT until now.

same for the rest of this file, obviously :)

mguetschow · 2025-01-02T14:29:49Z

drivers/include/tm1637.h

+ * and the number.
+ *
+ * @param[in] dev device descriptor of the display
+ * @param[in] number number to write, in the range of 9999 to -999


Suggested change

* @param[in] number number to write, in the range of 9999 to -999

* @param[in] number number to write, in the range of -999 to 9999

feels a bit more natural to me

mguetschow · 2025-01-02T14:32:42Z

drivers/tm1637/Makefile

+USEMODULE += ztimer
+USEMODULE += ztimer_msec
+USEMODULE += periph_gpio
+


Suggested change

USEMODULE += ztimer

USEMODULE += ztimer_msec

USEMODULE += periph_gpio

those should go (and mostly are already) in Makefile.dep. Specifying periph_gpio in FEATURES_REQUIRED should also be enough, I think.

mguetschow · 2025-01-02T14:43:47Z

drivers/tm1637/tm1637.c

+ *
+ * @param[in,out] segments  segments to enable the colon on
+ */
+static void enable_colon(uint8_t *segments)


Suggested change

static void enable_colon(uint8_t *segments)

inline static void enable_colon(uint8_t *segments)

mguetschow · 2025-01-02T14:45:56Z

drivers/tm1637/tm1637.c

+void tm1637_clear(const tm1637_t *dev)
+{
+    uint8_t segments[DIGIT_COUNT] = { 0, 0, 0, 0 };
+    transmit_segments(dev, segments, TM1637_PW_1_16);


why this specific brightness?

A brightness needs to be set for the transmit segments function. The actual brightness does not matter as the display is empty anyway.

mguetschow · 2025-01-02T14:48:13Z

drivers/tm1637/tm1637.c

+    assert(number <= 9999);
+    assert(number >= -999);
+
+    uint8_t segments[DIGIT_COUNT] = { 0, 0, 0, 0 };


Suggested change

uint8_t segments[DIGIT_COUNT] = { 0, 0, 0, 0 };

uint8_t segments[DIGIT_COUNT] = { 0 };

does the same, and doesn't hard-code DIGIT_COUNT ;-)

mguetschow · 2025-01-02T14:51:04Z

drivers/tm1637/tm1637.c

+    if (leading_zeros) {
+        for (int i = 0; i < DIGIT_COUNT; ++i) {
+            if (segments[i] == 0) {
+                segments[i] = segments_array[0];
+            }
+        }
+    }


wouldn't this overwrite the minus sign set above in line 251?

No, it only overwrites if the segment is equal to 0, therefore empty/clear. This means that only digits that aren't turned on become zeros.

mguetschow · 2025-01-02T14:52:03Z

drivers/tm1637/tm1637.c

+            if (number != 0) {
+                segments[DIGIT_COUNT - 1 - i] = segments_array[number % 10];
+                number /= 10;
+            }


Since this is an exact copy of lines 242-245, you could consolidate and reduce the levels of indentation :)

The lines 242-245 are part of an if-condition that can't easily be separated into a different function unless I move the entire branch into functions, i. e. one for negative and one for positive numbers.

riot-ci · 2025-01-03T09:42:47Z

Murdock results

✔️ PASSED

068c0e1 integrated most of the PR suggestions

Success	Failures	Total	Runtime
10289	0	10289	19m:17s

Artifacts

Documentation preview

Enoch247

I mostly reviewed the API only so far. I had a few suggestions that I noted. Thank you for the great contribution!

Enoch247 · 2025-01-16T02:39:01Z

drivers/include/tm1637.h

+     * @brief Configuration parameters
+     *
+     */
+    tm1637_params_t params;


The driver should hold a pointer to tm1637_params_t. Not a copy.

Enoch247 · 2025-01-16T02:41:29Z

drivers/include/tm1637.h

+    TM1637_PW_12_16 = 0x05,
+    TM1637_PW_13_16 = 0x06,
+    TM1637_PW_14_16 = 0x07
+} tm1637_brightness_t;


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?

Enoch247 · 2025-01-16T02:43:19Z

drivers/include/tm1637.h

+ *
+ * @param[out]  dev     device descriptor of the display
+ * @param[in]   params  configuration parameters
+ * @return 0 on success, error otherwise


Suggested change

* @return 0 on success, error otherwise

* @retval 0 on success

* @retval -1 on error

Even better than -1 would be an errno value if it makes sense to do so.

Enoch247 · 2025-01-16T02:53:53Z

drivers/tm1637/include/tm1637_params.h

+/**
+ * @brief see @ref tm1637_params_t
+ */
+#define TM1637_PARAM_CLK GPIO_PIN(0, 0)


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.

KSKNico requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners December 27, 2024 17:47

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels Dec 27, 2024

KSKNico force-pushed the tm1637 branch from 085465d to 6cba3c7 Compare December 27, 2024 19:37

tm1637 display driver

6cba3c7

mguetschow requested review from maribu, Enoch247 and mguetschow January 2, 2025 09:26

maribu reviewed Jan 2, 2025

View reviewed changes

mguetschow requested changes Jan 2, 2025

View reviewed changes

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 3, 2025

KSKNico added 2 commits January 7, 2025 17:26

removed modules that are already dependencies

72455c5

integrated most of the PR suggestions

068c0e1

Enoch247 reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TM1637 7-segment display driver #21112

TM1637 7-segment display driver #21112

KSKNico commented Dec 27, 2024

maribu left a comment

mguetschow left a comment

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

KSKNico Jan 14, 2025

mguetschow Jan 2, 2025

mguetschow Jan 2, 2025

KSKNico Jan 14, 2025

mguetschow Jan 2, 2025

KSKNico Jan 14, 2025

riot-ci commented Jan 3, 2025 •

edited

Loading

Enoch247 left a comment

Enoch247 Jan 16, 2025

Enoch247 Jan 16, 2025

Enoch247 Jan 16, 2025

Enoch247 Jan 16, 2025

	* the pulse width and must be on of the given values.
	* the pulse width and must be one of the given values.

	* With the leading zeros enabled, the display is padded with zeros.
	* When leading zeros are enabled, the display is padded with zeros.

	* @param[in] number number to write, in the range of 9999 to -999
	* @param[in] number number to write, in the range of -999 to 9999

	USEMODULE += ztimer
	USEMODULE += ztimer_msec
	USEMODULE += periph_gpio

	static void enable_colon(uint8_t *segments)
	inline static void enable_colon(uint8_t *segments)

	uint8_t segments[DIGIT_COUNT] = { 0, 0, 0, 0 };
	uint8_t segments[DIGIT_COUNT] = { 0 };

	* @return 0 on success, error otherwise
	* @retval 0 on success
	* @retval -1 on error

TM1637 7-segment display driver #21112

Are you sure you want to change the base?

TM1637 7-segment display driver #21112

Conversation

KSKNico commented Dec 27, 2024

Contribution description

Testing procedure

Issues/PRs references

Images

maribu left a comment

Choose a reason for hiding this comment

mguetschow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Jan 3, 2025 • edited Loading

Murdock results

Artifacts

Enoch247 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Jan 3, 2025 •

edited

Loading