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

plmorange · 2024-11-22T07:53:13Z

Add to force push cause the fixup commit message was more than 50 char.

drivers/digit7seg: add return value doc in header and remove unused params in digit7seg_t drivers/digit7seg: add return value doc in header drivers/digit7seg: add return value doc in header

drivers/digit7seg/digit7seg.c

plmorange · 2024-12-11T09:52:05Z

The PR is ready for review! :D

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?

mguetschow · 2024-12-16T10:11:41Z

drivers/include/digit7seg.h

+ * @param[in] value     the value as an uint8_t, a is equal to the first bit
+ *                       b the second.... dp the 8th bit.


As a user, I would expect the driver to do the number to 7-segment-bit conversion for me.

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

mguetschow · 2024-12-16T10:23:58Z

drivers/digit7seg/digit7seg.c

+
+static void _shift_display(void *arg, int chan)
+{
+    (void)chan;


why having the variable in this private function if it is unused?

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.

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_.

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

drivers/digit7seg/doc.txt

+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
+can be controlled into [RIOT-OS](https://github.com/RIOT-OS/RIOT).
+


I think it is worth mentioning one free HW timer as a prerequisites for using this driver here.

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

drivers/digit7seg: change _set_pin_value function

37ceefb

drivers/digit7seg: add return value doc in header and remove unused params in digit7seg_t drivers/digit7seg: add return value doc in header drivers/digit7seg: add return value doc in header

plmorange force-pushed the driver-sha5461AS branch from 50d790a to 37ceefb Compare November 22, 2024 08:00

benpicco reviewed Nov 22, 2024

View reviewed changes

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

benpicco reviewed Nov 22, 2024

View reviewed changes

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

plmorange added 2 commits November 25, 2024 11:13

drivers/digit7seg: add asset in init

b42ef80

drivers/digit7seg: Add assert on gpio

d548653

plmorange requested review from benpicco and Enoch247 December 2, 2024 14:34

plmorange added 2 commits December 2, 2024 16:26

drivers/digit7seg: sort import

1d1da21

drivers/digit7seg: Remove whitespace

c2d1329

RIOT-OS deleted a comment from Outman9900 Dec 16, 2024

mguetschow reviewed Dec 16, 2024

View reviewed changes

RIOT-OS deleted a comment from Outman9900 Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

plmorange commented Nov 13, 2024

plmorange commented Nov 13, 2024

Enoch247 commented Nov 13, 2024

Enoch247 left a comment

plmorange commented Nov 14, 2024

Enoch247 commented Nov 20, 2024

Enoch247 commented Nov 20, 2024

Enoch247 commented Nov 20, 2024

plmorange commented Nov 22, 2024 •

edited

Loading

plmorange commented Dec 11, 2024

Enoch247 commented Dec 12, 2024

Enoch247 commented Dec 16, 2024

plmorange commented Dec 16, 2024

mguetschow left a comment

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

mguetschow Dec 16, 2024

	* @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

		* @param[in] value the value as an uint8_t, a is equal to the first bit
		* b the second.... dp the 8th bit.

	* @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 /

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

Conversation

plmorange commented Nov 13, 2024

Contribution description

Testing procedure

Issues/PRs references

plmorange commented Nov 13, 2024

Enoch247 commented Nov 13, 2024

Enoch247 left a comment

Choose a reason for hiding this comment

plmorange commented Nov 14, 2024

Enoch247 commented Nov 20, 2024

Enoch247 commented Nov 20, 2024

Enoch247 commented Nov 20, 2024

plmorange commented Nov 22, 2024 • edited Loading

plmorange commented Dec 11, 2024

Enoch247 commented Dec 12, 2024

Enoch247 commented Dec 16, 2024

plmorange commented Dec 16, 2024

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

plmorange commented Nov 22, 2024 •

edited

Loading