Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/ws281x: Add gpio_ll and timer based driver #19891

Merged
merged 8 commits into from
Jan 3, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Aug 17, 2023

Contribution description

This adds a driver for WS281x style LEDs (tested here with SK6812 RGBW), which is based on gpio_ll and timers (with a small addition).

Review-wise, I'd ask for this to be reviewed as a whole. PR-wise (for change log etc) it may make sense to factor out the timer additions into a second PR that's then fast-tracked and only contains the first commits, but this is all better understood as a whole.

Changes are:

  • Fixes to existing timers. (Missing define, and a NULL callback made nRF5x crash).
  • Adds the periph_timer_poll feature on periph_timer -- when present, we get a new timer_poll_channel inline function, which checks whether a counter has been reached (even when interrupts are off, or when there is no handler for this timer).
  • Implements the bit-banging protocol. We have a rather tight time budget, with a single tick of the 16MHz counter making the difference on whether or not a one is actually read as a 1, but there'd some time to spare if we didn't choose the timing parameters this tightly. (But when doing the rounding properly, I didn't need to alter the existing nanosecond times).

There is a general alternative: Using SPI, PWM or really any other suitable general peripheral. But this works (albeit blockingly), and should be somewhat portable (although I wouldn't know what to test it on), and can still serve as a reference when doing anything more fancy.

Testing procedure

  • Take any nRF52 device (probably nRF5x), and wire up WS281x style LEDs.
  • I wired up SK6812 RGBW, so I had to apply this extra:
diff --git a/drivers/include/ws281x.h b/drivers/include/ws281x.h
index fe448ee198..14b2b02b6c 100644
--- a/drivers/include/ws281x.h
+++ b/drivers/include/ws281x.h
@@ -104 +104 @@ extern "C" {
-#define WS281X_BYTES_PER_DEVICE       (3U)
+#define WS281X_BYTES_PER_DEVICE       (4U)
  • Pick a pin, a number of LEDs and a timer for the configuration. (If a board were wired with LEDs, that'd be part of the board config). For me, I had a particle-xenon and attached to the bottom right pin (SDA), so I applied:
diff --git a/tests/drivers/ws281x/Makefile b/tests/drivers/ws281x/Makefile
index a8def79895..aa01ed28e9 100644
--- a/tests/drivers/ws281x/Makefile
+++ b/tests/drivers/ws281x/Makefile
@@ -5,2 +5,2 @@ include ../Makefile.drivers_common
-# PIN ?= GPIO_PIN(0, 0)
-# N ?= 8
+PIN ?= GPIO_PIN(0, 26)
+N ?= 8
@@ -9,2 +9,2 @@ include ../Makefile.drivers_common
-# TIMER ?= 2
-# FREQ ?= 16000000
+TIMER ?= 2
+FREQ ?= 16000000
  • make -C tests/drivers/ws281x BOARD=particle-xenon all flash term PARTICLE_MONOFIRMWARE=1 or whatever your board needs
  • Watch it show funny rainbows, and have fun until your power supply says that no to all the LEDs lighting up (I had a full strand connected, and the example lit up all of it while it lasted). Or turn down the 100 rainbows in tests/drivers/ws281x/main.c

Issues/PRs references

Closes: #19859

This might also be a band-aid for #19158 -- but given there is that neat PIO, that's probably the way to go.

@chrysn chrysn requested a review from maribu August 17, 2023 15:35
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Aug 17, 2023
@chrysn chrysn added Area: timers Area: timer subsystems and removed Area: boards Area: Board ports labels Aug 17, 2023
@github-actions github-actions bot added Area: boards Area: Board ports and removed Area: timers Area: timer subsystems labels Aug 17, 2023
@chrysn chrysn added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: timers Area: timer subsystems and removed Area: boards Area: Board ports labels Aug 17, 2023
@riot-ci
Copy link

riot-ci commented Aug 17, 2023

Murdock results

✔️ PASSED

6a0b1c5 boards/nrf9160dk: Override WS281X_TIMER_*

Success Failures Total Runtime
138167 0 138167 02h:32m:55s

Artifacts

@chrysn chrysn requested a review from jia200x as a code owner August 17, 2023 18:35
@github-actions github-actions bot added Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration and removed Area: timers Area: timer subsystems labels Aug 17, 2023
@jimporter
Copy link
Contributor

jimporter commented Aug 18, 2023

Coming from #19859, I tested this out on an Adafruit ItsyBitsy nRF52840 with an 8-LED strip.

It works sometimes (if I just set the colors once instead of in a loop) but overall it's pretty glitchy for me. Seems like timing issues, but I could be doing something wrong of course. To keep things as simple as possible, I used the following test program, which should just repeatedly set all the LEDs to red, but instead I get a random strobe effect:

Makefile

USEMODULE += ws281x
USEMODULE += xtimer

TIMER = 2
FREQ = 16000000

# ...

main.c

int main(void) {
  // NeoPixel strip
  ws281x_t neopixel;
  uint8_t neo_state[8];
  ws281x_params_t neo_params = {
    .buf = neo_state,
    .numof = 8,
    .pin = GPIO_PIN(0, 27)
  };
  if(ws281x_init(&neopixel, &neo_params) != 0) {
    printf("failed to initialize NeoPixel\n");
    return 3;
  }

  xtimer_ticks32_t last_wakeup = xtimer_now();
  while(true) {
    color_rgb_t neo_color = {255, 0, 0};
    for(int i = 0; i < 8; i++)
      ws281x_set(&neopixel, i, neo_color);
    ws281x_write(&neopixel);
    xtimer_periodic_wakeup(&last_wakeup, 100 * US_PER_MS);
  }

  return 0;
}

@chrysn
Copy link
Member Author

chrysn commented Aug 18, 2023 via email

@MrKevinWeiss MrKevinWeiss added CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2023
@MrKevinWeiss
Copy link
Contributor

Hmmm I wonder why of the no fail fast label has an issue with the merge queues

This never worked this way. It only affects the "quick build" prior to entering the merge queue / bors. You can add a CI: full build to turn the quick build into a full build, though.

You could also hack in a quick change to only build the ws281x test app in the quick build, but for all boards. That should cover anything that I would expect to fail here. If this quick build passed, the CI: full build and the hack commit could be dropped again.

True true, how we used to do it. I will just let it run in the night I suppose and save a commit drop.

* depending on the precise low and high times. A value of 16MHz works well.
* */
#ifndef WS281X_TIMER_FREQ
#define WS281X_TIMER_FREQ 16000000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define WS281X_TIMER_FREQ 16000000
#define WS281X_TIMER_FREQ MHZ(16)

reads a bit nicer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like using the MHZ macro causes some conflicts with the ESP so I think it best to just leave it out.

@chrysn chrysn requested a review from kaspar030 as a code owner December 18, 2023 09:22
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Dec 18, 2023
@MrKevinWeiss
Copy link
Contributor

The tests are passing (at least for the driver). Maybe @chrysn and @benpicco can see if they agree then I would squash and merge.

chrysn and others added 8 commits December 19, 2023 11:13
timer_set has no documented restriction on this being not null, other
implementations explicitly tolerate it (rpx0xx checks inside the ISR,
but doing it at init time keeps the ISR slim).

This is useful when using a timer just to read, without any action when
it triggers (the action is taken depending on read values, eg. in a
thread context).
The macro's presence is documented in `timer_init`, but was missing from
this platform.
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Dec 19, 2023
@maribu maribu added this pull request to the merge queue Jan 3, 2024
Merged via the queue into RIOT-OS:master with commit f860d96 Jan 3, 2024
25 checks passed
@MrKevinWeiss
Copy link
Contributor

Nice, thanks for the contribution and reviews! Our feature-nrf52840-sense board it becoming more powerful every day!

@chrysn
Copy link
Member Author

chrysn commented Jan 4, 2024

Thank you all for pushing this over the finish line!

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
@MrKevinWeiss MrKevinWeiss deleted the ws2181x_timer branch April 8, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for NRF52 to the WS281x driver
6 participants