v0.7.0 preparations #76
Conversation
Front-end ======== - Row renderer: show default filter (!Unused) Back-End ======== - StatefulService: _accessMutex wait 600ms and go (for recursive calls) - WIP - NodeManager: defaultNodeName size 32 - ModuleEffects: layer1 has (default) value 0 - FastLED: show status, use assignedPin - layer->layerP-> to layerP.
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Layer access refactor src/MoonBase/Nodes.h, src/MoonBase/Nodes.cpp, src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h, src/MoonLight/Nodes/Effects/*, src/MoonLight/Nodes/Effects/E_MoonModules.h, src/MoonLight/Nodes/Effects/E_MovingHeads.h, src/MoonLight/Nodes/Effects/E_WLED.h, src/MoonLight/Nodes/Effects/E__Sandbox.h |
Replace layer->layerP / gNode->layer->layerP with direct layerP member access across nodes, drivers, and effects (palette/brightness/lights header/channel usages). |
Art‑Net multi‑IP & UDP type changes src/MoonLight/Nodes/Drivers/D_ArtnetOut.h, src/MoonLight/Nodes/Drivers/D_ArtnetIn.h |
ArtNetOut: add CSV controllerIP3s (text), ipAddresses[16], nrOfOutputs, processedOutputs, and onUpdate() to parse IPs and cycle outputs per IP. ArtNetIn: swap UDP type from WiFiUDP → NetworkUDP. |
Drivers: FastLED & Parallel consistency src/MoonLight/Nodes/Drivers/D_FastLED.h, src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h |
FastLED: add status field/control, safe‑mode gate in onLayout, local pins[] and status UI update. Parallel driver: adopt layerP access and adjust logging/initialization. |
New Ants effect src/MoonLight/Nodes/Effects/E_WLED.h |
Add Ant struct, constants, and AntEffect class with controls, per‑ant state, movement/collision logic, and rendering. |
Node/effect allocation simplification src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Modules/ModuleEffects.h, src/MoonBase/NodeManager.h |
Replace long else‑if allocation chains with guarded checkAndAlloc<T>(name) pattern; increase NodeManager::defaultNodeName size and add checkAndAlloc helper. |
Module devices / IO / pin mappings src/MoonBase/Modules/ModuleDevices.h, src/MoonBase/Modules/ModuleIO.h |
Change deviceUDP type WiFiUDP→NetworkUDP; rewrite device list handling to build a local JSON payload (filter/sort by timestamp); expand SE16V1/MHCP4NanoV1 switch pin mappings and consolidate onUpdate server update logic. |
StatefulService & EventEndpoint lib/framework/StatefulService.h, lib/framework/EventEndpoint.h, src/MoonBase/SharedEventEndpoint.h |
Adjust beginTransaction() wait behavior (log at 100ms, retry up to 400ms, then WARN instead of indefinite block) and remove early-return guards so syncState() always creates/emits JSON even with no connected clients. |
UI: Row/Field renderer & telemetry interface/src/lib/components/moonbase/RowRenderer.svelte, interface/src/lib/components/moonbase/FieldRenderer.svelte, interface/src/lib/stores/telemetry.ts, interface/src/routes/+layout.svelte, interface/src/routes/statusbar.svelte |
RowRenderer: reactive init of per-property filter defaults. FieldRenderer: adjust MDNS link href and remove console log. Telemetry/UI: stop persisting hostName in setRSSI; render title/hostname from telemetry store with MoonLight fallback. |
Docs & build config docs/moonbase/inputoutput.md, docs/moonlight/drivers.md, platformio.ini |
Update docs for input/output and drivers (Controller IP → Controller IPs, per‑IP outputs guidance) and bump APP_DATE / platform version in platformio.ini. |
Physical layer channel limit src/MoonLight/Layers/PhysicalLayer.cpp |
Reduce maxChannels in non‑PSRAM path from 81923 to 40963. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant UI as UI / Config
participant Art as ArtNetOutDriver
participant Layer as layerP (LightsHeader)
participant Net as NetworkUDP
UI->>Art: Update `controllerIP3s` (CSV)
Art->>Art: parse CSV → ipAddresses[], nrOfIPAddresses
loop periodic send cycle
Art->>Layer: read lights.header, channels, offsets
Art->>Art: build universe packet for current output
Art->>Net: send packet to current IP:port
Net-->>Art: send result/ack
Art->>Art: processedOutputs++
alt processedOutputs >= nrOfOutputs
Art->>Art: advance to next IP, reset processedOutputs
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Areas needing extra attention:
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h— CSV parsing, ipAddresses bounds (16), per‑IP cycling, universe/adaptor selection, NetworkUDP usage.src/MoonLight/Nodes/Effects/E_WLED.h— Ants feature correctness, resource/performance impact, duplicated docs.src/MoonLight/Nodes/Drivers/D_FastLED.h&src/MoonBase/Modules/ModuleIO.h— pin mapping correctness, safe‑mode behavior, onUpdate/server update logic.- Cross‑cutting
layerPrefactor and UDP type swaps (WiFiUDP→NetworkUDP) for consistency.
Possibly related PRs
- Board presets more presets #71 — overlapping Art‑Net driver and UDP/Network handling changes (ArtNetIn/Out).
- Lots of effects #77 — related effects refactor and additions (palette access changes and new effects like Ants).
- rename UI components #75 — related UI component edits touching
RowRenderer.svelteand renderer logic.
Suggested labels
enhancement
Poem
🐰 I hopped through layers and parsed each IP line,
ants march in pixels, LEDs align,
filters wake up steady at dawn's first light,
pins found their places, transactions wait polite,
code carrots planted — springtime in the compile.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 25.25% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'v0.7.0 preparations' is vague and generic, using non-descriptive terms that don't clearly convey what the changeset accomplishes. | Provide a more specific title that summarizes the main changes (e.g., 'Refactor layerP access patterns and update driver/effect implementations' or 'Update platform dependencies and fix RGB2040 flickering issues'). |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
v070-preparations
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: 5
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
115-115: Consider logging the assigned pin for clarity.Line 115 logs
layerP.ledPins[i](the original pin), but the actual pin being used ispins[i](the assigned pin). For debugging clarity, consider logging both the original and assigned pins, or just the assigned pin that will actually be used.- EXT_LOGD(ML_TAG, "onLayout pin#%d of %d: %d -> %d #%d", i, nrOfPins, layerP.ledPins[i], pins[i], layerP.ledsPerPin[i]); + EXT_LOGD(ML_TAG, "onLayout pin#%d of %d: assigned:%d -> physical:%d #%d", i, nrOfPins, assignedPin, pins[i], layerP.ledsPerPin[i]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
interface/src/lib/components/moonbase/RowRenderer.svelte(1 hunks)lib/framework/StatefulService.h(1 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonBase/Nodes.cpp(5 hunks)src/MoonBase/Nodes.h(2 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(2 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(3 hunks)src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h(7 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(7 hunks)src/MoonLight/Nodes/Effects/E_MoonModules.h(11 hunks)src/MoonLight/Nodes/Effects/E_MovingHeads.h(6 hunks)src/MoonLight/Nodes/Effects/E_WLED.h(11 hunks)src/MoonLight/Nodes/Effects/E__Sandbox.h(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/MoonBase/Nodes.cppsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonBase/Nodes.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/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.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/MoonBase/Nodes.h
🧬 Code graph analysis (4)
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
src/MoonBase/Nodes.cpp (2)
reOrderAndDimRGBW(560-568)reOrderAndDimRGBW(560-560)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (2)
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
loop(81-152)src/MoonLight/Nodes/Drivers/parlio.cpp (1)
show_parlio(265-265)
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
src/MoonBase/Nodes.cpp (6)
setup(140-204)setup(140-140)setup(347-363)setup(347-347)updateControl(17-66)updateControl(17-17)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonBase/Nodes.h (2)
src/MoonLight/Layers/PhysicalLayer.cpp (4)
addLight(154-175)addLight(154-154)nextPin(177-196)nextPin(177-177)src/MoonLight/Layers/VirtualLayer.cpp (2)
addLight(342-359)addLight(342-342)
⏰ 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 (20)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (2)
50-75: LGTM! Clean refactoring in loop() method.The replacement of
layer->layerPwith directlayerPaccess is consistent across both build paths (HP_ALL_DRIVERS and ESP32_LEDSDRIVER). The logic remains unchanged, and the code is now more straightforward.
78-182: LGTM! Comprehensive refactoring across all build configurations.The refactoring is consistent throughout the onLayout() method across all build paths (HP_ALL_DRIVERS, ESP32P4, ESP32_LEDSDRIVER). All driver initialization and update calls now use direct
layerPmember access, and verification confirms no legacylayer->layerPreferences remain in the codebase.src/MoonLight/Nodes/Effects/E_MoonModules.h (3)
406-454: Consistent refactoring applied.The palette access refactoring in
GEQ3DEffectis consistent with the changes inGameOfLifeEffect. All three palette accesses correctly uselayerP.palette.
556-558: Consistent refactoring applied.The palette access refactoring in
PaintBrushEffectis consistent with the rest of the file. Both color modes (chaos and normal) correctly uselayerP.palette.
26-313: Refactoring verified: layerP is properly accessible as a global instance.The changes replacing
layer->layerP->palettewithlayerP.palettethroughout the GameOfLifeEffect class are correct.layerPis declared as an extern globalPhysicalLayerinstance inPhysicalLayer.hand defined inPhysicalLayer.cpp, making it consistently accessible from all effect classes. No remaininglayer->layerPreferences exist in the codebase, confirming the refactoring is complete.interface/src/lib/components/moonbase/RowRenderer.svelte (1)
152-157: The reactive filter initialization is correct. Mutating a$bindableprop inside$effectis the intended use case in Svelte 5 and poses no reactivity issues. The guard conditionif (data[property.name + '_filter'] == null)naturally prevents unnecessary reassignments, and the loose equality check intentionally catches bothnullandundefined.Likely an incorrect or invalid review comment.
src/MoonLight/Nodes/Drivers/D_FastLED.h (3)
49-49: LGTM: Status field addition.The new status field provides visibility into the driver state.
57-57: LGTM: Status control exposed in UI.The read-only status control provides appropriate visibility to users.
122-122: TheupdateControloverload is correctly defined and supports this usage.The two-parameter
updateControl(const char* name, const T value)template method inNodes.hhandles this call properly. When invoked withupdateControl("status", statusString.c_str()), the template deducesTasconst char*and functions as intended.src/MoonBase/Nodes.cpp (4)
186-186: Verify external variable registration for LiveScript.The change to
(CRGB*)layerP.lights.channelsfor theledsexternal variable is critical for LiveScript functionality. Ensure that:
layerPis accessible in theLiveScriptNode::setup()context- The pointer cast is correct for the LiveScript external variable system
- This change aligns with other LiveScript external variable declarations
402-402: LGTM - Consistent pointer access pattern.The refactoring to use
layerP.lights.headerdirectly is consistent with the PR objectives. The subsequent usage ofheader->throughout the function remains correct.
564-567: LGTM - Uniform refactoring of offset access.All four lines correctly updated to access offsets via
layerP.lights.headerinstead of the nested pointer path. The logic for RGB reordering and the conditional white channel handling remain correct. Refactoring is complete with no remaininglayer->layerPpatterns in the codebase.
79-80: The refactoring is correct;layerPis properly accessible in static functions.Static member functions can only access static data members, other static member functions and any other functions from outside the class. However,
layerPis not a class member—it is a global variable. The shell verification confirms thatlayerPis declared asextern PhysicalLayer layerP;insrc/MoonLight/Layers/PhysicalLayer.h(line 153) and defined asPhysicalLayer layerP;insrc/MoonLight/Layers/PhysicalLayer.cpp(line 22). Global variables are accessible from all functions, including static functions, so the direct access tolayerPat lines 79–80 is valid and requires no changes.src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
55-55: LGTM! Palette access refactored consistently.The changes successfully replace nested pointer dereferencing (
layer->layerP->palette) with direct member access (layerP.palette), aligning with the PR's refactoring objective. The ColorFromPalette calls now use the cleaner access pattern.Also applies to: 97-97, 103-103
src/MoonBase/Nodes.h (1)
210-210: LGTM! Node helper methods refactored cleanly.The refactoring successfully updates internal access patterns from
layer->layerP->tolayerP.across:
- Request mapping flags (lines 210, 214)
- Light addition delegation (line 227)
- Pin management delegation (line 230)
These changes maintain the same functional behavior while simplifying the access pattern, consistent with the broader PR refactoring.
Also applies to: 214-214, 227-227, 230-230
src/MoonLight/Nodes/Effects/E_MovingHeads.h (1)
28-28: LGTM! Brightness access refactored consistently across moving head effects.All brightness accesses have been updated from
layer->layerP->lights.header.brightnesstolayerP.lights.header.brightness. The commented lines (31, 124, 163, 242, 319) appropriately document that brightness is handled automatically in those contexts.Also applies to: 31-31, 121-121, 124-124, 163-163, 242-242, 284-284, 319-319
src/MoonBase/NodeManager.h (1)
22-22: LGTM! Increased buffer size for node names.Expanding
defaultNodeNamefromChar<20>toChar<32>provides more space for longer node names, which is a reasonable defensive change given the variety of effects and drivers.src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
84-84: LGTM! Art-Net driver refactored to use direct layer access.The refactoring updates all lighting header and channel accesses from
layer->layerP->lightstolayerP.lights. This includes:
- Header pointer initialization (line 84)
- Channel data access (line 116)
- RGB channel ordering and brightness adjustments (lines 119-123)
The changes maintain identical functionality while simplifying the access pattern.
Also applies to: 116-116, 119-123
src/MoonLight/Nodes/Effects/E_WLED.h (1)
79-79: LGTM! WLED effects refactored for direct palette and gamma access.All palette accesses across multiple WLED-inspired effects (BouncingBalls, Blurz, DistortionWaves, GEQ, Lissajous, Noise2D, NoiseMeter, PopCorn, Waverly) have been updated from
layer->layerP->palettetolayerP.palette.The gamma correction calls (lines 222-224) have also been updated to use
layerP.gamma8, ensuring consistent access patterns throughout.Also applies to: 88-88, 162-162, 169-169, 222-224, 412-412, 454-454, 477-477, 511-511, 596-596, 643-643
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
36-36: LGTM! MoonLight effects refactored consistently.The refactoring successfully updates palette and lighting header accesses across all MoonLight effects (Solid, Random, StarField, Praxis, FreqSaws, Particles, SpiralFire) from
layer->layerP->tolayerP.. This maintains functional equivalence while simplifying the access pattern throughout the effect implementations.Also applies to: 169-169, 446-446, 509-509, 674-674, 1155-1155, 1339-1339
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) { | ||
| ESP_LOGW("🐼", "_accessMutex wait too long"); | ||
| xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | ||
| ESP_LOGI("🐼", "_accessMutex wait 100ms"); | ||
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) { | ||
| ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️"); | ||
| // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Mutex acquisition failure leads to unprotected access and semaphore corruption.
If both xSemaphoreTakeRecursive attempts fail (lines 201 and 203), the function continues without holding the mutex. This causes:
- Race conditions: The code between
beginTransaction()andendTransaction()will execute without mutex protection, allowing concurrent access to_state. - Semaphore corruption:
endTransaction()(line 212) will callxSemaphoreGiveRecursivewithout a matching successful take, violating the semaphore contract. - Silent failure: Callers have no way to detect that mutual exclusion has failed.
Consider one of these solutions:
Option 1 (Recommended): Track acquisition success and conditionally release
inline void beginTransaction()
{
+ _mutexAcquired = false;
// 🌙 adding semaphore wait too long logging
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
- ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
- // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
+ ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
+ xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
}
}
+ _mutexAcquired = true;
}
inline void endTransaction()
{
- xSemaphoreGiveRecursive(_accessMutex);
+ if (_mutexAcquired) {
+ xSemaphoreGiveRecursive(_accessMutex);
+ _mutexAcquired = false;
+ }
}(Add bool _mutexAcquired = false; member variable)
Option 2: Restore indefinite blocking
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
- ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
- // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
+ ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
+ xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
}📝 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.
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) { | |
| ESP_LOGW("🐼", "_accessMutex wait too long"); | |
| xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | |
| ESP_LOGI("🐼", "_accessMutex wait 100ms"); | |
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) { | |
| ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️"); | |
| // xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | |
| } | |
| } | |
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) { | |
| ESP_LOGI("🐼", "_accessMutex wait 100ms"); | |
| if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) { | |
| ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely"); | |
| xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/framework/StatefulService.h around lines 201-207, the code continues when
both xSemaphoreTakeRecursive calls fail which leads to unprotected access and a
mismatched give; fix by tracking whether the mutex was actually acquired and
only releasing it if held. Add a local bool (e.g. mutexAcquired) initialized
false, set it true when either take succeeds, and if both attempts fail either
return an error/abort the transaction or block indefinitely per policy; update
endTransaction() to check mutexAcquired before calling xSemaphoreGiveRecursive
(or clear the flag after give) so give is only called on a successful take, and
ensure callers handle the failure path (propagate error or avoid accessing
_state when mutexAcquired is false).
There was a problem hiding this comment.
I agree this is a risk 2 processes mutating the same, however due to recursiveness, the same process can try to take the mutex twice, at the moment it is too much overhead to track if the same process has acquired it already, but you may suggest an easy fix for that.
I don't think xSemaphoreGiveRecursive will fail if no xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); has taken place, but any feedback on that statement appreciated
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any FreeRTOS documentation or comments about semaphore behavior in the codebase
rg -n -C3 "xSemaphoreGive" --type=cpp --type=c --type=h
Length of output: 4107
🌐 Web query:
FreeRTOS xSemaphoreGiveRecursive behavior without matching take
💡 Result:
Short answer: if you call xSemaphoreGiveRecursive() on a recursive mutex that the calling task does not hold, the call will fail (return pdFALSE) — it does not silently decrement an internal count or hand ownership to the caller. FreeRTOS documents that xSemaphoreGiveRecursive returns pdTRUE when the semaphore is successfully given and pdFALSE on error (for example when the semaphore was not previously obtained), and community reports describe xSemaphoreGiveRecursive returning FALSE when the mutex is not owned by the caller. [1][2]
Sources:
- FreeRTOS API reference (xSemaphoreGiveRecursive / behavior & return values). [1]
- FreeRTOS forum thread reporting xSemaphoreGiveRecursive returns FALSE if the mutex is not held by the calling task. [2]
@ewowi, I need to clarify a few points about FreeRTOS recursive mutexes:
About Recursive Tracking
Good news: There's no overhead! FreeRTOS recursive mutexes (xSemaphoreCreateRecursiveMutex) already track recursion internally. When the same task calls xSemaphoreTakeRecursive multiple times, FreeRTOS automatically:
- Increments an internal counter
- Allows the take to succeed immediately (same task)
- Requires matching
xSemaphoreGiveRecursivecalls to fully release
This is built into FreeRTOS - you don't need to implement any tracking yourself. The recursive mutex exists specifically to handle same-task re-entry without deadlock.
About xSemaphoreGiveRecursive Failure
Your assumption is incorrect: xSemaphoreGiveRecursive does fail when called without a matching take. It returns pdFALSE when the mutex is not held by the calling task. Your current code ignores this return value, so you won't notice the failure, but the semaphore state can become inconsistent.
The Real Problem
The critical issue isn't recursion tracking - it's that if both xSemaphoreTakeRecursive attempts fail (lines 201 and 203), your code continues to execute the critical section without mutex protection. This creates a race condition where multiple tasks can simultaneously access _state.
Easy Fix
The simplest solution is to restore the indefinite blocking you commented out:
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // ← uncomment this
}
}This ensures the mutex is always acquired before accessing _state, preventing race conditions. The logging provides diagnostics for abnormally long waits while maintaining safety.
🧬 Code Graph Analysis Results
[StatefulService.h:38-44] Constructor and recursive mutex initialization
- Summary: StatefulService(Args&&... args) initializes _state with forwarded args and creates _accessMutex using xSemaphoreCreateRecursiveMutex().
[StatefulService.h:100-115] beginTransaction
- Summary: Attempts to take _accessMutex recursively with a 100 ms timeout; if it fails, logs a 100 ms wait, then retries for 400 ms; if still failing, logs a warning and continues (comment indicates potential further attempt). This manages nested/protected access for state mutations.
[StatefulService.h:116-123] endTransaction
- Summary: Releases the recursive mutex via xSemaphoreGiveRecursive(_accessMutex).
[StatefulService.h:125-133] private members
- Summary: Private members include SemaphoreHandle_t _accessMutex and lists for update and hook handlers.
There was a problem hiding this comment.
Okay, correction: the thing is not that the same process tries to acquire it, I think the effect task processing a state change, hands over to the SvelteKit task to recursively call begin transaction, apparently before the effect task has freed it. Or something like this
What I can tell you that occasionally infinite locks happen.
So can you check the code to find the root cause for this locking between multiple processes? And provide a solution. Maybe some tasks need to explicitly give the mutex if they are done with whatever they are doing ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for addUpdateHandler calls to understand what callbacks are registered
echo "=== addUpdateHandler usage ==="
rg -n -C3 'addUpdateHandler\(' --type=cpp --type=h -g '!*.md'
Length of output: 13480
🏁 Script executed:
#!/bin/bash
# Search for update() method calls on StatefulService to see patterns
echo "=== StatefulService update() patterns ==="
rg -n -C3 '\.update\(' --type=cpp --type=h -g '!*.md' -g '!*Test*' | head -200
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Look for task creation patterns (xTaskCreate, xTaskCreatePinnedToCore)
echo "=== Task creation patterns ==="
rg -n -C5 'xTaskCreate' --type=cpp --type=h
Length of output: 3687
🏁 Script executed:
#!/bin/bash
# Search for callUpdateHandlers to see where it's invoked
echo "=== callUpdateHandlers invocations ==="
rg -n -C5 'callUpdateHandlers' --type=cpp --type=h
Length of output: 4064
🏁 Script executed:
#!/bin/bash
# Look for patterns where update handlers might trigger nested updates
echo "=== Nested state access patterns ==="
rg -n -C3 'beginTransaction|endTransaction' --type=cpp --type=h | head -150
Length of output: 4292
🏁 Script executed:
#!/bin/bash
# Search for patterns where update handlers call read() or update() on other modules
echo "=== Cross-module calls in update handlers ==="
rg -n -A10 'addUpdateHandler.*\[' --type=cpp --type=h | grep -E '(->read\(|->update\(|\.read\(|\.update\()' | head -30
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Look for FileManager update handlers that might trigger cascading updates
echo "=== FileManager update handler patterns ==="
rg -n -A15 '_fileManager->addUpdateHandler' --type=cpp --type=h
Length of output: 4972
🏁 Script executed:
#!/bin/bash
# Look for ModuleIO update handler patterns
echo "=== ModuleIO update handler patterns ==="
rg -n -A10 'moduleIO.*addUpdateHandler|_moduleIO->addUpdateHandler' --type=cpp --type=h
Length of output: 3465
🏁 Script executed:
#!/bin/bash
# Check if there's any async task queue or deferred execution mechanism
echo "=== Async/deferred execution patterns ==="
rg -n 'xQueue|QueueHandle|vTaskDelay|yield|defer' --type=cpp --type=h | head -50
Length of output: 1190
@ewowi, I've found the root cause of your deadlock issue! It's a classic circular dependency deadlock between multiple tasks, not a recursive lock issue.
Root Cause Analysis
Your system has multiple tasks that can update modules concurrently:
- effectTask (priority 3, core 1)
- driverTask (priority 3, core 1)
- ESP32SvelteKit loop task (priority 2, core 0)
The deadlock occurs because update handlers immediately acquire mutexes of other modules:
Example Deadlock Chain
From ModuleLiveScripts.h:40-48:
_fileManager->addUpdateHandler([this](const String& originId) {
// Handler runs in context of whatever task called FileManager.update()
_fileManager->read([&](FilesState& filesState) { // ← Re-acquires FileManager mutex!
_moduleEffects->read([&](ModuleState& effectsState) { // ← Also acquires Effects mutex!Deadlock scenario:
- Task A (effectTask): Updates
moduleEffects→ holds Effects mutex → triggers FileManager handler → tries_fileManager->read()→ blocks waiting for FileManager mutex - Task B (SvelteKit): Updates
FileManager→ holds FileManager mutex → triggers handler → tries_moduleEffects->read()→ blocks waiting for Effects mutex - Result: Task A waits for B's mutex, Task B waits for A's mutex → infinite deadlock 🔒
Similar chains exist for ModuleIO → ModuleDrivers/ModuleLightsControl.
Why Recursive Mutex Doesn't Help
Recursive mutexes only allow the same task to re-enter. Different tasks (effectTask vs SvelteKit loop) cannot share ownership, so the recursive property provides no protection against cross-task deadlocks.
The Current Code Problem
Your commented-out portMAX_DELAY creates two severe bugs:
- Unprotected access: Code continues without mutex → race conditions
- Semaphore corruption:
endTransaction()callsxSemaphoreGiveRecursive()without successful take → undefined behavior
Solutions
Immediate Fix: Restore Blocking (Required)
This prevents unprotected access and semaphore corruption:
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms, blocking indefinitely");
xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // ← REQUIRED
}
}However, this will cause permanent hangs on deadlock instead of unprotected access. You need the architectural fix below.
Architectural Fix: Defer Cross-Module Calls
The proper solution is to never call .read() or .update() on other modules from within update handlers. Instead, use a deferred execution pattern:
Option 1: Task Notification Pattern (Recommended)
Add a "dirty flag" mechanism to modules that need cross-module data:
class NodeManager {
private:
volatile bool _fileManagerDirty = false;
public:
void begin() {
// Set flag instead of immediate read
_fileManager->addUpdateHandler([this](const String& originId) {
_fileManagerDirty = true; // Atomic flag set
});
}
void loop() {
// Check and process in your own task context
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
// Safe: runs in NodeManager's task, not handler context
for (auto updatedItem : filesState.updatedItems) {
// ... process updates
}
});
}
// ... rest of loop
}
};Option 2: Pass Data in Handler (Alternative)
Modify handlers to receive necessary data as parameters:
// In StatefulService.h, create a variant that passes state copy
typedef std::function<void(const String &originId, const T& stateCopy)> StateUpdateCallbackWithData;
void callUpdateHandlers(const String &originId) {
T stateCopy = _state; // Copy outside mutex
for (const StateUpdateHandlerInfo_t &updateHandler : _updateHandlers) {
updateHandler._cb(originId, stateCopy); // Pass copy
}
}Then handlers don't need to call .read():
_fileManager->addUpdateHandler([this](const String& originId, const FilesState& filesState) {
// No read() call needed - data passed in
for (auto updatedItem : filesState.updatedItems) {
// ... process
}
});Specific Code Changes Needed
Looking at your codebase, these handlers need refactoring:
- NodeManager.h:41 - FileManager handler calls
_fileManager->read() - ModuleLiveScripts.h:40 - FileManager handler calls
_fileManager->read()AND_moduleEffects->read() - ModuleLightsControl.h:48 - FileManager handler calls
_fileManager->read() - ModuleLightsControl.h:68 - ModuleIO handler calls
moduleIO.read() - ModuleDrivers.h:76 - ModuleIO handler calls (presumably) other reads in
readPins()
All of these should either:
- Use dirty flags and check them in the module's
loop()method - Receive data as parameters instead of calling
.read()
Would you like me to generate a detailed refactoring for one of these as an example?
🧬 Code Graph Analysis Results
<filename=lib/framework/StatefulService.h>
Line range (approximate): Constructor
template
class StatefulService
{
public:
template <typename... Args>
StatefulService(Args &&...args) : _state(std::forward(args)...), _accessMutex(xSemaphoreCreateRecursiveMutex())
{
}
<filename=lib/framework/StatefulService.h>
Line range (approximate): beginTransaction
inline void beginTransaction()
{
// 🌙 adding semaphore wait too long logging
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
// xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY);
}
}
}
<filename=lib/framework/StatefulService.h>
Line range (approximate): endTransaction
inline void endTransaction()
{
xSemaphoreGiveRecursive(_accessMutex);
}
<filename=lib/framework/StatefulService.h>
Line range (approximate): private members
private:
SemaphoreHandle_t _accessMutex;
std::list<StateUpdateHandlerInfo_t> _updateHandlers;
std::list<StateHookHandlerInfo_t> _hookHandlers;
<no_relevant_code_snippets>
There was a problem hiding this comment.
@coderabbitai,regarding
Would you like me to generate a detailed refactoring for one of these as an example?
Yes for all 5 files please
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and extract the problematic update handler patterns from all 5 files
echo "=== 1. NodeManager.h - FileManager handler ==="
rg -n -A20 'addUpdateHandler' src/MoonBase/NodeManager.h | head -30
echo -e "\n=== 2. ModuleLiveScripts.h - FileManager handler ==="
rg -n -A25 'addUpdateHandler' src/MoonLight/Modules/ModuleLiveScripts.h | head -40
echo -e "\n=== 3. ModuleLightsControl.h - FileManager handler (line 48) ==="
rg -n -A20 'addUpdateHandler' src/MoonLight/Modules/ModuleLightsControl.h | head -60
echo -e "\n=== 4. ModuleLightsControl.h - ModuleIO handler (line 68) ==="
rg -n -A20 'addUpdateHandler' src/MoonLight/Modules/ModuleLightsControl.h | tail -40
echo -e "\n=== 5. ModuleDrivers.h - ModuleIO handler ==="
rg -n -A20 'addUpdateHandler' src/MoonLight/Modules/ModuleDrivers.h | head -40
Length of output: 8344
🏁 Script executed:
#!/bin/bash
# Get the full class definitions to understand member variables and loop() methods
echo "=== NodeManager class structure ==="
ast-grep --pattern $'class NodeManager {
$$$
}' src/MoonBase/NodeManager.h
echo -e "\n=== ModuleLiveScripts class structure ==="
rg -n 'class ModuleLiveScripts|void loop\(|private:|public:' src/MoonLight/Modules/ModuleLiveScripts.h | head -20
echo -e "\n=== ModuleLightsControl class structure ==="
rg -n 'class ModuleLightsControl|void loop\(|private:|public:' src/MoonLight/Modules/ModuleLightsControl.h | head -20
echo -e "\n=== ModuleDrivers class structure ==="
rg -n 'class ModuleDrivers|void loop\(|private:|public:' src/MoonLight/Modules/ModuleDrivers.h | head -20
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Check if these modules already have loop() methods
echo "=== Looking for existing loop() implementations ==="
echo "NodeManager.h:"
rg -n -A10 'void loop\(' src/MoonBase/NodeManager.h
echo -e "\nModuleLiveScripts.h:"
rg -n -A10 'void loop\(' src/MoonLight/Modules/ModuleLiveScripts.h
echo -e "\nModuleLightsControl.h:"
rg -n -A10 'void loop\(' src/MoonLight/Modules/ModuleLightsControl.h
echo -e "\nModuleDrivers.h:"
rg -n -A10 'void loop\(' src/MoonLight/Modules/ModuleDrivers.h
Length of output: 956
@ewowi, here are the detailed refactoring examples for all 5 files. The pattern is consistent: set flags in handlers, check flags in loop().
1. NodeManager.h
Add private member variable:
private:
volatile bool _fileManagerDirty = false;BEFORE (lines 41-61):
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
EXT_LOGV(ML_TAG, "updateHandler updatedItem %s", updatedItem.c_str());
Char<32> name;
name.format("/.config/%s.json", _moduleName);
if (equal(updatedItem.c_str(), name.c_str())) {
EXT_LOGV(ML_TAG, " %s updated -> call update %s", name.c_str(), updatedItem.c_str());
readFromFS(); // repopulates the state, processing file changes
}
}
});
});AFTER:
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of immediate read
});Add loop() method:
void loop() override {
Module::loop(); // Call parent if NodeManager inherits from Module
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
for (auto updatedItem : filesState.updatedItems) {
EXT_LOGV(ML_TAG, "Processing updatedItem %s", updatedItem.c_str());
Char<32> name;
name.format("/.config/%s.json", _moduleName);
if (equal(updatedItem.c_str(), name.c_str())) {
EXT_LOGV(ML_TAG, " %s updated -> call readFromFS %s", name.c_str(), updatedItem.c_str());
readFromFS(); // Safe: runs in NodeManager's task context
}
}
});
}
}2. ModuleLiveScripts.h
Add private member variables:
private:
volatile bool _fileManagerDirty = false;BEFORE (lines 40-65):
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
uint8_t index = 0;
_moduleEffects->read([&](ModuleState& effectsState) {
for (JsonObject nodeState : effectsState.data["nodes"].as<JsonArray>()) {
if (updatedItem == nodeState["name"]) {
EXT_LOGD(ML_TAG, "updateHandler equals current item -> livescript compile %s", updatedItem.c_str());
LiveScriptNode* liveScriptNode = (LiveScriptNode*)_moduleEffects->findLiveScriptNode(nodeState["name"]);
if (liveScriptNode) {
liveScriptNode->compileAndRun();
_moduleEffects->requestUIUpdate = true; // update the Effects UI
}
EXT_LOGD(ML_TAG, "update due to new node %s done", nodeState["name"].as<const char*>());
}
index++;
}
});
}
});
});AFTER:
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of nested reads
});Add loop() method:
void loop() override {
Module::loop();
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
// Safe: both reads execute in ModuleLiveScripts task context
_fileManager->read([&](FilesState& filesState) {
for (auto updatedItem : filesState.updatedItems) {
uint8_t index = 0;
_moduleEffects->read([&](ModuleState& effectsState) {
for (JsonObject nodeState : effectsState.data["nodes"].as<JsonArray>()) {
if (updatedItem == nodeState["name"]) {
EXT_LOGD(ML_TAG, "Compiling livescript %s", updatedItem.c_str());
LiveScriptNode* liveScriptNode = (LiveScriptNode*)_moduleEffects->findLiveScriptNode(nodeState["name"]);
if (liveScriptNode) {
liveScriptNode->compileAndRun();
_moduleEffects->requestUIUpdate = true;
}
EXT_LOGD(ML_TAG, "Update for node %s done", nodeState["name"].as<const char*>());
}
index++;
}
});
}
});
}
}3. ModuleLightsControl.h - FileManager Handler
Add private member variables:
private:
volatile bool _fileManagerDirty = false;
volatile bool _moduleIODirty = false;BEFORE (lines 48-67):
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s", originId.c_str());
// read the file state (read all files and folders on FS and collect changes)
_fileManager->read([&](FilesState& filesState) {
// loop over all changed files (normally only one)
bool presetChanged = false;
for (auto updatedItem : filesState.updatedItems) {
// if file is the current live script, recompile it (to do: multiple live effects)
EXT_LOGV(ML_TAG, "updateHandler updatedItem %s", updatedItem.c_str());
if (strstr(updatedItem.c_str(), "/.config/presets")) {
EXT_LOGV(ML_TAG, " preset.json updated -> call update %s", updatedItem.c_str());
presetChanged = true;
}
}
if (presetChanged) {
EXT_LOGV(ML_TAG, "setPresetsFromFolder");
setPresetsFromFolder(); // update the presets from the folder
}
});
});AFTER:
_fileManager->addUpdateHandler([this](const String& originId) {
EXT_LOGV(ML_TAG, "FileManager::updateHandler %s (deferred)", originId.c_str());
_fileManagerDirty = true; // Set flag instead of immediate read
});4. ModuleLightsControl.h - ModuleIO Handler
BEFORE (line 68):
moduleIO.addUpdateHandler([this](const String& originId) { readPins(); }, false);
readPins(); // initiallyAFTER:
moduleIO.addUpdateHandler([this](const String& originId) {
_moduleIODirty = true; // Set flag instead of immediate readPins()
}, false);
readPins(); // initially - still safe as it's in setup contextBEFORE readPins() (lines 72-88):
void readPins() {
moduleIO.read([&](ModuleState& state) {
pinRelayLightsOn = UINT8_MAX;
pinButtonLightsOn = UINT8_MAX;
for (JsonObject pinObject : state.data["pins"].as<JsonArray>()) {
uint8_t usage = pinObject["usage"];
uint8_t gpio = pinObject["GPIO"];
if (usage == pin_Relay_LightsOn) {
if (GPIO_IS_VALID_OUTPUT_GPIO(gpio)) {
pinRelayLightsOn = gpio;
pinMode(pinRelayLightsOn, OUTPUT);
uint8_t newBri = _state.data["lightsOn"] ? _state.data["brightness"] : 0;
digitalWrite(pinRelayLightsOn, newBri > 0 ? HIGH : LOW);
EXT_LOGD(ML_TAG, "pinRelayLightsOn found %d", pinRelayLightsOn);
} else
EXT_LOGE(MB_TAG, "gpio %d not valid", pinRelayLightsOn);
}
// ... rest of pin processing
}
});
}AFTER readPins() (no changes needed - method stays the same):
// readPins() stays unchanged - just call it from loop() instead of handler
void readPins() {
moduleIO.read([&](ModuleState& state) {
// ... same implementation ...
});
}Modify existing loop() (line 271):
void loop() override {
Module::loop();
// Process deferred FileManager updates
if (_fileManagerDirty) {
_fileManagerDirty = false;
_fileManager->read([&](FilesState& filesState) {
bool presetChanged = false;
for (auto updatedItem : filesState.updatedItems) {
EXT_LOGV(ML_TAG, "Processing updatedItem %s", updatedItem.c_str());
if (strstr(updatedItem.c_str(), "/.config/presets")) {
EXT_LOGV(ML_TAG, "preset.json updated");
presetChanged = true;
}
}
if (presetChanged) {
EXT_LOGV(ML_TAG, "setPresetsFromFolder");
setPresetsFromFolder();
}
});
}
// Process deferred ModuleIO updates
if (_moduleIODirty) {
_moduleIODirty = false;
readPins(); // Safe: runs in ModuleLightsControl task context
}
// ... existing preset loop code ...
uint8_t presetLoop = _state.data["presetLoop"];
if (presetLoop && millis() - lastPresetTime > presetLoop * 1000) {
lastPresetTime = millis();
// ... rest of existing loop code ...
}
}5. ModuleDrivers.h
Add private member variable:
private:
volatile bool _moduleIODirty = false;
bool initPins = false; // Existing variableBEFORE (line 76):
_moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false);AFTER:
_moduleIO->addUpdateHandler([this](const String& originId) {
_moduleIODirty = true; // Set flag instead of immediate readPins()
}, false);Modify existing loop() (lines 178-188):
BEFORE:
void loop() override {
NodeManager::loop();
if (!initPins) {
readPins(); // initially
initPins = true;
}
}AFTER:
void loop() override {
NodeManager::loop();
// Process deferred ModuleIO updates
if (_moduleIODirty) {
_moduleIODirty = false;
readPins(); // Safe: runs in ModuleDrivers task context
}
// Initial pin read (keeps existing behavior)
if (!initPins) {
readPins();
initPins = true;
}
}Summary of Changes
For each file, the refactoring follows this pattern:
- Add
volatile booldirty flags for each external module being accessed - Update handlers set flags instead of calling
.read()or other methods - loop() checks flags and processes updates in the module's own task context
- Eliminate cross-task mutex acquisition - each task only acquires its own mutex and those of other modules serially within its own execution context
This completely eliminates the circular dependency deadlock because:
- No nested mutex acquisition in handlers - handlers only set atomic flags
- Cross-module reads happen in loop() - a single task context acquires mutexes serially (FileManager → Effects) rather than concurrently
- No Task A holding Mutex 1 while waiting for Mutex 2 scenario
The volatile keyword ensures flag visibility across tasks without requiring mutex protection for the flag itself (it's a simple boolean).
Would you like me to also show how to implement this with a task notification pattern instead of polling flags, which can be more efficient?
🧬 Code Graph Analysis Results
lib/framework/MqttSettingsService.h, lines around 109-120
static StateUpdateResult update(JsonObject &root, MqttSettings &settings, const String &originId)
{
settings.enabled = root["enabled"] | FACTORY_MQTT_ENABLED;
settings.uri = root["uri"] | FACTORY_MQTT_URI;
settings.username = root["username"] | SettingValue::format(FACTORY_MQTT_USERNAME);
settings.password = root["password"] | FACTORY_MQTT_PASSWORD;
settings.clientId = root["client_id"] | SettingValue::format(FACTORY_MQTT_CLIENT_ID);
settings.keepAlive = root["keep_alive"] | FACTORY_MQTT_KEEP_ALIVE;
settings.cleanSession = root["clean_session"] | FACTORY_MQTT_CLEAN_SESSION;
settings.messageIntervalMs = root["message_interval_ms"] | FACTORY_MQTT_MIN_MESSAGE_INTERVAL_MS;
return StateUpdateResult::CHANGED;
}
lib/framework/MqttSettingsService.h, lines around 96-107
static void
read(MqttSettings &settings, JsonObject &root)
{
root["enabled"] = settings.enabled;
root["uri"] = settings.uri;
root["username"] = settings.username;
root["password"] = settings.password;
root["client_id"] = settings.clientId;
root["keep_alive"] = settings.keepAlive;
root["clean_session"] = settings.cleanSession;
root["message_interval_ms"] = settings.messageIntervalMs;
}
lib/framework/WiFiSettingsService.h, lines around 90-224
static void read(WiFiSettings &settings, JsonObject &root)
{
root["hostname"] = settings.hostname;
root["connection_mode"] = settings.staConnectionMode;
root["txPower"] = settings.txPower;//(uint8_t )WiFi.getTxPower();
root["txPowerMeasured"] = abs(WiFi.getTxPower());
root["trackAnalytics"] = settings.trackAnalytics;
// create JSON array from root
JsonArray wifiNetworks = root["wifi_networks"].to<JsonArray>();
// iterate over the wifiSettings
for (auto &wifi : settings.wifiSettings)
{
// create JSON object for each wifi network
JsonObject wifiNetwork = wifiNetworks.add<JsonObject>();
// add the ssid and password to the JSON object
wifiNetwork["ssid"] = wifi.ssid;
wifiNetwork["password"] = wifi.password;
wifiNetwork["static_ip_config"] = wifi.staticIPConfig;
// extended settings
JsonUtils::writeIP(wifiNetwork, "local_ip", wifi.localIP);
JsonUtils::writeIP(wifiNetwork, "gateway_ip", wifi.gatewayIP);
JsonUtils::writeIP(wifiNetwork, "subnet_mask", wifi.subnetMask);
JsonUtils::writeIP(wifiNetwork, "dns_ip_1", wifi.dnsIP1);
JsonUtils::writeIP(wifiNetwork, "dns_ip_2", wifi.dnsIP2);
}
ESP_LOGV(SVK_TAG, "WiFi Settings read");
}
static StateUpdateResult update(JsonObject &root, WiFiSettings &settings, const String &originId)
{
settings.hostname = root["hostname"] | SettingValue::format(FACTORY_WIFI_HOSTNAME);
settings.staConnectionMode = root["connection_mode"] | 1; // default to STRENGTH
// 🌙
#if CONFIG_IDF_TARGET_ESP32C3 | LOLIN_WIFI_FIX
settings.txPower = root["txPower"] | 34; // default to 8.5dBm
#else
settings.txPower = root["txPower"] | 0; // default (do not set the power)
#endif
settings.trackAnalytics = root["trackAnalytics"].isNull()?true:root["trackAnalytics"]; // 🌙
settings.wifiSettings.clear();
// create JSON array from root
JsonArray wifiNetworks = root["wifi_networks"];
if (root["wifi_networks"].is<JsonArray>())
{
// iterate over the wifiSettings
int i = 0;
for (auto wifiNetwork : wifiNetworks)
{
// max 5 wifi networks
if (i++ >= 5)
{
ESP_LOGE(SVK_TAG, "Too many wifi networks");
break;
}
// create JSON object for each wifi network
JsonObject wifi = wifiNetwork.as<JsonObject>();
// Check if SSID length is between 1 and 31 characters and password between 0 and 64 characters
if (wifi["ssid"].as<String>().length() < 1 || wifi["ssid"].as<String>().length() > 31 || wifi["password"].as<String>().length() > 64)
{
ESP_LOGE(SVK_TAG, "SSID or password length is invalid");
}
else
{
// add the ssid and password to the JSON object
wifi_settings_t wifiSettings;
wifiSettings.ssid = wifi["ssid"].as<String>();
wifiSettings.password = wifi["password"].as<String>();
wifiSettings.staticIPConfig = wifi["static_ip_config"];
// extended settings
JsonUtils::readIP(wifi, "local_ip", wifiSettings.localIP);
JsonUtils::readIP(wifi, "gateway_ip", wifiSettings.gatewayIP);
JsonUtils::readIP(wifi, "subnet_mask", wifiSettings.subnetMask);
JsonUtils::readIP(wifi, "dns_ip_1", wifiSettings.dnsIP1);
JsonUtils::readIP(wifi, "dns_ip_2", wifiSettings.dnsIP2);
// Swap around the dns servers if 2 is populated but 1 is not
if (IPUtils::isNotSet(wifiSettings.dnsIP1) && IPUtils::isSet(wifiSettings.dnsIP2))
{
wifiSettings.dnsIP1 = wifiSettings.dnsIP2;
wifiSettings.dnsIP2 = INADDR_NONE;
}
// Turning off static ip config if we don't meet the minimum requirements
// of ipAddress, gateway and subnet. This may change to static ip only
// as sensible defaults can be assumed for gateway and subnet
if (wifiSettings.staticIPConfig && (IPUtils::isNotSet(wifiSettings.localIP) || IPUtils::isNotSet(wifiSettings.gatewayIP) ||
IPUtils::isNotSet(wifiSettings.subnetMask)))
{
wifiSettings.staticIPConfig = false;
}
// reset scan result
wifiSettings.available = false;
settings.wifiSettings.push_back(wifiSettings);
// increment the wifi network index
i++;
}
}
}
else
{
// populate with factory defaults if they are present
if (String(FACTORY_WIFI_SSID).length() > 0)
{
settings.wifiSettings.push_back(wifi_settings_t{
.ssid = FACTORY_WIFI_SSID,
.password = FACTORY_WIFI_PASSWORD,
.staticIPConfig = false,
.localIP = INADDR_NONE,
.gatewayIP = INADDR_NONE,
.subnetMask = INADDR_NONE,
.dnsIP1 = INADDR_NONE,
.dnsIP2 = INADDR_NONE,
.available = false,
});
}
}
ESP_LOGV(SVK_TAG, "WiFi Settings updated");
return StateUpdateResult::CHANGED;
}
lib/framework/APSettingsService.h, lines around 115-144
static StateUpdateResult update(JsonObject &root, APSettings &settings, const String &originId)
{
APSettings newSettings = {};
newSettings.provisionMode = root["provision_mode"] | FACTORY_AP_PROVISION_MODE;
switch (settings.provisionMode)
{
case AP_MODE_ALWAYS:
case AP_MODE_DISCONNECTED:
case AP_MODE_NEVER:
break;
default:
newSettings.provisionMode = AP_MODE_DISCONNECTED;
}
newSettings.ssid = root["ssid"] | SettingValue::format(FACTORY_AP_SSID);
newSettings.password = root["password"] | FACTORY_AP_PASSWORD;
newSettings.channel = root["channel"] | FACTORY_AP_CHANNEL;
newSettings.ssidHidden = root["ssid_hidden"] | FACTORY_AP_SSID_HIDDEN;
newSettings.maxClients = root["max_clients"] | FACTORY_AP_MAX_CLIENTS;
JsonUtils::readIPStr(root, "local_ip", newSettings.localIP, FACTORY_AP_LOCAL_IP);
JsonUtils::readIPStr(root, "gateway_ip", newSettings.gatewayIP, FACTORY_AP_GATEWAY_IP);
JsonUtils::readIPStr(root, "subnet_mask", newSettings.subnetMask, FACTORY_AP_SUBNET_MASK);
if (newSettings == settings)
{
return StateUpdateResult::UNCHANGED;
}
settings = newSettings;
return StateUpdateResult::CHANGED;
}
lib/framework/APSettingsService.h, lines around 102-113
static void read(APSettings &settings, JsonObject &root)
{
root["provision_mode"] = settings.provisionMode;
root["ssid"] = settings.ssid;
root["password"] = settings.password;
root["channel"] = settings.channel;
root["ssid_hidden"] = settings.ssidHidden;
root["max_clients"] = settings.maxClients;
root["local_ip"] = settings.localIP.toString();
root["gateway_ip"] = settings.gatewayIP.toString();
root["subnet_mask"] = settings.subnetMask.toString();
}
| JsonObject control; // state.data has one or more properties | ||
| control = addControl(controls, "layer", "select"); | ||
| control["default"] = 1; | ||
| control["default"] = 0; // 1 is layer 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ModuleEffects.h" | head -5Repository: MoonModules/MoonLight
Length of output: 104
🏁 Script executed:
cat -n src/MoonLight/Modules/ModuleEffects.h | sed -n '55,75p'Repository: MoonModules/MoonLight
Length of output: 921
🏁 Script executed:
rg "control\[.default" -A 5 -B 5 src/MoonLight/Modules/ModuleEffects.hRepository: MoonModules/MoonLight
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Look for ModuleEffects.h and examine the context
find . -type f -name "ModuleEffects.h" 2>/dev/null
# Also search for usages of control["default"] in the codebase
echo "=== Searching for control[\"default\"] usages ==="
rg 'control\["default"\]' -B 3 -A 3 2>/dev/null | head -50
# Search for where this control value is consumed
echo "=== Searching for layer-related control usage ==="
rg 'control\[' src/MoonLight/Modules/ 2>/dev/null | head -30Repository: MoonModules/MoonLight
Length of output: 6333
🏁 Script executed:
#!/bin/bash
# Find the file
file_path=$(find . -type f -name "ModuleEffects.h" 2>/dev/null)
if [ -n "$file_path" ]; then
echo "Found file: $file_path"
echo "=== Context around line 63 (lines 55-75) ==="
cat -n "$file_path" | sed -n '55,75p'
echo ""
echo "=== Full file for reference ==="
wc -l "$file_path"
fi
# Search for control["default"] usage in the codebase
echo ""
echo "=== Searching for control[\"default\"] usage ==="
rg 'control\["default"\]' -B 3 -A 3 2>/dev/null | head -80Repository: MoonModules/MoonLight
Length of output: 6663
🏁 Script executed:
#!/bin/bash
# Search for where the layer control value is read/used
echo "=== Searching for usage of layer control value ==="
rg 'getOption.*layer|state\["layer"\]|controls\["layer"\]' -B 2 -A 2 src/MoonLight/Modules/ 2>/dev/null | head -100
# Also search for how select controls are processed
echo ""
echo "=== Searching for select control handling ==="
rg 'getOption|state\[' -A 2 -B 2 src/MoonLight/Modules/ModuleEffects.h 2>/dev/null | head -80Repository: MoonModules/MoonLight
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Check git history for this file to see what the default was before
cd ./src/MoonLight/Modules/ && \
git log -p --follow ModuleEffects.h 2>/dev/null | grep -A 5 -B 5 'control\["default"\]' | head -100Repository: MoonModules/MoonLight
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Try to find where layer control value is actually used
echo "=== Searching in ModuleEffects.h for layer control usage ==="
grep -n "layer\|state\|getOption" ./src/MoonLight/Modules/ModuleEffects.h | head -50
# Check entire ModuleEffects.h to understand context better
echo ""
echo "=== Full ModuleEffects.h ==="
cat ./src/MoonLight/Modules/ModuleEffects.hRepository: MoonModules/MoonLight
Length of output: 13981
🏁 Script executed:
#!/bin/bash
# Search for where state["layer"] is read
echo "=== Searching for state or controls usage of layer value ==="
rg 'state\["layer"\]|controls\["layer"\]|getOption.*layer' -B 3 -A 3 --type cpp --type h 2>/dev/null | head -80
# Check if there's validation or special handling for value 0
echo ""
echo "=== Searching for layer index handling ==="
rg 'layerP\.layers\[|layer.*index|layer.*value.*0' src/MoonLight/Modules/ModuleEffects.h -B 2 -A 2 2>/dev/nullRepository: MoonModules/MoonLight
Length of output: 1075
🏁 Script executed:
#!/bin/bash
# Search for addControlValue implementation
rg -A 10 'void addControlValue.*\(' MoonBase/ 2>/dev/null | head -80Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for addControlValue implementation and how select controls work
echo "=== Searching for addControlValue implementation ==="
rg -B 5 -A 15 'addControlValue' MoonBase/Nodes.h 2>/dev/null | head -150
# Also search for how Module handles select controls and defaults
echo ""
echo "=== Searching for select control handling ==="
rg 'select.*default|default.*select' MoonBase/ -B 2 -A 2 2>/dev/null | head -60Repository: MoonModules/MoonLight
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Search more broadly for addControlValue
echo "=== Searching for addControlValue in all files ==="
rg 'addControlValue' --type cpp --type h -B 2 -A 8 2>/dev/null | head -200
# Also check Module base class
echo ""
echo "=== Searching in Module.h/Module.cpp ==="
find . -name "Module.h" -o -name "Module.cpp" | xargs cat 2>/dev/null | grep -A 20 "addControlValue" | head -80Repository: MoonModules/MoonLight
Length of output: 15172
🏁 Script executed:
#!/bin/bash
# Compare with the D_ArtnetIn pattern which seems to handle this correctly
echo "=== Checking D_ArtnetIn pattern (similar layer selection) ==="
rg -B 5 -A 12 'view.*select' src/MoonLight/Nodes/Drivers/D_ArtnetIn.h 2>/dev/null
# Also check if there's version control history showing the change from 1 to 0
echo ""
echo "=== Checking git blame for line 63 ==="
cd ./src/MoonLight/Modules/ && git blame ModuleEffects.h 2>/dev/null | grep -A 2 -B 2 'default.*0.*layer'Repository: MoonModules/MoonLight
Length of output: 1308
Fix layer control default value mismatch.
The "layer" select control has default = 0, but the dropdown only contains values 1, 2, 3, etc. (from the loop starting at i = 1). This creates an invalid default that doesn't match any available option.
Either change the default to 1 (first layer), or add a special option like "Physical layer" at index 0 to match the D_ArtnetIn driver pattern. Also clarify the misleading comment "// 1 is layer 0".
🤖 Prompt for AI Agents
In src/MoonLight/Modules/ModuleEffects.h around line 63, the "layer" select
control currently sets control["default"] = 0 which doesn't match the options
generated starting at i = 1; change the default to control["default"] = 1 to
select the first valid layer, and update the misleading comment to something
like "// default to layer 1 (first layer)" so the default and comment reflect
available options; alternatively, if you need a special physical layer at index
0, add an explicit option for index 0 before the loop instead of leaving the
default at 0.
| if (safeModeMB) { | ||
| EXT_LOGW(ML_TAG, "Safe mode enabled, not adding FastLED driver"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP '\bsafeModeMB\b' -C3 --type=cpp --type=hRepository: MoonModules/MoonLight
Length of output: 5840
🏁 Script executed:
head -50 src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 1543
🏁 Script executed:
rg '#include' src/MoonLight/Nodes/Drivers/D_FastLED.h -A2 -B2Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
grep -n '#include' src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
wc -l src/MoonLight/Nodes/Drivers/D_FastLED.hRepository: MoonModules/MoonLight
Length of output: 108
🏁 Script executed:
cat -n src/MoonLight/Nodes/Drivers/D_FastLED.h | head -100Repository: MoonModules/MoonLight
Length of output: 4194
🏁 Script executed:
sed -n '1,150p' src/MoonLight/Nodes/Drivers/D_FastLED.h | grep -i includeRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
cat -n src/MoonLight/Nodes/Drivers/D_FastLED.h | grep -i includeRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
rg '#include.*D_FastLED' -C2Repository: MoonModules/MoonLight
Length of output: 417
🏁 Script executed:
grep -n '#include.*ESP32SvelteKit' src/MoonBase/Nodes.hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
cat -n src/MoonBase/Nodes.h | head -50Repository: MoonModules/MoonLight
Length of output: 2301
🏁 Script executed:
grep -n '#include.*ESP32SvelteKit' src/MoonLight/Layers/PhysicalLayer.h src/MoonLight/Layers/VirtualLayer.hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
cat -n src/MoonLight/Layers/VirtualLayer.h | head -30Repository: MoonModules/MoonLight
Length of output: 1080
🏁 Script executed:
cat -n src/MoonLight/Layers/PhysicalLayer.h | head -40Repository: MoonModules/MoonLight
Length of output: 2002
🏁 Script executed:
grep -n '#include' src/MoonBase/Nodes.h | head -30Repository: MoonModules/MoonLight
Length of output: 1458
🏁 Script executed:
cat -n src/MoonLight/Layers/PhysicalLayer.h | grep -i includeRepository: MoonModules/MoonLight
Length of output: 228
Add explicit #include <ESP32SvelteKit.h> to D_FastLED.h.
The safeModeMB variable is properly defined and accessible through the extern declaration in ESP32SvelteKit.h. However, D_FastLED.h should explicitly include this header to match the pattern used in D_ParallelLEDDriver.h and clarify the dependency. Add #include <ESP32SvelteKit.h> // for safeModeMB and restartNeeded near the top of the file, after the pragma guards and conditional compilation blocks.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_FastLED.h around lines 98 to 101, the file uses
safeModeMB (and restartNeeded elsewhere) but does not explicitly include the
header that declares them; add an explicit include directive for
ESP32SvelteKit.h near the top of the file (immediately after the pragma guards
and any conditional compilation blocks) with a comment like "// for safeModeMB
and restartNeeded" so the dependency matches D_ParallelLEDDriver.h and makes the
symbols' origin explicit.
Back-End ======= - IO module: SE16: Switch default to Infrared - NodeManager: reverse migration code - Art-Net Out: supporting multiple IP numbers
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
428-442: Fix RS485 pin assignments - all 4 pins assigned to TX.Lines 431-434 assign pins 3, 4, 22, and 24 all to
pin_RS485_TX. RS485 is a serial communication protocol that requires both TX and RX pins. Having 4 TX pins without any RX pins will break RS485 functionality.Please verify the correct pin configuration for this board. Typically, RS485 uses:
- 1 pin for
pin_RS485_TX- 1 pin for
pin_RS485_RX- Optionally, additional pins for DE (Driver Enable) or RE (Receiver Enable)
Apply this diff if the typical 2-pin RS485 configuration is intended (adjust pin numbers as needed based on hardware documentation):
- pinAssigner.assignPin(3, pin_RS485_TX); - pinAssigner.assignPin(4, pin_RS485_TX); - pinAssigner.assignPin(22, pin_RS485_TX); - pinAssigner.assignPin(24, pin_RS485_TX); + pinAssigner.assignPin(3, pin_RS485_TX); + pinAssigner.assignPin(4, pin_RS485_RX);Or, if all 4 pins have distinct purposes, please update to reflect the actual hardware design.
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
98-101: Past review comment still applies: missing explicit include forsafeModeMB.The code now uses
safeModeMBbut the file still lacks an explicit#include <ESP32SvelteKit.h>. While the symbol may be accessible through transitive includes, explicit inclusion improves clarity and matches the pattern in D_ParallelLEDDriver.h.Based on past review comments.
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
109-126: Consider capping status display at the FastLED RMT limit (4 pins).The status string loop (line 109) iterates over all
nrOfPins, but the actual configuration loop (line 126) is capped at 4 pins due to FastLED RMT constraints. This means the status might display assignments for 8 pins while only 4 are actually configured, potentially confusing users.Consider applying the same
&& i < 4constraint to the status-building loop, or add a note in the status string when pins beyond the 4th are ignored.src/MoonBase/NodeManager.h (1)
139-139: Clarify the migration comment.The comment uses double negatives and unclear terminology. Consider rephrasing for clarity:
Suggested revision:
- // to consider: not needed as we accept data updates, we only do not accept data definition updates! + // Migration: Accept data updates from legacy driver names, but don't update the data definitionsOr provide more context about what "data updates" vs "data definition updates" means in this scenario.
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
133-133: Redundant IP assignment in loop.Line 133 reassigns
controllerIP[3]on every iteration, butactualIPIndexonly changes at line 164 when switching to the next controller. This assignment should only be needed whenactualIPIndexchanges.Consider moving this assignment to only execute when the IP actually changes:
// send all the leds to artnet for (int indexP = 0; indexP < header->nrOfLights; indexP++) { - controllerIP[3] = ipAddresses[actualIPIndex]; - // fill a packageAnd keep the assignment only at line 167 where it's actually updated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/moonbase/inputoutput.md(1 hunks)docs/moonlight/drivers.md(1 hunks)src/MoonBase/Modules/ModuleIO.h(3 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(4 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(4 hunks)src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MoonLight/Modules/ModuleEffects.h
- src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Drivers/D_FastLED.h (3)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
setup(38-46)src/MoonBase/Nodes.cpp (6)
setup(140-204)setup(140-140)setup(347-363)setup(347-347)updateControl(17-66)updateControl(17-17)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[grammar] ~92-~92: Ensure spelling is correct
Context: ...../../gettingstarted/installer/) * Set Switch1 the same as you set the jumper on the b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/MoonBase/Modules/ModuleIO.h (1)
292-300: LGTM! Comment clarification improves readability.The updated comments clearly document the switch1 behavior: Ethernet configuration when enabled, Infrared (default) when disabled. The logic is correct and well-documented.
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
133-133: LGTM!The switch statement correctly uses
pins[pinIndex]to reference the mapped pin value, ensuring consistency with the new pin mapping logic introduced in lines 104-114.
554-559: LGTM! Addresses past review comment.The default case now correctly uses
pins[pinIndex]for logging, ensuring the reported pin number matches the value checked in the switch statement. The updated comments also reflect this consistency.Based on past review comments.
src/MoonBase/NodeManager.h (1)
22-22: LGTM!Widening
defaultNodeNamefromChar<20>toChar<32>provides more flexibility for longer node names and aligns with similar buffer expansions elsewhere in the PR (e.g.,controllerIP3sin D_ArtnetOut.h).src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (2)
30-30: LGTM! Multi-IP support properly configured.The changes correctly introduce multi-IP Art-Net support:
controllerIP3sfield holds comma-separated IP segments (Char<32>)nrOfOutputstracks outputs per controller- UI controls bind to these fields with appropriate constraints
Note: The control name "controllerIPs" differs from the field name "controllerIP3s". This appears intentional for UI clarity but consider documenting this mapping if it's not obvious to future maintainers.
Also applies to: 35-35, 40-40, 45-45
162-168: LGTM! IP cycling logic is correct.The logic correctly:
- Tracks outputs sent per IP with
processedOutputs- Switches to the next IP after
nrOfOutputsoutputs are processed- Validates that the next IP exists before advancing
- Resets universe counters for the new controller
docs/moonbase/inputoutput.md (1)
92-92: LGTM! Documentation correction for SE16 v1 default.The change correctly clarifies that the SE16 v1 board defaults to Infrared (off position) rather than Ethernet. This aligns the documentation with the actual board behavior.
| Char<32> statusString = "#"; | ||
| statusString += nrOfPins; | ||
| statusString += ": "; | ||
| for (int i = 0; i < nrOfPins; i++) { | ||
| uint8_t assignedPin = layerP.ledPinsAssigned[i]; | ||
| if (assignedPin < layerP.nrOfLedPins) | ||
| pins[i] = layerP.ledPins[assignedPin]; | ||
| else | ||
| pins[i] = layerP.ledPins[i]; | ||
| EXT_LOGD(ML_TAG, "onLayout pin#%d of %d: assigned:%d %d->%d #%d", i, nrOfPins, assignedPin, layerP.ledPins[i], pins[i], layerP.ledsPerPin[i]); | ||
| Char<12> tmp; | ||
| tmp.format(" %d#%d", pins[i], layerP.ledsPerPin[i]); | ||
| statusString += tmp; | ||
| } | ||
| EXT_LOGD(ML_TAG, "status: %s", statusString.c_str()); |
There was a problem hiding this comment.
Risk of buffer overflow in status string construction.
The statusString buffer is 32 characters, but the concatenation logic can easily exceed this with multiple pins. For example, with 4 pins the string could be "#4: 48#1000 47#1000 46#1000 45#1000" (40 characters), and with 8 pins it could reach 76+ characters. Ensure the Char<32> type either:
- Safely truncates without undefined behavior, or
- Use a larger buffer (e.g., Char<64> or Char<128>)
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_FastLED.h around lines 106-120 the fixed
Char<32> statusString can overflow when concatenating per-pin info; replace it
with a safely sized buffer (e.g., Char<128>) or compute required length before
building the string and ensure concatenation is bounds-checked/truncated (use
the Char API for safe append or snprintf-like formatting that respects
capacity); update code to either allocate a larger Char template or add guards
around each append so you never write past the buffer and log a truncated
indicator if capacity is exceeded.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
98-101: Consider adding explicit#include <ESP32SvelteKit.h>forsafeModeMB.The safe mode guard is a good defensive addition. However, per a previous review comment, this file should explicitly include
ESP32SvelteKit.hto clarify the dependency onsafeModeMB, matching the pattern used inD_ParallelLEDDriver.h.
106-120: Buffer size may be insufficient for status string.Per a previous review,
Char<32>can overflow when displaying 4 pins. Example worst-case:"#4: 48#1000 47#1000 46#1000 45#1000"is ~40 characters. While the comment indicates truncation intent, verify theCharclass safely truncates without undefined behavior, or increase toChar<64>.#!/bin/bash # Check Char class implementation for safe truncation behavior rg -n "class Char" --type=cpp --type=h -A 20 | head -60docs/moonlight/drivers.md (1)
102-102: Fix grammar errors in documentation.The line contains two grammar errors:
- "how much outputs" should be "how many outputs" (outputs are countable)
- "are send" should be "are sent" (past participle needed)
Apply this diff:
-* **Nr of Outputs per IP**: how much outputs does one Art-Net controller have. If all outputs are send, Art-Net will be sent to the next IP number. +* **Nr of Outputs per IP**: how many outputs does one Art-Net controller have. If all outputs are sent, Art-Net will be sent to the next IP number.
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
95-109: Consider increasingstatusStringcapacity or limiting pin entries.The
Char<32>buffer can overflow if many pins are configured. Each pin adds approximately 4–8 characters (" 12#500"), so with 5+ pins the string may silently truncate. While the comment at line 95 acknowledges this, consider either increasing the buffer size (e.g.,Char<64>) or limiting the loop to ensure the status remains readable.docs/moonlight/drivers.md (1)
99-99: Fix list indentation per markdown standards.The line uses 4-space indentation when markdown lists should use 2 spaces for consistency.
Apply this diff:
- * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested) + * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
53-69: Strengthen IP segment parsing validation.While the current implementation adds bounds checking (addressing the previous review comment),
atoi()silently converts invalid input like"abc"to0, which then passes validation as a seemingly valid IP segment. Consider usingstrtol()with explicit error checking to distinguish between actual zeros and parse failures.Apply this diff for more robust validation:
void onUpdate(const Char<20>& oldValue, const JsonObject& control) override { DriverNode::onUpdate(oldValue, control); // !! if (control["name"] == "controllerIPs") { EXT_LOGD(MB_TAG, "IPs: %s", controllerIP3s.c_str()); nrOfIPAddresses = 0; controllerIP3s.split(",", [this](const char* token, uint8_t nr) { - int ipSegment = atoi(token); - if (nrOfIPAddresses < std::size(ipAddresses) && ipSegment >= 0 && ipSegment <= 255) { + char* endPtr; + long ipSegment = strtol(token, &endPtr, 10); + if (nrOfIPAddresses < std::size(ipAddresses) && *endPtr == '\0' && ipSegment >= 0 && ipSegment <= 255) { EXT_LOGD(MB_TAG, "Found IP: %s (%d / %d)", token, nr, nrOfIPAddresses); ipAddresses[nrOfIPAddresses] = ipSegment; nrOfIPAddresses++; } else EXT_LOGW(MB_TAG, "Too many IPs provided (%d) or invalid IP segment: %d ", nrOfIPAddresses, ipSegment); }); } };src/MoonBase/NodeManager.h (1)
138-138: Clarify migration necessity in comment.The comment expresses uncertainty ("Maybe this is not needed"), yet the migration code remains. Consider either:
- Removing "Maybe" if migration is necessary for backward compatibility
- Removing the migration code if it's confirmed unnecessary
- Adding context explaining under what conditions the migration applies
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/moonlight/drivers.md(1 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(4 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(3 hunks)src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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_FastLED.hsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.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/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
setup(38-46)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
src/MoonLight/Nodes/Drivers/parlio.cpp (1)
show_parlio(265-265)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
src/MoonLight/Nodes/Drivers/D_FastLED.h (6)
49-49: LGTM!The new status field follows the same pattern used in
D_ParallelLEDDriver.h, maintaining consistency across drivers.
57-57: LGTM!The status control registration matches the pattern in
D_ParallelLEDDriver.h.
93-94: LGTM!The 4-pin cap correctly enforces FastLED RMT limitations, and the nested
MINguards ensurenrOfPinsnever exceeds any of the configured limits.
122-123: LGTM!Properly updates the status control and triggers a UI refresh.
126-133: LGTM!The refactoring to use the local
pins[]array provides cleaner separation between pin assignment logic (lines 109-114) and the FastLED configuration switch.
554-555: LGTM!The default case now correctly uses
pins[pinIndex], consistent with the switch statement, ensuring the logged pin number matches the value that was actually checked.src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (2)
54-174: LGTM: Refactoring fromlayer->layerPtolayerPis consistent.The replacement of
layer->layerP->with directlayerP.access is applied uniformly across all code paths (ESP32P4, HP_ALL_DRIVERS, ESP32_LEDSDRIVER). The logic flow is preserved, and all member references appear correct.
63-63: Theshow_parliocall assumes uniform LED distribution across all output pins.Line 63 passes
layerP.ledsPerPin[0]as theleds_per_outputparameter toshow_parlio(). Theshow_parlio()implementation (parlio.cpp:266) validates thatlength == outputs * leds_per_output, which enforces that all pins drive the same number of LEDs. While the logging loop at lines 104–109 iterates through all pins' LED counts, the parlio driver architecture requires uniform distribution. If pins are configured with different LED counts,show_parlio()will detect the mismatch via its validation check and return an error.src/MoonBase/NodeManager.h (2)
22-22: Good change - increased buffer size for node names.Expanding from
Char<20>toChar<32>provides more flexibility for longer node names without any breaking changes.
139-148: Thecontains()function is available but consider exact string comparison and clarify migration necessity.The
contains()function is defined inChar.has a custom utility usingstrnstr()for substring matching. However, the developer's own comment suggests uncertainty: "Maybe this is not needed as we accept data updates from legacy driver names, migration only needed for changes in the data definitions." Before investing in this migration logic, clarify whether it's actually necessary. If it is, consider using exact string comparison instead of substring matching to avoid false positives with custom node names that happen to contain these substrings (e.g., "My Physical Driver").
|
@coderabbitai, when using the Curtain RGB2040 light preset, it skips sending LED data from pixel 20..39, 60..79 and so on, but I see some lights flickering in these ranges, especially when I have effects which turn on most lights. Can you do a review of the code and see if in this situation values are written in the layerP.lights.channels array on these positions? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1963 🏁 Script executed: Length of output: 5650 🏁 Script executed: Length of output: 12999 🏁 Script executed: Length of output: 21039 🏁 Script executed: Length of output: 4190 🏁 Script executed: Length of output: 8020 🏁 Script executed: Length of output: 27898 🏁 Script executed: Length of output: 14460 🏁 Script executed: Length of output: 5251 @ewowi, I found the issue! The RGB2040 preset has a bug where certain code paths bypass the skip logic, causing data to be written to the pixels that should be skipped (20-39, 60-79, etc.). Root CauseIn case m_oneLight: {
uint16_t indexP = mappingTable[indexV].indexP;
if (layerP->lights.header.lightPreset == lightPreset_RGB2040) {
indexP += (indexP / 20) * 20; // Skip logic applied HERE
}
memcpy(&layerP->lights.channels[indexP * layerP->lights.header.channelsPerLight + offset], channels, length);
break;
}However, the "no mapping" code path does NOT apply this adjustment: } else if (indexV * layerP->lights.header.channelsPerLight + offset + length < layerP->lights.maxChannels) { // no mapping
memcpy(&layerP->lights.channels[indexV * layerP->lights.header.channelsPerLight + offset], channels, length);
// ^^^ NO RGB2040 adjustment here!
}Additional ProblemsThe optimization paths in if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1) {
fastled_fadeToBlackBy((CRGB*)layerP->lights.channels, layerP->lights.header.nrOfChannels / sizeof(CRGB), fadeBy);
// ^^^ Direct CRGB* cast processes ALL channels including the skip ranges!
}The FixYou need to either:
The flickering occurs because effects that turn on most lights write data to these skip regions through the no-mapping path or optimization paths, and that data leaks to unintended physical LEDs. Would you like me to prepare a fix for this issue? 🐰 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11256 🏁 Script executed: Length of output: 4329 🏁 Script executed: Length of output: 7899 🏁 Script executed: Length of output: 8890 🏁 Script executed: Length of output: 9385 🏁 Script executed: Length of output: 5910 🏁 Script executed: Length of output: 6603 🏁 Script executed: Length of output: 4453 🏁 Script executed: Length of output: 15161 @ewowi, I found the root cause of the flickering issue with the Curtain RGB2040 preset! The ProblemThe RGB2040 preset has special channel mapping that skips pixel ranges 20-39, 60-79, 100-119, etc. This is handled correctly in if (layerP->lights.header.lightPreset == lightPreset_RGB2040) {
indexP += (indexP / 20) * 20; // Skip the 20..39 range, so adjust group mapping
}This adjustment is applied in all The Root CauseLiveScript exposes direct access to the physical channels array, bypassing the VirtualLayer mapping. In addExternal("CRGB* leds", (void*)(CRGB*)layerP.lights.channels);This means when LiveScript code uses
These writes corrupt the "empty" ranges that RGB2040 hardware requires, causing the flickering you're seeing, especially when many lights are turned on. Potential Solutions
Would you like me to help implement one of these solutions? |
|
@coderabbitai, I am not using LiveScripts in this situation so this is not the problem , look further |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16745 🏁 Script executed: Length of output: 78 🏁 Script executed: Length of output: 90 @ewowi, I found it! The flickering is caused by FastLED optimization paths that bypass the RGB2040 skip logic. The Root CauseIn 1. fadeToBlackBy() (line 224)if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1) { // CRGB lights
fastled_fadeToBlackBy((CRGB*)layerP->lights.channels, layerP->lights.header.nrOfChannels / sizeof(CRGB), fadeBy);
}2. fill_solid() (line 266)if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1) { // faster, else manual
fastled_fill_solid((CRGB*)layerP->lights.channels, layerP->lights.header.nrOfChannels / sizeof(CRGB), color);
}3. fill_rainbow() (line 286)if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1) { // faster, else manual
fastled_fill_rainbow((CRGB*)layerP->lights.channels, layerP->lights.header.nrOfChannels / sizeof(CRGB), initialhue, deltahue);
}The ProblemWhen these conditions are true (RGB-only lights with a single layer), the code takes the fast path and processes the entire channels array sequentially, writing to positions 0, 1, 2, ... including the skip ranges (20-39, 60-79, etc.). The RGB2040 skip logic ( The FixAdd an RGB2040 check to the optimization conditions. Change: if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1)To: if (layerP->lights.header.channelsPerLight == 3 && layerP->layers.size() == 1 && layerP->lights.header.lightPreset != lightPreset_RGB2040)This will force the code to use the manual loop path (which calls Would you like me to prepare the complete fix for all three functions? |
|
@coderabbitai, although this was a nice found, this is also not the problem as the skipped LEDs (20..39 etc) are initialized with 0 and I only use fadeToBlack, so they will stay 0, look further |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22972 🏁 Script executed: Length of output: 21321 @ewowi, I found the actual root cause! The bug is in how the drivers read the channel data for RGB2040. The ProblemVirtualLayer WRITES with the skip adjustment: // VirtualLayer.cpp line 139-140
if (layerP->lights.header.lightPreset == lightPreset_RGB2040) {
indexP += (indexP / 20) * 20; // Skip the 20..39 range
}
memcpy(&layerP->lights.channels[indexP * layerP->lights.header.channelsPerLight + offset], ...);But D_ArtnetOut.h READS without it: // D_ArtnetOut.h line 135-140
for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
memcpy(&packet_buffer[packetSize + 18],
&layerP.lights.channels[indexP * header->channelsPerLight], // ← NO adjustment!
header->channelsPerLight);What HappensWhen the driver loops with
The same issue likely exists in D_ParallelLEDDriver.h and D_FastLED.h where they access the channels array. The FixApply the RGB2040 adjustment in the driver loop: // D_ArtnetOut.h around line 135
for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
uint16_t physicalIndexP = indexP;
if (header->lightPreset == lightPreset_RGB2040) {
physicalIndexP += (indexP / 20) * 20; // Apply same skip logic
}
memcpy(&packet_buffer[packetSize + 18],
&layerP.lights.channels[physicalIndexP * header->channelsPerLight],
header->channelsPerLight);
reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB],
&layerP.lights.channels[physicalIndexP * header->channelsPerLight + header->offsetRGB]);
// ... rest of the loop
}Would you like me to search for all locations where this fix needs to be applied across the different drivers? |
|
@coderabbitai, nope, this is not it. They should write the data but the not used channels should have value 0 and therefor should not have lights burning. So what you need to do is prove that indeed these channels have value 0 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10592 🏁 Script executed: Length of output: 173 🏁 Script executed: Length of output: 134 🏁 Script executed: Length of output: 1691 🏁 Script executed: Length of output: 42628 🏁 Script executed: Length of output: 5484 🏁 Script executed: Length of output: 17233 🏁 Script executed: Length of output: 3307 🏁 Script executed: Length of output: 4382 🏁 Script executed: Length of output: 1861 🏁 Script executed: Length of output: 7465 🏁 Script executed: Length of output: 346 @ewowi, I now understand the architecture better. Let me trace through what's happening: Key Finding: The Channel Buffer Layout for RGB2040Looking at line 201 in lights.header.nrOfChannels = lights.header.nrOfLights * lights.header.channelsPerLight * ((lights.header.lightPreset == lightPreset_RGB2040) ? 2 : 1);For RGB2040, the buffer is allocated with 2x the normal size to account for skip ranges. So if you have 80 virtual lights:
The Memory Layout Should Be:
The Driver BehaviorD_ArtnetOut.h (line 138) sends ALL physical positions sequentially: for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
memcpy(&packet_buffer[packetSize + 18], &layerP.lights.channels[indexP * header->channelsPerLight], header->channelsPerLight);At Proving Channels Are ZeroThe channels in skip ranges should be zero because:
However, I notice VirtualLayer only zeros unmapped lights when explicitly unmapped by modifiers (line 357 VirtualLayer.cpp). The skip ranges are never touched by VirtualLayer at all - they rely solely on the initial memset. Hypothesis: reOrderAndDimRGBW CorruptionCould 🏁 Script executed: Length of output: 4378 🏁 Script executed: Length of output: 4486 @ewowi, you're right - I need to prove whether those channels are actually zero. Let me propose adding debug logging to verify this. The critical question is: When the driver reads from the skip ranges (physical positions 20-39, 60-79, etc.), are those channel values actually zero? Diagnostic ApproachAdd temporary logging in for (int indexP = 0; indexP < header->nrOfLights; indexP++) {
// Debug: log skip range values
if ((indexP >= 20 && indexP < 40) || (indexP >= 60 && indexP < 80)) {
uint8_t r = layerP.lights.channels[indexP * header->channelsPerLight + 0];
uint8_t g = layerP.lights.channels[indexP * header->channelsPerLight + 1];
uint8_t b = layerP.lights.channels[indexP * header->channelsPerLight + 2];
if (r != 0 || g != 0 || b != 0) {
EXT_LOGW(ML_TAG, "Skip range non-zero! indexP:%d RGB:%d,%d,%d", indexP, r, g, b);
}
}
memcpy(&packet_buffer[packetSize + 18], &layerP.lights.channels[indexP * header->channelsPerLight], header->channelsPerLight);
// ... rest of loopAlternative: The Real Bug?Actually, I just realized something critical. Look at memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[...], ...);Even if you're not actively using Art-Net In, is it possible that:
Can you check if Art-Net In is enabled or receiving any packets? Or temporarily disable the Art-Net In driver to see if the flickering stops? |
======== - Devices, Art-Net In: Replace WiFiUDP by NetworkUDP - IO Module: swtiches are not changing the modded status, maxPower is (so not reset during boot) - NodeManager, Effects, Drivers: use checkAndAlloc - Art-Net out: fix check on ipAddresses left - WLED effects: Add Ants (not added in effects yet as WIP)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonLight/Modules/ModuleDrivers.h (1)
131-135: Bug:else if (!node)makes LiveScript fallback unreachable.The
elseon line 131 binds to theif (!node)on line 128. Since the condition!nodewas true when line 128 executed (otherwise we wouldn't enter that branch), theelsebranch is never taken—regardless of whethercheckAndAllocreturnednullptr.This means the LiveScript fallback will never execute.
🔎 Proposed fix
// custom if (!node) node = checkAndAlloc<SE16Layout>(name); #if FT_LIVESCRIPT - else if (!node) { + if (!node) { LiveScriptNode* liveScriptNode = allocMBObject<LiveScriptNode>(); liveScriptNode->animation = name; // set the (file)name of the script node = liveScriptNode; } #endifsrc/MoonLight/Modules/ModuleEffects.h (1)
253-257: Bug:else if (!node)makes LiveScript fallback unreachable.Same issue as in
ModuleDrivers.h. Theelseon line 253 binds toif (!node)on line 250, making this branch unreachable. This matches the code pattern inModuleDrivers.h(lines 131-135 from relevant snippets).🔎 Proposed fix
if (!node) node = checkAndAlloc<RippleYZModifier>(name); #if FT_LIVESCRIPT - else if (!node) { + if (!node) { LiveScriptNode* liveScriptNode = allocMBObject<LiveScriptNode>(); liveScriptNode->animation = name; // set the (file)name of the script node = liveScriptNode; } #endif
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
844-850: Dead code: no "bpm" control exists.The
onUpdatemethod checks for a "bpm" control that isn't defined insetup(). This appears to be leftover template code.🔎 Proposed fix
- void onUpdate(const Char<20>& oldValue, const JsonObject& control) { - // add your custom onUpdate code here - if (control["name"] == "bpm") { - if (control["value"] == 0) { - } - } - }Remove the method entirely if no custom update handling is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/moonbase/inputoutput.md(2 hunks)docs/moonlight/drivers.md(2 hunks)platformio.ini(1 hunks)src/MoonBase/Modules/ModuleDevices.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/NodeManager.h(3 hunks)src/MoonLight/Modules/ModuleDrivers.h(1 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(4 hunks)src/MoonLight/Nodes/Effects/E_WLED.h(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/moonbase/inputoutput.md
🧰 Additional context used
🧠 Learnings (3)
📚 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-12-18T15:15:57.808Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.808Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
🧬 Code graph analysis (4)
src/MoonBase/Modules/ModuleIO.h (3)
lib/framework/StatefulService.h (2)
update(123-134)update(144-155)src/MoonBase/Module.cpp (2)
update(271-302)update(271-271)src/MoonBase/Module.h (1)
ModuleState(47-137)
src/MoonLight/Modules/ModuleDrivers.h (1)
src/MoonLight/Modules/ModuleEffects.h (1)
if(253-257)
src/MoonBase/NodeManager.h (2)
src/MoonBase/Nodes.h (2)
Node(66-181)Node(237-237)src/MoonBase/Char.h (1)
equalAZaz09(171-184)
src/MoonLight/Modules/ModuleEffects.h (1)
src/MoonLight/Modules/ModuleDrivers.h (1)
if(131-135)
⏰ 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 (26)
src/MoonBase/Modules/ModuleIO.h (3)
292-300: LGTM!The switch1 handling for board_SE16V1 is well-structured, providing clear differentiation between Ethernet mode (SPI/PHY pins) and the default Infrared mode.
524-547: LGTM — good consolidation of the update pattern.The refactoring cleanly separates state mutation (accumulating changes in
object) from the update call. The conditionalif (object.size())at line 547 ensures the update is only triggered when actual changes are made, avoiding unnecessary state updates.
428-442: Confirm RS-485 pin assignment with hardware designer — all four pins assigned to TX.Lines 431-434 assign all four RS-485 pins to
pin_RS485_TX. Typically RS-485 requires both TX and RX for bidirectional communication. Thepin_RS485_RXenum exists but is unused throughout the codebase. Clarify whether this is intentional (e.g., transmit-only mode) or if some pins should be reassigned to RX for proper bidirectional operation.platformio.ini (1)
59-59: LGTM!Date bump to 20251220 for v0.7.0 preparations.
src/MoonBase/NodeManager.h (2)
22-22: LGTM!Increasing
defaultNodeNamesize from 20 to 32 characters accommodates longer node names, aligning with the 32-characterChartypes used elsewhere in the codebase.
147-157: LGTM! Migration code retained for legacy driver names.The migration code properly handles legacy names ("Physical Driver" →
ParallelLEDDriver, "IR Driver" →IRDriver), addressing the concern from the past review about breaking changes for saved configurations. The updated comment clarifies this is optional data migration vs. mandatory schema migration.docs/moonlight/drivers.md (1)
96-96: LGTM! Documentation updated for multi-IP support.Grammar fixes applied and new "Nr of Outputs per IP" control documented to support the multi-controller Art-Net Out functionality.
Also applies to: 102-102
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (2)
20-20: LGTM! Type change toNetworkUDPfor consistent network abstraction.This aligns with the project-wide migration from
WiFiUDPtoNetworkUDP.
132-136: Pre-existing concern: Direct physical layer writes may bypass RGB2040 skip adjustment.When
view == 0, data is written directly tolayerP.lights.channelsviamemcpy(lines 133, 163). Per the PR comments discussion, this bypasses the RGB2040 skip adjustment thatVirtualLayer::setLightapplies, potentially writing into skipped channel positions. This isn't new to this PR but is worth noting given the ongoing RGB2040 investigation.If RGB2040 light preset is used with Art-Net In configured to write to "Physical layer" (view=0), the skip adjustment won't be applied. Consider whether this path needs the same adjustment logic or if it should be documented as incompatible with RGB2040.
Also applies to: 162-166
src/MoonLight/Modules/ModuleDrivers.h (1)
108-128: LGTM! Clean refactor tocheckAndAllocpattern.The sequential
if (!node)pattern is cleaner than a long else-if chain, reduces code duplication, and centralizes the allocation logic.src/MoonLight/Modules/ModuleEffects.h (1)
172-250: LGTM! Clean refactor tocheckAndAllocpattern.The sequential
if (!node)pattern improves maintainability and reduces repetitive conditional logic across all effect categories.src/MoonBase/Modules/ModuleDevices.h (1)
27-27: LGTM! Type change toNetworkUDPfor better network abstraction.This aligns with the broader refactor across the PR to use
NetworkUDPinstead ofWiFiUDP, enabling support for both WiFi and Ethernet connections. All methods used (begin,parsePacket,read,beginPacket,write,endPacket,remoteIP) are part of the standard Arduino UDP API and fully supported byNetworkUDP.src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (3)
53-69: LGTM: IP parsing properly validated.The IP parsing implementation now includes proper bounds checking, input validation, and error logging. The previous review concerns have been addressed.
107-116: LGTM: Safe IP initialization with proper gating.The gating check at line 109 ensures
nrOfIPAddresses > 0before accessingipAddresses[actualIPIndex]at line 114, preventing invalid IP access.
166-172: LGTM: Per-IP cycling logic correctly implemented.The per-IP cycling logic properly:
- Tracks outputs per IP with
processedOutputscounter (initialized at line 102)- Validates bounds with
actualIPIndex + 1 < nrOfIPAddressescheck- Resets state (
processedOutputs,universe) when advancing to next IP- Updates
controllerIP[3]with the next IP addressThe previous concern about uninitialized memory access has been resolved by using the tracked count instead of sentinel checks.
src/MoonLight/Nodes/Effects/E_WLED.h (11)
81-92: LGTM!Palette access refactored correctly to use
layerP.palette.
161-171: LGTM!MIN macro and palette access changes are correct and consistent with the refactor.
224-226: LGTM!Gamma8 method calls correctly refactored to use
layerP.gamma8().
413-413: LGTM!Palette access refactored correctly.
455-455: LGTM!Palette access refactored correctly.
478-478: LGTM!Palette access refactored correctly.
512-512: LGTM!Palette access refactored correctly.
784-804: LGTM!Static helper functions
getFoodColorandhandleBoundaryare well-structured and correctly implement the ant boundary and food logic.
1168-1168: LGTM!Palette access refactored correctly.
1212-1215: LGTM!MIN macro usage with proper type cast and palette access refactored correctly.
1533-1533: LGTM!MIN macro usage is correct.
| for (int indexP = 0; indexP < header->nrOfLights; indexP++) { | ||
| // fill a package | ||
| memcpy(&packet_buffer[packetSize + 18], &layer->layerP->lights.channels[indexP * header->channelsPerLight], header->channelsPerLight); // set all the channels | ||
| memcpy(&packet_buffer[packetSize + 18], &layerP.lights.channels[indexP * header->channelsPerLight], header->channelsPerLight); // set all the channels | ||
|
|
||
| // correct the RGB channels for color order and brightness | ||
| reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB], &layer->layerP->lights.channels[indexP * header->channelsPerLight + header->offsetRGB]); | ||
| reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB], &layerP.lights.channels[indexP * header->channelsPerLight + header->offsetRGB]); | ||
|
|
||
| if (header->offsetRGB1 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB1], &layer->layerP->lights.channels[indexP * header->channelsPerLight + header->offsetRGB1]); | ||
| if (header->offsetRGB2 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB2], &layer->layerP->lights.channels[indexP * header->channelsPerLight + header->offsetRGB2]); | ||
| if (header->offsetRGB3 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB3], &layer->layerP->lights.channels[indexP * header->channelsPerLight + header->offsetRGB3]); | ||
| if (header->offsetRGB1 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB1], &layerP.lights.channels[indexP * header->channelsPerLight + header->offsetRGB1]); | ||
| if (header->offsetRGB2 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB2], &layerP.lights.channels[indexP * header->channelsPerLight + header->offsetRGB2]); | ||
| if (header->offsetRGB3 != UINT8_MAX) reOrderAndDimRGBW(&packet_buffer[packetSize + 18 + header->offsetRGB3], &layerP.lights.channels[indexP * header->channelsPerLight + header->offsetRGB3]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for RGB2040 lightPreset value and skip adjustment logic
echo "=== Searching for RGB2040 or lightPreset definitions ==="
rg -n "RGB2040|lightPreset.*9|preset.*RGB" --type=cpp --type=h -i
echo -e "\n=== Searching for skip adjustment logic in VirtualLayer ==="
rg -n "skip.*adjust|skipRange|nrOfChannels.*\*.*2" src/MoonLight/Nodes/VirtualLayer.cpp --type=cpp -A5 -B2
echo -e "\n=== Checking D_ArtnetOut.h for skip handling ==="
rg -n "skip|channelOffset" src/MoonLight/Nodes/Drivers/D_ArtnetOut.h --type=h -A3 -B1
echo -e "\n=== Searching for header structure definition (lightPreset) ==="
fd "\.h$" | xargs rg -l "lightPreset" | head -5Repository: MoonModules/MoonLight
Length of output: 3343
🏁 Script executed:
#!/bin/bash
echo "=== VirtualLayer.cpp RGB2040 skip adjustment logic ==="
sed -n '130,190p' src/MoonLight/Layers/VirtualLayer.cpp
echo -e "\n=== PhysicalLayer.cpp RGB2040 doubling logic ==="
sed -n '180,210p' src/MoonLight/Layers/PhysicalLayer.cpp
echo -e "\n=== D_ArtnetOut.h lines 130-160 (around the loop) ==="
sed -n '130,160p' src/MoonLight/Nodes/Drivers/D_ArtnetOut.h
echo -e "\n=== D_ArtnetOut.h full context around channel reading ==="
sed -n '135,155p' src/MoonLight/Nodes/Drivers/D_ArtnetOut.hRepository: MoonModules/MoonLight
Length of output: 8636
🏁 Script executed:
#!/bin/bash
echo "=== Checking lightPreset enum definitions in Nodes.h ==="
sed -n '20,36p' src/MoonBase/Nodes.h
echo -e "\n=== Verifying lightPreset_RGB2040 is NOT checked in D_ArtnetOut ==="
grep -n "lightPreset_RGB2040\|lightPreset == " src/MoonLight/Nodes/Drivers/D_ArtnetOut.hRepository: MoonModules/MoonLight
Length of output: 1063
Apply skip adjustment to RGB2040 channel reads in Art-Net output.
The RGB2040 preset uses a doubled buffer with skip regions (pixels 20–39, 60–79, etc. are skipped), similar to VirtualLayer's write logic. Currently, D_ArtnetOut reads channels sequentially (indexP * header->channelsPerLight) without adjusting for these skip regions, transmitting skip-region bytes to Art-Net controllers.
Apply the same skip adjustment used in VirtualLayer:
uint16_t adjustedIndexP = indexP;
if (header->lightPreset == lightPreset_RGB2040) {
adjustedIndexP += (indexP / 20) * 20;
}
memcpy(&packet_buffer[packetSize + 18], &layerP.lights.channels[adjustedIndexP * header->channelsPerLight], header->channelsPerLight);This matches the pattern at VirtualLayer.cpp lines 139, 149 to ensure consistent skip handling across read and write operations.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_ArtnetOut.h around lines 138 to 147, the code
reads light channels sequentially using indexP * header->channelsPerLight and
thus transmits bytes from skipped regions for the RGB2040 preset; change the
read index to use an adjustedIndexP that adds (indexP / 20) * 20 when
header->lightPreset == lightPreset_RGB2040, and use adjustedIndexP *
header->channelsPerLight for the memcpy and the subsequent reOrderAndDimRGBW
calls so the Art-Net output skips the same regions VirtualLayer already skips.
src/MoonLight/Nodes/Effects/E_WLED.h
Outdated
| void onSizeChanged(const Coord3D& prevSize) override { | ||
| Ant* newAlloc = reallocMB<Ant>(ants, MAX_ANTS); | ||
| if (newAlloc) { | ||
| ants = newAlloc; | ||
| // nrOfAnts = MAX_ANTS; | ||
| } else { | ||
| EXT_LOGE(ML_TAG, "(re)allocate ants failed"); // keep old (if existed) | ||
| } | ||
| } | ||
|
|
||
| ~AntEffect() override { freeMB(ants); }; | ||
|
|
||
| void onUpdate(const Char<20>& oldValue, const JsonObject& control) { | ||
| // add your custom onUpdate code here | ||
| if (control["name"] == "bpm") { | ||
| if (control["value"] == 0) { | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Helper function to calculate ant color | ||
| CRGB getAntColor(int antIndex, int numAnts, bool usePalette) { | ||
| // if (usePalette) | ||
| return ColorFromPalette(layerP.palette, antIndex * 255 / numAnts); | ||
| // Alternate between two colors for default palette | ||
| // return (antIndex % 3 == 1) ? SEGCOLOR(0) : SEGCOLOR(2); | ||
| } | ||
|
|
||
| // Helper function to render a single ant pixel with food handling | ||
| void renderAntPixel(int pixelIndex, int pixelOffset, int antSize, const Ant& ant, CRGB antColor, CRGB backgroundColor, bool gatherFood) { | ||
| bool isMovingBackward = (ant.velocity < 0); | ||
| bool isFoodPixel = gatherFood && ant.hasFood && ((isMovingBackward && pixelOffset == 0) || (!isMovingBackward && pixelOffset == antSize - 1)); | ||
| if (isFoodPixel) { | ||
| layer->setRGB(pixelIndex, getFoodColor(antColor, backgroundColor)); | ||
| } else { | ||
| layer->setRGB(pixelIndex, antColor); | ||
| } | ||
| } | ||
|
|
||
| // Initialize ants on first call | ||
| void initAnts() { | ||
| int confusedAntIndex = random(0, numAnts); // the first random ant to go backwards | ||
|
|
||
| for (int i = 0; i < MAX_ANTS; i++) { | ||
| ants[i].lastBumpUpdate = millis(); | ||
|
|
||
| // Random velocity | ||
| float velocity = VELOCITY_MIN + (VELOCITY_MAX - VELOCITY_MIN) * random16(1000, 5000) / 5000.0f; | ||
| // One random ant moves in opposite direction | ||
| ants[i].velocity = (i == confusedAntIndex) ? -velocity : velocity; | ||
| // Random starting position (0.0 to 1.0) | ||
| ants[i].position = random16(0, 10000) / 10000.0f; | ||
| // Ants don't have food yet | ||
| ants[i].hasFood = false; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
initAnts() is never called — ants remain uninitialized.
The initAnts() function at lines 872-888 properly initializes ant positions, velocities, and state, but it's never invoked. onSizeChanged() only allocates memory without calling initAnts(), leaving all ant members with garbage values. This causes undefined behavior when loop() accesses ants[i].lastBumpUpdate, ants[i].position, etc.
Additionally, initAnts() uses numAnts (line 873) which is only set in loop(), so if called from onSizeChanged(), it would use an uninitialized value.
🔎 Proposed fix
+ void initAnts() {
+ int confusedAntIndex = random(0, MAX_ANTS); // use MAX_ANTS since numAnts not yet set
+
+ for (int i = 0; i < MAX_ANTS; i++) {
ants[i].lastBumpUpdate = millis();
// ... rest of initialization
}
}
void onSizeChanged(const Coord3D& prevSize) override {
Ant* newAlloc = reallocMB<Ant>(ants, MAX_ANTS);
if (newAlloc) {
ants = newAlloc;
+ initAnts(); // Initialize ants after allocation
} else {
EXT_LOGE(ML_TAG, "(re)allocate ants failed");
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around lines 832-888, initAnts() is
never invoked after realloc so ants stay uninitialized and initAnts() also reads
numAnts which may be unused/unset; fix by initializing numAnts before calling
initAnts and call initAnts immediately after a successful realloc in
onSizeChanged (or set numAnts = MAX_ANTS there), ensuring initAnts uses a valid
numAnts (or change it to iterate to MAX_ANTS) so all Ant fields (position,
velocity, lastBumpUpdate, hasFood) are properly initialized before loop()
accesses them.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/moonlight/drivers.md (1)
107-107: Fix recurring typo: "ouput" → "output".The word "ouput" appears multiple times in the document (lines 107, 123, 133) and should be "output". Fixing these will improve consistency and readability.
🔎 Proposed fixes
-* **Channels per ouput**: each output can drive a maximum number of channels, determined by the universes per output +* **Channels per output**: each output can drive a maximum number of channels, determined by the universes per output-* **Channels per ouput**: 512 channels +* **Channels per output**: 512 channels-* **Channels per ouput**: max 8 * 512. +* **Channels per output**: max 8 * 512.Also applies to: 123-123, 133-133
♻️ Duplicate comments (1)
src/MoonLight/Modules/ModuleEffects.h (1)
63-68: Layer control default value still doesn't match available options.The
control["default"] = 0creates an invalid default because the loop populates options starting ati = 1(values 1, 2, 3, etc.). This means the UI will either show no selection or an invalid state initially.Change the default to
1to select the first valid layer, or explicitly add a layer option at index 0 before the loop if a special "physical layer" choice is needed.
🧹 Nitpick comments (1)
src/MoonLight/Modules/ModuleEffects.h (1)
172-258: Excellent refactor: Simplified allocation logic using template pattern.The replacement of long if-else chains with the
checkAndAlloc<T>(name)pattern significantly reduces cyclomatic complexity while maintaining identical behavior. The sequential guard pattern (if (!node) node = checkAndAlloc<T>(name)) is clean, the original order is preserved, and conditional compilation blocks are correctly maintained.The LiveScript fallback allocation (line 253) properly triggers only when no prior type matched, preserving the original logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/moonlight/drivers.md(2 hunks)src/MoonBase/NodeManager.h(3 hunks)src/MoonLight/Modules/ModuleDrivers.h(1 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Nodes/Effects/E_WLED.h(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Modules/ModuleDrivers.h
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T15:15:57.808Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.808Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:30:02.449Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:30:02.449Z
Learning: In DripEffect (E_WLED.h), the bounce at line 1669 uses heavy damping (`vel = -vel / 4`), which makes the bounce too weak to be visible. Reducing the damping factor from 4 to 2 (or lower) makes the bounce more pronounced and visible.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
docs/moonlight/drivers.md (2)
76-76: ✅ Grammar fix addressed. The port description has been corrected from previous review feedback.
96-96: ✅ Grammar fixes addressed. The Controller IPs description has been corrected from previous review feedback with proper grammar ("send" instead of "sent", "more than" instead of "more then").src/MoonBase/NodeManager.h (3)
22-22: LGTM: Buffer size increase accommodates longer node names.Increasing
defaultNodeNamefromChar<20>toChar<32>provides adequate space for longer driver names like "Parallel LED Driver" plus any tags.
82-89: LGTM: Clean allocation helper pattern.The
checkAndAlloc<T>()template method provides a unified allocation pattern that reduces code duplication across node managers. The format string issue from the previous review (using%dwithconst char*) has been correctly fixed to%s.
147-157: LGTM: Migration code preserves backward compatibility.The migration logic properly handles legacy driver names ("Physical Driver" and "IR Driver"), preventing configuration breakage. The approach uses
contains()for flexible matching and logs the migration for visibility.src/MoonLight/Nodes/Effects/E_WLED.h (5)
90-90: Palette access refactor looks consistent.The migration from pointer-based
layer->layerP->paletteto directlayerP.paletteaccess is applied uniformly across allColorFromPalettecalls in this file. This aligns with the broader refactoring mentioned in the PR.
771-804: Well-structured helper functions.The
Antstruct and helper functions (getFoodColor,handleBoundary) are cleanly separated and handle both bounce and wrap boundary modes correctly.
853-868: Initialization logic is sound.The loop correctly initializes all
MAX_ANTSentries with proper velocity bounds (VELOCITY_MINtoVELOCITY_MAX), normalized positions (0.0–1.0), and timestamps. The velocity/position calculations are well-formed.
884-953: Main loop implementation is correct with proper bounds checking.The ant update loop correctly:
- Bounds-checks pixel positions before rendering (line 946)
- Handles boundary conditions via
handleBoundary()- Clamps positions to valid range (line 937)
- Uses
fabsf()to avoid division by near-zero in collision detection (line 904)The collision detection physics at lines 906-932 is complex but mathematically sound for elastic collisions.
224-226: Gamma correction and MIN macro usage are correct.The
layerP.gamma8()calls are consistent with the direct member access pattern, and theMINmacro usages throughout the file have proper type consistency.
| void setup() override { | ||
| addControl(antSpeed, "antSpeed", "slider"); | ||
| addControl(nrOfAnts, "nrOfAnts", "slider", 1, MAX_ANTS); | ||
| addControl(antSizeControl, "antSize", "slider", 1, 20); | ||
| addControl(blur, "blur", "slider"); | ||
| addControl(gatherFood, "gatherFood", "checkbox"); | ||
| addControl(passByControl, "passBy", "checkbox"); | ||
|
|
||
| initAnts(); | ||
| } |
There was a problem hiding this comment.
Previous issues addressed; minor initialization order concern remains.
Good fixes:
antSizeControl = 8resolves the integer division issueinitAnts()is now called fromsetup()- Fixed-size array eliminates null pointer concerns
However, initAnts() uses numAnts (line 854) which is only computed in loop() (line 874). Since numAnts is uninitialized when setup() runs, random(0, numAnts) produces undefined behavior. This is low-severity since the loop initializes all MAX_ANTS entries regardless, but the confusedAntIndex selection is non-deterministic.
🔎 Proposed fix
void setup() override {
addControl(antSpeed, "antSpeed", "slider");
addControl(nrOfAnts, "nrOfAnts", "slider", 1, MAX_ANTS);
addControl(antSizeControl, "antSize", "slider", 1, 20);
addControl(blur, "blur", "slider");
addControl(gatherFood, "gatherFood", "checkbox");
addControl(passByControl, "passBy", "checkbox");
+ numAnts = MAX_ANTS; // Initialize before calling initAnts
initAnts();
}🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around lines 820-829, initAnts() is
called from setup() before numAnts is initialized (it’s only computed later in
loop()), causing random(0, numAnts) to be undefined; fix by initializing numAnts
from the control value before calling initAnts (e.g., compute/assign numAnts =
clamp(controlValue, 1, MAX_ANTS) or call the existing function that updates
numAnts), or change initAnts() to derive the count from the stable control value
directly and ensure confusedAntIndex is chosen using a valid range (1..numAnts
or 0..numAnts-1 as intended).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Modules/ModuleDevices.h (1)
119-129: Buffer over-read if packet is smaller than expected.If
packetSize < 6, accessingbuffer + 6at lines 125 and 127 reads beyond the buffer bounds. TheUDPMessagestruct expects at least 6 bytes of prefix plus the name. Add a size validation before processing:🔎 Proposed fix
void readUDP() { size_t packetSize = deviceUDP.parsePacket(); - if (packetSize > 0) { + if (packetSize >= sizeof(UDPMessage)) { char buffer[packetSize]; deviceUDP.read(buffer, packetSize); // deviceUDP.clear(); EXT_LOGD(ML_TAG, "UDP packet read from %d: %s (%d)", deviceUDP.remoteIP()[3], buffer + 6, packetSize); updateDevices(buffer + 6, deviceUDP.remoteIP()); } }
🧹 Nitpick comments (2)
interface/src/lib/components/moonbase/FieldRenderer.svelte (1)
38-38: Consider removing commented code or applying consistent logging cleanup.The commented-out log is appropriate for release preparation, but commented code can clutter the codebase over time. Consider either removing it entirely or introducing a debug flag to conditionally enable logging throughout the component (other console.log statements remain active at lines 21, 75, 285, 297, and 332).
🔎 Option 1: Remove the commented line entirely
- onDestroy(() => { - // console.log('FieldRenderer clearing interval'); + onDestroy(() => { clearInterval(interval); });src/MoonBase/Modules/ModuleDevices.h (1)
77-116: Well-structured refactor using local copy pattern.The approach of copying state to a local document, manipulating it, then filtering/sorting before pushing updates is clean and avoids race conditions. The 1-hour timeout filter for stale devices and alphabetical sorting improve usability.
One minor note: The variable name
deviceis reused in the loop at line 103 (shadowing the earlierdeviceat line 81), which is technically fine but could be slightly clearer with a different name likedev:for (JsonObject dev : devices) { if (time(nullptr) - dev["time"].as<time_t>() < 3600) devicesVector.push_back(dev); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
interface/src/lib/components/moonbase/FieldRenderer.svelte(1 hunks)lib/framework/EventEndpoint.h(0 hunks)src/MoonBase/Modules/ModuleDevices.h(4 hunks)src/MoonBase/SharedEventEndpoint.h(0 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(1 hunks)
💤 Files with no reviewable changes (2)
- src/MoonBase/SharedEventEndpoint.h
- lib/framework/EventEndpoint.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Modules/ModuleDevices.h (2)
src/MoonBase/SharedWebSocketServer.h (1)
begin(39-85)src/MoonBase/Module.h (1)
ModuleState(47-137)
⏰ 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 (5)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
571-573: Significant behavioral change in decay rate—confirm these tuning values are intentional.The
decreaserchange from 255 to 18 increases the decay rate by approximately 14× faster (line 622 uses it as a divisor:(bandSpeed * deltaMs) / (decreaser * 10)). This will cause the saw-wave animations to fade much more quickly when audio input stops.The other changes are more modest:
increaser: 255 → 211 reduces acceleration by ~17%bpmMax: 115 → 198 increases maximum animation speed by ~72%If these values were tuned based on user testing or visual preferences, the changes look correct.
Could you confirm these specific values (especially the drastically reduced
decreaser) were intentionally chosen based on testing the effect's behavior?src/MoonBase/Modules/ModuleDevices.h (4)
27-27: VerifyNetworkUDPinterface compatibility.The type change from
WiFiUDPtoNetworkUDPaligns with the broader refactor noted in the AI summary. EnsureNetworkUDPprovides the same interface (begin,parsePacket,read,remoteIP,beginPacket,write,endPacket).
40-40: LGTM!Making the devices control read-only (
"r") is appropriate since devices are auto-discovered via UDP rather than user-managed. The array initialization at line 63 ensures the devices array exists before UDP operations begin, which is good defensive programming.Also applies to: 63-63
98-99: LGTM!The early return when no clients are connected is a reasonable optimization—skipping UI updates when no one is watching. Since device state is ephemeral (rebuilt from UDP discovery), not persisting local modifications is acceptable.
131-142: LGTM!The broadcast-based device announcement and self-registration pattern is appropriate for UDP discovery.
Front-end ======== - Field renderer: mdnsName url goes to lights control Back-End ======= - Devices: show for max 24 hours
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
interface/src/lib/stores/telemetry.ts (1)
50-50: Prefer strict equality and consider a more robust falsy check.The loose equality operator
!=should be replaced with strict equality!==for better type safety. Additionally, the current check only handles empty strings but notnullorundefined. Consider usingdata.hostName || 'MoonLight'for a more idiomatic approach that handles all falsy values.Also note that lines 42 and 47 store
data.hostNamedirectly into the reactive state without applying this fallback logic, which could create an inconsistency where the store contains an empty/null/undefinedhostNamewhilelocalStoragealways has a valid value. Verify whether this behavior is intentional.🔎 Suggested refactor
- localStorage.setItem('telemetry.rssi.hostName', data.hostName != "" ? data.hostName : 'MoonLight'); + localStorage.setItem('telemetry.rssi.hostName', data.hostName || 'MoonLight');If you want to be more explicit and only fallback on empty strings (not other falsy values):
- localStorage.setItem('telemetry.rssi.hostName', data.hostName != "" ? data.hostName : 'MoonLight'); + localStorage.setItem('telemetry.rssi.hostName', data.hostName !== "" ? data.hostName : 'MoonLight');If the inconsistency is unintended, consider applying the same fallback logic to lines 42 and 47:
- rssi: { rssi: Number(data.rssi), ssid: data.ssid, disconnected: false, safeMode: data.safeMode, restartNeeded: data.restartNeeded, saveNeeded: data.saveNeeded, hostName: data.hostName } + rssi: { rssi: Number(data.rssi), ssid: data.ssid, disconnected: false, safeMode: data.safeMode, restartNeeded: data.restartNeeded, saveNeeded: data.saveNeeded, hostName: data.hostName || 'MoonLight' }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/moonlight/drivers.md(4 hunks)interface/src/lib/components/moonbase/FieldRenderer.svelte(2 hunks)interface/src/lib/stores/telemetry.ts(1 hunks)src/MoonBase/Modules/ModuleDevices.h(3 hunks)src/MoonLight/Modules/ModuleEffects.h(2 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- interface/src/lib/components/moonbase/FieldRenderer.svelte
- src/MoonLight/Nodes/Drivers/D_ArtnetIn.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/MoonBase/Modules/ModuleDevices.h (2)
src/MoonBase/Module.cpp (2)
update(271-302)update(271-271)src/MoonBase/Module.h (1)
ModuleState(47-137)
src/MoonLight/Modules/ModuleEffects.h (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
SolidEffect(14-39)MoonManEffect(1199-1260)PixelMapEffect(1494-1509)src/MoonLight/Nodes/Effects/E_MoonModules.h (3)
GameOfLifeEffect(17-344)GEQ3DEffect(348-490)PaintBrushEffect(494-566)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
docs/moonlight/drivers.md (1)
76-76: Verified grammar corrections from previous reviews.All grammar and clarity issues flagged in previous reviews (commits a946662, acdc095, 5cafe58) have been properly applied:
- Port description (line 76): Clear, grammatically correct.
- Controller IPs (line 96): Fixed plurals, verb tenses ("send" not "sent"), removed duplication.
- Nr of outputs (line 100): "more than" instead of "more then".
- Nr of Outputs per IP (line 102): "How many" for countable nouns, "are sent" passive voice.
Also applies to: 96-96, 100-100, 102-102
src/MoonLight/Modules/ModuleEffects.h (2)
175-261: ThecheckAndAllochelper function is correctly implemented and the refactor is sound.Verification confirms:
- Helper is properly defined in
NodeManager.h(base class) as a template that checks ifnamematchesT::name()usingequalAZaz09()- Allocates using
allocMBObject<T>()which uses placement new on a memory pool- Returns
nullptrwhen the name doesn't match- All effect classes implement the required static
name()methodThe uniform refactor from explicit comparisons to
checkAndAlloc<EffectClass>(name)is correct and improves readability without sacrificing functionality.
63-70: No breaking change - control system uses indices for storage, not strings.The code change only updates the UI display labels. The control system works as follows:
control["default"] = 0sets the default index (not a string value)addControlValue()adds display labels ("Layer 1", "Layer 2", etc.) to the UI's values array- The actual stored state value remains a numeric index (0, 1, 2, etc.)
- Code like
layerP.layers[view - 1]confirms indices are used internallyThe past review issue about
default = 0mismatch is also incorrect—index 0 correctly refers to the first entry ("Layer 1" at UI index 0). No state serialization, migration logic, or consumer code updates are needed.Likely an incorrect or invalid review comment.
src/MoonBase/Modules/ModuleDevices.h (6)
40-40: LGTM — devices table is now read-only.Removing delete capability ("rd" → "r") prevents accidental deletion of discovered devices from the UI. This is a reasonable UX choice for auto-discovered devices.
76-95: LGTM — working on a local copy avoids mutation during iteration.The refactor to copy
_state.datainto a localJsonDocumentbefore modification is a sound pattern that prevents issues with modifying the collection while iterating.
101-112: LGTM — filtering stale devices and sorting is correctly implemented.The 86400-second (1-day) filter effectively cleans up devices that haven't been seen recently. If
dev["time"]is missing,as<time_t>()returns 0, causing the device to be filtered out — a reasonable fallback for malformed entries.
114-115: LGTM!The constructed document is correctly passed to
update()which will diff against the current state and apply the sorted/filtered device list.
118-126: Good improvement — validating minimum packet size prevents malformed packet processing.Checking
packetSize >= sizeof(UDPMessage)ensures the buffer contains at least a completeUDPMessagebefore accessingbuffer + 6for the name field. This guards against buffer underread from truncated or malformed packets.
27-27: NetworkUDP interface compatibility could not be fully verified.The interface methods being used on
NetworkUDPin the code (begin(),parsePacket(),read(),write(),beginPacket(),endPacket(),remoteIP(),remotePort(),stop(),clear()) align with standard WiFiUDP signatures. However, theNetworkUDPclass definition could not be located in the project codebase, suggesting it originates from the framework layer (likely ESP32SvelteKit or Arduino core). Without access to the actual NetworkUDP definition or header file, complete interface compatibility verification cannot be confirmed. Verify that NetworkUDP is properly defined in the framework dependencies and that all expected method signatures match their usage patterns throughout the codebase.
| * **Controller IPs**: The last segment of the IP address within your local network, of the hardware Art-Net controller. Add more IPs if you send to more than one controller, comma separated. | ||
| * **Port**: The network port added to the IP address, 6454 is the default for Art-Net. | ||
| * **FPS Limiter**: set the max frames per second Art-Net packages are send out (also all the other nodes will run at this speed). | ||
| * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested) |
There was a problem hiding this comment.
Fix list indentation per markdown standards.
Line 99 uses 4-space indentation instead of the standard 2-space indentation for nested list items.
- * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested)
+ * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested)📝 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.
| * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested) | |
| * Art-Net specs recommend about 44 FPS but higher framerates will work mostly (up to until ~130FPS tested) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
99-99: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
docs/moonlight/drivers.md around line 99: the nested list item uses 4-space
indentation instead of the standard 2-space indentation for markdown lists;
change the indentation of the nested bullet(s) on line 99 from 4 spaces to 2
spaces so the list conforms to markdown nesting conventions and renders
correctly.
|
@coderabbitai, I have a lot of MoonLight devices in my network and it looks since recently some of the devices are less responding and looks like they loose their web server network (port 80, including web sockets) connection. UDP (WLED Audio Sync: port 11988, only receiving) and Module Devices: port 65506 both for sending and receiving small device info packages) seems to be more stable, although they also can stop working. It's only on some devices, not all. What could be te problem? A recent change I made? Network/router behavior? (I have a openwrt router) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5827 🏁 Script executed: Length of output: 778 🏁 Script executed: Length of output: 8234 🏁 Script executed: Length of output: 1188 🏁 Script executed: Length of output: 4920 🏁 Script executed: Length of output: 2288 🏁 Script executed: Length of output: 1491 🏁 Script executed: Length of output: 5069 🏁 Script executed: Length of output: 472 🏁 Script executed: Length of output: 2443 🏁 Script executed: Length of output: 1829 🏁 Script executed: Length of output: 1049 @ewowi, I found the root cause of your network stability issues! There are two critical problems in this PR, with one being extremely dangerous. 🔴 CRITICAL: StatefulService Mutex BugThe most serious issue is in Old behavior: xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // Block forever until acquiredNew behavior (lines 89-95): if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(100))==pdFALSE) {
ESP_LOGI("🐼", "_accessMutex wait 100ms");
if (xSemaphoreTakeRecursive(_accessMutex, pdMS_TO_TICKS(400))==pdFALSE) {
ESP_LOGW("🐼", "_accessMutex waited 500ms and continues 🤷♂️");
// xSemaphoreTakeRecursive(_accessMutex, portMAX_DELAY); // ← COMMENTED OUT!
}
}The ProblemIf the mutex cannot be acquired within 500ms, the code continues WITHOUT mutex protection! This means:
This explains why:
|
|
@coderabbitai, nope this is not the problem. I tried reversing 'f (!_socket->getConnectedClients()) return; ' but that didn't change anything . StatefulService mutex problem I accept as a risk - but didn't happen in the specific devices. So dig further please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12298 🏁 Script executed: Length of output: 3589 🏁 Script executed: Length of output: 6837 🏁 Script executed: Length of output: 6315 🏁 Script executed: Length of output: 12631 |
|
@coderabbitai, nope this is not the problem. I tried reversing 'f (!_socket->getConnectedClients()) return; ' but that didn't change anything . StatefulService mutex problem I accept as a risk - but didn't happen in the specific devices. Could you further review and see if you can find the root cause? Could it also be something in the browser or the network? |
|
@coderabbitai, the browser says: Unhandled Promise Rejection: TypeError: Importing a module script failed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3967 🏁 Script executed: Length of output: 4609 🏁 Script executed: Length of output: 13728 🏁 Script executed: Length of output: 4934 🏁 Script executed: Length of output: 6108 🌐 Web query: 💡 Result: Short summary — yes: multiple users have reported UDP/network instability and memory-related issues with Arduino-ESP32 core 3.x (drops or stops receiving UDP, lwIP/tcpip assertions, apparent leaks/heap fragmentation), and several reports point to regressions introduced in the 3.x line (including 3.2 RC). Workarounds used by the community are downgrading to 3.1.x, disabling certain lwIP features, and avoiding heavy dynamic allocation. Sources: [1][2][3][4]. Actionable checklist (concise)
Key references
If you want, tell me: which core version you’re using, your packet rate/packet size, whether PSRAM is enabled, and I’ll suggest specific sdkconfig options and a short test sketch to reproduce and measure the problem. 🌐 Web query: 💡 Result: Common causes and fixes for ESP32 + Arduino WebSocket (Network/TCP) disconnects Short checklist (try in this order)
Minimal example changes to try
References
If you want, tell me which WebSocket library and code snippet you’re using and I’ll give specific code changes and exact ping/timeout values to add. 🌐 Web query: 💡 Result: Short answer
Practical differences / when to use which
References
@ewowi, I found it! The root cause is a known bug in Arduino ESP32 core 3.x NetworkUDP combined with the JSON allocation pattern in 🔴 The Root CauseYou're using Arduino ESP32 core 3.x (platform 55.03.34), and
💥 Why This Causes Your SymptomsMultiple users have reported UDP/network instability and memory-related issues with Arduino-ESP32 core 3.x, with reports pointing to regressions introduced in the 3.x line. The combination of:
Results in:
🎯 Why Only Some Devices FailDevices with less free PSRAM/heap, more active effects, or higher network load hit the fragmentation threshold first. This perfectly matches your observations! 🔧 Recommended Fixes (in priority order)1. Revert NetworkUDP → WiFiUDP (Immediate fix)Since you're only using WiFi (not Ethernet), revert to src/MoonBase/Modules/ModuleDevices.h: - NetworkUDP deviceUDP;
+ WiFiUDP deviceUDP;src/MoonLight/Nodes/Drivers/D_ArtnetIn.h: - NetworkUDP artnetUdp;
+ WiFiUDP artnetUdp;
2. Disable WiFi Power Save (Helps WebSocket stability)WiFi.setSleep(false) before WiFi.begin() removes periodic radio sleep that causes drops. Add in your WiFi initialization: WiFi.setSleep(false);
WiFi.begin(ssid, pass);3. Optimize updateDevices() AllocationsReduce heap churn by eliminating the temporary JsonDocuments: void updateDevices(const char* name, IPAddress ip) {
if (_state.data["devices"].isNull()) _state.data["devices"].to<JsonArray>();
JsonArray devices = _state.data["devices"];
JsonObject device = JsonObject();
// Find existing device
for (JsonObject dev : devices) {
if (dev["ip"] == ip.toString()) {
device = dev;
break;
}
}
// Add new device
if (device.isNull()) device = devices.add<JsonObject>();
device["name"] = name;
device["time"] = time(nullptr);
device["ip"] = ip.toString();
// Only emit once per second
if (!_socket->getConnectedClients()) return;
static unsigned long lastEmit = 0;
unsigned long now = millis();
if (now - lastEmit < 1000) return;
lastEmit = now;
// Sorting/emitting only when actually needed
update();
}4. Consider Downgrading Arduino Core (If above doesn't help)Many users report regressions fixed/stable on 3.1.x, recommending rollback from 3.2.x. Update platform = https://github.com/pioarduino/platform-espressif32/releases/download/53.03.13/platform-espressif32.zip📊 Why This Wasn't ObviousThe 🐰 Next Steps
Would you like me to prepare a complete patch with fixes #1, #2, and #3? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2668 🏁 Script executed: Length of output: 2712 🏁 Script executed: Length of output: 8541 🏁 Script executed: Length of output: 2612 🏁 Script executed: Length of output: 10147 🏁 Script executed: Length of output: 12081 🏁 Script executed: Length of output: 10939 @ewowi, the browser error "Importing a module script failed" is very specific - it means the initial HTML page loads successfully, but when the browser tries to fetch the JavaScript module files, that second HTTP request fails. This points to either the server becoming unresponsive mid-page-load or network/router issues. 🔍 Diagnostic QuestionsTo isolate the root cause, I need more information:
🔧 Troubleshooting StepsTest 1: Is it the server crashing?Connect serial console and watch for:
Test 2: Is the server hung but device alive?When the browser error occurs: # From your PC, can you still ping the device?
ping <device-ip>
# Can you reach the REST API?
curl http://<device-ip>/rest/features
# Can the device still send/receive UDP?
# (check if Device Discovery still works)Test 3: Is it an OpenWRT router issue?OpenWRT routers can exhaust their TCP connection tracking table. Check: # On your OpenWRT router:
cat /proc/sys/net/netfilter/nf_conntrack_count
cat /proc/sys/net/netfilter/nf_conntrack_max
# If count is close to max, increase the limit:
sysctl -w net.netfilter.nf_conntrack_max=32768Test 4: Isolate one device
💡 Likely CulpritsGiven your symptoms (only some devices, UDP more stable than TCP): Most Likely: Memory exhaustion → HTTP server task crash
Second Most Likely: OpenWRT connection tracking exhaustion
Less Likely: Task starvation/blocking
🛠️ Immediate Actions You Can Take
Can you run these diagnostic steps and report back what you find? That will help us pinpoint the exact cause. 🐰 |
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)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (2)
132-136: Critical: Art-Net In bypasses RGB2040 remapping when writing to physical layer.When
view == 0(Physical layer), this code writes directly tolayerP.lights.channelsusing sequential LED indices viamemcpy. This bypasses the RGB2040 physical-index remapping that appliesindexP += (indexP / 20) * 20to skip every other 20-LED block.This directly causes the RGB2040 issue described in the PR comments: Art-Net packets will write pixel data into the "skip" ranges (20..39, 60..79, etc.), corrupting those channel values and causing the reported flickering.
The same issue exists in
handleDDP()at lines 162-166.Solution: When writing to physical layer with RGB2040 active, apply the same physical-index remapping before writing to channels, or route through VirtualLayer.setLight() which handles the mapping correctly.
🔎 Verification script to check if RGB2040 is active
#!/bin/bash # Search for RGB2040 preset usage and physical index remapping logic rg -nP -C3 'RGB2040|indexP \+= \(indexP / 20\)'Based on PR comments, this is a known active issue.
162-166: Critical: Same RGB2040 bypass issue in DDP handler.This DDP handler has the identical issue as
handleArtNet()- whenview == 0, it writes directly to sequential physical channel indices without applying RGB2040 remapping. This will corrupt the skip ranges (20..39, 60..79, etc.).Apply the same fix as recommended for the Art-Net handler.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
interface/src/lib/stores/telemetry.ts(1 hunks)interface/src/routes/+layout.svelte(1 hunks)interface/src/routes/statusbar.svelte(1 hunks)platformio.ini(1 hunks)src/MoonBase/Modules/ModuleDevices.h(3 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- interface/src/lib/stores/telemetry.ts
- platformio.ini
🧰 Additional context used
🧠 Learnings (2)
📚 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/Nodes/Drivers/D_ArtnetIn.h
📚 Learning: 2025-12-18T15:15:57.808Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.808Z
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
⏰ 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 (8)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (3)
20-20: NetworkUDP migration looks good.The change from WiFiUDP to NetworkUDP aligns with the broader UDP abstraction changes across the codebase.
24-24: Comment clarification improves readability.The updated description more clearly explains the view semantics.
126-126: Spacing fix improves code consistency.The corrected spacing in the arithmetic expression follows standard formatting conventions.
src/MoonBase/Modules/ModuleDevices.h (4)
27-27: NetworkUDP migration consistent with project-wide changes.This aligns with the NetworkUDP abstraction used throughout the codebase, including D_ArtnetIn.h.
40-40: CRUD mode changed to read-only.The change from
"rd"(read/delete) to"r"(read-only) removes delete capability from the devices control. Ensure this is the intended behavior.
59-70: loop10s now proceeds with UDP setup regardless of connected clients.Previously, this method would early-return if no clients were connected. Now it proceeds with UDP initialization and writeUDP(). This means device discovery broadcasts will continue even when no clients are viewing the UI, which could be desirable for maintaining an up-to-date device list.
120-120: More defensive UDP packet size check.The change from
packetSize > 0topacketSize >= sizeof(UDPMessage)ensures only complete UDP messages are processed, preventing potential buffer underruns.src/MoonLight/Layers/PhysicalLayer.cpp (1)
44-44: This change aligns with documented goals for non-PSRAM boards.The 4096 * 3 value directly implements the heap-optimization goal stated in the code comments: "goal is to have lights.channels as large as possible, preferable 12288 at least for boards without PSRAM." Since 4096 * 3 = 12288 channels exactly, this change correctly allocates the maximum safe buffer size for the ~60 KB heap available on non-PSRAM boards. The inline comment documenting the 1024→2048→4096 LED progression reflects the historical development of ESP32-D0 support limits, not a contradiction. No action required.
| JsonDocument doc; | ||
| doc.set(_state.data); // copy | ||
| JsonArray devices = doc["devices"]; | ||
|
|
||
| JsonObject device = JsonObject(); | ||
|
|
||
| for (JsonObject dev : _state.data["devices"].as<JsonArray>()) { | ||
| for (JsonObject dev : devices) { | ||
| if (dev["ip"] == ip.toString()) { | ||
| device = dev; | ||
| // EXT_LOGD(ML_TAG, "updated ...%d %s", ip[3], name); | ||
| } | ||
| } | ||
| if (device.isNull()) { | ||
| device = _state.data["devices"].as<JsonArray>().add<JsonObject>(); | ||
| device = devices.add<JsonObject>(); | ||
| EXT_LOGD(ML_TAG, "added ...%d %s", ip[3], name); | ||
| device["ip"] = ip.toString(); | ||
| } | ||
|
|
||
| device["name"] = name; // name can change | ||
| device["time"] = time(nullptr); // time will change | ||
| device["time"] = time(nullptr); // time will change, triggering update | ||
|
|
||
| if (!_socket->getConnectedClients()) return; | ||
| if (!_socket->getConnectedClients()) return; // no need to update if no clients | ||
| if (!WiFi.localIP() && !ETH.localIP()) return; | ||
|
|
||
| // sort in vector | ||
| std::vector<JsonObject> v; | ||
| for (JsonObject obj : _state.data["devices"].as<JsonArray>()) v.push_back(obj); | ||
| std::sort(v.begin(), v.end(), [](JsonObject a, JsonObject b) { return a["name"] < b["name"]; }); | ||
|
|
||
| // send only the readonly values ... | ||
| std::vector<JsonObject> devicesVector; | ||
| for (JsonObject dev : devices) { | ||
| if (time(nullptr) - dev["time"].as<time_t>() < 86400) devicesVector.push_back(dev); // max 1 day | ||
| } | ||
| std::sort(devicesVector.begin(), devicesVector.end(), [](JsonObject a, JsonObject b) { return a["name"] < b["name"]; }); | ||
|
|
||
| // add sorted to devices | ||
| JsonDocument devices; | ||
| devices["devices"].to<JsonArray>(); | ||
| for (auto& obj : v) devices["devices"].add(obj); | ||
| // create sorted devices | ||
| JsonDocument doc2; | ||
| doc2["devices"].to<JsonArray>(); | ||
| for (JsonObject device : devicesVector) { | ||
| doc2["devices"].add(device); | ||
| } | ||
|
|
||
| JsonObject object = devices.as<JsonObject>(); | ||
| update(object, ModuleState::update, _moduleName + "server"); | ||
| JsonObject controls = doc2.as<JsonObject>(); | ||
| update(controls, ModuleState::update, _moduleName + "server"); | ||
| } |
There was a problem hiding this comment.
Device updates may be lost when no clients are connected.
The method updates the device in a local copy of the state (lines 82-95), but then performs an early return at line 97 if no clients are connected. This means:
- The device update in the local
docis never persisted to_state.data - The
update()call at line 115 is never reached - Device information will be lost/stale when clients disconnect and reconnect
Additionally, filtering and sorting the entire device list on every update could be inefficient if updateDevices is called frequently, though this may be acceptable given typical device discovery rates.
🔎 Suggested fix
Consider persisting the device update to _state.data before the early return, or move the early return to before the sorting/broadcasting logic:
device["name"] = name; // name can change
device["time"] = time(nullptr); // time will change, triggering update
+ // Persist the update to state
+ _state.data["devices"] = devices;
+
if (!_socket->getConnectedClients()) return; // no need to broadcast if no clients
if (!WiFi.localIP() && !ETH.localIP()) return;This ensures device data is retained while still avoiding unnecessary broadcast/sorting work when no clients are listening.
Front-end ======== - hostName directly from telemetry Back-end ======== - Module Devices, refactor updateDevices
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleDevices.h (1)
103-107: Consider caching sorted device list to reduce overhead.The filtering and sorting logic runs on every device update when clients are connected. For typical device discovery rates, this overhead is acceptable. However, if
updateDevicesis called frequently (e.g., multiple times per second per device), the repeated sorting could impact performance.Consider adding a debounce mechanism or caching the sorted list with a short TTL if performance issues are observed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
interface/src/lib/stores/telemetry.ts(0 hunks)interface/src/routes/+layout.svelte(1 hunks)interface/src/routes/statusbar.svelte(1 hunks)src/MoonBase/Modules/ModuleDevices.h(4 hunks)
💤 Files with no reviewable changes (1)
- interface/src/lib/stores/telemetry.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/+layout.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/src/lib/stores/**/*.{ts,tsx} : Store global application state using Svelte stores in `interface/src/lib/stores/`
Applied to files:
interface/src/routes/+layout.svelte
⏰ 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 (4)
interface/src/routes/statusbar.svelte (1)
98-98: Previous review feedback properly addressed.The hostname is now rendered using the reactive telemetry store (
$telemetry.rssi.hostName) instead of direct localStorage access. This change correctly resolves the SSR compatibility issue and ensures the UI updates automatically when the hostname changes via WebSocket, consistent with the reactive pattern used throughout this file.interface/src/routes/+layout.svelte (1)
159-159: Previous review feedback properly addressed.The document title now uses the reactive telemetry store (
$telemetry.rssi.hostName) instead of direct localStorage access. This correctly resolves the SSR issue (wherelocalStorageis unavailable on the server) and ensures the title updates automatically when the hostname changes via WebSocket events (line 138).src/MoonBase/Modules/ModuleDevices.h (2)
27-27: Verify NetworkUDP stability and heap impact.The switch from
WiFiUDPtoNetworkUDPmay contribute to heap fragmentation and network instability, as discussed in PR objectives. The combination of NetworkUDP and frequentJsonDocumentallocations inupdateDevices(lines 76, 110) was identified as a potential cause of HTTP/WebSocket failures and browser module import errors.Consider monitoring heap usage and network stability after this change, or revert to
WiFiUDPif instability persists.Based on PR objectives discussion.
122-122: LGTM: Stricter packet size validation.The change from
packetSize > 0topacketSize >= sizeof(UDPMessage)is a good defensive improvement, ensuring a complete message is received before processing. The sizeof calculation is correct: UDPMessage is 38 bytes (6 bytes forrommel[6]+ 32 bytes forChar<32>).
Summary by CodeRabbit
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.