drivers: led_strip: ws2812_gpio: Add support for nRF52 SOC #72050

chaim-zax · 2024-04-28T11:51:05Z

The current driver contains assembly code which is specific for the nRF51 SOC which makes it incompatible with other SOC's. This patch adds support for the nRF52 SOC's as well. The change has minimal impact on existing code to make sure it's fully compatible with the nRF51.

Changes have been verified on a Adafruit Feather nRF52840 Express board, which contains a single NeoPixel RGB LED. Timings have been verified using a scope connected to the WS2812 data line.

github-actions · 2024-04-28T11:51:43Z

Hello @chaim-zax, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

thedjnK

Don't think I really like this, what SoC was this tested on? E.g. nRF52810 has no instruction cache, nRF52840 has an optional instruction cache, was this generated/tested with it on or off since that's going to have a big impact

chaim-zax · 2024-04-28T13:27:14Z

Good remark. It was testing on an nRF52840 with the instruction cache enabled (NRF_ENABLE_ICACHE). I checked the datasheets of all the nRF52's to check if they used the same ARM CPU and same (internal) clock structure. I did not take the impact of the instruction cache into account. Because I currently only have hardware to validate the nRF52840 I would like to suggest to change the CONFIG_SOC_SERIES_NRF52X define in the driver to CONFIG_SOC_NRF52840.

simonguinot · 2024-04-30T14:36:35Z

Hi @chaim-zax,

I am not very fond of the GPIO method for controlling an LED strip :) I think this should only be used for debugging purposes or when the other methods (SPI, I2S) cannot be used. So I don't like the ws2812_gpio driver and I'm not very enthusiastic on extending its support to another SoC... Please can you tell us more about your use case ? And why the ws2812_{i2s,spi} drivers cannot be used ?

chaim-zax · 2024-04-30T16:22:41Z

I'm using the Adafruit Feather nRF52840 Express which has a single RGB LED, so you can consider this a debug LED if you like. I initially started with the ws2812_spi driver, which worked. But this solution needs a minimum of two pins to configure SPI, the MOSI and CLK lines. Because I do not want to waste pins the GPIO solutions seems the only way to control the RGB LED using a single pin. I'm not fond of bit-banging my self as well, but for a single user LED it seems like a reasonable solution. Isn't is up to the end user to decide which driver to use, and if it fits the application? Perhaps the description of the driver should be extended to indicate these limitations?

simonguinot · 2024-05-01T11:13:18Z

I'm using the Adafruit Feather nRF52840 Express which has a single RGB LED, so you can consider this a debug LED if you like.

Sorry, I mean debugging as debugging the LED strip controller itself.

I initially started with the ws2812_spi driver, which worked. But this solution needs a minimum of two pins to configure SPI, the MOSI and CLK lines. Because I do not want to waste pins the GPIO solutions seems the only way to control the RGB LED using a single pin. I'm not fond of bit-banging my self as well, but for a single user LED it seems like a reasonable solution. Isn't is up to the end user to decide which driver to use, and if it fits the application?

Yes you are right, it is up to the end user to decide which driver to use. But it's up to maintainers to decide which code makes sense to maintain :)

The ws2812_gpio driver has been introduced almost 5 years ago. Since then it has not been extended to another SoC or compatible LED strip controller (and there are many out there with slightly different configurations which are actually not supported). So I don't feel a big trend for this driver... I am also concerned about the lack of genericity. Unlike the ws2812_spi driver (which allows output signal to be configured from DT) everything is hard-coded and the user can't supply its own configuration.

That's being said I am not sure what the right direction is and I'd like to hear from others.

Perhaps the description of the driver should be extended to indicate these limitations?

Let's wait for others to express their opinion.

chaim-zax · 2024-05-05T09:58:50Z

Thanks for the explanation. I can understand perfectly if this driver is deprecated and phased out. If that's the case I suggest to update the documentation/description so developers are aware. If this is not the case I don't mind to extend it a bit and make the timing related parameters available in the device tree so it could be used in a more generic way.

For me personally it's just about getting some hands on experience with Zephyr. Let me know what you want to do.

thedjnK · 2024-05-05T10:02:08Z

