Extend LED API #26101

simonguinot · 2020-06-10T14:26:37Z

The current LED API suffers from the following limitations:

The support for multicolor / multi-channel LEDs is very limited. The API provides a single function to that end: led_set_brightness(). This function only allows to update a single LED's channel. It means that led_set_brightness must be called one time per channel to configure a LED (e.g. three times for a RGB LED). This results in too many I/Os and in possible visual glitches as well.
Some LED controllers do have separate registers for color channels and brightness. A single function led_set_brightness() is not enough to expose them all.
The current LED API implements per-channel control. In the case of a mono-color LED a channel is equal to a LED. But in the case of a multicolor LED it is not. As a user I need the ability to control LEDs and not only channels. For example, when I call the led_{off,on} functions I actually expect to turn off/on a LED, and not only one channel of a LED. A LED to channel mapping abstraction should be provided to the user by the API.
The LED API don't provides functions to configure group of LEDs or array/strip of LEDs.

The lack of support for both RGB LEDs and strip of LEDs in the LED API is probably the reason why the LED strip API has been created. But the LED strip API also suffers its own limitations: lack of support for mono-color/single-channel LEDs, "pixel" colors hardcoded as RGB, ...

As an attempt to break the limitations of the LED API and also to close the gap with the LED strip API, this PR:

Introduces a DT binding for LED controllers. This binding provides optional properties for LED child nodes, such as a channel-to-color mapping or an index to map a logical LEDs to their real position on a controller.
Introduces the led_set_color syscall (and its respective callback) to configure multicolor / multi-channel LEDs. This function allows to set all the channels of a LED at once.
Claims that the led_set_color and led_set_brightness functions are dedicated to LED handling. The "led" parameter passed to this function is a logical LED, and no longer a channel.
Adds syscalls dedicated to channel access: led_set_channel and led_write_channels. The latter allows to write any number of contiguous channels. It can be used to update several/all LEDs of a strip. Hopefully this will help to close the gap with the LED strip API. The latter is a shortcut to configure a single LED channel.
Adds a driver (and its test sample) for the TI LP503x LED controller. This driver provides an example about how a color-to-channel mapping can be retrieved from DT for each LED and how it can be used to implement the led_set_{brightness,color} callbacks. The write_channels callback is implemented as well.
Adds a "led" shell command allowing to test the LED API functions from the Zephyr shell. Note that the work on this patch is still in progress.

Any comments and feedback are welcome. Thanks in advance.

Simon

carlescufi · 2020-06-10T14:29:55Z

Thanks for the proposal @simonguinot. Would you mind switching to C99 standard types now that we have deprecated the zephyr-specific types?

simonguinot · 2020-06-12T12:02:06Z

Thanks for the proposal @simonguinot. Would you mind switching to C99 standard types now that we have deprecated the zephyr-specific types?

Done

mbolivar-nordic

Hello @simonguinot and @mbittan, and thanks for your contribution.

Overall I think this seems like quite a nice extension and would like to know what others (@Mani-Sadhasivam , @arvid-r ?) think.

The led_strip API is still a bit orthogonal to this one in my opinion because it:

was designed to allow the caller-allocated 'pixel' buffers to be overwritten in place to transmit the data to the strips
generally works with daisy-chained controllers that don't let you address individual LEDs, but rather require updating the entire strip at once or FIFOing new colors in

So for now I still think there's some value to keeping both around, but having led.h close the gap in features, as you say, seems quite worthwhile.

I have some questions about the particulars and I haven't looked at the sample or shell in detail, but I think they're both nice to have (especially the shell).

dts/bindings/led/led-controller.yaml

include/drivers/led.h

drivers/led/lp503x.c

dts/bindings/led/led-controller.yaml

mbolivar-nordic

All my comments have been addressed. This PR LGTM.

I have one non-blocking question.

samples/drivers/led_lp503x/README.rst

andrewboie

I'm still looking over the system calls but one thing that sticks out to me is that this doesn't have ztest unit tests for the APIs, just a sample. It's not clear how much overall testing coverage we have in this PR's current version, but the system calls aren't being invoked from user mode. The sample doesn't report any success or failure either, it just exits if any API invocations fail without reporting up in a way that can be understood by CI. I just don't think the testing is adequate (yet).

