Peripheral: LED: Add pulsing LED API

[MetaGlennA]

This is a proposal to modify the Zephyr LED interface for multi-channel ‘breathing’ LED drivers.

A breathing LED driver may have the following parameters per channel:

1. Delay before first pulse begins
2. Rise time from off to on
3. On-time
4. Fall time from on to off
5. Off-time until the start of the next rise

The current LED API only has a square-wave On-Time and Off-Time for the led_blink() function.

The led_blink() function also does not support synchronizing the start of blinking on a multi-channel device.

I propose an led_pulse() function to handle multi-channel breathing LEDs.

The function signature would be:

int led_pulse(const struct device *dev, struct led_pulse_parameter *parameters)

The parameters argument takes a pointer to an array of struct led_pulse_parameter objects with the following members:

struct led_pulse_parameter {
	uint8_t brightness;
	uint32_t start_delay_ms;
	uint32_t rise_time_ms;
	uint32_t on_time_ms;
	uint32_t fall_time_ms;
	uint32_t off_time_ms;
};

The number of items in the array is the same as the number of channels on the device.

If a channel is to be set to always on or always off rather than pulsing this can be accomplished through the led_pulse() command as well:

• If on, set all time values to zero except on_time_ms
• If off, set all time values to zero except off_time_ms

The API already has a brightness command, so having it in the led_pulse_parameter struct is open to debate.

[nordicjm]

@simonguinot

[simonguinot]

Hello @MetaGlennA,

Thanks for starting this discussion.

Do you own a LED controller with a pulse capability you need to take advantage of ?

For me the question is: is the kernel the right place to implement software LED effects ? So far, my opinion is no. I think that sotfware LED effects should go in a user module/library/layer on the top of the kernel API. And the kernel API should be limited to expose the hardware capabilities to userland. So the relevance of a pulse API depends on the availability of a such feature on LED controllers.

About the API proposal itself, it could be interesting to split the brightness parameter into two parameters: brightness_low and brightness_high. This should allow to configure the minimum and maximum level of a breathing/pulse LED effect. Also you may want to be able to set the initial color of a LED (in the case of a multicolor LED).

On my side I am using the following definitions for a breathing/pulse LED effect:

/**
 * @brief Enumeration of the different states during a breath LED effect.
 *
 * @note The order of the states is important. They are applied by the LED
 *       effect function following this order.
 */
enum breath_state {
        /* @brief High brightness level. */
        BREATH_HIGH = 0,
        /* @brief Brightness level is moving from the high to the low state. */
        BREATH_RAMP_DOWN,
        /* @brief Low brightness level. */
        BREATH_LOW,
        /* @brief Brightness level is moving from the low to the high state. */
        BREATH_RAMP_UP,
        /* @brief Number of states during a breath effect. */
        BREATH_MAX_STATE,
};

/**
 * @brief Breath LED effect structure.
 *
 * @param colors           Color array to apply before starting the effect.
 *                         This parameter is only relevant for multicolor LEDs.
 *                         And if omitted for multicolor LEDs, then the breath
 *                         effect will apply to their current colors.
 * @param delay            Array to keep the delay to spend at each state
 *                         (in milliseconds).
 * @param brightness_high  Brightness at high state.
 * @param brightness_low   Brightness at low state.
 * @param state            Current breath state (see enum breath_state).
 */
struct breath {
        uint8_t         colors[LED_MAX_COLORS];
        uint16_t        delay[BREATH_MAX_STATE];
        uint8_t         brightness_high;
        uint8_t         brightness_low;
};

[simonguinot]

Do you own a LED controller with a pulse capability you need to take advantage of ?

Yes, we do. It has the parameters listed built into the hardware. We're currently using a driver-specific API for it but would like to make a suitable generic interface for all future LED drivers we may use.

OK. I think it makes sense.

For me the question is: is the kernel the right place to implement software LED effects ? So far, my opinion is no. I think that sotfware LED effects should go in a user module/library/layer on the top of the kernel API. And the kernel API should be limited to expose the hardware capabilities to userland. So the relevance of a pulse API depends on the availability of a such feature on LED controllers.

We already expose the led_blink() as part of the API, so are expecting hardware control over the LED by the driver beyond on/off/brightness/colour. led_breathe() would just implement a better version for more capable LED controllers.

