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: esp32 neopixel driver to set the rmt symbol high/low lengths at init instead of every write #21068

Merged

Conversation

IsikcanYilmaz
Copy link
Contributor

@IsikcanYilmaz IsikcanYilmaz commented Dec 8, 2024

Contribution description

Minor change to the WS281x driver, specifically the esp32 side.

The ws281x driver for the esp32 uses a hardware module called the RMT, which generates the data signal that the ws281xs expect. The RMT module is supplied with the information about how to generate this signal; for a 1 symbol, how long should the signal be held high, how long should it be low, so on. These 1 and 0 symbol high/low times are recalculated at every RMT write.

This PR does the calculation once at init. Marginal speed up (~10us on my 80 neopixel strip).

If there was a reason for doing it at every write, it was not obvious to me. Please let me know if so.

@IsikcanYilmaz IsikcanYilmaz requested a review from maribu as a code owner December 8, 2024 13:29
@github-actions github-actions bot added the Area: drivers Area: Device drivers label Dec 8, 2024
@IsikcanYilmaz
Copy link
Contributor Author

IsikcanYilmaz commented Dec 8, 2024

Currently the color_rgb_ts get converted to rmt_item32_ts at every write operation. As far as I understand, if we could avoid this conversion at every write, we could have the RMT driver do writes without blocking, and this would yield a better speedup.

Could be done by having ws281x_set_buffer to write to its buffer in the form of a rmt_item32_t instead of color_rgb_t, at least for the esp32 that uses the RMT.

Do the ESP gurus here see anything wrong with this potential solution?

@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 8, 2024
@maribu
Copy link
Member

maribu commented Dec 8, 2024

The code change looks good to me. Do you happen to already have tested that this doesn't cause any regressions? (Not that I would expect any from the changes, but better safe than sorry.)

Otherwise I can give the code a spin.

@IsikcanYilmaz
Copy link
Contributor Author

Do you happen to already have tested that this doesn't cause any regressions?

I'v been running it yes. So far so good.

Otherwise I can give the code a spin.

That would give me more confidence.

@IsikcanYilmaz
Copy link
Contributor Author

as far as i understand, this change would affect someone's application only if they change clock frequency in run time.

with this change in, should someone do such thing, they would have to deinit and reinit the neopixel driver.

@riot-ci
Copy link

riot-ci commented Dec 8, 2024

Murdock results

✔️ PASSED

53a4a8a drivers/ws281x: esp32 neopixel driver to set the rmt symbol data structures at init instead of every write

Success Failures Total Runtime
10249 0 10249 17m:49s

Artifacts

@maribu maribu enabled auto-merge December 13, 2024 08:36
@maribu maribu added this pull request to the merge queue Dec 13, 2024
Merged via the queue into RIOT-OS:master with commit a0327ae Dec 13, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants