pre 71 fixes #107
Open
pre 71 fixes #107
+272
−216
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
backend ======= - moduleIO: add DigNext2, readPins safeMode check - Physical layer: loop: check nrOfChannels < maxChannels - moduleDrivers, lights control, infrared: readPins safeMode check - MM effects GOL: return if no data allocated
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Board support & installer src/MoonBase/Modules/ModuleIO.h, docs/gettingstarted/installer.md, docs/moonbase/inputoutput.md, firmware/installer/manifest_esp32-d0-pico2.json, firmware/esp32-d0.ini |
Added QuinLEDDigNext2 enum/preset and esp32-pico2 installer manifest/env; reordered QuinLED presets and updated installer docs; duplicate PlatformIO env inserted. |
Safe-mode guards (pin init) src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Modules/ModuleLightsControl.h, src/MoonLight/Nodes/Drivers/D_Infrared.h, src/MoonBase/Modules/ModuleIO.h |
Added early returns when safeModeMB is true in various readPins() implementations; each logs a warning and skips pin discovery/configuration. |
Effects — 3D plane updates & memory safety src/MoonLight/Nodes/Effects/E_WLED.h, src/MoonLight/Nodes/Effects/E_MoonLight.h, src/MoonLight/Nodes/Effects/E_MoonModules.h |
Replaced single-LED writes with X/Z plane iterations for many effects; switched several counters to size_t; introduced named alloc/free via reallocMB2/freeMB(..., name()); added null-checks and safer allocation flows. |
Virtual layer bounds & mapping src/MoonLight/Layers/VirtualLayer.cpp, src/MoonLight/Layers/VirtualLayer.h |
Added bounds checks for unmapped reads/writes with warnings and safe defaults; mappingTableSize changed to size_t. |
Layer buffering & synchronization src/MoonLight/Layers/PhysicalLayer.cpp, src/main.cpp |
Moved per-buffer zeroing to occur under mutex after sync/delay; added header vs maxChannels guard; moved mutex release earlier in effectTask so memcpy runs after release (critical section boundary changed). |
Utilities & logging src/MoonBase/Utilities.cpp, src/MoonBase/Utilities.h |
Replaced raw Serial message with structured log (EXT_LOGE); standardized alloc/realloc/free log defaults; added reallocMB2 PSRAM-aware helper. |
Module IO / board pin mappings src/MoonBase/Modules/ModuleIO.h |
Reworked QuinLED board enum/preset ordering to include QuinLEDDigNext2 and adjusted pin assignment branches accordingly. |
Modifiers & API references src/MoonLight/Nodes/Modifiers/M_MoonLight.h, src/MoonLight/Modules/ModuleEffects.h, docs/moonlight/modifiers.md |
Renamed RippleYZ → RippleXZ, updated defaults (MirrorModifier mirrorY/Z → true), and replaced registrations/usages to RippleXZ. |
Drivers & node init logging src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Modules/ModuleEffects.h, src/MoonBase/Nodes.cpp |
Small comment additions and re-enabled debug log in Node::updateControl; added init comment for onSizeChanged calls. |
Other effects & scripts src/MoonLight/Nodes/Effects/E_MoonModules.h, src/MoonLight/Nodes/Effects/E_MoonLight.h, misc/livescripts/E_lines.sc |
Multiple effect-specific memory/logging adjustments; changed fadeToBlackBy value in a script. |
Sequence Diagram(s)
(Skipped — changes are broad and dispersed; no single concise multi-component sequence added.)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Board presets p4 #74: Overlaps ModuleIO board enum and QuinLED preset/pin mapping changes.
- double buffering #84: Related double-buffering, swapMutex and PhysicalLayer/onLayoutPre synchronization edits.
- Drive lots of LEDs (130K max) #93: Overlaps VirtualLayer/PhysicalLayer bounds checks and PSRAM-aware allocation helper usage.
Poem
🐇 I hopped through pins and boards anew,
DigNext2 joined the blinking crew,
Safe-mode stops my curious paws,
Memory tidy, buffers pause,
Planes of light now hop and view ✨
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 15.19% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The PR title 'pre 71 fixes' is vague and does not clearly convey the main changes or objectives. It references an issue number without descriptive context. | Use a more descriptive title that summarizes the primary fix, such as 'Fix GameOfLife rendering and synchronization race conditions' or similar. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 1655-1658: The inner loop in E_WLED.h shadows the outer zone index
variable named 'z'; rename the inner loop variable (e.g., to 'zi' or 'zz') and
update its usage inside that loop (the for (int z = 0; z < layer->size.z; z++)
header and the Coord3D(x, pos + led, z) call) so the outer 'z' remains distinct;
ensure you only change the inner loop identifier and its references within that
loop (including layer->setRGB) to the new name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/gettingstarted/installer.mdsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Utilities.cppsrc/MoonBase/Utilities.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Drivers/D_Infrared.hsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Effects/E_MoonModules.h
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Effects/E_MoonModules.h
📚 Learning: 2026-01-13T15:18:36.992Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:36.992Z
Learning: Ensure in code reviews that the three mutexes (effectsMutex, driversMutex, layerMutex) are created successfully at initialization time (checks in constructors log errors if xSemaphoreCreateMutex returns nullptr). Do not add runtime null checks for these mutex pointers after initialization, as the design treats allocation as an invariant. This pattern should be verified across MoonLight PhysicalLayer.cpp and MoonBase Nodes headers; consider documenting the invariant and ensuring no code path dereferences null mutex pointers. If extending, apply to all similar critical resources initialized during boot.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Effects/E_MoonModules.h
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonModules.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T15:24:11.096Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:11.096Z
Learning: In VirtualLayer.cpp (setLight and getLight), the unmapped path intentionally disables bounds checking when indexV >= mappingTableSize or oneToOneMapping is true to optimize performance for very large LED counts (100K+). This is a deliberate design decision; only rely on this behavior if you can guarantee calls stay within bounds. Document this contract and avoid adding defensive checks in the hot path. Use explicit bounds checks in all other code paths or when the input cannot be guaranteed to be in-range.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
src/MoonBase/Utilities.h (1)
Coord3D(150-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
docs/gettingstarted/installer.md (1)
24-24: LGTM! Improved documentation formatting.The revised LED range notation is more concise and professional. Changing the exclamation mark to a colon is more appropriate for introducing a hyperlink in technical documentation.
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
57-59: Behavioral change: Y and Z mirroring now enabled by default.This change makes the MirrorModifier apply mirroring on all three axes by default, which is more consistent than before (only X was true). However, this is a breaking change for existing users who relied on the previous defaults where
mirrorYandmirrorZwerefalse.The implementation is correct - just confirming this default change is intentional.
src/MoonBase/Utilities.cpp (1)
91-91: LGTM! Structured logging improvement.The change from direct hardware Serial logging to the
EXT_LOGEmacro aligns with the structured logging pattern used consistently throughout this file and the broader codebase.src/MoonLight/Layers/PhysicalLayer.cpp (1)
86-86: Good defensive guard for allocation failure.The early return prevents layer processing when memory allocation has failed (
maxChannelsis set to 0 on failure at line 77). This aligns with the broader safe-mode guard patterns being introduced across the codebase.One minor observation: the
>=comparison will also skip processing whennrOfChannelsexactly equalsmaxChannels. Verify this is intentional, as full capacity might still be a valid operational state.src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
41-44: LGTM! Safe mode guard prevents hardware initialization.The early return when
safeModeMBis active correctly prevents IR pin discovery and RMT channel configuration. This aligns with the consistent safe-mode guard pattern being introduced across multiple modules in this PR.src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
184-198: Good fix: Memory freed before size recomputation.The reordering correctly frees existing buffers before computing
dataSizeand reallocating, preventing use-after-free issues.Minor consideration: if allocation partially succeeds (e.g.,
cellsallocated butfutureCellsfails), the successfully allocated buffers remain until the destructor. The guard at line 204 inloop()prevents invalid access, and the destructor properly cleans up, so this is safe—but the intermediate state has orphaned allocations. If memory pressure is a concern, consider freeing successfully allocated buffers on partial failure.src/MoonLight/Layers/VirtualLayer.cpp (1)
170-176: LGTM! Documentation and formatting improvements.The added
// no mappingcomments clarify the else-branch purpose insetLightandgetLight. The log statement reformatting maintains identical behavior. No functional changes.Also applies to: 218-227, 325-325
src/MoonBase/Utilities.h (1)
195-220: Logging refinements look good.The consistent use of
"x"as the default name placeholder acrossallocMB,reallocMB, andfreeMBimproves log message clarity. The format string changes ("for %s of ...") provide better context in error scenarios.src/MoonLight/Nodes/Effects/E_WLED.h (1)
1146-1149: 3D expansion for PopCornEffect looks correct.The nested loop correctly expands the effect to fill the entire x-z plane at each y-position (
ledIndex), making this 1D effect work properly in 3D layouts.src/MoonBase/Modules/ModuleIO.h (5)
75-76: New QuinLED board enum values added correctly.The new
board_QuinLEDDig2Goandboard_QuinLEDDigNext2entries are properly placed in the enum and maintain sequential ordering with other QuinLED variants.
118-119: UI labels match enum ordering.The dropdown options correctly correspond to the enum values defined at lines 75-76.
371-390: QuinLED DigNext2 board configuration.The pin assignments appear reasonable for this board variant. The commented-out I2S_SCK (line 382) is acceptable as a work-in-progress marker.
391-426: Reworked QuinLED board configurations look correct.The pin assignments for DigUnoV3, DigQuadV3, and DigOctaV2 are properly structured with appropriate LED pins, buttons, and relay configurations for each board variant.
630-633: Safe mode guard prevents pin configuration during recovery.This early return pattern is consistent with other modules (
ModuleDrivers.h,ModuleLightsControl.h) and provides a unified approach to safe mode handling across the codebase.src/MoonLight/Modules/ModuleDrivers.h (1)
35-38: Safe mode guard is consistent with other modules.The early return prevents LED pin configuration during safe mode, which aligns with the broader PR changes adding safe-mode guards across
ModuleIO.handModuleLightsControl.h.src/MoonLight/Modules/ModuleLightsControl.h (1)
218-221: Safe mode guard prevents relay and button pin initialization.This completes the consistent safe-mode guard pattern across all
readPins()implementations (ModuleIO.h,ModuleDrivers.h, and this file), ensuring GPIO configurations are skipped during recovery mode.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Collaborator
Author
|
@coderabbitai, I have a problem with the GameOfLifeEffect. It works fine about a second and then the effect goes off both on the monitor and on the LEDs display and sometime you see a frame appearing, sometimes only part of the effect. Other effects don't seem to have an issue. I tried earlier commit but they do seem to have the same problem. Very odd! Can you do an analysis what could be going on? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4756 🏁 Script executed: Length of output: 13758 🏁 Script executed: Length of output: 19973 🏁 Script executed: Length of output: 529 🏁 Script executed: Length of output: 1667
Root CauseThe problem is the new guard added in PhysicalLayer.cpp at line 86 in this PR: void PhysicalLayer::loop() {
if (lights.header.nrOfChannels >= lights.maxChannels) return; // in case alloc mem is not successfulThis guard is stopping ALL layer and effect processing when
Why It HappensAfter about 1 second, something is causing
SolutionThe guard is too aggressive. You have a few options:
🏁 Script executed: Length of output: 7631 Perfect! Now I can see the exact bug: The BugLooking at the code flow:
The problem: If allocation failed, Even if allocation succeeded, once The FixThe guard is checking the wrong condition. It should check if allocation failed, not if capacity is exceeded: void PhysicalLayer::loop() {
if (lights.maxChannels == 0) return; // in case alloc mem is not successful
// OR even better:
// if (!lights.channelsE) return; // in case alloc mem is not successfulThis way:
Would you like me to create a fix for this issue? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, Regarding
No this is not the problem, I removed it and still have it. I think it is related to the mutexes we added earlier. What is really weird : if I refresh in the browser, sometimes I see one frame appearing on the display, after that immediately gone. So check swapMutex, updateMutex etc. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 17849 🏁 Script executed: Length of output: 13125
The BugThe
The problem is in ModuleLightsControl.h lines 515-525: if (isPositions == 2) { // send to UI
if (_socket->getActiveClients() && _state.data["monitorOn"]) {
// ... send positions ...
xSemaphoreTake(swapMutex, portMAX_DELAY);
layerP.lights.header.isPositions = 3; // ← THIS ONLY HAPPENS IF MONITOR IS ON
xSemaphoreGive(swapMutex);
}If monitor is disabled or no WebSocket clients are connected, the transition Meanwhile, in if (layerP.lights.header.isPositions == 0 && !newFrameReady) {
layerP.loop(); // ← NEVER RUNS if isPositions is stuck at 2Why Your Symptoms Match
The FixAlways transition if (isPositions == 2) { // send to UI
if (_socket->getActiveClients() && _state.data["monitorOn"]) {
// ... send positions ...
}
// Always transition, even if we didn't send
xSemaphoreTake(swapMutex, portMAX_DELAY);
EXT_LOGD(ML_TAG, "positions sent to monitor (2 -> 3)");
layerP.lights.header.isPositions = 3;
xSemaphoreGive(swapMutex);
}Would you like me to generate the complete fix? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
This is already the case! The change you suggest is already in the codebase, please verify ! The strange thing: I have 2 effects running : scrolling text and game of life. The first keeps being displayed, the second stops displaying. Game of life is one of the biggest effects in terms of memory and computation. Could that be the issue? The fact that nothing is displayed indicates that the channels memory is nullified (see main.cpp effect task) before the driver sent out all the LEDs |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6320 🏁 Script executed: Length of output: 9686
Found The Real Issue! 🎯Looking at the PR diff, the problem is in GameOfLifeEffect::onSizeChanged() with the allocation logic: The Bug Sequence
Why Scrolling Text Still WorksIt's a different effect with different memory requirements - its allocation isn't failing. The FixDon't free the old buffers until the new allocation succeeds: void onSizeChanged(const Coord3D& prevSize) override {
// Compute new size first
size_t newDataSize = (layer->size.x * layer->size.y * layer->size.z + 7) / 8;
// Try to allocate NEW buffers
uint8_t* newCells = allocMB<uint8_t>(newDataSize);
uint8_t* newFutureCells = allocMB<uint8_t>(newDataSize);
uint8_t* newCellColors = allocMB<uint8_t>(layer->size.x * layer->size.y * layer->size.z);
if (!newCells || !newFutureCells || !newCellColors) {
EXT_LOGE(ML_TAG, "allocation of cells/futureCells/cellColors failed - keeping old buffers");
// Free any partial allocations
if (newCells) freeMB(newCells);
if (newFutureCells) freeMB(newFutureCells);
if (newCellColors) freeMB(newCellColors);
return; // Keep old buffers, continue with old size
}
// Allocation succeeded - now safe to free old buffers
if (cells) freeMB(cells);
if (futureCells) freeMB(futureCells);
if (cellColors) freeMB(cellColors);
// Assign new buffers
cells = newCells;
futureCells = newFutureCells;
cellColors = newCellColors;
dataSize = newDataSize;
EXT_LOGD(ML_TAG, "allocation of cells futureCells cellColors successful %d %d", dataSize, layer->nrOfLights);
startNewGameOfLife();
}Immediate Debug StepCheck your serial logs - you should see: appearing about 1 second after boot. That confirms this diagnosis. Would you like me to create a fix for this? 🐰💭 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
this is not it, I don't see this and the fact that a refresh of the screen makes the effect show for a second also proves the effect is still running fine, there is only something wrong with storing the results in the channels array (channelsD or channelsE). The fact that the effect is shown for a second when doing a page refresh could also be because it generates CPU load which makes tasks running differently. Or it might be related to isPositions. FYI this is shown in the log when refreshing: [1577254][D][PhysicalLayer.cpp:155] onLayoutPre(): [💫] httpd (900) pass 1 mp:1 onSizeChanged is not always shown, sometimes. Which is very odd! Why is was to 0,0,0 and why don't I see an onSizechanged from 128x128 -> 0,0 ? So don't focus only on isPositions. It might be related although I suspect other causes are more likely. I feel strongly it has something to do with the channels array being nullified (which is not done in main.cpp in the effect task as I said earlier (there it is copied from channelsD to channelsE) . It is done on channelsE in onLayoutPre (pass 1) - which is called when doing an UI refresh (to fill the array with positions for the monitor). So you could even say that nullifying has a positive influence on the effect as it then shows a frame of the effect... well... very confusing. Please shine a light on this matter |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 5727 🏁 Script executed: Length of output: 6381 🏁 Script executed: Length of output: 3959
🏁 Script executed: Length of output: 4789
The BugThe issue is that pass 1 layout modifies shared state WITHOUT holding the swapMutex, while effectTask is concurrently running effects and writing to channelsE. Here's the exact sequence:Thread 1 (effectTask):
Thread 2 (httpd task OR driverTask):
Why GameOfLife Disappears
Why Browser Refresh Shows One FrameWhen you refresh, it triggers monitor layout, which:
The FixPhysicalLayer.cpp onLayoutPre needs to hold swapMutex during critical modifications: void PhysicalLayer::onLayoutPre() {
EXT_LOGD(ML_TAG, "pass %d mp:%d", pass, monitorPass);
if (pass == 1) {
// Hold mutex while modifying shared state!
if (layerP.lights.useDoubleBuffer) xSemaphoreTake(swapMutex, portMAX_DELAY);
lights.header.nrOfLights = 0;
lights.header.size = {0, 0, 0};
EXT_LOGD(ML_TAG, "positions in progress (%d -> 1)", lights.header.isPositions);
lights.header.isPositions = 1; // Stops effectTask from starting NEW frames
if (layerP.lights.useDoubleBuffer) xSemaphoreGive(swapMutex);
delay(100); // Wait for any in-progress frame to complete
// Now safe to zero the buffer - effectTask won't start new frames while isPositions == 1
if (layerP.lights.useDoubleBuffer) xSemaphoreTake(swapMutex, portMAX_DELAY);
memset(lights.channelsE, 0, lights.maxChannels);
if (layerP.lights.useDoubleBuffer) xSemaphoreGive(swapMutex);
// dealloc pins (non-critical, can be outside mutex)
if (!monitorPass) {
memset(ledsPerPin, 0xFF, sizeof(ledsPerPin));
memset(ledPinsAssigned, 0, sizeof(ledPinsAssigned));
nrOfAssignedPins = 0;
}Would you like me to create a proper fix for this race condition? 🐰🔍 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
Although this looks like a good addition it did not solve the problem. I found out that even pressing the save button shows the effect for a second, so I really think it has to do with tasks and how they work together. There is another mystery issue: #107 correction: #102 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5977 🏁 Script executed: Length of output: 7295 🏁 Script executed: Length of output: 7929 🏁 Script executed: Length of output: 8357 🏁 Script executed: Length of output: 10009
The BugVirtualLayer.cpp line 349 is the ONLY place where VirtualLayer's size gets updated: void VirtualLayer::onLayoutPre() {
nrOfLights = 0;
size = layerP->lights.header.size; // ← Only happens during pass 2!But this only runs during pass 2 ( The Flow
Why Refresh/Save Temporarily Fixes ItWhen you press save or refresh:
The FixEnsure pass 2 always runs after pass 1: Option 1: Auto-trigger pass 2 after pass 1 in loopDrivers() void PhysicalLayer::loopDrivers() {
if (requestMapPhysical) {
EXT_LOGD(ML_TAG, "mapLayout physical requested");
pass = 1;
mapLayout();
requestMapPhysical = false;
// Always run pass 2 after pass 1 to sync VirtualLayer sizes
requestMapVirtual = true;
}
if (requestMapVirtual) {
EXT_LOGD(ML_TAG, "mapLayout virtual requested");
pass = 2;
mapLayout();
requestMapVirtual = false;
}Option 2: Set both flags on boot (in setup or module initialization) Would you like me to create a complete fix? This should resolve the GameOfLife issue permanently. ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9341 🏁 Script executed: Length of output: 7955 🏁 Script executed: Length of output: 9639
Root CauseGameOfLife's expensive nested loops run WITHOUT yielding, starving the effectTask when higher-priority tasks (HTTP server) need CPU: The Killer Loop (line 257-323):for (int x = 0; x < layer->size.x; x++) // 128 iterations
for (int y = 0; y < layer->size.y; y++) // 128 iterations
for (int z = 0; z < layer->size.z; z++) { // 1 iteration
// ... Game of Life rules ...
for (int i = -1; i <= 1; i++) // 3 iterations
for (int j = -1; j <= 1; j++) // 3 iterations
for (int k = -zAxis; k <= zAxis; k++) // 2 iterations
// Check neighborsTotal operations per frame: 128 × 128 × 1 × (3 × 3 × 2) = 294,912 operations WITHOUT A SINGLE YIELD! Why UI Actions Show One Frame
Connection to Issue
|
Collaborator
Author
|
@coderabbitai, regarding
Nope this is not it. It's still very odd that the game of life effect shows one frame when selected, and also when some system interrupts occur (refresh screen, save). Focus on what makes game of life different from others, analyse it's code, could getRGB obtain data from the wrong channels array? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22093
The BugGameOfLife is the only effect that reads BACK from channelsE in the middle of rendering, AFTER channelsE has been zeroed. The Exact FlowIn main.cpp effectTask (lines ~140-150): if (layerP.lights.useDoubleBuffer) {
// Swap pointers
channelsE ↔ channelsD
}
memset(layerP.lights.channelsE, 0, layerP.lights.maxChannels); // ← ZERO the buffer!
layerP.loop(); // ← GameOfLife runs hereIn GameOfLife::loop() line 307: if (!cellValue) {
if (fadedBackground) {
CRGB val = layer->getRGB(cPos); // ← Reads from channelsE... which is all zeros!
if (fadedBackground < val.r + val.g + val.b)
layer->blendColor(cPos, bgColor, blur);Why GameOfLife Fails
Why Other Effects WorkLooking at E_WLED.h, other effects that use GameOfLife is unique because:
Why Refresh/Save Shows One FrameDuring
The FixRemove the memset that zeros channelsE, or only zero when specifically needed: Option 1: Don't zero at all (let effects overwrite) // Remove this line:
// memset(layerP.lights.channelsE, 0, layerP.lights.maxChannels);Option 2: Only zero during layout if (layerP.lights.header.isPositions == 1) { // Only during layout pass 1
memset(layerP.lights.channelsE, 0, layerP.lights.maxChannels);
}Which approach should we take? The memset was likely added for a reason - do you remember why channelsE needs to be zeroed every frame? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
but this is not done, it is only done if (isPositions == 2) isn't it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 6446
Looking at the actual code:
But I'm puzzled why GameOfLife fails. Let me ask a few clarifying questions:
Can you add debug logs to verify what size GameOfLife is using? Try adding this in GameOfLife::loop() at the start: if (generation % 100 == 0) { // Log every 100 generations
EXT_LOGD(ML_TAG, "GameOfLife size: %d,%d,%d generation: %d",
layer->size.x, layer->size.y, layer->size.z, generation);
}This might reveal if the size issue I found earlier is the actual root cause. 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, interesting I only see a few lines of the logging you proposed: 127616][D][E_MoonModules.h:213] loop(): [💫] AppEffects (2208) GameOfLife size: 128,128,1 generation: 100 I added some more log tests, it looks like GameOfLifeEffect::loop() is not called anymore. How is that possible ? the on/off button is on. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 6416
void PhysicalLayer::loop() {
if (lights.header.nrOfChannels >= lights.maxChannels) return; // ← THIS BLOCKS EVERYTHING
// runs the loop of all effects / nodes in the layer
for (VirtualLayer* layer : layers) {
if (layer) {
layer->loop(); // GameOfLife::loop() never reached!
}
}
}You mentioned earlier that you removed this, but the script output shows it's still present. This guard is blocking all effect execution when To ConfirmAdd this logging right before the guard to see when and why it triggers: void PhysicalLayer::loop() {
EXT_LOGD(ML_TAG, "PhysicalLayer::loop() nrOfChannels=%d maxChannels=%d",
lights.header.nrOfChannels, lights.maxChannels);
if (lights.header.nrOfChannels >= lights.maxChannels) {
EXT_LOGE(ML_TAG, "BLOCKING: nrOfChannels >= maxChannels!");
return;
}
// ...This will show:
Expected BehaviorYour matrix is 128×128 = 16,384 LEDs × 3 channels = 49,152 channels. If
Then the guard fires and all effects stop rendering. Can you add the logging and show me what values you see around the time GameOfLife stops? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
I removed it but did not commit it yet.So this is not the problem. Can you check if game of life does anything out of memory bounds which could corrupt things? check the channels array specially and look at setRGB and get RGB specifically |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 17603
The Memory Corruption BugLooking at the code: Allocation (line 191): cellColors = allocMB<uint8_t>(layer->size.x * layer->size.y * layer->size.z);Access throughout loop() (lines 160, 231, 235, 238, 286, 287, 301, 315): int index = layer->XYZUnModified(Coord3D(x, y, z));
cellColors[index] = random8(1, 255); // ← OUT OF BOUNDS!The Root CauseFrom my earlier analysis (confirmed by your logs), VirtualLayer.size is NOT synchronized until pass 2 runs. On boot:
What Gets CorruptedThis overflow writes into adjacent heap memory, potentially corrupting:
Why Refresh Shows One FrameWhen you refresh, pass 2 runs, The Fix - Need More InfoI need to verify what This will confirm whether 🏁 Script executed: Length of output: 3508
The Bug SequenceFrom your logs and the code:
The Core Problem
return position.x + position.y * size.x + position.z * size.x * size.y;When But The FixIn PhysicalLayer::loopDrivers(), ensure pass 2 runs after pass 1 to update VirtualLayer.size BEFORE onSizeChanged fires: void PhysicalLayer::loopDrivers() {
if (requestMapPhysical) {
EXT_LOGD(ML_TAG, "mapLayout physical requested");
pass = 1;
mapLayout();
requestMapPhysical = false;
// ALWAYS trigger pass 2 to sync VirtualLayer sizes before effects run
requestMapVirtual = true;
}
if (requestMapVirtual) {
EXT_LOGD(ML_TAG, "mapLayout virtual requested");
pass = 2;
mapLayout();
requestMapVirtual = false;
}
// Now all VirtualLayer.size values are correct, onSizeChanged will allocate correctly
// ... rest of loopDriversThis ensures VirtualLayer.size is updated to Would you like me to create a complete fix? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
nope , I added your change (not committed yet) but didn't help. Pass1 is only run without Pass2 when the monitor asks for it, in all other cases pass2 will run as well. Dig deeper |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 9950
🏁 Script executed: Length of output: 7325
Here's my theory:
The key question: What causes VirtualLayer.size to change in step 6? Can you add this logging to see if void onSizeChanged(const Coord3D& prevSize) override {
EXT_LOGW(ML_TAG, "GameOfLife onSizeChanged %d,%d,%d -> %d,%d,%d",
prevSize.x, prevSize.y, prevSize.z,
layer->size.x, layer->size.y, layer->size.z);
if (cells) freeMB(cells);
if (futureCells) freeMB(futureCells);
if (cellColors) freeMB(cellColors);
// ... rest of codeAlso add logging in VirtualLayer::loop() at line 60 to catch size changes: if (prevSize != size) {
EXT_LOGW(ML_TAG, "onSizeChanged V %d,%d,%d -> %d,%d,%d",
prevSize.x, prevSize.y, prevSize.z, size.x, size.y, size.z);
// ... existing codeThis will show us if/when VirtualLayer.size is changing unexpectedly and causing the reallocation failure. 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
And really all 2D effects run fine, only GameOfLife has this issue, so make an analysis what makes game of life special? getRGB is one of it. Could you analyze setLight and especially getLight, could anything be wrong there? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 21416
The Bug - Race Condition in main.cppLines 134-137 in effectTask: if (layerP.lights.useDoubleBuffer) {
xSemaphoreGive(swapMutex); // ← RELEASES MUTEX TOO EARLY!
memcpy(layerP.lights.channelsE, layerP.lights.channelsD, layerP.lights.header.nrOfChannels);
}The race sequence:
Why GameOfLife is SpecialGameOfLife line 163: CRGB val = layer->getRGB(cPos); // Reads PREVIOUS frame data
if (fadedBackground < val.r + val.g + val.b) layer->blendColor(cPos, bgColor, blur);GameOfLife relies on reading colors from the previous frame to decide whether to fade dead cells. When the memcpy is corrupted, it reads garbage data → wrong blending decisions → effect appears broken. Other effects (E_WLED.h lines 300, 1612, 1778, etc.): color = layer->getRGB(Coord3D(0, y - 1)); // Reads what was JUST written THIS frameThey only read pixels they just wrote in the current frame, so they're unaffected by the corrupted previous-frame copy. The FixHold the mutex during memcpy: if (layerP.lights.useDoubleBuffer) {
memcpy(layerP.lights.channelsE, layerP.lights.channelsD, layerP.lights.header.nrOfChannels);
xSemaphoreGive(swapMutex); // ← Release AFTER memcpy
}Or even better, keep mutex for the entire operation: xSemaphoreTake(swapMutex, portMAX_DELAY);
if (layerP.lights.header.isPositions == 0 && !newFrameReady) {
if (layerP.lights.useDoubleBuffer) {
memcpy(layerP.lights.channelsE, layerP.lights.channelsD, layerP.lights.header.nrOfChannels);
}
xSemaphoreGive(swapMutex); // Release after memcpy
layerP.loop();
// ... rest of codeThis ensures channelsD/channelsE pointers cannot be swapped while memcpy is reading/writing them. Would you like me to create a complete fix? ✏️ Learnings added
🧠 Learnings used |
docs ===== - Installer: add esp32-d0-pico2 - io: add dig-next2 (pico2) - esp32-d0.ini: esp32-d0-pico2 backend ======= - main: memcpy channels within swapMutex - Physical layer: memset channels within swapMutex - Virtual layer: get/setLight no mapping array range check (GOL needs it) - WIP - Effects: check on freeMB, add name argument
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
259-319: Fix OOB/UB whencolorCount == 0during birth.
uint8_t colorIndex = nColors[random8(colorCount)];can read uninitialized memory / go out of bounds when no neighbor colors were collected. This can corrupt heap/state and matches the class of “effect runs briefly then dies” failures.Proposed fix
} else if (!cellValue && birthNumbers[neighbors]) { // Reproduction setBitValue(futureCells, cIndex, true); - uint8_t colorIndex = nColors[random8(colorCount)]; + uint8_t colorIndex = (colorCount > 0) ? nColors[random8(colorCount)] : random8(); if (random8(100) < mutation) colorIndex = random8(); cellColors[cIndex] = colorIndex; layer->setRGB(cPos, colorByAge ? CRGB::Green : ColorFromPalette(layerP.palette, colorIndex)); } else {
259-323: Add a yield inside the outer loop to prevent task starvation on large panels.
This effect is computationally heavy; prior debugging indicates missing yields can starve higher-priority/UI tasks and/or trip watchdog timing (Based on learnings, ...).Proposed fix (minimal)
for (int x = 0; x < layer->size.x; x++) { + addYield(1); // or taskYIELD(); for (int y = 0; y < layer->size.y; y++) { for (int z = 0; z < layer->size.z; z++) { ...
🤖 Fix all issues with AI agents
In `@firmware/installer/manifest_esp32-d0-pico2.json`:
- Around line 1-17: The manifest version and binary path are out of sync with
APP_VERSION; update the "version" field from "0.7.0" to "0.7.1" and change the
binary filename in the "parts" entry from
"MoonLight_esp32-d0-pico2_0-7-0_webflash.bin" to
"MoonLight_esp32-d0-pico2_0-7-1_webflash.bin" (match the rename_fw.py convention
of replacing dots with dashes), so the manifest's version string and the
referenced firmware image name align with APP_VERSION defined in platformio.ini.
🧹 Nitpick comments (7)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1685-1695: Consider “allocate-new-then-free-old” forhueto avoid losing the previous buffer on alloc failure.
Current behavior is safe (effect just runs without hue), but it’s a small UX regression if allocations become tight.Proposed tweak
void onSizeChanged(const Coord3D& prevSize) override { - if (hue) freeMB(hue, name()); - hue = allocMB<uint8_t>(layer->size.y, name()); + uint8_t* newHue = allocMB<uint8_t>(layer->size.y, name()); + if (newHue) { + if (hue) freeMB(hue, name()); + hue = newHue; + } if (!hue) { EXT_LOGE(ML_TAG, "allocate hue failed"); } }src/MoonLight/Layers/VirtualLayer.cpp (1)
171-178: Bounds-checking is good, but protect against log spam + tighten format specifiers.
If this branch can be hit frequently (e.g., partial mapping / oversize spaces),EXT_LOGWper pixel/frame can dominate runtime and disturb timing. Also,%dwithuint32_t/size_tcan misreport values.Example: rate-limit + safer formatting (illustrative)
} else { // no mapping uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset; if (index + length <= layerP->lights.maxChannels) { memcpy(&layerP->lights.channelsE[index], channels, length); } else { - EXT_LOGW(ML_TAG, "%d + %d >= %d (%d %d)", indexV, length, layerP->lights.maxChannels, index, offset); + static uint32_t lastWarnMs = 0; + uint32_t now = pal::millis(); + if (now - lastWarnMs > 1000) { + lastWarnMs = now; + EXT_LOGW(ML_TAG, "setLight OOB: indexV=%u offset=%u len=%u index=%lu max=%lu", + (unsigned)indexV, (unsigned)offset, (unsigned)length, + (unsigned long)index, (unsigned long)layerP->lights.maxChannels); + } } }Also applies to: 219-228
src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
177-203: Allocation failure currently “disables” the effect; safe, but consider a clearer fallback.
Right now you free old buffers first, thenreturnif allocation fails (effect stops rendering). That’s safe, but if you want to preserve behavior on shrink or transient failures, considerreallocMB(...)patterns like inE_WLED.h. (Based on learnings, allocation-order mattered in prior debugging.)src/MoonLight/Nodes/Effects/E_WLED.h (2)
337-349: Minor polish: mark these destructors asoverridefor consistency/safety.
Some destructors now do real work; addingoverridemakes intent explicit and catches signature mismatches.Also applies to: 573-575, 1014-1016, 1750-1752, 1828-1830
1660-1668: Good: FlowEffect now renders across X/Z, but watch perf on large volumes.
If users push largesize.x * size.z, consider a lightweight yield or stepping to keep frame pacing stable.src/MoonLight/Layers/PhysicalLayer.cpp (2)
86-86: Consider a more specific allocation-failure check.The
>=condition is overly broad: it returns early both whenmaxChannels == 0(allocation failure) and whennrOfChannels == maxChannels(at capacity but valid). Per the PR discussion, a more targeted check would avoid skipping valid processing when exactly at capacity.Suggested refinement
- if (lights.header.nrOfChannels >= lights.maxChannels) return; // in case alloc mem is not successful + if (lights.maxChannels == 0 || !lights.channelsE) return; // skip if allocation failed
159-174: Mutex pattern correctly addresses the race condition.The approach of setting
isPositions = 1under mutex to stop new frames, then waiting for in-progress frames, then zeroing under mutex is sound. Two minor observations:
Inconsistent member access: Lines 160, 167, 172, 174 use
layerP.lights.useDoubleBuffer(global instance) instead oflights.useDoubleBuffer(member). Both work sincelayerPis the singleton, but member access is cleaner.Magic delay: The 100ms delay assumes all effect frames complete within this window. For heavy effects (e.g., GameOfLife with ~295K neighbor checks per frame), this may be insufficient under load.
Optional: Use member access for consistency
- if (layerP.lights.useDoubleBuffer) xSemaphoreTake(swapMutex, portMAX_DELAY); + if (lights.useDoubleBuffer) xSemaphoreTake(swapMutex, portMAX_DELAY); ... - if (layerP.lights.useDoubleBuffer) xSemaphoreGive(swapMutex); + if (lights.useDoubleBuffer) xSemaphoreGive(swapMutex);Apply similarly to all four occurrences in this block.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
firmware/installer/images/esp32-d0-pico2.jpgis excluded by!**/*.jpg
📒 Files selected for processing (14)
docs/gettingstarted/installer.mddocs/moonbase/inputoutput.mdfirmware/esp32-d0.inifirmware/installer/manifest_esp32-d0-pico2.jsonplatformio.inisrc/MoonBase/Nodes.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cpp
✅ Files skipped from review due to trivial changes (2)
- src/MoonLight/Modules/ModuleEffects.h
- src/MoonBase/Nodes.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Modules/ModuleDrivers.h
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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: 2026-01-15T15:13:55.357Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.573Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:06.470Z
Learning: In MoonLight: VirtualLayer.size is ONLY synchronized with PhysicalLayer.lights.header.size during pass 2 (VirtualLayer::onLayoutPre at line 349 sets size = layerP->lights.header.size). If pass 2 (requestMapVirtual) doesn't run, VirtualLayer.size remains at its default {16, 16, 1}, causing effects to allocate buffers and iterate over the wrong dimensions. Pass 2 must always run after pass 1 to ensure VirtualLayer gets the correct size from PhysicalLayer.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
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: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
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.
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-15T12:38:33.573Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.573Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-15T15:13:55.357Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T15:13:55.357Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-15T12:38:06.470Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:06.470Z
Learning: In MoonLight: VirtualLayer.size is ONLY synchronized with PhysicalLayer.lights.header.size during pass 2 (VirtualLayer::onLayoutPre at line 349 sets size = layerP->lights.header.size). If pass 2 (requestMapVirtual) doesn't run, VirtualLayer.size remains at its default {16, 16, 1}, causing effects to allocate buffers and iterate over the wrong dimensions. Pass 2 must always run after pass 1 to ensure VirtualLayer gets the correct size from PhysicalLayer.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-25T13:34:40.097Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/main.cppsrc/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/main.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
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
Applied to files:
platformio.inifirmware/esp32-d0.ini
📚 Learning: 2026-01-13T15:18:36.992Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:36.992Z
Learning: Ensure in code reviews that the three mutexes (effectsMutex, driversMutex, layerMutex) are created successfully at initialization time (checks in constructors log errors if xSemaphoreCreateMutex returns nullptr). Do not add runtime null checks for these mutex pointers after initialization, as the design treats allocation as an invariant. This pattern should be verified across MoonLight PhysicalLayer.cpp and MoonBase Nodes headers; consider documenting the invariant and ensuring no code path dereferences null mutex pointers. If extending, apply to all similar critical resources initialized during boot.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-06T15:24:11.096Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:11.096Z
Learning: In VirtualLayer.cpp (setLight and getLight), the unmapped path intentionally disables bounds checking when indexV >= mappingTableSize or oneToOneMapping is true to optimize performance for very large LED counts (100K+). This is a deliberate design decision; only rely on this behavior if you can guarantee calls stay within bounds. Document this contract and avoid adding defensive checks in the hot path. Use explicit bounds checks in all other code paths or when the input cannot be guaranteed to be in-range.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
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`
Applied to files:
firmware/esp32-d0.ini
📚 Learning: 2025-11-29T16:17:30.619Z
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/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
firmware/esp32-d0.ini
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Effects/E_WLED.h (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
onSizeChanged(105-105)onSizeChanged(1315-1321)onSizeChanged(1689-1695)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onSizeChanged(183-203)
src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
src/MoonLight/Nodes/Effects/E_WLED.h (7)
onSizeChanged(35-44)onSizeChanged(341-349)onSizeChanged(577-577)onSizeChanged(999-1012)onSizeChanged(1366-1375)onSizeChanged(1754-1768)onSizeChanged(1832-1846)
🪛 markdownlint-cli2 (0.18.1)
docs/moonbase/inputoutput.md
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
platformio.ini (1)
59-59: LGTM!Standard build date bump for the release.
docs/moonbase/inputoutput.md (1)
56-63: LGTM! Documentation updates for DigNext2 support.The new DigNext2 board documentation and esp32-d0-pico2 installer reference are well-structured and consistent with other QuinLED board entries.
Note: Static analysis flagged line 62's 4-space indentation (expects 2). The current indentation appears intentional for nested sub-bullet formatting under the Dig2Go/Dig Uno/Dig Quad entry. Consider adjusting if project style requires 2-space indentation for nested lists.
docs/gettingstarted/installer.md (1)
48-50: LGTM! Installer documentation updated for new board support.The new esp32-d0-pico2 entry is well-formatted and consistent with other device rows. The preset reordering (Dig2Go first) aligns with the recommended device callout at the top of the page.
firmware/esp32-d0.ini (1)
118-124: Configuration looks good. The partition fileboards/ESP32_8MB.csvexists and the environment setup is correct, following the pattern of other esp32-d0 environments.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
71-75: Good: name-scoped frees improve allocation/free traceability.
This aligns with the “free with context” pattern used elsewhere in the PR.src/MoonLight/Layers/VirtualLayer.cpp (2)
60-76: Good: size-change notification is applied consistently and underlayerMutex.
This is a clean, low-risk way to ensure virtual nodes react to resized fixtures.
326-334: Nice: allocation log is clearer/denser without changing behavior.src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
133-146: Good:generationdefault init removes an implicit-state edge case.src/MoonLight/Nodes/Effects/E_WLED.h (2)
31-44: Good: name-scoped free + name-aware realloc improves debuggability.
1154-1157: Good: PopCorn now fills the whole X/Z plane (matches 3D-plane objective).src/main.cpp (1)
131-159: Code is correctly balanced—useDoubleBufferis immutable after boot.
useDoubleBufferis set only once duringPhysicalLayer::setup()(based on PSRAM availability) and never modified afterward. The mutex balance is correct:
- If
useDoubleBuffer=trueat boot: takes at line 131 → gives at 136 → re-takes → gives at 158 (balanced)- If
useDoubleBuffer=falseat boot: takes at 131 → skips give at 136 → gives at 158 (balanced)Since the value never changes, there is no unbalanced semaphore scenario.
Likely an incorrect or invalid review comment.
src/MoonLight/Layers/PhysicalLayer.cpp (2)
123-123: LGTM!Helpful comment clarifying that the following loop handles physical layer nodes.
176-181: LGTM!Good documentation noting that pin deallocation is non-critical and correctly placed outside the mutex scope.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+1
to
+17
| { | ||
| "name": "MoonLight on esp32-d0-pico2", | ||
| "version": "0.7.0", | ||
| "home_assistant_domain": "esphome", | ||
| "funding_url": "https://esphome.io/guides/supporters.html", | ||
| "new_install_prompt_erase": true, | ||
| "new_install_immediately": false, | ||
| "builds": [ | ||
| { | ||
| "chipFamily": "ESP32", | ||
| "name": "esp32-d0", | ||
| "image": "./images/esp32-d0-pico2.jpg", | ||
| "parts": [ | ||
| { "path": "./MoonLight_esp32-d0-pico2_0-7-0_webflash.bin", "offset": 0 } | ||
| ] | ||
| } | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check platformio.ini for APP_VERSION
find . -name "platformio.ini" -type f | head -5Repository: MoonModules/MoonLight
Length of output: 81
🏁 Script executed:
# Look for webflash filename patterns in build scripts
rg -n "webflash|MoonLight.*0-7" --type-list | head -20Repository: MoonModules/MoonLight
Length of output: 454
🏁 Script executed:
# List all manifest files to see the pattern
find . -name "manifest*.json" -type fRepository: MoonModules/MoonLight
Length of output: 809
🏁 Script executed:
# Check the actual platformio.ini content
cat platformio.ini 2>/dev/null || echo "Not found at root"Repository: MoonModules/MoonLight
Length of output: 15082
🏁 Script executed:
# Find and read the rename_fw.py script
find . -name "rename_fw.py" -type fRepository: MoonModules/MoonLight
Length of output: 87
🏁 Script executed:
# Check the rename_fw.py script to understand binary naming
cat scripts/rename_fw.py 2>/dev/null || echo "Not found"Repository: MoonModules/MoonLight
Length of output: 2925
🏁 Script executed:
# Check another manifest file to see if versions match
cat firmware/installer/manifest_esp32-d0.json | head -20Repository: MoonModules/MoonLight
Length of output: 522
🏁 Script executed:
# Check a few manifest files for version patterns
for file in ./firmware/installer/manifest_esp32-d0.json ./firmware/installer/manifest_esp32-c3.json; do
echo "=== $file ==="
head -5 "$file"
doneRepository: MoonModules/MoonLight
Length of output: 486
Version mismatch: manifest shows 0.7.0 but platformio.ini defines 0.7.1.
The manifest version and binary filename must match the APP_VERSION defined in platformio.ini. The build script (rename_fw.py) generates binary filenames using APP_VERSION="0.7.1", converting dots to dashes (resulting in 0-7-1). However, the manifest references version 0.7.0 and a binary path with 0-7-0, causing the installer to fail when it cannot locate the expected binary file.
Proposed fix
{
"name": "MoonLight on esp32-d0-pico2",
- "version": "0.7.0",
+ "version": "0.7.1",
"home_assistant_domain": "esphome",
"funding_url": "https://esphome.io/guides/supporters.html",
"new_install_prompt_erase": true,
"new_install_immediately": false,
"builds": [
{
"chipFamily": "ESP32",
"name": "esp32-d0",
"image": "./images/esp32-d0-pico2.jpg",
"parts": [
- { "path": "./MoonLight_esp32-d0-pico2_0-7-0_webflash.bin", "offset": 0 }
+ { "path": "./MoonLight_esp32-d0-pico2_0-7-1_webflash.bin", "offset": 0 }
]
}
]
}📝 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.
Suggested change
| { | |
| "name": "MoonLight on esp32-d0-pico2", | |
| "version": "0.7.0", | |
| "home_assistant_domain": "esphome", | |
| "funding_url": "https://esphome.io/guides/supporters.html", | |
| "new_install_prompt_erase": true, | |
| "new_install_immediately": false, | |
| "builds": [ | |
| { | |
| "chipFamily": "ESP32", | |
| "name": "esp32-d0", | |
| "image": "./images/esp32-d0-pico2.jpg", | |
| "parts": [ | |
| { "path": "./MoonLight_esp32-d0-pico2_0-7-0_webflash.bin", "offset": 0 } | |
| ] | |
| } | |
| ] | |
| { | |
| "name": "MoonLight on esp32-d0-pico2", | |
| "version": "0.7.1", | |
| "home_assistant_domain": "esphome", | |
| "funding_url": "https://esphome.io/guides/supporters.html", | |
| "new_install_prompt_erase": true, | |
| "new_install_immediately": false, | |
| "builds": [ | |
| { | |
| "chipFamily": "ESP32", | |
| "name": "esp32-d0", | |
| "image": "./images/esp32-d0-pico2.jpg", | |
| "parts": [ | |
| { "path": "./MoonLight_esp32-d0-pico2_0-7-1_webflash.bin", "offset": 0 } | |
| ] | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
In `@firmware/installer/manifest_esp32-d0-pico2.json` around lines 1 - 17, The
manifest version and binary path are out of sync with APP_VERSION; update the
"version" field from "0.7.0" to "0.7.1" and change the binary filename in the
"parts" entry from "MoonLight_esp32-d0-pico2_0-7-0_webflash.bin" to
"MoonLight_esp32-d0-pico2_0-7-1_webflash.bin" (match the rename_fw.py convention
of replacing dots with dashes), so the manifest's version string and the
referenced firmware image name align with APP_VERSION defined in platformio.ini.
backend ======= - Utilities: reallocMB2 keeps old var and size - Modifiers: rippleYZ -> rippleXZ as 1D effects are on the Y axis - Effects: Scrolling Text: fTB 100, not 255
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
259-322: Yield inside the per-cell loop to avoid task starvation.This loop is heavy for large grids and can starve other tasks; add a yield in the outer loop to keep the scheduler responsive. Based on learnings, this is a known root cause for GameOfLife disappearing under load.
Based on learnings, add a yield inside the outer loop to prevent starvation.🐛 Proposed fix
for (int x = 0; x < layer->size.x; x++) { for (int y = 0; y < layer->size.y; y++) { for (int z = 0; z < layer->size.z; z++) { ... } } + taskYIELD(); }
🤖 Fix all issues with AI agents
In `@docs/moonlight/modifiers.md`:
- Line 25: The table row for "RippleXZ" in modifiers.md uses a trailing pipe
which violates the project's markdownlint `leading_only` style; edit the row
containing the "RippleXZ" entry (the line with the image links and description)
to remove the final trailing pipe and ensure the row uses leading pipes only (no
trailing |), and scan adjacent table rows to keep consistent leading_only table
formatting.
In `@src/MoonLight/Layers/VirtualLayer.h`:
- Line 81: The change of mappingTableSize to size_t removes the deliberate
65,535 light cap while other symbols (nrOfLights_t, mappingTableIndexes and
related indexing math) remain 16-bit, risking huge allocations and index
wrapping; revert mappingTableSize to the original 16‑bit type (use nrOfLights_t
or uint16_t) and add a defensive check that clamps/validates incoming counts to
the 65,535 limit, or if you prefer to lift the cap, widen all related types and
bounds logic (nrOfLights_t, mappingTableIndexes, indexing arithmetic and any
allocation size calculations) consistently across the pipeline to avoid overflow
and wrapping.
In `@src/MoonLight/Nodes/Effects/E_MoonModules.h`:
- Around line 183-201: In onSizeChanged, if any of the three allocations (cells,
futureCells, cellColors via allocMB in the onSizeChanged method) fails you must
free any buffers that succeeded and set their pointers to nullptr before
returning; update the failure branch to call freeMB for each non-null of cells,
futureCells, cellColors (using name()) and null them, so the effect never
remains in a partially allocated state and subsequent code can detect the clean
failure.
🧹 Nitpick comments (2)
src/MoonLight/Layers/VirtualLayer.cpp (1)
171-177: Consider gating the no‑mapping bounds checks to avoid hot‑path regressions.The unmapped path previously skipped bounds checks for performance at very large LED counts; adding checks and per‑call warnings can materially slow effects and flood logs if an effect writes out-of-range. If safe‑mode is the intent, consider guarding these checks/logs behind that flag or a debug build, and rate‑limit warnings. Based on learnings, this path was intentionally unchecked for performance.
Also applies to: 219-227
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
236-244: Comment may be misleading - code ripples in Y direction, not X.The comment says "each Y is rippled through the X-axis," but the implementation copies from
(x, y-1)to(x, y), which propagates pixels in the +Y direction (not along X). Consider clarifying:- // 1D->2D: each Y is rippled through the X-axis + // 1D->2D: ripple effect propagates along the Y-axis
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/gettingstarted/installer.mddocs/moonlight/modifiers.mdfirmware/esp32-d0.inimisc/livescripts/E_lines.scplatformio.inisrc/MoonBase/Utilities.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
🚧 Files skipped from review as they are similar to previous changes (2)
- platformio.ini
- src/MoonBase/Utilities.h
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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: 2026-01-15T15:13:55.357Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.573Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:06.470Z
Learning: In MoonLight: VirtualLayer.size is ONLY synchronized with PhysicalLayer.lights.header.size during pass 2 (VirtualLayer::onLayoutPre at line 349 sets size = layerP->lights.header.size). If pass 2 (requestMapVirtual) doesn't run, VirtualLayer.size remains at its default {16, 16, 1}, causing effects to allocate buffers and iterate over the wrong dimensions. Pass 2 must always run after pass 1 to ensure VirtualLayer gets the correct size from PhysicalLayer.
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: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2026-01-15T12:38:06.470Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:06.470Z
Learning: In MoonLight: VirtualLayer.size is ONLY synchronized with PhysicalLayer.lights.header.size during pass 2 (VirtualLayer::onLayoutPre at line 349 sets size = layerP->lights.header.size). If pass 2 (requestMapVirtual) doesn't run, VirtualLayer.size remains at its default {16, 16, 1}, causing effects to allocate buffers and iterate over the wrong dimensions. Pass 2 must always run after pass 1 to ensure VirtualLayer gets the correct size from PhysicalLayer.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2026-01-06T15:24:11.096Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:11.096Z
Learning: In VirtualLayer.cpp (setLight and getLight), the unmapped path intentionally disables bounds checking when indexV >= mappingTableSize or oneToOneMapping is true to optimize performance for very large LED counts (100K+). This is a deliberate design decision; only rely on this behavior if you can guarantee calls stay within bounds. Document this contract and avoid adding defensive checks in the hot path. Use explicit bounds checks in all other code paths or when the input cannot be guaranteed to be in-range.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-15T15:13:55.357Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T15:13:55.357Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2026-01-13T15:18:36.992Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:36.992Z
Learning: Ensure in code reviews that the three mutexes (effectsMutex, driversMutex, layerMutex) are created successfully at initialization time (checks in constructors log errors if xSemaphoreCreateMutex returns nullptr). Do not add runtime null checks for these mutex pointers after initialization, as the design treats allocation as an invariant. This pattern should be verified across MoonLight PhysicalLayer.cpp and MoonBase Nodes headers; consider documenting the invariant and ensuring no code path dereferences null mutex pointers. If extending, apply to all similar critical resources initialized during boot.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-15T12:38:33.573Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.573Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonModules.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:46:48.319Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:46:48.319Z
Learning: In DripEffect (E_WLED.h), the bounce damping at line 1669 needs to be `/1.5` or less (not `/4` or `/2`) to create a visible bounce effect. The continuous gravity application every frame (line 1652) requires a strong initial bounce velocity to overcome it. Using `/1.5` gives approximately 67% energy return, making the bounce visible and lasting for ~33 frames.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:30:02.486Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:30:02.486Z
Learning: In DripEffect (E_WLED.h), the bounce at line 1669 uses heavy damping (`vel = -vel / 4`), which makes the bounce too weak to be visible. Reducing the damping factor from 4 to 2 (or lower) makes the bounce more pronounced and visible.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-11-29T16:17:30.619Z
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
Applied to files:
firmware/esp32-d0.ini
📚 Learning: 2025-11-29T16:17:30.619Z
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`
Applied to files:
firmware/esp32-d0.ini
📚 Learning: 2025-11-29T16:17:30.619Z
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/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
firmware/esp32-d0.ini
🧬 Code graph analysis (6)
misc/livescripts/E_lines.sc (1)
src/MoonLight/Layers/VirtualLayer.cpp (2)
fadeToBlackBy(232-232)fadeToBlackBy(232-232)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
onSizeChanged(105-105)onSizeChanged(1315-1321)onSizeChanged(1691-1691)src/MoonLight/Nodes/Effects/E_WLED.h (7)
onSizeChanged(35-35)onSizeChanged(332-332)onSizeChanged(560-560)onSizeChanged(976-984)onSizeChanged(1338-1338)onSizeChanged(1717-1725)onSizeChanged(1789-1797)
src/MoonLight/Modules/ModuleEffects.h (1)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
RippleXZModifier(198-260)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onSizeChanged(183-203)
src/MoonLight/Nodes/Effects/E_WLED.h (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
onSizeChanged(105-105)onSizeChanged(1315-1321)onSizeChanged(1691-1691)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onSizeChanged(183-203)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/modifiers.md
25-25: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (22)
misc/livescripts/E_lines.sc (1)
2-2: LGTM — visual fade tweak looks safe.docs/gettingstarted/installer.md (2)
24-24: LGTM: Punctuation improvement.The change from "up to 16K - 98K LEDs !" to "up to 16-98K LEDs" improves consistency and removes informal punctuation.
50-50: Assets are properly configured. All referenced files and external links for the esp32-d0-pico2 entry are in place and valid: the image file exists atfirmware/installer/images/esp32-d0-pico2.jpg, the manifest file exists atfirmware/installer/manifest_esp32-d0-pico2.json, and both the ESP32 Pico Mini 02 datasheet and QuinLED DigNext2 documentation links are accessible.firmware/esp32-d0.ini (1)
118-124: Update partition file path to the existing file.Line 121 references
default_8MB.csv, which does not exist in the repository. The correct partition file for 8MB ESP32 boards isboards/ESP32_8MB.csv. Updateboard_build.partitions = boards/ESP32_8MB.csvto resolve build failures.⛔ Skipped due to learnings
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++ codesrc/MoonLight/Layers/VirtualLayer.cpp (2)
60-60: No review needed (comment-only change).
326-328:reallocMB2correctly preserves the mapping table invariant on failure.The function only updates
mappingTableSize(line 217) when the allocation succeeds; on failure, bothmappingTableandmappingTableSizeremain unchanged (lines 218–220). This preserves the invariant that(mappingTable == nullptr) ⇔ (mappingTableSize == 0).src/MoonLight/Nodes/Effects/E_WLED.h (9)
28-35: Allocator + size tracking update looks solid.Named frees plus
reallocMB2and theballsSizebounds make the effect’s memory lifecycle safer and clearer.Also applies to: 52-53
325-333: Good use of reallocMB2 for bar heights.Keeps allocation/size tracking consistent and eliminates manual bookkeeping.
553-584: PacMan character buffer management looks consistent.The named free and
reallocMB2use match the rest of the effect set.
973-988: Tetrix allocation lifecycle is clean.
reallocMB2plus guarded free is a nice upgrade for resize safety.
1126-1129: 3D-plane update is clear and consistent.Expanding the draw to all x/z coordinates reads well and matches the 3D plane intent.
1623-1631: Flow zone rendering across planes looks good.The X/Z propagation keeps the visual consistent on 3D layouts.
1710-1724: Rain drops lifecycle refactor looks correct.Named free and
reallocMB2with re-init logic are consistent with other effects.
1782-1796: Drip drops allocation/init is clean.The
reallocMB2+ per-drop initialization keeps state consistent after resize.
1313-1339: Octopus rMap lifecycle changes look solid.Size tracking and named free tighten memory management.
src/MoonLight/Modules/ModuleEffects.h (1)
158-160: RippleXZ wiring update looks consistent.Registry + allocation updates line up with the modifier rename, and the
onSizeChangedinit hook is a nice clarity add.Also applies to: 254-255, 273-274
src/MoonLight/Nodes/Effects/E_MoonLight.h (4)
69-76: StarSky cleanup looks good.Named frees improve logging/debuggability and align with the rest of the memory-management updates.
1669-1672: Ring plane write update is clear.Painting across x/z makes the ring effect behave consistently on 3D fixtures.
1684-1700: RingRandomFlow memory + loop refactor is solid.
reallocMB2withhueSizekeeps buffer sizing explicit and avoids manual bookkeeping.
330-330: Fade tweak is fine.The stronger fade aligns with the intended text readability.
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (2)
58-59: Default value changes affect existing configurations.Changing
mirrorYandmirrorZdefaults fromfalsetotruewill alter behavior for users who haven't explicitly set these values. Previously, only X-axis mirroring was enabled by default; now all three axes will mirror.If this is intentional to provide a better out-of-box experience, consider documenting this behavioral change in release notes.
198-232: Axis-swap logic is internally consistent.The rename from
RippleYZModifiertoRippleXZModifierwith the corresponding changes:
towardsY→towardsX- Shrink
layer->size.xandposition.xinstead of Y- Loop propagates in Y direction (perpendicular to shrunk X axis)
The transformation is applied consistently throughout, and the loop bounds are safe (reading
y-1wheny >= 1).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| | Checkerboard |  | <img width="320" alt="Checkerboard" src="https://github.com/user-attachments/assets/66d51dc7-b816-4ca7-b1e3-57b067566516" /> | | | ||
| | Pinwheel 🧊 |  | <img width="320" alt="PinWheel" src="https://github.com/user-attachments/assets/46585cea-d301-4221-9af2-65f8054543da" /> | Projects 1D/2D effects onto 2D/3D layouts in a pinwheel pattern.<br>**Swirl**: bend the pinwheel<br>**Rotation Symmetry**: rotational symmetry of the pattern<br>**Petals** Virtual width<br>**Ztwist** twist the pattern along the z-axis<br>Height: distance from center to corner | | ||
| | RippleYZ 🧊 |  | <img width="320" alt="RippleYZ" src="https://github.com/user-attachments/assets/90ecf22c-c4c1-4ee9-8096-fd5613fbb1a7" /> | 1D/2D effect will be rippled to 2D/3D (🚨)<br>Shrink: shrinks the original size towards Y and Z, towardsY: copies X into Y, towardsZ: copies XY into Z | | ||
| | RippleXZ 🧊 |  | <img width="320" alt="RippleXZ" src="https://github.com/user-attachments/assets/90ecf22c-c4c1-4ee9-8096-fd5613fbb1a7" /> | 1D/2D effect will be rippled to 2D/3D (🚨)<br>Shrink: shrinks the original size towards X and Z, towardsX: copies Y into X, towardsZ: copies XY into Z | |
There was a problem hiding this comment.
Fix table pipe style (trailing pipe).
Markdownlint flags this row for trailing pipe usage; align with the project’s leading_only style.
📝 Proposed fix
-| RippleXZ 🧊 |  | <img width="320" alt="RippleXZ" src="https://github.com/user-attachments/assets/90ecf22c-c4c1-4ee9-8096-fd5613fbb1a7" /> | 1D/2D effect will be rippled to 2D/3D (🚨)<br>Shrink: shrinks the original size towards X and Z, towardsX: copies Y into X, towardsZ: copies XY into Z |
+| RippleXZ 🧊 |  | <img width="320" alt="RippleXZ" src="https://github.com/user-attachments/assets/90ecf22c-c4c1-4ee9-8096-fd5613fbb1a7" /> | 1D/2D effect will be rippled to 2D/3D (🚨)<br>Shrink: shrinks the original size towards X and Z, towardsX: copies Y into X, towardsZ: copies XY into Z🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
25-25: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
In `@docs/moonlight/modifiers.md` at line 25, The table row for "RippleXZ" in
modifiers.md uses a trailing pipe which violates the project's markdownlint
`leading_only` style; edit the row containing the "RippleXZ" entry (the line
with the image links and description) to remove the final trailing pipe and
ensure the row uses leading pipes only (no trailing |), and scan adjacent table
rows to keep consistent leading_only table formatting.
| // they will be reused to avoid fragmentation | ||
| PhysMap* mappingTable = nullptr; | ||
| nrOfLights_t mappingTableSize = 0; | ||
| size_t mappingTableSize = 0; |
There was a problem hiding this comment.
Reintroduce the 65k light cap or widen all related types.
Switching mappingTableSize to size_t removes the implicit 65,535 limit while nrOfLights_t, mappingTableIndexes, and most indexing math remain 16‑bit. For large layouts this can trigger very large allocations while indices still wrap, leading to inconsistent mapping and memory pressure. Either keep the 16‑bit cap or comprehensively widen the indexing types and bounds logic across the pipeline. Based on learnings, the 65,535 cap was an explicit design decision for memory/perf balance.
🛠️ Proposed fix (keep the existing cap unless the whole pipeline is widened)
- size_t mappingTableSize = 0;
+ nrOfLights_t mappingTableSize = 0;📝 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.
Suggested change
| size_t mappingTableSize = 0; | |
| nrOfLights_t mappingTableSize = 0; |
🤖 Prompt for AI Agents
In `@src/MoonLight/Layers/VirtualLayer.h` at line 81, The change of
mappingTableSize to size_t removes the deliberate 65,535 light cap while other
symbols (nrOfLights_t, mappingTableIndexes and related indexing math) remain
16-bit, risking huge allocations and index wrapping; revert mappingTableSize to
the original 16‑bit type (use nrOfLights_t or uint16_t) and add a defensive
check that clamps/validates incoming counts to the 65,535 limit, or if you
prefer to lift the cap, widen all related types and bounds logic (nrOfLights_t,
mappingTableIndexes, indexing arithmetic and any allocation size calculations)
consistently across the pipeline to avoid overflow and wrapping.
Comment on lines
183
to
201
| void onSizeChanged(const Coord3D& prevSize) override { | ||
| dataSize = ((layer->size.x * layer->size.y * layer->size.z + 7) / 8); | ||
| // EXT_LOGW(ML_TAG, "GameOfLife onSizeChanged %d,%d,%d -> %d,%d,%d", prevSize.x, prevSize.y, prevSize.z, layer->size.x, layer->size.y, layer->size.z); | ||
|
|
||
| freeMB(cells); | ||
| freeMB(futureCells); | ||
| freeMB(cellColors); | ||
| if (cells) freeMB(cells, name()); | ||
| if (futureCells) freeMB(futureCells, name()); | ||
| if (cellColors) freeMB(cellColors, name()); | ||
|
|
||
| cells = allocMB<uint8_t>(dataSize); | ||
| futureCells = allocMB<uint8_t>(dataSize); | ||
| cellColors = allocMB<uint8_t>(layer->size.x * layer->size.y * layer->size.z); | ||
| dataSize = (layer->size.x * layer->size.y * layer->size.z + 7) / 8; | ||
|
|
||
| cells = allocMB<uint8_t>(dataSize, name()); | ||
| futureCells = allocMB<uint8_t>(dataSize, name()); | ||
| cellColors = allocMB<uint8_t>(layer->size.x * layer->size.y * layer->size.z, name()); | ||
|
|
||
| if (!cells || !futureCells || !cellColors) { | ||
| EXT_LOGE(ML_TAG, "allocation of cells || !futureCells || !cellColors failed"); | ||
| return; | ||
| } | ||
| EXT_LOGD(ML_TAG, "allocation of cells futureCells cellColors successful %d %d", dataSize, layer->nrOfLights); | ||
|
|
There was a problem hiding this comment.
Clean up partial allocations on failure.
If any allocation fails, you return without freeing the buffers that did succeed, leaving the effect in a half-allocated state and retaining memory. Consider freeing any successful allocations (and nulling pointers) before returning.
🐛 Proposed fix
cells = allocMB<uint8_t>(dataSize, name());
futureCells = allocMB<uint8_t>(dataSize, name());
cellColors = allocMB<uint8_t>(layer->size.x * layer->size.y * layer->size.z, name());
if (!cells || !futureCells || !cellColors) {
EXT_LOGE(ML_TAG, "allocation of cells || !futureCells || !cellColors failed");
+ if (cells) { freeMB(cells, name()); cells = nullptr; }
+ if (futureCells) { freeMB(futureCells, name()); futureCells = nullptr; }
+ if (cellColors) { freeMB(cellColors, name()); cellColors = nullptr; }
+ dataSize = 0;
return;
}🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Effects/E_MoonModules.h` around lines 183 - 201, In
onSizeChanged, if any of the three allocations (cells, futureCells, cellColors
via allocMB in the onSizeChanged method) fails you must free any buffers that
succeeded and set their pointers to nullptr before returning; update the failure
branch to call freeMB for each non-null of cells, futureCells, cellColors (using
name()) and null them, so the effect never remains in a partially allocated
state and subsequent code can detect the clean failure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.