drivers: Added WS281x RGB LED driver for ATmega platform by maribu · Pull Request #12693 · RIOT-OS/RIOT

maribu · 2019-11-11T19:54:40Z

Contribution description

This PR provides a driver to use the WS2812/SK6812 RGB LEDs (a.k.a. NeoPixels).

This PR contains a platform independent and a platform specific code, as sadly the protocol to access the LEDs is a custom single wire protocol with to rigid timing requirements to allow using xtimer and periph_gpio. In this PR the platform specific part is only provided for ATmega based platforms, but the infrastructure to add support for more platforms is in place.

Testing procedure

Hook up chain of one or more WS2812 or SK6812 LEDs to an ATmega clocked at either 8 MHz or 16 MHz and run the test application. See the documentation on limitations of the 8 MHz version.

Issues/PRs references

PR #12020 will need to be rebased on top of this.

benpicco · 2019-11-11T23:00:48Z

Tried it on the microduino-corerf, works like a charm! (I used PE4, chosen at random)

What I especially like about this solution is that it leaves the door open for e.g. that DMA based I2S implementation on esp8266/esp32, the ABC backend or other hardware specific implementations.

I'd suggest to call the driver ws281x since NeoPixel is just one vendor of those chips.

@herjulf I also tried it on the avr-rss2 with the ADC2 connector (PF2) but it did not work - is there some capacitor in the way?

When I use the SDA pin (PD1) it works just fine - I just thought I could make use of those neat screw terminals.

(Also works with N=300)

drivers/neopixel/atmega.c

benpicco · 2019-11-12T11:17:58Z

BTW: The timing of those LEDs in not so bad, it's even possible to drive them using SPI.
Now that would be a good motivation to implement DMA based SPI transfers so some more complex animations can be done.

But that would be just another backend for this driver.

maribu · 2019-11-12T12:36:18Z

BTW: The timing of those LEDs in not so bad

I actually used this already. (The Arduino lib referred to in the link unrolls the loop manually and did everything in assembly to strictly comply with the timings.)

But having confirmed that the low phase could be pretty long as long it is significantly shorter than the 50µs end of sequence signal, I might add a ws281x_write_chunk() function to allow users to drive huge chains without allocating huge buffers.

I'll update the name as suggested, but I cannot work on this during work time, so it has to wait a bit 😉

cpu/atmega_common/include/atmega_gpio.h

herjulf · 2019-11-12T17:13:42Z

@benpicco Yes the exposed AD ports has voltage dividers and and a small capacitor to reduce AD noise. Also an diode to protect from reverse voltage. Board has do handle ESD & immunity etc for CE. So you did the right thing taking one unprotected pin. The screw terminals was no success they were removed in the 2.4 rev.

Moved macros and static inline helper functions needed to access ATmega GPIOs to cpu/atmega_common/include/atmega_gpio.h in order to reuse them for the platform specific low level part of the Neopixel driver.

maribu · 2019-11-12T19:20:28Z

OK. Doing the rename was to much mess to handle via fixup commits. I squash therefore and updated the commit messages to the new name.

maribu · 2019-11-12T20:10:12Z

@benpicco: I now added low level functions (in addition to the current API) to allow users to use small buffers for huge LED chains. The high level API is a syntactic sugar static inline wrapper API on top of the low level API, so there shouldn't be much overhead when using the high level API.

tests/driver_ws281x/main.c

tests/driver_ws281x/Makefile

benpicco · 2019-11-15T23:52:23Z

I now added low level functions (in addition to the current API) to allow users to use small buffers for huge LED chains.

Can you provide an example of it's use in tests/driver_ws281x?

maribu · 2019-11-16T07:47:12Z

Can you provide an example of it's use in tests/driver_ws281x?

Sure, but I don't habe enough LEDs (in a row) to actually see the test result. But I guess you have?

maribu · 2019-11-16T11:09:30Z

I now added low level functions (in addition to the current API) to allow users to use small buffers for huge LED chains.

Can you provide an example of it's use in tests/driver_ws281x?