OK. Here is the next question :) Should we limit the new API method to the breathing/pulse LED effect or should we consider even more capable controllers that can handle more complex effects such as morphing ? Here, we would have ->blink() superseded by ->{pulse,breach}(). Eventually ->{pulse,breath}() will be superseded by ->morph(). So should we add a new method for every hardware LED effect or should we rather design a method able to represent/abstract them all ?

[MetaGlennA]

OK. Here is the next question :) Should we limit the new API method to the breathing/pulse LED effect or should we consider even more capable controllers that can handle more complex effects such as morphing ? Here, we would have ->blink() superseded by ->{pulse,breach}(). Eventually ->{pulse,breath}() will be superseded by ->morph(). So should we add a new method for every hardware LED effect or should we rather design a method able to represent/abstract them all ?

Very good question, and I'm ok with that if we can come up with an API quickly.

What would morph() look like? Something like:

struct led_morph_step {
    uint8_t brightness start;
    uint8_t brightness_end;
    struct led_color color_start;
    struct led_color color_end;
    uint32_t morph_time_ms;
    uint32_t morph_flags;
};

Where flag indicates things like linear/log curve or other features? We'd still also need the 'delay before loop starts' parameter for our use case, and the ability to pass in the morph pattern for each channel in the controller so we can synchronize patterns.

so:

led_morph(const struct device *dev, uint32_t start_delay_ms, int num_channels, int num_steps, struct led_morph_steps **morph_steps)

Perhaps?

[simonguinot]

About the API proposal itself, it could be interesting to split the brightness parameter into two parameters: brightness_low and brightness_high. This should allow to configure the minimum and maximum level of a breathing/pulse LED effect. Also, you may want to be able to set the initial color of a LED (in the case of a multicolor LED).

That makes sense and I welcome the suggestion. We do have led_brightness() and led_color() in the API which could handle those tasks separately to prevent the struct getting too unwieldy, given we need to pass in a copy for each channel.

We need to consider/study more LED controllers with pulse/breath hardware capability when designing the API.

For example a controller could offer different delay configurations for ramping down and up. Having a single delay field in the effect structure would loose this feature. Same goes with the minimum/maximum brightness.

[simonguinot]

OK. Here is the next question :) Should we limit the new API method to the breathing/pulse LED effect or should we consider even more capable controllers that can handle more complex effects such as morphing ? Here, we would have ->blink() superseded by ->{pulse,breach}(). Eventually ->{pulse,breath}() will be superseded by ->morph(). So should we add a new method for every hardware LED effect or should we rather design a method able to represent/abstract them all ?

Very good question, and I'm ok with that if we can come up with an API quickly.

What would morph() look like? Something like:

struct led_morph_step {
    uint8_t brightness start;
    uint8_t brightness_end;
    struct led_color color_start;
    struct led_color color_end;
    uint32_t morph_time_ms;
    uint32_t morph_flags;
};

Where flag indicates things like linear/log curve or other features? We'd still also need the 'delay before loop starts' parameter for our use case, and the ability to pass in the morph pattern for each channel in the controller so we can synchronize patterns.

so:

led_morph(const struct device *dev, uint32_t start_delay_ms, int num_channels, int num_steps, struct led_morph_steps **morph_steps)

Perhaps?

I have been using the following structure for morphing software LED effects:

#define MORPHING_MAX_COLORS 8

/**
 * @brief Morphing LED effect structure.
 *
 * @param color_count  Number of color states.
 * @param high_delay   Delay to stay high when a color state is reached (in
 *                     milliseconds).
 * @param high_delay   Transition delay between color states (in milliseconds).
 * @param brightness   Brightness array (one entry per color state).
 * @param colors       Color array (one entry per color state).
 */
struct morphing {
        uint8_t         color_count;
        uint16_t        high_delay;
        uint16_t        ramp_delay;
        uint8_t         brightness[MORPHING_MAX_COLORS];
        uint8_t         colors[MORPHING_MAX_COLORS][LED_MAX_COLORS];
};

But that was not my point. I was mentioning the morphing LED effect as an example. My question was more: should we add a pulse LED effect to the LED API ? Is that something commonly found on LED controllers ? Or should we go for something more generic / adaptable ?