I'm OK with this driver still being here, it's up to people to ensure if they use it that it works for their end application - for some applications this might be absolutely fine. Though should probably have a depends on/select for the instruction cache on nrf52x devices

chaim-zax · 2024-05-05T11:14:36Z

Agree

it's up to people to ensure if they use it that it works for their end application

So I guess the question is how do we support the end user in making sure it 'works'. Do we only support (specific) hardware/SoC if we can guarantee its correct behavior, do we expect the end user to patch this driver them selves if not, do we extend the driver to make it configurable (using the device tree)?

simonguinot · 2024-05-18T12:28:05Z

@soburi @kartben what do you think ? Should we move forward and extend ws2812_gpio driver support to another SoC ?

soburi · 2024-05-18T19:35:02Z

@simonguinot @chaim-zax

I am not against expanding the scope of support.

However, if we extend it, we should replace it with more general-purpose logic. I don't think it's desirable to add #ifdef for each board.

In this PR, the number of nop is changed, but is it possible to calculate it based on the clock frequency that can be obtained from dts? (We can use LOOPIFY macro here.）
And, If it needs individual adjustment values, I think it's better to set them using devicetree.

chaim-zax · 2024-05-19T11:19:23Z

@soburi

Thanks for your input. I was thinking about using the .rept directive to create a compile time loop of nop's. Could you provide a reference to the LOOPIFY macro, I couldn't find it. It shouldn't be to hard to supply the loop values using the device tree. I'm not sure how easy it is to calculate the values based on a clock frequency. As thedjnK indicated there can be optimizations (like caching) which could make these calculations impossible, although for some SoC's it might work. I wouldn't mind to make a first implementation based on the .rept directive which gets its value from dts, possibly calculating the default values derived from the clock frequency.

soburi · 2024-05-19T11:40:35Z

@chaim-zax

Could you provide a reference to the LOOPIFY macro,

It is a LISTIFY that is correct. Excuse me.
This is used for iteration in compile-time calculations.
I think it can be used for this case as well.

https://docs.zephyrproject.org/apidoc/latest/group__sys-util.html#ga81cbc0233cf73048d65b76f716653af6

simonguinot · 2024-05-19T12:33:25Z

I wouldn't mind to make a first implementation based on the .rept directive which gets its value from dts, possibly calculating the default values derived from the clock frequency.

@chaim-zax, it would be a nice contribution. Thanks for your help

chaim-zax · 2024-05-19T14:56:21Z

Thank you for the clarification. I'll have a look after my (short) holiday and start on an implementation using LISTIFY. Shall I close this pull request and create a new one when I'm ready, or update this pull request?

soburi · 2024-05-19T15:39:22Z

@chaim-zax

Frequently recreating the PR is not a good idea as it will make it difficult to follow the discussion process.
I think it's better to update this PR and force-push a completely new commit.

chaim-zax · 2024-05-27T10:32:15Z

I've updated the pull request to see if I'm heading in the right direction. It's not the final pull request. I have tried to make the timing dynamic based on a clock frequency, unfortunately C macro's don't support arithmetic operations like division (e.g. CLOCK_FREQ / DELAY_T1H). The LISTIFY macro only works if the resulting count argument (LEN) is an integer, and not a formula. If anyone has some ideas how to solve this I would very much like to hear it.

drivers/led_strip/Kconfig.ws2812

drivers/led_strip/ws2812_gpio.c

samples/drivers/led_strip/boards/bbc_microbit.overlay

soburi · 2024-08-06T12:37:55Z

@thedjnK @simonguinot
Could you take a look, please?

simonguinot

Thanks for this pathc @chaim-zax. Please, see my comments below.

simonguinot · 2024-08-06T20:54:12Z

drivers/led_strip/Kconfig.ws2812

+
+config DELAY_T1H
+	int
+	default $(div,$(mul,750,$(dt_node_int_prop_int,/cpus/cpu@0,clock-frequency)),1000000000)


I would prefer to store delays in nanoseconds with these Kconfig options and let the driver do the conversion to number of nops. It would be nice to spare users these calculations :)

In addition, there are different WS2812 compatible models out there, with different timing specifications. For example, for the Everlight B1414 controller, each bit is described with a 1.2 us pulse:

