drivers: led_strip: Add UART driver support for WS2812 #72713

AyushKot96 · 2024-05-14T07:44:38Z

DESCRIPTION

This PR adds a new uart driver for controlling WS2812 rgb led using tx pin.

TESTING

Testing has been done using samples/drivers/led_strip application with board stamp_c3.
For this driver to work with esp32C3, I added uart pin inversion feature in a seperate PR 72574.

west build -b stamp_c3 samples/drivers/led_strip -p

simonguinot

Hello @AyushKot96,

Thanks for this driver. It is very smart.

I wonder what protocol will be used next to drive WS2812 controllers :)

Please find my early feedback below. I like the idea but I have some concerns, especially with the baud rate and start stop bits usage.

drivers/led_strip/ws2812_uart.c

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

samples/drivers/led_strip/boards/stamp_c3.overlay

drivers/led_strip/Kconfig.ws2812

drivers/led_strip/ws2812_uart.c

samples/drivers/led_strip/boards/stamp_c3.overlay

simonguinot · 2024-06-23T10:26:20Z

Sorry for the delay @AyushKot96. I'll review it next week.

simonguinot · 2024-07-04T16:35:59Z

@AyushKot96 I'm on it but I'm having trouble to get it working with a strip of B1414 (WS2812 compatible) LED controllers.

@soburi @thedjnK please, can you help me with the review ? Thanks :)

simonguinot · 2024-07-06T12:10:54Z

I finally managed to successfully test the ws2812_uart driver with a strip of 18 B1414 LED controllers on an NXP LPC11U68 board.

I had a lot of troubles configuring the UART controller to a 3MHz speed. I suspect this will be the case with more or less all UART controllers...

I used the DT overlay below. Note that I had to use 5 UART bits to represent 1 RGB bit because the UART controller of this MCU is unable to send a frame with less than 7 data bits.

/*
 * Copyright (c) 2021 Seagate Technology LLC
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <zephyr/dt-bindings/led/led.h>

&uart4 {
        status = "okay";

        current-speed = <3000000>;
        pinctrl-0 = <&uart4_default>;
        pinctrl-names = "default";
        data-bits = <8>;
        tx-invert;

        led_strip: ws2812 {
                compatible = "everlight,b1414","worldsemi,ws2812-uart";
                status = "okay";

                /* B1414 */
                chain-length = <18>; /* arbitrary; change at will */
                color-mapping = <LED_COLOR_ID_BLUE
                                 LED_COLOR_ID_GREEN
                                 LED_COLOR_ID_RED>;
                reset-delay = <250>;

                /*
                 * Everlight B1414 LED controller;
                 *
                 * Each bit of the control signal (waveform) is described with a 1.2 us pulse:
                 * 0 bit: 300 ns high and 900 ns low.
                 * 1 bit: 900 ns high and 300 ns low.
                 *
                 * At 3 MHz, one UART bit pulse lasts 333 ns.
                 *
                 * 5 UART bits per RGB bit
                 * - RGB bit 0: 10000
                 * - RGB bit 1: 11100
                 */
                rgb-frame-per-uart-frame = <2>;
                zero-frame = <0x10>;
                one-frame = <0x1c>;
                frame-len = <5>;
        };
};
  
/ {
        aliases {
                led-strip = &led_strip;
        };
};

simonguinot

Hello @AyushKot96,

First of all, I apologize for the long delay in this review.

There are good news:

I finally managed to successfully test the ws2812_uart driver with a strip of 18 B1414 LED controllers on an NXP LPC11U68 board. This means that the driver and its macros are flexible enough to support a different controller and UART configuration than yours, which is great.
The description messages in worldsemi,ws2812-uart.yaml are much more understandable. And the new DT properties are really easier for users to configure.

And here are the negative points;

The macro are quite a nightmare to understand and probably to maintain. I wonder if providing users with an external program/script (or something else running a build time) to generate the lookup table in DT wouldn't be a better approach. I'd like to hear/read other people's thoughts on these macros. @thedjnK @soburi @kartben please can you have a look ?
There are still a lot of inaccuracies here and there.

Please find my review below.

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

simonguinot · 2024-06-23T10:23:48Z

