rpi_pico: Add PIO based WS2812 LED strip driver #55226
Conversation
soburi
force-pushed
the
pico_ws2812
branch
2 times, most recently
from
bdbe7a0 to
17a96a8
Compare
|
Thanks @soburi , I really want to get PIO merged before v3.4. |
yonsch
left a comment
yonsch
left a comment
There was a problem hiding this comment.
I'm integrating your proposal into my PR, I added some questions.
Another general question: Can two drivers use the same PIO given that it has enough state machines. If it can't, can we add an assertion to make sure two drivers aren't trying to use the same PIO?
There was a problem hiding this comment.
My problem with this is that it requires you to redefine the pin here (as 17), when you've already done so in the pinctrl state. Now to change the pin you have to change both. Ideally, pinctrl would allow us to define names for pins in the pinctrl state, but currently it isn't supported. Any other ideas?
There was a problem hiding this comment.
IMO, there isn't a better way at this point.
The only way to set things in one place is to change pinctrl, but there are some difficulties.
First, pinctrl has a hierarchical structure.
Creating a macro that statically resolves name-to-pin correspondences takes a lot of work.
(It should change dts parsing script.)
We also need to determine the valid scope definitions for names.
Also, pinctrl can switch between sets of settings depending on the state. (Only default can select for rpi_pico at this time.)
Since the changed setting may not always include the definition of the pin name, it is necessary to consider the behavior at that time.
Given the above, this is out of the current pinctrl design scope, so I'm choosing the current method.
It would be a good idea to add pin names in pinctrl, but it's a too big issue to address in this PR.
There was a problem hiding this comment.
Is there a reason for choosing PRE_KERNEL_2 and not earlier? If possible, I think it should be initialized as early as possible, because devices relying on this in the future might need it to be ready very early
There was a problem hiding this comment.
It makes sense.
I change it to PRE_KERNEL_1, the same as the reset-controller priority.
It is early enough before other driver initialization.
soburi
force-pushed
the
pico_ws2812
branch
3 times, most recently
from
7d6aaf3 to
8cf9d59
Compare
I added an assert to error if there are more than 2 child nodes. |
soburi
force-pushed
the
pico_ws2812
branch
3 times, most recently
from
cc3a865 to
2607a59
Compare
soburi
requested review from
MaureenHelm,
galak,
mbolivar-nordic and
nashif
as code owners
zephyrbot
added
area: Devicetree Binding
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
Hi @soburi,
The Raspberry Pi PIO interface looks really great. And it is nice to see how it can be used to handle WS2812 and compatible LED strip controllers.
In addition your driver looks good. Please see my comments and questions below.
drivers/led_strip/Kconfig.ws2812
Outdated
There was a problem hiding this comment.
Since it is a WS2812 sub-option you can shorten the prompt string to Raspberry Pi PIO.
There was a problem hiding this comment.
Thank you for reviewing.
I fixed it.
There was a problem hiding this comment.
I don't know how works PIO but in ws2812_rpi_pico_pio_init() you initialized a state machine returned by pio_claim_unused_sm(). So is it correct to use 0 here (which is eventually not the SM you initialized) ?
Again I don't know how pio_sm_put_blocking works but does it wait until all the data is in the Tx FIFO before sending it over the wire? Did you test this on a strip with several LEDs chained ? Or did you check the output signal ?
When a signal is passed along to several chained WS2812 controllers then there should be no extra delays between the configurations of each LED.
In addition you probably need to enforce the WS2812 reset delay somewhere in this function. See reset_delay in the ws2812_spi driver.
There was a problem hiding this comment.
I don't know how works PIO but in ws2812_rpi_pico_pio_init() you initialized a state machine returned by pio_claim_unused_sm(). So is it correct to use 0 here (which is eventually not the SM you initialized) ?
You are absolutely right. It my mistake.
Fixed it.
There was a problem hiding this comment.
Again I don't know how pio_sm_put_blocking works but does it wait until all the data is in the Tx FIFO before sending it over the wire? Did you test this on a strip with several LEDs chained ? Or did you check the output signal ?
pio_sm_put_blocking blocks only when fifo is full. otherwise, append data to last of fifo and returns.
So, This program works...
- Loop 4 times to fill the FIFO. (FIFO depth is 4)
- 5th execution will be blocked because FIFO is full.
- The blocking is released when that data transmission is finished. The next data will queue.
- Continue to end of data.
So the data will continuously transmit.
I verified it works with WS2812 x 144 pcs strip.
In addition you probably need to enforce the WS2812 reset delay somewhere in this function. See reset_delay in the ws2812_spi driver.
The delay insert to end of transmission.
There was a problem hiding this comment.
You can drop the const qualifier for the three non-pointer members above.
There was a problem hiding this comment.
Please add a description to the commit message: LED controller model, type of LED, pin number and driving information (PIO), etc
There was a problem hiding this comment.
And please add a description as well to the commit adding the driver. It would be useful to describe a bit the PIO interface and how it can be used to output a signal compatible with WS2812 LED strip controllers.
There was a problem hiding this comment.
Please add a description to the commit message: LED controller model, type of LED, pin number and driving information (PIO), etc
Added short description to commit message
And please add a description as well to the commit adding the driver. It would be useful to describe a bit the PIO interface and how it can be used to output a signal compatible with WS2812 LED strip controllers.
I added information to commit message about setting that used to verify this patch.
There was a problem hiding this comment.
Is the PIO mechanism only available on the RaspberryPi Pico device ?
There was a problem hiding this comment.
@soburi would it make sense to add on-bus: pio to this binding, so that the compatible could just be "worldsemi,ws2812"? That would work if the pio node can have bus: pio in the binding. (see https://docs.zephyrproject.org/latest/build/dts/bindings-syntax.html for documentation about on-bus: and bus:.)
I think we should have done this for the SPI binding -- @simonguinot would you agree? That is, I think that worldsemi,ws2812-spi.yaml could say on-bus: spi and have compatible: "worldsemi,ws2812". That seems a little cleaner than having the -spi suffix in the compatible since it is not really necessary.
(I think a similar change for the i2s and GPIO bindings would be a bit different but is probably worth doing too -- in that case, we could unify the bindings into a single worldsemi,ws2812.yaml that either has an i2s-dev (and associated properties) or an in-gpios property. A bit more boilerplate in this case than in the SPI and PIO cases where we have a parent bus-like device.)
There was a problem hiding this comment.
Hi @mbolivar-nordic. Having different bindings for a same compatible based on the bus type is a nice feature that I was not aware of. Yes I agree it would be a bit cleaner. For the I2C and GPIO unified binding how does it work to require a property OR another ?
There was a problem hiding this comment.
This is a widely effect topic.
I think it is good to discuss in separate topic.
There was a problem hiding this comment.
Well I only have a vague idea of what is happening above :) Please try to explain how data is serialized on the wire and how is build the signal expected by the WS2812 controller.
There was a problem hiding this comment.
Added comments for explain the PIO program behavior.
There was a problem hiding this comment.
You probably need to set the reset-delay property here.
There was a problem hiding this comment.
How relevant would it be to define signal characteristics here (such as spi-one-frame and spi-zero-frame in worldsemi,ws2812-spi.yaml) and handle them in the driver ? This would allow the driver to be used with other WS2812 compatible controllers that have slightly different signal characteristics (such as B1414).
There was a problem hiding this comment.
I'm also a bit confused by this description. The ws2812 protocol isn't really based on a clock frequency; it uses pulse widths. So how does this frequency affect the pulse widths? Can this be specified a bit more clearly?
There was a problem hiding this comment.
Does the driver support a different frequency ? I don't see in the driver how the signal is adjusted according to the frequency.
There was a problem hiding this comment.
@simonguinot perhaps this is done by the sm_config_set_clkdiv(&sm_config, clkdiv); call?
There was a problem hiding this comment.
Yes but then changing the frequency also changes the duration of the pulses output by the PIO program. AFAIU (and I don't understand much :) the pulses are "hardcoded" by the PIO program instructions. So I suspect that using a different frequency won't work.
There was a problem hiding this comment.
I see -- if that's the case, there seems to be no need for this property.
There was a problem hiding this comment.
This property intends to supporting WS2811(400kHz), WS2813 or WS2815(2MHz).
It is inherit specs from https://github.com/raspberrypi/pico-examples/tree/master/pio/ws2812, not yet tested in zephyr excluding default value.
There was a problem hiding this comment.
I understood it that is the optimal value is different by LED models.
But I'm not have other than ws2812.
I'm ordering ws2813 now. I'll consider to add the option for tune to particular LED model.
There was a problem hiding this comment.
So how does this frequency affect the pulses ?
Let's say a user selects the 400kHz frequency for a strip of WS2811 devices. In that case is it still correct for the PIO program to output 0001100000 and 0001111111 pulses for 0 and 1 bits ?
Did you consider to make the pulses configurable as in the SPI driver (see the spi-one-frame and spi-zero-frame DT properties) ? This would make this driver compatible with more LED controllers.
There was a problem hiding this comment.
I understood it that is the optimal value is different by LED models. But I'm not have other than ws2812. I'm ordering ws2813 now. I'll consider to add the option for tune to particular LED model.
The frequency "probably" affects the width pulse of a bit. So I don't understand how hardcoded pulse values for 0 and 1 bits can work with different frequencies. Indeed it would be interesting to test with another model.
|
Hi @soburi, I have an Currently, I'm running a 8 NeoPixel led strip with the Adafruit RP2040 using SPI. I'd like to drive the builtin NeoPixel but it appears that the current led_strip driver only works with nRF devices. Would I be able use your PIO based I tried using your PIO based WS2812 driver but ran into the errors below. Any suggestions on how I can resolve the issue(s)? Thank you.
|
It is not @zpm1066. I have been using the ws2812_spi driver with various ST and NXP devices. |
Hi @simonguinot. Thanks. Yes, the ws2812_spi driver works well with nRF52, STM, and RP2040 devices. |
mbolivar-nordic
left a comment
mbolivar-nordic
left a comment
There was a problem hiding this comment.
Thanks for the driver!
I'm definitely interested in getting more ws2812 drivers merged into Zephyr. They are such weird little devices and I think having SoC-specific drivers when possible that use nice features like pio makes total sense.
A few comments and questions from my side. I also agree with everything @simonguinot has posted so far.
There was a problem hiding this comment.
@soburi would it make sense to add on-bus: pio to this binding, so that the compatible could just be "worldsemi,ws2812"? That would work if the pio node can have bus: pio in the binding. (see https://docs.zephyrproject.org/latest/build/dts/bindings-syntax.html for documentation about on-bus: and bus:.)
I think we should have done this for the SPI binding -- @simonguinot would you agree? That is, I think that worldsemi,ws2812-spi.yaml could say on-bus: spi and have compatible: "worldsemi,ws2812". That seems a little cleaner than having the -spi suffix in the compatible since it is not really necessary.
(I think a similar change for the i2s and GPIO bindings would be a bit different but is probably worth doing too -- in that case, we could unify the bindings into a single worldsemi,ws2812.yaml that either has an i2s-dev (and associated properties) or an in-gpios property. A bit more boilerplate in this case than in the SPI and PIO cases where we have a parent bus-like device.)
There was a problem hiding this comment.
@simonguinot perhaps this is done by the sm_config_set_clkdiv(&sm_config, clkdiv); call?
There was a problem hiding this comment.
I'm also a bit confused by this description. The ws2812 protocol isn't really based on a clock frequency; it uses pulse widths. So how does this frequency affect the pulse widths? Can this be specified a bit more clearly?
Great suggestion! |
|
Sorry for the very slow response. I reorganized it based on your proposal at #55226 (comment). I'm also responding to your comments. #55226 (review)P.S. I was a little busy because I had an announcement at Open Source Summit Japan. I also gave a little introduction to this patch. |
rexut
left a comment
rexut
left a comment
There was a problem hiding this comment.
@soburi first of all, thank you very much for all your effort to work on this feature. You were my savior when I found a WS2812 LED on a "non-SPI" pin and only the PIO of the RP2040 could be used.
Please have a look at my little comment. If a small renaming is still possible, it would be good.
There was a problem hiding this comment.
suggestion (non-blocking): The new compatibility string is somewhat misleading and could suggest that this feature could only be used for one board. The name "rpi_pico" refers to a board, namely the "Raspberry Pi Pico", which is based on the SoC called "RP2040". It is precisely this SoC that has this special PIO unit, not the board. A naming "worldsemi,ws2812-rp2040-pio" would be more accurate and comprehensible here.
There was a problem hiding this comment.
This has been a topic of discussion since the beginning of support for rpi pico, but at this point the official name of the SoC family name is unknown, and the discussion will be reconsidered once a successor model is released and the family name is confirmed. This PR also follows that policy.
https://discord.com/channels/720317445772017664/938474761405726800/1177014983423442964
This is the discussion at the time of the first commit:
#34835 (review)
The conclusion is that the name of a clear "SoC family" is unknown now.
There isn't clear consistency, including Pico-SDK.The introduction of RP1 has made things even more confusing.
https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdfI think it would be okay to reconsider when the family name becomes clearer as variations increase, as was the case in the original discussion.
There was a problem hiding this comment.
Okay, I understand. I didn't realize how confusing the current situation with the chip name is. I had been wondering about it for months…
Hi @soburi, Thanks for the update. I'll review it this week-end. P.S. Nice talk at the Open Source Summit Japan ! |
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
Thanks for your patience @soburi. Great work ! It looks good to me. See below for my only comment.
Let's try to bring more eyes on this PR in order to get one more approval.
|
Hi @bbilas. Please can you have a look at this PR ? I know it is not led but led_strip, but it is quite the same story :) Thanks in advance ! |
Add driver that based on RPI-PICO's PIO feature for ws2812.
This driver can handle WS2812 or compatible LED strips.
The single PIO node can handle up to 4 strips.
Any pins that can be configured for PIO can be used for strips.
I verified the samples/driver/led_ws2812 sample
working with WS2812(144 pcs) led strip using following patches.
- samples/drivers/led_ws2812/boards/rpi_pico.overlay
```
/ {
aliases {
led-strip = &ws2812;
};
};
&pinctrl {
ws2812_pio0_default: ws2812_pio0_default {
ws2812 {
pinmux = <PIO0_P21>;
};
};
};
&pio0 {
status = "okay";
pio-ws2812 {
compatible = "worldsemi,ws2812-rpi_pico-pio";
status = "okay";
pinctrl-0 = <&ws2812_pio0_default>;
pinctrl-names = "default";
bit-waveform = <3>, <3>, <4>;
ws2812: ws2812 {
status = "okay";
output-pin = <21>;
chain-length = <144>;
color-mapping = <LED_COLOR_ID_GREEN
LED_COLOR_ID_RED
LED_COLOR_ID_BLUE>;
reset-delay = <280>;
frequency = <800000>;
};
};
};
```
- samples/drivers/led_ws2812/boards/rpi_pico.conf
```
CONFIG_WS2812_STRIP_RPI_PICO_PIO=y
```
Signed-off-by: TOKITA Hiroshi <[email protected]>
Adafruit KB2040 has one NeoPixel(WS2812) LED that attaches to GPIO17 pin. Add configuration for it. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add configuration to use PIO driver for adafruit_kb2040. Signed-off-by: TOKITA Hiroshi <[email protected]>
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
Thanks for the great driver @soburi !
|
Thanks @bbilas ! |
Deeply thanks for your long-term review. I'm relieved that the PR was approved. |
|
Can we now get a Zephyr-based alternative to WLED, please? 😬 |
I think It is possible with wired ethernet. such as https://docs.zephyrproject.org/latest/boards/arm/w5500_evb_pico/doc/index.html. Still, there is a major obstacle to implementing this with wireless LAN. |
Indeed, on the top of the LED (and LED strip) driver API, we are missing a Zepyr userland module to provides LED effects and expose an API for remote control through network, USB, I2C (or other busses). And also we would like to make it compatible enough with existing APIs such as Razer or Microsoft Dynamic Lighting. |
There is this which is very primitive: https://github.com/thedjnK/adalight_rgb but allows usage of https://github.com/psieg/Lightpack for doing effects on LED strips from a PC |
11 participants
This PR adds a WS2812 driver that uses the PIO functionality of the Raspberry Pi Pico.
#44316 already merged. I rebased this PR for newest.
Support for PIO functionality is already in progress in #44316, and this PR captures some of that commits.This demonstrates a different PIO use case than #44316.
I hope it will help discussion about implementing PIO function.
This PR proposes an extension to make the GPIO pins more configurable by adding the PIO driver that configures which pin the PIO uses.
The PIO driver mainly manages resources, and specific functions are implemented by each client driver.
Specifically, add two properties,
gpiosandgpio-namesto the PIO node of dts so that you can specify the pin by name. These properties allow client drivers to look up pins by symbolic name, allow to change pin assignments without driver modification.@yonsch
I made it based on your PR.
I would be happy if you could give me your opinion.