drivers/digit7seg: add 4-digit 7-segment display driver #20981

plmorange · 2024-11-13T10:36:39Z

Contribution description

Added riot/drivers/sha5461as to control a 4-digit 7-segment display (plus decimal point).

Testing procedure

A test is available in tests/drivers/sha5461as, designed for the Arduino Nano 33 BLE. The test will display "8" segment by segment on each digit, followed by "RIOT," and then it will stop.

Issues/PRs references

plmorange · 2024-11-13T10:52:46Z

Hi @Enoch247, I’ve finished my PR! I'm sure it can be improved, especially the sha5461as_display function. Could you please review it when you have a moment?

drivers/sha5461as/sha5461as.c

Enoch247 · 2024-11-13T18:20:41Z

Thanks for the contribution. I'll try to review this tonight.

Enoch247

I think the level of abstraction provided by the driver is too low. I expected to be able to pass it a number or array of digits to display. Currently the driver requires applications to manually set a bitfield with each bit corresponding to the display's segments. The application must also know the mapping of the bits to segments.

drivers/include/sha5461as.h

tests/drivers/sha5461as/main.c

drivers/include/sha5461as.h

plmorange · 2024-11-14T13:30:39Z

I think the level of abstraction provided by the driver is too low. I expected to be able to pass it a number or array of digits to display. Currently the driver requires applications to manually set a bitfield with each bit corresponding to the display's segments. The application must also know the mapping of the bits to segments.

Thank you for reviewing my PR.
Is it a solution to leave the low level abstraction as it is now and create another high-level module?

Enoch247 · 2024-11-20T18:11:30Z

I think the level of abstraction provided by the driver is too low. I expected to be able to pass it a number or array of digits to display. Currently the driver requires applications to manually set a bitfield with each bit corresponding to the display's segments. The application must also know the mapping of the bits to segments.

Is it a solution to leave the low level abstraction as it is now and create another high-level module?

It is not my preference, but if you have a reason to do it this way, can you please describe it so that I better understand.

Enoch247 · 2024-11-20T18:15:17Z

BTW, in the future, please hold off to squash/force push until requested once a PR is submitted. It makes the process a bit easier. See here for some helpful tips.

Enoch247 · 2024-11-20T18:22:50Z

Great work adding the documentation for the driver! Its not always the fun part of writing code, but it will really help users of your driver in the future.

drivers/include/digit7seg.h

Enoch247 · 2024-12-12T03:12:17Z

Thanks. Sorry for the delay. I will set aside some time this weekend to look it over, if not sooner.

Enoch247 · 2024-12-16T03:07:28Z

As state before, I think that there needs to be a way for applications to send numbers to the display. As far as I see, the driver's API allows to manually set the display's segments, but that is the exception rather than the normal way an application would want to interact with a numeric display driver. Something like:

int digit7seg_set(digit7seg_t *dev, int val, int scale);
int digit7seg_set_digit(digit7seg_t *dev, unsigned digit, unsigned val);

val and scale could be similar to that used by phaydat_t

plmorange · 2024-12-16T08:27:05Z

As state before, I think that there needs to be a way for applications to send numbers to the display. As far as I see, the driver's API allows to manually set the display's segments, but that is the exception rather than the normal way an application would want to interact with a numeric display driver. Something like:
int digit7seg_set(digit7seg_t *dev, int val, int scale);
int digit7seg_set_digit(digit7seg_t *dev, unsigned digit, unsigned val);
val and scale could be similar to that used by phaydat_t

OK, I'll do it as soon as I can!

mguetschow

Thanks for your work on this! Some first comments from my side below.

mguetschow · 2024-12-16T10:07:52Z

drivers/digit7seg/digit7seg.c

