Import RmtHI driver #4890
Import RmtHI driver #4890
Conversation
Pull the RMT High-priority Interrupt driver in to a vendored local library, pending inclusion in upstream NeoPixelBus. Driver is enabled only for XTensa chips; there's some unresolved issue with nested interrupts on RISCV.
WalkthroughAdds a new ESP32 high-priority RMT (HI) NeoPixel driver: public header API and typedefs, C++ interrupt-driven implementation, Xtensa ISR shim, library manifest, compile-time backend selector macro, and a few PlatformIO/usermods configuration tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
lib/NeoESP32RmtHI/library.json (1)
1-12: Optional: add version and keywords to the manifestConsider adding "version" and "keywords" to ease identification and reuse. No functional impact.
{ "name": "NeoESP32RmtHI", + "version": "0.1.0", + "keywords": ["esp32", "rmt", "ws2812", "neopixel", "wled"], "build": { "libArchive": false },lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S (3)
15-16: Redundant nested CONFIG_BTDM_CTRL_HLI checkYou already exclude CONFIG_BTDM_CTRL_HLI at the top; the inner #ifndef is redundant. Remove to simplify.
-/* If the Bluetooth driver has hooked the high-priority interrupt, we piggyback on it and don't need this. */ -#ifndef CONFIG_BTDM_CTRL_HLI +/* If the Bluetooth driver has hooked the high-priority interrupt, we piggyback on it and don't need this. */…and drop the matching #endif near the end accordingly.
58-63: Ensure interrupt stack alignmentExplicitly align the interrupt stack to 16 bytes to avoid ABI surprises under debug tools or future cores.
.data + .align 4 _rmt_intr_stack: .space RMT_INTR_STACK_SIZE + .align 4 _rmt_save_ctx: .space REG_SAVE_AREA_SIZE
166-170: Call arg register is unused; safe but confusingYou move SP into a6 and call NeoEsp32RmtMethodIsr, but the C handler ignores the arg. Either pass it in a2 (Xtensa first-arg) or drop the assignment to a6 for clarity. No functional change expected.
- mov a6, sp - call4 NeoEsp32RmtMethodIsr + mov a2, sp + call4 NeoEsp32RmtMethodIsrlib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (1)
485-503: Wait loop uses std::min; add header or avoid STLIf STL is undesirable on some configs, replace std::min with a trivial inline. Otherwise, the header addition above resolves it.
- TickType_t sleep = std::min(wait_time, (TickType_t) 5); + TickType_t sleep = (wait_time < (TickType_t)5) ? wait_time : (TickType_t)5;lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (4)
87-90: Remove dependency on non-standard macro in header
ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUTmay not exist in all builds; headers should avoid such macros.- return (ESP_OK == ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT(NeoEsp32RmtHiMethodDriver::WaitForTxDone(_channel.RmtChannelNumber, 0))); + return (ESP_OK == NeoEsp32RmtHiMethodDriver::WaitForTxDone(_channel.RmtChannelNumber, 0));If you want logging, do it in the .cpp or caller, not in the header.
29-36: Include what you useThis header uses std::swap and memcpy; add the standard headers to avoid relying on transitive includes.
#pragma once #if defined(ARDUINO_ARCH_ESP32) // Use the NeoEspRmtSpeed types from the driver-based implementation #include <NeoPixelBus.h> +#include <algorithm> // std::swap +#include <cstring> // memcpy
119-134: Write path: preserve API error propagation and consistencyYou mix
_WITHOU_ABORTfor Write start but not for the pre-wait; consider consistent non-aborting error handling inside Update and surface errors to caller (return bool/status).
- Return an
esp_err_tfromUpdate()to propagate failures (timeout, invalid state).- Keep logging out of headers; do it in .cpp with
ESP_LOGx.
1-27: Minor: license and attribution alignmentHeader says “cannot be mixed with the non-HI driver” but several typedefs did mix; after fixing, the statement holds. License text looks fine for LGPL; ensure vendoring notice in library.json too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h(1 hunks)lib/NeoESP32RmtHI/library.json(1 hunks)lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S(1 hunks)lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp(1 hunks)platformio.ini(1 hunks)wled00/bus_wrapper.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/bus_wrapper.h
🧬 Code graph analysis (1)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (2)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (8)
Install(360-436)Install(360-360)Uninstall(438-468)Uninstall(438-438)Write(470-483)Write(470-470)WaitForTxDone(485-503)WaitForTxDone(485-485)wled00/bus_wrapper.h (1)
getDataSize(1119-1215)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (5)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (2)
247-281: End marker item builds level from bit0; confirm intended polarityWhen padding the remainder with an end event, you copy level1 from bit0 to both level0 and level1. If the idle level differs for certain chip timings, this might hold the line at the wrong level during reset. Validate against each timing tuple you support.
169-215: Assembly unguarded:ld_include_hli_vectors_rmtis always defined for ESP32/S2/S3
The symbol is declared.globalin NeoEsp32RmtHI.S with no target‐specific guards, so the XTensa intr_alloc path will link correctly on ESP32, ESP32-S2, and ESP32-S3.wled00/bus_wrapper.h (3)
247-254: Backend selector looks good; riscv disabled as intendedConditional inclusion of the HI RMT method for Xtensa and fallback to N-methods elsewhere matches the PR’s stated scope. No issues spotted here.
255-304: Macro swap is mechanically correctAll B_32_RN_* macros now flow through NeoEsp32RmtMethod(x). This centralizes backend selection without changing semantics. Good change.
247-254: Guard for WLED_USE_SHARED_RMTIf WLED_USE_SHARED_RMT is defined, we skip including NeoEsp32RmtHIMethod.h. Ensure the shared-RMT path exposes the NeoEsp32RmtN* types for every x used here; otherwise builds will break under that flag.
PlatformIO's Library Dependency Finder will take care of it based on the #include.
While these were both fortunately included in Arduino.h, as @coderabbitai suggests, it's best practice to be explicit for anything one uses directly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (3)
31-36: OK to omit ESP32‑C3 compile‑time guard here (centralized disable in bus_wrapper.h)Matches the stated approach to keep RISCV (ESP32‑C3) disabled in one place while issues are investigated. Not requesting changes.
(Used your stated preference from learnings.)
72-85: Destructor pattern (wait → uninstall → GPIO reset) looks correct for HI driverAborting on uninstall failure aligns with upstream and the hardware‑safety rationale. No changes requested.
197-197: Typedefs now consistently reference HI variants — good fixAliases for WS2814 (and inverted) correctly map to HI Ws2805 variants, and all channel‑specific typedefs use HI classes. This resolves prior HI/non‑HI mixing.
Also applies to: 212-212, 227-227, 244-244, 259-259, 276-276, 291-291, 306-306, 321-321, 340-340, 355-355, 370-370, 387-387, 403-403, 419-419, 434-434, 449-449, 464-464
🧹 Nitpick comments (2)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (2)
114-135: Guard Update() against rare OOM casesIf either buffer failed to allocate in construct(), Update() would dereference nullptr. Cheap guard avoids crashes without diverging much from upstream behavior.
void Update(bool maintainBufferConsistency) { + if (!_dataEditing || !_dataSending) { + return; + } // wait for not actively sending data
34-36: Include for std::swapAvoid relying on transitive includes.
-#include <NeoPixelBus.h> +#include <NeoPixelBus.h> +#include <utility>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h(1 hunks)usermods/platformio_override.usermods.ini(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:57.991Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
📚 Learning: 2025-09-02T01:45:57.991Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:57.991Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-09-02T01:48:16.389Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:72-85
Timestamp: 2025-09-02T01:48:16.389Z
Learning: In NeoPixelBus driver code, ESP_ERROR_CHECK (which can abort) is used in destructors when driver uninstall failures indicate hardware is in an unsafe configuration, as such errors are considered unrecoverable hardware safety issues rather than typical software errors.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-09-02T01:56:43.822Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.822Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
🧬 Code graph analysis (1)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (1)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (8)
Install(360-436)Install(360-360)Uninstall(438-468)Uninstall(438-438)Write(470-483)Write(470-470)WaitForTxDone(485-503)WaitForTxDone(485-485)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (2)
usermods/platformio_override.usermods.ini (1)
5-5: Remove verification request:env:esp32dev_V4isn’t defined in platformio.ini; extendingenv:esp32devis correct and no flag diff check is neededLikely an incorrect or invalid review comment.
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (1)
229-324: Channel availability guards match SoC capabilities
- C3 block excludes channels >1 here, with broader disable centralized elsewhere.
- S2/S3 blocks exclude channels 4–7 as expected.
Looks consistent.
Also applies to: 372-467
|
Ready for merge or still a draft? |
|
I think that covers most of the AI's concerns; I'm glad to have gone through that process. I'm happy to merge now and get wider feedback. Note also that this driver is wholly incompatible with IDF v5 -- they've redone the RMT low level API again and it'll need yet another wrapper around the hardware operation. :( |
|
that is some nice piece of work, did you come up with all that assembly code or is it an adaptation from an existing driver? |
I borrowed most of it from the Espressif's high-priority interrupt bluetooth driver, as mentioned at the top of the file, tidied up a bit to get rid of the unnecessary RTOS interfacing. (Somewhat ironically, if that driver is linked, this code doesn't even bother with its own assembly layer: it piggybacks on theirs.) Maybe I need to improve the annotation again... The trouble with the C3 seems to be some kind of race with the scheduler. On RISCV, the scheduler is always run via an interrupt, so I don't think it's a bug in this driver code directly: setting the reference driver to use a higher priority triggers the same issue. ..but there's enough weirdness that I can't be 100% confident of any of the above. Interaction with the JTAG debugger is broken in the C3 Tasmota cores - wrong IDF options - which doesn't help. :( |
sounds far outside of my current knowledge on how the whole scheduling works, I am not very familiar with using any RTOS but from past knowledge from STM32 I would imagine that adding an explicit clear of an interrupt flag should be possible. But I am guessing that is part of the pre-built arduino core and inaccessible without building our own. |
I haven't analyzed it fully yet - given the debugger issues, it's been tough to get much information out. If the actual race can be characterized, it's not beyond the realm of possibility to patch around it. But yes, it looks like the problem is fairly deep in the IDF core. While I don't see any seemingly relevant changes in IDF v5, given that I couldn't find any bug reports about this issue, trying a newer IDF version is the next thing on my agenda here. |
|
please excuse me if I am being ignorant here but since the issue is not directly wled related: have you considered creating a simple test project, maybe even IDF based, that allows you to use the debugger? |
|
It's on my list. I did manage to get |
Pull the RMT High-priority Interrupt driver in to a vendored local library, pending inclusion in upstream NeoPixelBus.
Driver is enabled only for XTensa chips; there's some unresolved issue with nested interrupts on RISCV.
Fixes #4389 for everything except C3s.
Summary by CodeRabbit
New Features
Performance
Refactor
Chores