Done. With my chain of 10 LEDs, I can see part of the second rainbow being displayed. I'm optimistic that people with long enough chains will be able to see 100 rainbows.

May I squash?

benpicco · 2019-11-16T13:00:39Z

Hm, after applying 767f044 I only get

2019-11-16 13:59:47,439 # Animation: Moving rainbow...
2019-11-16 13:59:50,982 # Animation: Fading rainbow...
2019-11-16 13:59:51,021 # *** RIOT kernel panic:
2019-11-16 13:59:51,022 # 
2019-11-16 13:59:51,022 # 
2019-11-16 13:59:51,029 # *** halted.
2019-11-16 13:59:51,029 #

maribu · 2019-11-16T13:10:25Z

Can you add CFLAGS += -DDEBUG_ASSERT_VERBOSE to the Makefile to confirm a theory I have for the cause?

benpicco · 2019-11-16T13:19:20Z

I don't get any more output than that, even with CFLAGS += -DDEBUG_ASSERT_VERBOSE.
The LEDs don't update either.

maribu · 2019-11-16T13:20:27Z

Strange. It also doesn't work for me when I build with Ubuntu's old toolchain using BUILD_IN_DOCKER. But I do not even get debug output.

maribu · 2019-11-16T13:26:50Z

@benpicco: Does it work before the change to xtimer_periodic_wakeup()?

maribu · 2019-11-16T18:13:29Z

Let me extend the doc a bit to point out how backends are selected.

benpicco · 2019-11-16T18:43:59Z

tests/driver_ws281x/Makefile

-
-FEATURES_REQUIRED := arch_avr8 # Currently only a backend for AVR available
+USEMODULE += ws281x # <-- Main platform independent driver
+USEMODULE += ws281x_atmega # <-- Backend used to communicate with the LEDs


Can we resolve this in Makefile.dep?

I was more thinking of

ifneq (,$(filter ws281x,$(USEMODULE))) ifneq (,$(filter arch_avr8,$(FEATURES_PROVIDED))) USEMODULE += ws281x_atmega else $(error No ws281x backend available for the current platform) endif endif

Travis says that modules must never inspect FEASTURES_PROVIDED. I dropped this to comply

Does it also complain when you put this into drivers/Makefile.dep?

Hm that is silly.
How about ifneq (,$(filter atmega_common,$(USEMODULE)))?

This was also my first idea. But then I figured this is actually the same, but less elegant and readable. And being honest, it would be only to weasel around the check.

I'm not sure what the check wants to prevent. Maybe it wants to catch cases were FEATURES_REQUIRED should have been used, but the contributor was unaware and added a manual check of FEATURES_PROVIDED instead?

But until I know the actual reason, I thought it is better to back off. If my guess is correct, that message would be a false positive in the given case and the check could be improved. Or maybe there is a good reason why my first approach isn't a good idea that I'm just not aware of.

When reading up on #10179 it seems like the original intention was that modules should rely on FEATURES_USED instead of FEATURES_PROVIDED, because just because a feature (e.g. SPI) is available doesn't mean that it's being used.

Adding the arch to FEATURES_PROVIDED happened later, but the arch is never 'used'.

So using ifneq (,$(filter atmega_common,$(USEMODULE))) would not be against the spirit of that check, but it would make using the driver much more user friendly.

I want to add an SDL backend for native so creating animations becomes easier to test - I just want to change the BOARD for that, not the Makefile.

How about this?

Yes this will work!

herjulf · 2019-11-16T19:33:29Z

That's the travis problem I have with avr-rss2 PR as well.
Got runs through Travis. Suddenly I got this a well.
Can you retrigger a Travis check w/o adding a new commit in the PR?

benpicco · 2019-11-17T13:16:07Z

That's the travis problem I have with avr-rss2 PR as well.

Do you mean

Pull request needs squashing

That's normal. Once the PR is ACKed the author must squash the fixup commits. This check prevents accidental merging of un-squashed commits.

benpicco · 2019-11-17T23:47:11Z

@maribu looks good now!
Please squash.

benpicco

Code looks good and works well on arduino-mega2560.

benpicco · 2019-11-18T00:07:51Z

