Conversation
Backend ======= - Add D_FastLEDAudio.h Add FastLED audio variables to SharedData - WIP - Add Tubes Layout - Add FastLEDAudioDriver - Add FLAudioEffect
WalkthroughAdds FastLED audio support: new I2S-based FastLEDAudioDriver that populates new shared audio fields, a reactive FLAudioEffect, a TubesLayout, registration updates, a small WaterfallEffect 3D refactor, a PlatformIO config bump, and a minor startup serial change. Changes
Sequence Diagram(s)sequenceDiagram
participant I2S as I2S Audio HW
participant AudioDrv as FastLEDAudioDriver
participant Proc as AudioProcessor
participant Shared as SharedData
participant Effect as FLAudioEffect
participant LEDs as LED Renderer
I2S->>AudioDrv: deliver audio samples
AudioDrv->>Proc: feed samples
Proc->>Proc: analyze (beat, vocals, bands)
Proc->>Shared: update beat, vocalsActive, vocalConfidence, bass/mid/treble
AudioDrv->>Shared: clear transient beat flag each cycle
Effect->>Shared: read audio fields
alt vocalsActive
Effect->>Effect: run vocal-driven visuals
else beat
Effect->>Effect: trigger beat visuals
else
Effect->>Effect: idle rendering
end
Effect->>LEDs: render pixels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (4)
39-42: Unused member variablescurrentBPMandonsetCount.
currentBPMandonsetCountare declared but never read or written anywhere in this class. If they're planned for future use, consider removing them until needed to keep the interface clean (YAGNI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 39 - 42, The members currentBPM (float) and onsetCount (uint32_t) in the D_FastLEDAudio class are declared but never used; remove these two unused member declarations from the class in D_FastLEDAudio.h to keep the interface clean (or, if they are intentionally planned, add a clear TODO comment and minimal read/write usage to avoid unused-variable warnings). Locate the declarations named currentBPM and onsetCount in the D_FastLEDAudio class and delete them (or replace with a commented TODO) and rebuild to ensure no references remain.
21-28: Hardcoded I2S pins and static globals in a header file.Two concerns:
Pin configuration: The
#definemacros for I2S pins (6, 4, 5) are hardcoded and will silently shadow or collide with any same-named macros elsewhere. Consider making these configurable (e.g., viaonUpdate/UI controls, as done for other drivers) or at minimum wrapping them in#ifndefguards.Static globals in a header:
i2sConfig,config, andaudioInputarestaticat file scope — each translation unit including this header gets its own copy. If this header is included from multiple.cppfiles, you'll have duplicated state and theaudioInputshared_ptr won't actually be shared. If single-TU inclusion is guaranteed by the build, this works but is fragile.Consider moving the static variables inside the class (as
static inlinemembers or into the.cppfile), and making pin assignments configurable or at least guarded:- `#define` I2S_CLK_PIN 6 - `#define` I2S_WS_PIN 4 - `#define` I2S_SD_PIN 5 - `#define` I2S_CHANNEL fl::Left - -static fl::AudioConfigI2S i2sConfig(I2S_WS_PIN, I2S_SD_PIN, I2S_CLK_PIN, 0, I2S_CHANNEL, 44100, 16, fl::Philips); -static fl::AudioConfig config(i2sConfig); -static fl::shared_ptr<fl::IAudioInput> audioInput;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 21 - 28, The header hardcodes I2S_PIN macros (I2S_CLK_PIN, I2S_WS_PIN, I2S_SD_PIN) and declares file-scope static objects (i2sConfig, config, audioInput) which cause macro collisions and duplicated state across TUs; change the pin defines to be configurable or at minimum wrap them with `#ifndef` guards (so existing builds can override them) and move i2sConfig, config, and audioInput out of the header into the implementation (or declare them as static inline class members) so there is a single shared instance — update the driver class to accept pin/runtime config (e.g., via onUpdate/UI controls) and instantiate the AudioConfigI2S and shared_ptr<fl::IAudioInput> in the .cpp or as inline static members instead of file-scope statics in the header.
51-83: Callback capture lists are inconsistent — consider uniformity.
onBeat,onVocalStart,onVocalEndcapture[&](by reference), which capturesthisand all locals. These callbacks only accesssharedData(global) and member variables (beatCount,lastBeatTime).onVocalConfidence,onBass,onTrebleuse[](no capture) and only accesssharedData(global).The
[&]captures are safe as long as theFastLEDAudioDriverinstance outlivesaudioProcessor(which it does since it's a member). However, mixing capture styles is slightly inconsistent. TheonVocalStart/onVocalEndlambdas don't need[&]since they only touch the globalsharedData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 51 - 83, The callbacks use mixed capture styles; make them explicit and consistent: change audioProcessor.onBeat's lambda to capture [this] (it modifies member variables beatCount and lastBeatTime) and keep audioProcessor.onVocalStart / onVocalEnd as [this] if they will ever touch members (or change them to [] if they only touch the global sharedData), and leave audioProcessor.onVocalConfidence, onBass, onTreble as [] (no capture) since they only use the global sharedData; update the capture lists for the lambdas referenced by onBeat, onVocalStart, onVocalEnd, onVocalConfidence, onBass, and onTreble accordingly so each lambda explicitly captures only what it needs.
94-94: Stop audio input in destructor to release hardware resources.The
audioInputstatic shared_ptr is started insetup()but never stopped. When the driver is destroyed, the I2S hardware resource remains active. CallaudioInput->stop()(designed per FastLED API for this purpose) to properly release the resource:♻️ Proposed fix
- ~FastLEDAudioDriver() override {} + ~FastLEDAudioDriver() override { + if (audioInput) { + audioInput->stop(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` at line 94, The destructor ~FastLEDAudioDriver() currently does nothing but must stop the shared audioInput resource started in setup(); update the destructor for FastLEDAudioDriver to check the static std::shared_ptr audioInput and call audioInput->stop() (and optionally reset the pointer) to release the I2S hardware resource; locate the static member audioInput and the setup() method for context and ensure the destructor calls audioInput->stop() only if the pointer is non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Around line 207-209: Add the compiler define to force the legacy RMT4 driver:
in the platformio.ini's [moonlight] build_flags (the same place where -D
FASTLED_BUILD is set), append -D FASTLED_RMT5=0 so FastLED uses RMT4 instead of
RMT5; update the build_flags string that currently references FASTLED_BUILD so
it includes this new define to prevent the RMT5 ISR "Handler detached" issues
described.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 47-49: Check the result of fl::IAudioInput::create by verifying
audioInput and errorMsg before calling audioInput->start(); if create fails, log
the descriptive errorMsg (or a fallback message) and avoid calling start so you
don't dereference a null pointer. Add a null guard in the class's loop20ms()
(e.g., if (!audioInput) return;) to skip audio processing when audioInput wasn't
created. Ensure all locations that use audioInput (construction/start and
loop20ms) handle the null case consistently and do not assume successful
creation.
- Around line 86-92: The beat flag is being cleared at the end of loop20ms(), so
any onBeat-set sharedData.beat is erased before FLAudioEffect::loop() can
observe it; move the sharedData.beat = false reset to the start of loop20ms()
(before the while loop that calls audioInput->read() and
audioProcessor.update(sample)) so the flag can be set by the onBeat callback
during processing and persist until the next cycle.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 39-42: The members currentBPM (float) and onsetCount (uint32_t) in
the D_FastLEDAudio class are declared but never used; remove these two unused
member declarations from the class in D_FastLEDAudio.h to keep the interface
clean (or, if they are intentionally planned, add a clear TODO comment and
minimal read/write usage to avoid unused-variable warnings). Locate the
declarations named currentBPM and onsetCount in the D_FastLEDAudio class and
delete them (or replace with a commented TODO) and rebuild to ensure no
references remain.
- Around line 21-28: The header hardcodes I2S_PIN macros (I2S_CLK_PIN,
I2S_WS_PIN, I2S_SD_PIN) and declares file-scope static objects (i2sConfig,
config, audioInput) which cause macro collisions and duplicated state across
TUs; change the pin defines to be configurable or at minimum wrap them with
`#ifndef` guards (so existing builds can override them) and move i2sConfig,
config, and audioInput out of the header into the implementation (or declare
them as static inline class members) so there is a single shared instance —
update the driver class to accept pin/runtime config (e.g., via onUpdate/UI
controls) and instantiate the AudioConfigI2S and shared_ptr<fl::IAudioInput> in
the .cpp or as inline static members instead of file-scope statics in the
header.
- Around line 51-83: The callbacks use mixed capture styles; make them explicit
and consistent: change audioProcessor.onBeat's lambda to capture [this] (it
modifies member variables beatCount and lastBeatTime) and keep
audioProcessor.onVocalStart / onVocalEnd as [this] if they will ever touch
members (or change them to [] if they only touch the global sharedData), and
leave audioProcessor.onVocalConfidence, onBass, onTreble as [] (no capture)
since they only use the global sharedData; update the capture lists for the
lambdas referenced by onBeat, onVocalStart, onVocalEnd, onVocalConfidence,
onBass, and onTreble accordingly so each lambda explicitly captures only what it
needs.
- Line 94: The destructor ~FastLEDAudioDriver() currently does nothing but must
stop the shared audioInput resource started in setup(); update the destructor
for FastLEDAudioDriver to check the static std::shared_ptr audioInput and call
audioInput->stop() (and optionally reset the pointer) to release the I2S
hardware resource; locate the static member audioInput and the setup() method
for context and ensure the destructor calls audioInput->stop() only if the
pointer is non-null.
| -D FASTLED_BUILD=\"20260222\" | ||
| lib_deps = | ||
| https://github.com/FastLED/FastLED#9d0b0eb9b5e59e4093982e0c2bdcfdff72ca80cb ; master 20260221 | ||
| https://github.com/FastLED/FastLED#27c99130c83ab666c4c6dbf389425fec5f27db05 ; master 20260222 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the platformio.ini file around the specified lines and check for build flags
cat -n platformio.ini | head -230 | tail -50Repository: MoonModules/MoonLight
Length of output: 3265
🏁 Script executed:
#!/bin/bash
# Search for all FastLED-related configuration flags in the repo
rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_BUILD|FASTLED_" --type ini --type txt --type h --type cppRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the exact commit details and verify build string alignment
# Find the git repository containing the pinned FastLED commit
rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05" --type iniRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for FASTLED_RMT5, FASTLED_RMT4, and related driver flags without file type restrictions
rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_SPI" .Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the complete [moonlight] section and build_flags to see all configuration
sed -n '199,211p' platformio.iniRepository: MoonModules/MoonLight
Length of output: 811
🏁 Script executed:
#!/bin/bash
# Check all build_flags sections across the entire platformio.ini
rg -B2 -A5 "build_flags|FASTLED" platformio.iniRepository: MoonModules/MoonLight
Length of output: 2738
🏁 Script executed:
#!/bin/bash
# Verify the commit date by examining git if available, or search related documentation
# Check if there are any comments/docs about the FastLED version and RMT configuration
rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05|RMT5|RMT4|Handler detached" .Repository: MoonModules/MoonLight
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Check platform-espressif32 version 55.03.37 for ESP-IDF details
# Look for package.json or version info in typical locations
rg -n "esp-idf|ESP-IDF|5\.[0-9]" platformio.ini | head -20Repository: MoonModules/MoonLight
Length of output: 2966
🏁 Script executed:
#!/bin/bash
# Search for any Arduino core version or framework configuration
rg -n "framework|arduino|framework-esp32idf|framework-espidf" platformio.iniRepository: MoonModules/MoonLight
Length of output: 3009
Add -D FASTLED_RMT5=0 to force legacy RMT4 driver and prevent ISR handler degradation.
The FASTLED_BUILD string aligns correctly with the pinned commit. However, platform 55.03.37 uses ESP-IDF 5.5+ with Arduino core 3.x, which causes FastLED to default to the RMT5 driver. RMT5 has documented ISR handler lifecycle issues (see FastLED issue #1903) causing repeated "Handler detached" messages and severe performance drops (~2 FPS). Add -D FASTLED_RMT5=0 to the [moonlight] build_flags to force the stable legacy RMT4 driver.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` around lines 207 - 209, Add the compiler define to force the
legacy RMT4 driver: in the platformio.ini's [moonlight] build_flags (the same
place where -D FASTLED_BUILD is set), append -D FASTLED_RMT5=0 so FastLED uses
RMT4 instead of RMT5; update the build_flags string that currently references
FASTLED_BUILD so it includes this new define to prevent the RMT5 ISR "Handler
detached" issues described.
There was a problem hiding this comment.
Not true on newest FastLED
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastLED RMT5 driver fixes February 2026 handler detached
💡 Result:
I couldn’t find any FastLED change/issue (through the public repo + recent searches) specifically tied to a “handler detached” message for the ESP32 RMT5 driver in February 2026—so there doesn’t appear to be a known “Feb 2026” targeted fix labeled that way. (If you have the exact log line + platform/core version, I can search more precisely.)
What is documented/fixed recently around ESP32 RMT5 stability:
- FastLED 3.10.1 fixed an ESP32‑S3 RMT5 break where an internal improvement accidentally set DMA mode true, breaking RMT5. Updating beyond 3.10.1 is recommended if you hit DMA-related failures. [1]
- Ongoing RMT5 reliability discussions (especially Wi‑Fi flicker/jitter) note RMT5’s limits and practical mitigations (e.g., raising RMT interrupt priority to the max the driver allows and increasing buffering), but these are about underruns/flicker rather than a “handler detached” teardown error. [2]
- If you need a robust workaround today, FastLED explicitly recommends switching away from RMT5 in some cases:
- Force legacy RMT4 with
-D FASTLED_RMT5=0(typically requires older ESP32 Arduino core / IDF4-era setups; also note RMT4 and RMT5 can’t coexist). [3] - Or use alternative output paths (I2S/SPI/Adafruit bridge), which users/reporting/dev notes often suggest when RMT5 is unstable under load. [3]
- Force legacy RMT4 with
Most actionable next step: confirm whether your “handler detached” happens only with DMA enabled. If yes, try updating FastLED (>= 3.10.1) first. [1] If it persists, try FASTLED_RMT5=0 (RMT4) or switch to a non-RMT driver as above. [3]
Sources:
[1] FastLED releases (3.10.1 notes: DMA mode accidentally set true, breaking RMT5) https://github.com/fastled/fastled/releases
[2] FastLED issue #2082 (RMT5 + Wi‑Fi flicker; buffering/priority limits/knobs discussion) FastLED/FastLED#2082
[3] FastLED repository documentation (RMT4 vs RMT5 selection; FASTLED_RMT5=0; alternative drivers) https://github.com/FastLED/FastLED
@ewowi, thank you for the correction! I appreciate you letting me know that the RMT5 "Handler detached" issues have been resolved in the newest FastLED version.
Since you originally provided the learning about RMT5 issues (learning ID 9bb17d59-f1a1-4505-b40f-39a68c64be29) and you're now confirming it's no longer applicable with the latest FastLED, would you like me to remove that outdated learning from the knowledge base? This will prevent it from being referenced in future reviews.
✏️ Learnings added
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 130
File: platformio.ini:207-209
Timestamp: 2026-02-22T12:37:03.751Z
Learning: FastLED RMT5 driver issues (ISR handler "Handler detached" messages and performance degradation) have been resolved in FastLED versions as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222). The `-D FASTLED_RMT5=0` workaround is no longer needed.
<!-- </add_learning>
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-16T15:53:11.236Z
Learning: For ESP32-PICO-MINI-02 (and similar ESP32 boards with PSRAM), the critical configuration to enable PSRAM is `board_build.arduino.memory_type = qio_qspi` in platformio.ini. The board JSON may already include `-DBOARD_HAS_PSRAM` and `flash_mode: qio`, but without the memory_type setting, PSRAM won't be properly initialized, causing LoadStoreError crashes when code tries to access PSRAM addresses.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 121
File: src/MoonLight/Nodes/Drivers/D_FastLED.h:176-176
Timestamp: 2026-02-08T20:43:12.857Z
Learning: In MoonLight FastLED 3.11 driver (src/MoonLight/Nodes/Drivers/D_FastLED.h): Affinity is applied per-channel via fl::ChannelOptions.mAffinity passed to fl::ChannelConfig (not via FastLED.setExclusiveDriver). Keeping setExclusiveDriver commented out leaves all drivers available; FastLED then selects the appropriate driver for each channel based on the mAffinity preference ("RMT", "I2S", "SPI", "PARLIO", or "" for auto). This allows per-channel driver preferences rather than a global restriction.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T17:07:33.344Z
Learning: FastLED library does not support direct 4-byte-per-pixel (CRGBW) buffers. It always expects CRGB (3-byte) arrays and internally generates the W channel from RGB using .setRgbw() API. Casting a 4-byte RGBW buffer to CRGB* causes misalignment (W becomes next pixel's R). For MoonLight's FastLED driver with channelsPerLight=4: must either (1) repack RGB in-place before FastLED.show() then restore after, (2) use driver-specific channelsPerLight=3 layout, or (3) allocate temporary RGB-only buffer. Option 1 (in-place repacking) is best for ESP32-D0 memory constraints.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
There was a problem hiding this comment.
Yes remove it please
There was a problem hiding this comment.
@ewowi, I've removed the outdated learning about the FastLED RMT5 driver issues. The knowledge base now reflects the current state: that RMT5 issues have been resolved in FastLED as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222), and the -D FASTLED_RMT5=0 workaround is no longer needed. Thank you for keeping the information up to date!
🐰💭
(Learning tidied up!)
/) /)
( •.• )
/>📝✨
⛔ Learnings removed
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-16T15:53:11.236Z
Learning: For ESP32-PICO-MINI-02 (and similar ESP32 boards with PSRAM), the critical configuration to enable PSRAM is `board_build.arduino.memory_type = qio_qspi` in platformio.ini. The board JSON may already include `-DBOARD_HAS_PSRAM` and `flash_mode: qio`, but without the memory_type setting, PSRAM won't be properly initialized, causing LoadStoreError crashes when code tries to access PSRAM addresses.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 130
File: platformio.ini:207-209
Timestamp: 2026-02-22T12:37:03.751Z
Learning: FastLED RMT5 driver issues (ISR handler "Handler detached" messages and performance degradation) have been resolved in FastLED versions as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222). The `-D FASTLED_RMT5=0` workaround is no longer needed.
<!-- </add_learning>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 121
File: src/MoonLight/Nodes/Drivers/D_FastLED.h:176-176
Timestamp: 2026-02-08T20:43:12.857Z
Learning: In MoonLight FastLED 3.11 driver (src/MoonLight/Nodes/Drivers/D_FastLED.h): Affinity is applied per-channel via fl::ChannelOptions.mAffinity passed to fl::ChannelConfig (not via FastLED.setExclusiveDriver). Keeping setExclusiveDriver commented out leaves all drivers available; FastLED then selects the appropriate driver for each channel based on the mAffinity preference ("RMT", "I2S", "SPI", "PARLIO", or "" for auto). This allows per-channel driver preferences rather than a global restriction.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T17:07:33.344Z
Learning: FastLED library does not support direct 4-byte-per-pixel (CRGBW) buffers. It always expects CRGB (3-byte) arrays and internally generates the W channel from RGB using .setRgbw() API. Casting a 4-byte RGBW buffer to CRGB* causes misalignment (W becomes next pixel's R). For MoonLight's FastLED driver with channelsPerLight=4: must either (1) repack RGB in-place before FastLED.show() then restore after, (2) use driver-specific channelsPerLight=3 layout, or (3) allocate temporary RGB-only buffer. Option 1 (in-place repacking) is best for ESP32-D0 memory constraints.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR `#84`: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:#!/bin/bash # First, let's examine the platformio.ini file around the specified lines and check for build flags cat -n platformio.ini | head -230 | tail -50Repository: MoonModules/MoonLight
Length of output: 3265
🏁 Script executed:
#!/bin/bash # Search for all FastLED-related configuration flags in the repo rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_BUILD|FASTLED_" --type ini --type txt --type h --type cppRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash # Check the exact commit details and verify build string alignment # Find the git repository containing the pinned FastLED commit rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05" --type iniRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash # Search for FASTLED_RMT5, FASTLED_RMT4, and related driver flags without file type restrictions rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_SPI" .Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash # Check the complete [moonlight] section and build_flags to see all configuration sed -n '199,211p' platformio.iniRepository: MoonModules/MoonLight
Length of output: 811
🏁 Script executed:
#!/bin/bash # Check all build_flags sections across the entire platformio.ini rg -B2 -A5 "build_flags|FASTLED" platformio.iniRepository: MoonModules/MoonLight
Length of output: 2738
🏁 Script executed:
#!/bin/bash # Verify the commit date by examining git if available, or search related documentation # Check if there are any comments/docs about the FastLED version and RMT configuration rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05|RMT5|RMT4|Handler detached" .Repository: MoonModules/MoonLight
Length of output: 181
🏁 Script executed:
#!/bin/bash # Check platform-espressif32 version 55.03.37 for ESP-IDF details # Look for package.json or version info in typical locations rg -n "esp-idf|ESP-IDF|5\.[0-9]" platformio.ini | head -20Repository: MoonModules/MoonLight
Length of output: 2966
🏁 Script executed:
#!/bin/bash # Search for any Arduino core version or framework configuration rg -n "framework|arduino|framework-esp32idf|framework-espidf" platformio.iniRepository: MoonModules/MoonLight
Length of output: 3009
Add
-D FASTLED_RMT5=0to force legacy RMT4 driver and prevent ISR handler degradation.The FASTLED_BUILD string aligns correctly with the pinned commit. However, platform 55.03.37 uses ESP-IDF 5.5+ with Arduino core 3.x, which causes FastLED to default to the RMT5 driver. RMT5 has documented ISR handler lifecycle issues (see FastLED issue
#1903) causing repeated "Handler detached" messages and severe performance drops (~2 FPS). Add-D FASTLED_RMT5=0to the [moonlight] build_flags to force the stable legacy RMT4 driver.🤖 Prompt for AI Agents
Disabling RMT5 should not be necessary now? The RMT5 driver now allocates more memory during WIFI active mode. Please let me know if this isn't the case. RMT5 will have stress testing either this week if ai credits sustain. Otherwise post friday on refresh.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MoonBase/Nodes.h (1)
350-356: Optional: prefer0.0fliterals for float member initializers.
float vocalConfidence = 0,float bassLevel = 0, andfloat trebleLevel = 0are valid C++ but using integer literals for float members is less explicit than0.0f.✨ Proposed style fix
- float vocalConfidence = 0; - float bassLevel = 0; - float trebleLevel = 0; + float vocalConfidence = 0.0f; + float bassLevel = 0.0f; + float trebleLevel = 0.0f;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Nodes.h` around lines 350 - 356, Update the float member initializers to use explicit float literals: change the initializers for vocalConfidence, bassLevel, and trebleLevel from integer zero (e.g. = 0) to 0.0f; keep the boolean members vocalsActive and beat unchanged. This ensures types are explicit for members vocalConfidence, bassLevel, and trebleLevel in the class/struct where they are declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/MoonBase/Nodes.h`:
- Around line 350-356: Update the float member initializers to use explicit
float literals: change the initializers for vocalConfidence, bassLevel, and
trebleLevel from integer zero (e.g. = 0) to 0.0f; keep the boolean members
vocalsActive and beat unchanged. This ensures types are explicit for members
vocalConfidence, bassLevel, and trebleLevel in the class/struct where they are
declared.
|
@coderabbitai, can you verify this issue FastLED/FastLED#2170 and compare it with the commits in this PR and come with a review summary and propose follow up actions to do in this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: FastLED/FastLED Issue Source: [1] 🏁 Script executed: Length of output: 10286
FastLED Issue
|
|
@coderabbitai, regarding
How can I bring these in the FastLEDAudioDriver class: create them in setup() and destroy in the destructor |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2830
Recommended Approach (using pointers)class FastLEDAudioDriver : public Node {
private:
// Member variables for audio configuration
fl::unique_ptr<fl::AudioConfigI2S> i2sConfig;
fl::unique_ptr<fl::AudioConfig> config;
fl::shared_ptr<fl::IAudioInput> audioInput;
public:
static const char* name() { return "FastLED Audio"; }
static uint8_t dim() { return _NoD; }
static const char* tags() { return "☸️"; }
fl::AudioProcessor audioProcessor;
void setup() override {
Node::setup(); // !!
// Create configuration objects
i2sConfig = fl::make_unique<fl::AudioConfigI2S>(
I2S_WS_PIN, I2S_SD_PIN, I2S_CLK_PIN, 0,
I2S_CHANNEL, 44100, 16, fl::Philips);
config = fl::make_unique<fl::AudioConfig>(*i2sConfig);
// Create audio input using the config
fl::string errorMsg;
audioInput = fl::IAudioInput::create(*config, &errorMsg);
if (!audioInput) {
EXT_LOGE(ML_TAG, "Failed to create audio input: %s", errorMsg.c_str());
return;
}
audioInput->start();
// ... rest of your setup code (callbacks, etc.)
}
void loop20ms() override {
if (!audioInput) return;
sharedData.beat = false;
while (fl::AudioSample sample = audioInput->read()) {
audioProcessor.update(sample);
}
}
~FastLEDAudioDriver() override {
if (audioInput) {
audioInput->stop();
}
// unique_ptr will automatically clean up i2sConfig and config
}
};Alternative: If
|
FastLED Audio driver - Move static variables inside class - add controls (gain etc) - add onUpdate to set controls - add readPins to get I2S pins from IO module - add initAudio and stopAudio - destructor FLAudio effect - add Mid
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Effects/E_FastLED.h (1)
48-48:hueis declared but never used.It looks like a copy from
RainbowEffect. Remove it or add a future use to avoid dead state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_FastLED.h` at line 48, The field uint16_t hue in E_FastLED.h is unused (leftover from RainbowEffect); either delete the hue declaration to remove dead state or implement its intended use where FastLED effect color cycling is computed (e.g., inside the class/method that updates LED colors in this file) — locate the unused symbol "hue" and either remove it or wire it into the effect update routine so it affects color/hue calculations.src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (1)
26-27: Optional: replace raw owning pointers withunique_ptr.
i2sConfigandconfigare manual-new/manual-deleteowning pointers. The currentstartAudio()/stopAudio()/destructor pattern is correct, butstd::unique_ptrwould make ownership self-documenting and eliminate the need for the explicit null-checks anddeletecalls instopAudio().♻️ Proposed refactor
- fl::AudioConfigI2S* i2sConfig = nullptr; - fl::AudioConfig* config = nullptr; + std::unique_ptr<fl::AudioConfigI2S> i2sConfig; + std::unique_ptr<fl::AudioConfig> config;void startAudio() { - i2sConfig = new fl::AudioConfigI2S(pinI2SWS, pinI2SSD, pinI2SSCK, 0, - channel == 1 ? fl::Right : channel == 2 ? fl::Both : fl::Left, 44100, 16, fl::Philips); - config = new fl::AudioConfig(*i2sConfig); + i2sConfig = std::make_unique<fl::AudioConfigI2S>(pinI2SWS, pinI2SSD, pinI2SSCK, 0, + channel == 1 ? fl::Right : channel == 2 ? fl::Both : fl::Left, 44100, 16, fl::Philips); + config = std::make_unique<fl::AudioConfig>(*i2sConfig); ... } void stopAudio() { if (audioInput) { audioInput->stop(); audioInput.reset(); } - if (config) { delete config; config = nullptr; } - if (i2sConfig) { delete i2sConfig; i2sConfig = nullptr; } + config.reset(); + i2sConfig.reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 26 - 27, Replace the raw owning pointers i2sConfig and config with std::unique_ptr to make ownership explicit: change their declarations to std::unique_ptr<fl::AudioConfigI2S> i2sConfig and std::unique_ptr<fl::AudioConfig> config, allocate with std::make_unique (or reset) inside startAudio(), and remove explicit delete/null-check logic from stopAudio() and the destructor (use reset() if explicit teardown is needed). Update any pointer usages to dereference the unique_ptr (->) and ensure stopAudio() sets the unique_ptrs to nullptr via reset() if you need to release before scope exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Line 153: The log string passed to EXT_LOGI in the D_FastLEDAudio driver
contains an extra closing parenthesis inside the message "(re)creating
audioInput)"; remove the stray ')' so the call to EXT_LOGI(ML_TAG, "(re)creating
audioInput") prints the correct message. Locate the EXT_LOGI usage in
D_FastLEDAudio (the log call shown) and edit the string literal to drop the
unmatched parenthesis.
- Around line 81-86: Fix the two incorrect log formats in the audio callbacks:
in the audioProcessor.onMid lambda change the log label from "onBass" to "onMid"
so it matches the callback (reference: audioProcessor.onMid and
sharedData.midLevel, ML_TAG), and in the onVocalConfidence log change the printf
format specifier from %d to %f to match the float argument (reference:
audioProcessor.onVocalConfidence). Update those commented log lines accordingly
so they are correct when uncommented.
- Line 51: The lambda passed to moduleIO->addUpdateHandler in FastLEDAudioDriver
captures this and is registered with allowRemove=false causing a permanent
callback to readPins(), risking use-after-free; change the registration to store
the returned handler ID (from addUpdateHandler) as a member (e.g.,
updateHandlerId_), and in the FastLEDAudioDriver destructor call
moduleIO->removeUpdateHandler(updateHandlerId_) (or re-register with
allowRemove=true) to ensure the callback is removed before the object is
destroyed; update the class to hold the handler ID and perform removal in the
destructor (or clearly document framework guarantees if you choose not to
remove).
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Line 82: Guard against division-by-zero when computing the decrement for
beatLevel: ensure layer->size.y is not zero before using it in 255 /
layer->size.y (for example compute an integer divisor like int divisor =
layer->size.y ? layer->size.y : 1; or use a conditional that substitutes a safe
value) and then use that safe divisor in the MIN expression that updates
beatLevel (referencing beatLevel and layer->size.y in the existing line). This
prevents a hardware exception on Xtensa while preserving the original behavior
when size.y > 0.
- Around line 68-73: The drawLine calls in E_FastLED.h pass columnNr and
columnNr++ in the same expression which causes undefined/unspecified behavior;
fix each call by separating the increment from the function call so x1 and x2
are deterministic (e.g., call layer->drawLine(columnNr, ..., columnNr, ...) and
then increment columnNr on the next line), updating the four drawLine
invocations that use columnNr/columnNr++ to ensure columnNr is only read or
written once per expression.
- Around line 59-79: The drawLine calculations can divide by zero because
maxBass/maxMid/maxTreble/maxVocal may be 0 and the decay can undershoot below 0;
fix by clamping the maxima and decays: before each drawLine that uses e.g.
maxBass, ensure a safeDenom = (maxBass > 0.0001f) ? maxBass : 1.0f (or skip
drawing when both sharedData.bassLevel and maxBass are 0), and replace uses of
"maxX -= 0.01" with "maxX = max(0.0f, maxX - 0.01f)"; apply the same pattern for
maxMid, maxTreble, maxVocal and use those safeDenom variables in the
layer->drawLine calls referenced in this block.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 26-27: Replace the raw owning pointers i2sConfig and config with
std::unique_ptr to make ownership explicit: change their declarations to
std::unique_ptr<fl::AudioConfigI2S> i2sConfig and
std::unique_ptr<fl::AudioConfig> config, allocate with std::make_unique (or
reset) inside startAudio(), and remove explicit delete/null-check logic from
stopAudio() and the destructor (use reset() if explicit teardown is needed).
Update any pointer usages to dereference the unique_ptr (->) and ensure
stopAudio() sets the unique_ptrs to nullptr via reset() if you need to release
before scope exit.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Line 48: The field uint16_t hue in E_FastLED.h is unused (leftover from
RainbowEffect); either delete the hue declaration to remove dead state or
implement its intended use where FastLED effect color cycling is computed (e.g.,
inside the class/method that updates LED colors in this file) — locate the
unused symbol "hue" and either remove it or wire it into the effect update
routine so it affects color/hue calculations.
| addControlValue("Right"); | ||
| addControlValue("Both"); | ||
|
|
||
| moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check handler API definitions and usage patterns
rg -n "addUpdateHandler\|removeUpdateHandler" --type=h --type=cpp -A 3 -B 1 | head -100Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Find and examine the D_FastLEDAudio class definition and destructor
fd "D_FastLEDAudio" --type f | xargs cat -n | head -200Repository: MoonModules/MoonLight
Length of output: 7523
🏁 Script executed:
#!/bin/bash
# Search for all destructor implementations in driver files to see the pattern
rg -n "~D_\w+\(\)" --type=h --type=cpp -A 5 | head -150Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Get full line count of the file and check if it continues beyond line 200
wc -l src/MoonLight/Nodes/Drivers/D_FastLEDAudio.hRepository: MoonModules/MoonLight
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Get lines 200 onwards to see if destructor exists
tail -100 src/MoonLight/Nodes/Drivers/D_FastLEDAudio.hRepository: MoonModules/MoonLight
Length of output: 2729
🏁 Script executed:
#!/bin/bash
# Find the Node base class definition
fd "Node\.h" --type f -x cat -n {} \; | head -200Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for moduleIO type and handler methods across the codebase
rg "moduleIO.*addUpdateHandler\|class.*Node.*{" --type=h --type=cpp -B 2 -A 2 | head -150Repository: MoonModules/MoonLight
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Find Node class definition
fd "Node" --type f | grep -E "\.(h|cpp)$" | head -20Repository: MoonModules/MoonLight
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Search for Node class definition directly
rg "class Node" --type=h -A 10 | head -100Repository: MoonModules/MoonLight
Length of output: 2220
🏁 Script executed:
#!/bin/bash
# Search for addUpdateHandler definition or declaration
rg "addUpdateHandler" --type=h --type=cpp -B 3 -A 5 | head -200Repository: MoonModules/MoonLight
Length of output: 14747
🏁 Script executed:
#!/bin/bash
# Search for removeUpdateHandler or similar removal mechanism
rg "removeUpdateHandler\|remove.*Handler\|erase.*Handler" --type=h --type=cpp -B 2 -A 3 | head -150Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look at StatefulService to understand handler lifecycle
cat -n lib/framework/StatefulService.h | head -150Repository: MoonModules/MoonLight
Length of output: 5876
🏁 Script executed:
#!/bin/bash
# Search for Node destruction and module cleanup
rg "~Node|destroy.*node|NodeManager.*destroy" --type=h --type=cpp -A 5 | head -150Repository: MoonModules/MoonLight
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Check if there's cleanup logic for handlers when modules are destroyed
rg "callUpdateHandlers\|_updateHandlers" --type=h --type=cpp -B 2 -A 5 | head -200Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check destructor patterns in other driver files
rg "~D_\w+\(\)" --type=h -A 10 | head -250Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for Module base class and its lifecycle
fd "Module\.h" --type f -x head -100 {} \;Repository: MoonModules/MoonLight
Length of output: 3437
Prevent use-after-free when FastLEDAudioDriver is destroyed while handler is registered.
The addUpdateHandler at line 51 registers a lambda that captures this with allowRemove=false, making it permanently unremovable. The destructor does not unregister the handler. If moduleIO outlives the driver instance, the callback will invoke readPins() on a dangling pointer.
Either store the handler ID and call removeUpdateHandler() in the destructor with allowRemove=true, or document the framework guarantee that moduleIO callbacks are cancelled before node destruction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` at line 51, The lambda passed
to moduleIO->addUpdateHandler in FastLEDAudioDriver captures this and is
registered with allowRemove=false causing a permanent callback to readPins(),
risking use-after-free; change the registration to store the returned handler ID
(from addUpdateHandler) as a member (e.g., updateHandlerId_), and in the
FastLEDAudioDriver destructor call
moduleIO->removeUpdateHandler(updateHandlerId_) (or re-register with
allowRemove=true) to ensure the callback is removed before the object is
destroyed; update the class to hold the handler ID and perform removal in the
destructor (or clearly document framework guarantees if you choose not to
remove).
| audioProcessor.onMid([](float level) { | ||
| if (level > 0.01f) { | ||
| sharedData.midLevel = level; | ||
| // EXT_LOGD(ML_TAG, "onBass: %f", level); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Minor: copy-paste log label in onMid callback says "onBass".
Line 84 reads "onBass: %f" inside the onMid lambda — a copy-paste leftover. Also, the onVocalConfidence log (line 71) uses %d for a float argument; should be %f. Both are currently commented out, but worth fixing before uncomment.
🔧 Proposed fix
audioProcessor.onMid([](float level) {
if (level > 0.01f) {
sharedData.midLevel = level;
- // EXT_LOGD(ML_TAG, "onBass: %f", level);
+ // EXT_LOGD(ML_TAG, "onMid: %f", level);
}
}); audioProcessor.onVocalConfidence([](float confidence) {
sharedData.vocalConfidence = sharedData.vocalsActive ? confidence : 0.0;
- // EXT_LOGD(ML_TAG, "onVocalConfidence %d", confidence);
+ // EXT_LOGD(ML_TAG, "onVocalConfidence %f", confidence);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| audioProcessor.onMid([](float level) { | |
| if (level > 0.01f) { | |
| sharedData.midLevel = level; | |
| // EXT_LOGD(ML_TAG, "onBass: %f", level); | |
| } | |
| }); | |
| audioProcessor.onMid([](float level) { | |
| if (level > 0.01f) { | |
| sharedData.midLevel = level; | |
| // EXT_LOGD(ML_TAG, "onMid: %f", level); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 81 - 86, Fix the
two incorrect log formats in the audio callbacks: in the audioProcessor.onMid
lambda change the log label from "onBass" to "onMid" so it matches the callback
(reference: audioProcessor.onMid and sharedData.midLevel, ML_TAG), and in the
onVocalConfidence log change the printf format specifier from %d to %f to match
the float argument (reference: audioProcessor.onVocalConfidence). Update those
commented log lines accordingly so they are correct when uncommented.
| } | ||
| } | ||
| if (i2sPinsChanged && pinI2SSD != UINT8_MAX && pinI2SWS != UINT8_MAX && pinI2SSCK != UINT8_MAX) { | ||
| EXT_LOGI(ML_TAG, "(re)creating audioInput)"); |
There was a problem hiding this comment.
Minor: stray closing parenthesis in log string.
"(re)creating audioInput)" has an unmatched ) at the end.
🔧 Proposed fix
- EXT_LOGI(ML_TAG, "(re)creating audioInput)");
+ EXT_LOGI(ML_TAG, "(re)creating audioInput");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EXT_LOGI(ML_TAG, "(re)creating audioInput)"); | |
| EXT_LOGI(ML_TAG, "(re)creating audioInput"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` at line 153, The log string
passed to EXT_LOGI in the D_FastLEDAudio driver contains an extra closing
parenthesis inside the message "(re)creating audioInput)"; remove the stray ')'
so the call to EXT_LOGI(ML_TAG, "(re)creating audioInput") prints the correct
message. Locate the EXT_LOGI usage in D_FastLEDAudio (the log call shown) and
edit the string literal to drop the unmatched parenthesis.
| if (sharedData.bassLevel > maxBass) maxBass = sharedData.bassLevel; | ||
| if (sharedData.midLevel > maxMid) maxMid = sharedData.midLevel; | ||
| if (sharedData.trebleLevel > maxTreble) maxTreble = sharedData.trebleLevel; | ||
| if (sharedData.vocalConfidence > maxVocal) maxVocal = sharedData.vocalConfidence; | ||
|
|
||
| if (sharedData.beat) beatLevel = 255; | ||
|
|
||
| // EXT_LOGD(ML_TAG, "%f-%d %f-%d %d-%d %f-%d", sharedData.bassLevel, bassLevel, sharedData.trebleLevel, trebleLevel, sharedData.beat, beatLevel, sharedData.vocalsActive ? sharedData.vocalConfidence : 0, vocalsLevel); | ||
|
|
||
| uint8_t columnNr = 0; | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); | ||
|
|
||
| // correct for lower output | ||
| if (maxBass > 0) maxBass -= 0.01; | ||
| if (maxMid > 0) maxMid -= 0.01; | ||
| if (maxTreble > 0) maxTreble -= 0.01; | ||
| if (maxVocal > 0) maxVocal -= 0.01; |
There was a problem hiding this comment.
Critical: Division by zero when max levels are zero, and decay undershoots zero.
Two related issues:
-
Divide by zero: All four
maxXvalues start at0.0fand are only increased whensharedData.xLevel > maxX. While the audio driver hasn't delivered any signal yet (or when all levels remain0.0fbecause the driver skips values ≤ 0.01),maxBass == 0, anddrawLine(…, … * sharedData.bassLevel / maxBass, …)produces NaN or ±∞ as a float coordinate. Casting that to an integer (for the LED index) is undefined behaviour, and on Xtensa/ESP32 typically resolves toINT_MINor 0 — either an out-of-bounds LED write or a silent visual glitch. -
Decay undershoots zero:
if (maxBass > 0) maxBass -= 0.01fwill produce a small negative value (e.g.maxBass = 0.005f→maxBass = -0.005f) when the step is larger than the remaining value. On the very next iteration,sharedData.bassLevel (0.0f) > -0.005fistrue, somaxBassis set to0.0fand the divide-by-zero recurs.
🐛 Proposed fix
- if (sharedData.bassLevel > maxBass) maxBass = sharedData.bassLevel;
- if (sharedData.midLevel > maxMid) maxMid = sharedData.midLevel;
- if (sharedData.trebleLevel > maxTreble) maxTreble = sharedData.trebleLevel;
- if (sharedData.vocalConfidence > maxVocal) maxVocal = sharedData.vocalConfidence;
+ if (sharedData.bassLevel > maxBass) maxBass = sharedData.bassLevel;
+ if (sharedData.midLevel > maxMid) maxMid = sharedData.midLevel;
+ if (sharedData.trebleLevel > maxTreble) maxTreble = sharedData.trebleLevel;
+ if (sharedData.vocalConfidence > maxVocal) maxVocal = sharedData.vocalConfidence;
...
- if (maxBass > 0) maxBass -= 0.01;
- if (maxMid > 0) maxMid -= 0.01;
- if (maxTreble > 0) maxTreble -= 0.01;
- if (maxVocal > 0) maxVocal -= 0.01;
+ maxBass = fmaxf(0.0f, maxBass - 0.01f);
+ maxMid = fmaxf(0.0f, maxMid - 0.01f);
+ maxTreble = fmaxf(0.0f, maxTreble - 0.01f);
+ maxVocal = fmaxf(0.0f, maxVocal - 0.01f);And guard the drawLine calls:
+ if (maxBass > 0.0f)
layer->drawLine(columnNr, layer->size.y - 1, columnNr, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red);
+ columnNr++;(apply the same pattern to mid, treble, vocal)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h` around lines 59 - 79, The drawLine
calculations can divide by zero because maxBass/maxMid/maxTreble/maxVocal may be
0 and the decay can undershoot below 0; fix by clamping the maxima and decays:
before each drawLine that uses e.g. maxBass, ensure a safeDenom = (maxBass >
0.0001f) ? maxBass : 1.0f (or skip drawing when both sharedData.bassLevel and
maxBass are 0), and replace uses of "maxX -= 0.01" with "maxX = max(0.0f, maxX -
0.01f)"; apply the same pattern for maxMid, maxTreble, maxVocal and use those
safeDenom variables in the layer->drawLine calls referenced in this block.
| uint8_t columnNr = 0; | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); |
There was a problem hiding this comment.
Critical: columnNr++ as a function argument alongside columnNr is undefined/unspecified behaviour.
Each drawLine call passes columnNr (arg 1, x1) and columnNr++ (arg 3, x2) in the same expression. In C++14 and earlier this is undefined behaviour (unsequenced read and modification). In C++17 it is indeterminately sequenced — the compiler may evaluate arg 3 before arg 1, giving drawLine(1, …, 0, …) instead of the intended drawLine(0, …, 0, …). Since x1 ≠ x2 in that ordering, drawLine renders a diagonal segment rather than a vertical bar, and columnNr itself is left at an unexpected value for subsequent calls.
Separate the increment from the call:
🐛 Proposed fix
- uint8_t columnNr = 0;
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple);
+ uint8_t col = 0;
+ if (maxBass > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red);
+ ++col;
+ if (maxMid > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange);
+ ++col;
+ if (maxTreble > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green);
+ ++col;
+ if (maxVocal > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue);
+ ++col;
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint8_t columnNr = 0; | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); | |
| uint8_t col = 0; | |
| if (maxBass > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | |
| +col; | |
| if (maxMid > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | |
| +col; | |
| if (maxTreble > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | |
| +col; | |
| if (maxVocal > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | |
| +col; | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h` around lines 68 - 73, The drawLine
calls in E_FastLED.h pass columnNr and columnNr++ in the same expression which
causes undefined/unspecified behavior; fix each call by separating the increment
from the function call so x1 and x2 are deterministic (e.g., call
layer->drawLine(columnNr, ..., columnNr, ...) and then increment columnNr on the
next line), updating the four drawLine invocations that use columnNr/columnNr++
to ensure columnNr is only read or written once per expression.
| if (maxVocal > 0) maxVocal -= 0.01; | ||
|
|
||
| // beat delay | ||
| if (beatLevel) beatLevel -= MIN(255/layer->size.y, beatLevel); |
There was a problem hiding this comment.
Minor: integer division by zero if layer->size.y == 0.
255 / layer->size.y is integer division and will trigger a hardware exception on Xtensa if size.y is ever zero. Add a guard or substitute the formula.
🛡️ Proposed fix
- if (beatLevel) beatLevel -= MIN(255/layer->size.y, beatLevel);
+ if (beatLevel && layer->size.y > 0)
+ beatLevel -= MIN(255 / layer->size.y, beatLevel);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h` at line 82, Guard against
division-by-zero when computing the decrement for beatLevel: ensure
layer->size.y is not zero before using it in 255 / layer->size.y (for example
compute an integer divisor like int divisor = layer->size.y ? layer->size.y : 1;
or use a conditional that substitutes a safe value) and then use that safe
divisor in the MIN expression that updates beatLevel (referencing beatLevel and
layer->size.y in the existing line). This prevents a hardware exception on
Xtensa while preserving the original behavior when size.y > 0.
|
@coderabbitai, can you make an analysis of all the readPins() functions in the repository and come with proposals for improvement as it looks like a lot of similar code is done in all of them. Goal is to minimize the code in specific modules or nodes. The recurring pattern is now: each time local variables for pins are created, checks are done if the state.data["pins"] for these variables have changed and if so stop and start things using these pins. This should be done with minimal code on each place where such a mechanism is needed. This is one of the patterns . ,maybe there are more |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20959
Current State AnalysisFound 5 Common Boilerplate (100% duplicated):
Pattern Variations:
Proposed Solution: PinReader UtilityCreate a declarative, reusable pin reading system: 🏁 Script executed: Length of output: 670 🎯 Proposed Solution:
|
| Benefit | Impact |
|---|---|
| Code reduction | 67-85% fewer lines per implementation |
| Consistency | Same validation & error handling everywhere |
| Maintainability | Fix once, benefit everywhere |
| Readability | Declarative intent vs imperative loops |
| Type safety | Compile-time checks on validators |
| Testing | Test PinReader once vs 5+ implementations |
4. Migration Strategy
Phase 1: Add PinReader utility (this PR or follow-up)
- Create
src/MoonBase/PinReader.h - No breaking changes
Phase 2: Refactor one-by-one
- Start with simplest:
D_Infrared.h - Then:
D_FastLEDAudio.h - Then:
ModuleLightsControl.h,ModuleDrivers.h - Finally: Complex
ModuleIO.h
Phase 3: Extend as needed
- Add support for additional validation types
- Add logging levels as parameters
- Consider async pin updates
5. Alternative: Even Simpler Macro-Based Approach
If C++ lambdas feel too heavy, here's a macro-based alternative:
`#define` READ_PIN_WITH_CHANGE(moduleIO, usage, pinVar, restartFlag) \
do { \
uint8_t __old = pinVar; \
pinVar = PinReader::readSinglePin(moduleIO, usage, GPIO_IS_VALID_GPIO); \
if (pinVar != __old && pinVar != UINT8_MAX) restartFlag = true; \
} while(0)
// Usage:
bool changed = false;
READ_PIN_WITH_CHANGE(moduleIO, pin_I2S_SD, pinI2SSD, changed);
READ_PIN_WITH_CHANGE(moduleIO, pin_I2S_WS, pinI2SWS, changed);
if (changed) restart();🎬 Recommendation
Option 1 (Recommended): Implement the PinReader class-based approach
- More flexible, extensible
- Better for complex scenarios (multiple pins, arrays)
- Easier to test and debug
Option 2: Start with helper functions for common cases
readSinglePin(),readPinArray()only- Simpler, less boilerplate
Would you like me to:
- ✅ Generate the complete
PinReader.himplementation with full error handling? - ✅ Create a refactored version of one file (e.g.,
D_FastLEDAudio.h) as proof-of-concept? - ✅ Plan this as a separate PR with step-by-step migration?
2 participants
Summary by CodeRabbit
New Features
Improvements