T0H: 300 ns

T0L: 900 ns

T1H: 900 ns

T1L: 300 ns

So if the timings in nanoseconds can be configured here, then this would allow to use the ws2812_gpio driver with these controllers.

I would prefer to store delays in nanoseconds with these Kconfig options and let the driver do the conversion to number of nops. It would be nice to spare users these calculations :)

It is not possible by LISTIFY limitation.
The macro accepts only numeric value, and cannot calculate in C.

zephyr/include/zephyr/sys/util_macro.h

Line 428 in 3240b03

* @param LEN The length of the sequence. Must be an integer literal less

The LISTIFY macro concatinate the argument to UTIL_LISTIFY_#ARG, so ARG must be a numerical literal.

That's correct, the driver implements delays by adding (compile time) 'nop' asm instructions. The LISTIFY macro (which does this) can only do this with constant values. @soburi did a great job making this even possible by adding the option to use arithmetic functions to Kconfig files. Currently I don't see any other way of doing this.

OK, then maybe we can have in Kconfig DELAY_T{0,1}{L,H}_NS configured by user with values in nanoseconds (understandable by a human) and DELAY_T{0,1}{L,H}_NOP hidden, with number of nops and selected/computed automatically from the previous options ?

I don't think this will be a problem. I'll have a look. Although the cpu clock-frequency in the dts tree might not always be available.

Another answer.

Add frequency property to "worldsemi,ws2812-gpio".

Indeed, there is still an option to move some of these properties to the device tree. This would allow to do the calculations from nanoseconds to nop's as well (not the most pretty option, using both dts and Kconfig files), and possibly have a frequency option there too. @simonguinot what's your take on this?

I don't think this will be a problem. I'll have a look. Although the cpu clock-frequency in the dts tree might not always be available.

Note that you can use a second default as a fallback if the clock-frequency property is missing. For example:

default $(div,$(mul,750,$(dt_node_int_prop_int,/cpus/cpu@0,clock-frequency)),1000000000) \ if $(dt_node_has_prop,/cpus/cpu@0,clock-frequency) default 12

Indeed, there is still an option to move some of these properties to the device tree. This would allow to do the calculations from nanoseconds to nop's as well (not the most pretty option, using both dts and Kconfig files), and possibly have a frequency option there too. @simonguinot what's your take on this?

I understand the limitation. So we can't offer nanosecond configuration to the user but we have to stick to NOPs.

Here are my suggestions:

we add new properties to the ws2812-gpio DT bindings to allow the user to configure the number of NOPs for the T0L, T0H, T1L and T1H periods. There is no reason for not offering DT configuration like the other ws2812 bindings.

we write a clear and user friendly description for these new properties. So the user will be able to configure them wisely :)

we add the corresponding Kconfig options (as in your patch), with the first default set to the DT value, the second to a value calculated based on the clock-frequency property (if available) and the ws2812 timings (as found in the specification) and a third default fallback set to an hardcoded value (also wisely chosen). And we allow the user to override these values from their own configuration.

How does that sound ? :)

Sounds good, thanks for the feedback!

@soburi do you have any remarks on this, shall I just go ahead with the implementation as described here?

simonguinot · 2024-08-06T20:58:46Z

drivers/led_strip/Kconfig.ws2812

+	int
+	default $(div,$(mul,500,$(dt_node_int_prop_int,/cpus/cpu@0,clock-frequency)),1000000000)
+	help
+	  inter-bit low pulse delay: 500 nsec


And we should probably use the WS2812 default values:

T0H: 350 ns

T0L: 800 ns

T1H: 700 ns

T1L: 600 ns

The new implementation is based on the current (old) behavior of the driver, but I have no problem changing this. It makes sense to use the WS2812 default values. Aren't there concerns to break existing applications were this driver might already be in use?

Yes there are. But we already crossed that bridge by using clock-frequency to compute the default value...

The idea here is to salvage the driver by making it more generic and configurable. So I think the risk is acceptable. We'll try to minimize it by testing the new driver version with several WS2812-compatible controllers and several boards.

And of course we'll leave a note in the release notes :)

simonguinot · 2024-08-13T13:59:39Z