andrewboie · 2020-08-10T20:25:48Z

drivers/led/led_handlers.c

For this output parameter we vastly prefer to make copies in the verification function, rather than pass pointers from user threads directly to the implementation. We make an exception for large variably-sized buffers, but for something like this where we are just filling in a pointer it's better to make a copy.

See more discussion in https://docs.zephyrproject.org/latest/reference/usermode/syscalls.html#verification-memory-access-policies in particular the first example where there is an output parameter.

side note: in this API, we are returning to the user thread a pointer that it will access. In this case, the pointer is always in rodata, so it can read it, we currently allow user threads to read any read-only data. There may come a time when applications are separated from the kernel to a degree where this is not possible and we will need to make a copy, but we can cross that bridge when we get to it, as long as we understand that this API may have to change or an alternative syscall where a buffer is provided from the caller to place the data. No action needs to be taken here now.

I think this underscores the importance of tests which exercise this stuff, as noted earlier we don't have any test coverage of these syscalls in this PR yet. If we ever changed the kernel such that passing kernel rodata pointers to userspace stopped working I'd want a test which catches this. I think this is the first syscall contributed which relies on this behavior.

Hi @andrewboie. Thanks for taking the time to review this PR.

The idea here is to limit the overhead in RAM of struct led_info. That's why a pointer to the data stored in flash is passed. An application may need to fetch and keep this information for all the LEDs. And some boards are embedding a lot of LEDs. On such boards, the cost in RAM in no longer negligible. In order to keep the led_get_info() syscall attractive, I think it must be cheap in RAM. In addition, as you noticed the pointer passed is in rodata and then a user won't be able to modify the data used by the kernel. This kind of mitigates TOCTOU exploits.

Please let me know if you still think it is better to copy the data in this syscall.

I think this is OK as this will never be mutable data by the user thread.

andrewboie · 2020-08-10T20:45:29Z

Other than what I noted about the syscalls and the testing coverage, this otherwise looks very good to me.

carlescufi

API Meeting 11th August 2020

Looks good in general, but the driver/sample will not build in CI unless you add an instance of the ti,lp503x compatible either in an existing board .dts file, by adding a new board/shield that contains it, or just by adding a Devicetree .overlay file with an instance of the LED controller in it.

This filter here will never match as the PR stands today, which means we will not know if this breaks:

  sample.drivers.led.lp503x:
    filter: dt_compat_enabled("ti,lp503x")

carlescufi · 2020-08-11T16:17:44Z

@Mani-Sadhasivam could you please take another look?

galak

Need to see example DTS usage to better understand the updated bindings and if we also need some #defines for things like the color map.

This patch adds a DTS binding for the Texas Instruments LP5030 and LP5036 I2C LED controllers. They are respectively providing up to 30 and 36 channels (i.e. 10 or 12 RGB LEDs). Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

simonguinot · 2020-08-27T15:08:32Z

I'm still looking over the system calls but one thing that sticks out to me is that this doesn't have ztest unit tests for the APIs, just a sample. It's not clear how much overall testing coverage we have in this PR's current version, but the system calls aren't being invoked from user mode. The sample doesn't report any success or failure either, it just exits if any API invocations fail without reporting up in a way that can be understood by CI. I just don't think the testing is adequate (yet).

Hi Andrew. I have added to this PR some tests for the LED API. Please have a look at this patch. I also enhanced the sample of the lp503x driver as well. Let me know if it is good enough for you.

mbolivar-nordic · 2020-08-27T15:11:00Z

Dev-review: 2.4 milestone added, reviewers please revisit

carlescufi

The API meeting concerns have been fixed, this driver is now being tested with the new overlay file.

See #26101 (comment)

andrewboie

we're very close, appreciate the updated tests.

but there's some essential permission management of global data and the led device itself which would prevent this from working if run on a board that supports user mode. comments inline.

do you have HW to test on? frdm_k64f and sam_e70_xplained are the ones I generally use, don't know offhand if they implement LED support. any ARM board with an MPU should do.

