drivers: led: Provide a fallback implementation of blink API #90237

anobli · 2025-05-20T19:18:56Z

The LED blink API is quite useful but this is not supported by all the LED controller.
This provides a software implementation of blink that use a delayable work to blink a LED.
This could be enabled using Kconfig, as well the maximum number of LED that could blink at the same time using blink fallback. This does not requires any changes to driver to be used.

simonguinot

Hi @anobli,

Thanks for this PR but I can't agree with it in its current state. Please see my comments below.

simonguinot · 2025-05-21T14:52:54Z

drivers/led/blink_fallback.c

+	struct k_work_delayable work;
+	uint32_t delay_on;
+	uint32_t delay_off;
+};


The blink information should be attached to the LED (i.e. in a LED structure). Have a look at struct led_classdev in Linux and at the software blinking implementation. We are aiming at the same architecture here, maybe more optimized for memory usage

Note that you may have to reintroduce a LED class structure here. We are just about to remove the existing one with #90186 :)

I have added a new struct attached to the LED device. I tried to do it in a way that request less changes as possible in driver. I kept the same content in the struct. I think I can probably improve it a little bit by removing the led index and use pointer arithmetic to retrieve it.

simonguinot · 2025-05-21T14:54:07Z

drivers/led/blink_fallback.c

@@ -0,0 +1,130 @@
+/*


I think led-core.c (as in Linux) may be a better name. It may become useful for other LED software features.

simonguinot · 2025-05-21T15:13:26Z

drivers/led/blink_fallback.c

+	k_work_schedule(&data->work, K_MSEC(data->delay_on));
+}
+
+static void led_blink_fallback_off(struct k_work *work)


Some brightness/blink state should be stored in the data structure (as in Linux). This should allow to have the same work function to turn the LED on or off.

My intent was to save some memory here by only switching the work function.

simonguinot · 2025-05-21T15:22:57Z

drivers/led/Kconfig

+	default 3
+	help
+	  This set the maximum number of LED that could blink
+	  in the same time using blink fallback.


I don't think we should limit the number of LED allowed to blink. If there is no enough memory, it will simply fail.

I have updated the code to statically allocate a struct for each LED defined in the controller.
Probably not the best choice for memory usage, but, it was easier to manage.
We could probably use DTS to enable software blinking for a specific LED.

simonguinot · 2025-05-21T15:23:43Z

drivers/led/CMakeLists.txt

@@ -28,3 +28,5 @@ zephyr_library_sources_ifdef(CONFIG_TLC59108 tlc59108.c)
 zephyr_library_sources_ifdef(CONFIG_LED_SHELL   led_shell.c)

 zephyr_library_sources_ifdef(CONFIG_USERSPACE   led_handlers.c)
+
+zephyr_library_sources_ifdef(CONFIG_LED_BLINK_FALLBACK blink_fallback.c)


Please use "software" instead of "fallback".

simonguinot · 2025-05-21T15:30:08Z

drivers/led/blink_fallback.c

+	}
+
+	/* Stop blinking, more likely when we use led_on or led_off */
+	if (delay_on == 0 && delay_off == 0) {


No way. Please, just make a dedicated function to disable the blinking.

Done! And for the specific case where both delay are 0, I use the policy as Linux: 1Hz blinking. I hope this makes sense.

simonguinot · 2025-05-21T15:35:28Z

drivers/led/blink_fallback.c

+	}
+
+	/* Stop blinking, more likely when we use led_on or led_off */
+	if (delay_on == 0 && delay_off == 0) {


Also you may want to handle some special cases here:

if (!delay_on) { /* always off */ } if (!delay_off) { /* always on */ }

simonguinot · 2025-05-21T16:02:29Z

include/zephyr/drivers/led.h

@@ -128,6 +128,11 @@ __subsystem struct led_driver_api {
 	led_api_write_channels write_channels;
 };

+#ifdef CONFIG_LED_BLINK_FALLBACK
+int led_blink_fallback(const struct device *dev, uint32_t led, uint32_t delay_on,
+		       uint32_t delay_off);


Suggestion:

#ifdef CONFIG_LED_BLINK_SOFTWARE int led_blink_start(const struct device *dev, uint32_t led, uint32_t delay_on, uint32_t delay_off); int led_blink_stop(const struct device *dev, uint32_t led); #else #define led_blink_start(dev, led) do {} while (0) #define led_blink_stop(dev, led) do {} while (0) #endif

Or something like that...

Unless we want to make them "public" API, I prefer to keep them out of led.h.
Since I move some functions to led_core.c, this is not needed here anymore.

simonguinot · 2025-05-21T16:09:03Z

include/zephyr/drivers/led.h

+	if (api->blink == NULL) {
+		led_blink_fallback(dev, led, 0, 0);
+	}
+#endif


And then you can just call led_blink_stop here.

simonguinot · 2025-05-21T16:13:30Z

include/zephyr/drivers/led.h

 		return -ENOSYS;
+#endif
 	}
 	return api->blink(dev, led, delay_on, delay_off);


Maybe this should be moved into led-core.c as well.

faxe1008 · 2025-05-22T12:10:45Z

Random thought: maybe build it in away that allows to build more complex animations (like pulsing, breathing), and let a user provide a step function of sorts to determine the next brightness value.

simonguinot · 2025-05-22T15:18:10Z

Random thought: maybe build it in away that allows to build more complex animations (like pulsing, breathing), and let a user provide a step function of sorts to determine the next brightness value.

It should be discussed but I think that more complex LED effects (run-light, breathing, rainbow, etc) and other things (such as synchronization between devices, color correction, etc) should go in a user library, which is what we are missing the most right now. Blinking is a special case to me. It is cheap and commonly used by simple applications. For theses reasons I think it makes sense to provide a software implementation in Zephyr kernel.

thedjnK

the whole point in a driver is that it controls hardware, this is doing a complete software thing, it has no place in drivers, if this is wanted then it should be a software component i.e. go in subsys

fabiobaltieri · 2025-05-23T14:13:36Z

drivers/led/blink_fallback.c

+
+static void led_blink_fallback_off(struct k_work *work);
+
+static K_MUTEX_DEFINE(led_blink_fallback_mutex);


use a semaphore, lighter implementation, this does not need the extra stuff that comes with mutextes. https://youtu.be/p8OgqVQxklo?si=z8xJ5mp_Qi86zO9V&t=1305

I use static allocation now so this should not be needed anymore.

fabiobaltieri · 2025-05-23T14:15:40Z

drivers/led/blink_fallback.c

+	if (api->on) {
+		api->on(data->dev, data->led);
+	} else {
+		api->set_brightness(data->dev, data->led, LED_BRIGTHNESS_MAX);
+	}


just call led_on, it'll fall back to the right thing for you

I was not able using because it was stopping blinking. I added a function that just do on or off without stopping blinking so I don't need anymore to re-implements the fallback.

fabiobaltieri · 2025-05-23T14:16:20Z

drivers/led/blink_fallback.c

+	if (api->off) {
+		api->off(data->dev, data->led);
+	} else {
+		api->set_brightness(data->dev, data->led, 0);
+	}


same, led_off

In order to develop a sotfware implementation for led blinking, prepare the led API. This move z_impl_led_on and z_impl_led_off code to led_core.c so we could use and update them for blinking a led. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

Blinking data are expected to be attached to led device. In order to have a as much as possible generic implementation, we need a way to retreive the data from the drivers. This adds a new callback that will be used in that purpose. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

The LED blink API is quite useful but this is not supported by all the LED controller. This provides a software implementation of blink that uses a delayable work to blink a LED. To use it, a driver just have to allocate and init a struct for holding blinking led data and to call the software blinking functions. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

This updates the led_gpio driver to use software blinking. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

sonarqubecloud · 2025-06-11T20:21:31Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

anobli · 2025-06-11T20:35:48Z

the whole point in a driver is that it controls hardware, this is doing a complete software thing, it has no place in drivers, if this is wanted then it should be a software component i.e. go in subsys

I am confuse here. Are you talking about the suggestion from @faxe1008 or about doing software blinking in drivers?
If this about doing software blinking in drivers, I disagree. Although there is no hardware support, we are implementing driver API so I think it must be located here.

anobli · 2025-06-11T20:48:07Z

I did a lot of change since the previous version. I hope this is going in the direction even if some improvement are doable.
Software blinking is more complex than I was thinking!
I have experimented different implementation with different pro and cons, and I have not found a solution that fully satisfied me ...
This new version of the PR should have good tradeoff between complexity, memory footprint and the number of change required to enable it in a driver.
I have implemented only the software blinking for the led_gpio driver. I could update the other drivers later once this one get approved.

thedjnK · 2025-06-15T20:52:17Z

the whole point in a driver is that it controls hardware, this is doing a complete software thing, it has no place in drivers, if this is wanted then it should be a software component i.e. go in subsys

I am confuse here. Are you talking about the suggestion from @faxe1008 or about doing software blinking in drivers? If this about doing software blinking in drivers, I disagree. Although there is no hardware support, we are implementing driver API so I think it must be located here.

Have queried and seems people are OK with software implementations for driver functions so can be disregarded

software implementation of driver function is allowed

github-actions bot added the area: LED Label to identify LED subsystem label May 20, 2025

github-actions bot requested review from Mani-Sadhasivam and simonguinot May 20, 2025 19:19

github-actions bot assigned Mani-Sadhasivam and simonguinot May 20, 2025

anobli force-pushed the abailon/led-blink-fallback branch from 1ea7e7c to 27baa49 Compare May 21, 2025 06:35

simonguinot requested review from kartben, soburi, fabiobaltieri, thedjnK and jeppenodgaard May 21, 2025 14:40

simonguinot requested changes May 21, 2025

View reviewed changes

thedjnK previously requested changes May 22, 2025

View reviewed changes

fabiobaltieri reviewed May 23, 2025

View reviewed changes

anobli added 4 commits June 11, 2025 16:15

drivers: led: Create led_core.c

692ba2e

In order to develop a sotfware implementation for led blinking, prepare the led API. This move z_impl_led_on and z_impl_led_off code to led_core.c so we could use and update them for blinking a led. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

drivers: led: gpio: Add support of software blink

4c95949

This updates the led_gpio driver to use software blinking. Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

anobli force-pushed the abailon/led-blink-fallback branch from 27baa49 to 4c95949 Compare June 11, 2025 19:57

thedjnK self-requested a review June 15, 2025 20:52


		static void led_blink_fallback_off(struct k_work *work);

		static K_MUTEX_DEFINE(led_blink_fallback_mutex);

drivers: led: Provide a fallback implementation of blink API #90237

Are you sure you want to change the base?

drivers: led: Provide a fallback implementation of blink API #90237

Conversation

anobli commented May 20, 2025

Uh oh!

simonguinot 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!

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!

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!

faxe1008 commented May 22, 2025

Uh oh!

simonguinot commented May 22, 2025

Uh oh!

thedjnK 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!

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!

sonarqubecloud bot commented Jun 11, 2025

Quality Gate passed

Uh oh!

anobli commented Jun 11, 2025

Uh oh!

anobli commented Jun 11, 2025

Uh oh!

thedjnK commented Jun 15, 2025

Uh oh!

Uh oh!