Skip to content

drivers/ws281x: add SPI backend#22003

Merged
crasbe merged 4 commits intoRIOT-OS:masterfrom
fabian18:pr/ws281x_spi
Jan 30, 2026
Merged

drivers/ws281x: add SPI backend#22003
crasbe merged 4 commits intoRIOT-OS:masterfrom
fabian18:pr/ws281x_spi

Conversation

@fabian18
Copy link
Copy Markdown
Contributor

@fabian18 fabian18 commented Jan 19, 2026

Contribution description

SPI is a well known technique to drive the ws281x LED strip.
This PR add this as a possible backend.
There is unfortunately a glitch that pulls MOSI high on spi_acquire which is falsely interpreted as a 1.
Screenshot from 2026-01-17 18-36-03
This can be googled as the "phantom bit problem" or the "first LED always green problem".
The trick is to generate a RESET condition so that all LEDs get clean data afterwards.

Testing procedure

It works very reliable for me.
USEMODULE+=ws281x_spi make -C tests/drivers/ws281x/ -f Makefile.same54-xpro.mk flash

BOARD := same54-xpro

CFLAGS += -DWS281X_BYTES_PER_DEVICE=4
CFLAGS += -DWS281X_PARAM_PIN="GPIO_PIN(PB, 27)"
CFLAGS += -DWS281X_PARAM_PIN_IN="GPIO_PIN(PB, 29)"
CFLAGS += -DWS281X_PARAM_NUMOF=144

USEMODULE += ws281x_spi
ifneq (,$(filter ws281x_spi,$(USEMODULE)))
  CFLAGS += -DWS281X_SPI_DEV="SPI_DEV(0)"
  CFLAGS += -DWS281X_SPI_CLK="KHZ(3200)"
  # exact timing for sk6812
  CFLAGS += -DWS281X_T_DATA_ONE_NS=600
  CFLAGS += -DWS281X_T_DATA_ZERO_NS=300
endif

include Makefile

Issues/PRs references

There was a previous PR #20218. The author is added in the implementation.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers Area: cpu Area: CPU/MCU ports labels Jan 19, 2026
@fabian18
Copy link
Copy Markdown
Contributor Author

fabian18 commented Jan 19, 2026

IMG_3124.mp4

@crasbe crasbe 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 labels Jan 19, 2026
crasbe
crasbe reviewed Jan 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

Some style comments, I haven't tested it yet.

@riot-ci
Copy link
Copy Markdown

riot-ci commented Jan 19, 2026 •
edited
Loading

Murdock results

✔️ PASSED

38bd754 drivers/ws281x: remove units.h due to redefinition in ESP vendor header

Success Failures Total Runtime
11002 0 11004 09m:29s

Artifacts

benpicco
benpicco reviewed Jan 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good, just some nits

benpicco
benpicco reviewed Jan 20, 2026
View reviewed changes
benpicco
benpicco reviewed Jan 20, 2026
View reviewed changes
benpicco
benpicco reviewed Jan 20, 2026
View reviewed changes
@fabian18
Copy link
Copy Markdown
Contributor Author

fabian18 commented Jan 21, 2026 

In file included from esp32.c:38:
../../build/pkg/esp32_sdk/components/esp_hw_support/port/esp32/include/soc/rtc.h:53:9: error: "MHZ" redefined [-Werror]
   53 | #define MHZ (1000000)
      |         ^~~
In file included from include/ws281x_params.h:24,
                 from esp32.c:33:
../../core/lib/include/macros/units.h:45:9: note: this is the location of the previous definition
   45 | #define MHZ(x) (KHZ(x) * 1000UL)
      |         ^~~

mhm 🙄

@fabian18
Copy link
Copy Markdown
Contributor Author

fabian18 commented Jan 21, 2026

Can I simply guard our macros in units.h?

@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Jan 21, 2026

Can I simply guard our macros in units.h?

That would cause other issues because the ESP32 definition is not a function. So writing MHZ(12) would cause issues because the preprocessor would resolve that to 1000(12), which is a syntax error.

I think we had this issue before somewhere but I don't remember the solution.

@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Jan 21, 2026

Can I simply guard our macros in units.h?

That would cause other issues because the ESP32 definition is not a function. So writing MHZ(12) would cause issues because the preprocessor would resolve that to 1000(12), which is a syntax error.

I think we had this issue before somewhere but I don't remember the solution.

#21522 (comment)

The solution there was also not to use units.h. Quite annoying.

@fabian18 fabian18 force-pushed the pr/ws281x_spi branch 2 times, most recently from aecfce7 to 08f705c Compare January 23, 2026 13:17
benpicco
benpicco approved these changes Jan 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

looks good to me

@maribu maribu added this pull request to the merge queue Jan 28, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2026
@fabian18
Copy link
Copy Markdown
Contributor Author

fabian18 commented Jan 29, 2026 

Building application "senml_saul_example" for "adafruit-feather-nrf52840-express" with CPU "nrf52".

Cloning into '/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/nrf5x_nrfx_mdk/nrf52'...
done.
HEAD is now at 11f57e5 nrfx 3.14.0 release
Cloning into '/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/pkg/cmsis'...
done.
HEAD is now at 2b7495b85 Merge branch 'develop'
Cloning into '/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/pkg/mpaland-printf'...
done.
HEAD is now at 0dd4b64 test(test_suite): added support for PRINTF_DISABLE_SUPPORT_EXPONENTIAL
Cloning into '/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/pkg/nanocbor'...
done.
HEAD is now at 3e37004 Merge pull request #94 from slegouix/master
/opt/gcc-arm-none-eabi-10.3-2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/../../../../arm-none-eabi/bin/ld: /tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/ws281x/ws281x_saul.o: in function `set_rgb_led':
ws281x_saul.c:(.text.set_rgb_led+0x28): undefined reference to `_xtimer_tsleep'
collect2: error: ld returned 1 exit status
make: *** [/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/examples/advanced/senml_saul/../../../Makefile.include:734: /tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/senml_saul_example.elf] Error 1
make: Leaving directory '/tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/examples/advanced/senml_saul'
cat: /tmp/dwq.0.4752955123436924/14f39206fe38e44d7a2ff4513d5436ed/build/test-input-hash.sha1: No such file or directory
{"build/": 19276}

Seems like I have to keep the xtimer dependency.

@fabian18 fabian18 added this pull request to the merge queue Jan 29, 2026
@crasbe crasbe mentioned this pull request Jan 29, 2026
Closed
@crasbe crasbe removed this pull request from the merge queue due to a manual request Jan 29, 2026
crasbe
crasbe previously requested changes Jan 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

See https://github.com/RIOT-OS/RIOT/pull/22003/changes#r2741828124.

@crasbe crasbe dismissed their stale review January 29, 2026 15:55

Changes were applied.

@crasbe crasbe enabled auto-merge January 29, 2026 15:55
@crasbe crasbe added this pull request to the merge queue Jan 29, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2026
@crasbe crasbe added this pull request to the merge queue Jan 29, 2026
Merged via the queue into RIOT-OS:master with commit b253930 Jan 30, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@crasbe crasbe crasbe left review comments

@maribu maribu Awaiting requested review from maribu maribu is a code owner

@dylad dylad Awaiting requested review from dylad dylad is a code owner

@keestux keestux Awaiting requested review from keestux keestux is a code owner

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fabian18 @riot-ci @crasbe @benpicco @maribu