zephyrproject-rtos
drivers: led_strip: ws2812_gpio: Add support for nRF52 SOC
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.
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. 😊
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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 ?
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?
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.
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.
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
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)?
@soburi @kartben what do you think ? Should we move forward and extend ws2812_gpio driver support to another SoC ?
@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.
@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.
@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
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
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?
@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.
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.
I've updated the pull request to see if I'm heading in the right direction. It's not the final pull request.
I think this direction is preferable.
If anyone has some ideas how to solve this I would very much like to hear it.
I didn't notice it when I first commented, but this is a bit of a problem. One idea is to extend the script and do the calculations inside Kconfig.
https://github.com/soburi/zephyr/tree/pr72050 https://github.com/soburi/zephyr/commit/631266cbb26e4c5e083b71898d8cd9b29af62cc5
This may cause some controversy, but I think it's a necessary expansion.
One idea is to extend the script and do the calculations inside Kconfig.
@soburi Thank you for the two commits, this looks like a fine solution to the problem. I took the liberty to cherry pick them and integrated them with the other changes in the pull request. Let me know what you think.
I don't have the datasheets of the nRF54 series so currently I can't tell yet if this driver is compatible with these SoC's as well (probably are).
@soburi any news on your pull request https://github.com/zephyrproject-rtos/zephyr/pull/73489 ?
@soburi any news on your pull request #73489 ?
I have no response at this time. They are perhaps busy preparing release 3.7... 🤔
@simonguinot @soburi I made an attempt to implement the solution suggested. Please let me know what you think.
@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.
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! 🪁