Optimize mapping system #65
Conversation
Back end ======== - Virtual layer: mappingTable from vector to array
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
VirtualLayer storage & lifecycle src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Replaced mappingTable vector with PhysMap* mappingTable, removed mappingTableSizeUsed; moved mapping initialization/reset into lifecycle (onLayoutPre/onLayoutPost); adjusted destructor, addIndexP, addLight, setLight/getLight to use raw pointer and mappingTableIndexes; switched bounds checks to nrOfLights. |
PhysicalLayer allocation sizing src/MoonLight/Layers/PhysicalLayer.h, src/MoonLight/Layers/PhysicalLayer.cpp |
Added Lights::mappings and Lights::maxMappings; compute/set maxMappings alongside maxChannels (PSRAM-aware sizing); allocate lights.mappings and update allocation/failure logging and reset behavior. |
Module info / schema changes src/MoonLight/Modules/ModuleMoonLightInfo.h |
Reworked info construction to use addControl-driven schema; emit maxMappings top-level; removed per-layer mappingTable# emission and iterate up to nrOfLights; adjusted per-layer controls and data emission. |
Memory allocation helpers src/MoonBase/Utilities.h |
Changed allocMB to use heap_caps_malloc_prefer + memset zero-init; refined reallocMB error messaging; consolidated object allocation path with explicit construction and failure logging. |
Logging & socket/http/server tweaks lib/framework/EventSocket.cpp, src/MoonBase/SharedHttpEndpoint.h, src/MoonBase/SharedWebSocketServer.h |
Increased semaphore wait in one location; added heap-availability checks before buffer reserve and guarded reserve with try/catch; improved unregistered-event log (include length/origin) and added error logs when module not found; no return-behavior changes. |
| Minor formatting & comments multiple files ( src/MoonLight/Layers/VirtualLayer.cpp, etc.) |
Small whitespace/comment edits and added diagnostic logs (counts, state-transition comments); no functional signature changes. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Areas to inspect closely:
- VirtualLayer memory lifecycle (allocation/free across layout passes and destructor).
- All bounds-check substitutions from
mappingTableSizeUsed→nrOfLightsto avoid OOB access. - PhysicalLayer allocation math and interaction with new
maxMappings. - ModuleMoonLightInfo schema changes for consumers expecting
mappingTable#. - allocMB/allocMBObject zero-initialization and realloc error paths.
- EventSocket heap-reserve logic and related logs (race/heap-fragmentation concerns).
Possibly related PRs
- add heap-optimization: request heap optimization review #62 — Related changes to VirtualLayer/PhysicalLayer mapping storage and size-tracking; likely overlaps in mappingTable vs mappings handling.
Suggested labels
enhancement
Poem
🐰 I swapped my vector for a pointer neat,
I wake, allocate, and map each little light.
Counters now count by nrOfLights' beat,
Mappings march in memory, tidy and bright.
Hop, debug, repeat — carrot-powered delight! 🥕
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Optimize mapping system' directly aligns with the PR's main objective of converting mappingTable from vector-based to array-based structures to reduce memory fragmentation and improve performance on constrained devices. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
optimize-mapping
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.
ewowi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
134-134: Remove or document commented-out code.This commented-out line appears to be dead code. Either remove it entirely or add a comment explaining why it's retained (e.g., for future use).
- // data["layers"][index]["mappingTable#"] = layer->nrOfLights;src/MoonLight/Layers/VirtualLayer.cpp (1)
307-313: Consider early return on allocation failure.If
reallocMBfails, the code logs an error but continues execution. WhilenrOfLightsis reset to 0 at Line 282, other code paths might attempt to accessmappingTablebeforeaddLightsetsnrOfLights. Consider returning early or setting an error state.mappingTable = reallocMB<PhysMap>(mappingTable, size.x * size.y * size.z); if (mappingTable) { EXT_LOGD(ML_TAG, "%dx%dx%d %p", size.x, size.y, size.z, (void*)mappingTable); for (int i = 0; i < size.x * size.y * size.z; i++) mappingTable[i] = PhysMap(); } else { EXT_LOGD(ML_TAG, "create mappingTable failed %dx%dx%d %p", size.x, size.y, size.z, (void*)mappingTable); + return; // Prevent further processing with invalid state }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MoonLight/Layers/VirtualLayer.cpp(9 hunks)src/MoonLight/Layers/VirtualLayer.h(2 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
113-114: Bounds check relies on nrOfLights being within allocated size.The iteration now uses
layer->nrOfLightsas the bound. This is safe as long asnrOfLightsnever exceeds the allocatedmappingTablesize (size.x * size.y * size.z). Based onaddLightin VirtualLayer.cpp, this invariant is maintained sincenrOfLights = indexV + 1only whenindexV < size.x * size.y * size.z.src/MoonLight/Layers/VirtualLayer.cpp (5)
39-40: Proper memory cleanup in destructor.The explicit
freeMB(mappingTable)correctly releases the dynamically allocated memory, replacing the previous vector cleanup.
75-97: Good documentation of state transitions.The added comments clarifying the state machine transitions (
zero -> one,one -> more,more -> more) improve code readability.
123-125: Bounds check updated to usenrOfLights.The change from
mappingTableSizeUsedtonrOfLightsis consistent with the refactoring. However, similar toisMapped, consider adding a nullptr check formappingTableto prevent crashes if allocation fails.
155-156: Consistent bounds check update.Same as
setLight, the bounds check now usesnrOfLights. The same nullptr safety consideration applies here.
325-328: Good defensive check pattern.The nullptr check for
mappingTableand bounds check against allocated size (size.x * size.y * size.z) before accessing is correct. This pattern should be applied consistently to other accessors likeisMapped,setLight, andgetLight.
| PhysMap* mappingTable = nullptr; | ||
| std::vector<std::vector<uint16_t>, VectorRAMAllocator<std::vector<uint16_t>>> mappingTableIndexes; | ||
| uint16_t mappingTableSizeUsed = 0; | ||
| uint16_t mappingTableIndexesSizeUsed = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read VirtualLayer.h around the relevant lines
fd -name "VirtualLayer.h" -type fRepository: MoonModules/MoonLight
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Find VirtualLayer.h
fd "VirtualLayer.h" --type fRepository: MoonModules/MoonLight
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check file size
wc -l src/MoonLight/Layers/VirtualLayer.hRepository: MoonModules/MoonLight
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Read VirtualLayer.h around lines 70-72
sed -n '60,80p' src/MoonLight/Layers/VirtualLayer.hRepository: MoonModules/MoonLight
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Read around line 211 for isMapped implementation
sed -n '200,220p' src/MoonLight/Layers/VirtualLayer.hRepository: MoonModules/MoonLight
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Search for mappingTable allocation/initialization
rg "mappingTable\s*=" --type cpp -A2 -B2Repository: MoonModules/MoonLight
Length of output: 1393
🏁 Script executed:
#!/bin/bash
# Find the allocation in VirtualLayer.cpp with more context
rg "reallocMB.*mappingTable" --type cpp -A5 -B2Repository: MoonModules/MoonLight
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Get more context around the allocation failure handling
rg "create mappingTable failed" --type cpp -B5 -A10Repository: MoonModules/MoonLight
Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Search for all calls to isMapped
rg "isMapped\s*\(" --type cpp -B2 -A2Repository: MoonModules/MoonLight
Length of output: 6734
Raw pointer nullptr dereference vulnerability in isMapped() confirmed.
The conversion to raw pointer creates a critical issue: when reallocMB() fails to allocate memory, mappingTable remains nullptr but the VirtualLayer continues to operate. The isMapped() method at line 211 accesses mappingTable[indexV] without checking for nullptr. All call sites throughout the effects (E_MoonModules.h, E_MoonLight.h) invoke isMapped() without guarding against this allocation failure, creating a guaranteed crash scenario.
The allocation failure handling in VirtualLayer.cpp only logs an error and continues execution—no recovery mechanism exists. Either add a nullptr check to isMapped() or ensure allocation failure prevents further use of the VirtualLayer.
🤖 Prompt for AI Agents
In src/MoonLight/Layers/VirtualLayer.h around lines 70-72, mappingTable is a raw
pointer that can remain nullptr on reallocMB() failure; isMapped() (line ~211)
dereferences mappingTable without a nullptr check causing guaranteed crashes.
Fix by adding a nullptr guard at the start of isMapped(): if mappingTable is
nullptr, return false (and optionally log or set an internal error/invalid
flag); alternatively, on reallocMB() failure set an internal "unusable" state
and have isMapped() check that state and return false. Ensure all call sites
continue to work safely with the new behavior and consider adding a unit test
that simulates allocation failure to verify no dereference occurs.
|
I have a physical layer where all lights channels are stored in an uint8_t array called channels. They are allocated as follows: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12523 🏁 Script executed: Length of output: 24665 🏁 Script executed: Length of output: 12938 🏁 Script executed: Length of output: 24540 🏁 Script executed: Length of output: 4267 🏁 Script executed: Length of output: 12246 Great question! I've analyzed your memory allocation patterns. The PR's conversion of Current Memory AnalysisWhat you have:
Key Issues1. Vector-of-Vectors Fragmentation 🔴Your std::vector<std::vector<uint16_t>, VectorRAMAllocator<std::vector<uint16_t>>> mappingTableIndexes;Problems:
2. Dynamic Growth PatternIn case m_moreLights:
mappingTableIndexes[physMap.indexes].push_back(indexP); // Repeated reallocationsEach Optimization RecommendationsOption 1: Pool-Based Flat Array (Recommended) ⭐Replace the vector-of-vectors with a single flat array and an index table: // In VirtualLayer.h
struct MappingIndexPool {
uint16_t* pool = nullptr; // Flat array of all indexP values
uint16_t* offsets = nullptr; // Start offset for each mapping
uint16_t* counts = nullptr; // Count of indices for each mapping
uint16_t poolSize = 0; // Current used size in pool
uint16_t poolCapacity = 0; // Maximum capacity
uint16_t maxMappings = 0; // Number of mappings
};
MappingIndexPool mappingIndexPool;
uint16_t mappingTableIndexesSizeUsed = 0;Benefits:
Implementation changes: // In onLayoutPre()
void VirtualLayer::onLayoutPre() {
// ... existing code ...
// Reset pool usage
mappingIndexPool.poolSize = 0;
mappingTableIndexesSizeUsed = 0;
// Pre-allocate if needed (one-time allocation)
if (!mappingIndexPool.pool) {
uint16_t maxVirtualLights = 4096; // Adjust based on max expected
uint16_t maxPoolSize = 8192; // Estimate: assume average 2x multiplier for many-mappings
mappingIndexPool.pool = allocMB<uint16_t>(maxPoolSize, "mappingPool");
mappingIndexPool.offsets = allocMB<uint16_t>(maxVirtualLights, "mappingOffsets");
mappingIndexPool.counts = allocMB<uint16_t>(maxVirtualLights, "mappingCounts");
if (mappingIndexPool.pool && mappingIndexPool.offsets && mappingIndexPool.counts) {
mappingIndexPool.poolCapacity = maxPoolSize;
mappingIndexPool.maxMappings = maxVirtualLights;
EXT_LOGD(ML_TAG, "Allocated mapping pool: %d indices, %d mappings", maxPoolSize, maxVirtualLights);
}
}
// Zero out counts (offsets will be set during addIndexP)
if (mappingIndexPool.counts) {
memset(mappingIndexPool.counts, 0, mappingTableIndexesSizeUsed * sizeof(uint16_t));
}
}
// Modified addIndexP()
void VirtualLayer::addIndexP(PhysMap& physMap, uint16_t indexP) {
switch (physMap.mapType) {
case m_zeroLights:
physMap.indexP = indexP;
physMap.mapType = m_oneLight;
break;
case m_oneLight: {
uint16_t oldIndexP = physMap.indexP;
// Allocate space in pool for 2 indices
if (mappingIndexPool.poolSize + 2 <= mappingIndexPool.poolCapacity) {
uint16_t mappingIndex = mappingTableIndexesSizeUsed++;
physMap.indexes = mappingIndex;
physMap.mapType = m_moreLights;
mappingIndexPool.offsets[mappingIndex] = mappingIndexPool.poolSize;
mappingIndexPool.pool[mappingIndexPool.poolSize++] = oldIndexP;
mappingIndexPool.pool[mappingIndexPool.poolSize++] = indexP;
mappingIndexPool.counts[mappingIndex] = 2;
}
break;
}
case m_moreLights: {
uint16_t mappingIndex = physMap.indexes;
if (mappingIndexPool.poolSize < mappingIndexPool.poolCapacity) {
mappingIndexPool.pool[mappingIndexPool.poolSize++] = indexP;
mappingIndexPool.counts[mappingIndex]++;
}
break;
}
}
}
// Access in setLight()
case m_moreLights: {
uint16_t mappingIndex = mappingTable[indexV].indexes;
uint16_t offset = mappingIndexPool.offsets[mappingIndex];
uint16_t count = mappingIndexPool.counts[mappingIndex];
for (uint16_t i = 0; i < count; i++) {
uint16_t indexP = mappingIndexPool.pool[offset + i];
memcpy(&layerP->lights.channels[indexP * layerP->lights.header.channelsPerLight + offset],
channels, length);
}
break;
}Option 2: Reserve Capacity (Quick Fix)If you want to keep the current structure, at least reserve capacity: // In onLayoutPre()
mappingTableIndexes.reserve(size.x * size.y * size.z / 4); // Estimate 25% might be moreLights
// When transitioning from oneLight to moreLights
case m_oneLight: {
uint16_t oldIndexP = physMap.indexP;
mappingTableIndexesSizeUsed++;
if (mappingTableIndexes.size() < mappingTableIndexesSizeUsed) {
std::vector<uint16_t> newVec;
newVec.reserve(8); // Pre-reserve space for ~8 mappings
newVec.push_back(oldIndexP);
newVec.push_back(indexP);
mappingTableIndexes.push_back(std::move(newVec));
} else {
mappingTableIndexes[mappingTableIndexesSizeUsed - 1].clear();
mappingTableIndexes[mappingTableIndexesSizeUsed - 1].reserve(8);
mappingTableIndexes[mappingTableIndexesSizeUsed - 1].push_back(oldIndexP);
mappingTableIndexes[mappingTableIndexesSizeUsed - 1].push_back(indexP);
}
// ...
}Option 3: Memory Budget Management// In PhysicalLayer::setup()
void PhysicalLayer::setup() {
size_t availableHeap = heap_caps_get_free_size(MALLOC_CAP_8BIT);
if (psramFound()) {
lights.maxChannels = MIN(ESP.getPsramSize() / 2, 61440 * 3);
} else {
// Be more conservative on ESP32-D0
// Reserve ~20KB for system, leave rest for channels and mapping
size_t reservedForSystem = 20 * 1024;
size_t availableForLights = availableHeap - reservedForSystem;
// Allocate 70% to channels, 30% to mapping structures
size_t channelsSize = (availableForLights * 70) / 100;
lights.maxChannels = (channelsSize / 3) * 3; // Round to multiple of 3
EXT_LOGD(ML_TAG, "ESP32-D0: Available heap: %d, allocating %d channels",
availableHeap, lights.maxChannels);
}
lights.channels = allocMB<uint8_t>(lights.maxChannels);
// ...
}Memory Footprint ComparisonFor 2048 LEDs with 10% requiring 1:many mapping (average 3 physical LEDs per virtual):
Additional Recommendations
// Add to loop20ms()
static size_t minFreeHeap = SIZE_MAX;
size_t freeHeap = heap_caps_get_free_size(MALLOC_CAP_8BIT);
if (freeHeap < minFreeHeap) {
minFreeHeap = freeHeap;
EXT_LOGD(ML_TAG, "Min free heap: %d bytes", minFreeHeap);
}SummaryShort term: Implement Option 2 (reserve capacity) - minimal code changes The pool-based approach should allow you to support 3000-4000 LEDs on ESP32-D0 without PSRAM, compared to your current ~2048 limit. Would you like me to help implement any of these options? |
As at boot largest free block is available and prevents heap fragmentation later Back end ======== - Utilities: allocMB: use malloc instead of calloc - Physical layer: add maxMappings and allocate mappingTable as part of the channel array - Virtual layer: do not allocate and free mappingTable but set pointer to extended channel array, addLight, check if indexV within maxMappings size - MoonLightInfo: add maxMappings
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
52-57: Log message after failure shows zeros - consider logging attempted size before reset.On allocation failure (Lines 54-55),
maxChannelsandmaxMappingsare reset to 0 before logging on Line 56. The error log will show0for the allocation attempt, which loses diagnostic value.Consider logging the attempted allocation size before resetting:
} else { + EXT_LOGE(ML_TAG, "failed to allocate lights.channels %d free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_free_size(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)); lights.maxChannels = 0; lights.maxMappings = 0; - EXT_LOGE(ML_TAG, "after create lights.channels %d free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_free_size(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)); }src/MoonLight/Layers/VirtualLayer.cpp (3)
41-44: Guard mappingTable placement and consider stronger cast semanticsUsing the tail of
lights.channelsformappingTableis a good space optimization, but it currently assumes:
layerP->lights.channelsis non-null, andlayerP->lights.maxChannelskeeps the pointer aligned forPhysMap.To make this more robust, consider:
- Guarding the assignment, e.g.
mappingTable = layerP->lights.channels ? reinterpret_cast<PhysMap*>(layerP->lights.channels + layerP->lights.maxChannels) : nullptr;- Optionally asserting (in debug) that
layerP->lights.maxChannels % alignof(PhysMap) == 0so misalignment can’t silently creep in if allocation logic changes later.This keeps the optimization while tightening invariants around pointer validity and alignment.
121-150: Bounds logic around nrOfLights and mappingTable capacity is consistent, but relies on invariantsUsing
nrOfLightsas the upper bound insetLight/getLightand populating it viaaddLightonly when:if (mappingTable && indexV < layerP->lights.maxMappings / sizeof(PhysMap)) { nrOfLights = indexV + 1; addIndexP(mappingTable[indexV], layerP->indexP); }gives you a coherent set of invariants:
nrOfLights > 0impliesmappingTableis non-null andnrOfLights <= maxMappings / sizeof(PhysMap), somappingTable[indexV]is safe forindexV < nrOfLights.- Indices ≥
nrOfLightscorrectly fall back to the “no mapping” path guarded byindexV * channelsPerLight + offset + length < maxChannels/+ 3forgetLight.This is a good tightening over using a raw mapping-table size, and matches how other loops (fade, fill, etc.) already rely on
nrOfLights.Given that safety depends on
nrOfLightsonly being mutated viaaddLightand reset inonLayoutPre, it would be worth keeping that invariant explicit in a comment or assert nearnrOfLights’s declaration to prevent future regressions.Also applies to: 153-185, 309-321
280-307: Resetting full mappingTable region may conflict if multiple VirtualLayers share the same PhysicalLayerThe reset logic:
for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) { mappingTableIndex.clear(); } mappingTableIndexesSizeUsed = 0; memset(mappingTable, 0, layerP->lights.maxMappings);correctly:
- Reuses
mappingTableIndexescapacity across layouts, and- Zeroes all
PhysMapentries somapTypestarts from the “zero-lights” state.However, this assumes that the
mappingTablememory region is logically owned by thisVirtualLayer. IfPhysicalLayerhosts multipleVirtualLayerinstances sharing the samelights.channels(and thus the samemappingTablebase), each call toonLayoutPre()will wipe the others’ mappings.Please verify the intended ownership model:
- If only one
VirtualLayeruses this mapping region perPhysicalLayer, consider documenting that invariant.- If multiple layers are expected, you probably need per-layer slices of the mapping area (e.g., per-layer offset into
mappingTable) somemsetonly touches that layer’s slice.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/framework/EventSocket.cpp(1 hunks)src/MoonBase/SharedHttpEndpoint.h(1 hunks)src/MoonBase/SharedWebSocketServer.h(1 hunks)src/MoonBase/Utilities.h(4 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(1 hunks)src/MoonLight/Layers/PhysicalLayer.h(1 hunks)src/MoonLight/Layers/VirtualLayer.cpp(9 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/MoonBase/SharedWebSocketServer.h
- src/MoonBase/Utilities.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/MoonLight/Layers/PhysicalLayer.cpp (2)
setup(38-62)setup(38-38)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(355-364)addControl(355-355)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
lib/framework/EventSocket.cpp (2)
160-160: LGTM! Enhanced diagnostic logging.Adding event length and origin ID to the unregistered event warning improves debugging capability when tracking down unexpected event emission issues.
166-166: Minor formatting improvement.Consistent spacing around the
==operator improves readability.src/MoonBase/SharedHttpEndpoint.h (1)
93-94: LGTM! Added error logging for module lookup failure.Explicit error logging when a module is not found improves observability and aligns with the project-wide pattern of enhanced diagnostic output.
src/MoonLight/Layers/PhysicalLayer.h (1)
90-90: LGTM! New field for mapping allocation tracking.The
maxMappingsfield is properly added alongsidemaxChannelswith consistent initialization. This supports the PR's objective of transitioning from vector-based to array-based mapping structures.src/MoonLight/Modules/ModuleMoonLightInfo.h (3)
24-46: LGTM! Clean control-driven approach for schema definition.The refactoring to use
addControl()consistently improves maintainability and aligns with the pattern shown in the relevant code snippet fromModule.cpp.
58-58: LGTM! maxMappings properly exposed in readHook.The new
maxMappingsfield is correctly added to the data output, matching the schema definition.
68-68: Loop bound is correct — nrOfLights is the intended iteration count.The loop iterates using
layer->nrOfLights, which is the established pattern throughout the codebase (e.g., in VirtualLayer.cpp). ThenrOfLightsmember is dynamically set to track the actual valid entries inmappingTable, and accessingmappingTable[i]within the loop bound is safe. No out-of-bounds risk.src/MoonLight/Layers/VirtualLayer.cpp (3)
34-39: Destructor now correctly clears mappingTableIndexes by referenceThe switch to iterating by reference fixes the prior bug where
mappingTableIndex.clear()only affected copies; inner vectors now get cleared as intended beforemappingTableIndexes.clear().
71-99: addIndexP state machine & index pooling logic look soundThe 0→1→many transitions and reuse of
mappingTableIndexesslots viamappingTableIndexesSizeUsedare consistent and avoid unnecessary allocations:
m_zeroLights: simple transition to single index.m_oneLight: promotes tom_moreLights, adds{oldIndexP, indexP}to either a reused or newly pushed inner vector, and records the slot inphysMap.indexes.m_moreLights: just appends to the existing inner vector for that slot.This matches the intended pooling behavior and keeps
mappingTableIndexesstable across layouts.
359-377: Comment-only “single pixel” markers — no functional impactThe added
// single pixelcomments indrawLineanddrawLine3Dare purely cosmetic and don’t affect behavior. Nothing to change here.Also applies to: 440-462
|
@coderabbitai, why do I get the following crash on esp32-d0 and not on boards like esp32-s3 [ 15979][I][WiFiStatus.cpp:58] onStationModeGotIP(): [🐼] WiFi Got IP. localIP=http://192.168.1.152, hostName=http://esp32-d0-16mb.local abort() was called at PC 0x401e9ed3 on core 1 Backtrace: 0x4008f2a0:0x3ffd2000 0x4008f265:0x3ffd2020 0x40095b19:0x3ffd2040 0x401e9ed3:0x3ffd20c0 0x401e9f08:0x3ffd20e0 0x401e9fe3:0x3ffd2100 0x401ea076:0x3ffd2120 0x400fe3b1:0x3ffd2140 0x40133189:0x3ffd21b0 0x401331ea:0x3ffd2260 0x400fc966:0x3ffd2280 0x400fd935:0x3ffd22a0 0x40204cf5:0x3ffd2380 0x400e80ea:0x3ffd23a0 0x400e0079:0x3ffd2460 0x4018403d:0x3ffd24e0 0x40184908:0x3ffd2580 0x401833b0:0x3ffd25a0 0x402088ee:0x3ffd25c0 0x40183567:0x3ffd25e0 0x40090439:0x3ffd2670 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9670 I found the issue! The crash is happening at line 146 in EventSocket.cpp when trying to reserve memory for the MsgPack output buffer: static std::vector<uint8_t> outBuffer;
outBuffer.clear();
outBuffer.reserve(measureMsgPack(doc)); // ← CRASH HERE on ESP32-D0Root CauseThe crash is a
This is directly related to the memory pressure issue your PR is trying to solve! The FixAdd defensive heap checking before void EventSocket::emitEvent(const JsonDocument &doc, const char *originId, bool onlyToSameOrigin)
{
#if FT_ENABLED(EVENT_USE_JSON)
static String outBuffer;
outBuffer.clear();
outBuffer.reserve(measureJson(doc));
serializeJson(doc, outBuffer);
emitEvent(doc["event"], outBuffer.c_str(), outBuffer.length(), originId, onlyToSameOrigin);
#else
// --- MsgPack path ---
struct VecWriter {
std::vector<uint8_t> &v;
size_t write(uint8_t c) {
v.push_back(c);
return 1;
}
size_t write(const uint8_t *buf, size_t size) {
v.insert(v.end(), buf, buf + size);
return size;
}
};
static std::vector<uint8_t> outBuffer;
outBuffer.clear();
// 🌙 Check available heap before reserving to prevent crash on ESP32-D0
size_t requiredSize = measureMsgPack(doc);
size_t largestBlock = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT);
// Only reserve if we have enough memory + safety margin
if (requiredSize > 0 && largestBlock > requiredSize + 4096) {
try {
outBuffer.reserve(requiredSize);
} catch (const std::bad_alloc& e) {
ESP_LOGW(SVK_TAG, "Failed to reserve %d bytes for MsgPack buffer (largest block: %d)",
requiredSize, largestBlock);
// Continue anyway - vector will grow dynamically (slower but safer)
}
} else {
ESP_LOGW(SVK_TAG, "Insufficient heap for MsgPack buffer: need %d, available %d",
requiredSize, largestBlock);
// Skip reserve, let vector grow naturally
}
VecWriter writer{outBuffer};
serializeMsgPack(doc, writer);
emitEvent(doc["event"], (char *)outBuffer.data(), outBuffer.size(), originId, onlyToSameOrigin);
#endif
}Additional Recommendations1. Cap Maximum Event Size (Recommended)Prevent sending huge events that consume too much memory: void EventSocket::emitEvent(const JsonDocument &doc, const char *originId, bool onlyToSameOrigin)
{
// ... existing code ...
size_t requiredSize = measureMsgPack(doc);
// Cap at 16KB for ESP32-D0
const size_t MAX_EVENT_SIZE = psramFound() ? 65536 : 16384;
if (requiredSize > MAX_EVENT_SIZE) {
ESP_LOGE(SVK_TAG, "Event too large: %d bytes (max %d)", requiredSize, MAX_EVENT_SIZE);
return; // Drop the event
}
// ... rest of code ...
}2. Monitor Heap During DevelopmentAdd logging to track minimum heap: // In your main loop or periodic task
static size_t minFreeHeap = SIZE_MAX;
size_t freeHeap = heap_caps_get_free_size(MALLOC_CAP_8BIT);
if (freeHeap < minFreeHeap) {
minFreeHeap = freeHeap;
ESP_LOGW(ML_TAG, "Min free heap: %d bytes, largest block: %d",
minFreeHeap, heap_caps_get_largest_free_block(MALLOC_CAP_8BIT));
}3. Alternative: Pre-allocate Static BufferInstead of dynamic vector, use a fixed-size buffer: #define MAX_MSGPACK_BUFFER_SIZE 8192 // Adjust based on your needs
void EventSocket::emitEvent(const JsonDocument &doc, const char *originId, bool onlyToSameOrigin)
{
#if FT_ENABLED(EVENT_USE_JSON)
// ... JSON path ...
#else
static uint8_t outBuffer[MAX_MSGPACK_BUFFER_SIZE];
size_t requiredSize = measureMsgPack(doc);
if (requiredSize > MAX_MSGPACK_BUFFER_SIZE) {
ESP_LOGE(SVK_TAG, "Event too large: %d bytes (max %d)",
requiredSize, MAX_MSGPACK_BUFFER_SIZE);
return;
}
size_t len = serializeMsgPack(doc, outBuffer, MAX_MSGPACK_BUFFER_SIZE);
emitEvent(doc["event"], (char *)outBuffer, len, originId, onlyToSameOrigin);
#endif
}This eliminates dynamic allocation entirely for event emission. Why This Matters for Your PRYour PR is converting Immediate ActionApply the defensive heap checking fix above to prevent the crash. The Would you like me to create a more comprehensive memory management strategy for the entire codebase? |
|
@coderabbitai, implemented your recommendations, I still get these crashes: size_t requiredSize = measureMsgPack(doc); |
Back end ======== - EventSocket::emit: check on available memory - Physical layer: separate allocation for mappings
|
Tip For best results, initiate chat on the files or code changes. Understood! I see you've implemented the When you do, please provide:
I'll wait for your commit to see what's happening with the new crash. The issue may have moved to a different allocation point or there could be another vector/dynamic allocation failing under memory pressure. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/framework/EventSocket.cpp (1)
144-164: Heap availability check is a good defensive measure, but format specifiers need attention.The heap check before reserving is a sensible safeguard against
std::bad_alloccrashes on memory-constrained devices. However,%dis used forsize_tvalues (requiredSize,largestBlock). Use%zufor portability.- try { - outBuffer.reserve(requiredSize); - } catch (const std::bad_alloc& e) { - ESP_LOGW(SVK_TAG, "Failed to reserve %d bytes for MsgPack buffer (largest block: %d) %s %s", - requiredSize, largestBlock, doc["event"].as<const char *>(), originId); + try { + outBuffer.reserve(requiredSize); + } catch (const std::bad_alloc& e) { + ESP_LOGW(SVK_TAG, "Failed to reserve %zu bytes for MsgPack buffer (largest block: %zu) %s %s", + requiredSize, largestBlock, doc["event"].as<const char *>(), originId);- ESP_LOGW(SVK_TAG, "Insufficient heap for MsgPack buffer: need %d, available %d %s %s", - requiredSize, largestBlock, doc["event"].as<const char *>(), originId); + ESP_LOGW(SVK_TAG, "Insufficient heap for MsgPack buffer: need %zu, available %zu %s %s", + requiredSize, largestBlock, doc["event"].as<const char *>(), originId);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/framework/EventSocket.cpp(3 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(1 hunks)src/MoonLight/Layers/PhysicalLayer.h(1 hunks)src/MoonLight/Layers/VirtualLayer.cpp(9 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(355-364)addControl(355-355)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
lib/framework/EventSocket.cpp (1)
45-48: Timeout increase from 100ms to 200ms is reasonable.Doubling the initial timeout before falling back to blocking wait is a sensible adjustment for loaded systems.
src/MoonLight/Layers/PhysicalLayer.h (1)
89-91: LGTM!The new
mappingspointer andmaxMappingsfields follow the established pattern ofchannels/maxChannels, with proper default initialization.src/MoonLight/Modules/ModuleMoonLightInfo.h (2)
35-46: LGTM!The control schema is well-structured with consistent field names. The per-layer details correctly define controls for
nrOfLights,size, mapping statistics, andnodes#.
68-83: Bounds check usesnrOfLightscorrectly.The loop iteration now uses
layer->nrOfLightswhich aligns with the VirtualLayer changes wherenrOfLightstracks the actual number of mapped entries.src/MoonLight/Layers/VirtualLayer.cpp (5)
35-38: LGTM! Past issue addressed.The loop now correctly iterates by reference, ensuring
mappingTableIndex.clear()clears the actual vectors inmappingTableIndexes.
71-99: State transition logic inaddIndexPis correct.The three-state machine (zeroLights → oneLight → moreLights) properly handles:
- First mapping: stores indexP directly
- Second mapping: creates new entry in mappingTableIndexes
- Subsequent mappings: appends to existing vector
The reuse of
mappingTableIndexesentries viamappingTableIndexesSizeUsedavoids unnecessary allocations.
298-307: Mapping reset logic is correct.The reset sequence properly:
- Clears individual vectors by reference (line 301-303)
- Resets
mappingTableIndexesSizeUsedto 0 for reuse (line 304)- Uses
memsetwithlayerP->lights.maxMappingsbytes to zero the raw buffer (line 306)
316-326: Bounds check correctly accounts forPhysMapsize.The condition
indexV < layerP->lights.maxMappings / sizeof(PhysMap)correctly computes the maximum number of PhysMap entries that fit in the allocated buffer.
121-150:setLightbounds checking is sound.The dual-path logic correctly handles:
- Mapped lights (
indexV < nrOfLights): uses mappingTable- Unmapped lights: falls back to direct channel access with proper bounds check
src/MoonLight/Layers/PhysicalLayer.cpp (1)
41-47: PhysMap struct is confirmed to be 2 bytes; the sizing math is correct.The
PhysMapstruct insrc/MoonLight/Layers/VirtualLayer.h(line 34) is explicitly documented as 2 bytes. The union contains a 16-bit field, confirming the sizing math in PhysicalLayer.cpp lines 41-47:
- Non-PSRAM:
maxMappings = 4096 * 2 = 8192bytes ÷ 2 bytes/PhysMap = 4096 PhysMap entries ✓- Non-PSRAM:
maxChannels = 4096 * 3 = 12288÷ 3 channels/LED = 4096 LEDs ✓The sizing relationship between
maxChannelsandmaxMappingsis correctly aligned and intentional.
| lights.channels = allocMB<uint8_t>(lights.maxChannels); | ||
| lights.mappings = allocMB<uint8_t>(lights.maxMappings); | ||
|
|
||
| if (lights.channels) { | ||
| EXT_LOGD(ML_TAG, "allocated %d bytes in %s", lights.maxChannels, isInPSRAM(lights.channels) ? "PSRAM" : "RAM"); | ||
| if (lights.channels && lights.mappings) { | ||
| EXT_LOGI(ML_TAG, "after create channels and mappings %zu free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_free_size(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)); | ||
| } else { | ||
| EXT_LOGE(ML_TAG, "failed to allocated %d bytes of RAM or PSRAM", lights.maxChannels); | ||
| lights.maxChannels = 0; | ||
| lights.maxMappings = 0; | ||
| EXT_LOGE(ML_TAG, "after create channels and mappings %zu free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_free_size(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)); | ||
| } |
There was a problem hiding this comment.
Potential memory leak if one allocation succeeds but the other fails.
If lights.channels succeeds but lights.mappings fails (or vice versa), the successfully allocated buffer is not freed before resetting maxChannels and maxMappings to 0.
lights.channels = allocMB<uint8_t>(lights.maxChannels);
lights.mappings = allocMB<uint8_t>(lights.maxMappings);
if (lights.channels && lights.mappings) {
EXT_LOGI(ML_TAG, "after create channels and mappings %zu free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL));
} else {
+ // Free any partial allocation
+ if (lights.channels) { free(lights.channels); lights.channels = nullptr; }
+ if (lights.mappings) { free(lights.mappings); lights.mappings = nullptr; }
lights.maxChannels = 0;
lights.maxMappings = 0;
EXT_LOGE(ML_TAG, "after create channels and mappings %zu free:%d largest:%d internal:%d", lights.maxChannels + lights.maxMappings, psramFound() ? heap_caps_get_free_size(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), psramFound() ? heap_caps_get_largest_free_block(MALLOC_CAP_SPIRAM) : heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT), heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonLight/Layers/PhysicalLayer.cpp around lines 49 to 58, if one of the
allocMB calls succeeds and the other fails the code currently resets
lights.maxChannels and lights.maxMappings to 0 without freeing the successfully
allocated buffer which causes a memory leak; fix it by checking each pointer
(lights.channels and lights.mappings) when the combined allocation check fails,
free any non-null allocation with the matching deallocator (e.g., freeMB or the
project's deallocation function), set those pointers to nullptr, then set
maxChannels/maxMappings to 0 and log the error as before.
| void VirtualLayer::setup() { | ||
| mappingTable = (PhysMap*)(layerP->lights.mappings); | ||
| // no node setup here as done in addNode ! |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PhysMap struct definition
echo "=== PhysMap Definition ==="
rg -n "struct PhysMap|class PhysMap" -A 15 --type cpp --type h
echo ""
echo "=== lights.mappings allocation (allocMB) ==="
rg -n "allocMB|lights\.mappings" -B 2 -A 5 --type cpp --type h | head -80
echo ""
echo "=== Check for alignment attributes ==="
rg -n "alignas|__attribute__.*align" --type cpp --type h | grep -i physmapRepository: MoonModules/MoonLight
Length of output: 6256
🏁 Script executed:
#!/bin/bash
echo "=== Find lights structure definition ==="
rg -n "struct.*lights|class.*lights" -B 2 -A 10 --type cpp --type h | head -60
echo ""
echo "=== Search for lights.mappings allocation ==="
rg -n "lights\.mappings\s*=" -B 3 -A 3 --type cpp --type h
echo ""
echo "=== Search for mappings member in struct ==="
rg -n "mappings" --type cpp --type h | grep -E "(uint8_t|uint16_t|PhysMap|allocMB)" | head -20Repository: MoonModules/MoonLight
Length of output: 1578
Alignment of lights.mappings cast is practically safe for this platform, but consider explicit allocation for robustness.
The cast on line 42 assumes lights.mappings (allocated as uint8_t* via allocMB<uint8_t>()) aligns properly for PhysMap* access. Since PhysMap is 2 bytes and heap_caps_malloc_prefer() returns system-aligned pointers (typically 8+ bytes), the cast works in practice on ESP32. However, to avoid relying on malloc internals, consider allocating directly as PhysMap* or using alignas(sizeof(PhysMap)) for stricter compliance.
🤖 Prompt for AI Agents
In src/MoonLight/Layers/VirtualLayer.cpp around lines 41-43, the code casts
layerP->lights.mappings (allocated as uint8_t*) to PhysMap* which relies on
malloc alignment; change the allocation site so mappings is allocated as an
array of PhysMap (e.g. use allocMB<PhysMap>() or allocate sizeof(PhysMap)*count)
or use an aligned allocation (alignas(sizeof(PhysMap)) or
aligned_alloc/posix_memalign) so the pointer is guaranteed properly aligned
before casting; update the allocation and any related types so mappingTable =
(PhysMap*)... is safe without relying on malloc internals.
|
@coderabbitai, regarding
Go ahead! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10566 🏁 Script executed: Length of output: 2992 🏁 Script executed: Length of output: 5423 🏁 Script executed: Length of output: 6077 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2166 🏁 Script executed: Length of output: 4033 🏁 Script executed: Length of output: 4177 Good news: Your EventSocket.cpp implementation looks excellent! The heap checks and try/catch are correctly implemented. However, the crashes persist because the root cause is elsewhere. The Real Problem:
|
|
@coderabbitai, regarding
Yes please, go ahead ! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30708 🏁 Script executed: Length of output: 17247 🏁 Script executed: Length of output: 10660 🏁 Script executed: Length of output: 7824 🏁 Script executed: Length of output: 24367 🏁 Script executed: Length of output: 9349 Excellent! I've analyzed the entire codebase. Here's a comprehensive memory management strategy for MoonLight: 📊 Comprehensive Memory Management Strategy1️⃣ Executive SummaryCurrent State: ESP32-D0 crashes beyond ~2,048 LEDs due to heap fragmentation from Goal: Support 3,000-4,000 LEDs on ESP32-D0 through systematic memory optimization. Strategy: Three-phase approach (immediate fixes → pool-based architecture → comprehensive monitoring). 2️⃣ Memory Allocation TaxonomyI've identified all allocations in your codebase: A. Large Fixed Buffers (One-time allocation)
B. Dynamic Mapping Structures (Layout-dependent) 🔴
C. Node/Module Management
D. Event/WebSocket Buffers
|
| Phase | Timeline | Tasks | Expected LED Count |
|---|---|---|---|
| Phase 1 | Week 1 | • Add reserve() calls • Protect addIndexP() • Add heap monitoring |
~2,500-2,800 |
| Phase 2 | Week 2-3 | • Implement MappingIndexPool • Replace vector-of-vectors • Test on ESP32-D0 |
~3,500-4,000 |
| Phase 3 | Month 2 | • Memory budget system • Allocation tracking UI • Object pooling |
~4,500-5,000+ |
🔟 Immediate Action Items
- Add the reserve() and try/catch code from Section 3 to
VirtualLayer.cpp - Test on ESP32-D0 with 2,500-3,000 LEDs
- Merge this PR (array-based mappingTable is good foundation)
- Start Phase 2 in next PR (MappingIndexPool)
📈 Expected Results
| Metric | Current | Phase 1 | Phase 2 | Phase 3 |
|---|---|---|---|---|
| Max LEDs (ESP32-D0) | ~2,048 | ~2,800 | ~4,000 | ~5,000 |
| Heap fragmentation | ~35% | ~25% | ~10% | ~5% |
| Largest free block | ~15KB | ~20KB | ~28KB | ~32KB |
| Memory overhead | ~8KB | ~6KB | ~4KB | ~3KB |
Would you like me to:
- Generate the Phase 1 code changes (reserve + try/catch) as a complete patch?
- Create a separate PR for the MappingIndexPool implementation?
- Build a memory monitoring module with the debug endpoint?
Let me know which direction you'd like to pursue first! 🚀
Back end
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.