samples/drivers/led_strip/boards/nucleo_u575zi_q.overlay

Please, move the sample overlay files into another commit.

I agree that commit restructuring is required, but can we do it later to ease up review cycle?
for e.g. i'm planning to push all review point fixes in a separate commit so that reviewer can easily find them, and then once everything is passed i'll restructure commits.
I can do it with each review iteration too, if that is a preferred approach

I agree that commit restructuring is required, but can we do it later to ease up review cycle? for e.g. i'm planning to push all review point fixes in a separate commit so that reviewer can easily find them, and then once everything is passed i'll restructure commits. I can do it with each review iteration too, if that is a preferred approach

Please don't do that. Fix and amend your commits at each iteration. Reviewers will be able to track changes.

commits rearranged, please review

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

drivers/led_strip/ws2812_uart.c

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

drivers/led_strip/ws2812_uart.c

thedjnK · 2024-07-16T20:32:19Z

drivers/led_strip/ws2812_uart.c

I don't think this should assert, give a warning perhaps if it is not set, but it would be valid operation to use a normal UART that does not support this mode with an external single input invert logic gate

Looked around and unable to find a way to print warning based on condition in macro expansion.
Can you please provide some reference and guidance

It would need to be done through cmake e.g. message(WARNING "message here")

drivers/led_strip/ws2812_uart.c

samples/drivers/led_strip/boards/nucleo_u575zi_q.overlay

matt-wood-ct · 2024-07-16T21:38:01Z

This is very cool, great work man. These addressable LEDs are tricky so the more alternative drive methods the better 😄 I think the next one worth doing after UART is Espressif's RMT, that is a classic and the ESP series SOCs are very popular these days.

cx-anuj-pathak · 2024-08-12T08:22:03Z

@simonguinot @thedjnK
FYI, Since @AyushKot96 is busy in some other critical task, I'll resolve all pending and future review points.
I hope you are fine with it,

cx-anuj-pathak · 2024-08-12T09:51:14Z

And here are the negative points;

The macro are quite a nightmare to understand and probably to maintain. I wonder if providing users with an external program/script (or something else running a build time) to generate the lookup table in DT wouldn't be a better approach. I'd like to hear/read other people's thoughts on these macros. @thedjnK @soburi @kartben please can you have a look ?

Do we have any resolution on this?
In my view macro is integrated solution with some added effort on maintaining,
However a script is multistep solution (generate and then use it in board files). I don't know if we can integrate it in the build process or not.

Having said that i can change it back to a lut property in dts, with a python script to generate lut. If community inclined to have a python script rather than having a macro in the driver, Please let me know where to place that and add documentation of that to guide the user of this driver effectively.

Note: I'm unable to resolve review points. please resolve them or let me know if i should open a new PR just to resolve review points.

Added worldsemi,ws2812-uart DT binding. Signed-off-by: Ayush Kothari <ayush@croxel.com> Signed-off-by: Anuj Pathak <anuj@croxel.com>

- ws2812 or compatible led's can be driven using uart peripheral if internal or external tx-pin-inversion is used. We could send multiple pixel bits in one uart frame, making it ram efficient. Signed-off-by: Ayush Kothari <ayush@croxel.com> Signed-off-by: Anuj Pathak <anuj@croxel.com>

- added overlay file to test it on stamp_c3 board with on board led - added overlay file to test it on nucleo_u575 board with external led Signed-off-by: Ayush Kothari <ayush@croxel.com> Signed-off-by: Anuj Pathak <anuj@croxel.com>

cx-anuj-pathak · 2024-08-23T10:35:14Z

@simonguinot @thedjnK gentle reminder

thedjnK · 2024-08-27T17:44:11Z

drivers/led_strip/CMakeLists.txt

 zephyr_library_sources_ifdef(CONFIG_TLC5971_STRIP       tlc5971.c)
 zephyr_library_sources_ifdef(CONFIG_TLC59731_STRIP      tlc59731.c)
+
+if (CONFIG_WS2812_STRIP_UART)


Suggested change

if (CONFIG_WS2812_STRIP_UART)

if(CONFIG_WS2812_STRIP_UART)

thedjnK · 2024-08-27T17:44:29Z

drivers/led_strip/CMakeLists.txt