drivers/include/ws281x.h

+static inline void ws281x_end_transmission(void)
+{
+    xtimer_usleep(WS281X_T_END_US);
+}


I'm afraid I'll have to change this to

#ifdef MODULE_WS281X_ATMEGA static inline void ws281x_end_transmission(ws281x_t *dev) { (void) dev; xtimer_usleep(WS281X_T_END_US); } #else void ws281x_end_transmission(ws281x_t *dev); #endif

when adding another backend.

drivers/Makefile.dep

benpicco · 2019-11-18T00:32:10Z

drivers/ws281x/ws281x.c

+/* Default buffer used in ws281x_params.h. Will be optimized out if unused */
+uint8_t ws281x_buf[WS281X_PARAM_NUMOF * WS281X_BYTES_PER_DEVICE];
+
+int ws281x_init(ws281x_t *dev, const ws281x_params_t *params)


I just noticed that a backend-specific ws281x_init() will be necessary.

But I guess this should work for most backends. E.g. the GPIO ABC backend will also use this. And I guess for other slow platforms (e.g. MSP430) manual bitbanging with CPU cycle counting will also be the easiest path. I simple marked the symbol as weak to allow other backends (e.g. the SPI one) to provide their own.

benpicco · 2019-11-18T00:35:02Z

drivers/ws281x/ws281x.c

+    if (gpio_init(dev->params.pin, GPIO_OUT)) {
+        return -EIO;
+    }


better move this to atmega.c into some ws281x_init_backend(ws281x_t *dev) function.

benpicco · 2019-11-18T02:19:32Z

Hm turns out SDL is not as straightforward as I thought with all that syscall magic native is doing.
SDL_Init() always crashes in _XlcResolveLocaleName().

But who needs graphical output anyway?
This is the next best thing:

#include <stdio.h>
#include "ws281x.h"

void ws281x_write_buffer(ws281x_t *dev, const void *buf, size_t size)
{
    (void) dev;
    const uint8_t *src = buf;

    for (unsigned i = 0; i < size; ++i) {
        int r = src[WS281X_BYTES_PER_DEVICE * i + WS281X_OFFSET_R];
        int g = src[WS281X_BYTES_PER_DEVICE * i + WS281X_OFFSET_G];
        int b = src[WS281X_BYTES_PER_DEVICE * i + WS281X_OFFSET_B];
        printf("\033[48;2;%d;%d;%dm ", r, g, b);
    }
}

void ws281x_end_transmission(ws281x_t *dev)
{
    (void) dev;
    puts("\033[0m");
}

maribu · 2019-11-18T15:27:49Z

@benpicco: I adapted the ws281x_end_transmission() to expect a device and also provided an ws281x_prepare_transmission() that will come in handy for e.g. the SPI backend. With that, it should now be easier to add backends not using GPIOs and CPU cycle counting.

drivers/ws281x/ws281x.c

benpicco · 2019-11-18T15:41:39Z

Yes that will work!
You can squash again.

You can squash - feel free to remove that superfluous memset() while you're at it.

Added driver for the WS2812/SK6812 RGB LEDs often sold as NeoPixels, which due to their integrated RGB controller can be chained to arbitrary length and controlled with a single GPIO.

maribu · 2019-11-18T22:16:56Z

Squashed and both Murdock and Travis didn't find anything to complain :-)

maribu · 2019-11-19T07:57:32Z

@miri64: Can you confirm I addressed your comments?

maribu · 2019-11-21T20:39:40Z

@miri64: Ping?

Sorry, yes! My comments were addressed.

maribu · 2019-11-22T22:15:58Z

@benpicco, @miri64: Thanks for your reviews!

gschorcht · 2020-02-01T08:11:48Z

cpu/atmega_common/include/atmega_gpio.h

+    port_addr += ATMEGA_GPIO_BASE_PORT_A;
+    port_addr += ATMEGA_GPIO_OFFSET_PIN_PORT;
+
+#if defined (PORTG)


@maribu When I changed the ATmega implementation for the new GPIO API, I wondered if this should be:

- #if defined (PORTG) + #if defined (PORTH)

The additional offset is only required for ports starting from H. That is, a MCU which has port G but not port H such as ATmega128 doesn't require it.

Yes, indeed. (To my defense, while git blame will show me as author, I only moved the code to a different file without touching or authoring it.)

Yeah I know, you just exposed it from cpu/atmega_common/periph/gpio.c.

maribu requested a review from benpicco November 11, 2019 19:54

maribu added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 11, 2019

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 11, 2019

benpicco reviewed Nov 11, 2019

View reviewed changes

drivers/neopixel/atmega.c Show resolved Hide resolved

miri64 previously requested changes Nov 12, 2019

View reviewed changes

cpu/atmega_common/include/atmega_gpio.h Outdated Show resolved Hide resolved

cpu/atmega_common/include/atmega_gpio.h Outdated Show resolved Hide resolved

cpu/atmega_common: Restructured code

ba26aed

Moved macros and static inline helper functions needed to access ATmega GPIOs to cpu/atmega_common/include/atmega_gpio.h in order to reuse them for the platform specific low level part of the Neopixel driver.

maribu force-pushed the neopixel-atmega branch from 613aefb to 7154413 Compare November 12, 2019 19:19

maribu changed the title ~~drivers: Added NeoPixel (WS2812) driver for ATmega platform~~ drivers: Added WS281x RGB LED driver for ATmega platform Nov 12, 2019

benpicco reviewed Nov 15, 2019

View reviewed changes

tests/driver_ws281x/main.c Outdated Show resolved Hide resolved

benpicco reviewed Nov 15, 2019

View reviewed changes

tests/driver_ws281x/Makefile Outdated Show resolved Hide resolved

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 16, 2019

benpicco reviewed Nov 16, 2019

View reviewed changes

maribu force-pushed the neopixel-atmega branch from 0876fdf to 845df85 Compare November 17, 2019 23:48

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 17, 2019

benpicco approved these changes Nov 17, 2019

View reviewed changes

benpicco reviewed Nov 18, 2019

View reviewed changes

drivers/Makefile.dep Show resolved Hide resolved

benpicco reviewed Nov 18, 2019

View reviewed changes

drivers/Makefile.dep Show resolved Hide resolved

benpicco reviewed Nov 18, 2019

View reviewed changes

drivers/ws281x/ws281x.c Show resolved Hide resolved

maribu added 3 commits November 18, 2019 22:45

drivers/ws281x: Added driver for RGB LEDs

1ed1906

Added driver for the WS2812/SK6812 RGB LEDs often sold as NeoPixels, which due to their integrated RGB controller can be chained to arbitrary length and controlled with a single GPIO.

tests: Added test for the ws281x driver

74db500

cpu/atmega_common/periph: Fixed typo in gpio.c

8d0a9ea

maribu force-pushed the neopixel-atmega branch from 49f2f52 to 8d0a9ea Compare November 18, 2019 21:45

benpicco merged commit 09f647e into RIOT-OS:master Nov 22, 2019

maribu deleted the neopixel-atmega branch November 22, 2019 22:15

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019

gschorcht reviewed Feb 1, 2020

View reviewed changes

Conversation

maribu commented Nov 11, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

benpicco commented Nov 12, 2019

Uh oh!

maribu commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

herjulf commented Nov 12, 2019

Uh oh!

maribu commented Nov 12, 2019

Uh oh!

maribu commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

benpicco commented Nov 15, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

benpicco commented Nov 16, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

benpicco commented Nov 16, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

maribu commented Nov 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco Nov 16, 2019 • 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!

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!

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!

herjulf commented Nov 16, 2019

Uh oh!

benpicco commented Nov 17, 2019

Uh oh!

benpicco commented Nov 17, 2019

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Nov 18, 2019 • 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!

Uh oh!

Choose a reason for hiding this comment

benpicco commented Nov 11, 2019 •

edited

Loading

benpicco Nov 16, 2019 •

edited

Loading

benpicco Nov 18, 2019 •

edited

Loading

benpicco Nov 18, 2019 •

edited

Loading