drivers/led_strip/Kconfig.ws2812

+if WS2812_STRIP_GPIO
+
+config DELAY_T1H
+	int


Note that the prompt is missing on all these new options. And then the bbc_microbit board configuration of the led_strip sample can't be used:

$ west build -b bbc_microbit samples/drivers/led_strip ... Merged configuration '/home/simon/src/zephyrproject/zephyr/samples/drivers/led_strip/boards/bbc_microbit.conf' error: DELAY_T1H (defined at drivers/led_strip/Kconfig.ws2812:49) is assigned in a configuration file, but is not directly user-configurable (has no prompt). It gets its value indirectly from other symbols. See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_DELAY_T1H and/or look up DELAY_T1H in the menuconfig/guiconfig interface. The Application Development Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful too. CMake Error at /home/simon/src/zephyrproject/zephyr/cmake/modules/kconfig.cmake:389 (message): command failed with return code: 1 ...

I'll make sure this fix this, and do some better testing next time ;-)

chaim-zax · 2024-08-23T13:27:45Z

@simonguinot @soburi I made an attempt to implement the solution suggested. Please let me know what you think.

thedjnK

Looks acceptable. Compliance needs fixing

thedjnK · 2024-08-27T17:39:17Z

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

+    description: Number of NOP assembly operations to create a 350 nsec delay for an 0 bit (high pulse)
+
+  delay_t0l:
+    type: int
+    description: Number of NOP assembly operations to create a 800 nsec delay for an 0 bit (low pulse)


Thanks, will fix.

simonguinot · 2024-08-29T16:19:55Z

@simonguinot @soburi I made an attempt to implement the solution suggested. Please let me know what you think.

Thanks @chaim-zax. I'll look at it soon.

simonguinot

It looks good to me ! I like it :) The driver is much more generic and configurable with this patch.

Please see below for few comments.

Also, I will play a bit with the driver on some of my boards.

drivers/led_strip/Kconfig.ws2812

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

doc/build/dts/api/api.rst

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

…ndend The current driver contains assembly code which is specific for the nRF51 SOC which makes it incompatible with other SOC's. This patch adds support for other nRF SOC's as well. The timing is calucated based on the CPU clock frequency, but can be configured manually as well if needed. Changes have been verified on a Adafruit Feather nRF52840 Express board, which contains a single NeoPixel RGB LED. Timings have been verified using a scope connected to the WS1812 data line. Signed-off-by: Chaim Zax <chaim.zax@zaxx.pro>

simonguinot

Good work !

github-actions · 2024-09-06T15:31:40Z

Hi @chaim-zax!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

chaim-zax requested a review from mbolivar-ampere as a code owner April 28, 2024 11:51

zephyrbot added the area: LED Label to identify LED subsystem label Apr 28, 2024

zephyrbot requested review from simonguinot, soburi and thedjnK April 28, 2024 11:51

zephyrbot assigned mbolivar-ampere Apr 28, 2024

thedjnK reviewed Apr 28, 2024

View reviewed changes

simonguinot self-assigned this Apr 28, 2024

simonguinot requested a review from kartben April 30, 2024 14:38

chaim-zax force-pushed the nrf52_support_for_ws2812_gpio_driver branch from d8a99f3 to 121cfb7 Compare May 27, 2024 10:25

zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label May 27, 2024

zephyrbot requested review from decsny and galak May 27, 2024 10:26

chaim-zax commented May 27, 2024

View reviewed changes

drivers/led_strip/Kconfig.ws2812 Outdated Show resolved Hide resolved

drivers/led_strip/ws2812_gpio.c Outdated Show resolved Hide resolved

soburi reviewed Aug 5, 2024

View reviewed changes

samples/drivers/led_strip/boards/bbc_microbit.overlay Outdated Show resolved Hide resolved

chaim-zax force-pushed the nrf52_support_for_ws2812_gpio_driver branch from dc8d7c2 to 7ce785a Compare August 6, 2024 12:33

soburi previously approved these changes Aug 6, 2024

View reviewed changes

soburi requested a review from thedjnK August 6, 2024 12:38

simonguinot requested changes Aug 6, 2024

View reviewed changes