+message(WARNING "WS2812-UART driver depends on tx-pin-inversion. Please make sure \
+you either have it enabled in uart property (if supported by the driver) or \
+have external hardware setup in place to invert tx pin's logic level"
+)


indent with 2 spaces

thedjnK · 2024-08-27T17:45:19Z

drivers/led_strip/ws2812_uart.c

+#include <zephyr/drivers/led_strip.h>
+
+#include <string.h>
+
+#define LOG_LEVEL CONFIG_LED_STRIP_LOG_LEVEL
+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(ws2812_uart);
+
+#include <zephyr/kernel.h>
+#include <zephyr/device.h>
+#include <zephyr/drivers/uart.h>
+#include <zephyr/sys/math_extras.h>
+#include <zephyr/sys/util.h>
+#include <zephyr/dt-bindings/led/led.h>


group these includes, zephyr and non zephyr can be separated but don't need 3 sets of zephyr includes

thedjnK · 2024-08-27T17:47:35Z

drivers/led_strip/ws2812_uart.c

+	const struct ws2812_uart_cfg *cfg = dev->config;
+
+	/* Loop through each of the 3 color components (Red, Green, Blue) */
+	for (uint8_t i = 0; i < 3; i++) {


3 is not correct, someone might define 1 colour, 3 colours or even 8, this needs to get it from dts

simonguinot

Hello @AyushKot96,

First thanks for taking over this PR.

Now I'm not happy with the binding description and the overall quality of the driver code. I wish Zephyr had a staging area (as in Linux). It would have been a perfect place for this driver. Indeed I think this driver could be useful and then it would be nice to merge it into Zephyr somehow. But in its current state, it is not possible.

IMHO the biggest problem is macros. They are pretty much unreadable and impossible to maintain. Can you fix that ? I am not sure it is even possible... Eventually a better solution would be to use an external program to generate the lookup table at build time. Maybe you can try to investigate this solution...

Basically you have to bring the driver into a mergeable state. I'll help you as much as I can. But I can't propose a full rewrite of the worldsemi,ws2812-uart.yaml binding at each review. I don't have that kind of time. Reviewers can give you directions and advice, but you have to be more proactive on the reviews.

Thanks in advance !

simonguinot · 2024-08-28T12:58:17Z

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

+  UART Speed and Data-bits should satisfy following requirements too.
+  - (data-bits + 2) should be integer multiple of frame-len.
+  - speed = frame-len * bit_interval
+  Please correlate above equations with MCU and Led Controller datasheet to find compatibility


Suggestion:

Here are the requirements to use this driver: - an UART API driver must be available for your board - the tx-invert UART property must be supported - the UART controller must support frequencies of 2.5 MHz or higher. One bit pulse should last between 300 and 400 nanoseconds (depending on the timings of the WS2812 compatible model). - the UART controller must support a data-bits configuration (with the start and stop bits used) where one or more RGB frames can fit exactly. A data-bits and frame-len configuration with (data bits + 2) an integer multiple of frame-len must be possible.

Or something better... It is important to express these requirements in the most understandable way possible.

simonguinot · 2024-08-28T14:14:10Z

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

+  - speed = frame-len * bit_interval
+  Please correlate above equations with MCU and Led Controller datasheet to find compatibility
+
+  Data serialization explanation for WS2812:


Suggestion:

Here is a configuration and its explanation for a WS2812 controller used with an ESP32C3 MCU: &uart0 { status = "okay"; current-speed = <2500000>; pinctrl-0 = <&uart0_default>; pinctrl-names = "default"; data-bits = <7>; tx-invert; led_strip: ws2812 { compatible = "worldsemi,ws2812-uart"; /* WS2812 */ chain-length = <1>; color-mapping = <LED_COLOR_ID_GREEN LED_COLOR_ID_RED LED_COLOR_ID_BLUE>; rgb-frame-per-uart-frame = <3>; zero-frame = <0x4>; one-frame = <0x6>; frame-len = <3>; }; }; The UART baud rate of the controller (current-speed) is set to 2.5 Mbits per second. This allows to genera a 400 nanosecond pulse per UART bit. frame-len is set to 3. This means that 1 RGB bit is represented with 3 UART bits. Please refer to the WS2812 datasheet for the timings (https://cdn-shop.adafruit.com/datasheets/WS2812.pdf). zero-frame and one-frame represent an RGB bit with a value of 0 and 1, respectively: ___ RGB 0 bit: 0x4 or 0b100 -> _| 1 |_0__0_ _______ RGB 1 bit: 0x6 or 0b110 -> _| 1 1 |_0_ With data-bits set to 7, an UART frame carries 9 bits (7 data bits + start and stop bits). rgb-frame-per-uart-frame is set to 3. So 3 RGB bits will be sent in each UART frame. tx-invert is also enabled. This is needed to have the start and stop bits high and low respectively. Indeed they are used to build the first and last RGB bits stored in the UART frame. This whole configuration is well verified by the following equality: data-bits + 2 = frame-len * rgb-frame-per-uart-frame.

Or something better... Again, it is important to make this as clear as possible to the reader. This will avoid questions later.

simonguinot · 2024-08-28T14:49:55Z

samples/drivers/led_strip/boards/stamp_c3.overlay

+		compatible = "worldsemi,ws2812-uart";
+
+		/* WS2812 */
+		chain-length = <1>; /* arbitrary; change at will */


Is that not a single LED built-in ? If yes, then you can remove the comment.

simonguinot · 2024-08-28T15:32:58Z

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

+include: [uart-controller.yaml, ws2812.yaml]
+
+properties:
+  rgb-frame-per-uart-frame:


I don't like the rgb-frame-per-uart-frame name. It is too long and it adds to the confusion between UART and RGB frame lengths. Indeed, when I read frame-len, I first think to the UART frame length, but in fact it refers to the RGB frame length.

Maybe it would be more clear to assume that "frame" always refer to the RGB frame and then rename
rgb-frame-per-uart-frame into frame-count ?

simonguinot · 2024-08-28T15:36:26Z

dts/bindings/led_strip/worldsemi,ws2812-uart.yaml

+    description: |
+      The UART frame value representing a LED's bit 0.
+
+  one-frame:


Even if I don't like it (because of the RGB and UART frame confusion I mentioned previously), zero-frame and one-frame probably need to be renamed into uart-zero-frame and uart-one-frame, to be consistent with the worldsemi,ws2812-spi binding.

simonguinot · 2024-08-28T15:45:35Z

drivers/led_strip/ws2812_uart.c

+/* Lookup table formation macro definitions starts here*/
+
+/*
+ * Reverses the bits in an 8-bit octet.


simonguinot · 2024-08-28T15:47:11Z

drivers/led_strip/ws2812_uart.c

+ * Reverses the bits in an 8-bit octet.
+ * For example, if the input octet is 0b10110010, the output will be 0b01001101.
+ */
+#define WS2812_UART_BYTE_BITREVERSE(octet)                                                         \


I think you should replace octet by byte everywhere.

fwiw bluetooth subsystem seems to reverse bytes this way:
https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/controller/ll_sw/lll_chan.c#L194-L206

see also http://graphics.stanford.edu/%7Eseander/bithacks.html#ReverseByteWith32Bits

matt-wood-ct · 2024-08-29T08:28:11Z

@cx-anuj-pathak We just tried the driver out of tree on an IMXRT1042 series SOC and ran into some issues, the driver serialised the data OK but we found that any UART config with <8 data bits left time gaps between frames (NXP peripheral hardware issue I suspect) which ruined the timing and also we found the timing was inconsistent due to stuttering between frames due to the use of the polling API.

In the end we had to operate with 8 data bits so 10 bits per frame and run the UART at 8MHz using the async DMA based API yielding consistent timing of about 360uS and 875uS which seems to work fine for our setup.

The driver does not seem to support this setup as frame-len is limited to 3-9, so we ended up writing some test code in main to validate the hardware / firmware setup.

UART config:

tx-invert
databits = 8
baud rate 8mhz

We just manually generated at 24 byte buffer filled with 0xFC then we filled the first 8 bytes with 0x0C and wrote that out the uart using the asynchronous DMA based API to ensure there is not stuttering in the transmission.
This works perfectly for us. We will package this into a bare bones out of tree driver for the mean time, if this driver ends up supporting this arrangement and gets merged we will switch over to it.

github-actions · 2024-10-29T00:34:17Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

ubieda added DNM This PR should not be merged (Do Not Merge) and removed DNM This PR should not be merged (Do Not Merge) labels May 14, 2024

AyushKot96 force-pushed the feature-add-ws2812-uart-driver branch from bb26149 to fffc7df Compare May 14, 2024 15:01

ubieda self-requested a review May 14, 2024 15:45

AyushKot96 force-pushed the feature-add-ws2812-uart-driver branch 3 times, most recently from 03563e0 to a88ed77 Compare May 15, 2024 08:36

AyushKot96 marked this pull request as ready for review May 15, 2024 08:42

AyushKot96 requested a review from mbolivar-ampere as a code owner May 15, 2024 08:42

zephyrbot added area: LED Label to identify LED subsystem area: Samples Samples area: Devicetree Binding PR modifies or adds a Device Tree binding labels May 15, 2024

zephyrbot requested review from bbilas, decsny, galak, kartben, Mani-Sadhasivam, nashif, simonguinot, soburi and thedjnK May 15, 2024 08:43

zephyrbot assigned mbolivar-ampere May 15, 2024

simonguinot reviewed May 15, 2024

View reviewed changes

simonguinot self-assigned this May 15, 2024

thedjnK reviewed May 15, 2024

View reviewed changes

decsny removed their request for review May 15, 2024 18:24

AyushKot96 force-pushed the feature-add-ws2812-uart-driver branch 4 times, most recently from 1de1fb7 to 29df6ce Compare May 16, 2024 10:58

AyushKot96 force-pushed the feature-add-ws2812-uart-driver branch 3 times, most recently from 3ee181f to 2c2536f Compare June 15, 2024 12:23

AyushKot96 requested a review from simonguinot June 15, 2024 12:32

simonguinot mentioned this pull request Jul 6, 2024

drivers: serial: lpc11u6x: allow to configure data polarity #75537

Merged

simonguinot reviewed Jul 10, 2024

View reviewed changes

thedjnK reviewed Jul 16, 2024

View reviewed changes

simonguinot mentioned this pull request Aug 8, 2024

drivers: led: Introduce LP5521 driver #73194

Closed

cx-anuj-pathak force-pushed the feature-add-ws2812-uart-driver branch 3 times, most recently from 0889b3f to cc8530c Compare August 15, 2024 12:15

cx-anuj-pathak added 3 commits August 15, 2024 18:20

dts: bindings: led_strip: Added WS2812-UART dts

f313ea3

Added worldsemi,ws2812-uart DT binding. Signed-off-by: Ayush Kothari <ayush@croxel.com> Signed-off-by: Anuj Pathak <anuj@croxel.com>

cx-anuj-pathak force-pushed the feature-add-ws2812-uart-driver branch from cc8530c to f1d7650 Compare August 15, 2024 12:54

thedjnK reviewed Aug 27, 2024

View reviewed changes

thedjnK requested a review from simonguinot August 27, 2024 17:48

simonguinot requested changes Aug 28, 2024

View reviewed changes

github-actions bot added the Stale label Oct 29, 2024

github-actions bot closed this Nov 13, 2024

simonguinot mentioned this pull request Sep 11, 2025

drivers: led_strip: Add UART-based driver for WS2812 #95174

Merged

drivers: led_strip: Add UART driver support for WS2812 #72713

drivers: led_strip: Add UART driver support for WS2812 #72713

Uh oh!

Conversation

AyushKot96 commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DESCRIPTION

TESTING

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonguinot commented Jun 23, 2024

Uh oh!

simonguinot commented Jul 4, 2024

Uh oh!

simonguinot commented Jul 6, 2024

Uh oh!

simonguinot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

matt-wood-ct commented Jul 16, 2024

Uh oh!

cx-anuj-pathak commented Aug 12, 2024

Uh oh!

cx-anuj-pathak commented Aug 12, 2024

Uh oh!

cx-anuj-pathak commented Aug 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AyushKot96 commented May 14, 2024 •

edited

Loading

simonguinot left a comment •

edited

Loading

simonguinot left a comment •

edited

Loading

kartben Aug 28, 2024 •

edited

Loading