light_ws2812

[david-vankampen]

Contribution description

Adding 3rd party lib reference of light_ws2812, and example that leverages it

Testing procedure

if you hook up a ws2811 or ws2812 LED array to the pin specified with the arguments

CFLAGS += -DLIGHT_WS2812_GPIO_PIN=7
CFLAGS += -DLIGHT_WS2812_GPIO_PORT=GPIOA

you should see a random pattern being played across them when you flash the included example.

[maribu]

I think this PKG could be integrated into the existing ws281x driver as an alternative backend. See https://api.riot-os.org/group__drivers__ws281x.html

[david-vankampen]

I think this PKG could be integrated into the existing ws281x driver as an alternative backend. See https://api.riot-os.org/group__drivers__ws281x.html

@maribu to be clear, are you thinking keep the package, and also add a reference to it in the ws281x driver? or eliminate adding it as a standalone package and ONLY update the driver?

[maribu]

The current ws281x driver requires a you to select a backend (e.g. ws281x_atmega for an AVR ASM implementation). There are currently not many backends upstream yet, see e.g. https://forum.riot-os.org/t/about-how-to-interface-ws2812s-rgb-leds-with-nrf-based-board for some discussion about WIP backends. The benefit of that approach is that you can develop your RIOT application for board A, but later switch to board B for some unrelated reasons and still be able to use the same code unmodified. Even if you previously used e.g. the I2S peripheral to perform the bitbanging, or ATmega CPU cycle counting, or the SPI MOSI pin and later switch to the RP2040 PIO state machine and DMA for bitbanging.

If this package gets added as a backend, it would benefit from the existing test application and existing users could directly make use of it. If it is added with a different API, users will have a hard time switching between different modes of generating the ws281x signal.

[david-vankampen]

That makes sense. So I think I will leave it in as a pkg, but then add the needful references to that package to the ws281x driver. That way if a developer wants to go as-slim-as-possible they can just use the package directly, or they can use the driver as you detailed. Does that sound good?

[david-vankampen]

@maribu please see the most recent commit. I verified it builds via the following operation:

cd tests/driver_ws281x
CFLAGS="-DLIGHT_WS2812_UC_STM32G0XX -DLIGHT_WS2812_GPIO_PIN=7 -DLIGHT_WS2812_GPIO_PORT=GPIOA -DCONFIG_USE_CLOCK_HSI=1" make BOARD=nucleo-g070rb

[maribu]

Sorry for being so slow to reply. I currently have a lot of other things to tend to :-/

I have some comments inline, mostly minor style nitpicks. However, the package currently only requires the features periph_gpio, which results in the build system assuming it to be compatible with all boards that have a GPIO driver (which I believe each and every board has). Consequently, the CI will try to build it also for e.g. RISC-V boards. In addition, the package is not compatible with all Cortex-M boards, see the long inline comment for more details.

The feature requirements need to be narrowed down so that make info-boards-supported will indeed only list actually supported boards. That way, the CI will pass and users with incompatible boards will get a build time error with a message naming the missing feature.

[maribu]

Suggested change

[maribu]

Suggested change

	while(1) {
	while (1) {

[maribu]

Suggested change

	for (uint8_t idx = 0; idx < NUM_LEDS; idx++) {
	leds[idx] = (uint8_t)random_uint32();
	}
	random_bytes(leds, sizeof(leds));

[maribu]

Suggested change

	uint8_t leds[NUM_LEDS] = { 0x00};
	static uint8_t leds[3 * NUM_LEDS] = { 0x00 };

Shouldn't there be three bytes per LED?

[david-vankampen]

Ah, yes you're right. Good catch.

[maribu]

Suggested change

	USEMODULE += ws281x_arm
	USEMODULE += ws281x_light_ws2812

Maybe the module could be named after the pkg that provides the implementation? After all, light_ws2812 also contains an implementation for AVR MCUs, so one could use it there as well. (Even though I think that at least one would have to change the #include "light_ws2812_cortex.h" with some preprocessor magic to pick the header corresponding to the arch.)

[maribu]

Cool. GitHub swallowed my comment regarding the features required ☹️

My point was that the package has some requirements, namely:

1. ws2812_port_set and ws2812_port_clr needs to be defined for the MCU family used
2. The CPU architecture has to be ARM Cortex-M (or AVR, but that would require some work on the package that IMO could still be done later on, if somebody wants to use light_ws2812 on AVR)
3. The timing behavior of the CPU needs to match the assumptions the inline assembly is making

Some background regarding the timing behavior: The AVR backend that I implemented uses CPU cycle counting in very much the same way the light_ws2812 package does. I tried the same on my STM32F767ZI MCU (but rather using a delay loop than nops) and figured out that the CPU occasionally took 1 CPU cycle for each two-instruction loop iteration, but sometimes a dozen cycles or so. The CPU has a dual-issue feature that allows it to perform two CPU instructions in a single CPU cycle under some conditions, and a long pipeline that needs to be flushed on unexpected branches. So while the dynamic branch predictor incorrectly predicted the branch as not being taken, the two instruction loop took ages per iteration. But once the predictor got it right, it went down to a single cycle. Even though the delay code in light_ws2812 doesn't use a delay loop, I'm still almost certain that the dual-issue feature can run two nops in a single CPU cycle and that whether or not the instruction cache is hot will have a huge impact on the timing. So, I would be extremely surprised if the package would work with any Cortex-M7 CPU.

My suggestion is to introduce a new feature, e.g. named light_ws2812, that will be provided by the Cortex-M MCU families that match the timing assumptions and for which ws2812_port_set and ws2812_port_clr are defined.

[maribu]

I wonder if it would make sense to provide an light_ws2812_params.h e.g. similar to https://github.com/RIOT-OS/RIOT/blob/master/drivers/sht3x/include/sht3x_params.h

The main advantage would be that there would always be a valid fall-back configuration (so that the CI can compile applications using the pkg) and the macros could be documented there and would pop up in Doxygen. An application could provide its own light_ws2812_params.h in the application folder (the build system makes sure that the application folder is searched first for header files, so that one can rely on their own params overwriting the defaults).

[maribu]

This seems to need rebase now. Please ping me, when I should take another look.

[Teufelchen1]

@david-vankampen ping :)

[david-vankampen]

@maribu rebased

[maribu]

OK, I took a closer look at this PR. Right now, it will only add support for STM32F0 and STM32G0 MCU families. It would require to add and maintain glue code to allow accessing the GPIO registers for setting and clearing GPIOs for each new CPU family. This would also be largely a duplicate effort compared to the GPIO LL API, which also exposes access to the GPIO set and clear registers. However, the GPIO LL API provides the gpio_ll_set() and gpio_ll_clear() functions rather than direct memory access, so that MCU families that do not have set and clear registers can emulate this. (This also includes some STM32 families, which do not have a clear register, but only a "set and clear" register where the lower 16 bits set and the upper 16 bits clear GPIO pins.)

I think that #19891 is the better approach and would work directly for all STM32 families, if the STM32 timer driver would have the periph_timer_poll extension implemented.

I'll try to give a periph_timer_poll driver a shot soonish. It should not be much of a deal to implement this, but I have not found time for that so far.

Also note that there is #20218 as well, which (ab)uses SPI and DMA for bit-banging. That is IMO even better than periph_timer_poll, but requires a DMA peripheral and a free SPI peripheral. IMO we should aim to have those two options for STM32 upstream, so that when no DMA is available or no SPI bus can be spared, the timer variant could be used instead.

[maribu]

Would #20646 be a viable alternative for you?