simonguinot reviewed Aug 13, 2024

View reviewed changes

chaim-zax dismissed soburi’s stale review via 571d915 August 23, 2024 13:25

chaim-zax force-pushed the nrf52_support_for_ws2812_gpio_driver branch from 7ce785a to 571d915 Compare August 23, 2024 13:25

zephyrbot added the area: Build System label Aug 23, 2024

zephyrbot requested review from 57300, jeremybettis and nordicjm August 23, 2024 13:26

thedjnK reviewed Aug 27, 2024

View reviewed changes

simonguinot reviewed Sep 2, 2024

View reviewed changes

drivers/led_strip/Kconfig.ws2812 Show resolved Hide resolved

dts/bindings/led_strip/worldsemi,ws2812-gpio.yaml Outdated Show resolved Hide resolved

doc/build/dts/api/api.rst Show resolved Hide resolved

soburi reviewed Sep 2, 2024

View reviewed changes

dts/bindings/led_strip/worldsemi,ws2812-gpio.yaml Outdated Show resolved Hide resolved

chaim-zax force-pushed the nrf52_support_for_ws2812_gpio_driver branch from 571d915 to 7007654 Compare September 2, 2024 15:09

zephyrbot added the Release Notes To be mentioned in the release notes label Sep 2, 2024

chaim-zax force-pushed the nrf52_support_for_ws2812_gpio_driver branch from 7007654 to 7df7d30 Compare September 3, 2024 10:49

soburi approved these changes Sep 4, 2024

View reviewed changes

thedjnK approved these changes Sep 4, 2024

View reviewed changes

simonguinot self-requested a review September 6, 2024 14:55

simonguinot approved these changes Sep 6, 2024

View reviewed changes

nashif merged commit f54a53b into zephyrproject-rtos:main Sep 6, 2024
23 checks passed

This was referenced Dec 3, 2024

GPIO: need generic API for optimized pin read/write operations #11917

Open

Usage of led_strip (WS2812B) with gpio driver for non nRF devices #82479

Open

drivers: led_strip: ws2812_gpio: Add support for nRF52 SOC #72050

drivers: led_strip: ws2812_gpio: Add support for nRF52 SOC #72050

Conversation

chaim-zax commented Apr 28, 2024 • edited Loading

github-actions bot commented Apr 28, 2024

thedjnK left a comment

Choose a reason for hiding this comment

chaim-zax commented Apr 28, 2024

simonguinot commented Apr 30, 2024 • edited Loading

chaim-zax commented Apr 30, 2024 • edited Loading

simonguinot commented May 1, 2024 • edited Loading

chaim-zax commented May 5, 2024

thedjnK commented May 5, 2024

chaim-zax commented May 5, 2024 • edited Loading

simonguinot commented May 18, 2024

soburi commented May 18, 2024 • edited Loading

chaim-zax commented May 19, 2024

soburi commented May 19, 2024

simonguinot commented May 19, 2024

chaim-zax commented May 19, 2024

soburi commented May 19, 2024

chaim-zax commented May 27, 2024 • edited Loading

soburi commented Aug 6, 2024

simonguinot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soburi Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonguinot Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

chaim-zax Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonguinot Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

simonguinot Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonguinot Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaim-zax commented Aug 23, 2024

thedjnK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonguinot commented Aug 29, 2024

simonguinot left a comment

Choose a reason for hiding this comment

simonguinot left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024

chaim-zax commented Apr 28, 2024 •

edited

Loading

simonguinot commented Apr 30, 2024 •

edited

Loading

chaim-zax commented Apr 30, 2024 •

edited

Loading

simonguinot commented May 1, 2024 •

edited

Loading

chaim-zax commented May 5, 2024 •

edited

Loading

soburi commented May 18, 2024 •

edited

Loading

chaim-zax commented May 27, 2024 •

edited

Loading

soburi Aug 6, 2024 •

edited

Loading

simonguinot Aug 7, 2024 •

edited

Loading

chaim-zax Aug 8, 2024 •

edited

Loading

simonguinot Aug 13, 2024 •

edited

Loading

simonguinot Aug 13, 2024 •

edited

Loading

simonguinot Aug 13, 2024 •

edited

Loading