71 preps #104
71 preps #104
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Node sync & control encoding src/MoonBase/Nodes.h, src/MoonBase/Nodes.cpp |
Node stores const SemaphoreHandle_t* layerMutex; constructor now accepts it; per-node semaphore removed. Control size encodings remapped (uint32_t→32, int→33, float→34) and updateControl branches adjusted. |
Layer mutex plumbing & call sites src/MoonLight/Modules/ModuleEffects.h, src/MoonLight/Modules/ModuleDrivers.h, src/MoonBase/NodeManager.h, src/MoonLight/Layers/VirtualLayer.cpp, src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonLight/Layers/PhysicalLayer.h |
Added per-layer effectsMutex/driversMutex; nodes constructed with pointer to layer mutex; node locking changed to *node->layerMutex; PhysicalLayer initializes and deletes new mutexes. |
Parlio driver API & implementation src/MoonLight/Nodes/Drivers/parlio.h, src/MoonLight/Nodes/Drivers/parlio.cpp, src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h |
show_parlio and helpers now accept uint8_t components, uint16_t* leds_per_output, and offsetW; added max_leds_per_output and first_index_per_output; transpose/padding/packing logic updated to per-output counts and COMPONENTS_PER_PIXEL-based packing. |
Module update coordination & names src/MoonBase/Module.h, src/MoonBase/Module.cpp |
Replaced per-instance semaphores with static ModuleState::updateMutex, updatePending flag and static updatedItem; getUpdate() changed to void; Module constructor and _moduleName switched from String to const char* (call sites updated to use String(...) where needed). |
Persistence & event wiring src/MoonBase/SharedFSPersistence.h, src/MoonBase/SharedEventEndpoint.h, src/MoonBase/SharedHttpEndpoint.h, src/MoonBase/SharedWebSocketServer.h |
Persistence registry keys changed to const char* with CStrComparator; API methods updated to accept const char*; event/endpoint wiring adjusted to use module->_moduleName and explicit String(...) conversions at call sites. |
UI / monitor interface/src/routes/moonbase/monitor/monitor.ts |
setMatrixDimensions(width,height,depth=1) added matrixDepth; MVP/scale normalization updated to include depth. |
Effects, layouts, small fixes src/MoonLight/Nodes/Effects/E_MoonLight.h, src/MoonLight/Nodes/Effects/E__Sandbox.h, src/MoonLight/Nodes/Layouts/L_MoonLight.h |
VUMeter needle clamped to layer bounds; sandbox comment warns about bounds; TorontoBarGourdsLayout default granularity changed 0→2. |
Docs & nav docs/develop/architecture.md, docs/develop/drivers.md, docs/moonlight/drivers.md, mkdocs.yml |
Docs updated to reflect layerMutex usage and new Parlio driver details; new drivers.md added to nav. |
Build meta & misc UI platformio.ini, interface/src/routes/moonbase/filemanager/FileManager.svelte |
APP_DATE incremented (20260112 → 20260113); removed stray semicolon in Svelte file. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Client
participant NodeManager
participant Node
participant PhysicalLayer
participant ParlioDriver as Parlio(show_parlio)
participant Hardware
Client->>NodeManager: request control update
NodeManager->>Node: locate node, lock *node->layerMutex
Node->>PhysicalLayer: apply mapping/update (holds layer mutex)
PhysicalLayer->>ParlioDriver: call show_parlio(..., components, leds_per_output, offsets)
ParlioDriver->>ParlioDriver: compute max_leds_per_output, first_index_per_output, pad per-output data
ParlioDriver->>Hardware: DMA/write parallel outputs
Hardware-->>ParlioDriver: ack/complete
ParlioDriver-->>PhysicalLayer: return
PhysicalLayer-->>Node: return
NodeManager-->>Node: unlock *node->layerMutex
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Add node mutex #103 — Overlapping changes to per-node vs layer-level mutex design and Node constructor signature.
- Board presets ESP32-P4 #72 — Related Parlio driver refactor (transpose/padding and show_parlio signature changes).
- Board presets tuning #66 — Related Node control handling and updateControl/size-type adjustments.
Suggested labels
enhancement
Poem
🐰 I hop in layers, mutex snug and tight,
Parlio pads outputs through the night.
Depths and components hum in tune,
Modules whisper updates under the moon.
A rabbit refactors, happy to prune.
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title '71 preps' is vague and non-descriptive. It uses minimal phrasing that does not convey any meaningful information about what the changeset accomplishes, making it unclear to someone reviewing the commit history. | Consider revising the title to be more descriptive of the main changes, such as 'Refactor mutex usage, fix memory issues, and improve PARLIO driver support' or similar. A clear, specific title helps reviewers quickly understand the changeset's primary purpose. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/MoonBase/Nodes.h (1)
158-163: LGTM! Consider using named constants for size codes.The type detection logic is correct and consistent with the corresponding handling in
Nodes.cpp. The use ofstd::is_sameensures exact type matching.Minor suggestion: The magic numbers 33 and 34 for
intandfloat(since they're not actual bit sizes) could be replaced with named constants or an enum for improved readability and maintainability:enum ControlSize : uint8_t { SIZE_UINT8 = 8, SIZE_UINT16 = 16, SIZE_UINT32 = 32, SIZE_INT32 = 33, SIZE_FLOAT = 34 };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonBase/Nodes.cppsrc/MoonBase/Nodes.h
⏰ 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 (1)
src/MoonBase/Nodes.cpp (1)
34-42: LGTM! Changes are consistent with Nodes.h.The type casting aligns correctly with the size codes defined in
Nodes.h:
- Size 32 →
uint32_t*- Size 33 →
int*- Size 34 →
float*ArduinoJson's implicit conversion handles the value assignment appropriately for these numeric types, including negative values for
int.
|
@coderabbitai, for some reason the VUMeterEffect causes an idle watchdog crash (not encountered with other effects). Can you analyse the crash report: E (66625) task_wdt: Task watchdog got triggered. The following tasks/users did not reset the watchdog in time: Core 0 register dump: Stack memory: ELF file SHA256: 5229cc99a Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 4431 🏁 Script executed: Length of output: 1688 🏁 Script executed: Length of output: 10456 🏁 Script executed: Length of output: 1790 @ewowi, I've analyzed the crash report and identified the root cause. This is indeed the same watchdog starvation issue described in the retrieved learning from your previous work on extreme load scenarios. Root Cause AnalysisThe crash occurs because VUMeterEffect's nested loops combined with heavy drawing operations take too long without yielding, starving the IDLE task on CPU 0:
Recommended FixesBased on the retrieved learning, add Primary fix - VUMeterEffect.loop(): void loop() override {
layer->fadeToBlackBy(200);
uint8_t nHorizontal = 4;
uint8_t nVertical = 2;
uint8_t band = 0;
for (int h = 0; h < nHorizontal; h++) {
for (int v = 0; v < nVertical; v++) {
drawNeedle((float)sharedData.bands[2 * (band++)] / 2.0,
{layer->size.x * h / nHorizontal, layer->size.y * v / nVertical, 0},
{layer->size.x / nHorizontal, layer->size.y / nVertical, 0},
ColorFromPalette(layerP.palette, 255 / (nHorizontal * nVertical) * band));
taskYIELD(); // Add yield after each needle draw
}
}
}Defensive fixes - VirtualLayer.cpp: In while (x < y) {
// ... existing setRGB calls ...
x++;
taskYIELD(); // Give IDLE task a chance to run
}In for (uint8_t x = x0; x <= x1; x++) {
// ... existing setRGB calls ...
intersectY += gradient;
if (steep) std::swap(x, y);
taskYIELD(); // Give IDLE task a chance to run
}Would you like me to prepare these code changes for you? 🧠 Learnings used |
|
@coderabbitai, I first start with the VUMeter loop(), I. don't want to add it in drawCircle or drawLine (yet...). tried taskYield first but that gave a crash immediately. I think we concluded earlier that taskYield is not good, if so describe why and add as learning. So I used addYield(10) - the better alternative isn't it? But it also crashed. I will give you the crash report but with a few lines before - maybe that helps analyzing what led to the crash: [ 13496][D][NodeManager.h:87] checkAndAlloc(): [💫] ESP32 SvelteKit (4656) Allocate VU Meter ⏹️ ♫💫📺 Core 0 register dump: Stack memory: ELF file SHA256: 40577d602 Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7617 @ewowi, I can now explain the issues clearly: Why taskYIELD() Doesn't WorkYes, we already concluded this in a previous interaction (per retrieved learning ID taskYIELD() only yields to equal or higher priority tasks. Since your Effect Task runs at priority 3 and the IDLE task runs at priority 0 (lowest), Why addYield(10) at VirtualLayer Level Doesn't HelpLooking at the code, I see for (auto node : nodes) {
xSemaphoreTake(node->nodeMutex, portMAX_DELAY);
node->loop(); // ← VUMeterEffect.loop() executes here
xSemaphoreGive(node->nodeMutex);
addYield(10); // ← Too late if loop() took >5 seconds
}The problem: If The Real SolutionYou need to add void loop() override {
layer->fadeToBlackBy(200);
uint8_t nHorizontal = 4;
uint8_t nVertical = 2;
uint8_t band = 0;
for (int h = 0; h < nHorizontal; h++) {
for (int v = 0; v < nVertical; v++) {
drawNeedle((float)sharedData.bands[2 * (band++)] / 2.0,
{layer->size.x * h / nHorizontal, layer->size.y * v / nVertical, 0},
{layer->size.x / nHorizontal, layer->size.y / nVertical, 0},
ColorFromPalette(layerP.palette, 255 / (nHorizontal * nVertical) * band));
vTaskDelay(1); // ← Add here, after each drawNeedle
}
}
}This ensures the IDLE task gets CPU time every iteration (8 times per loop), preventing the watchdog timeout. Note: I don't see the ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai, I am testing on my local drive, not committed yet, but addYield(10) doesn't prevent crashing (which does a vTaskDelay(1) every 10 times, using vTaskDelay(1) directly also doesn't prevent it. One other theory. I removed the bound array checks when calling setLight (without a mapping) as I stated the effect should do it. I remembered the needles of the VUMeter effect can get out of the display, could that cause indirectly an idle error (e.g. if writing to another area e.g. the nr of lights could be changed to something ridiculously high - or something |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9203 🏁 Script executed: Length of output: 4061 🏁 Script executed: Length of output: 17268 @ewowi, excellent theory! You've found the root cause. The crash is definitely caused by memory corruption from out-of-bounds writes, not CPU starvation. Here's the complete vulnerability chain: The Memory Corruption Chain
Why vTaskDelay(1) Didn't HelpIt's not a scheduling issue—the system is experiencing memory corruption that damages FreeRTOS internals before the watchdog can be properly serviced. Recommended FixesOption 1: Restore bounds checking in setLight (safest): } else {
uint32_t index = indexV * layerP->lights.header.channelsPerLight + offset;
if (index + length <= layerP->lights.maxChannels) { // ✅ Restore this check
memcpy(&layerP->lights.channelsE[index], channels, length);
}
}Option 2: Add defensive bounds checking to drawCircle/drawLine before calling setRGB—check if coordinates are within Option 3: Fix VUMeterEffect to clamp needle coordinates: void drawNeedle(float angle, Coord3D topLeft, Coord3D size, CRGB color) {
int x0 = topLeft.x + size.x / 2;
int y0 = topLeft.y + size.y - 1;
layer->drawCircle(topLeft.x + size.x / 2, topLeft.y + size.y / 2, size.x / 2,
ColorFromPalette(layerP.palette, 35, 128), false);
int x1 = x0 - round(size.y * 0.7 * cos((angle + 30) * PI / 180));
int y1 = y0 - round(size.y * 0.7 * sin((angle + 30) * PI / 180));
// ✅ Clamp to valid bounds
x1 = max(0, min(x1, (int)layer->size.x - 1));
y1 = max(0, min(y1, (int)layer->size.y - 1));
layer->drawLine(x0, y0, x1, y1, color, true);
}I recommend Option 1 (restore bounds check) because it prevents all effects from causing memory corruption, not just VUMeter. Your original learnings about removing bounds checks for performance (IDs 9d55622b, fb7705fb) apply to the mapped path, but the unmapped path should retain safety checks since it's writing directly to the array based on calculated indices. Would you like me to prepare these changes? 🧠 Learnings used |
|
@coderabbitai, regarding
This solved the problem, cool. One more thing, the monitor is not aligned in the centre, it is slight shifted right, causing the right part to fall off the view window. Please come with a fix |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3362 @ewowi, Great that the clamping fixed the crash! The alignment issue is caused by integer division losing remainder pixels, making the grid left-aligned instead of centered. The ProblemWhen
The FixCalculate an offset to center the grid within the display: void loop() override {
layer->fadeToBlackBy(200);
uint8_t nHorizontal = 4;
uint8_t nVertical = 2;
// Calculate cell dimensions
uint16_t cellWidth = layer->size.x / nHorizontal;
uint16_t cellHeight = layer->size.y / nVertical;
// Calculate offset to center the grid
uint16_t offsetX = (layer->size.x - cellWidth * nHorizontal) / 2;
uint16_t offsetY = (layer->size.y - cellHeight * nVertical) / 2;
uint8_t band = 0;
for (int h = 0; h < nHorizontal; h++) {
for (int v = 0; v < nVertical; v++) {
drawNeedle(
(float)sharedData.bands[2 * (band++)] / 2.0,
{offsetX + cellWidth * h, offsetY + cellHeight * v, 0}, // Centered topLeft
{cellWidth, cellHeight, 0}, // Cell size
ColorFromPalette(layerP.palette, 255 / (nHorizontal * nVertical) * band)
);
}
}
}This ensures the 4×2 grid is perfectly centered regardless of display dimensions, preventing any part from falling off the edge. 🧠 Learnings used |
|
@coderabbitai, Regarding
It's related to Monitor.svelte and Monitor.ts, it happens with any effect displayed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ewowi, you're absolutely right - I apologize for the confusion. The monitor alignment issue is in the UI components, not the effect code. Let me investigate the Monitor.svelte and Monitor.ts files. 🏁 Script executed: Length of output: 5056 🏁 Script executed: Length of output: 6103 @ewowi, I found the issue! The problem is in the The Root CauseThe code calculates separate distances for width and height fit, then uses the maximum with padding. However, when one dimension requires more distance than the other, the smaller dimension isn't centered—it's just scaled and positioned at the origin without considering that it should be offset to appear centered in the viewport. The FixModify the function getMVPMatrix(): mat4 {
const canvas = gl!.canvas as HTMLCanvasElement;
const canvasAspect = canvas.width / canvas.height;
const fov = Math.PI / 6; // 30 degrees
const near = 0.1;
const far = 100.0;
const projection = mat4.create();
mat4.perspective(projection, fov, canvasAspect, near, far);
// Normalize the matrix dimensions to a unit square/rectangle
const maxDim = Math.max(matrixWidth, matrixHeight);
const normalizedWidth = matrixWidth / maxDim;
const normalizedHeight = matrixHeight / maxDim;
// Calculate LED matrix aspect ratio
const matrixAspect = matrixWidth / matrixHeight;
// Calculate scale factor to fit the matrix in the view while maintaining aspect ratio
let scale = 1.0;
if (matrixAspect > canvasAspect) {
// Matrix is wider than canvas - scale to fit width
scale = normalizedWidth;
} else {
// Matrix is taller than canvas - scale to fit height
scale = normalizedHeight / canvasAspect;
}
// Calculate camera distance to fit the scaled matrix
const effectiveSize = scale;
const cameraDistance = effectiveSize / (2 * Math.tan(fov / 2)) * 1.5; // 1.5 for padding
const view = mat4.create();
mat4.lookAt(view, [0, 0, cameraDistance], [0, 0, 0], [0, 1, 0]);
const model = mat4.create();
// Scale uniformly by the effective size to maintain aspect ratio and centering
mat4.scale(model, model, [normalizedWidth, normalizedHeight, 1]);
const mvp = mat4.create();
mat4.multiply(mvp, projection, view);
mat4.multiply(mvp, mvp, model);
return mvp;
}This approach:
The issue was that the previous code calculated distances for both width and height independently then took the max, which doesn't guarantee proper centering when aspects don't match. 🧠 Learnings used |
|
@coderabbitai, regarding
You proposed const cameraDistance = effectiveSize / (2 * Math.tan(fov / 2)) * 1.5; // 1.5 for padding But the effect was bigger then the display so had to change to const cameraDistance = effectiveSize / (2 * Math.tan(fov / 2)) * 5; // 5 for padding Could I use 5 instead of 1.5 or do I get off displays under certain circumstances? Can you also explain why 5 works and 1.5 not? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, yes, using 5 is perfectly safe and won't cause clipping issues under any circumstances. Here's why: Why 5 Works But 1.5 Doesn'tThe padding multiplier controls how far the camera moves away from the model:
The base formula
The reason 1.5 didn't work: My Why 5 works: It compensates for this underestimation by moving the camera much farther back, ensuring adequate margin around the LED matrix regardless of aspect ratio. Is 5 Safe?Absolutely. Using a larger padding factor like 5:
If you want less margin (bigger LED matrix), you could experiment with values like 3 or 4, but 5 is a safe, conservative choice that guarantees the entire matrix is always visible with comfortable padding. ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai, regarding
I found out with 5: 128 x 128 works okay 64 x 128 works okay, 128x 64 works okay, but 128 x 32 not (too small). This problem was not in the current solution, it was only shifted right. So can you look back at the original code and combine this with your solution so it scales well AND it aligns well |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4133 🏁 Script executed: Length of output: 6173 🏁 Script executed: Length of output: 2917 @ewowi, I found the issue! The problem is that vertices are already normalized to [-1, 1] in Monitor.svelte, then they're being scaled again in The Root CauseIn Monitor.svelte (vertex generation): x = width == 1 ? 0 : (x / (width - 1)) * 2.0 - 1.0;
y = height == 1 ? 0 : ((height - 1 - y) / (height - 1)) * 2.0 - 1.0;Vertices are normalized to [-1, 1] range and centered at origin. Then in monitor.ts (getMVPMatrix): mat4.scale(model, model, [normalizedWidth, normalizedHeight, 1]);They're scaled again by aspect ratio factors. The original camera distance calculation The FixKeep the original scaling and camera distance logic (it works!), but add an explicit translation to center the model properly: function getMVPMatrix(): mat4 {
const canvas = gl!.canvas as HTMLCanvasElement;
const canvasAspect = canvas.width / canvas.height;
const fov = Math.PI / 6; // 30 degrees
const near = 0.1;
const far = 100.0;
const projection = mat4.create();
mat4.perspective(projection, fov, canvasAspect, near, far);
// Normalize the matrix dimensions to a unit square/rectangle
const maxDim = Math.max(matrixWidth, matrixHeight);
const normalizedWidth = matrixWidth / maxDim;
const normalizedHeight = matrixHeight / maxDim;
// Calculate required distance for vertical fit
const verticalSize = normalizedHeight;
const distanceForHeight = verticalSize / (2 * Math.tan(fov / 2));
// Calculate required distance for horizontal fit
const horizontalFov = 2 * Math.atan(Math.tan(fov / 2) * canvasAspect);
const horizontalSize = normalizedWidth;
const distanceForWidth = horizontalSize / (2 * Math.tan(horizontalFov / 2));
// Use the larger distance to ensure both dimensions fit (keeps original scaling logic)
const cameraDistance = Math.max(distanceForHeight, distanceForWidth) * 2.5;
const view = mat4.create();
mat4.lookAt(view, [0, 0, cameraDistance], [0, 0, 0], [0, 1, 0]);
const model = mat4.create();
// Center the model at origin BEFORE scaling
mat4.translate(model, model, [-0.5 * (normalizedWidth - 1), -0.5 * (normalizedHeight - 1), 0]);
// Scale by the normalized dimensions
mat4.scale(model, model, [normalizedWidth, normalizedHeight, 1]);
const mvp = mat4.create();
mat4.multiply(mvp, projection, view);
mat4.multiply(mvp, mvp, model);
return mvp;
}The key addition is: mat4.translate(model, model, [-0.5 * (normalizedWidth - 1), -0.5 * (normalizedHeight - 1), 0]);This compensates for the asymmetry introduced by scaling the pre-normalized vertices, ensuring the scaled model remains centered at the origin before the camera views it. This should fix the right-shift while maintaining proper scaling for all aspect ratios including 128x32. 🧠 Learnings used |
|
@coderabbitai, regarding
This looks better, however it's not vertically centered anymore, please fix |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2496 @ewowi, I apologize - my translation formula was incorrect. Let me fix this properly. The Problem with My Previous FixThe translation formula I provided was mathematically wrong and would cause unpredictable shifts. Here's why: Vertices in The Correct FixRemove the translation entirely - the scaling alone should work: function getMVPMatrix(): mat4 {
const canvas = gl!.canvas as HTMLCanvasElement;
const canvasAspect = canvas.width / canvas.height;
const fov = Math.PI / 6; // 30 degrees
const near = 0.1;
const far = 100.0;
const projection = mat4.create();
mat4.perspective(projection, fov, canvasAspect, near, far);
// Normalize the matrix dimensions to a unit square/rectangle
const maxDim = Math.max(matrixWidth, matrixHeight);
const normalizedWidth = matrixWidth / maxDim;
const normalizedHeight = matrixHeight / maxDim;
// Calculate required distance for vertical fit
const verticalSize = normalizedHeight;
const distanceForHeight = verticalSize / (2 * Math.tan(fov / 2));
// Calculate required distance for horizontal fit
const horizontalFov = 2 * Math.atan(Math.tan(fov / 2) * canvasAspect);
const horizontalSize = normalizedWidth;
const distanceForWidth = horizontalSize / (2 * Math.tan(horizontalFov / 2));
// Use the larger distance to ensure both dimensions fit
const cameraDistance = Math.max(distanceForHeight, distanceForWidth) * 2.5;
const view = mat4.create();
mat4.lookAt(view, [0, 0, cameraDistance], [0, 0, 0], [0, 1, 0]);
const model = mat4.create();
// Scale by normalized dimensions - this preserves centering since vertices are pre-centered
mat4.scale(model, model, [normalizedWidth, normalizedHeight, 1]);
const mvp = mat4.create();
mat4.multiply(mvp, projection, view);
mat4.multiply(mvp, mvp, model);
return mvp;
}This is essentially the current code. If you're still seeing a right shift with this code, the issue is not in
Can you confirm whether the right shift occurs with the code exactly as shown above (no translation)? If so, we need to investigate other parts of the rendering pipeline. 🧠 Learnings used |
|
@coderabbitai, that's better, it only looks that 3D objects are slightly bigger than 2D objects, I can correct it by setting the cameradistance multiplier to 3 instead of 2.5, but then 2D is too small. Please find out why and correct |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8641 @ewowi, I found the issue! The problem is that 3D objects have z-coordinates ranging from -1 to 1, while 2D objects have all vertices at z=0. However, the model matrix only normalizes x and y dimensions—it always scales z by 1. The Root CauseIn Monitor.svelte (line 121):
In monitor.ts getMVPMatrix() (line 185): mat4.scale(model, model, [normalizedWidth, normalizedHeight, 1]); // ❌ z always scaled by 1Since z isn't normalized, 3D objects have vertices spread out in depth, with some closer to the camera. In perspective projection, closer vertices appear larger, making the entire 3D object look bigger overall compared to 2D objects (all at z=0). The FixTrack and normalize the depth dimension just like width and height: // Store LED matrix dimensions
let matrixWidth: number = 1;
let matrixHeight: number = 1;
let matrixDepth: number = 1; // ✅ Add depth tracking
// ... existing code ...
// New function to set the LED matrix dimensions
export function setMatrixDimensions(width: number, height: number, depth: number = 1) { // ✅ Add depth parameter
matrixWidth = width;
matrixHeight = height;
matrixDepth = depth;
}
function getMVPMatrix(): mat4 {
const canvas = gl!.canvas as HTMLCanvasElement;
const canvasAspect = canvas.width / canvas.height;
const fov = Math.PI / 6; // 30 degrees
const near = 0.1;
const far = 100.0;
const projection = mat4.create();
mat4.perspective(projection, fov, canvasAspect, near, far);
// Normalize ALL dimensions including depth
const maxDim = Math.max(matrixWidth, matrixHeight, matrixDepth); // ✅ Include depth in max
const normalizedWidth = matrixWidth / maxDim;
const normalizedHeight = matrixHeight / maxDim;
const normalizedDepth = matrixDepth / maxDim; // ✅ Normalize depth
// Calculate required distance for vertical fit
const verticalSize = normalizedHeight;
const distanceForHeight = verticalSize / (2 * Math.tan(fov / 2));
// Calculate required distance for horizontal fit
const horizontalFov = 2 * Math.atan(Math.tan(fov / 2) * canvasAspect);
const horizontalSize = normalizedWidth;
const distanceForWidth = horizontalSize / (2 * Math.tan(horizontalFov / 2));
// Use the larger distance to ensure both dimensions fit
const cameraDistance = Math.max(distanceForHeight, distanceForWidth) * 2.5;
const view = mat4.create();
mat4.lookAt(view, [0, 0, cameraDistance], [0, 0, 0], [0, 1, 0]);
const model = mat4.create();
// Scale by ALL normalized dimensions ✅
mat4.scale(model, model, [normalizedWidth, normalizedHeight, normalizedDepth]);
const mvp = mat4.create();
mat4.multiply(mvp, projection, view);
mat4.multiply(mvp, mvp, model);
return mvp;
}And update the call in Monitor.svelte (line 93): setMatrixDimensions(width, height, depth); // ✅ Pass depthThis ensures 2D and 3D objects are scaled consistently, making them appear the same size when they have the same width×height dimensions. 🧠 Learnings used |
frontend ======= - Monitor: fix scaling and aligning backend ======= - MoonLight Effects: fix VUMeter
instead of the first length First step to see if we can deal with different nr of leds per pin.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h:
- Around line 64-67: The loop computing maxLedsPerPin incorrectly iterates the
whole ledsPerPin array and can pick up UINT16_MAX sentinels; change it to
iterate only the first layerP.nrOfPins entries (use an index-based loop over
0..nrOfPins-1) when scanning layerP.ledsPerPin so only initialized pin counts
are considered; update references to maxLedsPerPin, layerP.ledsPerPin, and
layerP.nrOfPins accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/framework/WWWData.hsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-12T17:15:32.074Z
Learning: In MoonLight's Monitor.ts (interface/src/routes/moonbase/monitor/monitor.ts), the camera distance calculation uses a padding multiplier in the formula: `cameraDistance = effectiveSize / (2 * Math.tan(fov / 2)) * padding`. A padding value of 5 (not 1.5) is needed because the effectiveSize calculation underestimates the actual visible model extent after scaling by [normalizedWidth, normalizedHeight, 1]. Larger padding values move the camera farther away, making the object smaller with more margin. Using 5 is safe and prevents clipping for all LED matrix aspect ratios.
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Nodes/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: 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_ParallelLEDDriver.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
src/MoonLight/Nodes/Drivers/parlio.cpp (1)
show_parlio(267-267)
⏰ 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 (1)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
70-70: LGTM - correct semantic change for buffer sizing.Passing
maxLedsPerPininstead of the first pin's LED count ensures the Parlio driver allocates sufficient buffer for the largest output channel, which is the correct approach for parallel LED driving.
|
@coderabbitai, see issue #95 : A limitation of the parallel LED driver for esp32-p4 is that it all the leds_per_output / leds per pin should be the same for each pin. I try to find a solution to deal with setups with different leds per pin per pin. A necessary but probably not sufficient change is that the call to show_parlio() should set the argument leds_per_output to the max instead of only layerP.ledsPerPin[0] as I had before. Next step is that the implementation of show_parlio() should sent maxLedsPerPin to each pin, as the real world does not care if there are physical leds connected or not to a pin. So there will be a number of leds connected <= maxLedsPerPin on each pin. So my idea is that the implementation of show_parlio() will do that and it can use the layerP.lights.channelsD array it gets as input parameter but if for a specific pin maxLedsPerPin is larger it should send dummy data to that pin. See the file parlio.cpp for the implementation. You see there show_parlio() calls create_transposed_led_output_optimized() using parallel_buffer_remapped and parallel_buffer_repacked which is related to buffer_in which is related to my layerP.lights.channelsD (which contains just the real number of leds per pin, not maxLedsPerPin. So my question is: can you propose code changes such that show_parlio() sends maxLedsPerPin per pin, taking whatever it does with layerP.lights.channelsD but filling up the space with dummy values until maxLedsPerPin |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23403 @ewowi, I've analyzed the code and understand the issue. The problem is that Proposed SolutionModify void loop() override {
#ifdef ESP32_LEDSDRIVER
DriverNode::loop();
#ifndef CONFIG_IDF_TARGET_ESP32P4
if (ledsDriver.total_leds > 0) ledsDriver.showPixels(WAIT);
#else
uint8_t nrOfPins = min(layerP.nrOfLedPins, layerP.nrOfAssignedPins);
// Get max leds per pin
uint16_t maxLedsPerPin = 0;
for (uint16_t ledsPerPin : layerP.ledsPerPin) {
if (ledsPerPin > maxLedsPerPin) maxLedsPerPin = ledsPerPin;
}
// Calculate padded buffer size
uint8_t channelsPerPixel = layerP.lights.header.channelsPerLight;
uint32_t paddedLength = nrOfPins * maxLedsPerPin;
uint32_t paddedBufferSize = paddedLength * channelsPerPixel;
// Allocate padded buffer (static to avoid repeated allocations)
static uint8_t* paddedBuffer = nullptr;
static uint32_t lastPaddedBufferSize = 0;
if (paddedBuffer == nullptr || paddedBufferSize != lastPaddedBufferSize) {
if (paddedBuffer != nullptr) {
heap_caps_free(paddedBuffer);
}
// Allocate DMA-capable memory
paddedBuffer = (uint8_t*)heap_caps_malloc(paddedBufferSize, MALLOC_CAP_DMA);
lastPaddedBufferSize = paddedBufferSize;
if (paddedBuffer == nullptr) {
EXT_LOGE(ML_TAG, "Failed to allocate padded buffer for parallel output");
return;
}
}
// Zero out the entire buffer (dummy LEDs will be black)
memset(paddedBuffer, 0, paddedBufferSize);
// Copy actual LED data for each pin with padding
uint32_t srcOffset = 0; // Current position in channelsD
uint32_t dstOffset = 0; // Current position in paddedBuffer
for (uint8_t pin = 0; pin < nrOfPins; pin++) {
uint16_t actualLeds = layerP.ledsPerPin[pin];
uint32_t actualBytes = actualLeds * channelsPerPixel;
// Copy actual LED data for this pin
memcpy(paddedBuffer + dstOffset, layerP.lights.channelsD + srcOffset, actualBytes);
srcOffset += actualBytes;
dstOffset += maxLedsPerPin * channelsPerPixel; // Move to next pin's slot (includes padding)
}
// Call show_parlio with padded buffer
show_parlio(pins, paddedLength, paddedBuffer,
channelsPerPixel == 4, nrOfPins, maxLedsPerPin,
layerP.lights.header.offsetRed,
layerP.lights.header.offsetGreen,
layerP.lights.header.offsetBlue);
#endif
#else // ESP32_LEDSDRIVER
// ... existing code ...
#endif
}How It Works
Result
This approach keeps the complexity in the driver layer and doesn't require changes to the low-level 🧠 Learnings used |
|
@coderabbitai, your solution creates another padded buffer which I think is not optimal, could you come with an alternative solution where the padding is done within parlio.cpp where the transpose is taking place so we just use the buffers created there and pad these buffers |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, excellent point! Moving the padding into Modified parlio.cppChange extern "C" void show_parlio(
const unsigned char *pins,
unsigned int length, // Now: sum of actual LEDs across all pins
const unsigned char *buffer_in,
bool has_white,
unsigned int outputs,
unsigned int leds_per_output, // Now: maxLedsPerPin
const uint16_t *actual_leds_per_pin, // ✅ NEW: array of actual LED counts per pin
unsigned int offsetRed,
unsigned int offsetGreen,
unsigned int offsetBlue
)
{
// ... existing setup code ...
unsigned int channels_per_led = has_white ? 4 : 3;
unsigned int total_leds = outputs * leds_per_output; // Padded total
unsigned int total_channels = total_leds * channels_per_led;
// Allocate buffers for padded size
unsigned int total_transposed_size = leds_per_output * outputs * channels_per_led;
if (parallel_buffer_remapped == NULL || last_total_transposed_size != total_transposed_size) {
// ... existing reallocation logic ...
}
// ✅ NEW: Remap with per-pin padding
unsigned int src_offset = 0; // Position in buffer_in (actual data)
unsigned int dst_offset = 0; // Position in parallel_buffer_remapped (padded)
for (unsigned int pin = 0; pin < outputs; pin++) {
uint16_t actual_leds = actual_leds_per_pin[pin];
// Copy actual LEDs for this pin
for (unsigned int led = 0; led < actual_leds; led++) {
unsigned int src_base = src_offset + (led * channels_per_led);
unsigned int dst_base = dst_offset + (led * channels_per_led);
parallel_buffer_remapped[dst_base + 0] = buffer_in[src_base + offsetRed];
parallel_buffer_remapped[dst_base + 1] = buffer_in[src_base + offsetGreen];
parallel_buffer_remapped[dst_base + 2] = buffer_in[src_base + offsetBlue];
if (has_white && channels_per_led == 4) {
parallel_buffer_remapped[dst_base + 3] = buffer_in[src_base + 3];
}
}
// Pad remaining LEDs with zeros (black)
unsigned int padding_leds = leds_per_output - actual_leds;
unsigned int padding_start = dst_offset + (actual_leds * channels_per_led);
unsigned int padding_bytes = padding_leds * channels_per_led;
memset(parallel_buffer_remapped + padding_start, 0, padding_bytes);
// Advance offsets
src_offset += actual_leds * channels_per_led;
dst_offset += leds_per_output * channels_per_led; // Full slot including padding
}
// ✅ Rest of function unchanged - transpose already works with padded buffer
create_transposed_led_output_optimized(
parallel_buffer_remapped,
parallel_buffer_repacked,
leds_per_output,
channels_per_led,
outputs
);
// ... existing transmission code ...
}Modified D_ParallelLEDDriver.h call sitevoid loop() override {
#ifdef ESP32_LEDSDRIVER
DriverNode::loop();
#ifndef CONFIG_IDF_TARGET_ESP32P4
if (ledsDriver.total_leds > 0) ledsDriver.showPixels(WAIT);
#else
uint8_t nrOfPins = min(layerP.nrOfLedPins, layerP.nrOfAssignedPins);
// Calculate max LEDs per pin
uint16_t maxLedsPerPin = 0;
for (uint16_t ledsPerPin : layerP.ledsPerPin) {
if (ledsPerPin > maxLedsPerPin) maxLedsPerPin = ledsPerPin;
}
// Call show_parlio with actual LED counts - padding handled internally
show_parlio(
pins,
layerP.lights.header.numChannels / layerP.lights.header.channelsPerLight, // actual total LEDs
layerP.lights.channelsD, // actual data, no padding needed
layerP.lights.header.channelsPerLight == 4,
nrOfPins,
maxLedsPerPin, // max LEDs per pin
layerP.ledsPerPin, // ✅ NEW: pass actual LED counts array
layerP.lights.header.offsetRed,
layerP.lights.header.offsetGreen,
layerP.lights.header.offsetBlue
);
#endif
#else
// ... existing code ...
#endif
}Update parlio.h signatureextern "C" void show_parlio(
const unsigned char *pins,
unsigned int length,
const unsigned char *buffer_in,
bool has_white,
unsigned int outputs,
unsigned int leds_per_output,
const uint16_t *actual_leds_per_pin, // ✅ NEW parameter
unsigned int offsetRed,
unsigned int offsetGreen,
unsigned int offsetBlue
);Benefits✅ No intermediate buffer allocation 🧠 Learnings used |
|
@coderabbitai, regarding padding parallel_buffer_remapped... This is still creating more memory. Go deeper , into create_transposed_led_output_optimized Go to the level where transpose is taking place into dma regions and pad there. And confirm this is a better solution (or not) |
|
@coderabbitai, I didn't know SemaphoreHandle_t is about 80-100 bytes, especially the nodeMutex, one per node might be too much for no psram devices.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 21231 @ewowi, excellent analysis! I've completed a comprehensive mutex review. Here are the optimization opportunities: Memory Impact SummaryCurrent mutex overhead on ESP32 classic (no PSRAM):
Potential savings: ~1,850 bytes with these changes: 1. Replace
|
|
@coderabbitai, as saving 180B per module is the biggest gain, regarding updateReadySem and updateProcessedSem what about: void postUpdate(const UpdatedItem& updatedItem) { } does that work? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, this approach won't work and will cause system deadlocks. Here's why: Critical Problems1.
|
nodesMutex -> layerMutex - main: swapMutex from semaphore to portMUX_TYPE - Module: post/getUpdate: consolidate to updateMutex - Nodes: pointer to layerMutex - Physical layer: set effects and drivers Mutex (layerMutex) - Module drivers / effects: no default node (for esp32-d0), add node with layerNode
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 (2)
src/main.cpp (2)
131-158: Critical section held too long whenuseDoubleBufferis false.When
layerP.lights.useDoubleBufferis false, the code path skips theportEXIT_CRITICALat line 135 and executeslayerP.loop()(line 139) andlayerP.loop20ms()(line 143) while still holding the critical section. Based on learnings,layerP.loop()can take >5 seconds at extreme loads (49K channels). Holding a critical section this long will:
- Trigger watchdog timeouts (IDLE task starved)
- Block WiFi/BT interrupts on single-core ESP32
- Prevent
driverTaskfrom acquiring the lockThe double-buffer path correctly exits before heavy work and re-enters for the swap. The non-double-buffer path should follow the same pattern.
🐛 Proposed fix: exit critical section before heavy work in non-double-buffer path
if (layerP.lights.header.isPositions == 0 && !newFrameReady) { // within mutex as driver task can change this if (layerP.lights.useDoubleBuffer) { portEXIT_CRITICAL(&swapMutex); memcpy(layerP.lights.channelsE, layerP.lights.channelsD, layerP.lights.header.nrOfChannels); // Copy previous frame (channelsD) to working buffer (channelsE) + } else { + portEXIT_CRITICAL(&swapMutex); } layerP.loop(); if (millis() - last20ms >= 20) { last20ms = millis(); layerP.loop20ms(); } if (layerP.lights.useDoubleBuffer) { // Atomic swap channels portENTER_CRITICAL(&swapMutex); if (layerP.lights.header.isPositions == 0) { // Check if not changed while we were unlocked uint8_t* temp = layerP.lights.channelsD; layerP.lights.channelsD = layerP.lights.channelsE; layerP.lights.channelsE = temp; newFrameReady = true; } - } else + } else { + portENTER_CRITICAL(&swapMutex); newFrameReady = true; + } } portEXIT_CRITICAL(&swapMutex);
175-194: Same critical section issue in driverTask for non-double-buffer path.When
useDoubleBufferis false,layerP.loopDrivers()(line 190) executes inside the critical section. This mirrors the issue ineffectTaskand needs the same fix pattern.🐛 Proposed fix: exit critical section before loopDrivers in non-double-buffer path
if (layerP.lights.header.isPositions == 0) { if (newFrameReady) { newFrameReady = false; if (layerP.lights.useDoubleBuffer) { portEXIT_CRITICAL(&swapMutex); // Double buffer: release lock, then send mutexGiven = true; + } else { + portEXIT_CRITICAL(&swapMutex); // Non-double buffer: also release lock before heavy work + mutexGiven = true; } esp32sveltekit.lps++; layerP.loopDrivers(); } }
🤖 Fix all issues with AI agents
In @docs/develop/drivers.md:
- Line 9: The link fragment '#parallel-led-driver-for-esp32-p4' is invalid; edit
the line that reads "For end-user documentation, see [Parallel LED Driver for
ESP32-P4](#parallel-led-driver-for-esp32-p4)" and either change the fragment to
the correct anchor '#parallel-led-driver' (which points to the Parallel LED
Driver section in the moonlight drivers doc) or remove the internal link
entirely if it was not intended to be an anchor link.
In @src/MoonBase/Module.h:
- Around line 48-49: The binary semaphore created with xSemaphoreCreateBinary()
for updateMutex starts in the taken state and causes postUpdate() to block
forever on xSemaphoreTake; replace xSemaphoreCreateBinary() with
xSemaphoreCreateMutex() to get an initially available mutex (or, if you need a
binary semaphore, call xSemaphoreGive(updateMutex) once after creation) and keep
updatePending usage unchanged; update references to updateMutex creation in
Module.h accordingly so postUpdate() uses a semaphore that is initially
available.
In @src/MoonLight/Layers/PhysicalLayer.cpp:
- Around line 39-48: The destructor PhysicalLayer::~PhysicalLayer() uses
inverted null-checks causing vSemaphoreDelete to be called on NULL and leaking
semaphores; change the conditions to check for non-NULL (e.g., if (effectsMutex)
{ vSemaphoreDelete(effectsMutex); effectsMutex = NULL; }) and similarly for
driversMutex so semaphores are deleted only when allocated and pointers are
reset to NULL afterwards.
In @src/MoonLight/Layers/PhysicalLayer.h:
- Around line 124-128: The destructor PhysicalLayer::~PhysicalLayer() currently
uses inverted null-checks causing mutexes not to be deleted; change the
conditions to check for non-null (i.e., if (effectsMutex) and if (driversMutex))
before calling vSemaphoreDelete and then set effectsMutex/driversMutex to NULL
to avoid leaks and invalid deletes; ensure you reference and update
effectsMutex, driversMutex, vSemaphoreDelete and the
PhysicalLayer::~PhysicalLayer() implementation accordingly.
🧹 Nitpick comments (3)
src/MoonLight/Layers/VirtualLayer.cpp (1)
63-70: Same null-check concern forlayerMutexdereferencing.Similar to
NodeManager.h, this code dereferences*node->layerMutexwithout checking for null. While nodes should always havelayerMutexset viaconstructor(), a defensive check would prevent crashes if invariants are violated:for (Node* node : nodes) { if (prevSize != size) { + if (node->layerMutex) { xSemaphoreTake(*node->layerMutex, portMAX_DELAY); node->onSizeChanged(prevSize); xSemaphoreGive(*node->layerMutex); + } else { + node->onSizeChanged(prevSize); + } }Given the performance-critical nature of this code path (per retrieved learnings about 100K+ LED counts), you may choose to rely on the constructor contract instead.
docs/moonlight/drivers.md (2)
97-97: Minor: Consider hyphenating compound adjective."Art-Net compatible" should be "Art-Net-compatible" when used as a compound adjective before "packages".
Suggested fix
-Sends Lights in Art-Net compatible packages to an Art-Net controller specified by the IP address(es) provided. +Sends Lights in Art-Net-compatible packages to an Art-Net controller specified by the IP address(es) provided.
76-76: Consider adding alt text to images for accessibility.The images on this line lack alt text attributes, which impacts screen reader accessibility.
Suggested fix
- <img width="100" src="https://github.com/user-attachments/assets/98fb5010-7192-44db-a5c9-09602681ee15"/><img width="100" src="https://github.com/user-attachments/assets/c81d2f56-00d1-4424-a716-8e3c30e76636"/> + <img width="100" alt="Virtual LED Driver board" src="https://github.com/user-attachments/assets/98fb5010-7192-44db-a5c9-09602681ee15"/><img width="100" alt="Virtual LED Driver setup" src="https://github.com/user-attachments/assets/c81d2f56-00d1-4424-a716-8e3c30e76636"/>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/develop/architecture.mddocs/develop/drivers.mddocs/moonlight/drivers.mdmkdocs.ymlsrc/MoonBase/Module.hsrc/MoonBase/NodeManager.hsrc/MoonBase/Nodes.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Nodes/Drivers/parlio.hsrc/MoonLight/Nodes/Effects/E__Sandbox.hsrc/main.cpp
✅ Files skipped from review due to trivial changes (1)
- mkdocs.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Effects/E__Sandbox.h
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-08T11:39:52.878Z
Learning: In MoonLight WebSocket monitor: When browser tabs are hidden, browsers throttle JavaScript (including WebSocket onmessage handlers) but keep TCP connections open. Monitor data (48KB frames every 20-300ms) continues arriving at TCP level but isn't processed. This fills ESP32's httpd_ws_send_frame_async() buffers, causing ESP_FAIL. When tab becomes visible, hundreds of queued frames overwhelm the JavaScript main thread, freezing the UI. Solution: use Page Visibility API (client_visibility event) to stop emitting monitor data to hidden clients, preventing buffer overflow and backlog buildup.
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.hdocs/develop/architecture.mdsrc/main.cppsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonBase/NodeManager.hsrc/MoonLight/Layers/VirtualLayer.cppdocs/develop/drivers.mdsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonBase/Nodes.hsrc/MoonLight/Layers/PhysicalLayer.hsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.hdocs/moonlight/drivers.mdsrc/main.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonBase/Nodes.hsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-06T13:45:03.067Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.hdocs/develop/architecture.mdsrc/main.cppdocs/develop/drivers.mdsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.hsrc/main.cppsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonBase/Nodes.hsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-12-25T13:34:40.097Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Applied to files:
docs/moonlight/drivers.mdsrc/main.cppdocs/develop/drivers.mdsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/main.cppsrc/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} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/main.cpp
📚 Learning: 2026-01-06T15:24:11.096Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:11.096Z
Learning: In VirtualLayer.cpp (setLight and getLight), the unmapped path intentionally disables bounds checking when indexV >= mappingTableSize or oneToOneMapping is true to optimize performance for very large LED counts (100K+). This is a deliberate design decision; only rely on this behavior if you can guarantee calls stay within bounds. Document this contract and avoid adding defensive checks in the hot path. Use explicit bounds checks in all other code paths or when the input cannot be guaranteed to be in-range.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/PhysicalLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
docs/develop/drivers.mdsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In parlio.cpp's show_parlio function, the `components` parameter (color channels per pixel: 3 for RGB, 4 for RGBW) is guaranteed by API contract to never be 0. Invalid values are considered caller errors, and defensive checks are intentionally omitted for performance. This matches the design philosophy of placing responsibility on callers in performance-critical paths.
Applied to files:
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.hsrc/MoonLight/Nodes/Drivers/parlio.h
📚 Learning: 2026-01-12T17:15:32.074Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-12T17:15:32.074Z
Learning: In MoonLight's Monitor.ts (interface/src/routes/moonbase/monitor/monitor.ts), the camera distance calculation uses a padding multiplier in the formula: `cameraDistance = effectiveSize / (2 * Math.tan(fov / 2)) * padding`. A padding value of 5 (not 1.5) is needed because the effectiveSize calculation underestimates the actual visible model extent after scaling by [normalizedWidth, normalizedHeight, 1]. Larger padding values move the camera farther away, making the object smaller with more margin. Using 5 is safe and prevents clipping for all LED matrix aspect ratios.
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
🧬 Code graph analysis (4)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
src/MoonLight/Nodes/Drivers/parlio.cpp (1)
show_parlio(274-274)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonLight/Layers/PhysicalLayer.h (1)
PhysicalLayer(104-161)
src/MoonLight/Nodes/Drivers/parlio.h (1)
src/MoonLight/Nodes/Drivers/parlio.cpp (1)
show_parlio(274-274)
src/MoonLight/Layers/PhysicalLayer.h (1)
src/MoonLight/Layers/PhysicalLayer.cpp (2)
PhysicalLayer(26-37)PhysicalLayer(39-49)
🪛 Cppcheck (2.19.0)
src/MoonLight/Nodes/Drivers/parlio.cpp
[error] 415-415: Division by zero.
(zerodiv)
🪛 LanguageTool
docs/moonlight/drivers.md
[grammar] ~97-~97: Use a hyphen to join words.
Context: ...Controls.png"/> Sends Lights in Art-Net compatible packages to an Art-Net contro...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
35-35: Images should have alternate text (alt text)
(MD045, no-alt-text)
35-35: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
76-76: Images should have alternate text (alt text)
(MD045, no-alt-text)
76-76: Images should have alternate text (alt text)
(MD045, no-alt-text)
78-78: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
95-95: Images should have alternate text (alt text)
(MD045, no-alt-text)
99-99: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/develop/drivers.md
9-9: Link fragments should be valid
(MD051, link-fragments)
⏰ 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 (24)
src/MoonBase/Module.h (1)
122-136: LGTM on the getUpdate() pattern.The non-blocking mutex acquire with local copy pattern is well-designed:
- Uses timeout
0for polling without blocking the loop- Copies data locally before releasing mutex
- Processes callback outside the critical section
This minimizes mutex hold time and avoids potential deadlocks from callbacks.
src/MoonBase/Nodes.h (3)
75-78: LGTM on the layerMutex member addition.The new
layerMutexpointer member aligns with the project-wide migration from per-node to per-layer synchronization. Using a pointer allows all nodes in a layer to share the same mutex efficiently.
87-94: LGTM on constructor and destructor changes.The constructor now correctly accepts and stores the
layerMutexpointer, and the destructor is appropriately simplified since mutex ownership moved to the layer level.
151-156: Size 33 encoding forinttype is already properly implemented inupdateControl().The implementation in
src/MoonBase/Nodes.cpp(lines 38-40) correctly handles size 33 by casting the pointer asint*and assigning the control value, matching the encoding scheme inNodes.h.docs/develop/architecture.md (1)
84-93: LGTM on documentation updates.The architecture documentation accurately reflects the code changes, updating all references from
nodeMutextolayerMutexin the task synchronization descriptions.src/MoonLight/Modules/ModuleDrivers.h (2)
74-74: Intentional change to empty default node name.Setting
defaultNodeName = ""removes the automatic default layout. This appears intentional based on the PR objectives.
161-161: LGTM on passingdriversMutexto node constructor.The constructor call correctly passes
&layerP.driversMutex(address of theSemaphoreHandle_t), matching the newconst SemaphoreHandle_t*parameter expected byNode::constructor().src/MoonLight/Layers/VirtualLayer.cpp (2)
325-327: Good diagnostic logging addition.The added logging for
mappingTableallocation provides useful debugging information includingnrOfLights,sizeof(PhysMap), and total bytes. This will help diagnose memory allocation issues.
431-431: Good bounds safety fix.Changing the iteration bound from
nrOfLightstoMIN(nrOfLights, mappingTableSize)prevents potential out-of-bounds access whenmappingTableSizeis smaller thannrOfLights(e.g., if reallocation partially failed). This is a solid defensive improvement.src/MoonBase/NodeManager.h (1)
274-277: No null-check needed forlayerMutex.The code is safe as-is. The
constructor()method is always called immediately after node allocation (in bothModuleEffects::addNode()andModuleDrivers::addNode()), settinglayerMutexbefore the node is added to the vector. There is no path where a node can be in the vector without a validlayerMutexpointer.src/MoonLight/Modules/ModuleLightsControl.h (1)
503-520: LGTM! Appropriate use of portMUX_TYPE for short critical sections.Using
portENTER_CRITICAL/portEXIT_CRITICALwithportMUX_TYPEis well-suited for protecting single-variable access toisPositions. This reduces overhead compared to FreeRTOS semaphores for these brief critical sections. Based on learnings, this aligns with the goal of minimizing mutex hold time.src/MoonLight/Modules/ModuleEffects.h (1)
268-268: LGTM! Layer-level mutex consolidation.Passing
&layerP.effectsMutexto the node constructor centralizes synchronization at the layer level, reducing per-node semaphore overhead. This aligns with the PR's memory optimization goals for constrained devices.src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (1)
61-63: LGTM! Updated Parlio API call with per-output LED counts.The updated
show_parliocall correctly passes:
channelsPerLightdirectly (instead of boolean)ledsPerPinarray pointer for variable LEDs-per-pin support- New
offsetWhiteparameter for RGBW supportThis aligns with the Parlio API signature in
parlio.cpp.src/MoonLight/Layers/PhysicalLayer.cpp (2)
125-137: LGTM! Layer-level mutex synchronization for driver nodes.Using
*node->layerMutexcorrectly dereferences the pointer to the layer's shared mutex. This consolidates synchronization at the layer level and maintains proper protection foronSizeChangedandloopcalls.
160-163: LGTM! Conditional critical sections for double-buffer mode.Only entering critical sections when
useDoubleBufferis enabled avoids unnecessary overhead in single-buffer mode wherechannelsE == channelsD.docs/develop/drivers.md (1)
17-34: Good technical documentation of the Parlio padding mechanism.The documentation accurately describes the key functions, padding approach, and data flow. The code snippet correctly illustrates the conditional padding logic used in
transpose_32_slices.src/MoonLight/Nodes/Drivers/parlio.h (1)
17-18: LGTM! Updated Parlio API signature for variable LEDs-per-output.The signature correctly exposes:
components(3 for RGB, 4 for RGBW) instead of booleanleds_per_outputas pointer for per-pin LED countsoffsetWfor white channel offsetMatches the implementation in
parlio.cpp.docs/moonlight/drivers.md (1)
67-92: Good user documentation for the Parallel LED Driver.The new section comprehensively documents the Parlio driver's features including variable LEDs-per-strip, automatic padding, and memory efficiency. The feature list accurately reflects the implementation.
src/main.cpp (1)
113-113: Good change: portMUX_TYPE is appropriate for short critical sections.Switching from
SemaphoreHandle_ttoportMUX_TYPEreduces FreeRTOS object overhead and is the correct primitive for protecting brief shared-state access (pointer swaps, flag updates).src/MoonLight/Nodes/Drivers/parlio.cpp (5)
25-27: Per-output padding state tracked via globals - design acknowledged.The comment on line 25 correctly documents that these globals are safe because the driver runs on a dedicated core (APP_CPU). This is consistent with the project's architecture.
34-40: Elegant padding implementation in transpose logic.The ternary at line 40 (
pixel_in_pin < pixels_per_pin[pin] ? ... : 0) efficiently handles variable LEDs-per-pin by outputting black (0) for padded positions during the transpose phase. This avoids extra allocations and keeps the DMA buffer compact.
293-300: Per-output index calculation looks correct.The loop correctly computes
max_leds_per_outputas the maximum across all outputs and buildsfirst_index_per_outputas cumulative offsets into the input buffer. This enables the transpose logic to handle heterogeneous LED counts per pin.
409-415: Static analysis flags potential division by zero.Cppcheck reports division by zero at line 415 when
max_leds_per_chunkis 0. This occurs ifcomponents == 0(yieldingbytes_per_pixel == 0). Per learnings, the API contract guaranteescomponentsis 3 or 4, so this shouldn't trigger in practice. However, the static analysis tool lacks this context.If you want to silence the warning without runtime overhead, consider an early-return or assertion:
💡 Optional: Add assertion to document the contract
const uint32_t symbols_per_pixel = components * 32; + assert(components >= 3 && "components must be 3 (RGB) or 4 (RGBW)"); const uint32_t bits_per_pixel = symbols_per_pixel * parlio_config.data_width;Or if you prefer a soft guard:
const uint16_t max_leds_per_chunk = (bytes_per_pixel > 0) ? (HW_MAX_BYTES_PER_CHUNK / bytes_per_pixel) : 0; + if (max_leds_per_chunk == 0) return 1; // Invalid configuration const uint8_t num_chunks = (max_leds_per_output + max_leds_per_chunk - 1) / max_leds_per_chunk;
271-280: Good: Commented-out validation code documents the design assumption.The commented-out length check (lines 276-280) documents that the new per-output padding approach intentionally allows
length != outputs * max_leds_per_output. This is helpful for future maintainers.
|
|
||
| ### Parallel LED Driver Technical Implementation | ||
|
|
||
| For end-user documentation, see [Parallel LED Driver for ESP32-P4](#parallel-led-driver-for-esp32-p4). |
There was a problem hiding this comment.
Broken internal link fragment.
The anchor #parallel-led-driver-for-esp32-p4 doesn't exist in this document. Based on the docs/moonlight/drivers.md content, the correct link would be to that file's #parallel-led-driver section, or remove the link if it's meant to be internal.
Proposed fix
-For end-user documentation, see [Parallel LED Driver for ESP32-P4](#parallel-led-driver-for-esp32-p4).
+For end-user documentation, see [Parallel LED Driver](../../moonlight/drivers/#parallel-led-driver).🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
In @docs/develop/drivers.md at line 9, The link fragment
'#parallel-led-driver-for-esp32-p4' is invalid; edit the line that reads "For
end-user documentation, see [Parallel LED Driver for
ESP32-P4](#parallel-led-driver-for-esp32-p4)" and either change the fragment to
the correct anchor '#parallel-led-driver' (which points to the Parallel LED
Driver section in the moonlight drivers doc) or remove the internal link
entirely if it was not intended to be an anchor link.
| SemaphoreHandle_t effectsMutex = xSemaphoreCreateMutex(); | ||
| SemaphoreHandle_t driversMutex = xSemaphoreCreateMutex(); | ||
|
|
||
| PhysicalLayer(); | ||
| ~PhysicalLayer(); |
There was a problem hiding this comment.
Critical bug in destructor: inverted null-check causes memory leak.
The destructor in the corresponding .cpp file (from relevant code snippets) has inverted logic:
PhysicalLayer::~PhysicalLayer() {
if (!effectsMutex) { // BUG: should be `if (effectsMutex)`
vSemaphoreDelete(effectsMutex);
effectsMutex = NULL;
}
if (!driversMutex) { // BUG: should be `if (driversMutex)`
vSemaphoreDelete(driversMutex);
driversMutex = NULL;
}
}This means:
- When mutex is valid (non-null): skip deletion → memory leak
- When mutex is null: attempt to delete null → undefined behavior
The fix in PhysicalLayer.cpp:
PhysicalLayer::~PhysicalLayer() {
- if (!effectsMutex) {
+ if (effectsMutex) {
vSemaphoreDelete(effectsMutex);
effectsMutex = NULL;
}
- if (!driversMutex) {
+ if (driversMutex) {
vSemaphoreDelete(driversMutex);
driversMutex = NULL;
}
}🤖 Prompt for AI Agents
In @src/MoonLight/Layers/PhysicalLayer.h around lines 124 - 128, The destructor
PhysicalLayer::~PhysicalLayer() currently uses inverted null-checks causing
mutexes not to be deleted; change the conditions to check for non-null (i.e., if
(effectsMutex) and if (driversMutex)) before calling vSemaphoreDelete and then
set effectsMutex/driversMutex to NULL to avoid leaks and invalid deletes; ensure
you reference and update effectsMutex, driversMutex, vSemaphoreDelete and the
PhysicalLayer::~PhysicalLayer() implementation accordingly.
|
@coderabbitai, implemented the changes but get this: Guru Meditation Error: Core 1 panic'ed (Interrupt wdt timeout on CPU1). Core 1 register dump: Backtrace: 0x40090d1c:0x3ffed2f0 0x400918e1:0x3ffed310 0x4008fd65:0x3ffed330 0x40121389:0x3ffed370 0x401214ed:0x3ffed390 0x401158c6:0x3ffed3b0 0x4011806e:0x3ffed400 0x40090529:0x3ffed430 Core 0 register dump: Backtrace: 0x4008cf79:0x3ffe6de0 0x40090709:0x3ffe6e00 0x40117f09:0x3ffe6e30 0x40090529:0x3ffe6e50 ELF file SHA256: 060a1675c Rebooting... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6461 @ewowi, I found the root cause of the deadlock! The The Deadlock ChainCore 1 (driverTask):
Core 0 (effectTask):
Why This HappensWhen portENTER_CRITICAL(&swapMutex); // Line 131 - INTERRUPTS DISABLED
if (layerP.lights.header.isPositions == 0 && !newFrameReady) {
if (layerP.lights.useDoubleBuffer) {
portEXIT_CRITICAL(&swapMutex); // ❌ NOT EXECUTED when false
}
layerP.loop(); // ❌ RUNS WITH INTERRUPTS DISABLED
// Inside loop(): node->loop() → takes layerMutex semaphore
// ❌ CANNOT BLOCK WHILE HOLDING SPINLOCK!
// ...
}
portEXIT_CRITICAL(&swapMutex); // Finally releases, but too lateThe same issue in driverTask: portENTER_CRITICAL(&swapMutex); // Line 175
// ...
layerP.loopDrivers(); // Called while holding spinlock
// Inside: I2S driver calls xQueueSemaphoreTake() → DEADLOCKThe Fundamental Rule ViolatedYou CANNOT do ANY of these while holding a spinlock:
Spinlocks are ONLY for:
The Fix: Revert swapMutex to SemaphoreHandle_t// main.cpp - REVERT
// OLD (causes deadlock):
// portMUX_TYPE swapMutex = portMUX_INITIALIZER_UNLOCKED;
// NEW (safe):
SemaphoreHandle_t swapMutex = xSemaphoreCreateMutex();
// Revert all portENTER_CRITICAL/portEXIT_CRITICAL back to:
xSemaphoreTake(swapMutex, portMAX_DELAY);
// ... code ...
xSemaphoreGive(swapMutex);Also revert in:
Safe Mutex ConversionsCAN convert to portMUX_TYPE (saves 84 bytes each):
CANNOT convert to portMUX_TYPE:
Revised Memory Savings PlanKeep these optimizations:
Skip these for now:
Net savings: ~1,600 bytes (still substantial!) Would you like me to help identify the exact lines to revert in your code? 🐰✨ 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/MoonLight/Layers/PhysicalLayer.cpp:
- Around line 145-147: The code risks dereferencing null pointers when accessing
node->layerMutex and calling node->onLayout(); add the same defensive checks
used in loopDrivers: verify node is non-null and node->layerMutex is non-null
before calling xSemaphoreTake/xSemaphoreGive, and only call node->onLayout()
while the mutex is held; if any check fails, skip the semaphore and layout call
to avoid null deref. Ensure you reference the same symbols: node,
node->layerMutex, xSemaphoreTake, xSemaphoreGive, and node->onLayout().
🧹 Nitpick comments (3)
src/MoonBase/Module.h (2)
48-49: Consider usingxSemaphoreCreateMutex()for priority inheritance.The variable is named
updateMutexbutxSemaphoreCreateBinary()creates a binary semaphore, which lacks priority inheritance. For mutual exclusion with priority inheritance (avoiding priority inversion), usexSemaphoreCreateMutex()instead—it also starts in the "given" state, eliminating the need for the explicitxSemaphoreGive()in the constructor.♻️ Suggested change
- SemaphoreHandle_t updateMutex = xSemaphoreCreateBinary(); + SemaphoreHandle_t updateMutex = xSemaphoreCreateMutex(); volatile bool updatePending = false;Then remove the
xSemaphoreGive(updateMutex);call from the constructor (lines 55-56).
55-56: Clarifying comment would help future maintainers.The
xSemaphoreGive()call is required becausexSemaphoreCreateBinary()starts in the "taken" state. If you switch toxSemaphoreCreateMutex()as suggested above, this line can be removed. If kept, a brief comment explaining why would prevent confusion.src/MoonLight/Layers/PhysicalLayer.cpp (1)
34-37: Good diagnostic logging, but consider propagating failure state.The null checks provide useful diagnostics. However, if mutex creation fails, the code continues execution. Later calls to
xSemaphoreTake(*node->layerMutex, ...)with a null handle would cause undefined behavior.Consider either:
- Setting a failure flag that downstream code checks before using these mutexes, or
- Making this a fatal error if these mutexes are essential for correct operation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonBase/Module.hsrc/MoonLight/Layers/PhysicalLayer.cpp
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-08T11:39:52.878Z
Learning: In MoonLight WebSocket monitor: When browser tabs are hidden, browsers throttle JavaScript (including WebSocket onmessage handlers) but keep TCP connections open. Monitor data (48KB frames every 20-300ms) continues arriving at TCP level but isn't processed. This fills ESP32's httpd_ws_send_frame_async() buffers, causing ESP_FAIL. When tab becomes visible, hundreds of queued frames overwhelm the JavaScript main thread, freezing the UI. Solution: use Page Visibility API (client_visibility event) to stop emitting monitor data to hidden clients, preventing buffer overflow and backlog buildup.
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-06T13:45:03.067Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/PhysicalLayer.cpp
🧬 Code graph analysis (1)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonLight/Layers/PhysicalLayer.h (1)
PhysicalLayer(104-161)
⏰ 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)
src/MoonBase/Module.h (3)
90-90: LGTM with a minor note.The null check is good. Note that if any task is blocked on
updateMutexwhen this destructor runs,vSemaphoreDelete()behavior is undefined. Ensure module destruction only occurs after dependent tasks are stopped.
116-119: LGTM!The producer-side implementation is correct: acquire mutex, copy data, set flag, release mutex. The critical section is appropriately short.
124-138: LGTM!Well-structured consumer implementation:
- Non-blocking try prevents the loop from stalling
- Local copy before releasing mutex ensures data consistency
- Processing callback outside the critical section prevents deadlocks and long lock-hold times
src/MoonLight/Layers/PhysicalLayer.cpp (1)
39-48: LGTM!Proper cleanup with null guards. Since
layerPis a global object (line 24), the destructor runs during static destruction, which is acceptable for ESP32 firmware that typically doesn't terminate.
| xSemaphoreTake(*node->layerMutex, portMAX_DELAY); | ||
| node->onSizeChanged(prevSize); | ||
| xSemaphoreGive(node->nodeMutex); | ||
| xSemaphoreGive(*node->layerMutex); | ||
| } | ||
| if (node->on) { | ||
| xSemaphoreTake(node->nodeMutex, portMAX_DELAY); | ||
| xSemaphoreTake(*node->layerMutex, portMAX_DELAY); | ||
| node->loop(); | ||
| xSemaphoreGive(node->nodeMutex); | ||
| xSemaphoreGive(*node->layerMutex); |
There was a problem hiding this comment.
Potential null pointer dereference if mutex creation failed.
The code dereferences node->layerMutex without checking if the pointer or the underlying semaphore handle is valid. If effectsMutex or driversMutex creation failed (as logged in the constructor at lines 35-36), dereferencing will pass a null handle to xSemaphoreTake, causing undefined behavior.
Consider adding a guard:
Proposed defensive check
for (Node* node : nodes) {
if (prevSize != lights.header.size) {
+ if (node->layerMutex && *node->layerMutex) {
xSemaphoreTake(*node->layerMutex, portMAX_DELAY);
node->onSizeChanged(prevSize);
xSemaphoreGive(*node->layerMutex);
+ } else {
+ node->onSizeChanged(prevSize); // proceed without lock
+ }
}
if (node->on) {
+ if (node->layerMutex && *node->layerMutex) {
xSemaphoreTake(*node->layerMutex, portMAX_DELAY);
node->loop();
xSemaphoreGive(*node->layerMutex);
+ } else {
+ node->loop(); // proceed without lock
+ }
addYield(10);
}
}Alternatively, ensure mutex creation failure is fatal in the constructor.
|
@coderabbitai, for some reason there is not much more heap available on esp32-d0 (without param) due to the actions on mutexes we did (see above). Any idea why? Another possible improvement: the updateMutex is now created for each module while only one module at the time will be updated in most of the cases (and in the exceptional case there is more then one client it is no problem to wait a few ms more). So can we create a shared updateMutex for all the modules. Can you provide the code for that? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4253 @ewowi, I found the issue! The heap didn't improve much because you still have ~8+ updateMutex instances (one per module), consuming ~720 bytes. Here's why the previous savings aren't visible yet: Why Heap Didn't Improve
Solution: Shared updateMutex for All ModulesYou're absolutely right—only one module updates at a time in typical usage. Here's the implementation: Module.h Changes// Module.h
class ModuleState {
public:
JsonObject data = JsonObject();
UpdatedItem updatedItem;
// ✅ CHANGE: Make updateMutex static (shared by all modules)
static SemaphoreHandle_t updateMutex;
volatile bool updatePending = false; // Per-module flag (1 byte)
static Char<20> updateOriginId;
ModuleState() {
// ✅ CHANGE: Initialize shared mutex once
if (updateMutex == nullptr) {
updateMutex = xSemaphoreCreateBinary();
xSemaphoreGive(updateMutex); // Make it available
}
if (!gModulesDoc) {
EXT_LOGD(MB_TAG, "Creating doc");
gModulesDoc = new JsonDocument(&allocator);
}
}
~ModuleState() {
// ✅ CHANGE: Don't delete static mutex (shared resource)
// Cleanup happens once at shutdown if needed
}
std::function<void(const JsonArray& controls)> setupDefinition = nullptr;
std::function<void(const UpdatedItem& updatedItem)> processUpdatedItem = nullptr;
void postUpdate(const UpdatedItem& updatedItem) {
const char* taskName = pcTaskGetName(xTaskGetCurrentTaskHandle());
if (contains(taskName, "SvelteKit") || contains(taskName, "loopTask")) {
if (processUpdatedItem) processUpdatedItem(updatedItem);
} else {
// ✅ Uses shared mutex
if (xSemaphoreTake(updateMutex, portMAX_DELAY) == pdTRUE) {
this->updatedItem = updatedItem;
updatePending = true;
xSemaphoreGive(updateMutex);
}
}
}
void getUpdate() {
// ✅ Uses shared mutex
if (xSemaphoreTake(updateMutex, 0) == pdTRUE) {
if (updatePending) {
UpdatedItem localCopy = updatedItem;
updatePending = false;
xSemaphoreGive(updateMutex);
if (processUpdatedItem) processUpdatedItem(localCopy);
return;
}
xSemaphoreGive(updateMutex);
}
}
};
class Module : public StatefulService<ModuleState> {
// ... rest unchanged
};Module.cpp (or any .cpp file) - Define Static Member// Module.cpp (create this file if it doesn't exist)
#include "Module.h"
// ✅ Define static member
SemaphoreHandle_t ModuleState::updateMutex = nullptr;
Char<20> ModuleState::updateOriginId = ""; // Already static, keep definitionAlternative: If you don't want a separate Module.cpp, add to an existing file like // main.cpp (at top, after includes)
#include "MoonBase/Module.h"
// Define static members
SemaphoreHandle_t ModuleState::updateMutex = nullptr;Memory Savings SummaryBefore optimizations:
After all optimizations:
Net savings: 2,250 bytes! 🎉 Why You'll See Improvement After This Change
The key is that this change prevents future allocations rather than freeing existing ones. You'll see the improvement accumulate as:
Would you like me to prepare a complete code change for this optimization? 🐰✨ 🧠 Learnings used |
|
@coderabbitai, this is the situation before making updateMutex static: 
And this after 
Does this make sense? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, Yes, this makes perfect sense! 🎉 AnalysisHeap increased by 720 bytes (77648 - 76928 = 720), which means you successfully freed 720 bytes of memory. Why This Matches ExpectationsBefore optimization:
After optimization:
Your actual measurement shows 720 bytes freed, which suggests you had 8 module instances with individual mutexes that are now sharing a single mutex. Why Exactly 720 Bytes?The perfect match (720 bytes) indicates:
Cumulative Memory SavingsWith all optimizations combined:
This is a ~2% increase in available heap on ESP32 classic, which should help prevent the allocation failures you were experiencing! Excellent work! 🐰✨ 🧠 Learnings used |
|
@coderabbitai, can you make an analysis of class Module and class ModuleState , there are currently 8 modules, see moonbase and moonlight folder. Can we save more on memory in these classes, like making more variables static or shared classes. Also take a look at SharedEventPoint, SharedHttpEndpoint and SharedWebSocketServer, these are already shared versions of sveltekit classes for modules. There is also SharedFSPersistence, which is ready to be shared as well, there were some caveats back then, but can you also make an analysis with code suggestions how to share this between modules. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30241 @ewowi, excellent question! I've completed a comprehensive analysis of Module/ModuleState memory usage across all 8 modules. Here are significant optimization opportunities: Memory Analysis SummaryPer-Module Overhead (8 modules):
Total potential savings: 1,696 - 2,528 bytes Optimization 1: SharedFSPersistence Migration 🔥 (Saves ~800-1,600 bytes)The Problem: Each module has its own The Solution: Code ChangesModule.h - Remove per-module FSPersistence: class Module : public StatefulService<ModuleState> {
public:
String _moduleName = "";
bool requestUIUpdate = false;
Module(const String& moduleName, PsychicHttpServer* server, ESP32SvelteKit* sveltekit);
virtual void begin();
virtual void loop() {
_state.getUpdate();
if (requestUIUpdate) {
requestUIUpdate = false;
EXT_LOGD(ML_TAG, "requestUIUpdate");
update([&](ModuleState& state) { return StateUpdateResult::CHANGED; }, _moduleName + "server");
}
}
// ... rest of methods ...
protected:
EventSocket* _socket;
// ❌ REMOVE: FSPersistence<ModuleState> _fsPersistence;
// ✅ REMOVE: void readFromFS() { _fsPersistence.readFromFS(); }
private:
PsychicHttpServer* _server;
};Module.cpp - Update constructor: // OLD constructor body (remove _fsPersistence initialization):
Module::Module(const String& moduleName, PsychicHttpServer* server, ESP32SvelteKit* sveltekit)
: _moduleName(moduleName),
// _fsPersistence(StatefulService<ModuleState>::updateWithoutPropagation,
// StatefulService<ModuleState>::read,
// sveltekit->getFS(),
// ("/.config/" + moduleName + ".json").c_str()), // ❌ REMOVE
_server(server) {
// Setup handlers...
}main.cpp - Create shared instance and register modules: // Global shared persistence instance
SharedFSPersistence* sharedFSPersistence = nullptr;
void setup() {
// ... existing setup ...
// Create shared persistence service
sharedFSPersistence = new SharedFSPersistence(svelteKit.getFS());
// Register all modules with shared persistence
sharedFSPersistence->registerModule(&moonLightModule);
sharedFSPersistence->registerModule(&lightsModule);
sharedFSPersistence->registerModule(&channelsModule);
sharedFSPersistence->registerModule(&devicesModule);
sharedFSPersistence->registerModule(&ioModule);
sharedFSPersistence->registerModule(&tasksModule);
sharedFSPersistence->registerModule(&liveScriptsModule, true); // delayed writing
// ... register other modules ...
sharedFSPersistence->begin();
// ... rest of setup ...
}FileManager.cpp - Update delayed writes call: // OLD:
FSPersistence<int>::writeToFSDelayed('W');
// NEW:
SharedFSPersistence::writeToFSDelayed('W');Optimization 2: Shared UpdatedItem Buffer
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonBase/Module.cpp (1)
307-313: Bug: Invalid.c_str()call onconst char*.At Line 313,
moduleNameis nowconst char*, but the code callsmoduleName.c_str()which is invalid syntax for a raw pointer.🐛 Proposed fix
- EXT_LOGV(MB_TAG, "constructor %s", moduleName.c_str()); + EXT_LOGV(MB_TAG, "constructor %s", moduleName);src/MoonBase/SharedFSPersistence.h (1)
182-186: Bug:doc.as<JsonObject>()on empty document returns null.
applyDefaultscreates an emptyJsonDocumentand callsdoc.as<JsonObject>(), which returns a null/invalidJsonObjectsince nothing has been added to the document. This will causeupdateWithoutPropagationto receive an empty/null object.🐛 Proposed fix
void applyDefaults(ModuleInfo& info) { JsonDocument doc; - JsonObject obj = doc.as<JsonObject>(); + JsonObject obj = doc.to<JsonObject>(); info.module->updateWithoutPropagation(obj, ModuleState::update); }
🤖 Fix all issues with AI agents
In @src/MoonBase/SharedFSPersistence.h:
- Line 35: The map _modules is declared as std::map<const char*, ModuleInfo>
which compares pointer addresses not string contents; change the key type to
std::string (e.g., std::map<std::string, ModuleInfo> _modules) and update all
usages (calls like _modules.find(moduleName), _modules[module->_moduleName],
inserts, and lookups) to pass or construct std::string (e.g.,
_modules[std::string(module->_moduleName)] or rely on implicit conversion) so
lookups compare string content correctly; alternatively implement a custom
comparator for const char* only if you must keep pointer type, but prefer
switching to std::string for _modules and any functions that access it.
🧹 Nitpick comments (1)
src/MoonBase/Module.h (1)
56-59: Consider usingxSemaphoreCreateMutexinstead of binary semaphore.For mutual exclusion,
xSemaphoreCreateMutex()is the idiomatic choice as it provides priority inheritance to prevent priority inversion. Binary semaphores are typically used for signaling between tasks rather than mutual exclusion.♻️ Suggested change
if (updateMutex == nullptr) { - updateMutex = xSemaphoreCreateBinary(); - xSemaphoreGive(updateMutex); // Make it available + updateMutex = xSemaphoreCreateMutex(); + if (updateMutex == nullptr) { + EXT_LOGE(MB_TAG, "Failed to create updateMutex"); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Modules/ModuleTasks.hsrc/MoonBase/NodeManager.hsrc/MoonBase/SharedEventEndpoint.hsrc/MoonBase/SharedFSPersistence.hsrc/MoonBase/SharedHttpEndpoint.hsrc/MoonBase/SharedWebSocketServer.hsrc/MoonLight/Modules/ModuleChannels.hsrc/MoonLight/Modules/ModuleLightsControl.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-08T11:39:52.878Z
Learning: In MoonLight WebSocket monitor: When browser tabs are hidden, browsers throttle JavaScript (including WebSocket onmessage handlers) but keep TCP connections open. Monitor data (48KB frames every 20-300ms) continues arriving at TCP level but isn't processed. This fills ESP32's httpd_ws_send_frame_async() buffers, causing ESP_FAIL. When tab becomes visible, hundreds of queued frames overwhelm the JavaScript main thread, freezing the UI. Solution: use Page Visibility API (client_visibility event) to stop emitting monitor data to hidden clients, preventing buffer overflow and backlog buildup.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/PsychicHttp/**/*.{h,cpp} : Use the custom PsychicHttp library for HTTP server capabilities, request/response handling, WebSockets, and file serving
Applied to files:
src/MoonBase/SharedWebSocketServer.h
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/MoonBase/Module.hsrc/MoonBase/NodeManager.h
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
📚 Learning: 2026-01-06T15:24:29.770Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
📚 Learning: 2026-01-08T11:39:52.878Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-08T11:39:52.878Z
Learning: In MoonLight WebSocket monitor: When browser tabs are hidden, browsers throttle JavaScript (including WebSocket onmessage handlers) but keep TCP connections open. Monitor data (48KB frames every 20-300ms) continues arriving at TCP level but isn't processed. This fills ESP32's httpd_ws_send_frame_async() buffers, causing ESP_FAIL. When tab becomes visible, hundreds of queued frames overwhelm the JavaScript main thread, freezing the UI. Solution: use Page Visibility API (client_visibility event) to stop emitting monitor data to hidden clients, preventing buffer overflow and backlog buildup.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (9)
src/MoonBase/Modules/ModuleIO.h (2)
lib/framework/StatefulService.h (2)
update(123-134)update(144-155)src/MoonBase/Module.h (1)
ModuleState(43-141)
src/MoonLight/Modules/ModuleChannels.h (3)
src/MoonBase/Module.cpp (2)
update(274-305)update(274-274)src/MoonBase/Modules/FileManager.cpp (2)
update(58-139)update(58-58)src/MoonBase/Module.h (1)
ModuleState(43-141)
src/MoonBase/Modules/ModuleDevices.h (2)
src/MoonBase/Module.cpp (2)
update(274-305)update(274-274)src/MoonBase/Module.h (1)
ModuleState(43-141)
src/MoonBase/Module.cpp (2)
src/MoonBase/Module.h (1)
Module(143-210)lib/framework/StatefulService.h (4)
read(165-170)read(172-177)update(123-134)update(144-155)
src/MoonBase/SharedWebSocketServer.h (1)
lib/framework/WebSocketServer.h (1)
transmitData(137-155)
src/MoonBase/Module.h (1)
src/MoonBase/Module.cpp (1)
Module(307-319)
src/MoonBase/NodeManager.h (2)
src/MoonBase/Module.cpp (1)
Module(307-319)src/MoonBase/Module.h (1)
Module(143-210)
src/MoonBase/Modules/ModuleTasks.h (2)
src/MoonBase/Module.cpp (2)
update(274-305)update(274-274)src/MoonBase/Module.h (1)
ModuleState(43-141)
src/MoonBase/SharedFSPersistence.h (2)
lib/framework/FSPersistence.h (3)
writeToFS(94-118)readFromFS(44-67)writeToFSNow(69-92)src/MoonBase/Module.h (1)
readFromFS(203-205)
⏰ 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 (25)
src/MoonBase/Modules/ModuleIO.h (2)
549-549: LGTM!The explicit
String(_moduleName)wrapping ensures correct string concatenation since_moduleNameis aconst char*. This matches the expectedconst String¶meter type forupdate()and is consistent with the pattern applied across other modules in this PR.
578-578: LGTM!Consistent application of the
String(_moduleName)wrapping pattern for the conditional update call.src/MoonBase/Modules/ModuleTasks.h (1)
132-132: LGTM!Correct
String(_moduleName)wrapping for the update call, consistent with the codebase-wide pattern change.src/MoonBase/Modules/ModuleDevices.h (1)
117-117: LGTM!Correct
String(_moduleName)wrapping for the update call inupdateDevices(), consistent with the codebase-wide pattern change.src/MoonLight/Modules/ModuleChannels.h (1)
71-71: LGTM!Correct
String(_moduleName)wrapping for the update call inonUpdate(), consistent with the codebase-wide pattern change.src/MoonBase/SharedHttpEndpoint.h (2)
33-33: LGTM!Correct
String(module->_moduleName)wrapping for path construction. Since_moduleNameis nowconst char*, explicit String conversion is required for proper concatenation with the string literal"/rest/".
91-91: LGTM!The
endsWith()method on ArduinoStringacceptsconst char*directly, so no explicitString()wrapping is needed here. This is more efficient than the previous.c_str()call when_moduleNamewas aStringtype.src/MoonBase/SharedWebSocketServer.h (2)
34-36: LGTM!The changes correctly adapt to
_moduleNamebeingconst char*instead ofString. The logging and path construction usingString()wrapper are appropriate.
120-120: LGTM!The
path.endsWith(module->_moduleName)works correctly sinceString::endsWith()acceptsconst char*.src/MoonLight/Modules/ModuleLightsControl.h (5)
468-468: LGTM!The
String(_moduleName) + "server"pattern correctly handles theconst char*toStringconversion for the origin ID.
482-482: LGTM!Consistent string handling pattern for button-triggered updates.
495-495: LGTM!Consistent string handling pattern for toggle button updates.
513-514: LGTM!Direct usage of
_moduleName(nowconst char*) foremitEventis correct since the method accepts C-strings.
527-527: LGTM!Consistent with other
emitEventcalls using theconst char*module name directly.src/MoonBase/SharedEventEndpoint.h (2)
28-41: LGTM!The event registration correctly uses
const char*for event names. ArduinoJson handlesconst char*assignments appropriately, and the update handler pattern is consistent with the rest of the codebase.
48-57: LGTM!The
syncStatemethod correctly assignsmodule->_moduleNameto the JSON document and uses the module's read method for state serialization.src/MoonBase/NodeManager.h (2)
30-34: LGTM!Constructor signature correctly updated to accept
const char* moduleName, aligning with theModulebase class refactor.
273-276: LGTM!The synchronization change from per-node
nodeMutexto layer-level*nodeClass->layerMutexcorrectly consolidates mutex usage as intended by this PR. The dereference is appropriate sinceNode::layerMutexis aconst SemaphoreHandle_t*. Based on learnings, these mutexes are validated at creation time, so runtime null checks are intentionally omitted.src/MoonBase/Module.cpp (2)
38-41: LGTM!Static member definitions for the shared
updateMutexandupdatedItemare correctly placed here. This consolidates per-module semaphores into a single shared mutex, reducing memory usage on constrained devices.
333-344: LGTM!Server route construction correctly wraps
_moduleNamewithString()for concatenation.src/MoonBase/Module.h (5)
47-49: Design concern: StaticupdatedItemwith instance-levelupdatePendingmay cause issues.
updatedItemis static (shared across allModuleStateinstances), butupdatePendingis an instance member. This creates a potential mismatch:
- Module A's
postUpdatesets staticupdatedItemand sets A'supdatePending = true- Module B's
getUpdatechecks B'supdatePending(which isfalse), so it skips processing- Module A's
getUpdateprocesses correctlyThis seems intentional (each module tracks its own pending state), but if two modules post updates in quick succession before either processes them, the second will overwrite the static
updatedItem, and the first module will process the wrong data.If this is the intended design (only one update can be pending system-wide), consider making
updatePendingstatic as well, or document this constraint clearly.
117-123: LGTM!The
postUpdatemethod correctly acquires the mutex before modifying shared state and releases it afterward. The direct callback path for SvelteKit/loopTask avoids unnecessary synchronization overhead.
125-140: LGTM!The non-blocking
getUpdatewith timeout 0 is appropriate for a loop context—it avoids blocking the main loop while still processing pending updates when available. Processing the callback outside the mutex is correct and prevents potential deadlocks.
145-148: LGTM!The change from
Stringtoconst char*for_moduleNamereduces heap allocations. The constructor signature update is consistent with the implementation inModule.cpp.
168-168: LGTM!The
String(_moduleName) + "server"pattern correctly handles theconst char*toStringconversion for constructing the origin ID.
src/MoonBase/SharedFSPersistence.h
Outdated
| ModuleInfo() : module(nullptr), delayedWriting(false), hasDelayedWrite(false), updateHandlerId(0) {} | ||
| }; | ||
| std::map<String, ModuleInfo> _modules; // moduleName -> info | ||
| std::map<const char *, ModuleInfo> _modules; // moduleName -> info |
There was a problem hiding this comment.
Critical: std::map<const char*, ...> uses pointer comparison, not string comparison.
Using const char* as a std::map key compares pointer addresses, not string contents. If two different const char* pointers contain the same string (e.g., "lightscontrol"), they won't match in the map lookup unless they point to the exact same memory address.
This will cause _modules.find(moduleName) to fail unexpectedly when the pointer passed differs from the one stored, even if the string content is identical.
🐛 Proposed fix: Use `std::string` or a custom comparator
Option 1: Use std::string as key (recommended)
- std::map<const char *, ModuleInfo> _modules; // moduleName -> info
+ std::map<std::string, ModuleInfo> _modules; // moduleName -> infoThen update all _modules[module->_moduleName] to _modules[std::string(module->_moduleName)] or simply _modules[module->_moduleName] (implicit conversion).
Option 2: Use a custom comparator
+ struct CStrComparator {
+ bool operator()(const char* a, const char* b) const {
+ return strcmp(a, b) < 0;
+ }
+ };
- std::map<const char *, ModuleInfo> _modules;
+ std::map<const char*, ModuleInfo, CStrComparator> _modules;📝 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.
| std::map<const char *, ModuleInfo> _modules; // moduleName -> info | |
| std::map<std::string, ModuleInfo> _modules; // moduleName -> info |
🤖 Prompt for AI Agents
In @src/MoonBase/SharedFSPersistence.h at line 35, The map _modules is declared
as std::map<const char*, ModuleInfo> which compares pointer addresses not string
contents; change the key type to std::string (e.g., std::map<std::string,
ModuleInfo> _modules) and update all usages (calls like
_modules.find(moduleName), _modules[module->_moduleName], inserts, and lookups)
to pass or construct std::string (e.g.,
_modules[std::string(module->_moduleName)] or rely on implicit conversion) so
lookups compare string content correctly; alternatively implement a custom
comparator for const char* only if you must keep pointer type, but prefer
switching to std::string for _modules and any functions that access it.
|
@coderabbitai, can you make an analysis _moduleName is implemented rightly, in other words const char * never has a nullptr or invalid pointer? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17899 @ewowi, short answer: yes, with one tiny fix and a small safeguard, _moduleName stays valid and never becomes nullptr in the current code. What’s safe today
Two concrete fixes to make it airtight
Notes and optional improvement
Verdict
If you want, I can prepare a tiny patch with the logging fix and the constructor guard. 🧠 Learnings used |
|
@coderabbitai, Yes do it |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ewowi: Got it — I’ll prepare the small, safe patch for Module and ModuleState you approved: Plan
Proposed diffs
If you want, I can also include a quick scan-and-fix for any stray moduleName.c_str() and _moduleName + "server" occurrences outside Module.* in the same PR. 🏁 Script executed: Length of output: 2420 @ewowi: Applying now. I’ll prepare a minimal, safe patch that guarantees _moduleName is never null/empty and fixes the logging. Another agent will execute the plan below and open a patch for review. [plan_code_changes]
If you’d like, I can also scan the repo and patch any remaining occurrences of moduleName.c_str() or raw _moduleName + "server" in a follow-up step. 🧠 Learnings used |
|
@coderabbitai, should we change originId from String to const char * as than no String(moduleName) is needed |
|
Tip For best results, initiate chat on the files or code changes. @ewowi Short answer: I don’t recommend changing originId to const char*. Why
Also, changing originId won’t eliminate String(_moduleName) at the call sites that build paths/topics (e.g., String(_moduleName) + "server"). Those concatenations still need a transient String regardless of originId’s type. Recommendation
If you want, I can scan for all originId assignments to ensure we always copy into the Char<20> buffer (no borrowed pointers). 🧠 Learnings used |
- Module: updateMutex Binary to Mutex, _modules add CStrComparator
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/SharedFSPersistence.h (1)
187-191: Bug:applyDefaultscreates a null JsonObject instead of an empty one.
doc.as<JsonObject>()on a freshJsonDocumentreturns a null/invalid object because nothing has been added yet. Usedoc.to<JsonObject>()to create an empty object.🐛 Fix to create a valid empty object
void applyDefaults(ModuleInfo& info) { JsonDocument doc; - JsonObject obj = doc.as<JsonObject>(); + JsonObject obj = doc.to<JsonObject>(); info.module->updateWithoutPropagation(obj, ModuleState::update); }
🤖 Fix all issues with AI agents
In @src/MoonBase/Module.h:
- Around line 47-49: The static updatedItem and updateMutex are shared across
ModuleState instances while updatePending is per-instance, causing
lost/overwritten updates; fix by making updatePending static (declare it as
static volatile bool updatePending) so it reflects the shared state, and ensure
all accesses in functions that post or consume updates (e.g., postUpdate(),
getUpdate()) are guarded by updateMutex to avoid races, or alternatively
implement a per-instance update buffer/queue (e.g., per-instance
std::optional/queue of UpdatedItem with its own mutex) if modules must maintain
independent pending updates.
- Around line 55-59: The ModuleState constructor incorrectly calls
xSemaphoreGive(updateMutex) even though updateMutex is a static mutex created
once via xSemaphoreCreateMutex and is already in the available state; remove the
xSemaphoreGive call from the ModuleState constructor (keep the null check and
the EXT_LOGE(MB_TAG, "Failed to create updateMutex") path) so the constructor
only validates updateMutex != nullptr and does not alter the mutex state across
multiple Module instantiations.
In @src/MoonBase/SharedFSPersistence.h:
- Around line 35-40: The applyDefaults function creates an invalid JsonObject by
calling doc.as<JsonObject>(); change it to initialize a proper empty JsonObject
using doc.to<JsonObject>() so the view is valid before passing it to
info.module->updateWithoutPropagation(info, ModuleState::update); update the
applyDefaults implementation to use JsonDocument doc; JsonObject obj =
doc.to<JsonObject>(); and then call info.module->updateWithoutPropagation(obj,
ModuleState::update); ensuring ModuleInfo, updateWithoutPropagation,
ModuleState::update and JsonDocument/JsonObject are the referenced symbols.
🧹 Nitpick comments (1)
src/MoonBase/Module.cpp (1)
38-41: Good: Shared static mutex reduces per-module memory overhead.The static
updateMutexandupdatedItemconsolidate synchronization across modules, reducing heap usage from per-module semaphores.Minor nit: Line 39 has a trailing double semicolon (
;;).🔧 Remove extra semicolon
-SemaphoreHandle_t ModuleState::updateMutex = xSemaphoreCreateMutex();; +SemaphoreHandle_t ModuleState::updateMutex = xSemaphoreCreateMutex();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platformio.inisrc/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/NodeManager.hsrc/MoonBase/SharedFSPersistence.hsrc/MoonBase/SharedHttpEndpoint.hsrc/MoonBase/SharedWebSocketServer.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonBase/SharedHttpEndpoint.h
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/MoonBase/NodeManager.hsrc/MoonBase/Module.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
platformio.ini
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/PsychicHttp/**/*.{h,cpp} : Use the custom PsychicHttp library for HTTP server capabilities, request/response handling, WebSockets, and file serving
Applied to files:
src/MoonBase/SharedWebSocketServer.h
🧬 Code graph analysis (4)
src/MoonBase/NodeManager.h (2)
src/MoonBase/Module.cpp (1)
Module(307-319)src/MoonBase/Module.h (1)
Module(143-210)
src/MoonBase/Module.h (1)
src/MoonBase/Module.cpp (1)
Module(307-319)
src/MoonBase/SharedWebSocketServer.h (1)
lib/framework/WebSocketServer.h (1)
transmitData(137-155)
src/MoonBase/SharedFSPersistence.h (1)
lib/framework/FSPersistence.h (3)
writeToFS(94-118)readFromFS(44-67)writeToFSNow(69-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
platformio.ini (1)
59-59: LGTM!Build date updated to reflect the new release with the mutex and module name changes.
src/MoonBase/SharedWebSocketServer.h (2)
34-36: LGTM!The logging and path construction correctly adapt to
_moduleNamebeingconst char*. TheString("/rest/") + module->_moduleNamepattern properly handles the concatenation.
120-120: LGTM!
String::endsWith(const char*)is a valid overload; this works correctly with the newconst char*type.src/MoonBase/NodeManager.h (2)
30-31: LGTM!Constructor signature correctly updated to accept
const char*, consistent with theModulebase class changes.
273-276: Migration from per-node to layer-level mutex looks correct.The change from
nodeMutextolayerMutexconsolidates synchronization at the layer level as intended by this PR. The dereference*nodeClass->layerMutexis correct sincelayerMutexis aconst SemaphoreHandle_t*.Based on learnings, these mutex pointers are validated at creation time and runtime null checks are intentionally omitted (fail-fast at boot approach). Ensure that
updateControlandonUpdatedo not perform blocking operations (I/O, delays, driver calls) while holding this mutex.src/MoonBase/Module.cpp (2)
307-313: LGTM!Good defensive handling:
- Null/empty check with fallback to
"unnamed"prevents crashes from invalid module names- Path construction correctly uses
String("/.config/") + moduleName + ".json"for proper concatenation- Logging correctly passes
const char*to%sformat specifier
333-333: LGTM!Server route path construction correctly updated to use
String("/rest/") + _moduleName + "Def".src/MoonBase/SharedFSPersistence.h (3)
46-60: LGTM: Module registration logic.The registration correctly:
- Constructs the file path
- Sets up delayed write tracking
- Registers an update handler
- Reads initial state from filesystem
The lambda captures
moduleby value (pointer copy), which is appropriate.
67-80: LGTM!Enable/disable update handler methods correctly updated for
const char*parameter type.
106-139: LGTM: Delayed write implementation.The delayed write logic correctly:
- Enqueues a write function when
delayedWritingis enabled- Prevents duplicate entries with
hasDelayedWriteflag- Handles both write ('W') and cancel cases
- Restores state from FS on cancel and notifies UI
src/MoonBase/Module.h (3)
111-123: Mutex handling inpostUpdateis correct.The acquire/release pattern is properly balanced. The earlier flagged issue about static
updatedItemvs per-instanceupdatePendingapplies here but the synchronization mechanics themselves are sound.
125-140: Non-blockinggetUpdatewith callback processing outside the mutex is well-designed.The pattern of copying data under the lock, releasing, then processing is the correct approach to minimize mutex hold time. Both code paths correctly release the mutex.
145-148: Switching_moduleNametoconst char*is memory-efficient and safely implemented.The change from
Stringtoconst char*reduces heap allocations. The constructor properly guards against null/empty values (Module.cpp line 311), and all callers throughout the codebase pass string literals ("lightscontrol", "channels", "effects", "drivers", etc.) which have static lifetime. No temporary pointers or dangling references exist. The implementation is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Module.h (1)
110-122: Single pending update may cause lost updates.The static
updatedItemcan only hold one pending update at a time. If multiple updates arrive from non-SvelteKit/loopTask contexts beforegetUpdate()is called, earlier updates are silently overwritten.This may be acceptable if:
- Updates from external tasks are rare
- Only the latest state matters (eventual consistency)
However, if updates carry semantic meaning that must not be lost (e.g., swap operations), consider using a queue instead of a single item, or document this as an intentional design choice.
🤖 Fix all issues with AI agents
In @src/MoonBase/SharedFSPersistence.h:
- Around line 35-40: The map _modules uses const char* keys with CStrComparator
and relies on all Module instances supplying string-literal names; update the
code comments near CStrComparator and the declaration of _modules to clearly
state the invariant that keys must point to static-storage string literals (and
that Module's constructor assigns _moduleName by pointer without copying), warn
that passing dynamic strings will produce dangling keys, and add a brief note
advising future contributors to either pass string literals or change the map to
use std::string keys (or copy the name in Module) if dynamic names are needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
interface/src/routes/moonbase/filemanager/FileManager.sveltesrc/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/SharedFSPersistence.h
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
📚 Learning: 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/moonbase/filemanager/FileManager.svelte
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/interface/**/*.{svelte,ts,tsx,js,json} : Modify frontend code in the `interface/` directory using SvelteKit
Applied to files:
interface/src/routes/moonbase/filemanager/FileManager.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/routes/**/*.svelte : Create Svelte routes in `interface/src/routes/` for different pages of the web application
Applied to files:
interface/src/routes/moonbase/filemanager/FileManager.svelte
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/MoonBase/Module.h
🧬 Code graph analysis (2)
src/MoonBase/Module.cpp (2)
src/MoonBase/Module.h (1)
Module(142-209)lib/framework/StatefulService.h (4)
read(165-170)read(172-177)update(123-134)update(144-155)
src/MoonBase/SharedFSPersistence.h (2)
lib/framework/FSPersistence.h (3)
writeToFS(94-118)readFromFS(44-67)writeToFSNow(69-92)src/MoonBase/Module.h (1)
readFromFS(202-204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
interface/src/routes/moonbase/filemanager/FileManager.svelte (1)
37-37: LGTM!Correct fix removing the extraneous semicolon from the
$state({})initialization.src/MoonBase/SharedFSPersistence.h (5)
46-60: LGTM!The
registerModuleimplementation correctly builds the file path usingStringconcatenation, captures the module pointer appropriately in the lambda, and stores the module info. The update handler registration follows the expected pattern.
67-80: LGTM!The enable/disable handler methods correctly use
const char*parameters and properly look up modules in the map. The lambda capture inenableUpdateHandlercorrectly captures the module pointer.
106-139: LGTM!The
writeToFSimplementation correctly handles delayed writes, matching the pattern fromFSPersistence.h. The lambda defensively re-looks up the module in case it was unregistered between scheduling and execution.
141-162: LGTM!The
writeToFSNowmethod correctly creates directories, serializes module state to JSON, and writes to the filesystem. The use ofdoc.to<JsonObject>()is correct for ArduinoJson 7.x.
186-191: LGTM!Using
doc.to<JsonObject>()is correct here since the document is newly created and empty. This properly initializes the JsonObject before passing toupdateWithoutPropagation.src/MoonBase/Module.cpp (4)
38-42: LGTM on static synchronization primitives.The static mutex and shared state variables are correctly initialized. The mutex is created once at program startup via static initialization, which aligns with the project's "fail-fast at boot" philosophy mentioned in the learnings. The
updatePendingflag and sharedupdatedItemenable the consolidated update signaling pattern.
312-314: Good defensive guard for null/empty module name.The null and empty string check on line 312 is a good defensive measure that prevents issues with invalid module names. The fallback to
"unnamed"ensures the module always has a valid identifier.
334-345: LGTM!The REST endpoint registration uses the same string construction pattern. The
PsychicHttpServer::on()method typically copies the path internally, making this safe.
308-320: This concern is not valid —FSPersistencesafely copies the file path.The
_filePathmember is declared asString(line 153), notconst char*. The constructor initializes it with_filePath(filePath)(line 37), which invokes theStringconstructor. The ArduinoStringclass's constructor acceptsconst char*and copies the string data into its own managed buffer. Although the temporaryStringfrom line 310 of Module.cpp is destroyed after the expression,FSPersistencealready owns a deep copy via itsStringmember. No dangling pointer exists.Likely an incorrect or invalid review comment.
src/MoonBase/Module.h (3)
47-58: LGTM on static synchronization members.The static members are properly declared here and defined in Module.cpp. The constructor's null check on
updateMutexfollows the project's "fail-fast at boot" pattern from the learnings—logging an error if creation failed but not blocking runtime checks. This is intentional per the project's design philosophy.
124-139: LGTM on getUpdate implementation.The non-blocking mutex acquisition (
timeout 0) prevents the loop task from blocking. The pattern of copying the update data before releasing the mutex, then processing outside the critical section, is correct and avoids holding the lock during potentially slow callbacks. This enables proper producer-consumer decoupling.
142-169: LGTM on Module class API changes.Changing
_moduleNametoconst char*reduces heap overhead compared toString, which aligns with the PR's memory optimization goals for no-PSRAM devices. The explicitString(_moduleName)conversion on line 167 correctly handles the concatenation for the update origin ID.
| struct CStrComparator { | ||
| bool operator()(const char* a, const char* b) const { | ||
| return strcmp(a, b) < 0; | ||
| } | ||
| }; | ||
| std::map<const char *, ModuleInfo, CStrComparator> _modules; // moduleName -> info |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the SharedFSPersistence.h file to understand the module management
rg -n "registerModule|unregisterModule|_modules" src/MoonBase/SharedFSPersistence.h -A 5Repository: MoonModules/MoonLight
Length of output: 2456
🏁 Script executed:
# Check the module structure and _moduleName definition
rg -n "struct.*Module|class.*Module|_moduleName" src/MoonBase/SharedFSPersistence.h -B 2 -A 3Repository: MoonModules/MoonLight
Length of output: 1794
🏁 Script executed:
# Look for module lifecycle - are modules ever removed?
rg -n "\.erase\(|\.clear\(\)|_modules\[" src/MoonBase/SharedFSPersistence.h -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 400
🏁 Script executed:
# Search across the codebase for ModuleInfo and module management patterns
fd . src/MoonBase -type f \( -name "*.h" -o -name "*.cpp" \) | head -20Repository: MoonModules/MoonLight
Length of output: 235
🏁 Script executed:
# Search for unregisterModule or any cleanup patterns
rg -n "unregisterModule|erase.*_modules|_modules.*erase" --type-list | head -5Repository: MoonModules/MoonLight
Length of output: 454
🏁 Script executed:
# Search for Module class definition and _moduleName
rg -n "class Module|struct Module" src/ -A 10 | head -40Repository: MoonModules/MoonLight
Length of output: 2589
🏁 Script executed:
# Check if there's any code that removes entries from _modules
rg -n "\.erase|\.clear" src/MoonBase/SharedFSPersistence.hRepository: MoonModules/MoonLight
Length of output: 101
🏁 Script executed:
# Look for Module.h file
fd "Module\.h" src/Repository: MoonModules/MoonLight
Length of output: 86
🏁 Script executed:
# Search for _moduleName declaration to understand its type and storage
rg -n "_moduleName" src/ | head -20Repository: MoonModules/MoonLight
Length of output: 2267
🏁 Script executed:
# Check Module constructor implementation
rg -n "Module::Module|_moduleName =" src/MoonBase/Module.h src/MoonBase/Module.cpp -A 5 | head -50Repository: MoonModules/MoonLight
Length of output: 1301
🏁 Script executed:
# Verify if _moduleName is ever reassigned after construction
rg -n "_moduleName\s*=" src/MoonBase/ -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 771
🏁 Script executed:
# Check the complete Module.h to understand _moduleName lifecycle
sed -n '140,160p' src/MoonBase/Module.hRepository: MoonModules/MoonLight
Length of output: 737
🏁 Script executed:
# Verify if any Module subclasses use non-literal strings
rg -n "Module(" src/MoonBase/ | grep -v "Module\(" | head -20Repository: MoonModules/MoonLight
Length of output: 153
🏁 Script executed:
# Check all Module instantiations to confirm string literal pattern
rg -n "Module\(" src/ | grep ":" | head -30Repository: MoonModules/MoonLight
Length of output: 2869
🏁 Script executed:
# Check NodeManager instantiations to see if dynamic strings are used
rg -n "NodeManager(" src/ -B 2 -A 2 | head -30Repository: MoonModules/MoonLight
Length of output: 139
The const char* map keys are safe due to current usage patterns, but the design is fragile.
The _modules map stores const char* pointers to module->_moduleName as keys. While this works correctly in practice, the design relies on a critical invariant: all modules must use string literal names. The Module constructor (Module.cpp:312) assigns _moduleName directly from the parameter without copying—_moduleName = (moduleName && moduleName[0] != '\0') ? moduleName : "unnamed". If a dynamic string is passed, the map key becomes a dangling pointer.
Current safety is guaranteed because all Module subclass instantiations use string literals (Module("tasks", ...), Module("channels", ...), etc.), which have static storage duration. Additionally, modules are never removed from the _modules map during runtime.
Document this string-literal invariant to prevent future regressions.
🤖 Prompt for AI Agents
In @src/MoonBase/SharedFSPersistence.h around lines 35 - 40, The map _modules
uses const char* keys with CStrComparator and relies on all Module instances
supplying string-literal names; update the code comments near CStrComparator and
the declaration of _modules to clearly state the invariant that keys must point
to static-storage string literals (and that Module's constructor assigns
_moduleName by pointer without copying), warn that passing dynamic strings will
produce dangling keys, and add a brief note advising future contributors to
either pass string literals or change the map to use std::string keys (or copy
the name in Module) if dynamic names are needed.
Summary by CodeRabbit
New Features
Bug Fixes
Changes
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.