tests/drivers/led/led_api/src/test_led_api.c

tests/drivers/led/led_api/src/main.c

simonguinot · 2020-08-27T16:14:42Z

we're very close, appreciate the updated tests.

but there's some essential permission management of global data and the led device itself which would prevent this from working if run on a board that supports user mode. comments inline.

do you have HW to test on? frdm_k64f and sam_e70_xplained are the ones I generally use, don't know offhand if they implement LED support. any ARM board with an MPU should do.

Yes this tests are running on the FaZe board (PR #27778) but there is no MPU on it...

Mani-Sadhasivam

Minor nits...

drivers/led/lp503x.c

samples/drivers/led_lp503x/src/main.c

andrewboie · 2020-08-27T16:51:27Z

Yes this tests are running on the FaZe board (PR #27778) but there is no MPU on it...

@simonguinot I chatted with @carlescufi and I think if you make the changes noted today, I'm willing to +1 this. The overall user mode integration looks good to me, I would expect that if we did encounter issues they would be oneliner fixes and not some architectural refactoring, and we'll just cross that bridge if we get there.

simonguinot · 2020-08-27T17:00:38Z

Yes this tests are running on the FaZe board (PR #27778) but there is no MPU on it...

@simonguinot I chatted with @carlescufi and I think if you make the changes noted today, I'm willing to +1 this. The overall user mode integration looks good to me, I would expect that if we did encounter issues they would be oneliner fixes and not some architectural refactoring, and we'll just cross that bridge if we get there.

Thank you @andrewboie for the guidance with the tests and the userspace thing.

simonguinot · 2020-08-27T17:18:22Z

Minor nits...

Hi @Mani-Sadhasivam. Thanks for your review. I made all the changes your requested.

Mani-Sadhasivam

LGTM, thanks!

drivers/led/Kconfig.lp503x

This patch adds support for the Texas Instruments LP5030 and LP5036 I2C LED controllers. They are respectively providing up to 30 and 36 channels (i.e. 10 or 12 RGB LEDs). In addition to the channel/color registers this LED controller provides a per-LED brigthness register. This driver implements both LED-based and channel-based API methods: - led_on - led_off - led_get_info - led_set_brightness - led_set_color - led_set_channel - led_write_channels Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

This sample controls up to 12 LEDs connected to a LP503x driver. First, for each LED information is retrieved using the led_get_info syscall and printed in the log messages. Next, from an infinite loop, a test pattern (described below) is applied to all the LEDs simultaneously (using the led_write_channels syscall) and then to each LED one by one (using the led_set_{brightness,color} syscalls). Test pattern: For each color in red green blue white yellow purple cyan orange: - set the color - turn on - turn off - set the brightness gradually to the maximum level - turn off Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

This patch introduces the "led" shell command. This allows to run the LED API functions (and to test the LED drivers) from the Zephyr shell. The following subcommands are supported: - on - off - get_info - set_brightness - set_color - set_channel - write_channels Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

This patch introduces some tests for the LED drivers. The following functions of the LED API are tested: - led_get_info - led_on - led_off - led_set_color - led_set_brightness Note that the led-controller-0 alias must be defined in the DTS of the board used to run this tests. Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

simonguinot requested review from Mani-Sadhasivam, MaureenHelm, carlescufi, galak and nashif as code owners June 10, 2020 14:26

carlescufi requested a review from mbolivar-nordic June 10, 2020 14:29

zephyrbot added area: Devicetree area: API Changes to public APIs area: Samples Samples labels Jun 10, 2020

This comment has been minimized.

Sign in to view

simonguinot force-pushed the led-api branch from 332dfe0 to 789d18d Compare June 12, 2020 10:24

mbolivar-nordic reviewed Jun 18, 2020

View reviewed changes

KAGA164 mentioned this pull request Jun 23, 2020

Add generic gpio leds driver with sample #21578

Closed

github-actions bot added area: Documentation area: Device Tree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jun 29, 2020

galak removed the area: Device Tree label Jul 1, 2020

simonguinot force-pushed the led-api branch 2 times, most recently from 4f15bfa to a3b8f9e Compare July 1, 2020 20:30

simonguinot changed the title ~~RFC: extend LED API~~ Extend LED API Jul 11, 2020

simonguinot mentioned this pull request Jul 30, 2020

drivers: led: add shell support #25946

Closed

mbolivar-nordic approved these changes Aug 3, 2020

View reviewed changes

samples/drivers/led_lp503x/README.rst Outdated Show resolved Hide resolved

andrewboie suggested changes Aug 10, 2020

View reviewed changes

andrewboie reviewed Aug 10, 2020

View reviewed changes

carlescufi requested changes Aug 11, 2020

View reviewed changes

galak previously requested changes Aug 11, 2020

View reviewed changes

dts: add binding for TI LP503x LED controllers

d3a4187

This patch adds a DTS binding for the Texas Instruments LP5030 and LP5036 I2C LED controllers. They are respectively providing up to 30 and 36 channels (i.e. 10 or 12 RGB LEDs). Signed-off-by: Simon Guinot <simon.guinot@seagate.com>

simonguinot force-pushed the led-api branch from 92c58f0 to c437455 Compare August 27, 2020 14:49

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Aug 27, 2020

carlescufi added this to the v2.4.0 milestone Aug 27, 2020

carlescufi approved these changes Aug 27, 2020

View reviewed changes

andrewboie reviewed Aug 27, 2020

View reviewed changes

Mani-Sadhasivam requested changes Aug 27, 2020

View reviewed changes

drivers/led/lp503x.c Outdated Show resolved Hide resolved

drivers/led/lp503x.c Outdated Show resolved Hide resolved

samples/drivers/led_lp503x/src/main.c Outdated Show resolved Hide resolved

samples/drivers/led_lp503x/src/main.c Outdated Show resolved Hide resolved

simonguinot force-pushed the led-api branch from c437455 to d078b33 Compare August 27, 2020 16:55

andrewboie approved these changes Aug 27, 2020

View reviewed changes

simonguinot force-pushed the led-api branch from d078b33 to c51a635 Compare August 27, 2020 17:15

nashif requested a review from Mani-Sadhasivam August 28, 2020 04:03

Mani-Sadhasivam approved these changes Aug 28, 2020

View reviewed changes

Mani-Sadhasivam reviewed Aug 28, 2020

View reviewed changes

drivers/led/Kconfig.lp503x Outdated Show resolved Hide resolved

simonguinot force-pushed the led-api branch from c51a635 to b3af6e6 Compare August 28, 2020 11:56

Mani-Sadhasivam reviewed Aug 28, 2020

View reviewed changes

drivers/led/Kconfig.lp503x Outdated Show resolved Hide resolved

simonguinot added 4 commits August 28, 2020 14:01

simonguinot force-pushed the led-api branch from b3af6e6 to f9f084e Compare August 28, 2020 12:02

carlescufi merged commit 7efe988 into zephyrproject-rtos:master Aug 28, 2020

simonguinot deleted the led-api branch October 2, 2020 17:11

simonguinot mentioned this pull request Feb 8, 2024

led: Add color setting and fading to LED API #8946

Closed

Extend LED API #26101

Extend LED API #26101

Uh oh!

Conversation

simonguinot commented Jun 10, 2020

Uh oh!

carlescufi commented Jun 10, 2020

Uh oh!

This comment has been minimized.

simonguinot commented Jun 12, 2020

Uh oh!

mbolivar-nordic 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!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewboie left a comment

Choose a reason for hiding this comment

Uh oh!

andrewboie Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

simonguinot Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

andrewboie Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

andrewboie commented Aug 10, 2020

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Aug 11, 2020

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

simonguinot commented Aug 27, 2020

Uh oh!

mbolivar-nordic commented Aug 27, 2020

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

andrewboie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonguinot commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mani-Sadhasivam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewboie commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonguinot commented Aug 27, 2020

Uh oh!

simonguinot commented Aug 27, 2020

Uh oh!

Mani-Sadhasivam left a comment

Choose a reason for hiding this comment

simonguinot commented Aug 27, 2020 •

edited

Loading

andrewboie commented Aug 27, 2020 •

edited

Loading