drivers: led_strip: Add UART-based driver for WS2812 #95174
Conversation
zephyrbot
requested review from
jeppenodgaard,
simonguinot,
soburi and
thedjnK
waihongtam
force-pushed
the
ws2812_uart
branch
2 times, most recently
from
e79c8b8 to
b5f5982
Compare
waihongtam
force-pushed
the
ws2812_uart
branch
2 times, most recently
from
00042f7 to
171a528
Compare
| /* Lock the driver to ensure thread-safe access to the buffer and UART */ | ||
| k_mutex_lock(&data->lock, K_FOREVER); |
There was a problem hiding this comment.
This is a good question. While a binary semaphore can be used to implement mutual exclusion, a mutex is more appropriate here. A mutex clearly states that the code is protecting a shared resource (px_buf), whereas a semaphore is typically for signaling (the ISR signaling the thread). Moreover, a mutex provides priority inversion protection that a semaphore does not.
There was a problem hiding this comment.
Where is priority inversion needed here?
There was a problem hiding this comment.
Sempahores are normally preferred for driver code, few reasons for that, Andy goes in more details in https://youtu.be/p8OgqVQxklo?si=iaVjKvUwvntmViGn&t=1304 but yeah if you don't need priority inversion semaphore seems to be the way to go, also since most core code do not use mutexes the code has a chance of being compiled out if we keep not using it in drivers.
There was a problem hiding this comment.
Thank you both for the feedback, and especially for the link to Andy's talk. It provided great context on Zephyr's common practices.
I understand the general preference for semaphores in Zephyr drivers, as they are a smaller and faster implementation when priority inversion protection is not required.
My choice to use a mutex here is a trade-off that prioritizes robustness over performance gains. I particularly aim to support the following scenario:
- A low-priority thread (e.g., a workqueue thread) handles a continuous LED animation, like a breathing effect, by periodically calling
update_rgb(). - A high-priority thread handles commands from an external host (the application processor) that must immediately overwrite the LED state for factory testing or to indicate a critical status. This thread also calls
update_rgb().
In this situation, the high-priority thread could be blocked if the low-priority thread is preempted by a medium-priority task while holding the lock. A mutex with priority inheritance solves this problem transparently, making the driver more robust.
Given that we can't predict how developers will integrate this driver, I feel the safer approach is better suited for a general-purpose driver.
|
|
||
| for (int i = 0; i < cfg->num_colors; i++) { | ||
| switch (cfg->color_mapping[i]) { | ||
| case LED_COLOR_ID_WHITE: |
There was a problem hiding this comment.
if not supported as above, then should return an error?
There was a problem hiding this comment.
The current behavior is consistent with other ws2812 drivers.
Due to an API limitation, the update_rgb() API doesn't support the WHITE color, as the argument pixels is an array of RGB colors, not RGBW. For WS2812 devices that support a dedicated WHITE channel, the API will set the WHITE channel to a value of 0. An error is not returned because the update_rgb() API still successfully performs its intended task.
waihongtam
force-pushed
the
ws2812_uart
branch
from
171a528 to
3f57c9e
Compare
|
Here is a previous attempt to merge an UART driver for WS2812 controller: #72713 |
|
Hi @matt-wood-ct. Maybe you would be interested in testing this driver ? |
There was a problem hiding this comment.
Hi @waihongtam,
Thanks for this very nice driver. I don't have much to say. Please see my comments below.
Additionally, I'll test it on an Everlight B1414 controller. The timings are different from the Worldsemi WS2812 (4 bits are needed to represent a "zero" or "one" bit).
Add a new devicetree binding, worldsemi,ws2812-uart, for controlling WS2812-compatible LED strips using a UART peripheral. Signed-off-by: Wai-Hong Tam <waihong@google.com>
This commit introduces a new driver for WS2812 and compatible LED strips that uses a UART peripheral. The driver generates the precise, high-speed signal required by the WS2812 protocol by encoding each data bit into a multi-bit "symbol" and using a frame-aware packing strategy for transmission: - Signal Inversion: The UART's TX line must be inverted (tx-invert) to create the protocol's required idle-low signal. A UART start bit then generates the initial high pulse of a WS2812 bit. - Frame-Aware Packing: The driver reuses the UART's hardware- generated start and stop bits as the first and last bits of the on-wire symbol. The inner bits of the symbol are packed into the UART data payload. This packing scheme imposes a configuration constraint: the total number of bits in a UART frame (1 start + N data + 1 stop) must be an integer multiple of the symbol's length (bits-per-symbol). Signed-off-by: Wai-Hong Tam <waihong@google.com>
Add the `uart-controller-pin-inversion.yaml` include to the `vnd,serial` test binding. This enables testing for other drivers that require pin inversion to function correctly. For example, the `worldsemi,ws2812-uart` LED strip driver relies on `tx-invert`. Signed-off-by: Wai-Hong Tam <waihong@google.com>
…driver This commit adds a build test for the new `worldsemi,ws2812-uart` driver. A devicetree node for the driver is added to the app.overlay, instantiated on a `vnd,serial` test UART. Signed-off-by: Wai-Hong Tam <waihong@google.com>
waihongtam
force-pushed
the
ws2812_uart
branch
from
3f57c9e to
5213ea1
Compare
|
Sorry I don't have HW to test this on at the moment, but I had a quick review of the code and it looks good to me |
There was a problem hiding this comment.
Hi @waihongtam,
Sorry for the delay. It took me a while to test the driver. I had trouble finding a board with a serial controller capable of delivering a 2.5-3 Mbps rate. And I also had trouble configuring it...
So I was able to test the driver with a strip of 18 Everlight B1414 controllers (with slightly different timings than the WS2812) connected to an LPC11U68 LPCXpresso board (with modified serial and clock drivers).
The driver looks very good and I only have one comment / question left.
This commit introduces a new driver for WS2812 and compatible LED strips that uses a UART peripheral.
The driver generates the precise, high-speed signal required by the WS2812 protocol by encoding each data bit into a multi-bit "symbol" and using a frame-aware packing strategy for transmission:
This packing scheme imposes a configuration constraint: the total number of bits in a UART frame (1 start + N data + 1 stop) must be an integer multiple of the symbol's length (bits-per-symbol).