+/**
+ * @ingroup     drivers_digit7seg
+ * @file
+ * @brief       Device driver for less than 5 digits of 7 segments without IC


Suggested change

* @brief Device driver for less than 5 digits of 7 segments without IC

* @brief Device driver for up to 4 digits of 7 segments without IC

do we really want such restriction?

Yes, most 7-digit segments are composed of 4 digits, which is easier to manage.

drivers/include/digit7seg.h

mguetschow · 2024-12-16T10:12:01Z

drivers/include/digit7seg.h

+ *                       b the second.... dp the 8th bit.
+ *
+ * @return              0 on success
+ * @return              -1 when an incorrect digit is passed


Suggested change

* @return -1 when an incorrect digit is passed

* @return -1 when an invalid index is passed

?

mguetschow · 2024-12-16T10:13:42Z

drivers/digit7seg/doc.txt

+
+## About
+
+This driver was developed to works with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)


Suggested change

This driver was developed to works with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)

This driver was developed to work with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)

mguetschow · 2024-12-16T10:14:05Z

drivers/digit7seg/doc.txt

+## About
+
+This driver was developed to works with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)
+in a way that all 4 digits (and less) with 7 segments without integrated controller


Suggested change

in a way that all 4 digits (and less) with 7 segments without integrated controller

in a way that up to 4 digits with 7 segments without integrated controller

drivers/digit7seg/digit7seg.c

mguetschow · 2024-12-16T10:26:17Z

drivers/digit7seg/digit7seg.c

+    uint32_t temp_value = value << (index * BYTE_BITS);
+    uint32_t up_value = dev->value >> ((index + 1) * BYTE_BITS);
+    up_value <<= ((index + 1) * BYTE_BITS);
+    uint32_t down_value = ((0b00000001 << (index * BYTE_BITS)) - 1) & dev->value;


maybe it's worth adding some explaining comments to what this code does.

You are right

mguetschow · 2024-12-16T10:28:54Z

drivers/include/digit7seg.h

+
+#define DIGIT7SEG_MAX_DIGITS  (4)     /**< Max pin number, can be below or equal */
+#ifndef DIGIT7SEG_TIMER_HZ
+#   define DIGIT7SEG_TIMER_HZ    MHZ(1)  /**< default timer hz */


Suggested change

# define DIGIT7SEG_TIMER_HZ MHZ(1) /**< default timer hz */

# define DIGIT7SEG_TIMER_HZ MHZ(1) /**< default timer hz */

as per the Coding Convention. please also check other places

mguetschow · 2024-12-16T10:30:12Z

drivers/include/digit7seg.h

+#ifndef DIGIT7SEG_TIMER_HZ
+#   define DIGIT7SEG_TIMER_HZ    MHZ(1)  /**< default timer hz */
+#endif
+#ifndef DIGIT7SEG_DELAY


I think it would be good to prefix all those "over-writeable" parameters with CONFIG_.

You raise a good point, but as all drivers do it this way, is it worth doing it any other way?

Older code my do otherwise, but my understanding is that new code should prefix with CONFIG_ if it is a value that is allowed to be changed/overridden and not just a constant.

@Enoch247 is correct. The CONFIG_ prefix allows for easier greping and allows to expose configuration options via KConfig.

Code not following this convention is due to the code base being older than this convention. And also: Reviewers will not always spot any issue or deviation from the coding convention

drivers/digit7seg/doc.txt

plmorange · 2025-02-10T12:41:50Z

Hello @mguetschow I hope you're well!
The PR is ready for another review when you have some time.
Thank you for the previous comments.

plmorange · 2025-02-10T14:01:50Z

I think the level of abstraction provided by the driver is too low. I expected to be able to pass it a number or array of digits to display. Currently the driver requires applications to manually set a bitfield with each bit corresponding to the display's segments. The application must also know the mapping of the bits to segments.

@Enoch247, 2 months later, it's done! :)

plmorange · 2025-03-27T12:39:33Z

@mguetschow @Enoch247 I've made all the requested changes, what do you think?

mguetschow · 2025-03-27T13:12:06Z

