[upstream backport] RMT High-priority Interrupt driver - solves flickering problems on esp32, esp32-S2 and esp32-S3 #308
Conversation
(0.15) RMT High-priority Interrupt driver backport
only esp32dev_legacy stays on NPB 2.7.5
fingers crossed
📝 WalkthroughWalkthroughAdds a High‑Interrupt RMT NeoPixel driver for ESP32: Xtensa assembly ISR shim, ISR-driven C++ driver with per‑channel streaming, a templated double‑buffered method base with many concrete typedefs, a library manifest, build‑flag integration, bus wrapper macro, and conditional file‑wait guards. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Method as NeoEsp32RmtHIMethodBase
participant Driver as NeoEsp32RmtHiMethodDriver
participant RMT as RMT Hardware
participant ISR as ISR (xt_highintx → NeoEsp32RmtMethodIsr)
App->>Method: construct(pin,pixelCount,...)
Method->>Method: allocate _dataEditing/_dataSending
Method->>Driver: Install(channel,rmtBit0,rmtBit1,reset)
Driver->>RMT: configure hardware & bind ISR
Driver-->>Method: ESP_OK
Method->>Method: Update(maintainConsistency)
Method->>Method: SwapBuffers()
Method->>Driver: Write(channel,_dataSending,size)
Driver->>RMT: RmtStartWrite()
loop streaming
RMT->>ISR: TX threshold interrupt
ISR->>Driver: NeoEsp32RmtMethodIsr()
Driver->>RMT: RmtFillBuffer() (stream bytes -> RMT memory)
end
RMT->>Driver: TX complete
Driver-->>Method: signal completion
sequenceDiagram
participant Method as NeoEsp32RmtHIMethodBase
participant Driver as NeoEsp32RmtHiMethodDriver
Method->>Driver: WaitForTxDone(channel, timeout=0)
alt transmitting
Driver-->>Method: false
else idle
Driver-->>Method: true
end
Method->>Method: Destructor()
Method->>Driver: WaitForTxDone()
Method->>Driver: Uninstall(channel)
Driver->>Driver: free channel state & buffers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:
- Around line 192-199: The construct() method does raw malloc for _dataEditing
and _dataSending without checking for NULL; update construct() to check each
malloc return, and if either allocation fails free any successful allocation,
set the pointer(s) to nullptr and set/return an error state (e.g., set _sizeData
= 0 or an _initOk flag) so callers won't dereference null; reference the
construct() function and the _dataEditing and _dataSending members and also add
defensive NULL checks in getData(), Update(), and the destructor to handle
nullptr safely.
In @wled00/bus_wrapper.h:
- Around line 186-195: The RMT driver selection conditional may incorrectly pick
the HI driver or fail on builds where ESP_IDF_VERSION_MAJOR isn't defined;
verify that ESP_IDF_VERSION_MAJOR is defined for all ESP32 build targets (or add
a safe fallback check) and ensure build profiles that must avoid the HI driver
(e.g., esp32dev_compat) explicitly undefine WLED_USE_SHARED_RMT or define
__riscv appropriately so the conditional doesn't select HI; also confirm that
the concatenated macro NeoEsp32RmtMethod(x) maps to real symbols declared in
NeoEsp32RmtHIMethod.h (e.g., NeoEsp32RmtHINWs2812xMethod etc.) and adjust the
conditional or provide alternate typedefs if those method names are absent or
incompatible with the NeoPixelBus version.
🧹 Nitpick comments (4)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (3)
41-43: Dead code: RISC-V include is unreachable.This include is guarded by
#ifdef __riscv, but the file itself is excluded from compilation on RISC-V platforms at line 33 via!defined(__riscv). This code path will never be reached.🔎 Proposed fix
-#ifdef __riscv -#include "riscv/interrupt.h" -#endif
370-403: Consider atomic initialization of global driverState.The first-time initialization of
driverState(lines 370-404) is not protected against concurrent calls toInstall()from multiple threads. While unlikely in typical WLED usage, if two channels are initialized simultaneously from different tasks, a race condition could occur.For embedded environments with limited concurrency, this is likely acceptable, but worth noting for future-proofing.
446-446: Hardcoded timeout value.The 10-second timeout (
10000 / portTICK_PERIOD_MS) is used in multiple places (lines 446, 477, 95 in header). Consider extracting this to a named constant for consistency and maintainability.lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (1)
91-104: Destructor should check for null before freeing.While
free(nullptr)is safe in C/C++, the destructor callsNeoEsp32RmtHiMethodDriver::Uninstall()andgpio_matrix_out()before freeing buffers. Ifconstruct()failed to allocate memory, theInitialize()call would have proceeded with null buffers, potentially causing issues. Consider adding null checks for defensive programming.🔎 Proposed fix
~NeoEsp32RmtHIMethodBase() { // wait until the last send finishes before destructing everything // arbitrary time out of 10 seconds ESP_ERROR_CHECK_WITHOUT_ABORT(NeoEsp32RmtHiMethodDriver::WaitForTxDone(_channel.RmtChannelNumber, 10000 / portTICK_PERIOD_MS)); ESP_ERROR_CHECK(NeoEsp32RmtHiMethodDriver::Uninstall(_channel.RmtChannelNumber)); gpio_matrix_out(_pin, SIG_GPIO_OUT_IDX, false, false); pinMode(_pin, INPUT); - free(_dataEditing); - free(_dataSending); + if (_dataEditing) free(_dataEditing); + if (_dataSending) free(_dataSending); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.hlib/NeoESP32RmtHI/library.jsonlib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cppplatformio.iniwled00/bus_wrapper.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_wrapper.h
🧠 Learnings (7)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.iniwled00/bus_wrapper.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
platformio.iniwled00/bus_wrapper.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
platformio.inilib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cppwled00/bus_wrapper.h
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/bus_wrapper.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/bus_wrapper.h
🧬 Code graph analysis (1)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (1)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (8)
Install(363-439)Install(363-363)Uninstall(441-471)Uninstall(441-441)Write(473-486)Write(473-473)WaitForTxDone(488-506)WaitForTxDone(488-488)
⏰ 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). (20)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (11)
lib/NeoESP32RmtHI/library.json (1)
1-12: LGTM!The library manifest is correctly structured with appropriate platform restriction and version constraint on NeoPixelBus that aligns with the PR objectives.
platformio.ini (1)
255-257: LGTM!The NeoPixelBus version updates to 2.7.9 are consistent across all V4 ESP32 build environments, while legacy V3 builds correctly remain on 2.7.5 as stated in the PR objectives. The commented-out lines at 255-256 serve as documentation for the version choices.
Also applies to: 433-434, 467-468, 530-531, 557-558
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S (3)
8-9: Guard condition redundancy.Line 8 checks
defined(ESP32)and line 9 checksdefined(ARDUINO_ARCH_ESP32). While both checks serve different purposes (ESP32 macro vs Arduino framework macro), they're effectively testing the same platform. This is intentional for robustness but worth noting for maintainability.
59-63: Static allocation in .data section.The interrupt stack (
_rmt_intr_stack) and save context (_rmt_save_ctx) are allocated in.datarather than.bss. While.bsswould be slightly more efficient (no initialization needed), this approach is correct and avoids potential issues with uninitialized memory on some toolchains.
70-252: LGTM!The interrupt entry/exit sequence correctly implements the Xtensa high-level interrupt pattern: saving all physical registers via window rotation, switching to dedicated interrupt stack, calling the C handler with proper PS configuration, and restoring state in reverse order. The
call4convention andRFIinstruction usage are appropriate for windowed ABI.lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (3)
309-325: LGTM - ISR implementation is safe.The ISR correctly handles potential race conditions:
- Checks
driverState[channel]for null before dereferencing (line 314)- Stops TX if driver state is unexpectedly null (line 320)
- Clears interrupt after processing each channel (line 322)
The while loop processing multiple pending channels is appropriate for shared interrupt handling.
488-506: Potential underflow in wait_time decrement.Line 502 decrements
wait_timebysleep, butsleepis aTickType_tderived fromstd::min(wait_time, (TickType_t) 5). IfportTICK_PERIOD_MSis not 1 (e.g., on systems with different tick rates), the timeout calculation at line 477 (10000 / portTICK_PERIOD_MS) could result in very small values, and the decrement logic would still work correctly. The implementation is safe.
192-198: Remove unreachable RISC-V interrupt level definition.Lines 196-198 define
INT_LEVEL_FLAGfor__riscv, but the file is guarded at line 33 with!defined(__riscv), making this code path unreachable. This block should be removed; only the Xtensa fallbacks at lines 199-203 are active on supported platforms.Likely an incorrect or invalid review comment.
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (2)
202-343: LGTM - Type definitions are comprehensive.The extensive typedefs cover all supported LED types (Ws2811, Ws2812x, Ws2805, Sk6812, Tm1814, etc.) across all available RMT channels (0-7 for classic ESP32, 0-3 for S2/S3, 0-1 for C3). The conditional compilation correctly limits channel availability based on target platform.
345-486: LGTM - Inverted speed variants follow same pattern.The inverted speed typedefs mirror the normal variants with appropriate
NeoEsp32RmtInvertedSpeed*types, maintaining consistency across all LED types and channels.wled00/bus_wrapper.h (1)
200-254: LGTM! Clean macro-based driver abstraction.The consistent use of the
NeoEsp32RmtMethod(x)macro across all ESP32 RMT bus type definitions provides a clean abstraction layer. This centralizes the driver selection logic, making it easy to switch between the HI and standard drivers based on platform capabilities.The macro correctly routes different LED types (Ws2812x, 400Kbps, Tm1814, Tm1829) to their appropriate driver implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio.ini
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
platformio.ini
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
platformio.ini
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
platformio.ini
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
platformio.ini
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
platformio.ini
⏰ 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). (20)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (5)
platformio.ini (5)
335-335: Confirm build_flags_compat section has correct WLED_USE_SHARED_RMT flag.Line 335 adds the same WLED_USE_SHARED_RMT flag to the esp8266 compatibility flags. This is consistent with the main esp8266 build flags (line 304) and appropriate for the v0.14.0 compatibility builds.
255-256: Commented NeoPixelBus entries in [common] section may cause confusion.Lines 255-256 show commented-out NeoPixelBus entries:
;;makuna/NeoPixelBus @ 2.7.9 ;; WLEDMM will be added in board specific sections ;;makuna/NeoPixelBus @ 2.8.3This is intentional (per the inline comment) because platform-specific sections define lib_deps, which override the [common] section. However, the comment on line 255 should clarify why it's done this way—likely to enforce per-platform version pinning to avoid accidental upgrades to 2.8.3 or higher. The current approach is correct for enforcing the constraint "cannot use higher versions due to API incompatibilities".
359-359: Verify esp32_legacy (V3) section correctly blocks RMTHI driver.Line 359 in the [esp32_legacy] section should add
-D WLED_USE_SHARED_RMTwith a comment explaining it blocks the RMTHI driver for esp-idf v3.x compatibility. Ensureesp32dev_compat(lines 667-683) inherits fromesp32_legacyand thus receives this flag through the inheritance chain.
304-304: WLED_USE_SHARED_RMT flag correctly disables RMTHI driver on incompatible platforms.The flag is properly implemented in wled00/bus_wrapper.h (line 188). When
-D WLED_USE_SHARED_RMTis defined in platformio.ini, the condition!defined(WLED_USE_SHARED_RMT)evaluates to false, causing the else branch to select the standardNeoEsp32RmtNmethod instead of the HI variant. Line 304 correctly adds this flag to the esp8266 build, preventing RMTHI usage on platforms that don't support it.
527-527: WLED_USE_SHARED_RMT flag added to esp32c3 — inheritance verified.Line 527 adds
-D WLED_USE_SHARED_RMTto the [esp32c3] section build_flags with the comment "don't use the RMTHI driver (not compatible with -C3)". This is appropriate because ESP32-C3 has a simpler CPU architecture without the classic RMT peripheral supporting high-priority interrupts.All esp32c3 environment variants inherit this flag correctly:
esp32c3dev_4MB_Mincludes${esp32c3.build_flags}in its build_flags, whileesp32c3mini_dio_4MB_M,esp32c3dev_2MB_M, andseeed_esp32c3_4MB_Sinherit it through their extension ofesp32c3dev_4MB_M.
willmmiles
left a comment
There was a problem hiding this comment.
Looks good to me. I'm surprised that you needed to disable the driver for v3 compiles; upstream 0.15 is still using v3 for baseline ESP32, and this driver is working well for us there.
@willmmiles thanks for the review. I had problems to build the driver on V3, but it may also be caused by a too-old version of neoPixelBus - wanted to keep the V3 as a "safe fallback" option with the older NPB 2.7.5. WLED-MM users should default to V4 builds, especially to get the flicker-free RMT driver. |
reversed the logic, to make it more straight forward.
my Lolin S2 mini "ESP32-S2FNR2 (revision v0.0)" get into a boot loop with the RMTHI driver enabled. More debugging tomorrow.
same as on -S3: interrupt watchdog reset at the time when Wifi tries to connect.
|
@willmmiles I'm getting regular interrupt watchdog resets on -S2 and -S3. Any idea? Edit: S3 doesn't crash when building with -DWLED_USE_SHARED_RMT, but I see repeated warnings when compiling with -DCORE_DEBUG_LEVEL=5 |
There can be a couple of reasons.
Otherwise if you can share your full test config, I'd be happy to see if I can replicate the problem on one of my test boards.
Would you mind looking up this address?
This can be the result of missed interrupt timing -- Espressif's RMT driver can get stuck without ever completing the transaction. |
* re-enabled RMTI on -S3 * added [env:esp32s3dev_8MB_PSRAM_opi]
@willmmiles I've created a simpler buildenv for reproducing the crash Line 816 in 68eee6b You can use these cfg.json and presets.json files Its a very simple setup with two outputs on RMT (64 and 256 LEDs).
Its in the panic handler (slightly different address in my testenv from above, but same locaction
The crash looks like this on |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @platformio.ini:
- Line 560: The ESP32-S3 profiles incorrectly comment out -D WLED_USE_SHARED_RMT
while the comment says RMTHI should be disabled; update the S3 config entries
that currently have ";-D WLED_USE_SHARED_RMT" (the occurrences corresponding to
the two S3 sections) to enable the macro (remove the leading semicolon) so
WLED_USE_SHARED_RMT is defined and RMTHI is disabled, matching the ESP32-S2
behavior and the bus_wrapper.h logic; if this change is intentionally postponed
for testing, instead update the inline comment near those S3 entries to
explicitly state the intention and rationale.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio.ini
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:57.030Z
Learning: In MoonModules/WLED-MM platformio.ini, [esp32_legacy] section intentionally uses NeoPixelBus 2.7.5 while V4 platform builds use 2.7.9 or later. The older V3 Platform builds don't have the same flickering problems that newer versions address, and NPB 2.7.9 provides no added value over 2.7.5 for these legacy builds.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
📚 Learning: 2026-01-05T21:35:51.697Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:51.697Z
Learning: In MoonModules/WLED-MM/platformio.ini, in the [esp32_legacy] section, pin NeoPixelBus to version 2.7.5. For V4 platform builds, use 2.7.9 or later; older V3 builds do not have the same flickering issues and do not gain from 2.7.9. This rule is specifically for the legacy build section and explains why the older version is intentional there.
Applied to files:
platformio.ini
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.ini
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
platformio.ini
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
platformio.ini
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
platformio.ini
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
platformio.ini
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
platformio.ini
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
platformio.ini
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
platformio.ini
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always build at least one common PlatformIO environment before finishing
Applied to files:
platformio.ini
⏰ 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). (20)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
🔇 Additional comments (5)
platformio.ini (5)
816-840: New ESP32-S3 PSRAM environment activated — verify RMTHI driver consistency.The new environment
[env:esp32s3dev_8MB_PSRAM_opi](lines 816–840) is now active and extends the S3 base configuration. Line 830 inherits the commented-out-D WLED_USE_SHARED_RMTflag from its parent, which means RMTHI will be enabled on this board.Given that line 560 and 830 both note "RMTHI causes interrupt watchdog reset," verify that this new environment is intentionally testing the RMTHI driver under PSRAM conditions, or if it should also explicitly enable
WLED_USE_SHARED_RMTto disable RMTHI for safety.
504-505: Consistent WLED_USE_SHARED_RMT configuration for ESP32-S2 with explicit rationale.The S2 build correctly enables
-D WLED_USE_SHARED_RMTwith a specific comment: "RMTHI fails miserably on my Lolin S2 mini 'ESP32-S2FNR2 (revision v0.0)' - I don't know why (boot loop when trying to connect wifi)."This is the right approach: disable the problematic RMTHI driver on platforms where it causes issues. The accompanying NeoPixelBus 2.7.9 update (line 508) is correctly placed.
Also applies to: 508-508
304-304: ✓ Correct platform-specific RMTHI driver disabling for ESP8266 and ESP32-C3.Lines 304, 335, and 359 correctly add
-D WLED_USE_SHARED_RMTto platforms that cannot use the new RMTHI driver:
- ESP8266 (both standard and compat): RMTHI is not applicable to 8266 architecture.
- ESP32-C3 (line 531): RMTHI is incompatible with C3 (no RMT hardware unit suitable for high-priority interrupts).
- ESP32-legacy V3 (line 359): Correctly disables RMTHI on older V3 platform builds.
This aligns with the PR objectives: "Do not enable the new RMT High-priority Interrupt (RMTHI) driver on older V3 builds (notably esp32dev_compat)."
Also applies to: 335-335, 359-359, 531-531
436-436: ✓ NeoPixelBus library upgraded to 2.7.9 across V4 and multi-variant ESP32 builds.Lines 436, 470, 508, 535, and 563 consistently update
makuna/NeoPixelBusto version 2.7.9 across:
lib_depsV4(line 436)esp32_idf_V4(line 470)esp32s2(line 508)esp32c3(line 535)esp32s3(line 563)This aligns with the PR objective: "Upgrade all ESP32 builds to NeoPixelBus 2.7.9." The earlier learnings confirm that
[esp32_legacy]intentionally remains on NeoPixelBus 2.7.5, as V3 builds do not experience the flickering issues addressed by 2.7.9.Also applies to: 470-470, 508-508, 535-535, 563-563
255-256: NeoPixelBus library dependency placement is correct. All build environments inherit explicit NeoPixelBus dependencies from their respective base platform sections ([esp8266]@ 2.7.5,[esp32_legacy]@ 2.7.5,[esp32_idf_V4]/[esp32s2]/[esp32c3]/[esp32s3]@ 2.7.9). No environment lacks coverage after removal from[common].
| */ | ||
|
|
||
| #if defined(__XTENSA__) && defined(ESP32) && !defined(CONFIG_BTDM_CTRL_HLI) | ||
| #if !defined(WLED_USE_SHARED_RMT) && !defined(__riscv) && defined(ARDUINO_ARCH_ESP32) && (ESP_IDF_VERSION_MAJOR >= 4) // WLEDMM don't compile this file on unsupported platforms |
There was a problem hiding this comment.
This line is the cause of your problems: the high priority interrupt assembly layer is being disabled here. Neither ARDUINO_ARCH_ESP32 or ESP_IDF_VERSION_MAJOR are available as they haven't been included, only command-line defines are available at this point in the file. (Also !defined(__riscv) is implied by defined(__XTENSA__)).
(FWIW the !defined(CONFIG_BTDM_CTRL_HLI) check there above is also wrong -- it can never be defined at that point in the file since sdkconfig.h hasn't been included yet.)
This change is also unnecessary: the linker will discard the object file if it's never referenced by the application code. (This is the same approach used by all the other unused drivers in NPB.)
The error isn't caught at build time because the NeoESPRmtHIMethod.cpp code that asks for it to be linked is playing some games with __attribute__((used)) in a way that cues the linker to include the file, but at no actual code space or runtime cost. However, if the assembly file is missing, the link will still succeed as the symbol it calls for is never in fact truly used. I'd prioritized runtime performance over developer convenience - sorry about that!
Anyways my recommendation is revert all the changes to lib/NeoESP32RmtHI from this patch. If bus_wrapper.h never calls for any RMTHI instances, the linker will discard the .cpp and .S object files, so it ultimately won't affect any build products.
There was a problem hiding this comment.
If you're curious as to the derivation of the symptoms:
- Tasmota ESP32 (no suffix) cores include
CONFIG_BTDM_CTRL_HLI, so the HI driver piggybacks on the Bluetooth driver's handler (as there's only one high priority interrupt level available). This is why it worked on basic ESP32s, but not -S2s and -S3s. - Without the contents of the .S file from the library, the linker falls back to the default highint handler (a no-op). So as soon as an RMT interrupt was generated, the core in question looped forever, as the interrupt would never be cleared.
There was a problem hiding this comment.
This line is the cause of your problems: the high priority interrupt assembly layer is being disabled here. Neither
ARDUINO_ARCH_ESP32orESP_IDF_VERSION_MAJORare available as they haven't been included, only command-line defines are available at this point in the file. (Also!defined(__riscv)is implied bydefined(__XTENSA__)).
wow, that's unexpected, but it explains the problems. I still have a problem with my "emergency fallback" esp32dev_compat V3 buildenv, because its still on NPB 2.7.5.
Compiling .pio\build\esp32dev_compat\src\wled00.ino.cpp.o
Linking .pio\build\esp32dev_compat\firmware.elf
.pio\build\esp32dev_compat\libafb\NeoESP32RmtHI\NeoEsp32RmtHI.S.o:(.iram1.literal+0x14): undefined reference to `NeoEsp32RmtMethodIsr'
.pio\build\esp32dev_compat\libafb\NeoESP32RmtHI\NeoEsp32RmtHI.S.o: In function `_highint_stack_switch':
(.iram1+0x92): undefined reference to `NeoEsp32RmtMethodIsr'
collect2.exe: error: ld returned 1 exit status
--> i'll do some more experimenting, most likely I can still protect compiling the .s file with #if !defined(WLED_USE_SHARED_RMT) because this is a command line define.
😅 means another round of testing, but at least we now have an explanation that makes sense 👍
There was a problem hiding this comment.
Maybe try lib_ignore = NeoESP32RmtHI in the environment def for those using older NPB? It might be cleaner.
see discussion in PR #308 experimental sanity checks added, will possibly remove them later.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:
- Around line 192-199: The construct() method currently assigns malloc results
to _dataEditing and _dataSending without checking for nullptr; add null checks
after each malloc in NeoEsp32RmtHIMethod::construct(), set an internal error
flag (e.g., _allocFailed or reuse an existing error indicator), and bail out on
failure; if the second allocation fails, free the first buffer and null it to
avoid leaks; ensure getData()/Update() and any callers check the error flag or
nullptrs before using the buffers so crashes are prevented.
🧹 Nitpick comments (3)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (2)
285-307: Consider documenting magic numbers in timing calculations.Line 296 uses hardcoded values
100and1400without explanation. These appear to be timing-related constants (possibly microseconds) for the reset event duration calculation.📝 Suggested improvement
// Add constants at file scope with explanatory comments: constexpr uint32_t RMT_RESET_MIN_DURATION = 100; // Minimum reset duration in ticks constexpr uint32_t RMT_RESET_PREAMBLE = 1400; // Preamble duration in ticks (7 * 200) // Then in RmtStartWrite: fill.duration1 = state.resetDuration > RMT_RESET_PREAMBLE ? (state.resetDuration - RMT_RESET_PREAMBLE) : RMT_RESET_MIN_DURATION;
488-506: Consider event-based synchronization to reduce high-priority ISR scheduler pressure.The RMTHI driver on ESP32-S3 is known to cause interrupt watchdog resets (as noted in platformio.ini). While the root cause appears to be the high-priority ISR's interaction with the FreeRTOS scheduler, using semaphores or task notifications instead of polling in
WaitForTxDonecould reduce scheduler pressure from the ISR.Replace the
vTaskDelaypolling loop with:⚙️ Recommended refactor: Use task notification
- Add a
UBaseType_t notificationValuefield toNeoEsp32RmtHIChannelStatefor each channel- In the ISR, call
xTaskNotifyFromISR()when transmission completes (or enable TX end interrupt to notify instead of threshold)- In
WaitForTxDone, usexTaskNotifyWait()with timeout instead of pollingThis reduces ISR execution time and scheduler interference, which may help investigate the S3 watchdog issue further.
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S (1)
108-112: Consider adding a compile-time assertion to document the 64-register assumption.The FIXME correctly identifies that the code assumes 64 physical registers. All current ESP32 variants (ESP32, S2, S3, C3) use 64 registers, but this is not explicitly enforced. Since
XCHAL_NUM_AREGSis available from the SDK (as evidenced by the use ofXCHAL_HAVE_LOOPS), a static assertion would improve code clarity and catch unsupported configurations at build time:_Static_assert(XCHAL_NUM_AREGS == 64, "This RMT ISR assumes 64 physical registers");This converts the informal FIXME into an explicit, verifiable constraint.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.hlib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:57.030Z
Learning: In MoonModules/WLED-MM platformio.ini, [esp32_legacy] section intentionally uses NeoPixelBus 2.7.5 while V4 platform builds use 2.7.9 or later. The older V3 Platform builds don't have the same flickering problems that newer versions address, and NPB 2.7.9 provides no added value over 2.7.5 for these legacy builds.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpplib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2026-01-05T21:35:57.030Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:57.030Z
Learning: In MoonModules/WLED-MM platformio.ini, [esp32_legacy] section intentionally uses NeoPixelBus 2.7.5 while V4 platform builds use 2.7.9 or later. The older V3 Platform builds don't have the same flickering problems that newer versions address, and NPB 2.7.9 provides no added value over 2.7.5 for these legacy builds.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
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(363-439)Install(363-363)Uninstall(441-471)Uninstall(441-441)Write(473-486)Write(473-473)WaitForTxDone(488-506)WaitForTxDone(488-488)
🪛 Cppcheck (2.19.0)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp
[error] 46-46: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
⏰ 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). (20)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
🔇 Additional comments (12)
lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S (4)
1-10: LGTM - Clear documentation and platform checks.The buildenv sanity checks ensure compilation only on supported platforms.
12-37: LGTM - Proper conditional compilation and interrupt level selection.The guards ensure this ISR shim is only compiled for Xtensa ESP32 platforms where RMTHI driver is enabled. Interrupt level selection correctly adapts to platform configuration.
46-67: LGTM - Appropriate stack and context save area sizing.The 512-byte interrupt stack and 284-byte register save area are correctly sized for the Xtensa windowed ABI and ISR context switching.
74-256: LGTM - Correct Xtensa ISR context switching implementation.The assembly correctly implements the high-priority interrupt handler with proper register save/restore, stack switching, and return sequence for the Xtensa windowed ABI.
lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp (4)
46-170: LGTM - Well-documented compatibility layer for ESP-IDF v3.The shims provide missing rmt_ll_* functions for older ESP-IDF v3, properly attributed to Espressif's Apache-licensed code.
473-486: LGTM - Proper error handling prevents overlapping transmissions.The method correctly waits for the previous transmission to complete before starting a new one, returning timeout errors to the caller.
309-325: LGTM - ISR uses defensive null-check pattern.The ISR correctly handles the case where
driverState[channel]might be null (line 318-321), stopping the channel if invoked unexpectedly. The synchronization withInstall()is implicit through interrupt enable/disable (Install enables the interrupt only after state initialization at line 436).
441-471: LGTM - Proper cleanup with defensive timeout handling.The uninstall sequence correctly waits for transmission completion, then proceeds with cleanup even if the wait times out (line 448). Global state cleanup only occurs when all channels are uninstalled.
lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h (4)
37-54: LGTM - Well-structured compatibility macro.The macro backport from NeoPixelBus 2.8.3 properly handles the timeout case and only defines if not already present. The conditional compilation based on NDEBUG/CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_SILENT is correct.
91-104: LGTM - Proper cleanup sequence in destructor.The destructor correctly waits for transmission completion, uninstalls the driver, resets GPIO, and frees memory. Error handling with ESP_ERROR_CHECK macros is appropriate.
133-154: LGTM - Correct double-buffering with optional consistency guarantee.The Update method implements proper double-buffering. The
maintainBufferConsistencyparameter allows callers to choose whether to preserve pixel color consistency during async operations, trading off CPU time for data consistency.
202-486: LGTM - Comprehensive type definitions with correct platform conditionals.The typedef declarations cover all LED types, speeds, channels, and variants (normal/inverted), with appropriate conditional compilation for ESP32-C3 (2 channels), ESP32-S2/S3 (4 channels), and classic ESP32 (8 channels).
| void construct() | ||
| { | ||
| _dataEditing = static_cast<uint8_t*>(malloc(_sizeData)); | ||
| // data cleared later in Begin() | ||
|
|
||
| _dataSending = static_cast<uint8_t*>(malloc(_sizeData)); | ||
| // no need to initialize it, it gets overwritten on every send | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file structure and content around the flagged lines
find . -name "*NeoEsp32RmtHI*" -type f | head -20Repository: MoonModules/WLED-MM
Length of output: 200
🏁 Script executed:
# Look for malloc usage patterns in the entire NeoPixelBus library
rg "malloc" -A 3 --type=cpp --type=h | head -100Repository: MoonModules/WLED-MM
Length of output: 6518
🏁 Script executed:
# Check for error handling patterns with malloc in the codebase
rg "malloc.*if|if.*malloc|nullptr.*malloc" --type=cpp --type=h | head -50Repository: MoonModules/WLED-MM
Length of output: 543
🏁 Script executed:
# Examine the getData() and Update() methods mentioned in the review
rg -B 2 -A 8 "getData\(\)|Update\(" --type=cpp --type=h | grep -A 8 "_dataEditing\|_dataSending" | head -80Repository: MoonModules/WLED-MM
Length of output: 1517
🏁 Script executed:
# Check if construct() is actually part of the public API or internal
rg "construct\(" --type=cpp --type=h -B 3 -A 3 | head -60Repository: MoonModules/WLED-MM
Length of output: 1644
Add malloc failure checks in construct() method.
The method allocates _dataEditing and _dataSending without verifying success. If malloc fails (possible on ESP32 with limited heap), nullptr pointers are silently stored. Later calls to getData() return these invalid pointers directly, and Update() dereferences them, causing crashes or data corruption.
This violates WLED-MM's error-handling pattern seen elsewhere in FX_fcn.cpp and bus_manager.cpp, where malloc failures are checked and error flags are set:
void construct()
{
_dataEditing = static_cast<uint8_t*>(malloc(_sizeData));
if (!_dataEditing) {
// Handle error - log, set error flag, or abort
return; // or throw if using exceptions
}
_dataSending = static_cast<uint8_t*>(malloc(_sizeData));
if (!_dataSending) {
free(_dataEditing);
_dataEditing = nullptr;
return;
}
}Alternatively, implement an error flag accessible to callers so they can detect construction failure.
🤖 Prompt for AI Agents
In @lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h around lines 192 - 199, The
construct() method currently assigns malloc results to _dataEditing and
_dataSending without checking for nullptr; add null checks after each malloc in
NeoEsp32RmtHIMethod::construct(), set an internal error flag (e.g., _allocFailed
or reuse an existing error indicator), and bail out on failure; if the second
allocation fails, free the first buffer and null it to avoid leaks; ensure
getData()/Update() and any callers check the error flag or nullptrs before using
the buffers so crashes are prevented.
| esp_err_t NeoEsp32RmtHiMethodDriver::Install(rmt_channel_t channel, uint32_t rmtBit0, uint32_t rmtBit1, uint32_t reset) { | ||
| // Validate channel number | ||
| if (channel >= RMT_CHANNEL_MAX) { | ||
| return ESP_ERR_INVALID_ARG; | ||
| } | ||
|
|
||
| esp_err_t err = ESP_OK; | ||
| if (!driverState) { | ||
| // First time init | ||
| driverState = reinterpret_cast<NeoEsp32RmtHIChannelState**>(heap_caps_calloc(RMT_CHANNEL_MAX, sizeof(NeoEsp32RmtHIChannelState*), MALLOC_CAP_INTERNAL)); | ||
| if (!driverState) return ESP_ERR_NO_MEM; | ||
|
|
||
| // Ensure all interrupts are cleared before binding | ||
| RMT.int_ena.val = 0; | ||
| RMT.int_clr.val = 0xFFFFFFFF; | ||
|
|
||
| // Bind interrupt handler | ||
| #if defined(CONFIG_BTDM_CTRL_HLI) | ||
| // Bluetooth driver has taken the empty high-priority interrupt. Fortunately, it allows us to | ||
| // hook up another handler. | ||
| err = hli_intr_register(NeoEsp32RmtMethodIsr, nullptr, (uintptr_t) &RMT.int_st, 0xFF000000); | ||
| // 25 is the magic number of the bluetooth ISR on ESP32 - see soc/soc.h. | ||
| intr_matrix_set(cpu_hal_get_core_id(), ETS_RMT_INTR_SOURCE, 25); | ||
| intr_cntrl_ll_enable_interrupts(1<<25); | ||
| #elif defined(NEOESP32_RMT_CAN_USE_INTR_ALLOC) | ||
| // Use the platform code to allocate the interrupt | ||
| // If we need the additional assembly bridge, we pass it as the "arg" to the IDF so it gets linked in | ||
| err = esp_intr_alloc(ETS_RMT_INTR_SOURCE, INT_LEVEL_FLAG | ESP_INTR_FLAG_IRAM, HI_IRQ_HANDLER, (void*) HI_IRQ_HANDLER_ARG, &isrHandle); | ||
| //err = ESP_ERR_NOT_FINISHED; | ||
| #else | ||
| // Broken IDF API does not allow us to reserve the interrupt; do it manually | ||
| static volatile const void* __attribute__((used)) pleaseLinkAssembly = (void*) ld_include_hli_vectors_rmt; | ||
| intr_matrix_set(xPortGetCoreID(), ETS_RMT_INTR_SOURCE, ESP32_LV5_IRQ_INDEX); | ||
| ESP_INTR_ENABLE(ESP32_LV5_IRQ_INDEX); | ||
| #endif | ||
|
|
||
| if (err != ESP_OK) { | ||
| heap_caps_free(driverState); | ||
| driverState = nullptr; | ||
| return err; | ||
| } | ||
| } | ||
|
|
||
| if (driverState[channel] != nullptr) { | ||
| return ESP_ERR_INVALID_STATE; // already in use | ||
| } | ||
|
|
||
| NeoEsp32RmtHIChannelState* state = reinterpret_cast<NeoEsp32RmtHIChannelState*>(heap_caps_calloc(1, sizeof(NeoEsp32RmtHIChannelState), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)); | ||
| if (state == nullptr) { | ||
| return ESP_ERR_NO_MEM; | ||
| } | ||
|
|
||
| // Store timing information | ||
| state->rmtBit0 = rmtBit0; | ||
| state->rmtBit1 = rmtBit1; | ||
| state->resetDuration = reset; | ||
|
|
||
| // Initialize hardware | ||
| rmt_ll_tx_stop(&RMT, channel); | ||
| rmt_ll_tx_reset_pointer(&RMT, channel); | ||
| rmt_ll_enable_tx_err_interrupt(&RMT, channel, false); | ||
| rmt_ll_enable_tx_end_interrupt(&RMT, channel, false); | ||
| rmt_ll_enable_tx_thres_interrupt(&RMT, channel, false); | ||
| rmt_ll_clear_tx_err_interrupt(&RMT, channel); | ||
| rmt_ll_clear_tx_end_interrupt(&RMT, channel); | ||
| rmt_ll_clear_tx_thres_interrupt(&RMT, channel); | ||
|
|
||
| rmt_ll_tx_enable_loop(&RMT, channel, false); | ||
| rmt_ll_tx_enable_pingpong(&RMT, channel, true); | ||
| rmt_ll_tx_set_limit(&RMT, channel, rmtBatchSize); | ||
|
|
||
| driverState[channel] = state; | ||
|
|
||
| rmt_ll_enable_tx_thres_interrupt(&RMT, channel, true); | ||
|
|
||
| return err; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the exact file and verify the code structure
find . -name "NeoEsp32RmtHIMethod.cpp" -type fRepository: MoonModules/WLED-MM
Length of output: 110
🏁 Script executed:
# Let's examine the Install method and surrounding context to understand the synchronization issue
# Look for the file mentioned in the review
fd -e cpp | grep -i "neoesp32rmt" | head -20Repository: MoonModules/WLED-MM
Length of output: 108
🏁 Script executed:
# Search for the specific file more broadly
find . -type f -name "*NeoEsp32RmtHI*"Repository: MoonModules/WLED-MM
Length of output: 200
🏁 Script executed:
# Try a broader search for RMT-related files
fd "RmtHI" -type fRepository: MoonModules/WLED-MM
Length of output: 233
🏁 Script executed:
# Let's check what files exist in the repository
git ls-files | grep -i "rmt" | head -20Repository: MoonModules/WLED-MM
Length of output: 225
🏁 Script executed:
# Read the full file to understand the code structure and synchronization
wc -l lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cppRepository: MoonModules/WLED-MM
Length of output: 112
🏁 Script executed:
# Read key sections of the file
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | head -100Repository: MoonModules/WLED-MM
Length of output: 4270
🏁 Script executed:
# Read the Install method specifically
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | sed -n '360,445p'Repository: MoonModules/WLED-MM
Length of output: 4168
🏁 Script executed:
# Find the ISR implementation mentioned in the review (lines 314-321)
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | sed -n '310,330p'Repository: MoonModules/WLED-MM
Length of output: 1174
🏁 Script executed:
# Look for any existing mutex or synchronization patterns
rg "portMUX|xSemaphore|critical" lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cppRepository: MoonModules/WLED-MM
Length of output: 45
🏁 Script executed:
# Check for global state declarations
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | sed -n '230,260p'Repository: MoonModules/WLED-MM
Length of output: 1516
🏁 Script executed:
# Check Uninstall method to see if it also needs synchronization
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | sed -n '441,480p'Repository: MoonModules/WLED-MM
Length of output: 1813
Critical race condition on unprotected driverState access.
Multiple concurrent calls to Install() can race on the !driverState check at line 370, causing multiple allocations and memory leaks. Additionally, the ISR (line 314), Install() (line 434), Uninstall() (line 451), and Write() methods access driverState[channel] without synchronization, creating potential use-after-free or inconsistent state access.
The suggested fix in the original review is incorrect: it places heap_caps_calloc() inside a critical section, which violates FreeRTOS constraints—heap operations must not be called inside critical sections as they can block and involve the scheduler.
Correct approach: Allocate buffers outside the critical section, then use a critical section only for the brief pointer assignment. Alternatively, for more complex synchronization of the global state initialization, consider using a recursive mutex (xSemaphoreTakeRecursive()/Give()) rather than a spinlock critical section, as the allocation and initialization logic is not a short, non-blocking operation.
|
Hi there, regarding the idle watchdog, for what it's worth, I am currently experimenting with heavy load running 16K LEDs over artnet and the idle watchdog kicked in quite a lot, on the producer and consumer tasks I created, but even the WiFi task reported it now and then. It's working better now due to a number of 'good practices' I believe, mainly managing vTaskDelay(1). You could have a look at my architecture document, scroll down to the end: Architecture. Is this related / helpful to the problems you experience? |
I think for the problems we had here, the watchdog is doing its job correctly - there was an actual code issue causing infinite looping. From an outside view, it's an interesting system design choice to trigger an exception if a program uses 100% CPU for a while. (I looked up what the Arduino core does to manage |
Tested and the previous crash is gone 👌
|
@willmmiles , thx for the feedback, I use that in MoonLight and as in this issue idle watchdog was also mentioned I thought this maybe was related. In ML I am not throttling frames, they go full speed until now. My issues were related to driving 16K and more LEDs over Art-Net, this needs all the power the cpus have, so a more aggressive fight to drive cpu % as high as possible without making the system too angry. @troyhacks knows about this as well 🙂 using it in WLED-MM-Troy. So this is fyi as I don't want to pollute this issue with side talks . But might be an interesting discussion in a separate issue . |
* add NeoESP32RmtHI to lib_ignore for unsupported platforms (secondary safety net) * comment out [env:esp32s3dev_8MB_PSRAM_opi]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
platformio.ini (1)
565-565: Add clarifying comment for intentionally enabled RMTHI on ESP32-S3.Line 565 shows
;-D WLED_USE_SHARED_RMT(commented out, enabling RMTHI). Per PR learnings and comments, S3 is under active investigation for RMTHI watchdog reset issues. This comment should clarify the temporary testing state:💡 Suggested comment clarification
- ;-D WLED_USE_SHARED_RMT ;; un-comment to use the standard RMT driver instead of RMTHI + ;-D WLED_USE_SHARED_RMT ;; RMTHI enabled for S3 testing - under investigation for interrupt watchdog resets (see PR #308)Add explicit comment documenting the temporary RMTHI-enabled state on S3 to inform future maintainers and clarify this is not accidental.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio.ini
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:57.030Z
Learning: In MoonModules/WLED-MM platformio.ini, [esp32_legacy] section intentionally uses NeoPixelBus 2.7.5 while V4 platform builds use 2.7.9 or later. The older V3 Platform builds don't have the same flickering problems that newer versions address, and NPB 2.7.9 provides no added value over 2.7.5 for these legacy builds.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
📚 Learning: 2026-01-05T21:35:51.697Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:436-436
Timestamp: 2026-01-05T21:35:51.697Z
Learning: In MoonModules/WLED-MM/platformio.ini, in the [esp32_legacy] section, pin NeoPixelBus to version 2.7.5. For V4 platform builds, use 2.7.9 or later; older V3 builds do not have the same flickering issues and do not gain from 2.7.9. This rule is specifically for the legacy build section and explains why the older version is intentional there.
Applied to files:
platformio.ini
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ensure ESP32 environment flags related to WLED_USE_SHARED_RMT are consistent across ESP32-S2 and ESP32-S3 (e.g., enable shared RMT on both after investigation). Do not leave mismatched flags as a long-term state; add a clear comment explaining the temporary inconsistency and planned unification, and align RMTHI/RMT driver usage once the interrupt watchdog issue is understood.
Applied to files:
platformio.ini
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.ini
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
platformio.ini
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
platformio.ini
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
platformio.ini
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
platformio.ini
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
platformio.ini
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
platformio.ini
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
platformio.ini
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
platformio.ini
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
platformio.ini
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
platformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
platformio.ini
⏰ 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). (20)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (4)
platformio.ini (4)
322-322: Ensure consistent lib_ignore pattern across ESP8266 environments.Both ESP8266 and ESP32 Legacy sections add
lib_ignore = NeoESP32RmtHIwhen using the standard RMT driver. This is correct—NeoESP32RmtHI should not be compiled when RMTHI is disabled. Confirm this pattern is maintained across all environments that set-D WLED_USE_SHARED_RMT.Also applies to: 371-371
439-439: Missing lib_ignore entries for NeoESP32RmtHI in V4 environments with NPB 2.7.9.Several V4 platform environments (lines 439, 473, 538, 568) update to NeoPixelBus 2.7.9 but do not ignore the NeoESP32RmtHI library. When RMTHI is enabled (no
-D WLED_USE_SHARED_RMTflag), the library should be included. However, when it's disabled, it should be ignored to avoid unused symbol warnings and potential build issues. Verify the build intention for each environment:
- Line 439 (esp32_idf_V4.lib_depsV4): No flag set → RMTHI enabled → lib_ignore should NOT be present ✓
- Line 473 (esp32_idf_V4.lib_deps): No flag set → RMTHI enabled → lib_ignore should NOT be present ✓
- Line 538 (esp32c3.lib_deps): WLED_USE_SHARED_RMT flag set (line 534) → RMTHI disabled → lib_ignore IS present (line 541) ✓
- Line 568 (esp32s3.lib_deps): No flag set (line 565 is commented out) → RMTHI enabled → lib_ignore should NOT be present ✓
Confirm whether the V4 base environments (lines 439, 473) intentionally enable RMTHI by not setting
-D WLED_USE_SHARED_RMT. If RMTHI is expected to be enabled in these environments, the current configuration is correct. If not, add the correspondinglib_ignoreentries.Also applies to: 473-473, 538-538
507-515: Clarify RMT driver configuration for ESP32-S2.Line 507 shows a commented flag:
; -D WLED_USE_SHARED_RMT ;; un-comment to use the standard RMT driver instead of RMTHI. However, based on the PR learnings, ESP32-S2 should use the standard shared RMT driver to avoid boot loops. The current configuration (flag commented out) enables RMTHI, which conflicts with the safety guidance.Recommend either:
- Uncomment the flag (line 507) to use the standard RMT driver on S2, making this consistent with the documented intent to avoid S2 boot issues.
- Add a comment explaining why RMTHI is intentionally enabled on S2 despite prior boot loop issues, if this represents a resolved fix.
Verify the test status of RMTHI on ESP32-S2. Per PR learnings, the S2 watchdog resets were a known issue; confirm whether this has been resolved or if the shared RMT driver should be the default.
819-844: Review large commented-out esp32s3dev_8MB_PSRAM_opi test block.Lines 819–844 contain an extensive commented-out S3 test environment. Per PR learnings, this block includes RMTHI testing and notes the watchdog reset issue (line 834:
; -D WLED_USE_SHARED_RMT ;; RMTHI causes interrupt watchdog reset - needs more investigation).Clarification needed:
- Is this block meant to be preserved for future RMTHI debugging, or should it be removed?
- If preserved, add a GitHub issue reference or inline note explaining the watchdog investigation status.
- If this environment has been superseded by
env:esp32S3_8MB_PSRAM_M(line 2239), consider removing the dead code.Confirm whether this commented-out block serves ongoing debugging purposes. If yes, add a dated comment referencing the tracking issue. If no, remove it to reduce clutter and confusion.
Wled-MM backport of upstream wled#4980
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.