Drive lots of LEDs (130K max) #93
Conversation
Frontend ======== - monitor: support new header format Backend ======= - main: Updated core assignment: sveltekit and driver on Core0, effects on Core1 - tasks; show core in summary - nodes: brightnessmW capped at 65K LEDs - Physical layer: nrOfLights_t (PSRAM dependent), updated header format, use nrOfLights_t on maxChannels, indexV/P - Virtual layer: Adjust PhysMap for PSRAM (4 bytes), use nrOfLights_t for XYZ, set/getRGB/..., mappingTable
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Monitor / ModuleLightsControl interface/src/routes/moonbase/monitor/Monitor.svelte, src/MoonLight/Modules/ModuleLightsControl.h |
Monitor header threshold changed to headerPrimeNumber = 41; header emission/parsing updated to use 32‑bit little‑endian fields and headerPrimeNumber-sized header with static_assert guards; handleHeader offsets adjusted. |
Core typedef & LightsHeader src/MoonLight/Layers/PhysicalLayer.h, src/MoonLight/Layers/PhysicalLayer.cpp |
Added conditional typedef nrOfLights_t (PSRAM → uint32_t, else uint16_t); LightsHeader expanded (nrOfLights→uint32_t, nrOfChannels, presets, many per‑light offsets/bitfields); maxChannels/indexP updated to nrOfLights_t. |
VirtualLayer & PhysMap src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Replaced uint16_t indices with nrOfLights_t across APIs and internals; reworked PhysMap layout for PSRAM vs non‑PSRAM; mapping table/indexing redesigned (mappingTableIndexes element type, indexes→indexesIndex); added createMappingTableAndAddOneToOne() and oneToOneMapping path; PSRAM fast paths added. |
Physical/Virtual logic & helpers src/MoonLight/Layers/* |
Many loop counters, iterators, and helpers switched to nrOfLights_t; PSRAM-aware allocations and buffer caps adjusted; taskYIELD inserted after node->loop() when node->on is true. |
Module IO / Tasks UI src/MoonBase/Modules/ModuleIO.h, src/MoonBase/Modules/ModuleTasks.h |
Battery pin sentinels for uint8_t pins changed from -1 to UINT8_MAX; ethernet pins remain signed int8_t using -1; UI removed public "core" control and task summaries now include core in-line. |
Drivers / Art‑Net src/MoonLight/Nodes/Drivers/D_ArtnetOut.h |
Art‑Net header built at runtime; added universeSize, nrOfOutputsPerIP, computed usedChannelsPerUniverse/totalUniverses/totalChannels; payload sizing uses universeSize; buffer size increased and output/IP rotation logic adjusted. |
Nodes / Brightness logic src/MoonBase/Nodes.cpp |
For very large arrays (nrOfLights > 8096) skip power‑based correctedBrightness calculation and use brightness directly; retains prior behavior for ≤8096 LEDs (with masked count). |
Layout additions src/MoonLight/Nodes/Layouts/L_MoonLight.h |
Added TorontoBarCubesLayout class with config fields (nrOfLightsPerCube, granularity), setup, onLayout, and cube rendering helpers. |
Info / Limits / UI src/MoonLight/Modules/ModuleMoonLightInfo.h |
UI control limits made dynamic from layerP.lights.maxChannels; runtime counters (nrOfZeroLights, nrOfOneLight, nrOfMoreLights) switched to nrOfLights_t; mappingTableIndexes lookup adjusted to indexesIndex. |
Main / Tasks / Framework src/main.cpp, lib/framework/ESP32SvelteKit.cpp, docs/... |
Renamed tasks (AppEffects/AppDrivers), swapped core affinities (effect→core1, driver→core0), adjusted priorities/stack sizes and SvelteKit stack conditional on PSRAM; added esp_task_wdt calls in tasks; docs updated. |
Event socket / framework cleanup lib/framework/EventSocket.cpp, lib/framework/*, src/MoonBase/Module.* |
EventSocket send now checks esp_err_t and removes failing subscriptions safely via iterator; removed several heap‑optimization comment blocks in framework/module headers. |
Build config platformio.ini |
APP_DATE bumped, ESP32SVELTEKIT_RUNNING_CORE set to 0, I2SClocklessLedDriver lib_deps hash swapped (driver version change). |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- double buffering #84 — PSRAM-aware double-buffering, channelsE/channelsD coordination and monitor/header emission overlap.
- Board presets p4 #74 — Overlapping changes to LightsHeader layout and monitor/header parsing offsets.
- Move from main to module IO #90 — Related changes to ModuleIO battery pin sentinel initializers.
Suggested labels
enhancement
Poem
🐰 Tiny paws tap keys tonight,
Headers grown and indices bright.
PSRAM shelves and cubes take place,
Cores swapped neatly, pins embrace.
Hoppy builds — the lights ignite! ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Drive lots of LEDs (130K max)' directly summarizes the main objective: supporting very large LED counts up to ~130k, which is the core intent of this PR targeting extreme workloads. |
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.cpp (1)
328-344: Core affinity swap will break LED timing due to WiFi preemption on Core 0.The driver task uses DMA/PARLIO/I2S hardware and must run on Core 1 (APP_CPU) to avoid preemption by WiFi (priority ~23) on Core 0. The current assignment reverses this:
effectTask→ Core 1 (acceptable: pure computation)driverTask→ Core 0 (problematic: will be preempted by WiFi)The driver task's priority is 3, ensuring it loses the CPU to WiFi tasks whenever they wake. This will cause LED timing glitches. Swap the cores back: effects on Core 0, driver on Core 1.
🧹 Nitpick comments (3)
src/MoonBase/Modules/ModuleIO.h (1)
616-620: Consider clarifying the comment placement.The comment on Line 616 appears to apply to all four ethernet pin assignments (lines 616-620), but its placement makes it read as if it only applies to
v_ETH_SPI_SCK. Consider moving it above Line 616 or expanding it to clarify scope.🔎 Suggested comment improvement
+ // Note: Ethernet pin types are signed (int8_t) and use -1, not UINT8_MAX, to indicate unset state - ess->v_ETH_SPI_SCK = -1; // type is not unsigned and uses -1, not UINT8_MAX + ess->v_ETH_SPI_SCK = -1; ess->v_ETH_SPI_MISO = -1; ess->v_ETH_SPI_MOSI = -1; ess->v_ETH_PHY_CS = -1; ess->v_ETH_PHY_IRQ = -1;src/MoonLight/Layers/PhysicalLayer.h (1)
37-70: Verify LightsHeader struct size matches the documented 41 bytes.The struct layout with comments suggests 41 bytes total, but the actual size depends on compiler alignment. Consider adding a
static_assertto guarantee the expected size:static_assert(sizeof(LightsHeader) == 41, "LightsHeader must be exactly 41 bytes for Monitor protocol");This prevents silent breakage if fields are reordered or padding changes.
src/MoonLight/Modules/ModuleLightsControl.h (1)
503-503: Consider usingsizeof(LightsHeader)instead of hardcoded 41.The magic number 41 must stay synchronized with
LightsHeader. Usingsizeof(LightsHeader)would be safer:- _socket->emitEvent("monitor", (char*)&layerP.lights.header, 41); + _socket->emitEvent("monitor", (char*)&layerP.lights.header, sizeof(LightsHeader));However, if the "prime number" protocol detection relies on the exact value 41, add a
static_assertinPhysicalLayer.hto enforce the constraint.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
interface/src/routes/moonbase/monitor/Monitor.sveltelib/framework/WWWData.hplatformio.inisrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Modules/ModuleTasks.hsrc/MoonBase/Nodes.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Modules/ModuleMoonLightInfo.hsrc/main.cpp
🧰 Additional context used
🧠 Learnings (10)
📚 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/main.cpp
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
src/main.cppsrc/MoonLight/Layers/PhysicalLayer.cppplatformio.ini
📚 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:
interface/src/routes/moonbase/monitor/Monitor.sveltesrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Layers/PhysicalLayer.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: 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/Modules/ModuleMoonLightInfo.hsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.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.cppsrc/MoonLight/Layers/VirtualLayer.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/Layers/PhysicalLayer.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.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:
platformio.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: Be aware of the integrated build process where the PlatformIO build automatically builds and embeds the SvelteKit frontend into the ESP32 firmware
Applied to files:
platformio.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/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
platformio.ini
🧬 Code graph analysis (2)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/MoonLight/Layers/VirtualLayer.h (3)
setRGB(118-129)setRGB(130-130)XYZUnModified(116-116)
⏰ 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 (23)
src/MoonBase/Modules/ModuleIO.h (1)
754-756: LGTM!Using
UINT8_MAXforuint8_tbattery pin members is idiomatic and clearly indicates an unset/invalid state.src/MoonBase/Modules/ModuleTasks.h (2)
45-45: UI simplification: core info moved to summary.The core field has been removed as a separate column and is now embedded in the summary string (Line 100 shows
@C%d). This simplifies the UI while preserving the information.
100-100: Format enhancement: consolidated task status display.The summary format now includes core affinity (
@C%d) and removes the extra space in the priority marker. The format is more compact while showing all essential information (state, CPU%, priority, core).src/MoonLight/Layers/PhysicalLayer.cpp (2)
190-190: LGTM!Type change from
uint16_ttonrOfLights_tis necessary to support >65,535 LEDs. This aligns with the PR objective of driving up to 130K LEDs.
237-237: Consistency in commented code.Updating the type in commented reference code maintains consistency for future use. This is good practice.
interface/src/routes/moonbase/monitor/Monitor.svelte (3)
88-90: Dimension parsing upgraded to 32-bit.The change from 16-bit to 32-bit dimension fields is appropriate for supporting larger LED matrices. Using
getInt32(signed) works correctly, though dimensions are typically unsigned in nature.
71-81: All header offsets are correctly aligned with the C++LightsHeaderstruct.The "gaps" mentioned in the original review are not missing data. Offsets 22–25 contain
brightness,red,green, andbluefields; offset 27 is a bitfield struct containingisPositions,offsetRed,offsetGreen, andoffsetBlue. The Monitor.svelte code correctly reads only the fields it needs (nrOfLights,nrOfChannels,lightPreset,channelsPerLight,offsetRGB,offsetWhite) at their correct offsets. The commented-out code attempting to extract color offsets from the bitfield at offset 27 was appropriately removed since those fields are unused in the UI. The struct is 41 bytes total with 3-byte padding at the end, as confirmed in the C++ definition.
41-41: Header size expanded to support 32-bit LED counts.The change from 37 to 41 bytes accommodates the upgrade from 16-bit to 32-bit fields for
nrOfLightsandnrOfChannels, enabling support for >65K LEDs. The struct includes explicit padding to maintain the 41-byte alignment used for monitor protocol recognition.platformio.ini (1)
218-219: The I2SClocklessLedDriver dependency update is confirmed.The new commit
4363655d3921fc4f6a40f13dd33d368e93d59ca3exists and correctly implements "Add deleteDriver checks on DMABuffersTampon" (Jan 2, 2026). The update is documented and legitimate for supporting the expanded LED count.src/MoonLight/Layers/PhysicalLayer.h (3)
31-35: Conditional typedef for scalable LED indexing looks good.The
nrOfLights_ttypedef correctly gates 32-bit indices behind PSRAM availability, preserving memory efficiency on constrained boards while enabling 130K+ LEDs with PSRAM.
101-101: Type consistency formaxChannels.Using
nrOfLights_tformaxChannelsaligns with the new indexing type. This correctly scales buffer capacity based on PSRAM availability.
124-124:indexPtype update is consistent with the refactor.Changing from
uint16_ttonrOfLights_tensures the physical layer index can address the full LED range.src/MoonBase/Nodes.cpp (1)
374-374: Good defensive clamping for FastLED compatibility.The
MIN(layerP.lights.header.nrOfLights, UINT16_MAX)correctly handles the FastLED limitation. For setups exceeding 65K LEDs, the brightness/power calculation will use a capped count, which may result in slightly higher actual power draw than calculated.src/MoonLight/Modules/ModuleMoonLightInfo.h (3)
27-46: Dynamic UI bounds correctly scale with available memory.Using
layerP.lights.maxChannels / 3for light count bounds andmaxChannelsfor channel bounds appropriately reflects the PSRAM-dependent capacity.
66-68: Counter types updated tonrOfLights_tfor consistency.This ensures the counters can handle the full range of lights on PSRAM-enabled boards.
79-79: Field access updated fromindexestoindexesIndex.Aligns with the renamed field in
PhysMapstruct.src/MoonLight/Layers/VirtualLayer.cpp (5)
20-20: FastLED wrapper retainsuint16_tlimit - appropriate given library constraint.The function signature correctly reflects FastLED's internal limitation. The caller at line 232-233 properly guards against exceeding this limit.
75-104:addIndexPcorrectly updated for larger index types.The function handles the mapping state machine properly with
nrOfLights_tindices. TheindexesIndexfield rename is consistently applied.
126-166: PSRAM-conditional RGB storage insetLightlooks correct.
- PSRAM path: Direct 3-byte memcpy for full color fidelity
- Non-PSRAM path: 14-bit (5-5-4 RGB) encoding to save memory
The encoding adds small rounding (
+3,+7) before shifting to reduce quantization error.
168-214:getLighttemplate correctly decodes based on PSRAM availability.The non-PSRAM decoding (lines 197-199) properly reverses the 14-bit encoding:
- Red: bits 9-13 → left-shift by 3
- Green: bits 4-8 → left-shift by 3
- Blue: bits 0-3 → left-shift by 4
This matches the encoding logic in
setLight.
232-233: FastLED bounds check prevents overflow.The condition
nrOfChannels / 3 < UINT16_MAXensures FastLED's 16-bit limit isn't exceeded before calling the optimized path.src/MoonLight/Layers/VirtualLayer.h (2)
77-91: Member type updates tonrOfLights_tare consistent.The
nrOfLights,mappingTableSize,mappingTableIndexes, andmappingTableIndexesSizeUsedcorrectly use the new scalable type.
111-217: API signatures consistently updated tonrOfLights_t.All index-based functions (
addIndexP,XYZ,setRGB,getLight, etc.) now usenrOfLights_tfor indices, enabling full 130K LED support on PSRAM boards while maintaining compatibility on constrained hardware.
|
@coderabbitai, an AI friend of us made the following analysis: Scenario 1: Local LEDs (SPI/DMA) Effects run uninterrupted on Core 1 Cons: WiFi bursts might slightly delay driver (but DMA handles this well) Option B: Driver on Core 1 (away from WiFi) Both tasks compete on Core 1 Verdict for local LEDs: Option A (Driver on Core 0) Scenario 2: Art-Net over WiFi (UDP) Driver and WiFi stack are co-located - no inter-core communication overhead Cons: None really - this is the optimal design Option B: Driver on Core 1 (away from WiFi) UDP call from Core 1 → WiFi on Core 0 = cross-core overhead Verdict for Art-Net: Definitely Option A (Driver on Core 0) Recommended Architecture: // Driver on Core 0 (co-located with WiFi/network stack) DMA controller handles actual sending (minimal CPU) For Art-Net (UDP/WiFi): Driver calls udp.write() → stays on Core 0 with WiFi stack Additional Optimization for Art-Net: That's why I changed it do some tests what works better. Can you review it's advice and come with a new advice on core assignment for the drivers and effects tasks |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: Short answer:
How to control it:
Practical rules:
References:
If you want, tell me your ESP‑IDF version and I’ll show the exact menuconfig keys and recommended priority values for that version. 🌐 Web query: 💡 Result: Short answer: I2S DMA can interfere with Wi‑Fi on ESP32 if I2S/its interrupts or your app tasks starve the CPU/core where the Wi‑Fi protocol runs. Fixes: make I2S work non-blocking and IRQ-safe, tune DMA buffers/interrupt priority, or move the Wi‑Fi task or your heavy I2S processing to the other core via menuconfig. Key facts and actionable steps
Practical checklist to resolve interference
If you want, tell me which ESP32 variant and ESP‑IDF version you’re using and I’ll give exact menuconfig options and example code to pin tasks and move processing off the ISR. Citations:
@ewowi, I need to respectfully disagree with your AI friend's analysis. After reviewing ESP-IDF documentation, there's a critical flaw in the reasoning: priority levels. The Priority ProblemWiFi runs at priority ~23 on Core 0 by default, while your AI friend recommends driver priorities of 3 or 5. This means: The claim that "driver can preempt WiFi if needed" is incorrect—priority 3 cannot preempt priority 23. ESP-IDF Official GuidanceESP-IDF warns against pinning higher-priority tasks on the same core as WiFi and recommends leaving protocol tasks room to run. More specifically, for deterministic I2S timing, prefer running that processing on the other core or keep priority below protocol tasks. Corrected RecommendationRevert to the original setup: void setup() {
// Effects on Core 0 (pure computation, can tolerate WiFi preemption)
xTaskCreatePinnedToCore(
effectTask,
"Effects",
8192,
NULL,
2, // Below WiFi priority
NULL,
0 // Core 0 - with WiFi
);
// Driver on Core 1 (timing-critical hardware, needs uninterrupted execution)
xTaskCreatePinnedToCore(
driverTask,
"Driver",
4096,
NULL,
3, // Priority doesn't matter much on Core 1 (no WiFi)
NULL,
1 // Core 1 - APP_CPU (away from WiFi)
);
}Why This Works for Both ScenariosFor Local LEDs (DMA/I2S/LCD/PARLIO):
For Art-Net (UDP/WiFi):
The Root IssueYour AI friend assumed driver priority could be high enough to preempt WiFi. To actually do that, you'd need priority ≥24, but ESP-IDF explicitly warns against starving WiFi tasks—that destabilizes the network stack. Recommendation: Revert the core assignments back to driver on Core 1, effects on Core 0. This matches ESP-IDF best practices and your validated learnings. 🧠 Learnings used |
|
@coderabbitai, can you also advice on core for the sveltekit task, I assume it should be set back to 1 now effects are on 0 again. and also advice on priorities for the 3 tasks (effects, drivers, sveltekit) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1390 🏁 Script executed: Length of output: 39 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 28829 🏁 Script executed: Length of output: 1266 🏁 Script executed: Length of output: 549 🏁 Script executed: Length of output: 486 🏁 Script executed: Length of output: 0 @ewowi, yes, you're correct! With the reversion to effects on Core 0, the SvelteKit task should stay on Core 0 as well. Here's my comprehensive recommendation: Recommended ConfigurationCore Assignments// Effects Task - Core 0 (PRO_CPU)
xTaskCreateUniversal(effectTask, "AppEffectTask", ..., 10, ..., 0);
// Driver Task - Core 1 (APP_CPU)
xTaskCreateUniversal(driverTask, "AppDriverTask", ..., 3, ..., 1);platformio.ini: -D ESP32SVELTEKIT_RUNNING_CORE=0 // Keep on Core 0Priority Recommendations
Why This WorksCore 0 (PRO_CPU):
Core 1 (APP_CPU):
Action RequiredRevert the changes in xTaskCreateUniversal(effectTask, "AppEffectTask", ..., 10, ..., 0); // Core 0
xTaskCreateUniversal(driverTask, "AppDriverTask", ..., 3, ..., 1); // Core 1Keep -D ESP32SVELTEKIT_RUNNING_CORE=0 // Already correctThe current priority levels (10 for effects, 3 for driver, 2 for SvelteKit) are well-balanced and don't need adjustment. All are safely below WiFi's priority ~23, so the network stack remains responsive while your application tasks run efficiently. 🧠 Learnings used |
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 (1)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
188-206: Fix uint16_t overflow risk in ledsPerPin array assignment.
ledsPerPin[]is declared asuint16_t[MAXLEDPINS](max value 65,535 per pin). With support for 130K LEDs, distributing across a small number of pins causes overflow on line 198. For example, 130K LEDs across 3 pins with RGB2040 multiplier yields 86,666 LEDs per pin, which silently truncates when assigned to the uint16_t array. This corrupts LED mapping and driver initialization.Change
ledsPerPintouint32_t[MAXLEDPINS]to safely accommodate large LED counts per pin.
♻️ Duplicate comments (1)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
44-45: Duplicate: Misleading comment still present.This comment was already flagged in a previous review. The inline comment mentions "120 pins * 512..1024 LEDs" but the actual calculation is for 16 Hub75 panels (128×64 pixels × 16 panels × 3 channels = 393,216 channels).
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
253-258: Consider removing or implementing the commented-out dimension controls.The commented-out width/height/depth controls suggest the cube grid dimensions were originally intended to be configurable, but the current implementation uses a hardcoded 4×3×2 grid in
onLayout(). Either remove these comments if the fixed grid is intentional, or implement configurable dimensions if flexibility is desired.
303-303: Consider clarifying or removing the uncertain comment.The comment "keep this for now ..." suggests uncertainty about the
nextPin()placement. If you intend all cubes to share a single pin (which seems appropriate for a unified layout), consider clarifying or removing the tentative comment.src/MoonLight/Layers/VirtualLayer.h (1)
52-55: Minor: Consider using consistent bit-field types for portable packing.Mixing
uint16_t rgb : 14withuint8_t mapType : 2can lead to implementation-defined packing behavior. While GCC/ESP32 typically packs these into a single 16-bit unit as intended, using the same underlying type would be more portable.🔎 Suggested fix
// 2 bytes struct struct { // condensed rgb uint16_t rgb : 14; // 14 bits (554 RGB) - uint8_t mapType : 2; // 2 bits (4) + uint16_t mapType : 2; // 2 bits (4) }; // 16 bits
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/framework/WWWData.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MoonLight/Modules/ModuleLightsControl.h
- src/MoonBase/Modules/ModuleIO.h
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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: 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} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
📚 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.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: 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/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.h
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.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.h
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (3)
src/MoonBase/Nodes.h (3)
Node(67-182)Node(238-238)addLight(228-228)src/MoonLight/Layers/PhysicalLayer.cpp (4)
setup(40-73)setup(40-40)addLight(165-186)addLight(165-165)src/MoonBase/Nodes.cpp (2)
onLayout(211-216)onLayout(211-211)
src/MoonLight/Layers/VirtualLayer.h (1)
src/MoonLight/Layers/VirtualLayer.cpp (8)
addIndexP(75-104)addIndexP(75-75)XYZ(105-113)XYZ(105-105)setLight(126-166)setLight(126-126)getLight(169-214)getLight(169-169)
⏰ 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 (10)
src/MoonLight/Layers/PhysicalLayer.cpp (2)
190-190: LGTM: Correct type migration for large LED counts.The change from
uint16_ttonrOfLights_tis necessary for accumulating LED counts that can exceed 65,535 when supporting up to 130K LEDs.
237-237: Type consistency in commented-out code.The type change from
uint16_ttonrOfLights_tmaintains consistency with the active codebase. Since this code is commented out, the change is low-priority but helpful if this function is re-enabled in the future.src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
244-252: LGTM - Class structure follows established patterns.The class declaration properly inherits from Node and provides the required metadata methods (name, dim, tags) consistent with other 3D layouts in the codebase.
260-262: Verify whether stacked lights at identical positions is intentional.All
nrOfLightsPerCubelights are currently added at the exact sameCoord3D(pos.x, pos.y, pos.z)position. This creates stacked point lights rather than a volumetric cube distribution. The comment indicates this is temporary ("for the time being").Is this placeholder implementation acceptable for the current PR, or should the lights be distributed within each cube's volume (e.g., in a 3D grid or along edges)?
src/MoonLight/Modules/ModuleDrivers.h (2)
85-85: LGTM - Layout properly registered in control list.The TorontoBarCubesLayout is correctly added to the driver control using the standard
getNameAndTags<>()pattern.
124-124: LGTM - Layout allocation follows established pattern.The
checkAndAlloc<TorontoBarCubesLayout>()call correctly enables instantiation of the layout when requested by name, consistent with other layout nodes.src/MoonLight/Layers/VirtualLayer.h (4)
61-69: Past review issue addressed: conditional initialization is now correct.The constructor properly handles both PSRAM and non-PSRAM layouts. The
memset(rgb, 0, 3)is now correctly guarded by#ifdef BOARD_HAS_PSRAM, preventing the buffer overrun that was flagged in the previous review.
80-94: Type migration tonrOfLights_tlooks correct and consistent.The member variables
nrOfLights,mappingTableSize,mappingTableIndexes, andmappingTableIndexesSizeUsedare all consistently updated to usenrOfLights_t, properly supporting the expanded LED count (up to 130K). This aligns with thePhysMapchanges where PSRAM builds support 24-bit indices.
114-120: Method signatures correctly updated for expanded LED support.The public API methods (
addIndexP,XYZ,XYZUnModified, and all set/get methods) are consistently updated to usenrOfLights_tfor indices. This ensures the API can handle the expanded LED counts throughout the call chain.
135-142:setWhitelogic correctly distinguishes RGBW lights from moving heads.The conditional handling for 4-channel RGBW lights (hardcoded offset 3) versus moving heads (using
offsetWhite) is appropriate and maintains backward compatibility.
This is spectacular ;-)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
308-339: LGTM - duplicate coordinate issue resolved.The cube array now correctly defines a complete 4×3×2 grid with 24 unique positions. The previously flagged duplicate
Coord3D(0, 0, 0)has been fixed toCoord3D(0, 0, 1)at line 325.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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.
📚 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.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.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (4)
src/MoonLight/Layers/VirtualLayer.cpp (2)
addLight(351-368)addLight(351-351)src/MoonBase/Utilities.h (1)
Coord3D(151-163)src/MoonLight/Nodes/Layouts/L_SE16.h (2)
onLayout(44-80)onLayout(111-128)src/MoonLight/Nodes/Layouts/L__Sandbox.h (1)
onLayout(36-46)
⏰ 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 (6)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (4)
244-263: LGTM!Class declaration and setup follow established conventions. The
nrOfLightsPerCube = 61default correctly matches the physical cube configuration (5 sides × 12 LEDs + 1 middle LED).
265-268: LGTM!Correctly maps all physical LEDs in a cube to a single virtual position for uniform coloring.
270-282: LGTM!Side mapping correctly places 5 sides × 12 LEDs + 1 middle LED = 61 total, matching the default
nrOfLightsPerCube.
341-352: LGTM!Granularity switch correctly maps to the three rendering strategies, and
nextPin()appropriately assigns all cubes to a single output pin.src/MoonLight/Layers/VirtualLayer.h (2)
80-80: Type migration to nrOfLights_t successfully enables larger LED counts.The changes to
nrOfLights,mappingTableSize,mappingTableIndexesSizeUsed, and vector element types are consistent with the PR goal of supporting up to 130K LEDs. On PSRAM boards,nrOfLights_texpands to uint32_t with 24-bit PhysMap indices (16M capacity); on non-PSRAM boards it remains uint16_t with 14-bit indices (16K capacity).This supersedes the previous learning that "mappingTableSize is uint16_t with a maximum of 65535 LEDs."
Based on learnings, the previous 65535 LED limit is now lifted on PSRAM boards.
Also applies to: 92-94, 97-97
114-220: Function signatures consistently migrated to nrOfLights_t.All virtual LED indexing functions (
addIndexP,XYZ,XYZUnModified,setRGB,setWhite,setBrightness,setPan,setTilt,setZoom,setRotate,setGobo,setRGB1/2/3,setBrightness2,setLight,getRGB,blendColor,getWhite,getRGB1/2/3,getLight) have been updated to usenrOfLights_tfor index parameters and return values. The migration is complete and maintains type safety across the virtual layer API.
Docs ==== - Architecture: update core assignments Back end ======== - Lights control: use headerPrimeNumber for clarity, add PSRAM error check - MoonLight Layouts: tweak Toronto Bar Cube Layout, add pin assignment for every 10 cubes
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/MoonLight/Layers/PhysicalLayer.h (1)
73-92: Consider removing commented code.The commented-out line 76 with explanation is clear, but for cleaner code you could remove the commented assignment and keep only the explanatory comment:
🔎 Suggested cleanup
channelsPerLight = 3; // RGB default - // lightPreset = lightPreset_GRB; // don't reset as managed by Drivers + // Note: lightPreset is not reset here as it's managed by Drivers offsetRGB = 0;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/develop/architecture.mddocs/moonbase/tasks.mdinterface/src/routes/moonbase/monitor/Monitor.sveltelib/framework/ESP32SvelteKit.cpplib/framework/EventEndpoint.hlib/framework/HttpEndpoint.hlib/framework/WWWData.hlib/framework/WebSocketServer.hplatformio.inisrc/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/main.cpp
💤 Files with no reviewable changes (5)
- lib/framework/EventEndpoint.h
- src/MoonBase/Module.h
- lib/framework/HttpEndpoint.h
- lib/framework/WebSocketServer.h
- src/MoonBase/Module.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main.cpp
- interface/src/routes/moonbase/monitor/Monitor.svelte
- src/MoonLight/Nodes/Layouts/L_MoonLight.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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.
📚 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:
docs/moonbase/tasks.mddocs/develop/architecture.mdplatformio.ini
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
docs/moonbase/tasks.mddocs/develop/architecture.mdplatformio.ini
📚 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.cppdocs/develop/architecture.mdsrc/MoonLight/Modules/ModuleLightsControl.hplatformio.ini
📚 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/PhysicalLayer.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Layers/VirtualLayer.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/Layers/PhysicalLayer.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: Be aware of the integrated build process where the PlatformIO build automatically builds and embeds the SvelteKit frontend into the ESP32 firmware
Applied to files:
lib/framework/ESP32SvelteKit.cppplatformio.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:
lib/framework/ESP32SvelteKit.cppplatformio.ini
📚 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.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:
platformio.ini
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
lib/framework/ESP32SvelteKit.cpp (1)
226-234: Configuration already correct—no changes needed.The SvelteKit loop task is already pinned to Core 0 via the
ESP32SVELTEKIT_RUNNING_CORE=0compiler flag inplatformio.ini, aligning with the PR objective to colocate SvelteKit (computation/effects) with WiFi on Core 0. Task priority (tskIDLE_PRIORITY + 2) and stack sizing (8 KB with PSRAM, 6 KB without) are appropriate for the framework's requirements.platformio.ini (2)
66-66: LGTM: SvelteKit correctly assigned to Core 0.The
ESP32SVELTEKIT_RUNNING_CORE=0setting correctly places the networking/web server task on Core 0 (PRO_CPU) alongside Wi-Fi, as recommended in the PR objectives. This allows the timing-critical driver task to run uninterrupted on Core 1.Based on learnings: Driver tasks require Core 1 isolation; networking tasks colocate with Wi-Fi on Core 0.
218-219: Driver library updated to latest version with bug fixes.The I2SClocklessLedDriver was updated to commit
4363655d39(Jan 2, 2026), which adds deleteDriver checks on DMABuffersTampon. This aligns with the PR's focus on supporting larger LED counts and improved memory management.src/MoonLight/Layers/PhysicalLayer.cpp (2)
40-41: LGTM: Expanded channel capacity for large LED installations.The updated maxChannels calculation now supports up to 393,216 channels (128×64×16×3), enabling 16 Hub75 128×64 panels or equivalent LED configurations. The MIN() ensures the limit doesn't exceed PSRAM/4, maintaining memory safety for double buffering.
186-186: LGTM: Type widened to support >65K LEDs.Changed from
uint16_ttonrOfLights_t, which conditionally expands touint32_twhen PSRAM is available. This enables the accumulator to handle LED counts exceeding 65,535 without overflow.src/MoonLight/Modules/ModuleLightsControl.h (2)
78-80: Good defensive check for PSRAM availability.The runtime check ensures that boards configured with
BOARD_HAS_PSRAMactually have PSRAM available at boot. This helps catch hardware configuration mismatches early, which is important since the code relies on PSRAM for double buffering and large LED counts.
496-511: Monitor protocol updated to support expanded header.The header size increased from 37 to 41 bytes (
headerPrimeNumber) to accommodate the expandedLightsHeaderstructure that supports >65K LEDs. Thestatic_assertprovides compile-time safety, ensuring the header is always large enough for the monitor protocol.Note: The prime number choice (41) helps the monitor distinguish between header frames and data frames by making collisions unlikely.
docs/develop/architecture.md (1)
296-296: LGTM: Correct single-buffer fallback.When PSRAM is not available,
channelsDcorrectly points to the same buffer aschannelsE, eliminating the overhead of double buffering on memory-constrained devices.src/MoonLight/Layers/VirtualLayer.h (3)
30-66: LGTM! PhysMap constructor fix properly applied.The constructor now correctly handles initialization for both PSRAM and non-PSRAM builds:
- PSRAM: Uses
memset(rgb, 0, 3)for the 4-byte struct- Non-PSRAM: Uses
rgb = 0for the 2-byte structThis addresses the buffer overrun issue flagged in the previous review. The no-op
sizeof(PhysMap);statement has also been removed as recommended.
104-210: LGTM! Method signatures consistently updated to nrOfLights_t.All method signatures have been uniformly updated from
uint16_ttonrOfLights_tfor virtual LED indexing:
addIndexP,XYZ,XYZUnModified- All
setRGB,setWhite,setBrightness,setPan,setTilt, etc. methods- All corresponding
getRGB,getLight, etc. methodsThe conditional logic in
setWhite(lines 127-130) correctly handles both RGBW lights (4 channels, white at offset 3) and moving heads (variable channel layout usingoffsetWhite).The changes maintain API consistency and enable support for up to 130K LEDs when PSRAM is available.
76-84: Verify overflow handling for large virtual layer dimensions.The type changes from
uint16_ttonrOfLights_t(uint32_t with PSRAM) expand the supported LED count beyond the previous 65535 limit. However, there is no validation for overflow when calculatingmappingTableSize = size.x * size.y * size.z(line 342). If a user defines dimensions that multiply to exceed UINT32_MAX, the calculation overflows silently, leading to undefined behavior or memory corruption.Additionally,
mappingTableIndexesnow usesnrOfLights_telements, doubling the memory footprint per element compared to the previousuint16_t. With complex mappings on large virtual layers (e.g., 128×128×16), this could consume significant PSRAM.The current code gracefully handles allocation failure by retaining the previous mapping (line 344), but this assumes the dimension overflow is caught by the realloc itself. Add explicit validation:
if (size.x * size.y * size.z > UINT32_MAX / sizeof(PhysMap))or equivalent bounds check before allocation.src/MoonLight/Layers/PhysicalLayer.h (2)
31-35: Runtime PSRAM validation is properly implemented; no changes needed.The typedef is safe. The code includes runtime checks via
psramFound()(PhysicalLayer.cpp:39), memory allocation usingheap_caps_malloc_prefer()with fallback to internal RAM (Utilities.h), and explicit handling of allocation failures (PhysicalLayer.cpp:49-64) with error logging. Large allocations do not proceed without confirmed PSRAM availability at runtime.
40-67: No changes required - Monitor protocol implementation is correct.The LightsHeader struct changes and Monitor.svelte have been properly coordinated. Only 41 bytes (not the full 42-byte struct) are transmitted to the frontend as a deliberate design choice—41 is a prime number unlikely to match
nrOfChannels, allowing the monitor to distinguish header packets from channel data packets. Monitor.svelte correctly expects this 41-byte header and parses all new struct fields at the proper offsets (e.g.,nrOfLightsat offset 12,nrOfChannelsat offset 16,lightPresetat offset 20, etc.). The protocol remains consistent between PhysicalLayer.h and Monitor.svelte.
UI == - page.svelte: replace 'start' by 'wifi/io/drivers/effects' Backend ======= - Nodes: DriverNode: no correctedBrightness if nroflights > 8K - Art-Net out: change universeSize to control, add read only values for usedChannels, total universes, total channels - WIP
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @interface/src/routes/+page.svelte:
- Around line 21-51: The inline onclick handlers on the anchor elements (the
WiFi, IO, Drivers, and Effects links in +page.svelte) call
notifications.success('You did it!', 1000) immediately, which is misleading;
remove those notifications.success calls (or replace them with a neutral
notification like notifications.info('Opening setup...') if you want feedback)
from the anchor onclick attributes, and instead trigger a success notification
from the actual setup completion logic on the respective pages/components (e.g.,
the WiFi, module inputoutput, drivers, and effects pages) once the configuration
is confirmed.
- Around line 70-74: Update the anchor text for the YouTube link in
+page.svelte: locate the <a> element with href
"https://www.youtube.com/@MoonModulesLighting" (class "link", target "_blank")
and change its inner text from "Youtube" to the proper brand capitalization
"YouTube".
In @src/main.cpp:
- Around line 324-340: The core assignments for the FreeRTOS tasks are inverted:
swap the core parameter in the xTaskCreateUniversal calls so effectTask is
pinned to core 0 (PRO_CPU) and driverTask is pinned to core 1 (APP_CPU); locate
the xTaskCreateUniversal invocation for effectTask and change its last argument
from 1 to 0, and locate the xTaskCreateUniversal invocation for driverTask and
change its last argument from 0 to 1, and optionally update or remove the
comment on the driverTask line to reflect that driver is intentionally pinned to
APP_CPU for timing-critical DMA/I2S/LCD/PARLIO isolation from WiFi preemption.
In @src/MoonLight/Nodes/Drivers/D_ArtnetOut.h:
- Line 47: The addControl call that registers totalChannels uses an overly small
max (currently addControl(totalChannels, "total #", "number", 0, 65538, true));
update that max to match the uint32_t storage (either remove the restrictive
max, use UINT32_MAX, or set a practical upper bound like 500000) and ensure any
UI/validation that relies on this value is consistent with the new limit; modify
the addControl invocation accordingly so totalChannels can represent the full
expected range.
- Around line 45-47: The UI labels for the controls are duplicated: the
addControl calls for totalUniverses and totalChannels both use the label "total
#", which is confusing; update the addControl invocation for totalUniverses to
use a distinct label like "total universes" and update totalChannels to use
"total channels" (leave channelsPerOutput unchanged) so the UI shows unique,
descriptive names; locate the calls to addControl for the symbols totalUniverses
and totalChannels in D_ArtnetOut.h and change their label string arguments
accordingly.
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/Drivers/D_ArtnetOut.h (1)
121-121:processedOutputsnot reset at frame start — potential data corruption.
processedOutputsis a class member that persists acrossloop()calls. It's only reset when switching IPs (line 187), but if there's only one IP, it accumulates indefinitely. After the first frame,processedOutputs >= nrOfOutputsPerIPwill always be true, causinguniverseto reset to 0 within each frame iteration, leading to incorrect packet addressing.Reset
processedOutputsat the start of each frame alongsideuniverseandpacketSize.🔎 Proposed fix
universe = 0; packetSize = 0; channels_remaining = channelsPerOutput; + processedOutputs = 0;Also applies to: 151-152
src/MoonBase/Nodes.cpp (1)
373-379: Clarify the 8096 LED threshold for power calculation bypass.The bypass threshold (8096 LEDs) is inconsistent with the actual limits:
calculate_max_brightness_for_power_mWsupports up to 65535 LEDs (UINT16_MAX)- System can support ~131K LEDs with PSRAM (via
lights.maxChannelsallocation: 128×64×16×3)Issues:
- Between 8097 and 65535 LEDs, the power calculation is bypassed unnecessarily
- Above 65535 LEDs,
MIN(..., UINT16_MAX)clamps to 65535, calculating power for only 65K LEDs when more existSuggestion: Consider either:
- Raising the threshold to 65535 to use power calculation where supported
- Documenting why 8096 was chosen and the power supply implications for large installations
- Implementing a more sophisticated power model for >65K setups (or warning users about power requirements)
Do you have testing data showing safe power operation above 8096 LEDs? For a 130K LED installation, assuming "decent power" without calculation could lead to unexpected behavior.
♻️ Duplicate comments (1)
docs/develop/architecture.md (1)
10-15: Resolve documentation inconsistency before merging.The table (lines 10-15) and code example (lines 317-333) still show contradictory core assignments:
- Table: Effect Task on Core 0, Driver Task on Core 1
- Code:
effectTaskon Core 1,driverTaskon Core 0The "Parking as currently experimenting" notes (lines 22-23, 29-30) indicate this is experimental, but leaving this inconsistency in documentation will confuse users.
Before merging:
- If experiments confirm the reversed configuration works better, update the table to match
- If experiments show issues, revert the code to match the table
- Document the rationale and test results
This issue was previously discussed with your response "Currently experimenting with Driver on Core 0 and effects on Core 1." Please ensure documentation consistency before final merge.
Also applies to: 317-333
🧹 Nitpick comments (3)
interface/src/routes/+page.svelte (2)
52-60: Consider adding accessible labels for emoji indicators.The 🛟 emoji provides a visual cue but screen readers may not convey its meaning clearly to users. Consider wrapping emojis in
<span role="img" aria-label="Help">or adding descriptive text.
61-84: Consider adding accessible labels for emoji indicators.Similar to the Help section, the 👀 emoji at line 62 provides a visual cue but may not be clear to screen reader users. Consider wrapping emojis in
<span role="img" aria-label="Social links">or adding descriptive text for better accessibility.src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
267-279:nrOfLightsPerCubecontrol only affects granularity mode 0.The
addCubeSides()method hardcodes 5 sides × 12 LEDs + 1 middle = 61 LEDs, ignoring thenrOfLightsPerCubesetting. The same applies toaddCubePixels().This is likely intentional since modes 1 and 2 have fixed pixel patterns, but consider adding a comment in
setup()or making the control description clearer, e.g.,"nrOfLightsPerCube (mode 0 only)".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/develop/architecture.mddocs/moonbase/tasks.mdinterface/src/routes/+page.sveltelib/framework/WWWData.hsrc/MoonBase/Nodes.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Nodes/Drivers/D_ArtnetOut.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/moonbase/tasks.md
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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.
📚 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/interface/src/routes/**/*.svelte : Create Svelte routes in `interface/src/routes/` for different pages of the web application
Applied to files:
interface/src/routes/+page.svelte
📚 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/interface/**/*.{svelte,ts,tsx,js,json} : Modify frontend code in the `interface/` directory using SvelteKit
Applied to files:
interface/src/routes/+page.svelte
📚 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/interface/svelte.config.js : Configure SvelteKit build and adapter settings in `interface/svelte.config.js`
Applied to files:
interface/src/routes/+page.svelte
📚 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:
docs/develop/architecture.mdsrc/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/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
docs/develop/architecture.md
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
docs/develop/architecture.mdsrc/main.cpp
📚 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:
docs/develop/architecture.mdsrc/MoonLight/Layers/PhysicalLayer.hsrc/main.cpp
📚 Learning: 2026-01-04T09:31:01.321Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: lib/framework/ESP32SvelteKit.cpp:229-229
Timestamp: 2026-01-04T09:31:01.321Z
Learning: In lib/framework/ESP32SvelteKit.cpp, the SvelteKit loop task stack size of 6KB (6 * 1024 bytes) for non-PSRAM devices has been validated through testing, showing approximately 1500 bytes of free stack remaining under load, providing adequate ~24% headroom.
Applied to files:
docs/develop/architecture.mdsrc/main.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: 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/PhysicalLayer.h
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (4)
src/MoonBase/Nodes.h (4)
Node(67-182)Node(238-238)addLight(228-228)nextPin(231-231)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onLayout(40-40)src/MoonLight/Nodes/Layouts/L__Sandbox.h (1)
onLayout(36-46)src/MoonLight/Nodes/Drivers/D_Hub75.h (1)
onLayout(26-26)
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (2)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
setup(29-139)src/MoonBase/Module.cpp (2)
addControl(353-362)addControl(353-353)
⏰ 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 (10)
interface/src/routes/+page.svelte (1)
19-20: LGTM!The bold attribution text clearly identifies the project and its creator.
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (4)
26-33: LGTM!The new configuration fields and calculated statistics provide good flexibility for Art-Net topology management with large LED counts.
81-87: Calculations look correct.The ceiling division for
totalUniversesand the rounding logic forusedChannelsPerUniverseare implemented correctly. Consider guarding againstchannelsPerLight == 0if it's ever possible (would cause division by zero), though this is likely validated at the layer level.
94-94: Buffer size is adequate.With 18-byte header and max
universeSize=1000, the 1024-byte buffer provides sufficient capacity (1018 bytes needed).
110-110: Defensive clamping is appropriate.The
MIN(packetSize, universeSize)provides a safety bound, though the loop logic at line 175 should already preventpacketSizefrom exceedinguniverseSize.src/MoonLight/Nodes/Layouts/L_MoonLight.h (5)
244-260: LGTM!The class declaration, static identifiers, and
setup()follow the established patterns from other layout classes in this file.
262-265: LGTM!The
addCube()method correctly maps all physical LEDs in a cube to a single virtual pixel position for the "One Cube One Light" granularity mode.
281-300: LGTM!The
addCubePixels()method correctly maps individual LEDs for each of the 5 cube faces (front, back, left, right, bottom) using a 12-pixel square outline pattern plus a center LED. The coordinate transformations are appropriate for each face orientation.
302-336: LGTM!The cubes array correctly defines a 4×3×2 grid (24 cubes total). The previous duplicate coordinate issue has been fixed.
338-354: LGTM!The
onLayout()loop correctly iterates through cubes, applies the selected granularity mode, and assigns pins every 10 cubes with proper handling for the final partial group. This follows the same pattern used inPanelsLayout.
|
@coderabbitai, in D_ArtNetOut.h I (re)calculate some readonly controls and then set moduleNodes->requestUIUpdate = true; |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 73 🏁 Script executed: Length of output: 6248 🏁 Script executed: Length of output: 8653 🏁 Script executed: Length of output: 3823 🏁 Script executed: Length of output: 81 🏁 Script executed: Length of output: 43 🏁 Script executed: Length of output: 3510 🏁 Script executed: Length of output: 1138 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 9889 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 127 🏁 Script executed: Length of output: 2292 🏁 Script executed: Length of output: 87 🏁 Script executed: Length of output: 1569 🏁 Script executed: Length of output: 4595 🏁 Script executed: Length of output: 919 🏁 Script executed: Length of output: 4277 🏁 Script executed: Length of output: 1283 @ewowi, I found the issue! You're updating the C++ member variables but not updating the JSON control values that get sent to the UI. The ProblemWhen you set The SolutionYou need to call updateControl("status", statusString.c_str());
moduleNodes->requestUIUpdate = true;Code Fix for D_ArtnetOut.hIn your totalChannels = layerP.lights.header.nrOfLights * layerP.lights.header.channelsPerLight;
usedChannelsPerUniverse = universeSize / layerP.lights.header.channelsPerLight * layerP.lights.header.channelsPerLight;
totalUniverses = (totalChannels + usedChannelsPerUniverse - 1) / usedChannelsPerUniverse;
moduleNodes->requestUIUpdate = true;To: totalChannels = layerP.lights.header.nrOfLights * layerP.lights.header.channelsPerLight;
usedChannelsPerUniverse = universeSize / layerP.lights.header.channelsPerLight * layerP.lights.header.channelsPerLight;
totalUniverses = (totalChannels + usedChannelsPerUniverse - 1) / usedChannelsPerUniverse;
// Update the JSON control values so UI sees the changes
updateControl("used", usedChannelsPerUniverse);
updateControl("totalUniverses", totalUniverses); // See note below about naming!
updateControl("totalChannels", totalChannels);
moduleNodes->requestUIUpdate = true;
|
|
@coderabbitai, I got this crash, what does it mean? assert failed: lwip_netconn_do_write /IDF/components/lwip/lwip/src/api/api_msg.c:1919 (state!) Backtrace: 0x4037ea49:0x3fcbd150 0x4037ea11:0x3fcbd170 0x40385e8e:0x3fcbd190 0x420d2612:0x3fcbd2d0 0x420bec68:0x3fcbd2f0 0x420d0ccb:0x3fcbd310 0x420d0f8d:0x3fcbd330 0x420d0fd9:0x3fcbd380 0x420bda64:0x3fcbd3b0 0x420f93dd:0x3fcbd3e0 0x420f9bad:0x3fcbd400 0x4201a1cb:0x3fcbd440 0x4201a1ed:0x3fcbd460 0x4202eb15:0x3fcbd490 0x4202ec79:0x3fcbd4f0 0x4202ede1:0x3fcbd560 0x4202a635:0x3fcbd600 0x4202a6ad:0x3fcbd6a0 0x42028bd4:0x3fcbd6c0 0x42028c95:0x3fcbd700 0x4037fd89:0x3fcbd720 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9633 🏁 Script executed: Length of output: 6699 🏁 Script executed: Length of output: 651 🌐 Web query: 💡 Result: Short answer: that assertion means lwIP's netconn was not in the NETCONN_WRITE state when lwip_netconn_do_write expected it — i.e. your code (or another task) tried to write or close the socket while the connection was being closed/was already closed. It's a race between netconn_write / sent callback / netconn_close (or a write issued from the wrong context), and on ESP builds there are a few platform-specific timing/close-handling quirks that expose it. Why it happens (brief):
How to fix (practical checklist):
If you want, paste the crash log + minimal repro (which websocket library and ESP-IDF version you're using) and I’ll point to the exact line and recommend the safest code change. Sources: lwIP netconn/api implementation and behavior, discussion about write/close races, and ESP-specific netconn changes. [1][2][3] References @ewowi, this is a classic race condition between WebSocket disconnect and broadcast — you're trying to send to a closed or closing socket. Let me break down what's happening and how to fix it. What the Crash MeansThe assertion
The Root Cause in Your CodeLooking at EventSocket.cpp lines 234-238: auto *client = _socket.getClient(subscription);
if (!client)
{
subscriptions.remove(subscription);
continue;
}This checks if the client object exists, but not if the underlying socket connection is still valid/open. The client object can persist in memory while the socket is dead. Recommended FixesOption 1: Check Connection State Before Sending (Best)Wrap the // In EventSocket.cpp, around line 234
auto *client = _socket.getClient(subscription);
if (!client)
{
subscriptions.remove(subscription);
continue;
}
// Check if the socket is still valid before sending
httpd_ws_client_info_t ws_info = httpd_ws_get_fd_info(client->server(), client->socket());
if (ws_info != HTTPD_WS_CLIENT_WEBSOCKET)
{
ESP_LOGW(SVK_TAG, "Client socket %d is no longer a valid WebSocket, removing subscription", client->socket());
subscriptions.remove(subscription);
continue;
}
if (event != "monitor")
ESP_LOGV(SVK_TAG, "Emitting event: %s to %s[%u], Message[%d]: %s", event.c_str(), client->remoteIP().toString().c_str(), client->socket(), len, output);
#if FT_ENABLED(EVENT_USE_JSON)
esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_TEXT, output, len);
#else
esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_BINARY, output, len);
#endif
// If send failed, remove the subscription
if (result != ESP_OK)
{
ESP_LOGW(SVK_TAG, "Failed to send to client %d: %s, removing subscription", client->socket(), esp_err_to_name(result));
subscriptions.remove(subscription);
}Option 2: Handle Send Errors Gracefully (Simpler)If checking state is too complex, just handle the error return: // Around line 245 in EventSocket.cpp
#if FT_ENABLED(EVENT_USE_JSON)
esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_TEXT, output, len);
#else
esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_BINARY, output, len);
#endif
if (result != ESP_OK)
{
ESP_LOGW(SVK_TAG, "Failed to send event %s to client %d: %s", event.c_str(), client->socket(), esp_err_to_name(result));
subscriptions.remove(subscription);
}However, this might not prevent the crash if the assertion fires inside Option 3: Add Connection State Check in PsychicWebSocket (Defense in Depth)Modify PsychicWebSocket.cpp to validate before calling lwIP: // In PsychicWebSocketClient::sendMessage (around line 57)
esp_err_t PsychicWebSocketClient::sendMessage(httpd_ws_frame_t * ws_pkt)
{
// Check if socket is still valid before sending
httpd_ws_client_info_t ws_info = httpd_ws_get_fd_info(this->server(), this->socket());
if (ws_info != HTTPD_WS_CLIENT_WEBSOCKET)
{
return ESP_ERR_INVALID_STATE; // Socket is closed/invalid
}
return httpd_ws_send_frame_async(this->server(), this->socket(), ws_pkt);
}Why This HappensThe ESP-IDF WebSocket library is asynchronous. When a client disconnects:
During that window, if Immediate ActionI recommend Option 2 (error handling) as the quickest fix + Option 3 (validation in PsychicWebSocket) as defense in depth. This way:
Would you like me to prepare a complete patch for EventSocket.cpp and PsychicWebSocket.cpp? 🧠 Learnings used |
Frontend ======== - Page.svelte: tweaks BackEnd ======= - EventSocket: error check on sendMessage - Virtual layer: add oneToOneMapping check for mappingless mapping , set/getLight: tune no mapping - Art-Net out: read only variables fix, processedOuputs reset
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 (2)
docs/develop/architecture.md (2)
9-15: 🔴 Fix SvelteKit core assignment in table (must be Core 0, not Core 1).The table shows SvelteKit on Core 1 (APP_CPU), but PR objectives and platformio.ini both require SvelteKit on Core 0 (PRO_CPU) to colocate with Effects and WiFi. The table is your primary reference and it is incorrect.
🔎 Proposed fix for the table
| **Effect Task** | 0 (PRO_CPU) | 5 | 3-4KB | ~60 fps | Calculate LED colors and effects | - | **ESP32SvelteKit** | 1 (APP_CPU) | 2 | System | 10ms | HTTP/WebSocket UI framework | + | **ESP32SvelteKit** | 0 (PRO_CPU) | 2 | System | 10ms | HTTP/WebSocket UI framework | | **Driver Task** | 1 (APP_CPU) | 3 | 3-4KB | ~60 fps | Output data to LEDs via DMA/I2S/LCD/PARLIO |
303-306: Update build flag: SvelteKit core should be 0 (matching updated platformio.ini).This section shows:
-DESP32SVELTEKIT_RUNNING_CORE=1But platformio.ini line 66 was just changed to
ESP32SVELTEKIT_RUNNING_CORE=0. Update this example to match:-DESP32SVELTEKIT_RUNNING_CORE=0
🤖 Fix all issues with AI Agents
In @lib/framework/EventSocket.cpp:
- Around line 208-217: In EventSocket.cpp fix two consistency issues: (1)
replace the range-based loop over subscriptions (the loop using subscription
variable in the EventSocket::dispatch/send logic) with an iterator-based loop
(or use erase-remove idiom) so you can safely call subscriptions.erase(it) when
a send fails instead of subscriptions.remove(subscription) which invalidates the
iterator; (2) in the onlyToSameOrigin branch where client->sendMessage(...) is
called for the origin client, capture the esp_err_t result and apply the same
error handling as the broadcast path (log via ESP_LOGW with
esp_err_to_name(result) and remove the subscription from subscriptions when
result != ESP_OK) so both paths handle send/close races consistently.
In @src/MoonLight/Layers/VirtualLayer.cpp:
- Around line 210-218: The unmapped get path wrongly casts a single byte to T
(losing bytes for multi-byte types like CRGB); change the return to read
sizeof(T) bytes from channelsE by casting the address at index to T* and
dereferencing it (same pattern as the mapped path used around the getLight
mapping code), and ensure you still check bounds against
layerP->lights.maxChannels before dereferencing.
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
254-260: Consider adding a description to clarify control applicability.The
nrOfLightsPerCubeslider only affects granularity mode 0 ("One Cube One Light"), but this isn't obvious to users from the UI. Consider adding adescparameter to guide users.🔎 Suggested improvement
- addControl(nrOfLightsPerCube, "nrOfLightsPerCube", "slider", 1, 128); // mode 0 only + addControl(nrOfLightsPerCube, "nrOfLightsPerCube", "slider", 1, 128, false, "Only used in 'One Cube One Light' mode");src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
82-94: Calculation and UI sync logic is correct.The calculations for
totalChannels,usedChannelsPerUniverse, andtotalUniversesare accurate (ceiling division ensures all LEDs are covered). CallingupdateControl()for each calculated value before settingrequestUIUpdatecorrectly syncs the read-only controls to the UI, addressing the PR objective to fix Art-Net read-only control synchronization.Minor: Log format inconsistency on line 93.
The format string uses
#c%dbut the pattern established earlier isc/u:%d #u:%d, so for consistency consider#c:%d.🔎 Proposed fix
- EXT_LOGD(ML_TAG, "c/u:%d #u:%d #c%d (%d)", usedChannelsPerUniverse, totalUniverses, totalChannels, totalUniverses * usedChannelsPerUniverse); + EXT_LOGD(ML_TAG, "c/u:%d #u:%d #c:%d (%d)", usedChannelsPerUniverse, totalUniverses, totalChannels, totalUniverses * usedChannelsPerUniverse);lib/framework/EventSocket.cpp (1)
178-191: Consider adding error checking to the origin-only send path for consistency.The broadcast path (lines 208-217) now checks
sendMessagereturn values and removes failed subscriptions. However, theonlyToSameOriginpath at lines 186-189 still ignores send failures.For consistency and robustness, consider capturing and checking the result here as well, though the impact is lower since this affects only a single origin client rather than all subscribers.
Proposed fix to add error checking
if (client) { if (event != "monitor") ESP_LOGV(SVK_TAG, "Emitting event: %s to %s[%u], Message[%d]: %s", event.c_str(), client->remoteIP().toString().c_str(), client->socket(), len, output); #if FT_ENABLED(EVENT_USE_JSON) - client->sendMessage(HTTPD_WS_TYPE_TEXT, output, len); + esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_TEXT, output, len); #else - client->sendMessage(HTTPD_WS_TYPE_BINARY, output, len); + esp_err_t result = client->sendMessage(HTTPD_WS_TYPE_BINARY, output, len); +#endif + if (result != ESP_OK) + { + ESP_LOGW(SVK_TAG, "Failed to send event %s to origin client %d: %s", event.c_str(), client->socket(), esp_err_to_name(result)); + subscriptions.remove(originSubscriptionId); + } #endif }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/develop/architecture.mdinterface/src/routes/+page.sveltelib/framework/EventSocket.cpplib/framework/WWWData.hplatformio.inisrc/MoonBase/Nodes.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Drivers/D_ArtnetOut.hsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- interface/src/routes/+page.svelte
- src/MoonBase/Nodes.cpp
- src/main.cpp
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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.
📚 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:
docs/develop/architecture.mdplatformio.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:
docs/develop/architecture.mdplatformio.ini
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
docs/develop/architecture.mdplatformio.ini
📚 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:
docs/develop/architecture.mdplatformio.ini
📚 Learning: 2026-01-04T09:31:07.533Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: lib/framework/ESP32SvelteKit.cpp:229-229
Timestamp: 2026-01-04T09:31:07.533Z
Learning: In lib/framework/ESP32SvelteKit.cpp, the SvelteKit loop task stack size of 6KB (6 * 1024 bytes) for non-PSRAM devices has been validated through testing, showing approximately 1500 bytes of free stack remaining under load, providing adequate ~24% headroom.
Applied to files:
docs/develop/architecture.md
📚 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/Drivers/D_ArtnetOut.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/VirtualLayer.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.cppsrc/MoonLight/Layers/VirtualLayer.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:
platformio.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: Be aware of the integrated build process where the PlatformIO build automatically builds and embeds the SvelteKit frontend into the ESP32 firmware
Applied to files:
platformio.ini
🧬 Code graph analysis (4)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
src/MoonBase/Module.cpp (2)
addControl(353-362)addControl(353-353)
src/MoonLight/Layers/VirtualLayer.cpp (2)
src/MoonLight/Layers/VirtualLayer.h (3)
setRGB(113-124)setRGB(125-125)XYZUnModified(111-111)src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (3)
src/MoonBase/Nodes.h (4)
Node(67-182)Node(238-238)addLight(228-228)nextPin(231-231)src/MoonLight/Layers/VirtualLayer.cpp (2)
addLight(367-399)addLight(367-367)src/MoonLight/Nodes/Layouts/L__Sandbox.h (1)
onLayout(36-46)
src/MoonLight/Layers/VirtualLayer.h (1)
src/MoonLight/Layers/VirtualLayer.cpp (12)
addIndexP(74-103)addIndexP(74-74)XYZ(104-112)XYZ(104-104)setLight(125-169)setLight(125-125)getLight(172-220)getLight(172-172)createMappingTableAndAddOneToOne(315-334)createMappingTableAndAddOneToOne(315-315)onLayoutPost(401-446)onLayoutPost(401-401)
⏰ 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 (17)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
262-300: LGTM!The three helper methods correctly implement different granularity levels:
addCube: Maps all physical LEDs to one virtual position (mode 0)addCubeSides: Maps 12 LEDs per side to 5 virtual pixels + 1 center (mode 1)addCubePixels: Maps each of 61 LEDs to distinct virtual positions in a 7×7×7 grid (mode 2)The pixel pattern correctly traces the 12-LED outline of each face, and the 5-face design (no top) matches the physical cube structure.
302-355: LGTM!The
onLayoutimplementation correctly:
- Defines a 4×3×2 grid of 24 cubes (verified: z=0 layer at lines 307-320, z=1 layer at lines 322-336)
- Dispatches to the appropriate helper based on granularity mode
- Assigns pins every 10 cubes with proper handling of the final partial group
The previously reported duplicate coordinate issue has been addressed.
platformio.ini (2)
66-66: ✅ Correct core assignment for SvelteKit, but verify architecture.md alignment.Setting
ESP32SVELTEKIT_RUNNING_CORE=0is correct per PR objectives (SvelteKit on Core 0 with Effects/WiFi). However, the architecture.md table (line 14) currently shows SvelteKit on Core 1, creating a mismatch between config and documentation. Ensure architecture.md is updated to reflect this choice.
218-219: Verify I2SClocklessLedDriver upgrade is non-breaking.The driver library hash changed from
131988ed...to4363655d...with the note "Add deleteDriver checks on DMABuffersTampon". Confirm:
- This upgrade is compatible with the current physical layer code.
- The deleteDriver checks don't introduce new validation constraints that could break existing mappings.
- No API changes affect the driver initialization or buffer management paths.
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (3)
49-59: Art-Net header construction is correct.The inline initialization properly constructs the 18-byte Art-Net header with the correct OpCode (0x5000 ArtDMX, little-endian), protocol version 14, and placeholder fields for sequence, universe, and length. Dynamic fields are correctly filled at runtime in
writePackage().
116-116: DefensiveMINcheck is good practice.The
MIN(packetSize, universeSize)bounds the write size even though the loop guard at line 182 should preventpacketSizefrom exceedinguniverseSize. This defensive check protects against future logic changes and is a reasonable safety measure.
160-160:processedOutputsreset is correct.Resetting
processedOutputsto zero at the start of each frame (line 160) ensures proper IP rotation when distributing outputs across multiple Art-Net controllers, matching the PR objective to fixprocessedOutputsreset.src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
135-135: LGTM! UI label refinement.The control label shortened from "swirlVal" to "swirl" improves UI clarity while preserving the underlying variable name.
src/MoonLight/Layers/VirtualLayer.cpp (5)
20-20: Good documentation of FastLED constraint.The comment correctly notes FastLED's
fadeToBlackByis limited to UINT16_MAX LEDs. Line 238's guard (layerP->lights.header.nrOfChannels / 3 < UINT16_MAX) ensures this limit is respected before calling the FastLED function.
149-167: oneToOneMapping optimization correctly implemented.The new code paths handle two scenarios:
m_zeroLights case (lines 149-159): Stores color in the mappingTable itself when a virtual LED has no physical mapping. PSRAM builds store full 24-bit RGB; non-PSRAM builds pack to 14-bit (5-5-4).
Unmapped path (lines 161-167): When
indexV >= mappingTableSize, computes direct channel index. This is the fast path foroneToOneMappinglayouts where virtual indices map 1:1 to physical indices and the mapping table has been freed (seeonLayoutPostlines 414-418).The logic correctly optimizes away mapping table overhead for regular layouts.
315-334: LGTM! Transition from oneToOneMapping to mapped mode is sound.The function correctly:
- Reallocates the mapping table to accommodate the virtual space size (with fallback on failure)
- Zeros the table (with null-pointer guard)
- Fills 1:1 entries for all existing physical lights (
indexV == indexP)The
MIN(layerP->indexP, mappingTableSize)bound at line 331 prevents overflow when the realloc partially succeeds or layout size changes.
364-398: oneToOneMapping lazy detection is well-designed.The optimization assumes 1:1 mapping until proven otherwise, then retroactively builds the mapping table:
- Line 364: Assumes
oneToOneMappingat the start of layout- Lines 377-380: Detects irregularity when
layerP->indexP != indexV(physical ≠ virtual index)- Line 379: Calls
createMappingTableAndAddOneToOne()to fill all previously processed lights (0..layerP->indexP-1) with 1:1 entries- Lines 384-388: Continues mapping subsequent lights (including the current irregular one)
- Lines 390-394: Also detects unmapped lights (position.x == UINT16_MAX from modifiers like checkerboard)
The lazy evaluation correctly avoids mapping table overhead for regular layouts while seamlessly transitioning to mapped mode when needed.
406-443: Memory optimization for regular layouts is correct.When
oneToOneMappingis true (lines 406-418), the code frees the mapping table and index arrays, saving memory. SubsequentsetLight/getLightcalls will hit the unmapped fast path (lines 161-167, 210-218) for direct channel access.For irregular layouts (lines 419-443), the code counts and logs mapping statistics, which aids debugging and performance analysis.
src/MoonLight/Layers/VirtualLayer.h (4)
30-66: PhysMap union layout is correct and memory-efficient.The conditional PSRAM layout trades space for capacity:
- PSRAM (4 bytes): 24-bit indices (16M LEDs), full 24-bit RGB,
mapTypestored in byte 3- Non-PSRAM (2 bytes): 14-bit indices (16K LEDs), packed 5-5-4 RGB,
mapTypein top 2 bitsThe union views (
rgb,indexP,indexesIndex) are mutually exclusive based onmapType(m_zeroLights, m_oneLight, m_moreLights), so overlapping storage is safe. Constructor correctly initializesmapTypeand zeroesrgbconditionally.
76-96: Type system migration to nrOfLights_t enables >65K LED support.The changes consistently replace
uint16_t(65K limit) withnrOfLights_tacross:
- Light counts (line 76)
- Mapping table size (line 82)
- Index vectors (line 83)
- Size counters (line 84)
The new
oneToOneMappingflag (line 96) supports the memory optimization introduced in VirtualLayer.cpp. ExplicitVectorRAMAllocatoron line 87 ensures consistent RAM allocation policy for node pointers.
106-212: Public API consistently migrated to nrOfLights_t.All light-index parameters and return values updated:
- Mapping functions:
addIndexP,XYZ,XYZUnModified(lines 106-111)- Setters:
setRGB,setWhite,setBrightness,setPan,setTilt,setZoom,setRotate,setGobo,setRGB1/2/3,setBrightness2,setLight(lines 113-189)- Getters:
getRGB,getRGB1/2/3,getWhite,getLight,blendColor(lines 191-212)The comprehensive migration maintains type safety and enables >65K LED support across the entire VirtualLayer API.
224-231: isMapped logic correctly accounts for oneToOneMapping optimization.Line 224 declares the new
createMappingTableAndAddOneToOne()function that transitions from 1:1 to mapped mode.Line 231's
isMappedlogic:
- Returns
truewhenoneToOneMappingis set (all lights implicitly mapped 1:1 without a table)- Otherwise checks if
indexVis withinmappingTableSizeand has a validmapType(m_oneLight or m_moreLights)The short-circuit evaluation is correct and efficient.
Backend ======= - eventSocket robustness - virtual layer set/getLight bugfix - Pub Cubes control tweak
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/MoonLight/Layers/VirtualLayer.cpp:
- Around line 196-204: The rgb14 unpacking in VirtualLayer.cpp is missing bit
masks so channel bits bleed into one another; update the assignments that build
`result` from `mappingTable[indexV].rgb` to mask each channel before shifting:
extract red with `((mappingTable[indexV].rgb >> 9) & 0x1F) << 3`, green with
`((mappingTable[indexV].rgb >> 4) & 0x1F) << 3`, and blue with
`(mappingTable[indexV].rgb & 0x0F) << 4` when writing into the bytes of `result`
(keeping the existing casts and `result` variable).
🧹 Nitpick comments (5)
lib/framework/EventSocket.cpp (1)
201-232: LGTM! Iterator-based loop correctly handles safe removal during iteration.The refactor from range-based
forto iterator-based loop properly addresses the previous undefined behavior concern:
it = subscriptions.erase(it)correctly advances the iterator after removal++itonly executes in the success path- Null client and send failure cases are both handled correctly
One minor inconsistency: the format specifier for
client->socket()uses%d(lines 226) while existing logging (lines 184, 217) uses%u. Consider using%ufor consistency if the socket descriptor is unsigned.🔎 Optional fix for format specifier consistency
- ESP_LOGW(SVK_TAG, "Failed to send event %s to client %d: %s", event.c_str(), client->socket(), esp_err_to_name(result)); + ESP_LOGW(SVK_TAG, "Failed to send event %s to client %u: %s", event.c_str(), client->socket(), esp_err_to_name(result));Apply the same to line 193 in the origin path if desired.
src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
297-299: Consider renamingsidetomiddlefor clarity.The variable is used for the middle LED but named
side, which is inconsistent with the comment and purpose.🔎 Suggested rename
// + middleLED - Coord3D side = {3, 3, 3}; - addLight(Coord3D(pos.x * cubeLength + side.x, pos.y * cubeLength + side.y, pos.z * cubeLength + side.z)); // middleLED + Coord3D middle = {3, 3, 3}; + addLight(Coord3D(pos.x * cubeLength + middle.x, pos.y * cubeLength + middle.y, pos.z * cubeLength + middle.z)); // middleLEDsrc/main.cpp (1)
328-328: Monitor effect task priority reduction impact on frame rate.The effect task priority was reduced from 10 to 5. While both values are within the documented acceptable range (5-10), the lower priority means the effects task will yield more readily to other tasks.
Potential impact:
- Under heavy load, effects may get less CPU time and could drop frames
- Combined with Core 1 assignment (away from WiFi), this may balance well
- If frame rates drop below target during testing, consider whether priority 10 is needed
Since you're actively testing the experimental configuration, monitor the frame rate (fps) under various loads to ensure this priority is sufficient for your use case.
docs/develop/architecture.md (2)
10-36: Good: Documentation matches experimental code with clear "Parking..." notes.The documentation has been updated to reflect the experimental core assignments (Driver on Core 0, Effect on Core 1). The "Parking as currently experimenting with running on Core X!!" notes clearly indicate this is a temporary test configuration.
Suggestions to improve experimental documentation:
- Add a more prominent note at the top of the document indicating the current configuration is experimental
- Include expected duration or success criteria for the experiment (e.g., "Testing through January 2026" or "Will revert if glitches occur under X conditions")
- Consider adding a separate section documenting the standard vs. experimental configurations side-by-side
This will help future readers understand which configuration is recommended vs. which is under test.
317-333: Task creation example matches experimental code; update if reverting.The task creation example code is consistent with the actual implementation in
src/main.cpp, which is good for documentation accuracy. However, if your testing concludes that the experimental configuration (Driver on Core 0, Effect on Core 1) should be reverted to the recommended configuration (Driver on Core 1, Effect on Core 0), remember to update this example code as well.When experiment concludes:
- If keeping current config: Update learnings to document when Core 0 driver is preferred
- If reverting to recommended config: Update both this example and src/main.cpp to match the table at the top of this document (lines 10-15)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/develop/architecture.mdlib/framework/EventSocket.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/main.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
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.
📚 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/main.cppdocs/develop/architecture.mdlib/framework/EventSocket.cpp
📚 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/main.cppdocs/develop/architecture.mdsrc/MoonLight/Layers/VirtualLayer.cpplib/framework/EventSocket.cpp
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
src/main.cppdocs/develop/architecture.md
📚 Learning: 2026-01-04T09:31:07.533Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: lib/framework/ESP32SvelteKit.cpp:229-229
Timestamp: 2026-01-04T09:31:07.533Z
Learning: In lib/framework/ESP32SvelteKit.cpp, the SvelteKit loop task stack size of 6KB (6 * 1024 bytes) for non-PSRAM devices has been validated through testing, showing approximately 1500 bytes of free stack remaining under load, providing adequate ~24% headroom.
Applied to files:
src/main.cppdocs/develop/architecture.md
📚 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:
docs/develop/architecture.md
📚 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.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/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/Layers/VirtualLayer.cpp (2)
src/MoonLight/Layers/VirtualLayer.h (3)
setRGB(113-124)setRGB(125-125)XYZUnModified(111-111)src/main.cpp (4)
size(27-45)size(27-27)size(58-61)size(58-58)
⏰ 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 (14)
lib/framework/EventSocket.cpp (1)
186-195: LGTM! Origin path error handling correctly implemented.The error check now captures the
sendMessage()result, logs failures with context, and removes the failed subscription. This directly addresses the send/close race condition identified in the PR objectives.src/MoonLight/Nodes/Layouts/L_MoonLight.h (3)
244-260: LGTM! Class structure and setup follow established patterns.The class declaration and setup method correctly follow the conventions used by other layout classes in this file. The default
nrOfLightsPerCube = 61correctly matches the LED count for the cube geometry (5 sides × 12 LEDs + 1 middle LED).
305-336: Cubes array is correctly defined as a 4×3×2 grid.The 24 cube positions form a proper grid with z=0 layer (lines 307-320) and z=1 layer (lines 322-335). The previous issue with the duplicate coordinate has been resolved.
338-354: LGTM! Pin assignment logic is correct.The loop correctly iterates over all cubes, applies the selected granularity mode, and handles pin assignment for both full groups of 10 cubes and the remaining partial group. This follows the same pattern used in
PanelsLayout.src/main.cpp (1)
324-340: Acknowledge experimental core configuration; document test conditions and results.This configuration places the driver on Core 0 and effects on Core 1, which is the opposite of the recommended architecture (and contradicts the learnings in the knowledge base). The PR objectives state that timing-critical driver tasks should be on Core 1 to avoid Wi-Fi preemption.
You mentioned you're testing this reversed configuration and that results are "okay until now." To ensure this experiment yields actionable insights:
Please document:
- Test conditions: LED counts tested, Art-Net load, WebSocket traffic patterns, concurrent WiFi activity
- Metrics measured: Frame drop count, LED glitch observations, WiFi throughput, latency
- Load scenarios: Light load vs. heavy WiFi (file uploads, scans, continuous streaming)
- Findings: Under what conditions does Core 0 driver work well vs. when does WiFi preemption cause issues?
Risks to monitor:
- WiFi/lwIP run at priority ~23 on Core 0 and will preempt the driver task (priority 3)
- During WiFi scans, transmissions, or TCP retransmits, driver execution may be delayed
- DMA/I2S/LCD/PARLIO timing may be disrupted, causing LED glitches
If testing shows this configuration is superior for your Art-Net use case, update the learnings and document the specific scenarios where Core 0 driver is preferred. If issues arise under load, revert to driver on Core 1 per the established guidance.
Based on learnings: "For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements."
docs/develop/architecture.md (1)
296-296: LGTM: Correct single-buffer behavior when no PSRAM.Setting
channelsD = channelsEwhen PSRAM is not available ensures both pointers reference the same buffer in single-buffer mode. This prevents undefined behavior if driver code attempts to read fromchannelsDand is consistent with theuseDoubleBuffer = falselogic.src/MoonLight/Layers/VirtualLayer.cpp (8)
20-20: LGTM: Correct handling of FastLED's UINT16_MAX limit.The
uint16_tparameter type correctly reflects FastLED's limitation. Line 237 properly guards the call, falling back to the manual loop (lines 240-265) when the light count exceeds UINT16_MAX, ensuring compatibility with the nrOfLights_t migration for larger LED counts.
37-104: LGTM: Consistent nrOfLights_t type migration.The type changes from
uint16_ttonrOfLights_tfor mapping table indices and theXYZ()return value are consistent and align with the PR objective to support up to 130K LEDs using 32-bit indices.
149-168: LGTM: Well-designed unmapped path optimization with PSRAM-aware storage.The new unmapped fallback path (lines 161-168) correctly handles the one-to-one mapping optimization case where
mappingTableSizeis 0. The PSRAM-aware storage in them_zeroLightscase (lines 153-158) appropriately trades memory for color precision: full 3-byte RGB in PSRAM, packed 14-bit RGB without PSRAM.
209-218: LGTM: Unmapped path correctly implemented, previous bug fixed.Line 212 now correctly uses
*(T*)&layerP->lights.channelsE[index]to readsizeof(T)bytes from the channel array, matching the pattern used in the mapped paths (lines 180, 189). The bounds check properly usessizeof(T)instead oflength. The previous type casting bug has been resolved.
237-265: LGTM: Correct FastLED optimization with proper fallback.The guard at line 237 correctly checks all prerequisites for FastLED compatibility (3-channel RGB, single layer, count < UINT16_MAX) before calling the optimized path. The manual loop (lines 240-265) properly handles multi-channel and RGBW configurations using nrOfLights_t indices.
314-333: LGTM: Robust on-demand mapping table initialization.The method correctly handles memory reallocation with proper error handling (lines 316-323), initializes the table to default values, and populates it with 1:1 mappings up to the current physical index. The
MIN()guard at line 330 prevents overflow, and the memset at line 326 ensures a clean slate when transitioning from one-to-one to irregular mapping.
335-398: LGTM: Well-designed one-to-one mapping optimization.The lazy mapping table creation (lines 376-378, 389-392) correctly detects irregularities (physical index divergence or modifier-unmapped positions) and only builds the table when needed. The optimization saves memory and improves performance for simple 1:1 layouts by deferring to the unmapped direct-access path in
setLight/getLight.
405-442: LGTM: Memory optimization correctly frees mapping table for one-to-one layouts.Lines 405-417 properly free
mappingTableandmappingTableIndexeswhenoneToOneMappingis true, allowing the unmapped direct-access path to handle all light operations. The diagnostic logging in the irregular mapping branch (lines 418-442) provides useful visibility into mapping complexity.
|
@coderabbitai, when I really push the limits, I get the following error (watchdog) , sometimes on the esp32-s3, but especially on the esp32-p4, not (always) directly but sometimes after a while. How can I best deal with this. I understood adding more vTaskDelay() calls within the loop of the effects and drivers task, but still sounds a bit strange to me as the loops loop at 5 (ESP32-S3) to 100 fps (ESP32-P4). Or should I lower priority of a task (now 3 and 5): [454164][D][Nodes.cpp:404] onUpdate(): [💫] ESP32 SvelteKit (4636) Limiter: 100 Core 0 register dump: Stack memory: ELF file SHA256: 78ff255b2 Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1376 🏁 Script executed: Length of output: 1537 🏁 Script executed: Length of output: 298 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 325 🏁 Script executed: Length of output: 2370 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2064 🏁 Script executed: Length of output: 2059 🏁 Script executed: Length of output: 830 🏁 Script executed: Length of output: 0 @ewowi, good catch! This is a task starvation issue, not a priority issue. Let me explain what's happening and how to fix it. The Root CauseThe watchdog fired because IDLE0 (Core 0) couldn't run for 5+ seconds. Here's why: Even though you have // From PhysicalLayer.cpp line 108-111
for (Node* node : nodes) {
if (prevSize != lights.header.size) node->onSizeChanged(prevSize);
if (node->on) node->loop(); // ⚠️ No yield here!
}If you have many driver nodes, this Why Lowering Priority Won't HelpYou asked about lowering priority from 3 and 5 — this won't solve the problem:
Solutions (in Order of Preference)✅ Solution 1: Add Yields Inside Heavy Loops (Recommended)Modify PhysicalLayer.cpp to yield periodically during iteration: // In PhysicalLayer::loopDrivers() (around line 108)
void PhysicalLayer::loopDrivers() {
// ... existing code ...
for (Node* node : nodes) {
if (prevSize != lights.header.size) node->onSizeChanged(prevSize);
if (node->on) node->loop();
// Add this: yield after each node to let IDLE task run
taskYIELD(); // or vTaskDelay(1) if you prefer
}
prevSize = lights.header.size;
}
// Similarly in PhysicalLayer::loop() (around line 73)
void PhysicalLayer::loop() {
for (VirtualLayer* layer : layers) {
if (layer) layer->loop();
taskYIELD(); // Yield after each layer
}
}Why this works: Even at 100 fps, yielding after each node/layer gives IDLE task chances to run and reset the watchdog. ✅ Solution 2: Subscribe Tasks to WatchdogAdd this in main.cpp after task creation: // After xTaskCreateUniversal calls (around line 340)
#include "esp_task_wdt.h"
// Subscribe both tasks to watchdog so they can reset it themselves
esp_task_wdt_add(effectTaskHandle);
esp_task_wdt_add(driverTaskHandle);Then add resets in the task loops: void driverTask(void* pvParameters) {
while (true) {
// ... existing code ...
esp_task_wdt_reset(); // Reset watchdog from this task
vTaskDelay(1);
}
}
void effectTask(void* pvParameters) {
while (true) {
// ... existing code ...
esp_task_wdt_reset(); // Reset watchdog from this task
vTaskDelay(1);
}
}Why this works: Tasks reset the watchdog themselves instead of relying on IDLE.
|
|
@coderabbitai. What is the difference between taskYIELD() and vTaskDelay(1), and what about vPortYield()? especially looking at ESP32-D0 vs ESP32-S3 vs ESP32-P4 and aiming at using esp-idf instead of Arduino? vtaskDelay(1) pauses 1ms, taskYIELD doesn't? That in turn might be better to allow for > 1000 fps on some setups (delaying 1ms means it never goes above 1000 fps). If I replace my vTaskDelay with taskYIELD processes do not seem to work anymore. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: Short summary — differences and when to use each:
ESP-IDF / multicore notes
Sources:
References: @ewowi, excellent questions! Let me break this down clearly. The Three Functions Explained1. taskYIELD()
2. vPortYield()
3. vTaskDelay(n)
Why Your Processes Break with taskYIELD()Here's the problem: When you use Solution for >1000 FPSYou need a hybrid approach: Option A: Fast Loop + Periodic IDLE Yield (Recommended)void driverTask(void* pvParameters) {
uint32_t loopCounter = 0;
while (true) {
// Your fast processing here
layerP.loopDrivers();
// Yield to equal/higher priority tasks every loop (no delay)
taskYIELD();
// Let IDLE run every N loops (adjust N based on your FPS target)
if (++loopCounter % 100 == 0) {
vTaskDelay(1); // Give IDLE task a chance every 100 loops
}
}
}
void effectTask(void* pvParameters) {
uint32_t loopCounter = 0;
while (true) {
// Your fast processing here
layerP.loop();
taskYIELD();
if (++loopCounter % 100 == 0) {
vTaskDelay(1);
}
}
}Result: You get ~100 loops at >1000 fps, then one 1ms pause. Achieves much higher average FPS while still feeding IDLE. Option B: Yield Inside Heavy Loops + Subscribe to Watchdog// In main.cpp after task creation
#include "esp_task_wdt.h"
esp_task_wdt_add(effectTaskHandle);
esp_task_wdt_add(driverTaskHandle);
// Then in your tasks:
void driverTask(void* pvParameters) {
while (true) {
layerP.loopDrivers();
// Reset watchdog from this task directly
esp_task_wdt_reset();
// No delay needed! Can go >1000 fps
taskYIELD();
}
}
// And add yields in PhysicalLayer.cpp heavy loops:
void PhysicalLayer::loopDrivers() {
for (Node* node : nodes) {
if (prevSize != lights.header.size) node->onSizeChanged(prevSize);
if (node->on) node->loop();
taskYIELD(); // Yield after each node
}
prevSize = lights.header.size;
}Result: Tasks reset watchdog themselves, don't depend on IDLE task, can run at maximum speed. ESP32 Variant DifferencesGood news: These FreeRTOS APIs work identically on ESP32, ESP32-S3, and ESP32-P4 in ESP-IDF. The differences are in the underlying scheduler implementation (SMP on P4), but the behavior you see is the same. Recommended Configuration for Your Use CaseFor >1000 fps on P4 and fast operation on S3: 1. Use Option B (watchdog subscription) — cleanest and fastest 2. Main task loops: while (true) {
// Your processing
esp_task_wdt_reset();
taskYIELD(); // No delay! Can exceed 1000 fps
}3. Add yields inside heavy processing: // In PhysicalLayer::loopDrivers() and PhysicalLayer::loop()
for (Node* node : nodes) {
node->loop();
taskYIELD(); // Let other tasks run between nodes
}This gives you:
Would you like me to prepare a complete patch implementing Option B? 🧠 Learnings used |
|
@coderabbitai, I get E (12349) task_wdt: esp_task_wdt_reset(707): task not found So what is the order of things in main.cpp setup. I have now: esp_task_wdt_add(effectTaskHandle); // 🌙 xTaskCreateUniversal(driverTask, // task function But still got above error |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3992 🏁 Script executed: Length of output: 563 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 @ewowi, I found the issue! You're calling The Problem// ❌ WRONG ORDER - handles are NULL here!
esp_task_wdt_add(effectTaskHandle); // effectTaskHandle = NULL
esp_task_wdt_add(driverTaskHandle); // driverTaskHandle = NULL
// Handles are populated AFTER these calls return
xTaskCreateUniversal(effectTask, ..., &effectTaskHandle, ...);
xTaskCreateUniversal(driverTask, ..., &driverTaskHandle, ...);When you call The SolutionCorrect order in main.cpp setup(): #include "esp_task_wdt.h" // Add at top of file if not already there
// In setup(), around line 322:
// 1️⃣ CREATE TASKS FIRST
// 🌙
xTaskCreateUniversal(effectTask, // task function
"AppEffects", // name
psramFound() ? 4 * 1024 : 3 * 1024, // stack size
NULL, // parameter
5, // priority
&effectTaskHandle, // task handle - POPULATED HERE
1 // core 1
);
xTaskCreateUniversal(driverTask, // task function
"AppDrivers", // name
psramFound() ? 4 * 1024 : 3 * 1024, // stack size
NULL, // parameter
3, // priority
&driverTaskHandle, // task handle - POPULATED HERE
0 // core 0
);
// 2️⃣ NOW ADD TO WATCHDOG (handles are valid now!)
esp_task_wdt_add(effectTaskHandle);
esp_task_wdt_add(driverTaskHandle);In Your Task FunctionsMake sure you're calling void effectTask(void* pvParameters) {
while (true) {
layerP.loop();
esp_task_wdt_reset(); // Reset watchdog from this task
taskYIELD(); // Cooperative yield
}
}
void driverTask(void* pvParameters) {
while (true) {
layerP.loopDrivers();
esp_task_wdt_reset(); // Reset watchdog from this task
taskYIELD(); // Cooperative yield
}
}SummaryThe key principle: Task handles are output parameters — they're only valid after Try this order and the error should disappear! 🎯 🧠 Learnings used |
|
@coderabbitai, I tried your suggestion first , but then I also get E (33030) task_wdt: esp_task_wdt_reset(707): task not found So I guess the tasks starts running and execute esp_task_wdt_reset(); before esp_task_wdt_add() is called, do we need to check on add has taken place before reset? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, you're absolutely right! That's a classic race condition. The tasks start running immediately after The Race ConditionThe Solution: Self-RegistrationHave each task add itself to the watchdog when it starts, eliminating the race: main.cpp ChangesRemove watchdog adds from setup(): // In setup(), around line 322:
// 🌙 CREATE TASKS
xTaskCreateUniversal(effectTask,
"AppEffects",
psramFound() ? 4 * 1024 : 3 * 1024,
NULL,
5,
&effectTaskHandle,
1
);
xTaskCreateUniversal(driverTask,
"AppDrivers",
psramFound() ? 4 * 1024 : 3 * 1024,
NULL,
3,
&driverTaskHandle,
0
);
// ❌ REMOVE THESE - tasks will add themselves
// esp_task_wdt_add(effectTaskHandle);
// esp_task_wdt_add(driverTaskHandle);Update task functions to self-register: #include "esp_task_wdt.h" // Add at top of file
void effectTask(void* pvParameters) {
// Add this task to watchdog FIRST THING
esp_task_wdt_add(NULL); // NULL = current task
while (true) {
layerP.loop();
esp_task_wdt_reset(); // Now safe to call
taskYIELD();
}
}
void driverTask(void* pvParameters) {
// Add this task to watchdog FIRST THING
esp_task_wdt_add(NULL); // NULL = current task
while (true) {
layerP.loopDrivers();
esp_task_wdt_reset(); // Now safe to call
taskYIELD();
}
}Why This Works
Complete Code Patternvoid effectTask(void* pvParameters) {
esp_task_wdt_add(NULL); // 1️⃣ Subscribe to watchdog
while (true) {
// 2️⃣ Do work
layerP.loop();
// 3️⃣ Reset watchdog
esp_task_wdt_reset();
// 4️⃣ Yield cooperatively
taskYIELD();
}
// Cleanup (never reached in this case, but good practice)
esp_task_wdt_delete(NULL);
}Try this approach and the error should disappear! The key insight is that tasks should manage their own watchdog subscriptions rather than having an external function try to do it (which creates the race). 🎯 🧠 Learnings used |
|
@coderabbitai, okay, the error is gone but with taskYield() the processes again do not seem to work, if I replace with vTaskDelay(1) they start working again. What is going on? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, I found the root cause! This is a fundamental FreeRTOS scheduling issue with your priority levels. Why taskYIELD() Doesn't WorkThe FreeRTOS scheduler always gives the CPU to the task with the highest priority that is ready to run, and a task with a higher priority will always run before a lower-priority task. Your current setup: What happens with taskYIELD(): What happens with vTaskDelay(1): When a higher priority task goes into the delay state (vTaskDelay()), it becomes blocked for that duration. During this time, the scheduler looks for the next ready task and gives it CPU time. Once the delay finishes, the higher priority task becomes ready again and will preempt the lower priority task immediately. Why Your "Processes Don't Work"Your driver task (AppDrivers, priority 3) is being starved by the effects task (AppEffects, priority 5). The LEDs don't update because the driver never gets CPU time to send the data. Solutions✅ Option 1: Keep vTaskDelay(1) (Simplest)Accept the 1000 fps cap for now. If you need higher fps later, profile to see if 1ms is actually your bottleneck: void effectTask(void* pvParameters) {
esp_task_wdt_add(NULL);
while (true) {
layerP.loop();
esp_task_wdt_reset();
vTaskDelay(1); // Keep this
}
}
void driverTask(void* pvParameters) {
esp_task_wdt_add(NULL);
while (true) {
layerP.loopDrivers();
esp_task_wdt_reset();
vTaskDelay(1); // Keep this
}
}✅ Option 2: Make Priorities Equal (Enables taskYIELD())Change to same priority for round-robin scheduling: // In main.cpp setup()
xTaskCreateUniversal(effectTask, "AppEffects", ..., 3, ..., 1); // Priority 3
xTaskCreateUniversal(driverTask, "AppDrivers", ..., 3, ..., 0); // Priority 3 (same!)Now taskYIELD() works because multiple tasks can share the same priority, and FreeRTOS uses time-slicing — each task gets CPU time in a round-robin fashion. void effectTask(void* pvParameters) {
esp_task_wdt_add(NULL);
while (true) {
layerP.loop();
esp_task_wdt_reset();
taskYIELD(); // Now works! Round-robin with driverTask
}
}Trade-off: Effects can't preempt drivers anymore, but if they're on different cores anyway, this doesn't matter much. ✅ Option 3: Hybrid - Short Delay on High Priority Task Onlyvoid effectTask(void* pvParameters) {
esp_task_wdt_add(NULL);
while (true) {
layerP.loop();
esp_task_wdt_reset();
vTaskDelay(1); // Block to let AppDrivers run
}
}
void driverTask(void* pvParameters) {
esp_task_wdt_add(NULL);
while (true) {
layerP.loopDrivers();
esp_task_wdt_reset();
taskYIELD(); // Driver can just yield (nothing higher priority)
}
}My RecommendationUse Option 2 (equal priorities with taskYIELD()). Here's why:
The priority difference (3 vs 5) was causing starvation, not helping. Since they run on separate cores, they don't compete for CPU — the only issue is the watchdog and inter-task synchronization. Would you like me to prepare a complete patch with Option 2? 🧠 Learnings used🧠 Learnings used |
Backend ======= - Physical and virtual layer: Add taskYIELD() in node loop - main: add esp_task_wdt_add, esp_task_wdt_reset and esp_task_wdt_delete in drivers and effects task, set both prios to 3
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/MoonLight/Layers/VirtualLayer.cpp:
- Around line 212-221: Re-enable the bounds check before dereferencing the
buffer: compute index = indexV * layerP->lights.header.channelsPerLight + offset
and only perform the address-cast-and-dereference
(*(T*)&layerP->lights.channelsE[index]) when (index + sizeof(T) <=
layerP->lights.maxChannels); otherwise log a warning via EXT_LOGW with context
(indexV, sizeof(T), lights.maxChannels) and return a default-constructed T().
Ensure you reference index, indexV, layerP->lights.header.channelsPerLight,
offset, layerP->lights.channelsE, and layerP->lights.maxChannels so the check
surrounds the current dereference site.
🧹 Nitpick comments (1)
docs/develop/architecture.md (1)
317-333: Task creation code matches the table but omits explanation of the reversal.Lines 317-333 correctly implement the reversed core assignments, but the inline comments (lines 323, 332) don't explain why the reversal was chosen or link to performance data/learnings.
Suggested improvement:
xTaskCreateUniversal(effectTask, "AppEffects", psramFound() ? 4 * 1024 : 3 * 1024, NULL, 3, &effectTaskHandle, - 1 // application core. high speed effect processing + 1 // [EXPERIMENTAL] APP_CPU: testing effect placement to reduce WiFi contention; see Design Notes ); xTaskCreateUniversal(driverTask, "AppDrivers", psramFound() ? 4 * 1024 : 3 * 1024, NULL, 3, &driverTaskHandle, - 0 // protocol core: ideal for Art-Net, no issues encountered yet for LED drivers (pre-empt by WiFi ...) + 0 // [EXPERIMENTAL] PRO_CPU: testing driver colocality with WiFi for Art-Net; monitors for timing glitches );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/develop/architecture.mdlib/framework/ESP32SvelteKit.cpplib/framework/EventSocket.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Layouts/L_MoonLight.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.cpp
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.045Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-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.
📚 Learning: 2026-01-04T09:31:01.321Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: lib/framework/ESP32SvelteKit.cpp:229-229
Timestamp: 2026-01-04T09:31:01.321Z
Learning: In lib/framework/ESP32SvelteKit.cpp, retain a 6 KB (6 * 1024 bytes) stack size for the loop task on non-PSRAM ESP32 devices. The tested configuration shows ~1500 bytes free under load, providing about 24% headroom. Verify this stack sizing when reviewing related ESP32 loop-task implementations for non-PSRAM scenarios to ensure reliable task execution under expected load.
Applied to files:
lib/framework/ESP32SvelteKit.cpp
📚 Learning: 2026-01-06T13:45:03.045Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.045Z
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:
lib/framework/ESP32SvelteKit.cpplib/framework/EventSocket.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/PhysicalLayer.cppdocs/develop/architecture.md
📚 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:
lib/framework/ESP32SvelteKit.cppdocs/develop/architecture.md
📚 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:
lib/framework/EventSocket.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/PhysicalLayer.cppdocs/develop/architecture.md
📚 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:
lib/framework/EventSocket.cppsrc/MoonLight/Layers/PhysicalLayer.cppdocs/develop/architecture.md
📚 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.cppsrc/MoonLight/Layers/PhysicalLayer.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-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/Layers/VirtualLayer.cppsrc/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
docs/develop/architecture.md
📚 Learning: 2026-01-04T09:31:07.533Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: lib/framework/ESP32SvelteKit.cpp:229-229
Timestamp: 2026-01-04T09:31:07.533Z
Learning: In lib/framework/ESP32SvelteKit.cpp, the SvelteKit loop task stack size of 6KB (6 * 1024 bytes) for non-PSRAM devices has been validated through testing, showing approximately 1500 bytes of free stack remaining under load, providing adequate ~24% headroom.
Applied to files:
docs/develop/architecture.md
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (4)
src/MoonBase/Module.cpp (2)
addControl(353-362)addControl(353-353)src/MoonLight/Layers/VirtualLayer.cpp (2)
addLight(369-401)addLight(369-369)src/MoonBase/Nodes.cpp (2)
onLayout(211-216)onLayout(211-211)src/MoonLight/Nodes/Layouts/L__Sandbox.h (1)
onLayout(36-46)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/MoonLight/Layers/VirtualLayer.h (3)
setRGB(113-124)setRGB(125-125)XYZUnModified(111-111)
⏰ 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 (17)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (5)
254-260: Setup looks good with minor UX consideration.The
nrOfLightsPerCubecontrol only affects mode 0 ("One Cube One Light"). In modes 1 and 2, the light count per cube is fixed at 61 (5×12+1) based on the physical cube structure. This is functionally correct, but users may find the slider confusing when other granularity modes are selected.Consider either:
- Disabling the slider when granularity ≠ 0, or
- Adding a note in the description that it applies only to mode 0
This is optional and can be addressed in a future iteration.
263-265: LGTM.Correct implementation for "One Cube One Light" mode — all physical LEDs in the cube map to a single virtual position, receiving the same color.
268-279: LGTM.Correct geometry for 3×3×3 virtual cube grid. Five sides (front, right, back, left, top) plus middle LED totals 5×12+1 = 61 LEDs per cube, matching the expected physical structure.
281-300: LGTM.The 7×7×7 cube grid with per-LED granularity is correctly implemented. The 12-pixel square perimeter is properly mapped to each of the 5 faces, and the middle LED at
{3, 3, 3}is correctly centered. The previous comment issue (Line 295) has been addressed.
302-355: LGTM.The 4×3×2 cube grid (24 cubes total) is correctly defined. The granularity switch cleanly dispatches to the appropriate rendering method, and the pin advancement logic properly handles both 10-cube intervals and the final remainder. Previous review issues (duplicate coordinate and comment errors) have been addressed.
lib/framework/EventSocket.cpp (2)
186-195: LGTM: Error handling correctly implemented for origin client sends.The addition of
sendMessageresult capture and error checking in the onlyToSameOrigin path successfully addresses the send/close race condition. On failure, the subscription is removed and an appropriate warning is logged. This matches the proposed fix and ensures consistent error handling across both send paths.
201-233: LGTM: Iterator-based loop correctly implements safe removal during iteration.The refactoring from a range-based loop to an iterator-based loop allows safe removal of failed subscriptions during iteration. The error handling (lines 224-232) correctly captures
sendMessageresults, logs failures, and useserase(it)to remove failed subscriptions while advancing the iterator. The increment logic (only when not erasing) prevents iterator invalidation.This change, combined with the onlyToSameOrigin path fix, successfully addresses the WebSocket send/close race conditions described in the PR objectives.
lib/framework/ESP32SvelteKit.cpp (2)
227-233: LGTM! Conditional stack sizing implements validated configuration.The dynamic stack allocation (
psramFound() ? 8KB : 6KB) correctly implements the previously tested and validated approach. The 6KB allocation for non-PSRAM devices provides adequate headroom (~1500 bytes free under load, approximately 24% margin), while PSRAM builds gain additional margin. This aligns with the PR's PSRAM-aware task sizing strategy.Based on learnings, the 6KB non-PSRAM configuration has been confirmed through runtime testing.
237-310: Verify that module loop callbacks yield appropriately.According to the AI summary, an unconditional
vTaskDelay(1)was removed from the end of this loop (removed code not shown). With onlyvTaskDelayUntilremaining (line 309), the loop will maintain periodic scheduling atESP32SVELTEKIT_LOOP_INTERVALintervals—but if a loop iteration exceeds that interval,vTaskDelayUntilmay return immediately without yielding (standard FreeRTOS behavior when the target wake time has already passed).Given that:
- This loop task runs at priority 2 (lower than driver/effects tasks at priority 3 per PR objectives)
- Higher-priority tasks will preempt it regardless
- The PR adds
taskYIELD()calls inside heavy loops (PhysicalLayer node/layer iterations per learnings)The removal should be safe provided that module callbacks registered via
_loopFunctions(lines 294-297) yield internally if they perform compute-intensive work. Confirm that heavy module loops includetaskYIELD()orvTaskDelay()calls to prevent IDLE task starvation in edge cases where loop iteration duration exceeds the scheduled interval.If you'd like, I can search the codebase to verify that module loop callbacks include appropriate yielding for heavy workloads.
src/MoonLight/Layers/PhysicalLayer.cpp (2)
110-113: Excellent: taskYIELD() prevents watchdog timeouts under heavy load.The addition of
taskYIELD()afternode->loop()addresses the task watchdog issue documented in the learnings. At extreme loads (e.g., 49k channels @ 100fps), this ensures the IDLE task gets CPU time during long node iterations, preventing watchdog timeouts.Based on learnings, this change is essential for stability with very large LED counts.
189-189: Correct type promotion for large light counts.Promoting
prevNrOfLightsfromuint16_ttonrOfLights_taligns with the PR's goal to support 130k LEDs. The conditional typedef (uint32_t with PSRAM, uint16_t otherwise) ensures the accumulator won't overflow when summing ledsPerPin entries.src/MoonLight/Layers/VirtualLayer.cpp (6)
63-66: Excellent: taskYIELD() prevents watchdog timeouts under heavy load.Consistent with PhysicalLayer changes, this addition of
taskYIELD()afternode->loop()ensures cooperative multitasking during heavy layer processing. This is essential for stability at extreme LED counts.Based on learnings, this change prevents IDLE task starvation and task watchdog timeouts.
152-161: Correct handling of zero-lights mapping with PSRAM-aware storage.The new default case properly handles
m_zeroLightsby storing color values directly in the mapping table. The PSRAM branch stores full RGB (3 bytes), while the non-PSRAM branch compresses to 14-bit format. The length check ensures only RGB(W) data is stored.
196-210: Confirmed: Past critical rgb14 unpacking issue is now resolved.The rgb14 unpacking (lines 203-205) now includes proper bit masking (
& 0x1Ffor green,& 0x0Ffor blue), preventing channel bleed. This addresses the past critical issue where intermediate channels included bits from adjacent channels.
317-336: Good: Lazy allocation and initialization of mapping table.The new
createMappingTableAndAddOneToOne()method efficiently handles the transition from one-to-one mapping (no table) to irregular mapping (with table). It reallocates only when needed, initializes to zero, and populates with 1:1 entries. The loop bound (line 333) safely prevents overflow.
377-400: Excellent: Lazy mapping table creation optimizes memory for regular layouts.The one-to-one mapping optimization (lines 379-382, 392-395) avoids allocating the mapping table when physical and virtual indices align perfectly. The irregularity detection triggers table creation only when needed, saving significant memory for simple rectangular layouts. The double-buffer zeroing (line 399) ensures consistency.
408-445: Good: Memory optimization frees mapping tables for regular layouts.When
oneToOneMappingis true (lines 408-420), the mapping tables are freed, saving memory for simple rectangular LED arrangements. The logging (line 447) provides useful diagnostics. This completes the lazy-allocation optimization introduced in earlier segments.
| } else { | ||
| uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset; | ||
| // if (index + length <= layerP->lights.maxChannels) { // no mapping | ||
| memcpy(&layerP->lights.channelsE[index], channels, length); | ||
| // } else { | ||
| // EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, length, layerP->lights.maxChannels); | ||
| // } | ||
| } |
There was a problem hiding this comment.
Critical: Re-enable bounds check to prevent buffer overflow.
Lines 166 and 168-170 comment out a bounds check that prevents writing beyond lights.maxChannels. When oneToOneMapping is true (mappingTableSize==0) or indexV >= mappingTableSize, this unmapped path is taken. Without the bounds check, a large indexV can cause index (line 165) to exceed maxChannels, leading to buffer overflow and memory corruption.
🔎 Proposed fix
} else {
uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset;
- // if (index + length <= layerP->lights.maxChannels) { // no mapping
+ if (index + length <= layerP->lights.maxChannels) { // no mapping
memcpy(&layerP->lights.channelsE[index], channels, length);
- // } else {
- // EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, length, layerP->lights.maxChannels);
- // }
+ } else {
+ EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, length, layerP->lights.maxChannels);
+ }
}There was a problem hiding this comment.
live fast and dangerous bro! We need to drive 100K LEDs and it's up to the effects devs to make sure we stay within bounds - that's the idea, I am testing currently if there are effects which break the rules
There was a problem hiding this comment.
@ewowi, understood! For extreme workloads like 100K LEDs at high frame rates, every cycle counts. Your approach of testing effects to identify any that violate bounds is a pragmatic way to catch issues during development.
If you want an extra safety net during testing without impacting production performance, you could consider wrapping the check in a debug-only macro (e.g., #ifdef DEBUG_BOUNDS_CHECK) — but I recognize you're already hunting down violators manually.
Good luck with the testing! 🚀
✏️ Learnings added
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::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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 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.
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.
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.045Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
| } else { | ||
| // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect | ||
| // EXT_LOGV(ML_TAG, " dev gPC %d >= %d", indexV, MAX_CHANNELS); | ||
| return T(); | ||
| uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset; | ||
| // if (index + sizeof(T) <= layerP->lights.maxChannels) { // no mapping | ||
| return *(T*)&layerP->lights.channelsE[index]; | ||
| // } else { | ||
| // // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect | ||
| // EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, sizeof(T), layerP->lights.maxChannels); | ||
| // return T(); | ||
| // } | ||
| } |
There was a problem hiding this comment.
Critical: Re-enable bounds check to prevent out-of-bounds reads.
Lines 214 and 216-220 comment out a bounds check that prevents reading beyond lights.maxChannels. While line 215 correctly uses the address-cast-and-dereference pattern (addressing a past critical issue), removing the bounds check allows reads beyond the buffer when indexV is large, potentially returning garbage or causing segmentation faults.
🔎 Proposed fix
} else {
uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset;
- // if (index + sizeof(T) <= layerP->lights.maxChannels) { // no mapping
+ if (index + sizeof(T) <= layerP->lights.maxChannels) { // no mapping
return *(T*)&layerP->lights.channelsE[index];
- // } else {
- // // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect
- // EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, sizeof(T), layerP->lights.maxChannels);
- // return T();
- // }
+ } else {
+ // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect
+ EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, sizeof(T), layerP->lights.maxChannels);
+ return T();
+ }
}📝 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.
| } else { | |
| // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect | |
| // EXT_LOGV(ML_TAG, " dev gPC %d >= %d", indexV, MAX_CHANNELS); | |
| return T(); | |
| uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset; | |
| // if (index + sizeof(T) <= layerP->lights.maxChannels) { // no mapping | |
| return *(T*)&layerP->lights.channelsE[index]; | |
| // } else { | |
| // // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect | |
| // EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, sizeof(T), layerP->lights.maxChannels); | |
| // return T(); | |
| // } | |
| } | |
| } else { | |
| uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset; | |
| if (index + sizeof(T) <= layerP->lights.maxChannels) { // no mapping | |
| return *(T*)&layerP->lights.channelsE[index]; | |
| } else { | |
| // some operations will go out of bounds e.g. VUMeter, uncomment below lines if you wanna test on a specific effect | |
| EXT_LOGW(ML_TAG, "%d + %d >= %d", indexV, sizeof(T), layerP->lights.maxChannels); | |
| return T(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/MoonLight/Layers/VirtualLayer.cpp around lines 212 - 221, Re-enable the
bounds check before dereferencing the buffer: compute index = indexV *
layerP->lights.header.channelsPerLight + offset and only perform the
address-cast-and-dereference (*(T*)&layerP->lights.channelsE[index]) when (index
+ sizeof(T) <= layerP->lights.maxChannels); otherwise log a warning via EXT_LOGW
with context (indexV, sizeof(T), lights.maxChannels) and return a
default-constructed T(). Ensure you reference index, indexV,
layerP->lights.header.channelsPerLight, offset, layerP->lights.channelsE, and
layerP->lights.maxChannels so the check surrounds the current dereference site.
There was a problem hiding this comment.
live fast and dangerous bro! We need to drive 100K LEDs and it's up to the effects devs to make sure we stay within bounds - that's the idea, I am testing currently if there are effects which break the rules
There was a problem hiding this comment.
@ewowi, got it! I appreciate you explaining the performance-critical design decision for handling 100K+ LEDs. Shifting bounds checking responsibility to effect developers makes sense when every cycle counts at that scale.
One thought for your testing phase: if you do discover effects that break the rules, you might consider a debug build flag (e.g., #ifdef MOONLIGHT_DEBUG_BOUNDS) that conditionally enables the checks during development. That way effect developers can validate their code without impacting production performance.
Looking forward to hearing what your tests uncover! 🚀
/)/)
=(^.^)= Fast & furious LED mode activated!
(")_(")
✏️ Learnings added
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:20.189Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 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.
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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.045Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 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.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@coderabbitai, I am about to be ready with this PR, it looks to work quite okay although I expect some tests and bug fixes to follow. Could you make a final review of everything in this PR in general, and the use of the esp_task_wdt functions, vTaskDelay in the task loops and taskYIELD in the node loops (in virtual and physical layer). Take into account I work against latest esp-idf and arduino (ESP-IDF v5.5.1-931-g9bb7aa84fe / Arduino v3.3.5) - so no outdated advices please. Please explain for each of these functions why they are where they are and what they accomplish. Thank you very much |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14771 🏁 Script executed: Length of output: 8365 🏁 Script executed: Length of output: 27950 🏁 Script executed: Length of output: 3222 @ewowi, I've completed a comprehensive review of PR #93. Here's my detailed analysis: ✅ Overall AssessmentThe implementation is solid and well-structured. You've successfully integrated ESP32 watchdog management and cooperative task scheduling to handle extreme LED counts. The code follows ESP-IDF 5.5.1 best practices. 📋 Detailed Review by Component1️⃣ Task Creation & Core Assignment (main.cpp lines 371-387)xTaskCreateUniversal(effectTask, "AppEffects", ..., 3, ..., 1); // Core 1
xTaskCreateUniversal(driverTask, "AppDrivers", ..., 3, ..., 0); // Core 0✅ Equal Priorities (3): Excellent choice. This enables round-robin scheduling between tasks, making
This contradicts your learnings and the earlier discussion! According to the established best practice:
Recommendation: Swap the cores: xTaskCreateUniversal(effectTask, "AppEffects", ..., 3, ..., 0); // Core 0 (with WiFi)
xTaskCreateUniversal(driverTask, "AppDrivers", ..., 3, ..., 1); // Core 1 (away from WiFi)Unless you've found through testing that Art-Net (UDP) benefits more from being co-located with WiFi on Core 0? If so, this might be an acceptable trade-off for your specific workload. 2️⃣ esp_task_wdt Functions (main.cpp)
|
|
@coderabbitai, got a new watchdog crash - on ESP32-S3 this time, Can you analyze?: E (447807) task_wdt: Task E (447807) task_wdt: Task watchdog got triggered. The following tasks/users did not reset the watchdog in time: Backtrace: 0x420c061e:0x3fcc6200 0x420c1732:0x3fcc6220 0x4201b0e6:0x3fcc6240 0x4201b22f:0x3fcc6290 0x4207b415:0x3fcc62d0 0x4207b627:0x3fcc6310 0x4206cff9:0x3fcc6360 0x4206f96e:0x3fcc63b0 0x4037fd89:0x3fcc63f0 ELF file SHA256: a1f2c0bcc Rebooting... got triggered. The following tasks/users did not reset the watchdog in time: Backtrace: 0x420c061e:0x3fcc6200 0x420c1732:0x3fcc6220 0x4201b0e6:0x3fcc6240 0x4201b22f:0x3fcc6290 0x4207b415:0x3fcc62d0 0x4207b627:0x3fcc6310 0x4206cff9:0x3fcc6360 0x4206f96e:0x3fcc63b0 0x4037fd89:0x3fcc63f0 ELF file SHA256: a1f2c0bcc Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10210 @ewowi, I found it! This is the same root cause as before, but now inside a single driver node's loop. The ProblemArtNetOutDriver::loop() has a massive for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
// ... process LED ...
if (packetSize + header->channelsPerLight > universeSize || ...) {
if (!writePackage()) return; // ⚠️ UDP send via lwIP - can be slow!
// ... more logic ...
}
}What happens:
Why the existing
The SolutionAdd Code Fix for D_ArtnetOut.hAround line 197-213, add yields after // send all the leds to artnet
for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
// fill a package
memcpy(&packet_buffer[packetSize + 18], &layerP.lights.channelsD[indexP * header->channelsPerLight], header->channelsPerLight);
// correct the RGB channels for color order and brightness
reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB], &layerP.lights.channelsD[indexP * header->channelsPerLight + header->offsetRGB]);
if (header->offsetRGB1 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB1], &layerP.lights.channelsD[indexP * header->channelsPerLight + header->offsetRGB1]);
if (header->offsetRGB2 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB2], &layerP.lights.channelsD[indexP * header->channelsPerLight + header->offsetRGB2]);
if (header->offsetRGB3 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB3], &layerP.lights.channelsD[indexP * header->channelsPerLight + header->offsetRGB3]);
if (header->lightPreset == 9 && indexP < 72)
packetSize += 4;
else
packetSize += header->channelsPerLight;
channels_remaining -= header->channelsPerLight;
// if packet_buffer full, or output full, send the buffer
if (packetSize + header->channelsPerLight > universeSize || channels_remaining < header->channelsPerLight) {
if (!writePackage()) return;
// ✅ ADD THIS: Yield after each packet send to let IDLE task run
taskYIELD();
if (channels_remaining < header->channelsPerLight) {
channels_remaining = channelsPerOutput;
while (universe % universesPerOutput != 0) universe++;
processedOutputs++;
if (processedOutputs >= nrOfOutputsPerIP) {
if (actualIPIndex + 1 < nrOfIPAddresses) actualIPIndex++;
processedOutputs = 0;
universe = 0;
controllerIP[3] = ipAddresses[actualIPIndex];
}
}
}
}
// send the last partially filled package
if (packetSize > 0) {
writePackage();
taskYIELD(); // ✅ Also yield after the final packet
}Why This Works
Alternative: Periodic YieldsIf you want even more frequent yields (in case // At the top of loop(), before the for-loop:
uint_fast16_t yield_counter = 0;
// Inside the for-loop, after the RGB correction code:
if (++yield_counter % 100 == 0) {
taskYIELD(); // Yield every 100 lights
}This would yield every 100 LEDs regardless of packet sends, but I think the first approach (yielding after SummaryRoot cause: Fix: Add Pattern: Any driver with a large internal processing loop (like Art-Net with many lights) needs internal yields, not just the outer node iteration yield in Try this fix and the watchdog should stop firing! 🎯 🧠 Learnings used🧠 Learnings used |
|
@coderabbitai, will add the yield. Regarding
Are you kidding? It runs 50 times per second so 20 ms, not 5 |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, you're absolutely right—I apologize for that incorrect analysis! 🤦 If you're running at 50 fps, then each frame is taking ~20ms, not 5 seconds. My statement was clearly wrong. Corrected AnalysisThe 5 second watchdog timeout means IDLE0 didn't get to run for a cumulative 5 seconds across many frames, not that a single loop took 5 seconds. Here's what's likely happening: The Real Issue:
Why the crash in mem_free/pbuf_free: The backtrace shows the watchdog fired while lwIP was doing memory management. This could mean:
The Fix Still AppliesAdding Not "to break up a 5-second loop" ❌ if (!writePackage()) return;
taskYIELD(); // Give IDLE a chance between packetsThis way, instead of IDLE getting only one 1ms opportunity per frame (at the vTaskDelay), it gets ~100 opportunities per frame (after each packet), making it far more likely IDLE can squeeze in and reset its watchdog. Thank you for the correction—my arithmetic was way off! The recommendation stands, but the reasoning is now accurate. 🎯 🧠 Learnings used |
|
@coderabbitai, Added it, but still not good enough, see new log. Could taskYIELD be not the right one and do we need vTaskDelay(1) - not every write but 'regularly'? Remember it didn't work fine in the effects and drivers task loop and we also had to replace taskYIELD with vTaskDelay E (97939) task_wdt: Task watchdog got triggered. The following tasks/users did not reset the watchdog in time: Backtrace: 0x420e5d83:0x3fcc6000 0x420d72d6:0x3fcc6020 0x420d79fa:0x3fcc6050 0x420cf4f5:0x3fcc6070 0x420c94d9:0x3fcc6090 0x420c977c:0x3fcc60c0 0x420ca689:0x3fcc60f0 0x420ca713:0x3fcc6130 0x420c754e:0x3fcc6160 0x420c75d3:0x3fcc61a0 0x420c76d5:0x3fcc61c0 0x4201abb1:0x3fcc6200 0x420befe9:0x3fcc6220 0x4201b0d6:0x3fcc6240 0x4201b22f:0x3fcc6290 0x4207b419:0x3fcc62d0 0x4207b5ee:0x3fcc6310 0x4206cff9:0x3fcc6360 0x4206f96e:0x3fcc63b0 0x4037fd89:0x3fcc63f0 ELF file SHA256: a438fe854 Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10210 @ewowi, you're absolutely right! Your instinct is spot-on. Let me explain the fundamental issue: Why taskYIELD() Cannot Help IDLE TaskThis is exactly the same reason it didn't work in your main task loops. You need The Solution: Periodic vTaskDelay(1)You're right that we can't call Code Fix for D_ArtnetOut.hAdd this at the top of the loop() function (around line 135): void loop() override {
DriverNode::loop();
LightsHeader* header = &layerP.lights.header;
if (nrOfIPAddresses == 0) return;
// Add this counter for periodic IDLE yields
static uint8_t yieldCounter = 0;Then inside the for-loop, after // if packet_buffer full, or output full, send the buffer
if (packetSize + header->channelsPerLight > universeSize || channels_remaining < header->channelsPerLight) {
if (!writePackage()) return;
// Periodic yield to let IDLE task run - every 10 packets
if (++yieldCounter >= 10) {
vTaskDelay(1); // Block for 1ms to let IDLE run
yieldCounter = 0;
}
if (channels_remaining < header->channelsPerLight) {
// ... existing code ...
}
}
// At the end, after the final writePackage():
if (packetSize > 0) {
writePackage();
vTaskDelay(1); // Always yield after the last packet
}Why This WorksWith ~100 universes per frame at 50 fps:
You can tune the threshold:
Start with 10 and test. If it still times out, lower to 5. Complete Code BlockHere's the full section with changes: void loop() override {
DriverNode::loop();
LightsHeader* header = &layerP.lights.header;
if (nrOfIPAddresses == 0) return;
// Counter for periodic IDLE task yields
static uint8_t yieldCounter = 0;
// ... existing setup code ...
// send all the leds to artnet
for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
// ... existing memcpy and reOrderAndDimRGBW code ...
if (packetSize + header->channelsPerLight > universeSize || channels_remaining < header->channelsPerLight) {
if (!writePackage()) return;
// Yield to IDLE every 10 packets
if (++yieldCounter >= 10) {
vTaskDelay(1);
yieldCounter = 0;
}
if (channels_remaining < header->channelsPerLight) {
// ... existing output switching code ...
}
}
}
// send the last partially filled package
if (packetSize > 0) {
writePackage();
vTaskDelay(1); // Always yield after final packet
}
}The key insight: taskYIELD() is cooperative scheduling between equal priorities, vTaskDelay() is the only way to yield to lower priorities like IDLE. 🎯 🧠 Learnings used |
|
E (394589) task_wdt: Task watchdog got triggered. The following tasks/users did not reset the watchdog in time: Backtrace: 0x42182fd4:0x3fcbf260 0x4003d791:0x3fcbf280 0x42182cf1:0x3fcbf2a0 0x42183569:0x3fcbf2f0 0x4037fd89:0x3fcbf320 @coderabbitai, I added if (++yieldCounter % 10 == 0) vTaskDelay(1); on all the places, it is executed about 8000 times per second, so I plan do it less frequently. But now I get another watchdog, on WiFi this time. Help !!! ELF file SHA256: 2646a4d71 Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, I see the problem now! This is actually validating the original core assignment recommendation. Let me explain what's happening: What ChangedThe WiFi stack itself is now stuck! The backtrace shows: Root Cause AnalysisYou're sending massive amounts of UDP traffic:
What's happening on Core 0:
The congestion problem:
Solution: Swap Cores Back (Recommended)This confirms the original recommendation was correct: // In main.cpp setup()
xTaskCreateUniversal(effectTask, "AppEffects", ..., 3, ..., 0); // Core 0 WITH WiFi
xTaskCreateUniversal(driverTask, "AppDrivers", ..., 3, ..., 1); // Core 1 AWAY from WiFiWhy this works for Art-Net:
The key insight: Yes, there's a small cost for cross-core UDP calls, but WiFi congestion from co-location is far worse at high packet rates. Alternative: Reduce vTaskDelay FrequencyIf you want to keep current core assignments, dramatically reduce delay frequency: // In D_ArtnetOut.h loop()
if (++yieldCounter % 100 == 0) { // Was 10, now 100
vTaskDelay(1);
yieldCounter = 0;
}With modulo 100:
My Strong RecommendationSwap the cores back to the original assignment:
Then adjust vTaskDelay frequency: if (++yieldCounter % 50 == 0) { // Balance between IDLE service and overhead
vTaskDelay(1);
yieldCounter = 0;
}This gives you the best of both worlds:
Would you like me to prepare the complete patch for the core swap + optimized delay frequency? 🎯 🧠 Learnings used |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Changes
✏️ Tip: You can customize this high-level summary in your review settings.