[MetaGlennA]

I'm good with a generic API that encompasses any use cases you can think of, so long as we can get our start-delayed single-colour breathing LED driven by it.

My only concern is a too-generic interface becomes harder to interact with for the user. The original API had six parameters total. Now we're looking at multiple data items per LED phase. Do we want to implement blink/pulse/morph separately to contain the complexity to whichever device or use case you need, or one API to rule them all?

[simonguinot]

@soburi @bbilas @mbolivar @Mani-Sadhasivam @kartben

[simonguinot]

@soburi @bbilas @mbolivar @Mani-Sadhasivam @kartben

Please can you comment guys ? :)

[soburi]

The policy is good.

As mentioned in the discussion, it is important to make it look simple for the user and minimize the implementation on the driver side.
If there is an inclusion relationship about functionality, that is, if blinking can be achieved with breath or morph,

static inline int z_impl_led_blink(const struct device *dev, uint32_t led,
				   uint32_t delay_on, uint32_t delay_off)
{
	const struct led_driver_api *api =
		(const struct led_driver_api *)dev->API;

        if (api->blink) {
                api->blink(...);
        } else if (api->breath) {
                api->breath(blink_parameter);
        }
     ....
}

I think the implementation can be somewhat consolidated in this format.

[MetaGlennA]

I'm good with that.

[MetaGlennA]

Any thoughts on the led_morph() suggestion?

[simonguinot]

@MetaGlennA What LED controller model with pulse/breath capability are you using ?

Among the LED controllers supported by Zephyr I can see the NCP5623 which comes with a "dimming" feature. There is also the IS31FL3194 which provides support for automatic breathing as well. And finally I believe that the LP5562 and LP5569 LED controllers also supports hardware breathing (or any other LED effect actually) via some internal engine programming.

[MetaGlennA]

AW2026.

We'll upstream the driver when we have an API that supports breathing.

[simonguinot]

Oh nice. Note that you can (and should actually) upstream this driver independently from the breathing API.

About the breathing API, I spent some time the last few days to look at the hardware capabilities of the LED controllers available nowadays. And I believe it makes sense to add a simple breathing API method to the LED subsystem. So I am all in favor for that.

[MetaGlennA]

Just to confirm, we can do 'breathing' API now and worry about morphing or other LED effects later.

Since we need the breathing effect for our application, I've already implemented my version of breathing in the LED driver and stripping it out just to upstream it would be a chore. Let's get the API updated, then upstreaming the driver will be trivial.

[simonguinot]

Just to confirm, we can do 'breathing' API now and worry about morphing or other LED effects later.

IMO, yes. I didn't find much controllers supporting hardware morphing effect. On the contrary I found several supporting breathing. That's why I believe that a breathing API method is relevant. There are also LED controllers with programmable engines that can run all kinds of effects. It would be nice to have an API for them too but it is a different story.

Since we need the breathing effect for our application, I've already implemented my version of breathing in the LED driver and stripping it out just to upstream it would be a chore. Let's get the API updated, then upstreaming the driver will be trivial.

In your PR adding the breathing API you'll need to demonstrate it by adding the ->breathing() method to a LED driver.

[MetaGlennA]

Since we need the breathing effect for our application, I've already implemented my version of breathing in the LED driver and stripping it out just to upstream it would be a chore. Let's get the API updated, then upstreaming the driver will be trivial.

In your PR adding the breathing API you'll need to demonstrate it by adding the ->breathing() method to a LED driver.

That's a bit chicken-and-egg. Can we introduce the AW2026 driver as a commit in the same PR as the LED API update?

Or I guess I can write the 'breathing' method and not wire it up to the API until the API is upstreamed.

[simonguinot]

In your PR adding the breathing API you'll need to demonstrate it by adding the ->breathing() method to a LED driver.

That's a bit chicken-and-egg. Can we introduce the AW2026 driver as a commit in the same PR as the LED API update?

Or I guess I can write the 'breathing' method and not wire it up to the API until the API is upstreamed.

For the review I think it is best to have two different PRs: one for the driver and another for the breathing API. Probably you can build the API PR on the top of the driver PR and indicate the dependency. Simply we will have to merge the driver first.