I'm sorry I haven't found the time to take another look at it yet (it's on my todo), but one high-level remark: After another 7-segment display is now in with #21112, this driver should be renamed back to refer to the models it works with. In a separate step, a more generic 7-segment-driver could be introduced as a higher-level API to both #21112 and this one.

Enoch247 · 2025-03-29T12:09:57Z

I'm sorry I haven't found the time to take another look at it yet (it's on my todo), but one high-level remark: After another 7-segment display is now in with #21112, this driver should be renamed back to refer to the models it works with. In a separate step, a more generic 7-segment-driver could be introduced as a higher-level API to both #21112 and this one.

The problem with renaming is that this driver can be used with any generic 7 segment display. So it doesn't make since to rename it after any hardware module. If you wanted to reserve the generic name for a top level API, I supose you cold prefix this one with soft_ just like the soft_uart and soft_spi modules?

I will try review the PR this coming week.

plmorange · 2025-04-14T07:00:42Z

I'm sorry I haven't found the time to take another look at it yet (it's on my todo), but one high-level remark: After another 7-segment display is now in with #21112, this driver should be renamed back to refer to the models it works with. In a separate step, a more generic 7-segment-driver could be introduced as a higher-level API to both #21112 and this one.

The problem with renaming is that this driver can be used with any generic 7 segment display. So it doesn't make since to rename it after any hardware module. If you wanted to reserve the generic name for a top level API, I supose you cold prefix this one with soft_ just like the soft_uart and soft_spi modules?

I will try review the PR this coming week.

Thank you, I look forward to hearing your comments on the PR!

plmorange · 2025-06-23T07:20:47Z

Hi @mguetschow @Enoch247, It seems the PR has been blocked for a long time, are there any problems?

riot-ci · 2025-06-25T12:52:02Z

Murdock results

✔️ PASSED

8a00609 Merge branch 'RIOT-OS:master' into driver-sha5461AS

Success	Failures	Total	Runtime
10390	0	10390	15m:28s

Artifacts

Documentation preview

maribu

Looks good to me, thx.

I resolved all comments that have been addressed and added some of my own. Sorry that the review process as been a bit slow here; all maintainers are volunteers and sometimes life get's in the way.

Feel free to ping me again when this is ready. You may squash at will to safe one round-trip-time before getting this merged.

maribu · 2025-06-28T20:09:20Z

drivers/include/digit7seg.h

+#ifndef DIGIT7SEG_TIMER_HZ
+#   define DIGIT7SEG_TIMER_HZ    MHZ(1)  /**< default timer hz */
+#endif
+#ifndef DIGIT7SEG_DELAY


@Enoch247 is correct. The CONFIG_ prefix allows for easier greping and allows to expose configuration options via KConfig.

Code not following this convention is due to the code base being older than this convention. And also: Reviewers will not always spot any issue or deviation from the coding convention

maribu · 2025-06-28T20:14:00Z

drivers/include/digit7seg.h

+/**
+ * @brief   Display bitfield for numbers between [0, 9]
+ */
+static const uint8_t digit7seg_bitfield[] = {


Should this really be in the header?

Every compilation unit that actually makes use of this array will end up adding its own copy. If it would be added to a C file, we don't get duplicates.

maribu · 2025-06-28T20:17:44Z

drivers/include/digit7seg.h

+ * @param[in] value     the value between [0, 9]
+ *
+ */
+int digit7seg_set_int_value(digit7seg_t *dev, int index, uint8_t value);


Suggested change

int digit7seg_set_int_value(digit7seg_t *dev, int index, uint8_t value);

int digit7seg_set_int_value(digit7seg_t *dev, unsigned index, uint8_t value);

Since negative indices are not allowed per API doc, we could encode this also into thr type.

maribu · 2025-06-28T20:26:13Z

drivers/include/digit7seg.h

+ *                       will be displayed as 234,1 but with
+ *                       precision = 100 it will be 34,15
+ */
+int digit7seg_set_float_value(digit7seg_t *dev, float value, int precision);


We support a lot of MCUs that do not have an FPU. And soft FPU implementations are surprisingly large (in terms of ROM size) and also slowish.

How about providing the value as integer type and a second integer gives the position of the decimal?

E.g. value=1337 and decimal=2 would be printed as 13.37?

mguetschow · 2025-06-30T09:25:00Z

I'm sorry I haven't found the time to take another look at it yet (it's on my todo), but one high-level remark: After another 7-segment display is now in with #21112, this driver should be renamed back to refer to the models it works with. In a separate step, a more generic 7-segment-driver could be introduced as a higher-level API to both #21112 and this one.

The problem with renaming is that this driver can be used with any generic 7 segment display. So it doesn't make since to rename it after any hardware module. If you wanted to reserve the generic name for a top level API, I supose you cold prefix this one with soft_ just like the soft_uart and soft_spi modules?

Do you have examples of other such generic 7 segment displays? I'd just like to confirm that the driver would indeed fit them, too. Otherwise I'd favor your suggestion of prefixing with soft_. Maybe we could also link to the TM1637 7-segment-display driver in the documentation of this driver.

plmorange added 2 commits November 13, 2024 09:59

Create driver and example for SHA5461AS

0a2de60

Refacto files with uncrustify

bc0545d

plmorange requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners November 13, 2024 10:36

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

Correcting spelling mistakes

7a1beca

benpicco reviewed Nov 13, 2024

View reviewed changes

drivers/sha5461as/sha5461as.c Outdated Show resolved Hide resolved

mguetschow changed the title ~~Driver sha5461as~~ drivers/sha5461as: add 4-digit 7-segment display driver Nov 13, 2024

plmorange marked this pull request as draft November 13, 2024 13:51

Enoch247 self-assigned this Nov 13, 2024

Enoch247 requested changes Nov 13, 2024

View reviewed changes

plmorange changed the title ~~drivers/sha5461as: add 4-digit 7-segment display driver~~ drivers/digit7seg: add 4-digit 7-segment display driver Nov 14, 2024

plmorange force-pushed the driver-sha5461AS branch from 597003f to 9050c76 Compare November 14, 2024 13:06

drivers/digit7seg: add 4-digit 7-segment display driver

005250f

plmorange force-pushed the driver-sha5461AS branch from 9050c76 to 005250f Compare November 14, 2024 14:24

plmorange marked this pull request as ready for review November 14, 2024 14:24

plmorange requested review from Enoch247 and benpicco November 14, 2024 14:25

drivers/digit7seg: initialize all var of digit7seg_t in digit7seg_init

4d541ba

Enoch247 requested changes Nov 22, 2024

View reviewed changes

plmorange force-pushed the driver-sha5461AS branch from 61d694f to 50d790a Compare November 22, 2024 07:52

RIOT-OS deleted a comment Dec 16, 2024

mguetschow reviewed Dec 16, 2024

View reviewed changes

RIOT-OS deleted a comment Dec 16, 2024

N11cc00 mentioned this pull request Dec 27, 2024

drivers/tm1637: add driver and tests for tm1637 7-segment display #21112

Merged

plmorange added 6 commits February 10, 2025 10:32

drivers/digit7seg: Fix documentation and add commentary in code

bbe3ef4

drivers/digit7seg: fix compilation errors

e48e5aa

drivers/digit7seg: Add float support

b865a56

Merge branch 'driver-digit7seg-abstraction' into driver-sha5461AS

f49b847

drivers/digit7seg: Add Int and Float support

4c79bca

drivers/digit7seg: Correct errors in documentation

b47622f

Merge branch 'RIOT-OS:master' into driver-sha5461AS

8a00609

plmorange requested a review from mguetschow April 22, 2025 12:37

crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 25, 2025

maribu reviewed Jun 28, 2025

View reviewed changes

	* @brief Device driver for less than 5 digits of 7 segments without IC
	* @brief Device driver for up to 4 digits of 7 segments without IC

	* @return -1 when an incorrect digit is passed
	* @return -1 when an invalid index is passed


		## About

		This driver was developed to works with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)

	This driver was developed to works with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)
	This driver was developed to work with [5461AS](http://www.topliteusa.com/uploadfile/2014/0825/A-5461AS.pdf)

	in a way that all 4 digits (and less) with 7 segments without integrated controller
	in a way that up to 4 digits with 7 segments without integrated controller

	# define DIGIT7SEG_TIMER_HZ MHZ(1) /*< default timer hz /
	# define DIGIT7SEG_TIMER_HZ MHZ(1) /*< default timer hz /

	int digit7seg_set_int_value(digit7seg_t *dev, int index, uint8_t value);
	int digit7seg_set_int_value(digit7seg_t *dev, unsigned index, uint8_t value);

drivers/digit7seg: add 4-digit 7-segment display driver #20981

Are you sure you want to change the base?

drivers/digit7seg: add 4-digit 7-segment display driver #20981

Uh oh!

Conversation

plmorange commented Nov 13, 2024

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

plmorange commented Nov 13, 2024

Uh oh!

Uh oh!

Enoch247 commented Nov 13, 2024

Uh oh!

Enoch247 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plmorange commented Nov 14, 2024

Uh oh!

Enoch247 commented Nov 20, 2024

Uh oh!

Enoch247 commented Nov 20, 2024

Uh oh!

Enoch247 commented Nov 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enoch247 commented Dec 12, 2024

Uh oh!

Enoch247 commented Dec 16, 2024

Uh oh!

plmorange commented Dec 16, 2024

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plmorange commented Feb 10, 2025