Board presets tuning #66
Conversation
Back end ======== - ModuleIO, add On off button pin - ModuleLightsControl: add pinToggleOnOff
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Core module & state APIs src/MoonBase/Module.h, src/MoonBase/Module.cpp |
Signatures renamed to use const JsonArray& controls, stateJson, newData; added requestUIUpdate and addControlValue helper; adjusted Module loop/update/invoke origins. |
Module definitions (addControl/addControlValue) src/MoonBase/Modules/ModuleIO.h, src/MoonBase/Modules/ModuleDevices.h, src/MoonBase/Modules/ModuleTasks.h, src/MoonLight/Modules/ModuleChannels.h, src/MoonLight/Modules/ModuleLiveScripts.h, src/MoonLight/Modules/ModuleMoonLightInfo.h, src/MoonLight/Modules/ModuleEffects.h, src/MoonBase/Modules/FileManager.* |
setupDefinition now accepts controls; replaced in-place Json construction with addControl() and addControlValue() and introduced rows-style control population. |
Nodes, NodeManager & control APIs src/MoonBase/Nodes.h, src/MoonBase/Nodes.cpp, src/MoonBase/NodeManager.h |
Added moduleNodes linkage, addControlValue helper; many method signatures changed to take const JsonArray& controls / const JsonObject& control; updateControl/onUpdate simplified; nextPin extended to accept optional ledPinDIO. |
Parallel LED driver & PhysicalLayer src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h, src/MoonLight/Layers/PhysicalLayer.h, src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonLight/Nodes/Drivers/parlio.cpp |
PhysicalDriver → ParallelLEDDriver; version uses Char<32>, new status field; added ledPinsAssigned[]; nextPin() → nextPin(uint8_t) with assigned-pin tracking and maxChannels adjustments. |
Lights control: button & debounce src/MoonLight/Modules/ModuleLightsControl.h |
Added pinToggleOnOff, renamed pinRelaisBrightness → pinRelayBrightness, introduced debounce fields/logic, INPUT_PULLUP handling, and updates pushed to _moduleName + "server". |
Layouts & pin mapping features src/MoonLight/Nodes/Layouts/L_MoonLight.h, src/MoonLight/Layers/PhysicalLayer.* |
Layouts now support ledPinDIO and panelsPerPin; layout logic updated to group panels per pin and call nextPin(ledPinDIO). |
Driver/node registration & option population src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Nodes/Drivers/* |
addNodes now accepts control objects; many drivers replaced manual values.add(...) with addControlValue(control, ...); node->moduleNodes is set for UI linkage; many onUpdate signatures now take const JsonObject&. |
Removed Virtual driver src/MoonLight/Nodes/Drivers/D_VirtualDriver.h, docs/develop/drivers.md |
Deleted the VirtualDriver implementation and removed the large drivers.md documentation file. |
String utilities / Char src/MoonBase/Utilities.h, src/MoonBase/Char.h |
Removed Char template and related helpers from Utilities.h; added Char<N> implementation in Char.h; logging macros gated by CORE_DEBUG_LEVEL. |
Minor drivers/effects/refactors src/MoonLight/Nodes/Drivers/D_ArtnetIn.h, src/MoonLight/Nodes/Drivers/D_Infrared.h, many src/*/Nodes/Effects/* and src/*/Nodes/Drivers/* |
Replaced manual option arrays with addControlValue, changed several onUpdate signatures to pass const JsonObject& control, small naming/label tweaks. |
Frontend & UI interface/src/lib/components/moonbase/MultiRow.svelte, interface/svelte.config.js, interface/src/routes/moonbase/monitor/Monitor.svelte |
MultiRow: CRUD-based gating for Add/Delete/drag handles and dragDisabled prop; Svelte kit output bundleStrategy set to 'single'; removed a commented variable. |
WiFi behavior lib/framework/WiFiSettingsService.cpp |
Re-enabled STA-mode check: path now calls connectToWiFi() when WiFi mode lacks STA. |
Build config & docs nav platformio.ini, firmware/esp32-s3.ini, mkdocs.yml |
Swapped I2SClocklessLedDriver dependency commit, bumped APP_DATE, comment edits, and added docs nav entry for standards/guidelines. |
Documentation & guides docs/develop/*, docs/moonbase/*, docs/moonlight/*, docs/gettingstarted/* |
Added standardsguidelines.md, updated IO/pin docs, renamed driver terminology to Parallel LED Driver, and updated developer/process docs. |
| Small signature/const-ref fixes many headers across src/* and misc/livescripts/* |
Numerous small signature updates to use const JsonObject& control and other const-ref improvements. |
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Areas to focus on:
- Module state read/update refactor in
Module.cpp/ModuleState(JSON diff/merge correctness, originId handling). - Pin assignment and mapping changes:
PhysicalLayer::nextPin(uint8_t),ledPinsAssigned[], and interactions with layouts (ledPinDIO,panelsPerPin). ParallelLEDDriverrename andChar<32>adoption — memory/copy semantics and UI bindings.ModuleLightsControldebounce logic and INPUT_PULLUP polarity/edge handling and the new server update routing.- Global migration to
addControl/addControlValue— verify defaults, values arrays, and backward-compatible UI expectations. - Ensure removal of
VirtualDriverdoes not leave unresolved references or missing expected behavior.
Possibly related PRs
- add heap-optimization: request heap optimization review #62 — overlapping refactor of Module/ModuleState APIs, control-definition patterns, and Char/utilities changes.
- d0 tuning #63 — related changes touching update/origin handling and Utilities/Char adjustments.
Suggested labels
enhancement
Poem
🐰 a rabbit notes in the build log:
controls renamed, rows lined up in a row,
pins now remember where their tiny currents go.
a button bounces softly, debounced in the night,
Char hopped to a new file and everything feels right.
Parallel drivers hum — the codebase took a leap.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 25.64% 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 'Board presets tuning' is directly related to the main changeset, which focuses on tuning board-level presets including pin assignments, relay naming, button controls, and related configuration changes across ModuleIO and ModuleLightsControl modules. |
✨ 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
board-preset-tuning
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonBase/Modules/ModuleIO.h(3 hunks)src/MoonLight/Modules/ModuleLightsControl.h(3 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 (5)
src/MoonBase/Modules/ModuleIO.h (2)
275-275: LGTM - Board preset updated correctly.The SE16V1 board pin 46 usage is correctly updated to use the new
pin_Button_OnOfftype.
380-384: LGTM - Default LED pin assignments expanded.The default wiring now includes LED outputs for pins 4-8, providing better out-of-box coverage for boards without specific presets.
src/MoonLight/Modules/ModuleLightsControl.h (3)
28-29: LGTM - Clearer initialization for unsigned pin tracking.Changing from
-1toUINT8_MAXmakes the sentinel value explicit foruint8_ttypes. AddingpinToggleOnOfffollows the same pattern aspinRelaisBrightness.
74-84: LGTM - Pin configuration read correctly.The logic properly resets pin tracking variables and scans the IO configuration to populate
pinToggleOnOffwhenpin_Button_OnOffusage is found.
286-286: Note: Update origin changed to server suffix.The update call now uses
_moduleName + "server"instead of just_moduleName. This change also appears at line 298 for the button toggle. Verify this is the intended pattern for distinguishing server-originated updates from UI updates.
Back end ======== - Nodes: add addControlValue, add ledPinDIO as argument to nextPin - Physical layer: add ledPinsAssigned, add ledPinDIO as argument to nextPin - Physical driver: use ledPinsAssigned to fill pins array (to do for other drivers!) - MoonLight layouts: add ledPinDIO in SingleLineLayout and SingleRowLayout
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (4)
259-259: Uninitialized member variableledPinDIO.The
ledPinDIOmember is uninitialized, which means it could hold a garbage value beforesetup()is called or if the control's default isn't properly set. Consider initializing to0orUINT8_MAXto ensure predictable behavior.- uint8_t ledPinDIO; + uint8_t ledPinDIO = 0;
266-271: Magic number20should referenceMAXLEDPINS.The loop uses a hardcoded
20which corresponds toMAXLEDPINS. Consider using the constant directly to ensure consistency if the limit changes.- for (int i = 0; i < 20; i++) { + for (int i = 0; i < MAXLEDPINS; i++) {
299-299: Uninitialized member variableledPinDIO.Same issue as in
SingleLineLayout- initialize to a default value.- uint8_t ledPinDIO; + uint8_t ledPinDIO = 0;
306-311: Consider extracting duplicate control setup logic.The LED pin DIO control setup (lines 306-311) is identical to
SingleLineLayout(lines 266-271). Consider extracting this into a helper function to reduce duplication.src/MoonLight/Layers/PhysicalLayer.cpp (1)
175-193: LGTM with minor logging suggestion.The implementation correctly handles both explicit pin assignment (
ledPinDIO != UINT8_MAX) and default sequential ordering. Consider enhancing the log message to include the assigned pin value for easier debugging.- EXT_LOGD(ML_TAG, "nextPin #%d ledsPerPin:%d of %d", i, ledsPerPin[i], MAXLEDPINS); + EXT_LOGD(ML_TAG, "nextPin #%d ledsPerPin:%d of %d assigned:%d", i, ledsPerPin[i], MAXLEDPINS, ledPinsAssigned[i]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/MoonBase/Nodes.h(2 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(2 hunks)src/MoonLight/Layers/PhysicalLayer.h(1 hunks)src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h(2 hunks)src/MoonLight/Nodes/Layouts/L_MoonLight.h(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/MoonBase/Nodes.h (1)
src/MoonLight/Layers/PhysicalLayer.cpp (2)
nextPin(175-194)nextPin(175-175)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
src/MoonBase/Nodes.h (1)
nextPin(188-188)src/MoonLight/Layers/PhysicalLayer.cpp (2)
nextPin(175-194)nextPin(175-175)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonBase/Nodes.h (1)
nextPin(188-188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
285-285: LGTM!The
nextPin(ledPinDIO)call correctly passes the user-selected pin index to the physical layer for pin assignment mapping.
325-325: LGTM!The
nextPin(ledPinDIO)call is consistent with theSingleLineLayoutimplementation.src/MoonBase/Nodes.h (1)
188-188: LGTM!The updated
nextPinsignature with the defaultUINT8_MAXparameter maintains backward compatibility while enabling explicit pin DIO mapping when needed.src/MoonLight/Layers/PhysicalLayer.h (1)
129-129: LGTM!The updated
nextPinsignature correctly accepts an optionalledPinparameter with a sensible default ofUINT8_MAX(sentinel value).src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (2)
106-106: LGTM!The
initledcall correctly uses the mappedpinsarray instead of the rawledPins, ensuring the LED DIO assignment is respected during initialization.
119-119: LGTM!The
updateDrivercall consistently uses the mappedpinsarray, maintaining alignment with the initialization path.
Back end ======== - ModuleIO.h: bugfix add Button OnOff to select - Nodes.h: addControlValue multiple types - Physical layer: onLayoutPre: memset ledPinsAssigned - Lights control: pinToggleOnOff pinmode pull up, loop pinToggleOnOff check on debounce and LOW - Layout SingleLine/Row: ledPinDIO default
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/MoonBase/Utilities.h (1)
402-410: Good refactor, but remove commented-out code.The refactoring to use
allocMB<T>(1, "object")improves consistency and adds error logging. However, the commented-out old implementation (line 403) should be removed to keep the codebase clean.Apply this diff to remove the commented-out code:
T* allocMBObject(Args&&... args) { void* res = allocMB<T>(1, "object"); - // T* res = (T*)heap_caps_malloc_prefer(sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_8BIT); // MALLOC_CAP_8BIT General-purpose byte-addressable RAM (safe for objects) - if (res) { return new (res) T(std::forward<Args>(args)...); } else {lib/framework/EventSocket.cpp (1)
144-164: Good defensive programming for heap-constrained devices.The heap-aware reservation logic prevents crashes on ESP32-D0 by checking available memory before reserving. The try/catch block and graceful degradation to dynamic growth is well-designed.
Consider extracting the 4096-byte safety margin as a named constant for clarity:
+ constexpr size_t HEAP_SAFETY_MARGIN = 4096; + // Only reserve if we have enough memory + safety margin - if (requiredSize > 0 && largestBlock > requiredSize + 4096) { + if (requiredSize > 0 && largestBlock > requiredSize + HEAP_SAFETY_MARGIN) { try { outBuffer.reserve(requiredSize); } catch (const std::bad_alloc& e) {src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (1)
90-98: LED pin remapping vialedPinsAssignedlooks correct and bounds-safe
nrOfPins = min(layerP.nrOfLedPins, layerP.nrOfAssignedPins)and theassignedPin < layerP.nrOfLedPinsguard ensure all accesses tolayerP.ledPins[assignedPin]andpins[i]stay within initialized ranges, while the fallback tolayerP.ledPins[i]keeps behavior sane for out-of-range assignments. Usingpinsfor bothinitledandupdateDrivercleanly wires the driver to the new mapping.Optionally, you could log
assignedPin(and/orledPinsAssigned[i]) in theEXT_LOGDto make it easier to diagnose mismatches between logical DIO indices and physical pins during debugging.Also applies to: 106-107, 119-119
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
259-272: SingleLineLayout: LED DIO selection correctly wired intonextPinThe
ledPinDIOselect and its values (0= "Default",1..MAXLEDPINS= "LED NN") map cleanly intonextPin(ledPinDIO == 0 ? UINT8_MAX : ledPinDIO - 1), which aligns with the updatednextPin(uint8_t ledPinDIO)API (usingUINT8_MAXas “no override”). Label generation viaChar<8>andMAXLEDPINSis safe and straightforward.You might slightly expand the comment on
ledPinDIOto spell out the value semantics (e.g.,// 0 = default order, 1..N = explicit LED DIO index) for future readers.Also applies to: 286-287
300-313: SingleRowLayout: mirrors SingleLineLayout DIO override as expectedThis layout mirrors the
ledPinDIOhandling fromSingleLineLayout, with the same select options and the samenextPin(ledPinDIO == 0 ? UINT8_MAX : ledPinDIO - 1)conversion. That keeps behavior consistent between horizontal and vertical single-strip layouts and cleanly hooks into the PhysicalLayer override logic.As above, consider a brief clarifying comment on the meaning of the
ledPinDIOvalues to make the override behavior obvious at a glance.Also applies to: 327-328
src/MoonLight/Layers/PhysicalLayer.cpp (1)
41-47: Mappings allocation andnextPin(uint8_t)override behavior are coherent with the new LED pin modelSplitting PSRAM (or fixed heap) between
lights.maxChannelsandlights.maxMappings, then allocating bothlights.channelsandlights.mappings, brings the physical layer in line with the richer mapping model. ResettingledsPerPinto0xFFFFandledPinsAssignedto 0 inonLayoutPre()(for pass 1, non-monitor) matches the sentinel logic used innextPinand avoids stale data between layouts.The new
nextPin(uint8_t ledPinDIO)correctly:
- Finds the first free pin slot via the
UINT16_MAXsentinel inledsPerPin.- Stores the number of lights on that pin as
lights.header.nrOfLights - prevNrOfLights.- Uses
ledPinDIO != UINT8_MAXto distinguish explicit overrides from default wiring, assigning either the provided DIO index or the defaulti.This lines up with the updated Node
nextPinsignature and with howledPinsAssignedis later consumed by the driver. If you ever need to be extremely tight on heap, you could consider freeinglights.channelswhenlights.mappingsfails to allocate (or vice versa), but that’s a minor, one-shot optimization rather than a correctness issue.Also applies to: 50-53, 56-57, 141-143, 181-198
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/framework/EventSocket.cpp(3 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/Nodes.h(2 hunks)src/MoonBase/SharedHttpEndpoint.h(1 hunks)src/MoonBase/SharedWebSocketServer.h(1 hunks)src/MoonBase/Utilities.h(4 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(4 hunks)src/MoonLight/Layers/PhysicalLayer.h(2 hunks)src/MoonLight/Layers/VirtualLayer.cpp(9 hunks)src/MoonLight/Layers/VirtualLayer.h(2 hunks)src/MoonLight/Modules/ModuleLightsControl.h(3 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(3 hunks)src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h(2 hunks)src/MoonLight/Nodes/Layouts/L_MoonLight.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/MoonBase/Modules/ModuleIO.h
- src/MoonBase/Nodes.h
- src/MoonLight/Layers/PhysicalLayer.h
🧰 Additional context used
🧬 Code graph analysis (4)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
src/MoonBase/Nodes.h (1)
nextPin(190-190)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(355-364)addControl(355-355)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonBase/Nodes.h (1)
nextPin(190-190)
src/MoonLight/Modules/ModuleLightsControl.h (3)
src/MoonBase/Modules/FileManager.cpp (2)
update(58-139)update(58-58)src/MoonBase/Module.cpp (2)
update(262-293)update(262-262)src/MoonBase/Module.h (1)
ModuleState(47-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (20)
src/MoonLight/Modules/ModuleLightsControl.h (4)
24-29: Good use of UINT8_MAX sentinels for pin fieldsInitializing
pinRelaisBrightnessandpinToggleOnOfftoUINT8_MAXmatches theuint8_ttype and the later!= UINT8_MAXchecks, avoiding the old signed/unsigned mismatch.
72-85: Pin scanning reset and button pin configuration look correctResetting both pin members to
UINT8_MAXbefore iteratingstate.data["pins"]prevents stale GPIO usage, and configuringpinToggleOnOffasINPUT_PULLUPis consistent with the active‑low handling inloop(). Logging both pins should also help diagnose wiring/mapping issues.
283-288: Origin ID change to"<module>server"is reasonableRouting preset auto‑advance updates through
update(newState, ModuleState::update, _moduleName + "server")is consistent with the new on/off button path and should help distinguish server‑initiated changes from UI ones, assuming listeners are aware of / expect this origin ID.
292-308: Debounced on/off button handling is robustThe new block correctly:
- Guards on
pinToggleOnOff != UINT8_MAX.- Debounces using
lastDebounceTimeand a 50 ms window.- Triggers only on the HIGH→LOW edge (for
INPUT_PULLUP), then toggleslightsOnviaupdate(..., _moduleName + "server").This addresses the earlier concerns about missing
pinMode, no debounce, and double‑edge triggering.src/MoonLight/Layers/VirtualLayer.h (1)
70-70: Heap optimization: raw pointer for mappingTable.The change from
std::vectorto a raw pointer aligns with the stated heap optimization goals. The memory is now managed externally vialayerP->lights.mappings, which is appropriate for embedded systems with limited RAM.src/MoonLight/Layers/VirtualLayer.cpp (7)
35-38: Good use of reference in range-based for loop.Using
std::vector<uint16_t>&instead of copying each inner vector improves efficiency during cleanup.
41-44: Cast assumes proper alignment oflayerP->lights.mappings.The cast to
PhysMap*is acceptable assuming the source buffer is properly aligned. SincePhysMapis a 2-byte structure, this should work on typical embedded platforms.
74-96: Improved state transition comments.The added comments clearly document the mapping state transitions, improving code readability.
121-150: Bounds check updated to usenrOfLights.The change from
mappingTableSizeUsedtonrOfLightsis consistent with the new mapping table design wherenrOfLightsrepresents the number of valid virtual light entries.
152-185: Consistent bounds check ingetLight.The update to use
nrOfLightsas the upper bound is consistent with thesetLightfunction changes.
309-326: Proper bounds validation inaddLight.The null check and bounds validation
indexV < layerP->lights.maxMappings / sizeof(PhysMap)correctly converts byte size to element count. This confirmsmaxMappingsis in bytes.
359-377: Minor formatting improvement.The added inline comment clarifies the single-pixel case handling.
src/MoonBase/Utilities.h (2)
361-361: LGTM! Error message now matches the actual operation.The error log correctly identifies
heap_caps_reallocas the failing operation, improving debugging accuracy.
425-427: LGTM! Formatting improvement.Minor formatting adjustments improve readability without changing logic.
lib/framework/EventSocket.cpp (1)
179-179: LGTM! Enhanced logging improves debugging.Adding
lenandoriginIdto the warning message provides valuable context when troubleshooting unregistered event emissions.src/MoonBase/SharedWebSocketServer.h (1)
122-122: LGTM! Error logging improves debugging.Adding explicit error logging when a module is not found makes troubleshooting easier and aligns with the similar improvement in SharedHttpEndpoint.h.
src/MoonBase/SharedHttpEndpoint.h (3)
36-38: LGTM! Standard CORS preflight handling.The OPTIONS handler is properly feature-gated and implements standard CORS preflight response.
93-93: LGTM! Consistent error logging.Error logging when module is not found improves debugging and aligns with SharedWebSocketServer.h.
64-86: This is a new endpoint, not a modification to existing behavior.SharedHttpEndpoint.h is a new file introducing unified HTTP endpoint handling for modules. The POST handler returning updated state is the intentional design of this new implementation, consistent with the GET handler pattern. No clients previously existed for this endpoint, so this cannot be a breaking change.
Likely an incorrect or invalid review comment.
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
24-47: MoonLight info schema refactor andmaxMappingsexposure look consistentRefactoring
setupDefinitionto useaddControlfor the root metrics plus alayersrows control makes the schema much clearer and more in line with the rest of the module framework. AddingmaxMappingsboth to the schema and to thereadHook(data["maxMappings"] = layerP.lights.maxMappings;) keeps the UI in sync with the new backend capacity field.Within the per-layer loop, counting
nrOfZeroLights/nrOfOneLight/nrOfMoreLightsoveri < layer->nrOfLightsis consistent with a per-lightmappingTableand matches how the rest of the struct is populated, so the derived stats should be accurate given the current data model.Also applies to: 58-59, 68-95
lib/framework/EventSocket.cpp
Outdated
| { | ||
| // 🌙 adding semaphore wait too long logging | ||
| if (xSemaphoreTake(clientSubscriptionsMutex, pdMS_TO_TICKS(100))==pdFALSE) { | ||
| if (xSemaphoreTake(clientSubscriptionsMutex, pdMS_TO_TICKS(200)) == pdFALSE) { |
There was a problem hiding this comment.
Inconsistent mutex timeout values across similar code paths.
The mutex wait timeout in onWSClose is increased to 200ms, while emitEvent at line 185 remains at 100ms. If the original 100ms timeout was problematic enough to warrant increasing it to 200ms in one location, the same issue likely applies to both code paths.
Consider standardizing the timeout value:
- if (xSemaphoreTake(clientSubscriptionsMutex, pdMS_TO_TICKS(100)) == pdFALSE) {
+ if (xSemaphoreTake(clientSubscriptionsMutex, pdMS_TO_TICKS(200)) == pdFALSE) {
ESP_LOGW(SVK_TAG, "clientSubscriptionsMutex wait too long");
xSemaphoreTake(clientSubscriptionsMutex, portMAX_DELAY);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/framework/EventSocket.cpp around line 45 (and also update emitEvent at
line 185), the mutex wait timeouts are inconsistent (200ms at onWSClose vs 100ms
in emitEvent); replace the magic numbers with a single named constant (e.g.
CLIENT_SUBSCRIPTIONS_MUTEX_TIMEOUT_MS = 200) and use
pdMS_TO_TICKS(CLIENT_SUBSCRIPTIONS_MUTEX_TIMEOUT_MS) in both places (and any
other similar code paths) to standardize behavior and make future adjustments
easy.
src/MoonBase/Utilities.h
Outdated
| T* res = (T*)heap_caps_malloc_prefer(n * sizeof(T), 2, MALLOC_CAP_SPIRAM, MALLOC_CAP_DEFAULT); // calloc is malloc + memset(0); | ||
| if (res) { | ||
| memset(res, 0, n * sizeof(T)); //calloc not always save so do it explicit |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for heap_caps_calloc_prefer usage
echo "=== Searching for heap_caps_calloc_prefer usage ==="
rg -n 'heap_caps_calloc' --type cpp --type c -C 2
echo -e "\n=== Searching for calloc-related safety comments ==="
rg -n 'calloc.*safe|calloc.*bug|calloc.*issue' --type cpp --type c -C 3
echo -e "\n=== Searching for comments about this specific change ==="
rg -n 'calloc not always' --type cpp --type c -C 3
echo -e "\n=== Checking the full context of allocMB function ==="
rg -n 'allocMB|heap_caps_malloc_prefer' src/MoonBase/Utilities.h -B 5 -A 10Repository: MoonModules/MoonLight
Length of output: 5222
Fix typo and reconsider the malloc+memset replacement—no documented issue with heap_caps_calloc_prefer found.
The claim that "calloc not always save" (typo: "safe") is unsubstantiated. The codebase itself uses heap_caps_calloc_prefer elsewhere (e.g., src/MoonLight/Nodes/Drivers/parlio.cpp lines 328–333) without any documented issues. The change to malloc + memset appears unnecessary unless there's a specific platform issue that should be documented.
Additionally, fix the typo in the comment: "save" → "safe".
For consistency, either:
- Revert
allocMBto useheap_caps_calloc_prefer(preferred, simpler and equivalent), or - Document the specific issue/platform limitation with a reference if the change is required.
🤖 Prompt for AI Agents
In src/MoonBase/Utilities.h around lines 345 to 347, the replacement of
heap_caps_calloc_prefer with malloc + memset contains a typo in the comment
("save" -> "safe") and is undocumented/unnecessary; revert allocMB to use
heap_caps_calloc_prefer for zero-initialized allocation (to match other usages
like src/MoonLight/Nodes/Drivers/parlio.cpp) OR if malloc+memset is truly
required, add a clear comment documenting the specific platform/implementation
bug with a reference, then fix the comment typo to "safe".
|
|
||
| // resetMapping | ||
|
|
||
| for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) { | ||
| mappingTableIndex.clear(); | ||
| } | ||
| mappingTableIndexesSizeUsed = 0; // do not clear mappingTableIndexes, reuse it | ||
|
|
||
| memset(mappingTable, 0, layerP->lights.maxMappings); // reset mappingTable | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for maxMappings definition and usage
rg -n "maxMappings" --type=cpp -B2 -A2 | head -100Repository: MoonModules/MoonLight
Length of output: 4884
🏁 Script executed:
# Find mappingTable declaration and type
rg -n "mappingTable\s*=" --type=cpp -B2 -A2 | head -100Repository: MoonModules/MoonLight
Length of output: 1310
🏁 Script executed:
# Find PhysMap type definition
rg -n "struct\s+PhysMap|class\s+PhysMap|typedef.*PhysMap" --type=cpp -B2 -A2Repository: MoonModules/MoonLight
Length of output: 378
🏁 Script executed:
# Look at the context around the code in question
sed -n '280,320p' src/MoonLight/Layers/VirtualLayer.cppRepository: MoonModules/MoonLight
Length of output: 1386
🏁 Script executed:
# Search for onLayoutPre calls and setup order
rg -n "onLayoutPre|\.setup\(\)" --type=cpp src/MoonLight/Layers/VirtualLayer.cpp | head -20Repository: MoonModules/MoonLight
Length of output: 103
🏁 Script executed:
# Search for when onLayoutPre is called
rg -n "onLayoutPre" --type=cpp -B2 -A2 | head -100Repository: MoonModules/MoonLight
Length of output: 3727
🏁 Script executed:
# Check Layer base class or initialization patterns
rg -n "class VirtualLayer|class Layer" --type=h src/MoonLight/Layers/ -B2 -A5Repository: MoonModules/MoonLight
Length of output: 1461
Add null check before memset on mappingTable.
The mappingTable is initialized as nullptr and only assigned in setup(), but onLayoutPre() may be called during layout mapping before setup completes. Without a null check, calling memset on a null pointer causes undefined behavior. This pattern is already used defensively elsewhere in the code (line 318: if (mappingTable && ...)).
The size parameter (layerP->lights.maxMappings) is correct—it represents bytes allocated for the mappingTable buffer, not a count.
// resetMapping
for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) {
mappingTableIndex.clear();
}
mappingTableIndexesSizeUsed = 0; // do not clear mappingTableIndexes, reuse it
- memset(mappingTable, 0, layerP->lights.maxMappings); // reset mappingTable
+ if (mappingTable) memset(mappingTable, 0, layerP->lights.maxMappings); // reset mappingTable📝 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.
| // resetMapping | |
| for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) { | |
| mappingTableIndex.clear(); | |
| } | |
| mappingTableIndexesSizeUsed = 0; // do not clear mappingTableIndexes, reuse it | |
| memset(mappingTable, 0, layerP->lights.maxMappings); // reset mappingTable | |
| } | |
| // resetMapping | |
| for (std::vector<uint16_t>& mappingTableIndex : mappingTableIndexes) { | |
| mappingTableIndex.clear(); | |
| } | |
| mappingTableIndexesSizeUsed = 0; // do not clear mappingTableIndexes, reuse it | |
| if (mappingTable) memset(mappingTable, 0, layerP->lights.maxMappings); // reset mappingTable | |
| } |
🤖 Prompt for AI Agents
In src/MoonLight/Layers/VirtualLayer.cpp around lines 298 to 307, the code calls
memset(mappingTable, 0, layerP->lights.maxMappings) without ensuring
mappingTable is non-null; add a null check so the memset runs only if
mappingTable is not null (e.g. if (mappingTable) { memset(...); }), leaving the
size parameter layerP->lights.maxMappings unchanged and preserving the existing
comment about resetting mappingTable.
src/MoonLight/Layers/VirtualLayer.h
Outdated
|
|
||
| // checks if a virtual light is mapped to a physical light (use with XY() or XYZ() to get the indexV) | ||
| bool isMapped(int indexV) const { return indexV < mappingTableSizeUsed && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); } | ||
| bool isMapped(int indexV) const { return indexV < nrOfLights && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); } |
There was a problem hiding this comment.
Potential null pointer dereference in isMapped().
If isMapped() is called before setup() initializes mappingTable, accessing mappingTable[indexV] will dereference a null pointer. Consider adding a null check:
- bool isMapped(int indexV) const { return indexV < nrOfLights && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); }
+ bool isMapped(int indexV) const { return mappingTable && indexV < nrOfLights && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); }📝 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.
| bool isMapped(int indexV) const { return indexV < nrOfLights && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); } | |
| bool isMapped(int indexV) const { return mappingTable && indexV < nrOfLights && (mappingTable[indexV].mapType == m_oneLight || mappingTable[indexV].mapType == m_moreLights); } |
🤖 Prompt for AI Agents
In src/MoonLight/Layers/VirtualLayer.h around line 211, isMapped() dereferences
mappingTable without checking it; add a guard to first verify mappingTable is
non-null and indexV is within valid range (indexV >= 0 && indexV < nrOfLights)
and return false if either check fails, then perform the existing mapType
comparison; optionally add an assertion or error log for unexpected states.
ewowi
force-pushed
the
board-preset-tuning
branch
from
4cff6c2 to
a1341fd
Compare
Front end ======== - svelte config: add build strategy single Back end ======= - WiFi settings service: revert check on getmode and wifi sta - Module: move requestUIUpdate from Node manager to module (used also in driver) - Nodes: add moduleNodes , add updateControl by control name - Physical layer: increase max channels for non psram to 8192 - Drivers: readPins: do not check on ledsPerPin! add moduleNodes (for UI Update) - Physical driver: add status control and set onLayout
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
50-56: Const-refcontrolis fine; fixDDPvsddpname mismatch.The new
onUpdate(const JsonObject& control)matches the base API and is fine. However,setup()registers the checkbox as"DDP"(upper-case), whileonUpdatechecks for"ddp", so the condition never fires and the handler is effectively dead code.Consider aligning the names:
- if (control["name"] == "ddp") { + if (control["name"] == "DDP") { if (control["value"] == 0) { } }lib/framework/WiFiSettingsService.cpp (1)
314-321: TheWiFi.getMode()gate blocks reconnection in AP+STA modeThis condition prevents reconnection after WiFi disconnects when the device is in AP+STA mode. After a STA disconnection,
WiFi.getMode()remainsWIFI_AP_STA(value 3), so(3 & 1)evaluates to 1 (non-zero), causing the condition to fail andconnectToWiFi()to be skipped. This is the exact issue reported in ESP32-sveltekit theelims#109.The existing guards already handle the critical cases:
WiFi.isConnected(),_state.wifiSettings.empty(), and_state.staConnectionMode == OFFLINEprevent spurious reconnection attempts. Combined with the caller's throttling viaWIFI_RECONNECTION_DELAY, these are sufficient safeguards.Remove the
getMode()conditional to allow reconnection:- // Connect or reconnect as required - if ((WiFi.getMode() & WIFI_STA) == 0) // 🌙 , see https://github.com/theelims/ESP32-sveltekit/issues/109 🚧 - { -#ifdef SERIAL_INFO - Serial.println("Connecting to WiFi..."); -#endif - connectToWiFi(); - } + // Connect or reconnect as required +#ifdef SERIAL_INFO + Serial.println("Connecting to WiFi..."); +#endif + connectToWiFi();
🧹 Nitpick comments (6)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
176-193: nextPin overload and assignment logic are consistent; consider future‑proofing the index typeThe new
nextPin(uint8_t ledPinDIO)overload cleanly supports explicit pin ordering:
- You scan for the first
UINT16_MAXslot, accumulateprevNrOfLights, and store the delta inledsPerPin[i]as before.ledPinsAssigned[i]is set either to the explicitledPinDIOor to the default sequential indexiwhenledPinDIO == UINT8_MAX, which lines up with the wrapper inNodes.husingUINT8_MAXas the sentinel.- Logging now reports the assigned pin, which should simplify debugging mismatched wiring vs. layout.
Given the comment that you target up to 120 pins, the
uint8_t iindex is fine today, but ifMAXLEDPINSever increases beyond 255 this could overflow and produce incorrect iteration. As a low‑priority hardening, you could widen the index type:- uint8_t i = 0; + uint16_t i = 0;Everything else in this block looks logically sound and aligned with the new per‑pin assignment model.
src/MoonBase/Modules/ModuleIO.h (1)
381-387: Commented-out default LED pins could be simplified into a TODOThe commented assignments for
LED_02–LED_06and the note about esp32-d0-16MB are harmless, but commented-out code tends to age poorly. Consider replacing these lines with a single// TODOthat briefly describes the incompatibility and links to an issue, or remove them once the pin selection is finalized.src/MoonLight/Modules/ModuleEffects.h (1)
283-286: WiringmoduleNodesto the owning module is appropriate.Hooking
node->moduleNodes = (Module*)this;ensures effects can request UI updates via the parent module, consistent with the newModule::requestUIUpdateflow. You might preferstatic_cast<Module*>(this)for clearer intent, but behavior is correct as-is.src/MoonBase/Module.h (1)
142-143: Module-levelrequestUIUpdateflow looks consistent with the new node linkage.Centralizing UI refresh via
Module::requestUIUpdateand triggering aStateUpdateResult::CHANGEDinloop()aligns with the newnode->moduleNodeswiring and removes the NodeManager-specific flag. Assuming this flag is only set from the same task (or benign races are acceptable), the approach is fine; if you ever start toggling it from other tasks, consider a slightly more robust signaling mechanism (e.g., using the existing semaphore-based update path) to avoid lost UI updates.Also applies to: 155-165
src/MoonLight/Modules/ModuleDrivers.h (1)
54-56: Pin compaction and node–module wiring look sound.
- The compaction loop now keeps any
ledPins[readPos] != UINT8_MAX, which is consistent with using UINT8_MAX as a sentinel and keepsnrOfLedPinsin sync with the compacted prefix.- Assigning both
node->moduleIOandnode->moduleNodes = (Module*)thiscorrectly exposes IO configuration and the module-level UI update hook to driver/layout nodes.If you ever change the type of
ledPinsaway fromuint8_t, consider swappingsizeof(layerP.ledPins)for an explicit element-count to avoid subtle bugs, but it's fine with the current definition.Also applies to: 160-164
src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (1)
86-87: Pin remapping and driver updates look correct; consider guardingmoduleNodes.
nrOfPins = min(layerP.nrOfLedPins, layerP.nrOfAssignedPins)combined withassignedPin < layerP.nrOfLedPinsensures you never indexledPinsout of bounds; the fallback toledPins[i]keeps behavior sensible if an assignment index is invalid.- Using a local
pins[MAX_PINS]and passing it (withnrOfPins) intoinitledandupdateDriverkeeps the originallayerP.ledPinsarray untouched while still honoring the per-pin mapping.- Building a human-readable
statusStringfrompins[i]andledsPerPin[i]is useful for debugging and is correctly pushed back into the control before requesting a UI update.To make this a bit more robust, it would be safer to guard the UI signal:
updateControl("status", statusString.c_str()); if (moduleNodes) { moduleNodes->requestUIUpdate = true; }so that accidental use of
PhysicalDriveroutside aModuleDriverscontext (wheremoduleNodesmay not be initialized) cannot crash.Also applies to: 93-105, 117-118, 130-131
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
interface/svelte.config.js(1 hunks)lib/framework/WiFiSettingsService.cpp(1 hunks)platformio.ini(2 hunks)src/MoonBase/Module.h(2 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/NodeManager.h(1 hunks)src/MoonBase/Nodes.cpp(2 hunks)src/MoonBase/Nodes.h(6 hunks)src/MoonBase/Utilities.h(1 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(4 hunks)src/MoonLight/Modules/ModuleDrivers.h(2 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(1 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(1 hunks)src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h(4 hunks)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h(1 hunks)src/MoonLight/Nodes/Drivers/D__Sandbox.h(1 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(2 hunks)src/MoonLight/Nodes/Effects/E_MoonModules.h(1 hunks)src/MoonLight/Nodes/Effects/E__Sandbox.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platformio.ini
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/interface/svelte.config.js : Configure SvelteKit build and adapter settings in `interface/svelte.config.js`
Applied to files:
interface/svelte.config.js
📚 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/svelte.config.js
📚 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/vite.config.ts : Configure Vite build tool, proxy settings, and LittleFS compatibility in `interface/vite.config.ts`
Applied to files:
interface/svelte.config.js
📚 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/svelte.config.js
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
lib/framework/WiFiSettingsService.cpp
🧬 Code graph analysis (5)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonBase/Nodes.h (1)
nextPin(199-199)
src/MoonLight/Nodes/Drivers/D__Sandbox.h (5)
src/MoonBase/Nodes.cpp (2)
onUpdate(401-541)onUpdate(401-401)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
onUpdate(926-930)onUpdate(1122-1126)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onUpdate(121-125)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonLight/Nodes/Effects/E_MoonModules.h (7)
src/MoonBase/Modules/ModuleIO.h (1)
onUpdate(401-421)src/MoonBase/NodeManager.h (1)
onUpdate(133-284)src/MoonBase/Nodes.cpp (2)
onUpdate(401-541)onUpdate(401-401)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
onUpdate(926-930)onUpdate(1122-1126)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonLight/Nodes/Effects/E_MoonLight.h (4)
src/MoonBase/Nodes.cpp (2)
onUpdate(401-541)onUpdate(401-401)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onUpdate(121-125)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (2)
src/MoonBase/Module.cpp (2)
addControl(355-364)addControl(355-355)src/MoonBase/Nodes.cpp (2)
updateControl(17-66)updateControl(17-17)
⏰ 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 (18)
src/MoonLight/Layers/PhysicalLayer.cpp (2)
44-44: Non‑PSRAM channel capacity increase looks safeBumping the non‑PSRAM
lights.maxChannelsto8192 * 3(≈24 KB) stays comfortably below the ~30 KB single‑alloc note in the header comment while doubling the LED capacity. Behavior and intent remain clear and consistent with the comment; no functional issues spotted here.
136-138: Good: pin mapping state is fully reset between layoutsZeroing
ledPinsAssignedalongside resettingledsPerPinavoids stale pin assignments leaking across layout remaps when!monitorPass. This keepsnextPin’s per‑pin mapping logic operating on a clean slate each pass and matches hownrOfAssignedPinsis recomputed.src/MoonBase/Modules/ModuleIO.h (3)
76-78: New IO_PinUsage entry is added safely before the sentinelPlacing
pin_Button_OnOffimmediately beforepin_countpreserves all existing enum values and keepspin_countas a valid upper bound. This is a safe, backward‑compatible extension.
148-207: UI usage list stays aligned with enum ordinalsAppending
"Button OnOff"at the end of theusageselect values matches the newIO_PinUsageordinal, so stored integer usages won’t shift. The mapping between enum and UI label remains consistent.
270-280: SE16V1 preset correctly wires GPIO 46 to the new On/Off button usageMapping
pins[46]["usage"]topin_Button_OnOffcleanly introduces the dedicated On/Off button for the SE16V1 preset without conflicting with the LED pin array or other button assignments. This looks coherent with the new button semantics.src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
31-31: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and aligns with the broader refactoring pattern across the codebase.interface/svelte.config.js (1)
11-13: LGTM! Single bundle strategy appropriate for embedded deployment.The
bundleStrategy: 'single'configuration is well-suited for embedded systems where filesystem constraints (limited file count in LittleFS) make a single bundle preferable over multiple chunks.src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
35-35: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and aligns with the broader refactoring pattern.src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
126-126: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and is consistent with the broader refactoring pattern across driver and effect nodes.src/MoonBase/Utilities.h (1)
230-234: LGTM! Useful cross-size concatenation operator.The templated
operator+=enables concatenation betweenCharinstances of different sizes, mirroring the existingoperator+=(const char*)pattern. The use ofstrlcatensures buffer safety.src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
121-121: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and aligns with the broader refactoring pattern.src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
69-69: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and is consistent with the broader refactoring pattern across the codebase.src/MoonBase/Nodes.cpp (2)
17-17: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies. The method body only reads from the control object, making const-correctness appropriate.
401-401: LGTM! Const-correctness improvement.The signature change to pass
controlby const-reference avoids unnecessary copies and aligns with the broader refactoring pattern. The method implementation only reads from the control object.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
926-930: Const-refonUpdateoverrides look correct.Both
RubiksCubeEffect::onUpdateandParticlesEffect::onUpdatenow match the baseNode::onUpdatesignature and only read fromcontrol, so passingconst JsonObject&is appropriate and avoids copies.Also applies to: 1122-1126
src/MoonBase/NodeManager.h (1)
260-261: Control object plumbing now correctly uses a sharedJsonObjectreference.Using a local
JsonObject control = nodeState["controls"][updatedItem.index[1]];and passing it by const reference intoupdateControl(control)andonUpdate(oldValue, control)matches the updated Node API and avoids redundant lookups/copies.Also applies to: 267-269
src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (1)
29-31:version/statuscontrols integrate cleanly with the Node control API.Exposing
versionandstatusas read-only"text"controls and refreshing them viaupdateControl("version", HP_ALL_VERSION)/"status"lines up with the newNode::updateControl(const char*, T)helper and should give the UI immediate visibility into driver state.Also applies to: 42-45
src/MoonBase/Nodes.h (1)
56-57: Node–module linkage and control helpers align well with the new architecture.
- Adding
Module* moduleNodesand using it (via callers) to flipModule::requestUIUpdatecleanly connects nodes to the centralized UI update path.- Switching
constructorandonUpdate/updateControlto takeconstreferences avoids unnecessary Json copies and matches how you now pass control objects around fromNodeManager.addControl’s explicitJsonObject control = JsonObject();plus the"valid"flagging continues to support reuse of existing controls.- The new
addControlValuehelper with a size-0 guard safely appends values to the last control, addressing the previous potential underflow.updateControl(const char* name, const T value)is a nice utility that you’re already using inPhysicalDriverfor"version"/"status".- Extending
nextPinto accept an optionalledPinDIOand delegating tolayer->layerP->nextPin(ledPinDIO)fits the new per-pin assignment flow.Overall these changes look consistent and low-risk; any future nodes that rely on
moduleNodesjust need to ensure their owningNodeManagerinitializes it, as you’ve done inModuleDriversandModuleEffects.Also applies to: 65-68, 82-105, 145-147, 152-159, 162-172, 174-175, 199-200, 286-287
|
I have this in my logging: [ 1465][D][Nodes.cpp:403] onUpdate(): [💫] loopTask (3504) status: ok You can see status is created as {"name":"status","type":"text","default":"ok","p":1009122202,"ro":true,"max":32,"valid":true,"value":"ok","size":32} But then a moment later there is only {"size":32}. Status is a new control in the Physical driver node, so the json for physical driver doesn't contain the status, All other - already existing controls in physical driver work fine. This is managed by NodeManager.h. Can you check what could be wrong? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14390 🏁 Script executed: Length of output: 7551 🏁 Script executed: Length of output: 12713 🏁 Script executed: Length of output: 24180 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2004 🏁 Script executed: Length of output: 0 @ewowi, I found the issue! The problem is in the template Here's the problematic code: template <typename T>
void updateControl(const char* name, const T value) {
for (JsonObject control : controls) {
if (control["name"] == name) {
control["value"] = value;
updateControl(control);
break;
}
}
}The issue is that when iterating with The fix is to use indexed access instead of range-based iteration to ensure we're working with a stable reference: template <typename T>
void updateControl(const char* name, const T value) {
for (size_t i = 0; i < controls.size(); i++) {
JsonObject control = controls[i];
if (control["name"] == name) {
control["value"] = value;
updateControl(control);
break;
}
}
}This ensures that when we access The reason your existing controls work fine is that they already exist in the state before the first access, but the new "status" control is being added dynamically and might not be fully synchronized between the node's control array and the state's control array when Can you try this fix and let me know if it resolves the issue? |
for (JsonObject control ... statements in the repo. From what I understand all of these are okay to use like that. Please dig deeper |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1389 🏁 Script executed: Length of output: 916 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2048 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3707 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1195 🏁 Script executed: Length of output: 1189 🏁 Script executed: Length of output: 148 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 216 🏁 Script executed: Length of output: 1946 🏁 Script executed: Length of output: 2830 🏁 Script executed: Length of output: 1538 🏁 Script executed: Length of output: 271 🏁 Script executed: Length of output: 152 🏁 Script executed: Length of output: 5202 🏁 Script executed: Length of output: 2602 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 11462 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1200 @ewowi, I found the root cause! The issue is in the Here's what's happening:
Look at Module.cpp, lines 240-246: } else { // if property is key/value
if (key != "p") {
changed = true;
updatedItem.name = key;
updatedItem.oldValue = stateValue;
stateData[key.c_str()] = newValue; // ← THIS LINE OVERWRITES!
updatedItem.value = stateData[key.c_str()];When The fix: Controls added after initial setup (in Suggested solution: In Try this fix in Module.cpp, around line 240-246: } else { // if property is key/value
if (key != "p") {
// NEW: For controls array items, preserve control structure if incoming data is incomplete
bool isControlInArray = (parent == "controls" && depth == 1);
bool shouldPreserve = isControlInArray &&
(key == "name" || key == "type" || key == "ro" ||
key == "max" || key == "min" || key == "desc" || key == "default") &&
newValue.isNull();
if (!shouldPreserve) {
changed = true;
updatedItem.name = key;
updatedItem.oldValue = stateValue;
stateData[key.c_str()] = newValue;
updatedItem.value = stateData[key.c_str()];This preserves control metadata properties when the incoming data doesn't have them, preventing them from being nulled out. Can you try this fix? |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2510 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 404 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 5359 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 4477 🏁 Script executed: Length of output: 2048 🏁 Script executed: Length of output: 6178 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1272 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1571 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 18492 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1610 🏁 Script executed: Length of output: 791 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 4078 🏁 Script executed: Length of output: 281 🏁 Script executed: Length of output: 810 🏁 Script executed: Length of output: 0 @ewowi, I think I've found the root cause! Let me explain what's happening: When The key issue is in NodeManager.h lines 161-179: // invalidate controls
if (nodeState["controls"].isNull()) {
nodeState["controls"].to<JsonArray>();
} else {
for (JsonObject control : nodeState["controls"].as<JsonArray>()) {
control["valid"] = false; // ← Mark all controls as invalid
}
}
Node* nodeClass = addNode(updatedItem.index[0], updatedItem.value, nodeState["controls"]); // set controls to valid
// remove invalid controls
for (int i = nodeState["controls"].as<JsonArray>().size() - 1; i >= 0; i--) {
JsonObject control = nodeState["controls"][i];
if (!control["valid"].as<bool>()) {
EXT_LOGD(ML_TAG, "remove control %d", i);
nodeState["controls"].remove(i); // ← Remove invalid controls!
}
}This logic is designed to sync controls when a node name changes. But here's the problem:
The fix: In D_PhysicalDriver.h, after adding the status control in void onLayout() override {
// ... existing code ...
// Add status control
addControl(status, "status", "text", 0, 32, true);
status["valid"] = true; // ← ADD THIS LINE!
// ... rest of code ...
}Or better yet, modify the cleanup logic in NodeManager.h to only remove controls that are explicitly marked invalid, not controls that don't have the "valid" key at all. Change line 175: if (!control["valid"].as<bool>()) {To: if (control.containsKey("valid") && !control["valid"].as<bool>()) {This way, controls without the "valid" key (newly added ones) won't be removed. Can you try either of these fixes? |
[ 547][I][WiFiSettingsService.cpp:103] reconfigureWiFiConnection(): [🐼] Reconfiguring WiFi connection to: STRENGTH
|
|
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: 1348 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1177 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 9108 🏁 Script executed: Length of output: 1195 🏁 Script executed: Length of output: 3612 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1629 🏁 Script executed: Length of output: 5212 🏁 Script executed: Length of output: 2747 🏁 Script executed: Length of output: 8413 🏁 Script executed: Length of output: 2807 🏁 Script executed: Length of output: 2388 @ewowi, You're absolutely right - I apologize for not analyzing the logs carefully enough. Let me trace through what's actually happening: Key observations from the logs:
The (#2) is the smoking gun! This means state synchronization is happening again—likely from the UI sending its current state back to the backend after connecting. Root cause: When the UI connects (around timestamp [363948]), it sends its cached state to sync with the backend. That cached state was persisted before the new "status" control existed. When
The problem is in Module.cpp around lines 230-240. When comparing array items (like controls arrays), if the arrays have different sizes or if an object exists in both but has different properties, Here's the actual bug: Look at Module.cpp lines 131-135: for (JsonPair newProperty : newData.as<JsonObject>()) {
if (stateData[newProperty.key()].isNull()) {
stateData[newProperty.key()] = nullptr; // Initialize the key in stateData if it doesn't exist
}
}This initializes keys from newData into stateData. So if the UI sends But wait—there's another path: If the persisted state file ( The fix: In D_PhysicalDriver.h, the status control is being added dynamically in Try this solution: In D_PhysicalDriver.h, after adding the status control (around line 44-45), you need to ensure it's added to the node's controls array which will be synced to Check if there's a timing issue where:
Look at NodeManager.h line 168 where Node* nodeClass = addNode(updatedItem.index[0], updatedItem.value, nodeState["controls"]);The controls JsonArray is passed to setup(), but if there's a state sync happening immediately after, it could overwrite it. Quick fix to test: Can you add this logging right after the status control is added in setup(): addControl(status, "status", "text", 0, 32, true);
// ADD THIS:
EXT_LOGD(ML_TAG, "status control added, controls array size: %d", controls.size());
String dbg;
serializeJson(controls[controls.size()-1], dbg);
EXT_LOGD(ML_TAG, "last control: %s", dbg.c_str());This will help us see if the control is properly added to the array, and whether it survives until the state sync happens. |
@coderabbitai, added:
Yes looks like it! here is the updated logging: [ 1358][D][ModuleDrivers.h:158] addNode(): [💫] loopTask (3568) Physical Driver ☸️ (p:0x3c25fb54 pr:1) Also note this is all happening during boot, running in loopTask, so even before the app tasks are created and loopTask is destroyed, so not much concurrency going on yet (read: other tasks interfering) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8413 🏁 Script executed: Length of output: 3679 🏁 Script executed: Length of output: 449 🏁 Script executed: Length of output: 1177 🏁 Script executed: Length of output: 9095 🏁 Script executed: Length of output: 4285 🏁 Script executed: Length of output: 2560 @ewowi, I think I've found the issue! The problem is in how Here's what's happening:
The key question: What does To verify this theory, can you:
// Add before line 281
if (_moduleName == "drivers") {
String rootStr, stateStr;
serializeJson(root, rootStr);
serializeJson(state.data, stateStr);
EXT_LOGD(ML_TAG, "BEFORE compareRecursive - root: %s", rootStr.c_str());
EXT_LOGD(ML_TAG, "BEFORE compareRecursive - state: %s", stateStr.c_str());
}
if (state.compareRecursive("", state.data, root, updatedItem)) {This will show us exactly what's in the file vs what's in memory before they're compared. 🏁 Script executed: Length of output: 52 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 5202 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 673 🏁 Script executed: Length of output: 236 @ewowi, I need your help to pinpoint exactly where the properties are being stripped. Based on my analysis of However, something IS stripping the properties. Can you add this specific logging to help us track it down? Add this logging in NodeManager.h around line 247 (right before the log that shows "on: true (#2)"): else if (updatedItem.name == "on" && updatedItem.parent[1] == "") { // nodes[i].on
// ADD THIS BLOCK:
if (strcmp(nodeState["name"].as<const char*>(), "Physical Driver ☸️") == 0) {
String ctrlStr;
serializeJson(nodeState["controls"], ctrlStr);
EXT_LOGD(ML_TAG, "BEFORE on handler - controls: %s", ctrlStr.c_str());
}
if (updatedItem.index[0] < nodes->size()) {
EXT_LOGD(ML_TAG, "%s on: %s (#%d)", nodeState["name"].as<const char*>(), updatedItem.value.as<String>().c_str(), nodes->size());
// ADD THIS BLOCK TOO:
if (strcmp(nodeState["name"].as<const char*>(), "Physical Driver ☸️") == 0) {
String ctrlStr2;
serializeJson(nodeState["controls"], ctrlStr2);
EXT_LOGD(ML_TAG, "AFTER on log - controls: %s", ctrlStr2.c_str());
}Also, add this at the END of Module.cpp's compareRecursive function (right before the // ADD THIS before return:
if (parent == "controls" && depth == 1) {
String stateStr, newStr;
serializeJson(stateData, stateStr);
serializeJson(newData, newStr);
EXT_LOGD(MB_TAG, "compareRecursive controls[%d] - state: %s", index, stateStr.c_str());
EXT_LOGD(MB_TAG, "compareRecursive controls[%d] - new: %s", index, newStr.c_str());
}
return changed;
}This will show us:
Can you add this logging and share the new output? |
[ 10070][D][ModuleDrivers.h:158] addNode(): [💫] loopTask (3056) Physical Driver ☸️ (p:0x3c26107c pr:1) I think I know what is going on: compare recursive reads from file and there is no status (yet) so it removes the just added status (remove nodes.controls[3] d: 0 ({})). This is no issue if there is a new node added (there is nothing on file), but for existing nodes it needs to remove if things are not added in addControl, and it needs to add if it is in addControl. So this looks like a matter of the sequence of things, e.g. first complete compare recursive so it matches what is on file and do addControl() statements after that. Can you suggest how to do that? |
|
Tip For best results, initiate chat on the files or code changes. @ewowi, Excellent analysis! You've identified the root cause perfectly. The issue is that The good news is that the "valid" flag mechanism already exists for exactly this purpose! Looking at your logs, the status control is added with The fix: Modify Around line 209 in Module.cpp where it says: EXT_LOGD(MB_TAG, "remove %s%s[%d] d: %d (%s)", parent.c_str(), stateArray.name().c_str(), index, depth, dbg2.c_str());
stateArray.remove(index);Change it to: // Don't remove array items that are marked as valid (e.g., newly added controls)
JsonObject itemObj = stateData.as<JsonObject>();
bool isValid = itemObj.containsKey("valid") && itemObj["valid"].as<bool>();
if (!isValid) {
EXT_LOGD(MB_TAG, "remove %s%s[%d] d: %d (%s)", parent.c_str(), stateArray.name().c_str(), index, depth, dbg2.c_str());
stateArray.remove(index);
} else {
EXT_LOGD(MB_TAG, "skip remove %s%s[%d] d: %d - marked valid (%s)", parent.c_str(), stateArray.name().c_str(), index, depth, dbg2.c_str());
}This way:
Can you try this fix? |
@coderabbitai, nice suggestion but I think this is better: else if (i >= newArray.size()) { // newArray has deleted a row Can you do an after action review if this fits the general architecture of the compareRecursive mechanism and it is not only a fix for this particular case?: Why is this needed for controls to work and will it work for other (edge) cases as well, e.g. if a node will remove an earlier saved control? Or for non-node related controls : we have also module controls (see Module.h/.cpp): what if a control will be added or removed there? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2307 🏁 Script executed: Length of output: 4449 🏁 Script executed: Length of output: 3864 @ewowi, Excellent fix! 🎉 Let me provide a comprehensive architectural review: ✅ Architectural FitYour fix is architecturally sound and leverages the existing "valid" flag mechanism that's already central to the node control lifecycle. Here's why: The "valid" flag pattern:
This is a natural extension of the existing architecture, not a one-off patch. 🎯 Edge Case Analysis1. Node removes a previously saved control ✅Scenario: A node's What happens:
Verdict: ✅ Works correctly 2. Module controls (non-node controls)
|
Back end ======== - Module: compareRecursive: do not remove valid node controls - modules: add addControlValue, rename root to controls or stateJson or newData, rename property to control, details to rows, use addControl() and addControlValue() - Module drivers and effects: addNodes argument from values to control - Layout MoonLight: PanelsLayout: add panelsPerPin
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonBase/Nodes.h (1)
135-139: Fix the coord3D type check inaddControlThe coord3D branch currently reads:
} else if (control["type"] == "coord3D") { if (!sizeof(!std::is_same<ControlType, Coord3D>::value)) { EXT_LOGE(ML_TAG, "type for %s is not Coord3D", name); } else control["size"] = sizeof(Coord3D); }
sizeof(!std::is_same<...>::value)is always 1, so theifcondition is always false and the error log never triggers, even ifControlTypeis notCoord3D.Suggested fix to mirror the checkbox branch:
- } else if (control["type"] == "coord3D") { - if (!sizeof(!std::is_same<ControlType, Coord3D>::value)) { - EXT_LOGE(ML_TAG, "type for %s is not Coord3D", name); - } else - control["size"] = sizeof(Coord3D); - } else + } else if (control["type"] == "coord3D") { + if (!std::is_same<ControlType, Coord3D>::value) { + EXT_LOGE(ML_TAG, "type for %s is not Coord3D", name); + } else { + control["size"] = sizeof(Coord3D); + } + } elseThis keeps existing behavior for correct uses and makes the error path actually reachable if someone passes the wrong type in the future.
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
137-169: Fix DDP offset handling: useledIndexfor the destination channel indexIn
handleDDP()you correctly compute:uint32_t offset = ...; // byte offset in the LED array uint16_t dataLen = ...; int startPixel = offset / 3; int numPixels = min((uint16_t)(dataLen / 3), (uint16_t)(layer->layerP->lights.header.nrOfLights - startPixel));but then write using
iinstead ofledIndex:for (int i = 0; i < numPixels; i++) { int ledIndex = startPixel + i; if (ledIndex < layer->layerP->lights.header.nrOfLights) { memcpy(&layer->layerP->lights.channels[i * layer->layerP->lights.header.channelsPerLight], &pixelData[i * layer->layerP->lights.header.channelsPerLight], layer->layerP->lights.header.channelsPerLight); } }This ignores
startPixel, so any packet with a non-zero offset overwrites from channel 0 instead of the intended LED range.Suggested fix:
- for (int i = 0; i < numPixels; i++) { - int ledIndex = startPixel + i; - if (ledIndex < layer->layerP->lights.header.nrOfLights) { - memcpy(&layer->layerP->lights.channels[i * layer->layerP->lights.header.channelsPerLight], - &pixelData[i * layer->layerP->lights.header.channelsPerLight], - layer->layerP->lights.header.channelsPerLight); - } - } + for (int i = 0; i < numPixels; i++) { + int ledIndex = startPixel + i; + if (ledIndex < layer->layerP->lights.header.nrOfLights) { + memcpy(&layer->layerP->lights.channels[ledIndex * layer->layerP->lights.header.channelsPerLight], + &pixelData[i * layer->layerP->lights.header.channelsPerLight], + layer->layerP->lights.header.channelsPerLight); + } + }This preserves the existing bounds check while correctly honoring the DDP byte offset.
🧹 Nitpick comments (3)
src/MoonLight/Modules/ModuleLiveScripts.h (1)
93-99: Unnecessary initialization on line 96.The
rowsvariable is initialized tocontrolson line 96, but is immediately reassigned on line 99. This initialization serves no purpose and may confuse readers.Apply this diff:
void setupDefinition(const JsonArray& controls) override { EXT_LOGV(ML_TAG, ""); JsonObject control; // state.data has one or more properties - JsonArray rows = controls; // if a control is an array, this is the rows of the array + JsonArray rows; // if a control is an array, this is the rows of the array control = addControl(controls, "scripts", "rows"); rows = control["n"].to<JsonArray>();src/MoonBase/Modules/ModuleTasks.h (1)
67-72: Consider renamingcontrolstodatafor clarity.The variable
controlson line 69 represents the full JSON data object (containingtasks,core0,core1), not a controls array as used insetupDefinition. This naming may cause confusion.Apply this diff:
JsonDocument doc; doc["tasks"].to<JsonArray>(); - JsonObject controls = doc.as<JsonObject>(); + JsonObject data = doc.as<JsonObject>(); for (UBaseType_t i = 0; i < taskCount; i++) { - JsonObject task = controls["tasks"].as<JsonArray>().add<JsonObject>(); + JsonObject task = data["tasks"].as<JsonArray>().add<JsonObject>();And update the remaining references on lines 123-124 and 128-131.
src/MoonLight/Modules/ModuleLightsControl.h (1)
81-85: Button handling with INPUT_PULLUP + debounce is sound; consider avoiding static localsThe new
pin_Button_OnOffpath correctly:
- Configures the pin as
INPUT_PULLUPwhen discovered.- Checks
pinToggleOnOff != UINT8_MAXbefore reading.- Debounces and triggers only on the HIGH→LOW transition, then pushes a minimal
lightsOndiff viaupdate(..., _moduleName + "server").Given there is effectively a single
ModuleLightsControlinstance, thestaticdebounce state inloop()is fine. If you ever support multiple instances or hot-swapping pins, it would be cleaner to movelastDebounceTime/lastStateinto member fields so their lifetime and ownership are explicit.Also applies to: 289-305
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
docs/develop/modules.md(1 hunks)src/MoonBase/Module.cpp(10 hunks)src/MoonBase/Module.h(5 hunks)src/MoonBase/Modules/FileManager.cpp(3 hunks)src/MoonBase/Modules/FileManager.h(1 hunks)src/MoonBase/Modules/ModuleDevices.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/Modules/ModuleTasks.h(3 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonBase/Nodes.cpp(3 hunks)src/MoonBase/Nodes.h(6 hunks)src/MoonLight/Modules/ModuleChannels.h(1 hunks)src/MoonLight/Modules/ModuleDrivers.h(3 hunks)src/MoonLight/Modules/ModuleEffects.h(2 hunks)src/MoonLight/Modules/ModuleLightsControl.h(3 hunks)src/MoonLight/Modules/ModuleLiveScripts.h(1 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(1 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(1 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(2 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(5 hunks)src/MoonLight/Nodes/Effects/E_MoonModules.h(2 hunks)src/MoonLight/Nodes/Layouts/L_MoonLight.h(9 hunks)src/MoonLight/Nodes/Modifiers/M_MoonLight.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Modules/ModuleLiveScripts.h (4)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)src/MoonBase/Modules/ModuleTasks.h (1)
setupDefinition(23-46)src/MoonLight/Modules/ModuleChannels.h (1)
setupDefinition(24-54)
src/MoonLight/Modules/ModuleMoonLightInfo.h (6)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)src/MoonBase/Modules/ModuleTasks.h (1)
setupDefinition(23-46)src/MoonBase/NodeManager.h (1)
setupDefinition(83-112)src/MoonLight/Modules/ModuleChannels.h (1)
setupDefinition(24-54)src/MoonLight/Modules/ModuleLiveScripts.h (1)
setupDefinition(93-114)
src/MoonBase/Nodes.cpp (6)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
onUpdate(920-924)onUpdate(1116-1120)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onUpdate(120-124)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
src/MoonLight/Nodes/Drivers/D_Infrared.h (3)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (3)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonBase/Nodes.h (1)
nextPin(201-201)src/MoonLight/Layers/PhysicalLayer.cpp (2)
nextPin(176-195)nextPin(176-176)
src/MoonLight/Modules/ModuleChannels.h (1)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Effects/E_MoonModules.h (3)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonLight/Modules/ModuleEffects.h (3)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/NodeManager.h (2)
setupDefinition(83-112)NodeManager(20-34)src/MoonLight/Modules/ModuleChannels.h (1)
setupDefinition(24-54)
src/MoonLight/Modules/ModuleLightsControl.h (1)
src/MoonBase/Module.h (1)
ModuleState(47-137)
src/MoonBase/Modules/ModuleDevices.h (5)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/Modules/ModuleIO.h (1)
setupDefinition(110-214)src/MoonBase/Modules/ModuleTasks.h (1)
setupDefinition(23-46)src/MoonBase/NodeManager.h (1)
setupDefinition(83-112)src/MoonLight/Modules/ModuleLiveScripts.h (1)
setupDefinition(93-114)
src/MoonBase/Modules/ModuleIO.h (2)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)
src/MoonBase/Modules/ModuleTasks.h (4)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)src/MoonBase/Modules/ModuleIO.h (1)
setupDefinition(110-214)src/MoonLight/Modules/ModuleLiveScripts.h (1)
setupDefinition(93-114)
src/MoonBase/NodeManager.h (1)
src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (2)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)
src/MoonBase/Module.h (2)
lib/framework/NTPSettingsService.h (1)
update(66-73)lib/framework/StatefulService.h (3)
update(123-134)update(144-155)StateUpdateResult(26-106)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonBase/Module.cpp (4)
src/MoonBase/Module.h (1)
postUpdate(116-127)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)src/MoonBase/Modules/ModuleTasks.h (1)
setupDefinition(23-46)src/MoonLight/Modules/ModuleLiveScripts.h (1)
setupDefinition(93-114)
src/MoonBase/Nodes.h (1)
src/MoonBase/Nodes.cpp (4)
onUpdate(400-540)onUpdate(400-400)updateControl(17-66)updateControl(17-17)
⏰ 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 (44)
docs/develop/modules.md (1)
41-59: Documentation accurately reflects the new control-definition API refactoring.The example correctly illustrates the parameter rename (
root→controls), therowspattern for managing nested arrays, and the builder-styleaddControl()/addControlValue()API. Verification confirms this pattern is consistently used across allsetupDefinitionimplementations in the codebase (ModuleMoonLightInfo, ModuleLiveScripts, ModuleEffects, NodeManager, ModuleLightsControl, ModuleChannels, ModuleTasks, ModuleIO, ModuleDevices). The inline comment and the builder approach align well with the PR's shift toward helper-driven control construction.src/MoonBase/Modules/ModuleDevices.h (1)
33-45: setupDefinition refactor is consistent with other modulesThe updated signature and the
controls/rowsusage match the shared pattern used inModuleTasks,NodeManager, andModuleLiveScripts. Control definitions fordevices(name/ip/time/mac) remain read‑only and behaviorally unchanged. No issues from this change.src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
50-56: LGTM! Label naming is now consistent.The "version" label is now lowercase, matching the naming convention of other controls in this
setup()method (chipSet,colorOrder, etc.).src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
99-106: LGTM! Clean migration to addControlValue pattern.The ruleset select control now uses the
addControl/addControlValuebuilder pattern, which is cleaner and consistent with the PR-wide refactor.
120-124: LGTM! Const-reference parameter improves efficiency.Changing from
const JsonObject controltoconst JsonObject& controlavoids an unnecessary copy. This aligns with the same signature change in other files (e.g.,D_VirtualDriver.h,E__Sandbox.h).src/MoonLight/Modules/ModuleLiveScripts.h (1)
101-112: LGTM! Control definitions migrated to addControl pattern.The scripts row controls are now properly defined using
addControlcalls, consistent with other modules likeModuleDevices.handModuleTasks.h.src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
22-44: LGTM! Clean migration to addControl pattern.The
setupDefinitionmethod now uses the standardizedcontrolsparameter andaddControl/addControlValuepattern, consistent with other modules across the PR. The layers row definition is properly structured.src/MoonBase/Modules/ModuleIO.h (4)
76-77: LGTM! New enum value correctly positioned.The
pin_Button_OnOffenum value is added at the correct position (afterpin_Ethernet), and the corresponding UI label "Button OnOff" is added at line 204 in matching order.
110-213: LGTM! Clean migration to addControl/addControlValue pattern.The
setupDefinitionmethod is properly refactored to use the new builder pattern. The "Button OnOff" label at line 204 correctly matches thepin_Button_OnOffenum position.
273-276: LGTM! SE16V1 board preset uses the new On/Off button pin.Pin 46 is now assigned to
pin_Button_OnOff, enabling the new on/off toggle functionality for this board.
375-384: Informative comment about ESP32-D0-16MB compatibility.The commented-out pin assignments with the note about ESP32-D0-16MB limitations are helpful for future maintainers. Consider converting this to a more formal documentation format if these limitations become relevant.
src/MoonBase/Modules/ModuleTasks.h (2)
23-45: LGTM! Clean migration to the new pattern.The
setupDefinitionmethod correctly uses thecontrolsparameter andaddControlcalls withrowsfor nested task properties.
123-131: LGTM! Core task names and update call are correct.The core0/core1 names are properly stored and the update call correctly emits the state changes.
src/MoonLight/Modules/ModuleLightsControl.h (1)
28-29: Sentinel initialization for pin fields is appropriateUsing
UINT8_MAXas the “not configured” value forpinRelaisBrightnessandpinToggleOnOffmatches the later checks (!= UINT8_MAX) and avoids the old signed/unsigned mismatch you’d get with-1on auint8_t.src/MoonLight/Nodes/Layouts/L_MoonLight.h (1)
249-281: Per-pin selection for SingleLine/SingleRow layouts is coherentThe new
ledPinDIOselect (with “Default” +LED 01..MAXLEDPINS) combined with:nextPin(ledPinDIO == 0 ? UINT8_MAX : ledPinDIO - 1);fits the
PhysicalLayer::nextPincontract (0-based pin index vs.UINT8_MAX= default ordering) and keeps default behavior when left at 0.Given
MAXLEDPINSis the compile-time ceiling andModuleDrivers::readPinscompacts actual pins intoledPins[], this is a reasonable first version of per-layout pin override.Also applies to: 290-322
src/MoonBase/Nodes.h (1)
64-68: Control plumbing improvements look goodThe changes here tighten up the Node control API:
constructor(VirtualLayer*, const JsonArray&)keeps a live handle to the controls array without copying.addControlValue(const T& value)now guards against an emptycontrolsarray and appends to the last control’svalueslist, which is exactly what the various layout/effectselectcontrols expect.- The
updateControl(const char* name, const T value)overload is a useful helper: it updates"value"and delegates toupdateControl(const JsonObject&), ensuring all update side effects stay centralized.nextPin(uint8_t ledPinDIO = UINT8_MAX)simply forwards tolayerP->nextPin(ledPinDIO), aligning Node code with the new per-pin mapping semantics.No behavioral concerns here.
Also applies to: 154-161, 163-174, 200-202
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
278-287: Rotate direction select wiring matches runtime logicThe new
directionselect with values"Clockwise","Counter-Clockwise", and"Alternate"correctly maps to the existing logic (direction == 0/1/2inloop()), and the use ofaddControlValuekeeps it consistent with other selects in the codebase.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
222-245: Select control refactors and const-correct onUpdate are consistent
ScrollingTextEffect’sfontandpresetselects now enumerate options viaaddControlValue, and the resulting indices line up with thechoice/switchlogic (cases 1–9).WaveEffect’stypeselect defines six entries, matching theswitch (type)cases 0–5.FreqSawsEffect’smethodselect defines three entries, matching theif (method == 0/1/2)paths.RubiksCubeEffect::onUpdateandParticlesEffect::onUpdatenow takeconst JsonObject&, which matches the updated base signature and avoids copying control objects.No functional issues spotted in these refactors.
Also applies to: 526-536, 599-610, 919-924, 1116-1120
src/MoonBase/Modules/FileManager.h (1)
41-44: Renamed parameters clarify intent of FileManager state APIsSwitching the
FilesState::read/updateparameter names tostateJsonandnewDatamakes their roles clearer and aligns with the broader naming cleanup in Module/ModuleState. Assuming the .cpp definitions were updated accordingly, this is a straight semantic win.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
33-45: Art-Net/DDP controls and onUpdate signature are wired correctly
DDPcheckbox,portnumber, andviewselect (with “Physical layer” + per-layer entries) are now built via the standard Node control helpers, matching patterns elsewhere in the codebase.- Updating
onUpdateto takeconst JsonObject& controlis consistent with the new base signature.The current onUpdate body is a no-op aside from checking
"DDP"; that’s fine if protocol switching is handled elsewhere or still WIP.Also applies to: 47-53
src/MoonLight/Modules/ModuleDrivers.h (1)
51-57: Driver module node wiring and pin compaction look consistent
readPins()now compactslayerP.ledPinspurely on!= UINT8_MAX, which keeps only configured LED pins and simplifies the logic. GivenledsPerPin[]is computed later during mapping, this is acceptable.addNodes(const JsonObject& control)correctly populates the driver/layout selector usingaddControlValue(control, ...)for all supported nodes, includingSE16Layout, and aligns with the updated NodeManager API.- In
addNode(), wiringmoduleControl,moduleIO, and the newmoduleNodes = (Module*)thisensures each Node has access to lights control, IO, and its owning Module for UI updates.No issues spotted in these changes.
Also applies to: 78-102, 160-164
src/MoonLight/Nodes/Drivers/D_Infrared.h (2)
86-89: LGTM! Clean migration to addControl/addControlValue pattern.The refactor from manual JsonObject construction to the helper-based approach is consistent with the codebase-wide changes and improves maintainability.
123-123: Consistent signature update to const reference.The
const JsonObject¶meter improves const-correctness and avoids unnecessary copies. This aligns with otheronUpdateimplementations across the codebase (e.g.,D_VirtualDriver.h,D__Sandbox.h).src/MoonLight/Modules/ModuleChannels.h (1)
24-54: LGTM! Properly migrated to controls-based API.The refactor correctly:
- Renames parameter to
controlsfor clarity- Uses
addControl()to create controls andaddControlValue()to populate select options- Maintains the same functional behavior while improving consistency with the Module base class API
src/MoonBase/Modules/FileManager.cpp (2)
45-56: Consistent parameter naming in read().The rename from
roottostateJsonimproves clarity and aligns with the broader API refactor acrossModuleState::readimplementations.
58-132: Consistent parameter naming in update().The rename from
roottonewDataclarifies the purpose of the parameter (incoming data to be applied) and aligns with theModuleState::updatesignature changes.src/MoonBase/Nodes.cpp (3)
17-66: Simplified updateControl signature.The removal of
oldValueparameter and use ofconst JsonObject&is appropriate since the function operates on the control object directly and doesn't need the old value for its current implementation.
348-362: Clean migration to addControlValue pattern for light presets.The refactor from manual array construction to sequential
addControlValue()calls is more readable and consistent with the codebase convention.
400-400: Consistent onUpdate signature.The
const JsonObject¶meter aligns with other driver implementations.src/MoonLight/Modules/ModuleEffects.h (3)
60-75: LGTM! Clean setupDefinition refactor.The migration to
addControl()andaddControlValue()patterns is consistent with the base class API. The layer select control properly populates options viaaddControlValue(control, i).
77-147: Signature change enables cleaner value population.Changing
addNodes(const JsonArray& values)toaddNodes(const JsonObject& control)allows direct use ofaddControlValue(control, ...)which is more consistent with the module-wide pattern.
270-272: Commented module assignments are intentional and correct for effect nodes.Effect nodes only require
moduleNodesto request UI updates. Driver nodes (like D_Infrared) require bothmoduleControlandmoduleIOand receive these assignments in ModuleDrivers.h. Effect nodes do not use these members, so the commented assignments at lines 270-271 are intentional and should remain commented out.src/MoonBase/NodeManager.h (3)
78-78: Consistent addNodes signature change.The change from
const JsonArray& valuestoconst JsonObject& controlaligns with the pattern where the control object contains the values array, enablingaddControlValue()usage.
83-112: Clean setupDefinition refactor with nested control structure.The refactor properly builds the nodes/controls hierarchy using
addControl()and stores nested arrays incontrol["n"]. The pattern is consistent with other module implementations.
241-257: Improved control object passing to node methods.Extracting the control object (
nodeState["controls"][updatedItem.index[1]]) and passing it directly toupdateControl()andonUpdate()is cleaner than passing the entire nodeState. This aligns with the simplifiedupdateControl(const JsonObject& control)signature.src/MoonBase/Module.h (4)
99-99: Consistent setupDefinition signature in std::function.The parameter rename from
roottocontrolsin the std::function type matches the virtual method signature.
111-112: Clear parameter naming for read/update signatures.Renaming to
stateJsonandnewDataclarifies the purpose of each parameter in the static methods.
142-166: Centralized UI update handling in Module::loop().Moving
requestUIUpdatehandling to the baseModuleclass is a good design decision. This eliminates the need for derived classes (likeNodeManager) to implement their own loop override for this purpose. The implementation correctly:
- Resets the flag before processing
- Logs the action
- Triggers an update with the appropriate origin ID
185-190: Useful generic addControlValue template.The template function provides a clean, type-safe way to add values to a control's values array. The lazy initialization of the
valuesarray (to<JsonArray>()if null) is a nice defensive touch.src/MoonBase/Module.cpp (5)
18-36: Consistent setDefaults refactor.The parameter rename from
roottocontrolsanddefinitionmaintains clarity. The function correctly iterates over the definition array and sets defaults on the controls object.
63-67: Clear read() implementation with renamed parameter.The
stateJsonparameter name clearly indicates this is the output JSON being populated from state.
196-221: Key fix: Preserve valid controls during state sync.This is the critical fix addressing the PR objectives. The logic correctly:
- Checks if the array item is marked as
valid(line 198)- If not valid, proceeds with the existing removal logic (lines 200-215)
- If valid, skips removal and logs the action (lines 217-220)
This prevents newly added controls (marked valid in
setup()) from being incorrectly removed when the persisted/UI state lacks those controls.
271-302: Clean update() implementation with clear parameter naming.The rename to
newDataclarifies that this is the incoming data to be compared and merged with existing state. The logic flow remains unchanged.
352-370: Consistent setupDefinition and addControl implementations.The base implementations correctly use the
controlsparameter name and demonstrate the expected pattern for derived classes.
Back end ======== - Nodes: addControl coord3D fix - Artnet in: channel offset fix - Panales: PanelsPerPin fix
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
159-167: Align DDP offset/length math withchannelsPerLightto prevent overruns
startPixelandnumPixelsare computed with/ 3, but the updated memcpy useschannelsPerLightbytes per pixel. WhenchannelsPerLight != 3, this can overreadpixelDataand corrupt LED mapping.Suggested fix:
- // Calculate starting pixel from byte offset (3 bytes per pixel) - int startPixel = offset / 3; - int numPixels = min((uint16_t)(dataLen / 3), (uint16_t)(layer->layerP->lights.header.nrOfLights - startPixel)); + // Calculate starting pixel from byte offset (channelsPerLight bytes per pixel) + uint8_t cpl = layer->layerP->lights.header.channelsPerLight; + if (cpl == 0) return; // defensive guard + int startPixel = offset / cpl; + int numPixels = min( + (uint16_t)(dataLen / cpl), + (uint16_t)(layer->layerP->lights.header.nrOfLights - startPixel)); @@ - int ledIndex = startPixel + i; - if (ledIndex < layer->layerP->lights.header.nrOfLights) { - memcpy(&layer->layerP->lights.channels[ledIndex * layer->layerP->lights.header.channelsPerLight], &pixelData[i * layer->layerP->lights.header.channelsPerLight], layer->layerP->lights.header.channelsPerLight); - } + int ledIndex = startPixel + i; + if (ledIndex < layer->layerP->lights.header.nrOfLights) { + memcpy( + &layer->layerP->lights.channels[ledIndex * cpl], + &pixelData[i * cpl], + cpl); + }This keeps DDP parsing consistent with the configured channel layout per LED and avoids source-buffer overruns.
🧹 Nitpick comments (1)
src/MoonLight/Modules/ModuleLightsControl.h (1)
241-244: Consider usingstatic constexprinstead of#definefor debounce delay.The
#define debounceDelay 50macro works but doesn't respect scope and can cause issues if this identifier is used elsewhere. A typed constant would be safer.- //see pinToggleOnOff - unsigned long lastDebounceTime = 0; - #define debounceDelay 50 // 50ms debounce - int lastState = HIGH; + // see pinToggleOnOff + unsigned long lastDebounceTime = 0; + static constexpr unsigned long debounceDelay = 50; // 50ms debounce + int lastState = HIGH;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/develop/modules.md(1 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonBase/Nodes.h(6 hunks)src/MoonLight/Modules/ModuleLightsControl.h(4 hunks)src/MoonLight/Modules/ModuleLiveScripts.h(1 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)src/MoonLight/Nodes/Layouts/L_MoonLight.h(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/MoonLight/Modules/ModuleLiveScripts.h (4)
src/MoonLight/Modules/ModuleLightsControl.h (1)
setupDefinition(92-138)src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-47)src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
setupDefinition(22-45)src/MoonLight/Modules/ModuleEffects.h (1)
setupDefinition(60-147)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (2)
src/MoonBase/Nodes.h (1)
nextPin(202-202)src/MoonLight/Layers/PhysicalLayer.cpp (2)
nextPin(176-195)nextPin(176-176)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (5)
src/MoonBase/NodeManager.h (1)
onUpdate(115-266)src/MoonLight/Modules/ModuleLightsControl.h (1)
onUpdate(141-212)src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
onUpdate(123-193)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
src/MoonLight/Modules/ModuleLightsControl.h (2)
src/MoonBase/Module.cpp (6)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)update(271-302)update(271-271)src/MoonBase/Module.h (1)
ModuleState(47-137)
src/MoonBase/NodeManager.h (3)
src/MoonBase/Nodes.h (9)
void(65-68)void(71-71)void(191-191)void(192-192)void(193-193)void(196-196)void(205-205)Node(46-153)Node(209-209)src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonBase/Nodes.h (4)
src/MoonLight/Modules/ModuleLiveScripts.h (1)
onUpdate(117-142)src/MoonBase/Nodes.cpp (4)
onUpdate(400-540)onUpdate(400-400)updateControl(17-66)updateControl(17-17)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
⏰ 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 (21)
src/MoonBase/NodeManager.h (3)
83-111: LGTM! Clean refactor to addControl/addControlValue pattern.The setupDefinition implementation now uses the consistent builder pattern with
addControlandaddControlValuehelpers. The nested structure fornodesandcontrolsis correctly established using therowsarray pattern.
241-256: LGTM! Proper control extraction before update.The code correctly extracts the specific control object before calling
updateControlandonUpdate, ensuring the node receives the actual control data rather than raw state elements.
78-78: All callers and implementations ofaddNodesare correctly updated for the new signature.The signature change from
const JsonArray& valuestoconst JsonObject& controlis consistently applied:
- Base virtual declaration at line 78 has the new signature
- Call site at line 94 passes the correct
JsonObjectparameter- All overrides in
ModuleDrivers.handModuleEffects.hmatch the new signatureNo signature mismatches detected.
src/MoonLight/Modules/ModuleLightsControl.h (4)
28-29: LGTM! Proper initialization to invalid pin value.Using
UINT8_MAXas the invalid/unassigned sentinel is consistent and appropriate for GPIO pin variables.
72-89: LGTM! Pin configuration properly handles INPUT_PULLUP.The
readPins()function correctly resets both pins toUINT8_MAXbefore scanning, and configurespinToggleOnOffwithINPUT_PULLUPmode as recommended in the previous review.
92-138: LGTM! Clean refactor to addControl/addControlValue pattern.The setupDefinition now uses the consistent builder pattern. All control definitions follow the established convention with proper defaults and value options.
293-306: LGTM! Debounce logic correctly implemented.The implementation properly:
- Checks if the pin is configured (
!= UINT8_MAX)- Applies debounce timing before accepting state changes
- Triggers only on button press (HIGH→LOW for INPUT_PULLUP)
- Updates
lastStateafter processingThis addresses the previous review feedback about debouncing and edge detection.
src/MoonBase/Nodes.h (6)
56-56: LGTM! Added module reference for UI updates.The
moduleNodespointer enables nodes to request UI updates when needed, aligning with the refactored update signaling approach.
65-68: LGTM! Const-correctness improvement.Changing the
controlsparameter toconst JsonArray&correctly reflects that the constructor shouldn't modify the passed array reference.
155-162: LGTM! Guard against empty controls addressed.The guard
if (controls.size() == 0) return;prevents undefined behavior from the underflow issue identified in the previous review. The implementation correctly appends values to the last control's values array.
165-175: LGTM! Convenient template for updating controls by name.The
updateControl(const char* name, const T value)template provides a clean API to update a control's value and propagate the change throughupdateControl(control).
177-177: LGTM! Const-correctness for onUpdate.Changing to
const JsonObject&correctly indicates the control should not be modified by the handler.
202-202: LGTM! Extended nextPin with optional LED pin assignment.The optional
ledPinDIOparameter enables per-pin LED assignment, forwarding to the physical layer's implementation.docs/develop/modules.md (1)
41-61: LGTM! Documentation accurately reflects the new API pattern.The updated example correctly demonstrates:
- The renamed
controlsparameter- The
rowspattern for nested arrays- Proper use of
addControlandaddControlValue- Capturing the control before setting defaults (addressing previous review feedback)
src/MoonLight/Nodes/Layouts/L_MoonLight.h (5)
94-96: LGTM: Cleaner control definition pattern.The refactor from JSON-based options to explicit
addControlValuecalls improves clarity and aligns with the broader API changes across the codebase.
131-185: Past review concerns resolved.The implementation correctly addresses both issues flagged in the previous review:
- Division by zero prevented: Line 151 enforces a minimum value of 1 for
panelsPerPin, eliminating the possibility of modulo-by-zero.- Final partial group handled: Line 184 ensures any remaining panels after the loop are assigned to a pin.
The solution is cleaner than the originally suggested fix—enforcing the constraint at the control definition level rather than adding runtime guards. The panel grouping logic is correct.
201-207: LGTM: Complete axis ordering options.The explicit enumeration of all six axis permutations is clear and matches the
axisOrdersarray. Consistent with the refactoring pattern applied across other layouts.
254-281: LGTM: LED pin selection implemented correctly.The
ledPinDIOfield and UI control properly map user selections to pin indices:
- "Default" (0) →
UINT8_MAX→ default pin ordering inPhysicalLayer::nextPin- "LED 01" through "LED N" (1–N) → pins 0 through N-1
The
Char<8>buffer is sufficient for the format string, andPhysicalLayer::nextPinvalidates pin indices, ensuring safe handling even if out-of-range values are set via the JSON API.
295-322: LGTM: Consistent LED pin selection.The implementation mirrors
SingleLineLayoutcorrectly, maintaining consistency across the codebase. The same safe mapping and validation logic applies.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (2)
33-44:viewselect control population looks consistent with addControl/addControlValue pattern
addControl(view, "view", "select");followed byaddControlValue("Physical layer");and then oneaddControlValueperlayerP.layersentry matches the usual “define select then enumerate options” pattern and decouples options from any persisted state. No functional issues spotted here.
47-53: onUpdate signature and DDP control name alignmentChanging
onUpdateto takeconst JsonObject& controland checkingcontrol["name"] == "DDP"aligns this driver with the other drivers’ onUpdate signatures and the label passed toaddControl(ddp, "DDP", ...). The method body is still a stub, but that’s acceptable if no side effects are needed when toggling DDP beyond the standard state update.
Docs ==== - Add Standards and Guidelines Back end ======== - Move Char out of Utilities to separate file - Artnet in: use channelsPerLight
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
120-130: Add defensive guard forchannelsPerLight == 0(avoid division by zero).While the code now uses
channelsPerLightinstead of hardcoded 3 (partially addressing the previous review), it still lacks a defensive guard against division by zero. IfchannelsPerLightis 0, line 123 will crash.Additionally, the comment on line 122 is now misleading—it still says "3 channels per LED: RGB" but the code uses the dynamic
channelsPerLight.Apply this diff to add the guard and update the comment:
- // Map DMX channels to LEDs (3 channels per LED: RGB) - int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights)); + // Map DMX channels to LEDs (channelsPerLight channels per LED) + uint8_t cpl = layerP.lights.header.channelsPerLight; + if (cpl == 0) return; // defensive guard + int numPixels = min( + (uint16_t)(dataLength / cpl), + (uint16_t)(layerP.lights.header.nrOfLights)); for (int i = 0; i < numPixels; i++) { - memcpy(&layerP.lights.channels[i * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); + memcpy( + &layerP.lights.channels[i * cpl], + &dmxData[i * cpl], + cpl); }
🧹 Nitpick comments (10)
docs/develop/standardsguidelines.md (5)
38-38: Capitalize "GitHub".Line 38 has "github reposistory"; should be "GitHub repository" (also fixes typo: reposistory → repository).
- ...consequences on our github reposistory. > + ...consequences on our GitHub repository. >
55-58: Apply hyphenation to compound adjectives.Several compound adjectives before nouns lack hyphens: "AI generated code" → "AI-generated code", "front end files" → "front-end files", "AI generated source code" → "AI-generated source code". Apply consistently throughout per standard English grammar.
- * best practice: add a comment like ``'// below section of my code was generated by an AI``, when larger parts of your source code were not written by you personally. + * best practice: add a comment like ``'// below section of my code was generated by an AI``, when larger parts of your AI-generated source code were not written by you personally.- We use Clang-format for c-files (see .clang-format in the repo for the definition) and Prettier for front end files. + We use Clang-format for c-files (see .clang-format in the repo for the definition) and Prettier for front-end files.- Right click Format Document on each file you edit before committing. + Right-click Format Document on each file you edit before committing.Also applies to: 65-67
29-29: Specify language for fenced code block.Line 29 has an unspecified code fence. Specify a language hint for syntax highlighting.
- ``` + ```markdown
62-62: Use fenced code blocks instead of indented.Line 62 uses indented code block format; convert to fenced blocks with language specifier for consistency (MD046).
- Code block style - Expected: fenced; Actual: indented + ```markdown + // This is a comment.
17-17: Fix markdown list indentation.Multiple list items use 4-space indentation instead of 2-space standard (MD007). Lines 17, 26, 78–87 and others. Normalize indentation throughout.
- * Want to make changes: fork the repo (see installation)and submit pull requests, see [creating-a-pull-request-from-a-fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork): + * Want to make changes: fork the repo (see installation) and submit pull requests, see [creating-a-pull-request-from-a-fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork): - * Press Branches... + * Press Branches...Also applies to: 26-26, 78-87
src/MoonLight/Modules/ModuleLiveScripts.h (2)
109-109: Document or remove commented-out "free" button.The
freebutton control is commented out here and its handler is commented at line 129. If this feature is temporarily disabled, add a TODO comment explaining when/why it should be re-enabled. If it's permanently unused, remove the commented code.
127-131: Consider error handling for LiveScript operations.The calls to
kill(),execute(), andkillAndDelete()lack error handling. If these methods can fail, consider checking return values or wrapping calls in try-catch to provide user feedback on failures.src/MoonBase/Char.h (2)
14-20: Include<functional>to make this header self‑contained.
std::functionis used insplit()but<functional>isn’t included here. Relying on transitive includes is brittle; please add the header explicitly.You can verify with a clean build where no other headers include
<functional>indirectly. A minimal fix:-#include <Arduino.h> -#include "ArduinoJson.h" +#include <Arduino.h> +#include <functional> +#include "ArduinoJson.h"
116-119: Avoidstd::string::npossentinel to keep this type STL‑free.
indexOf()returnsstd::string::nposwhen the token isn’t found, which requires<string>and pulls in the STL on an embedded target. SinceCharis a lightweight helper, it’s better to use a rawsize_tsentinel instead.A minimal change:
- size_t indexOf(const char* token) const { - const char* pos = strnstr(s, token, sizeof(s)); - return pos ? (pos - s) : std::string::npos; // or SIZE_MAX - } + size_t indexOf(const char* token) const { + const char* pos = strnstr(s, token, sizeof(s)); + return pos ? static_cast<size_t>(pos - s) + : static_cast<size_t>(-1); // npos sentinel without std::string + }If you prefer, you can also define a
static constexpr size_t npos = static_cast<size_t>(-1);insideCharand return that instead.src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (1)
86-111: Status string construction is safe; consider capacity if many pins are used.The
statusStringassembly ("#" + nrOfPins + ": " + " pin#leds" per pin) is bounded byChar<32>and usesstrlcat, so it’s safe but may truncate once you have ~4+ pins. If you expect setups with many LED pins and want the full mapping visible in the UI, bumping this toChar<64>` (and adjusting the control’s max length) would make it more future‑proof.A minimal change if you decide to expand it:
- Char<32> status = "ok"; + Char<64> status = "ok"; ... - addControl(status, "status", "text", 0, 32, true); // read only + addControl(status, "status", "text", 0, 64, true); // read only ... - Char<32> statusString = "#"; + Char<64> statusString = "#";Re‑run a layout with many pins to verify the full status string appears as expected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/develop/development.md(1 hunks)docs/develop/standardsguidelines.md(1 hunks)interface/src/routes/moonbase/monitor/Monitor.svelte(0 hunks)mkdocs.yml(1 hunks)src/MoonBase/Char.h(1 hunks)src/MoonBase/NodeManager.h(2 hunks)src/MoonBase/Utilities.h(1 hunks)src/MoonLight/Modules/ModuleLightsControl.h(4 hunks)src/MoonLight/Modules/ModuleLiveScripts.h(4 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(5 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(1 hunks)src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h(4 hunks)
💤 Files with no reviewable changes (1)
- interface/src/routes/moonbase/monitor/Monitor.svelte
✅ Files skipped from review due to trivial changes (1)
- src/MoonLight/Nodes/Drivers/D_ArtnetOut.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
docs/develop/development.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Use ArduinoJson for JSON serialization and deserialization in C++ backend code
Applied to files:
src/MoonBase/Utilities.h
🧬 Code graph analysis (5)
src/MoonBase/NodeManager.h (3)
src/MoonBase/Nodes.h (9)
void(65-68)void(71-71)void(191-191)void(192-192)void(193-193)void(196-196)void(205-205)Node(46-153)Node(209-209)src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (3)
src/MoonBase/Nodes.cpp (2)
onUpdate(400-540)onUpdate(400-400)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h (1)
onUpdate(69-73)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
src/MoonBase/Char.h (1)
src/MoonBase/NodeManager.h (2)
begin(36-74)void(78-78)
src/MoonLight/Modules/ModuleLiveScripts.h (2)
src/MoonBase/NodeManager.h (1)
setupDefinition(83-112)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
🪛 Clang (14.0.6)
src/MoonBase/Char.h
[error] 14-14: 'Arduino.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
docs/develop/standardsguidelines.md
[style] ~12-~12: Consider changing the order of words to improve your wording.
Context: ...ould contain compilable code and tested to not crash the system at minimal and support...
(TO_NOT_VB)
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...btle and unexpected consequences on our github reposistory. > For example, we regularl...
(GITHUB)
[style] ~52-~52: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... descriptions into English. AI bots are very good at understanding language. However, alw...
(EN_WEAK_ADJECTIVE)
[grammar] ~55-~55: Use a hyphen to join words.
Context: ... sure you understand exactly what the AI generated code does * best practice: a...
(QB_NEW_EN_HYPHEN)
[grammar] ~58-~58: Use a hyphen to join words.
Context: ...nts for correctness * always review AI generated source code * If the AI has ...
(QB_NEW_EN_HYPHEN)
[grammar] ~65-~65: Use a hyphen to join words.
Context: ...r the definition) and Prettier for front end files. Right click Format Document ...
(QB_NEW_EN_HYPHEN)
[grammar] ~67-~67: Use a hyphen to join words.
Context: ...and Prettier for front end files. Right click Format Document on each file you e...
(QB_NEW_EN_HYPHEN)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... what it does, including its arguments. Comments are also formatted using Clang-format a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/develop/standardsguidelines.md
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
62-62: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
78-78: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
docs/develop/development.md (1)
10-11: Navigation update looks good.The link to Standards and Guidelines improves user experience by directing readers to a centralized resource for development governance.
mkdocs.yml (1)
39-39: Navigation entry is correctly placed.The new standards-guidelines entry is properly positioned in the Develop section and follows the existing naming convention.
src/MoonBase/NodeManager.h (3)
83-112: setupDefinition UI schema looks consistent with control helpersThe new
setupDefinitionbuilds thenodes→name/on/controlsstructure usingaddControl/addControlValueand defaults that match the described model (selectFile for nodename, checkboxon, nestedcontrolsrows withname/type/value). This is coherent with the helper patterns inModuleandNodes.h.
241-251: Passing full control object into Node control update is appropriateUsing
JsonObject control = nodeState["controls"][updatedItem.index[1]]; nodeClass->updateControl(control); nodeClass->onUpdate(updatedItem.oldValue, control);aligns with the
Node::updateControl(const JsonObject&)/onUpdate(oldValue, control)pattern and ensures node logic has access to the entire control (name, type, value, metadata) rather than just the new value.
78-78: No action needed. All existingaddNodesoverrides in the codebase already use the correct signaturevoid addNodes(const JsonObject& control). BothModuleDriversandModuleEffectshave matching overrides with theoverridekeyword. No implementations using the oldJsonArraysignature were found.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (3)
36-44: LGTM: Clean refactor to helper methods.The migration from manual JSON construction to
addControl/addControlValuehelpers improves maintainability and aligns with the codebase pattern described in the PR objectives.
47-47: LGTM: Const-correctness improvement.Passing
controlby const reference avoids unnecessary copying and matches the pattern used elsewhere in the codebase (as shown in relevant snippets from D_VirtualDriver.h and D__Sandbox.h).
49-49: LGTM: Fixes control name mismatch.The comparison now correctly matches the control name "DDP" defined on line 34. The previous lowercase "ddp" would never have matched, so this fixes a bug where the onUpdate handler would not trigger for this control.
src/MoonLight/Modules/ModuleLightsControl.h (7)
28-29: LGTM! Clean pin field declarations.Both pins use
UINT8_MAXas the sentinel value for "no pin assigned," which is consistent and appropriate for optional GPIO configuration.
74-75: LGTM! Proper pin reset before scanning.Resetting both pins to
UINT8_MAXbefore scanning the configuration ensures clean state and prevents stale assignments if the configuration changes.
81-84: LGTM! Button pin properly configured.The
pinModeconfiguration is now present (addressed from past review), andINPUT_PULLUPis the correct mode for a button input with internal pull-up resistor.
92-138: LGTM! Clean refactor to addControl helpers.The migration from manual JSON construction to
addControlandaddControlValuehelpers significantly improves readability and maintainability while preserving the original control definitions.
288-288: LGTM! Consistent server update routing.The change to
_moduleName + "server"as the update destination is consistent with the same pattern used for button toggles (line 302), establishing clear routing for automated/server-side updates.
293-306: LGTM! Debouncing correctly implemented.The button handling now includes:
- 50ms debounce delay to prevent contact bounce
- Edge detection (triggers only on button press, HIGH→LOW transition with INPUT_PULLUP)
- State tracking to avoid repeated triggers
This addresses all concerns from the past review and implements the debouncing correctly.
241-244: Initialize lastDebounceTime to prevent undefined behavior.The
lastDebounceTimefield is declared without an initializer, giving it an indeterminate value. While the debounce logic may work in practice due to unsigned arithmetic behavior, this is a latent bug that could cause unexpected behavior on the first button press.Apply this diff to initialize the field:
unsigned long lastPresetTime = 0; // see pinToggleOnOff - unsigned long lastDebounceTime; + unsigned long lastDebounceTime = 0; static constexpr unsigned long debounceDelay = 50; // 50ms debounce int lastState = HIGH;Likely an incorrect or invalid review comment.
src/MoonLight/Modules/ModuleLiveScripts.h (1)
61-61: LGTM: Formatting consistency.The spacing adjustment in
as<const char*>()improves const-correctness formatting without changing functionality.Also applies to: 79-79
src/MoonLight/Nodes/Drivers/D_PhysicalDriver.h (2)
28-45: Version/status controls wiring looks consistent with the new Char API.Using
Char<32>forversionandstatus, exposing them as read‑only text controls, and callingupdateControl("version", HP_ALL_VERSION)on setup aligns well with the module control helpers and should keep existing UIs in sync.
93-107: Pins remapping and driver updates look correct and bounded bynrOfPins.Using
assignedPin < layerP.nrOfLedPinsbefore indexinglayerP.ledPins[assignedPin]and falling back tolayerP.ledPins[i]keepspins[]within the compacted valid range, and passingpins+nrOfPinsinto bothinitled()andupdateDriver()ensures the physical mapping and the status string stay in sync.Please sanity‑check a few boards with non‑trivial
ledPinsAssignedmappings (and reconfigure presets) to confirm that the reported status string exactly matches the actual physical pin order used by the driver.Also applies to: 119-133
src/MoonBase/Utilities.h (1)
17-18: Char utilities extraction intoChar.his wired correctly here.Including
"Char.h"from Utilities.h maintains backward compatibility for code that only includes Utilities.h while centralizing theCharimplementation in its own header, which is a good clean‑up.
| Char<N> substring(uint16_t begin, uint16_t end = sizeof(s) - 1) { | ||
| Char<N> sub; | ||
| if (begin >= sizeof(s) || end >= sizeof(s)) | ||
| sub = ""; | ||
| else { | ||
| strlcpy(sub.s, s + begin, end - begin + 1); | ||
| } | ||
| return sub; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Char.h" -type fRepository: MoonModules/MoonLight
Length of output: 86
🏁 Script executed:
cat -n ./src/MoonBase/Char.hRepository: MoonModules/MoonLight
Length of output: 7980
🏁 Script executed:
find . -name "*.cpp" -o -name "*.h" -o -name "*.ino" | xargs grep -l "substring" | head -20Repository: MoonModules/MoonLight
Length of output: 474
🏁 Script executed:
find . -name "*test*" -o -name "*Test*" | grep -E "\.(cpp|h|ino)$" | head -20Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
rg "substring\(" --type cpp --type h -B 2 -A 2 | head -60Repository: MoonModules/MoonLight
Length of output: 3513
🏁 Script executed:
cat -n ./src/MoonBase/Char.h | sed -n '102,110p'Repository: MoonModules/MoonLight
Length of output: 363
Fix off‑by‑one truncation and missing bounds check in substring().
The substring() method has two bugs: (1) substring(0, 0) returns an empty string instead of a single character because strlcpy is called with size 0 - 0 + 1 = 1, which only leaves room for a NUL terminator. (2) There is no guard for end < begin, which causes uint16_t underflow and undefined behavior. These are behavioral bugs, though memory-safe.
The proposed fix correctly adds the missing bounds check and properly calculates the buffer size for strlcpy. Consider adding unit tests for substring(0, 0), substring(1, 3), and out-of-range cases.
| addControl(rows, "isRunning", "checkbox", false, true, true); | ||
| addControl(rows, "isHalted", "checkbox", false, true, true); | ||
| addControl(rows, "exeExist", "checkbox", false, true, true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where addControl is defined
fd "\.h$|\.cpp$" | head -20
# Search for addControl function definitions
echo "=== Looking for addControl definitions ==="
rg -n "void.*addControl|addControl\s*\(" --type=cpp --type=h -A 2 | head -100Repository: MoonModules/MoonLight
Length of output: 8450
🏁 Script executed:
# Find the Module files to understand addControl
find . -name "Module*.h" -o -name "Module*.cpp" | head -20Repository: MoonModules/MoonLight
Length of output: 481
🏁 Script executed:
# Search for addControl signatures in header files
rg -n "addControl" --type=h -B 1 -A 3Repository: MoonModules/MoonLight
Length of output: 50377
🏁 Script executed:
# Look at addControl implementation in Module.cpp
cat -n src/MoonBase/Module.cpp | grep -A 20 "^.*addControl.*{" | head -40Repository: MoonModules/MoonLight
Length of output: 631
🏁 Script executed:
# Check the pattern more specifically - look at checkbox usage with parameters
rg -n 'addControl.*checkbox.*true.*true' --type=cpp --type=hRepository: MoonModules/MoonLight
Length of output: 712
Use standard 3-parameter addControl for checkbox controls.
Lines 100-102 unnecessarily pass false, true, true as min/max/ro parameters for checkbox controls. This generates "max": 1 in the JSON output and is inconsistent with how all other checkboxes are defined in the codebase. Use the standard 3-parameter form: addControl(rows, "isRunning", "checkbox") and set the read-only flag separately if needed, or verify that these checkboxes actually require explicit min/max constraints.
🤖 Prompt for AI Agents
In src/MoonLight/Modules/ModuleLiveScripts.h around lines 100 to 102, the
checkbox controls call addControl(rows, "isRunning", "checkbox", false, true,
true) (and similar) which incorrectly supplies min/max/ro parameters causing a
"max": 1 JSON entry; change each to use the standard 3-parameter form
addControl(rows, "isRunning", "checkbox") (and likewise for isHalted and
exeExist), and if any need read-only behavior set the read-only flag via the
established separate API or property after creation rather than passing extra
min/max args.
| addControl(rows, "stop", "button"); | ||
| addControl(rows, "start", "button"); | ||
| // addControl(rows, "free", "button"); | ||
| addControl(rows, "delete", "button"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find getScriptsJson implementation
echo "=== Searching for getScriptsJson implementation ==="
rg -n "getScriptsJson" --type=cpp -B 2 -A 20
echo -e "\n=== Look for button/control field definitions in the file ==="
fd . -name "*.h" -o -name "*.cpp" | xargs rg -l "ModuleLiveScripts" | head -5
echo -e "\n=== Check related files in ModuleLiveScripts directory ==="
fd "ModuleLiveScripts" -type fRepository: MoonModules/MoonLight
Length of output: 5787
Button name mismatch with getScriptsJson output requires clarification.
The setupDefinition defines buttons named stop, start, and delete (lines 107-110), but getScriptsJson emits fields named kill, execute, delete, and free (src/MoonBase/Nodes.cpp lines 332-335). While the onUpdate handler maps user clicks (stop → kill, start → execute), typical UI frameworks expect consistent naming between button names and JSON field names for proper control binding. Verify that the UI framework correctly handles this name mismatch, or consider renaming buttons to match the JSON field names (kill, execute, delete, free).
| int startPixel = offset / layerP.lights.header.channelsPerLight; | ||
| int numPixels = min((uint16_t)(dataLen / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights - startPixel)); | ||
|
|
||
| // Update LEDs | ||
| for (int i = 0; i < numPixels; i++) { | ||
| int ledIndex = startPixel + i; | ||
| if (ledIndex < layer->layerP->lights.header.nrOfLights) { | ||
| memcpy(&layer->layerP->lights.channels[i * layer->layerP->lights.header.channelsPerLight], &pixelData[i * layer->layerP->lights.header.channelsPerLight], layer->layerP->lights.header.channelsPerLight); | ||
| if (ledIndex < layerP.lights.header.nrOfLights) { | ||
| memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &pixelData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); | ||
| } |
There was a problem hiding this comment.
Add defensive guard for channelsPerLight == 0 in DDP handler.
The DDP handler has the same division-by-zero vulnerability as the ArtNet handler. Lines 159-160 divide by channelsPerLight without checking if it's zero.
Apply this diff to add the guard:
uint8_t* pixelData = packetBuffer + sizeof(DDPHeader);
- // Calculate starting pixel from byte offset (3 bytes per pixel)
- int startPixel = offset / layerP.lights.header.channelsPerLight;
- int numPixels = min((uint16_t)(dataLen / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights - startPixel));
+ // Calculate starting pixel from byte offset (channelsPerLight bytes per pixel)
+ uint8_t cpl = layerP.lights.header.channelsPerLight;
+ if (cpl == 0) return; // defensive guard
+ int startPixel = offset / cpl;
+ int numPixels = min((uint16_t)(dataLen / cpl), (uint16_t)(layerP.lights.header.nrOfLights - startPixel));
// Update LEDs
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex < layerP.lights.header.nrOfLights) {
- memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &pixelData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight);
+ memcpy(&layerP.lights.channels[ledIndex * cpl], &pixelData[i * cpl], cpl);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_ArtnetIn.h around lines 159 to 167, add a
defensive guard to handle layerP.lights.header.channelsPerLight == 0 before any
division or use; detect zero channelsPerLight and bail out (return or skip
processing) with an early exit so you don't perform offset / channelsPerLight or
dataLen / channelsPerLight, and avoid further memcpy operations when
channelsPerLight is invalid; keep behavior consistent with the ArtNet handler’s
guard and ensure no further computations run when the value is zero.
Front end ======== - Multirow: add crud(s) to display add, delete and sort/drag Back end ======= - Devices, IO, Tasks, Live scripts, Info: add crud - Nodes: addControl: always update definition - Virtual driver: remove (will be part of Physical driver -> rename...) - Utilities: EXT_LOGx: conditional (saves flash) - Physical layer: layoutPre: set nrOfAssignedPins = 0 - Channels: check if channels defined - Infrared: no logging if symbol is 1
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Modules/ModuleIO.h (1)
19-78: LED color select values are reversed relative to the enum.The enum defines RGB pins as
pin_LED_B(23),pin_LED_G(24),pin_LED_R(25), but the select control adds them as"LED R"(23),"LED G"(24),"LED B"(25). Since select controls use sequential array indices that map directly to enum values, a user selecting "LED R" from the UI will store index 23, which the code interprets aspin_LED_B, causing red/blue to swap.Reorder the select values to match the enum:
- addControlValue(control, "LED R"); - addControlValue(control, "LED G"); - addControlValue(control, "LED B"); + addControlValue(control, "LED B"); + addControlValue(control, "LED G"); + addControlValue(control, "LED R");
♻️ Duplicate comments (1)
src/MoonLight/Modules/ModuleLiveScripts.h (1)
108-111: Button names vs LiveScriptNode::getScriptsJson: confirm consistencyThe builder defines
stop,start, anddeletebuttons, andonUpdatemaps them tokill(),execute(), andkillAndDelete()respectively, which is internally consistent here. However, a prior review already flagged a possible mismatch withLiveScriptNode::getScriptsJsonfield names (kill,free,delete,execute). Please confirm thatgetScriptsJsonnow emits fields compatible with these button names, or that the UI framework does not depend on matching state keys for buttons.
🧹 Nitpick comments (10)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
177-196: LGTM! Consider adding range validation if ledPinDIO is constrained.The function signature change and conditional assignment logic are correct. The use of
UINT8_MAXas a sentinel for default behavior is appropriate and maintains backward compatibility through the default parameter. The array bounds are properly checked (i < MAXLEDPINS).If
ledPinDIOvalues are expected to be within a specific range (e.g., valid GPIO pin numbers), consider adding validation or documenting the expected range in the function/parameter documentation. However, since the value is only stored here and not used as an array index, the current implementation is safe.interface/src/lib/components/moonbase/MultiRow.svelte (1)
187-187: Consider simplifying the double-negative logic.The expression
!(findItemInDefinition.crud == null || findItemInDefinition.crud.includes('s'))is correct but uses a double negative. Applying De Morgan's law improves readability:-dragDisabled={!(findItemInDefinition.crud == null || findItemInDefinition.crud.includes('s'))} +dragDisabled={findItemInDefinition?.crud != null && !findItemInDefinition?.crud?.includes('s')}This also incorporates the null safety fix from the previous comment.
src/MoonLight/Modules/ModuleLiveScripts.h (2)
91-113: setupDefinition refactor to controls/rows pattern looks correctThe new
setupDefinition(const JsonArray& controls)implementation follows the shared builder pattern:
- Defines a single
"scripts"control of type"rows"with"crud": "ru", then populatesrows = control["n"].to<JsonArray>(), which is consistent with other modules’ array controls.- Per-row fields (
name,isRunning,isHalted,exeExist,handle,binary_size,data_size,error,stop,start,delete) align with whatloop1s()pushes into_state.data["scripts"]viaLiveScriptNode::getScriptsJson(...)and with theonUpdatehandler below (at least forstop,start,delete).The checkbox
addControlcalls pass explicitmin=0,max=1, andro=true. That’s functionally fine (booleans as 0/1) but still slightly inconsistent with the simpler three‑parameter checkbox uses elsewhere; consider simplifying to the common form if you want consistency, but it’s not required.
100-103: Checkbox min/max/ro usage is functionally fine; style-only concernUsing
addControl(rows, "...", "checkbox", false, true, true)correctly results in[0,1]range with read‑only flag set through themin/max/roparameters ofModule::addControl. If the rest of the codebase mostly uses the 3‑arg form for checkboxes, you could switch to that for style consistency, but there’s no correctness issue here.docs/develop/nodes.md (1)
57-57: Typo: "choosen" should be "chosen".-* Uses ESPLiveScripts, see compileAndRun. compileAndRun is started when in Nodes a file.sc is choosen +* Uses ESPLiveScripts, see compileAndRun. compileAndRun is started when in Nodes a file.sc is chosensrc/MoonBase/Modules/ModuleTasks.h (1)
68-73: Consider renaming local variable to avoid conceptual shadowing.The local
JsonObject controlsinloop1s()could be confused with thecontrolsparameter insetupDefinition(). Consider renaming tostateortaskDatafor clarity:JsonDocument doc; doc["tasks"].to<JsonArray>(); - JsonObject controls = doc.as<JsonObject>(); + JsonObject taskData = doc.as<JsonObject>();And update subsequent references accordingly.
src/MoonBase/Modules/ModuleIO.h (2)
269-279: SE16V1 preset:pin_Button_OnOffmapping is reasonable, but verify hardware expectation.For
board_SE16V1, the preset now assigns:
- Buttons: GPIO 0 →
pin_Button_01, GPIO 45 →pin_Button_02, GPIO 46 →pin_Button_OnOff.This appears coherent, but it’s hardware-specific. Please confirm that GPIO 46 on SE16V1 is indeed the intended On/Off button input and that no other module expects it to retain a previous usage (e.g.,
pin_Button_03).
270-377: Looping over fixed arrays withsizeofworks but could be made more robust.Patterns like:
uint8_t ledPins[16] = { ... }; for (int i = 0; i < sizeof(ledPins); i++) pins[ledPins[i]]["usage"] = pin_LED_01 + i;rely on
sizeof(uint8_t) == 1. That’s safe today, but slightly brittle. Consider using an element-count idiom for future-proofing and readability:- for (int i = 0; i < sizeof(ledPins); i++) + for (size_t i = 0; i < sizeof(ledPins) / sizeof(ledPins[0]); i++)(and similarly for
ethernetPinsand other arrays).src/MoonBase/Nodes.h (2)
18-19: IncludingModuleIO.hdirectly inNodes.his functional but creates a tighter coupling.Pulling in
ModuleIO.hto getIO_PinUsageandModulealso brings in all of ModuleIO’s dependencies into this core header. If layering becomes a concern, consider:
- Forward-declaring
class Module;here, and- Moving
IO_PinUsageinto a lighter-weight shared header (e.g.,IO_Pins.h) that bothModuleIOand nodes/drivers can include.This is non-blocking but worth considering to keep
Nodes.hlean.
52-57: NewmoduleNodespointer is fine; ensure it’s initialized by the owner.Adding
Module* moduleNodes = nullptr;for UI-update signaling is reasonable. Just ensure the code that constructs/configures nodes (likely NodeManager or the owning Module) assigns this pointer appropriately before any logic that relies on it, to avoid null dereferences when requesting UI updates.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/develop/nodes.md(1 hunks)docs/moonbase/devices.md(1 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(4 hunks)src/MoonBase/Modules/ModuleDevices.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/Modules/ModuleTasks.h(3 hunks)src/MoonBase/Nodes.h(6 hunks)src/MoonBase/Utilities.h(2 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(4 hunks)src/MoonLight/Modules/ModuleChannels.h(2 hunks)src/MoonLight/Modules/ModuleDrivers.h(3 hunks)src/MoonLight/Modules/ModuleLiveScripts.h(4 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(1 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(3 hunks)src/MoonLight/Nodes/Drivers/D_VirtualDriver.h(0 hunks)src/MoonLight/Nodes/Drivers/D__Sandbox.h(2 hunks)
💤 Files with no reviewable changes (1)
- src/MoonLight/Nodes/Drivers/D_VirtualDriver.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonBase/Modules/ModuleDevices.h
🧰 Additional context used
🧠 Learnings (1)
📚 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} : Use ArduinoJson for JSON serialization and deserialization in C++ backend code
Applied to files:
src/MoonBase/Utilities.h
🧬 Code graph analysis (7)
src/MoonBase/Modules/ModuleTasks.h (3)
src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-48)src/MoonLight/Modules/ModuleLiveScripts.h (1)
setupDefinition(91-113)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonLight/Modules/ModuleChannels.h (2)
src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-48)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
src/MoonBase/Nodes.h (1)
nextPin(210-210)
src/MoonLight/Nodes/Drivers/D_Infrared.h (5)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)src/MoonBase/NodeManager.h (1)
onUpdate(115-266)src/MoonBase/Nodes.cpp (2)
onUpdate(400-540)onUpdate(400-400)src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
onUpdate(920-924)onUpdate(1116-1120)
src/MoonLight/Modules/ModuleMoonLightInfo.h (2)
src/MoonBase/Modules/ModuleDevices.h (1)
setupDefinition(33-48)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)
src/MoonBase/Nodes.h (5)
src/MoonLight/Modules/ModuleLiveScripts.h (1)
onUpdate(116-141)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)src/MoonBase/Nodes.cpp (4)
onUpdate(400-540)onUpdate(400-400)updateControl(17-66)updateControl(17-17)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onUpdate(120-124)src/MoonLight/Nodes/Effects/E__Sandbox.h (1)
onUpdate(35-41)
src/MoonLight/Modules/ModuleLiveScripts.h (2)
src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonLight/Modules/ModuleEffects.h (1)
setupDefinition(60-147)
🪛 LanguageTool
docs/develop/nodes.md
[style] ~17-~17: In American English, abbreviations like “etc.” require a period.
Context: ...pes (sliders/range, checkboxes, numbers etc). They are added with the addControl() ...
(ETC_PERIOD)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...e supports. They control when a physical to virtual mapping is recalculated *...
(QB_NEW_EN_HYPHEN)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...upports. They control when a physical to virtual mapping is recalculated * **...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...support different lights e.g a mix of 15 channel lights and 32 channel lights etc...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...ts e.g a mix of 15 channel lights and 32 channel lights etc. You could set channe...
(QB_NEW_EN_HYPHEN)
[style] ~31-~31: Consider using polite language here.
Context: ...wheel could be used to really go crazy. Let us know when you have one of these setups 🚨 * Movin...
(INSERT_PLEASE)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...he LEDs / channels array and the virtual to physical layer mappings. * specif...
(QB_NEW_EN_HYPHEN)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...LEDs / channels array and the virtual to physical layer mappings. * specify w...
(QB_NEW_EN_HYPHEN)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...n is started when in Nodes a file.sc is choosen * To do: kill running scripts, e.g. when c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~62-~62: In American English, abbreviations like “etc.” require a period.
Context: ...tPan, setTilt, setZoom etc. Also getRGB etc functions exists. ## Drivers ### Init...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.18.1)
docs/moonbase/devices.md
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/develop/nodes.md
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (32)
docs/moonbase/devices.md (1)
5-5: Documentation updates are clear and accurate.The reworded description better explains the Devices module's purpose, and the added WiFi station link helps users locate where to configure the device name. The changes align well with the PR's UI control refactoring.
Also applies to: 7-7
src/MoonLight/Modules/ModuleMoonLightInfo.h (3)
22-25: LGTM! Signature and variable declarations align with the new API pattern.The parameter rename to
controlsmatches the base class signature, and the local variables follow the established pattern for the addControl-based API.
27-31: LGTM! Top-level controls properly defined.The addControl calls correctly define read-only info controls that are populated by the readHook (lines 54-60). The pattern is clean and consistent.
33-45: LGTM! Nested rows control follows the established pattern.The layers control is properly structured as a read-only rows type, and the nested control definitions match the data population in the readHook (lines 83-92). The use of
control["n"].to<JsonArray>()to obtain the rows array is consistent with the pattern shown in ModuleDevices.h.src/MoonLight/Layers/PhysicalLayer.cpp (2)
136-139: LGTM!The initialization of
ledPinsAssignedandnrOfAssignedPinsis correct and consistent with the existing pattern forledsPerPin. The array is properly cleared and the counter is reset before the layout pass.
44-44: Allocation is within the stated heap limit and includes proper failure handling.The 24,576-byte allocation (8192 × 3) is under the 30KB max allocation limit mentioned in the comments. The code already includes allocation failure detection (lines 48–53): if the allocation fails, it logs an error, sets
maxChannelsto 0, and continues. This graceful degradation means the concern about OOM is mitigated by existing error handling. No additional verification is required.interface/src/lib/components/moonbase/MultiRow.svelte (1)
101-101: LGTM – Type annotations improve clarity.The explicit type annotations on the callback parameters are good for readability and type safety.
Also applies to: 105-105, 107-107
src/MoonLight/Modules/ModuleLiveScripts.h (2)
40-84: FileManager update handler logging change is safeThe added debug logs using
nodeState["name"].as<const char*>()look correct and match common patterns elsewhere; no functional impact beyond improved traceability.
128-133: onUpdate action mapping is coherent with defined buttonsThe updated
onUpdatelogic cleanly maps:
"stop"→liveScriptNode->kill()"start"→liveScriptNode->execute()"delete"→liveScriptNode->killAndDelete()combined with the
updatedItem.oldValue != ""guard to avoid firing at boot. This matches the button names defined insetupDefinitionand looks correct.docs/develop/nodes.md (1)
17-24: Documentation aligns well with the addControl API changes.The terminology updates from
setControl()toaddControl()correctly reflect the code refactoring in this PR. The expanded explanations forhasOnLayout()and driver ordering are helpful for developers.src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
31-37: LGTM! Signature change to const reference is correct.The
onUpdatesignature change fromconst JsonObject controltoconst JsonObject& controlis consistent with the broader API refactor visible insrc/MoonBase/Nodes.cppand other driver files. Passing by const reference avoids unnecessary copies.src/MoonLight/Nodes/Drivers/D_Infrared.h (3)
86-89: LGTM! Clean migration to addControlValue pattern.The setup now uses the streamlined
addControlValue()pattern instead of intermediate JsonObject/JsonArray construction, which is cleaner and consistent with the broader API refactor.
123-123: LGTM! Signature change to const reference is correct.Consistent with
D__Sandbox.hand the base class signature insrc/MoonBase/Nodes.cpp.
408-408: Verify logging condition logic.The condition
rx_data.num_symbols != 1will log for valid NEC frames (34 symbols for standard frames, 2 for repeat codes), since the comment indicates only 34 and 2 are processed. Did you intend to log only for invalid symbol counts?If the intent is to log unexpected frames, consider:
- if (rx_data.num_symbols != 1) EXT_LOGD(IR_DRIVER_TAG, "Received symbols: #%d", rx_data.num_symbols); // will not be processed (only 34 and 2) + if (rx_data.num_symbols != 34 && rx_data.num_symbols != 2) EXT_LOGD(IR_DRIVER_TAG, "Received symbols: #%d (unexpected)", rx_data.num_symbols);src/MoonLight/Modules/ModuleDrivers.h (3)
54-57: LGTM! Simplified compacting logic.The condition now only checks for valid pins (
!= UINT8_MAX) without the previousledsPerPinchecks, which is cleaner and correctly focuses on pin validity during array compaction.
78-101: LGTM! Consistent migration to addControlValue pattern.The signature change and use of
addControlValue(control, ...)aligns with the broader API refactor across the codebase.
159-160: LGTM! Node wiring for UI updates.Assigning
moduleIOandmoduleNodesenables proper pin allocation tracking and UI update signaling, which aligns with the PR objectives for module-level UI updates.src/MoonBase/Utilities.h (2)
18-18: LGTM! Clean extraction of Char template to separate header.Moving
Char<N>and related utilities toChar.himproves modularity and reduces the size of this utility header.
26-77: LGTM! Compile-time logging level gates.Gating logging macros by
CORE_DEBUG_LEVELeliminates logging overhead entirely when disabled, rather than relying on runtime checks. This is a good optimization for embedded systems.src/MoonLight/Modules/ModuleChannels.h (2)
24-54: LGTM! Consistent migration to addControl/addControlValue pattern.The signature change and control population align with the broader API refactor seen in
ModuleDevices.hand the baseModule::setupDefinitioninsrc/MoonBase/Module.cpp.
77-77: Good defensive null check to prevent crash during init.The early return when
layerP.lights.channelsis null prevents potential null pointer dereference during initialization.src/MoonBase/Modules/ModuleTasks.h (1)
23-46: LGTM! Consistent migration to controls/rows pattern.The refactored
setupDefinitionfollows the same pattern asModuleDevices.handModuleLiveScripts.h, usingaddControlwith a "rows" type and populating nested fields viacontrol["n"].to<JsonArray>().src/MoonBase/Modules/ModuleIO.h (3)
110-215:setupDefinitionrefactor toaddControl/addControlValuelooks correct and aligns with the valid-flag pattern.The refactored
setupDefinition(const JsonArray& controls)cleanly rebuilds:
boardPresetas a"select"with ordered board names,moddedas a"checkbox",maxPoweras a"number",pinsas a"rows"control with nestedGPIO,usage,summary,Level, andDriveCap.Using
addControl+addControlValueis consistent with the Node/control pattern elsewhere, and the pins rows configuration (filter!Unused,crud = "ru") matches the expected semantics for editing pin usages and viewing derived summaries.No functional issues stand out here, assuming the underlying
Module::addControl/addControlValueimplementations mirror the Node versions described in Nodes.h.
148-207: New “Button OnOff” usage is wired consistently into the pins-usage select.The
"usage"select under thepinsrows now includes"Button OnOff"as the last value (Line 206), which matches the placement ofpin_Button_OnOffat the end ofIO_PinUsage. This keeps the index/value mapping consistent for the new usage type.Once the LED RGB ordering noted earlier is settled, this values list looks structurally correct.
377-386: Default-board preset comments are clear and non-invasive.The added comments around the default board’s extra LED pin mappings clarify ESP32-D0-16MB limitations without affecting behavior (pins remain commented out). Looks good as documentation-only change.
src/MoonBase/Nodes.h (7)
64-68: Constructorconstructor(VirtualLayer*, const JsonArray&)change is sensible.Accepting
const JsonArray& controlsand simply storing it intothis->controlsavoids unnecessary copies while keeping the interface clear. As long as the underlying JsonDocument outlives the node (which is how these arrays are usually managed), this is a safe and more efficient signature.
75-160:addControlrefactor: good reuse of existing controls and correct use ofvalidflag; minor behavior notes.Key points in the new
addControlimplementation:
- It first searches
controlsbynameand reuses an existing control if found, only creating a new one when missing (Lines 80–94). This preserves persistedvaluewhile updating metadata.- For a new control, it sets
name, initialvalue, anddefaultfromvariable, then updatestype, pointerp,valid = true, and optionalro/min/max/descfields (Lines 96–117).- It computes
sizebased ontypeandControlType, including the correctedCoord3Dhandling (Lines 119–147), aligning with howupdateControlinterprets pointers.- When
newControlis true, it callsonUpdatewith an emptyoldValueto allow nodes to react to the initial value (Lines 151–154).control["valid"] = trueensures that newly added controls participate correctly in the "valid flag" lifecycle described in the PR (NodeManager invalidates then cleans up only invalid controls), which is important for avoiding the status-control truncation issue you debugged.Two behavioral nuances to be aware of:
- For existing controls,
control["value"]is not overwritten withvariable, which is desirable to preserve user state across setups.defaultis always reset tovariable, so changing defaults in code will update the metadata even for existing controls.Overall this looks consistent with the intended control lifecycle and the
compareRecursive/NodeManager behavior you outlined.
163-170: New templatedaddControlValuewith empty-controls guard fixes the underflow risk.The new template:
if (controls.size() == 0) return; JsonObject control = controls[controls.size() - 1]; ... values.add(value);prevents the previous potential underflow when
addControlValuewas called without any prioraddControl. Making it templated also improves usability across value types.This resolves the earlier bounds-check concern.
172-183:updateControlby name correctly reuses the shared update path.The new overload:
template <typename T> void updateControl(const char* name, const T value) { for (JsonObject control : controls) { if (control["name"] == name) { control["value"] = value; updateControl(control); break; } } }keeps a single implementation of the pointer write logic in
updateControl(const JsonObject& control)and ensures the Json state is updated before propagating the change. This is straightforward and avoids code duplication.Just be sure that
namecomparisons here are consistent with how names are declared inaddControl(including exact case/spacing).
185-186:onUpdatesignature change toconst JsonObject&is compatible with new control handling.Changing the base hook to:
virtual void onUpdate(const Char<20>& oldValue, const JsonObject& control) {}matches the way controls are now passed around (read-only JsonObjects) and aligns with the implementation of
updateControl. Subclasses can still read all metadata fromcontrolwhile relying onNode::updateControlto perform the actual pointer writes.Ensure all overrides (drivers, effects, layouts, sandbox nodes) have been updated to this new signature, which it appears you’ve done based on the PR summary.
209-210:nextPinconvenience overload withledPinDIOparameter is a nice extension.Forwarding an optional
ledPinDIOdown tolayer->layerP->nextPin(ledPinDIO)gives callers fine-grained control while keeping the default behavior (UINT8_MAX) unchanged. As long aslayerandlayer->layerPare always set before use (which is the existing assumption), this is a safe change.
296-298: DriverNodeonUpdateoverride now matches the base signature.Updating
DriverNode::onUpdateto:void onUpdate(const Char<20>& oldValue, const JsonObject& control) override;keeps it consistent with the new base
Node::onUpdateand ensures polymorphic dispatch works correctly. This is necessary for driver nodes to participate in the unified control-update flow.
Docs ==== - Multiple updates
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/moonlight/drivers.md (1)
33-33: Consider clarifying Virtual LED Driver relationship.The table row mentions "Virtual LED Driver will be part of the Parallel LED driver," but lines 41-43 present Virtual LED Driver as a separate standalone section. This could confuse readers about whether Virtual LED Driver is:
- A sub-component of Parallel LED Driver, or
- A separate driver type
Consider adding a clarifying sentence such as:
Virtual LED Driver: Driving max 120! outputs. E.g. 48 panels of 256 LEDs each run at 50-100 FPS using shift registers. Currently integrated within the Parallel LED Driver architecture. Not implemented yet as a standalone option.src/MoonBase/Nodes.h (1)
176-185: Consider returning a success indicator.The template method silently does nothing if no control matches the given name. If callers might need to know whether the update succeeded, consider returning
boolto indicate whether a matching control was found.- void updateControl(const char* name, const T value) { + bool updateControl(const char* name, const T value) { for (JsonObject control : controls) { if (control["name"] == name) { control["value"] = value; updateControl(control); - break; + return true; } } + return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/develop/development.md(1 hunks)docs/develop/drivers.md(0 hunks)docs/develop/installation.md(1 hunks)docs/develop/nodes.md(1 hunks)docs/develop/standardsguidelines.md(1 hunks)docs/gettingstarted/installation.md(1 hunks)docs/moonbase/inputoutput.md(1 hunks)docs/moonlight/drivers.md(2 hunks)firmware/esp32-s3.ini(1 hunks)platformio.ini(3 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonBase/NodeManager.h(3 hunks)src/MoonBase/Nodes.cpp(4 hunks)src/MoonBase/Nodes.h(8 hunks)src/MoonLight/Layers/PhysicalLayer.h(2 hunks)src/MoonLight/Modules/ModuleDrivers.h(4 hunks)src/MoonLight/Nodes/Drivers/D_FastLED.h(2 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(4 hunks)src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h(7 hunks)src/MoonLight/Nodes/Drivers/parlio.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- docs/develop/drivers.md
✅ Files skipped from review due to trivial changes (2)
- firmware/esp32-s3.ini
- src/MoonLight/Nodes/Drivers/parlio.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- platformio.ini
- src/MoonBase/Nodes.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
docs/develop/development.md
🧬 Code graph analysis (6)
src/MoonBase/Modules/ModuleIO.h (4)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
setupDefinition(22-46)src/MoonLight/Modules/ModuleLightsControl.h (1)
setupDefinition(92-138)src/MoonBase/Module.cpp (4)
setupDefinition(352-359)setupDefinition(352-352)addControl(361-370)addControl(361-361)src/MoonLight/Modules/ModuleEffects.h (1)
setupDefinition(60-147)
src/MoonLight/Nodes/Drivers/D_Infrared.h (3)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)src/MoonBase/Nodes.cpp (2)
onUpdate(400-540)onUpdate(400-400)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
src/MoonLight/Nodes/Drivers/D_FastLED.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonBase/NodeManager.h (3)
src/MoonBase/Nodes.h (9)
void(67-70)void(73-73)void(201-201)void(202-202)void(203-203)void(206-206)void(215-215)Node(48-163)Node(219-219)src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)src/MoonBase/Char.h (2)
contains(115-115)contains(209-214)
src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (3)
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
dim(33-33)tags(34-34)src/MoonLight/Nodes/Drivers/D_Infrared.h (2)
dim(34-34)tags(35-35)src/MoonBase/Nodes.cpp (2)
updateControl(17-66)updateControl(17-17)
src/MoonLight/Modules/ModuleDrivers.h (2)
src/MoonLight/Nodes/Drivers/D_Hub75.h (1)
HUB75Driver(16-27)src/MoonBase/Char.h (1)
equalAZaz09(194-207)
🪛 LanguageTool
docs/moonbase/inputoutput.md
[grammar] ~38-~38: Ensure spelling is correct
Context: ...on On/Off * Planned soon * Battery * Relais brightness * DMX (in) * Planned l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/develop/nodes.md
[style] ~17-~17: In American English, abbreviations like “etc.” require a period.
Context: ...pes (sliders/range, checkboxes, numbers etc). They are added with the addControl() ...
(ETC_PERIOD)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...e supports. They control when a physical to virtual mapping is recalculated *...
(QB_NEW_EN_HYPHEN)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...upports. They control when a physical to virtual mapping is recalculated * **...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...support different lights e.g a mix of 15 channel lights and 32 channel lights etc...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...ts e.g a mix of 15 channel lights and 32 channel lights etc. You could set channe...
(QB_NEW_EN_HYPHEN)
[style] ~31-~31: Consider using polite language here.
Context: ...wheel could be used to really go crazy. Let us know when you have one of these setups 🚨 * Movin...
(INSERT_PLEASE)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...he LEDs / channels array and the virtual to physical layer mappings. * specif...
(QB_NEW_EN_HYPHEN)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...LEDs / channels array and the virtual to physical layer mappings. * specify w...
(QB_NEW_EN_HYPHEN)
[grammar] ~57-~57: Ensure spelling is correct
Context: ...n is started when in Nodes a file.sc is choosen * To do: kill running scripts, e.g. when c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~62-~62: In American English, abbreviations like “etc.” require a period.
Context: ...tPan, setTilt, setZoom etc. Also getRGB etc functions exists. ## Drivers ### Init...
(ETC_PERIOD)
docs/develop/standardsguidelines.md
[style] ~12-~12: Consider changing the order of words to improve your wording.
Context: ...ould contain compilable code and tested to not crash the system at minimal and support...
(TO_NOT_VB)
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...btle and unexpected consequences on our github reposistory. > For example, we regularl...
(GITHUB)
[style] ~52-~52: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... descriptions into English. AI bots are very good at understanding language. However, alw...
(EN_WEAK_ADJECTIVE)
[grammar] ~55-~55: Use a hyphen to join words.
Context: ... sure you understand exactly what the AI generated code does * best practice: a...
(QB_NEW_EN_HYPHEN)
[grammar] ~58-~58: Use a hyphen to join words.
Context: ...nts for correctness * always review AI generated source code * If the AI has ...
(QB_NEW_EN_HYPHEN)
[grammar] ~65-~65: Use a hyphen to join words.
Context: ...r the definition) and Prettier for front end files. Right click Format Document ...
(QB_NEW_EN_HYPHEN)
[grammar] ~67-~67: Use a hyphen to join words.
Context: ...and Prettier for front end files. Right click Format Document on each file you e...
(QB_NEW_EN_HYPHEN)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... what it does, including its arguments. Comments are also formatted using Clang-format a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/develop/development.md
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/moonbase/inputoutput.md
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
31-31: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/moonlight/drivers.md
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
33-33: Images should have alternate text (alt text)
(MD045, no-alt-text)
33-33: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
34-34: Images should have alternate text (alt text)
(MD045, no-alt-text)
34-34: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
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)
36-36: Images should have alternate text (alt text)
(MD045, no-alt-text)
36-36: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
37-37: Images should have alternate text (alt text)
(MD045, no-alt-text)
37-37: Images should have alternate text (alt text)
(MD045, no-alt-text)
37-37: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
38-38: Images should have alternate text (alt text)
(MD045, no-alt-text)
38-38: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
42-42: Images should have alternate text (alt text)
(MD045, no-alt-text)
42-42: Images should have alternate text (alt text)
(MD045, no-alt-text)
docs/develop/standardsguidelines.md
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (48)
src/MoonLight/Nodes/Drivers/D_FastLED.h (2)
50-56: UI label change insetup()is safe and consistentChanging the control label from
"Version"to"version"(while keeping the backingversionbuffer and other parameters the same) is purely cosmetic and aligns with the lower‑case control naming used elsewhere in the PR. No behavioral or API impact here.
89-92:onLayoutguard condition remains correct after cleanupThe
if (layerP.pass == 1 && !layerP.monitorPass)guard still clearly restricts FastLED layout work to the primary layout pass and skips monitor passes. Removing the trailing comment/whitespace does not change behavior; the control flow is still appropriate.docs/develop/development.md (2)
10-11: LGTM! Documentation consolidation is well-executed.The link to the centralized Standards and Guidelines document improves maintainability by reducing duplication across documentation files. The change aligns with the new
standardsguidelines.mdfile introduced in this PR.
17-17: Clear and actionable branching guidance.The instruction "Only branch from the main branch!" is prominently placed and reinforces the workflow described in the PR objectives and related documentation.
docs/moonbase/inputoutput.md (2)
6-16: Documentation update aligns with PR architecture changes.The ESP32-focused content accurately reflects the new board presets and pin assignment tracking introduced in this PR. The structure with Modded, Max Power, and Pins sections provides clear guidance for users.
29-44: Pin usage roadmap is well-organized.The categorization into Supported/Planned soon/Planned later provides clear expectations for feature availability. The inclusion of "Button On/Off" aligns with the PR description mentioning "add On off button pin" in ModuleIO.
src/MoonLight/Layers/PhysicalLayer.h (3)
25-25: Terminology update consistent with PR-wide driver renaming.The comment change from "Physical driver" to "Parallel LED Driver" aligns with the broader renaming effort visible across documentation and code files in this PR.
129-129: Well-designed API extension for explicit pin assignment.The default parameter allows
nextPin()to maintain backward compatibility (auto-assignment when called without arguments) while enabling explicit pin specification when needed. This is a clean, non-breaking API evolution.
134-134: Pin assignment tracking array added appropriately.The
ledPinsAssignedarray complements the existingledPinsandledsPerPinarrays to track which GPIO pins are assigned to which slots. The past review comment confirms initialization has been properly addressed.docs/gettingstarted/installation.md (2)
142-142: Driver recommendation updated with new terminology.The guidance correctly replaces "Physical driver" with "Parallel LED Driver" and provides clear use cases (>4 strips/panels or non-standard LEDs). The recommendation structure remains clear and actionable.
146-146: ESP32-P4-Nano guidance includes proper attribution.The P4-Nano-specific instructions now reference the "Parallel LED Driver" and correctly attribute the "Parallel IO driver" to @troyhacks. The wiring diagram reference enhances clarity for users.
docs/develop/installation.md (1)
41-41: Branch workflow documentation accurately reflects current practice.The updated description clarifies that development occurs in branches from main rather than a dedicated dev branch. This aligns with the guidance in
docs/develop/development.md(line 17) and the newstandardsguidelines.mdfile.docs/moonlight/drivers.md (2)
11-11: Driver list terminology updated consistently.The change from "Physical LED Drivers" to "Parallel LED Drivers" maintains consistency with the PR-wide renaming effort.
46-46: Max Power relocation documented correctly.The note about Max Power moving to board presets in Module IO aligns with the PR objectives and the changes visible in
docs/moonbase/inputoutput.md.docs/develop/standardsguidelines.md (4)
1-12: Comprehensive development principles clearly articulated.The principles section effectively communicates key architectural decisions:
- One firmware per board (3MB flash constraint)
- Documentation integrated with code changes
- Minimal upstream modifications with //🌙 markers
- Main-based branching workflow
These align with the PR objectives and provide valuable guidance for contributors.
36-39: Excellent emphasis on force-push dangers.The CAUTION block appropriately warns about force-push consequences, specifically mentioning lost review comments. This is a common pain point in collaborative development and the strong warning is justified.
44-60: Pragmatic and balanced AI guidance.The AI-generated code section strikes the right balance:
- Acknowledges AI assistance is acceptable
- Emphasizes contributor responsibility and understanding
- Requires transparency (commenting AI-generated sections)
- Warns about AI limitations ("Often-Wrong")
This guidance is well-suited for modern development practices while maintaining code quality standards.
65-72: Code formatting requirements clearly specified.The documentation correctly references the project's formatting tools (Clang-format for C/C++, Prettier for frontend) and emphasizes the "Right click Format Document" workflow in VS Code. The comment documentation requirements are appropriate for maintaining code quality.
src/MoonLight/Nodes/Drivers/D_Infrared.h (4)
33-35: Driver naming and status appropriately updated.The name change to "Infrared Driver" (from "IR Driver") improves clarity, and removing the "🚧" tag signals the driver is no longer considered work-in-progress. Both changes align with the driver's maturity.
86-89: Excellent refactoring to addControl/addControlValue pattern.The new control-building approach is cleaner and more maintainable than explicit JsonObject construction. This aligns with the pattern introduced in
Module.cpp(see relevant snippet showingaddControlimplementation) and used throughout this PR.Before: Manual JsonObject/JsonArray construction
After: Declarative control building with helpers
123-123: Const-correctness improvement in onUpdate signature.Changing the parameter from
JsonObject controltoconst JsonObject& controlprevents unnecessary copies and adds const-correctness. This matches the pattern seen in other drivers (e.g.,D__Sandbox.hin relevant snippets) and the base class signature update.
408-408: Smart logging guard reduces noise for standard frames.The condition
if (rx_data.num_symbols != 1)ensures that only non-standard frames are logged, as standard NEC frames (34 or 2 symbols) are processed normally. This reduces log clutter while preserving diagnostic information for unexpected frame types.src/MoonBase/NodeManager.h (3)
78-78: LGTM - API signature change aligns with broader refactoring.The signature change from
const JsonArray& valuestoconst JsonObject& controlis consistent with the newaddControlValue(control, ...)pattern used across other modules (ModuleEffects, ModuleDrivers).
83-111: Clean refactoring to the addControl/addControlValue pattern.The control definition structure is well-organized with proper nesting for the "nodes" → "controls" hierarchy. The pattern matches other modules in the codebase (ModuleMoonLightInfo, ModuleLightsControl).
253-268: LGTM - Control update handling refactored correctly.The control object is properly fetched by index and passed to
updateControlandonUpdate. The bounds check on line 253 ensures safe access.src/MoonBase/Modules/ModuleIO.h (3)
76-76: LGTM - New button pin type added correctly.The
pin_Button_OnOffenum value is added at the correct position (afterpin_Ethernet), and the corresponding UI label "Button On/Off" is added at line 206 in the correct order to match the enum.
110-215: Clean refactoring to addControl/addControlValue pattern.The control definitions are well-structured and the UI labels correctly match the
IO_PinUsageenum order. The read-only flags for summary, Level, and DriveCap fields are appropriate.
275-275: LGTM - SE16V1 board preset updated for on/off button.Pin 46 is correctly assigned to the new
pin_Button_OnOfftype for the SE16V1 board.docs/develop/nodes.md (3)
17-17: LGTM - Documentation updated for addControl() API.The terminology update from
setControl()toaddControl()aligns with the code refactoring.
36-63: Good technical documentation expansion.The new technical section provides useful implementation details about Node class structure, live scripts, and lights. The content aligns well with the codebase.
68-80: Documentation correctly reflects driver rename.The "Parallel LED Driver" terminology is consistently used throughout, matching the code rename from PhysicalDriver.
src/MoonLight/Modules/ModuleDrivers.h (4)
54-56: Verify the simplified pin validation is intentional.The validation condition was simplified from checking
ledsPerPinvalues to only checkingledPins[readPos] != UINT8_MAX. The old validation is visible in the comment. Confirm thatledsPerPinvalidation is handled elsewhere or is no longer needed for correct pin compaction.
78-101: LGTM - addNodes refactored to use addControlValue pattern.The implementation correctly uses
addControlValue(control, getNameAndTags<...>())for all layout and driver registrations, consistent with the new API.
127-128: LGTM - PhysicalDriver correctly replaced with ParallelLEDDriver.The driver class rename is consistently applied in both the registration (line 91) and instantiation (lines 127-128).
159-160: LGTM - Node wiring extended for UI updates.The new
moduleNodesassignment enables driver nodes to trigger UI updates viarequestUIUpdate, which is used by the ParallelLEDDriver to refresh the status display.src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (4)
22-30: LGTM - Class renamed with new status member.The class is cleanly renamed to
ParallelLEDDriverwith:
- Updated
name()method returning "Parallel LED Driver"versionchanged fromchar[30]toChar<32>for consistency- New
statusmember to display pin assignment status in UI
38-45: LGTM - UI controls for version and status added correctly.The read-only text controls for
versionandstatusare properly defined, and the initialupdateControlcall ensures the version is displayed for existing nodes.
97-107: LGTM - Pin assignment bounds check correctly implemented.The bounds check at line 99 properly validates against
nrOfLedPins(notMAX_PINS), addressing the previous review concern. The fallback toledPins[i]is safe.
179-187: LGTM - Destructor correctly renamed.The destructor is properly renamed to
~ParallelLEDDriver()and the comment is updated accordingly.src/MoonBase/Nodes.h (9)
40-43: LGTM!Good defensive check to avoid appending an empty string and trailing space when no tags are present.
56-58: LGTM!Consistent with the existing pattern for module references (
moduleControl,moduleIO). The null initialization ensures safe default state.
67-70: LGTM!Passing
JsonArrayby const reference avoids an unnecessary copy.
98-120: Control metadata management looks solid.Setting
control["valid"] = true(line 102) is essential for the state synchronization fix described in the PR objectives. The conditional property management (lines 104-119) properly adds only non-default values and cleans up stale properties.Consider adding a brief comment above line 102 explaining the "valid" flag's role in preventing premature control removal during state sync, as discussed in the PR comments.
165-172: Bounds check addressed.The guard at line 167 properly handles the edge case of an empty controls array, preventing undefined behavior from underflow. The template approach allows flexible value types.
187-187: LGTM!Const references for both parameters are appropriate. The empty default implementation allows derived classes to optionally override.
212-212: LGTM!The optional
ledPinDIOparameter enables explicit LED pin assignment while maintaining backward compatibility via theUINT8_MAXdefault.
299-299: LGTM!Signature correctly matches the updated base class virtual method with proper
overridespecifier.
340-340: New driver header exists and old references are cleaned up.The include path change to
D_ParallelLEDDriver.his valid. The header file exists at the expected location, and no remaining references to the oldD_PhysicalDriver.horD_VirtualDriver.hheaders remain in the codebase.
src/MoonBase/NodeManager.h
Outdated
| // conversion 20251204 | ||
| if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) { | ||
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | ||
| nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags | ||
| EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); | ||
| } | ||
| if (contains(updatedItem.value.as<const char*>(), "IR Driver")) { | ||
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | ||
| nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags | ||
| EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); | ||
| } |
There was a problem hiding this comment.
Logging uses wrong variable for the updated name.
The second log statement logs updatedItem.value instead of nodeState["name"], which means it will show the old value rather than the newly assigned name.
Apply this diff to fix the logging:
if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) {
EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>());
nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags
- EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>());
+ EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>());
}
if (contains(updatedItem.value.as<const char*>(), "IR Driver")) {
EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>());
nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags
- EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>());
+ EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>());
}📝 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.
| // conversion 20251204 | |
| if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) { | |
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | |
| nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags | |
| EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); | |
| } | |
| if (contains(updatedItem.value.as<const char*>(), "IR Driver")) { | |
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | |
| nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags | |
| EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); | |
| } | |
| // conversion 20251204 | |
| if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) { | |
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | |
| nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags | |
| EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>()); | |
| } | |
| if (contains(updatedItem.value.as<const char*>(), "IR Driver")) { | |
| EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); | |
| nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags | |
| EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>()); | |
| } |
🤖 Prompt for AI Agents
In src/MoonBase/NodeManager.h around lines 138 to 148, the second debug log
prints updatedItem.value (the old name) after assigning nodeState["name"];
change those log calls to log nodeState["name"] instead so the message shows the
newly assigned name and tags; ensure both the "Physical Driver" and "IR Driver"
branches use nodeState["name"] in the trailing EXT_LOGD call.
| updateControl("status", statusString.c_str()); | ||
| moduleNodes->requestUIUpdate = true; |
There was a problem hiding this comment.
Add null check before accessing moduleNodes.
If moduleNodes is not assigned (e.g., in a different usage context), this line will cause a null pointer dereference.
Apply this diff:
updateControl("status", statusString.c_str());
- moduleNodes->requestUIUpdate = true;
+ if (moduleNodes) moduleNodes->requestUIUpdate = true;📝 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.
| updateControl("status", statusString.c_str()); | |
| moduleNodes->requestUIUpdate = true; | |
| updateControl("status", statusString.c_str()); | |
| if (moduleNodes) moduleNodes->requestUIUpdate = true; |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h around lines 110-111, the
code assumes moduleNodes is non-null before calling updateControl and setting
requestUIUpdate; add a null check to guard access: only call updateControl(...)
and set moduleNodes->requestUIUpdate = true when moduleNodes is not null,
otherwise skip those operations or handle the null case appropriately (e.g.,
return or log) to avoid a potential null pointer dereference.
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)
interface/src/lib/components/moonbase/MultiRow.svelte (1)
51-55: Reordering while a filter is active will permanently drop non-matching items.
filteredItems(lines 94–125) maps items as{ item }only—it lacksoriginalIndextracking. When a filter is applied,DraggableListreceives only the filtered subset:// Both paths skip originalIndex return data[property.name].map((item: any) => ({ item })) // no filter // or return data[property.name] .map((item: any) => ({ item })) // with filter .filter(({ item }: { item: any }) => { /* ... */ });When
handleReorderis called (line 51), it receives only the reordered filtered items and overwrites the entire array:data[property.name] = reorderedItems.map((wrapper: any) => wrapper.item);Any items that did not match the filter are permanently lost.
Fix: Preserve
originalIndexinfilteredItemsand use it to reorder only the filtered positions within the full array:-let filteredItems = $derived.by(() => { +let filteredItems = $derived.by(() => { - if (!query) return data[property.name].map((item: any) => ({ item })); + if (!query) return data[property.name].map((item: any, index: number) => ({ item, originalIndex: index })); - return data[property.name] - .map((item: any) => ({ item })) + return data[property.name] + .map((item: any, index: number) => ({ item, originalIndex: index })) .filter(({ item }: { item: any }) => {Then update
handleReorder:-function handleReorder(reorderedItems: any[]) { +function handleReorder(reorderedItems: { item: any; originalIndex: number }[]) { console.log('handleReorder', property.name, reorderedItems); - data[property.name] = reorderedItems.map((wrapper: any) => wrapper.item); + const full = [...data[property.name]]; + reorderedItems.forEach(({ item }, pos) => { + const originalIndex = filteredItems[pos].originalIndex; + full[originalIndex] = item; + }); + data[property.name] = full; onChange(); }Also affects: lines 94–125 (filteredItems definition) and 183–188 (DraggableList usage).
♻️ Duplicate comments (5)
src/MoonBase/Char.h (1)
102-110: Missing bounds check forend < begin.When
end < begin, the expressionend - begin + 1underflows (uint16_t), causing unintended behavior. Whilestrlcpylimits the copy tosizeof(sub.s), the function would copy the wrong content instead of returning an empty string.This was flagged in a previous review. Consider applying:
Char<N> substring(uint16_t begin, uint16_t end = sizeof(s) - 1) { Char<N> sub; - if (begin >= sizeof(s) || end >= sizeof(s)) + if (begin >= sizeof(s) || end >= sizeof(s) || end < begin) sub = ""; else { strlcpy(sub.s, s + begin, end - begin + 1); } return sub; }src/MoonBase/NodeManager.h (1)
138-148: Log message still prints the old node name instead of the migrated one.In the migration block you assign:
nodeState["name"] = getNameAndTags<ParallelLEDDriver>();but the second log line still prints
updatedItem.value:EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>());So the log does not show the newly assigned name/tags.
You likely want to log
nodeState["name"]instead:- if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) { - EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); - nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags - EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); - } + if (contains(updatedItem.value.as<const char*>(), "Physical Driver")) { + EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); + nodeState["name"] = getNameAndTags<ParallelLEDDriver>(); // set to current combination of name and tags + EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>()); + } @@ - if (contains(updatedItem.value.as<const char*>(), "IR Driver")) { - EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); - nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags - EXT_LOGD(ML_TAG, "... to [%s]", updatedItem.value.as<const char*>()); - } + if (contains(updatedItem.value.as<const char*>(), "IR Driver")) { + EXT_LOGD(ML_TAG, "update [%s] to ...", updatedItem.value.as<const char*>()); + nodeState["name"] = getNameAndTags<IRDriver>(); // set to current combination of name and tags + EXT_LOGD(ML_TAG, "... to [%s]", nodeState["name"].as<const char*>()); + }docs/develop/nodes.md (1)
17-62: Documentation updates accurately reflect API changes.The updates to use
addControl()terminology, the expanded sections onhasOnLayout(), moving heads, and technical details all align well with the codebase changes. The restructured content improves clarity for developers implementing nodes.Note: The spelling error "choosen" → "chosen" on line 57 was already flagged in a previous review.
docs/moonbase/inputoutput.md (1)
6-43: ESP32 board documentation is clear and comprehensive.The new sections on board presets, pin usage, and naming conventions provide valuable hardware-specific guidance that aligns with the codebase changes in ModuleIO.
Note: The spelling "Relais brightness" on line 39 was already flagged in a previous review (should likely be "Relay brightness").
src/MoonLight/Modules/ModuleLightsControl.h (1)
241-244: LGTM: Button debouncing correctly implemented.The debounce logic is sound:
- 50ms debounce delay is appropriate for mechanical button bounce
- Only triggers on button press (HIGH→LOW transition with
INPUT_PULLUP)lastStateis updated only after the debounce period, preventing spurious toggles during bouncepinModeconfiguration is handled inreadPins()(line 83)The past review concerns have been properly addressed.
Note: The past review comment about button debouncing and pinMode configuration has been fully addressed.
Also applies to: 293-307
🧹 Nitpick comments (4)
src/MoonBase/Char.h (2)
130-141:strncpydoesn't guarantee null-termination.If the source string fills the buffer,
strncpywon't null-terminate the destination. While the class's use ofstrlcpyelsewhere ensuressis always properly terminated, usingstrlcpyhere would be more defensive.void split(const char* splitter, std::function<void(const char*, uint8_t)> callback) { char savedS[N]; - strncpy(savedS, s, N); // save s + strlcpy(savedS, s, N); // save s char* token = strtok(s, splitter); // eat s uint8_t sequence = 0; while (token != nullptr) { callback(token, sequence); sequence++; token = strtok(nullptr, splitter); } - strncpy(s, savedS, N); // restore s + strlcpy(s, savedS, N); // restore s }
172-200: Preferinlineoverstaticfor header-defined free functions.
staticgives each translation unit its own copy of these functions, causing code bloat if the header is included in multiple files. Usinginlineallows the linker to merge duplicates.-static bool equal(const char* a, const char* b) { +inline bool equal(const char* a, const char* b) { if (a == nullptr || b == nullptr) { return false; } return strcmp(a, b) == 0; } -static bool equalAZaz09(const char* a, const char* b) { +inline bool equalAZaz09(const char* a, const char* b) { // ... } -static bool contains(const char* a, const char* b) { +inline bool contains(const char* a, const char* b) { // ... }docs/develop/standardsguidelines.md (1)
12-12: Polish language in standards doc (optional but recommended).A few small wording/consistency fixes would make this standards document crisper:
- Line 12: sentence is a bit awkward.
- Line 38: capitalize “GitHub”.
- Lines 55, 58: prefer “AI-generated”.
- Line 67: prefer “Right-click”.
Example patch:
-* A pull request should contain compilable code and tested to not crash the system at minimal and support also boards without PSRAM, e.g. ESP32-D0. Code may be work in progress. +* A pull request should contain compilable code that is tested to, at minimum, not crash the system and also supports boards without PSRAM, e.g. ESP32-D0. Code may be work in progress. @@ -> Do not use "force-push" while your PR is open! -> It has many subtle and unexpected consequences on our github repository. +> Do not use "force-push" while your PR is open! +> It has many subtle and unexpected consequences on our GitHub repository. @@ - * As the person who contributes source code to MoonLight, make sure you understand exactly what the AI generated code does + * As the person who contributes source code to MoonLight, make sure you understand exactly what the AI-generated code does @@ - * always review AI generated source code + * always review AI-generated source code @@ -Right click Format Document on each file you edit before committing. +Right-click Format Document on each file you edit before committing.Also applies to: 38-38, 55-55, 58-58, 67-67
src/MoonBase/Modules/ModuleIO.h (1)
44-45: Enum/string alignment and new Button On/Off preset mapping look correct.
- The
IO_PinUsageenum order matches theusageselect’saddControlValueentries, including the new"Button On/Off"entry forpin_Button_OnOff.- Board presets correctly use the new usages:
board_SE16V1:pins[46]["usage"] = pin_Button_OnOff;- Relay-related presets now refer to
pin_Relay/pin_Relay_Brightnessand corresponding labels.setupDefinition(const JsonArray& controls)is consistent with the new controls-based API elsewhere.If you touch this code again, you might consider using
std::size(ledPins)(orsizeof(ledPins) / sizeof(ledPins[0])) in the LED pin loops for future-proofing, but it’s not urgent.Also applies to: 76-77, 110-115, 148-207, 269-276, 312-333
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/develop/development.md(3 hunks)docs/develop/installation.md(2 hunks)docs/develop/nodes.md(2 hunks)docs/develop/standardsguidelines.md(1 hunks)docs/develop/sveltekit.md(1 hunks)docs/gettingstarted/overview.md(1 hunks)docs/moonbase/devices.md(1 hunks)docs/moonbase/inputoutput.md(1 hunks)docs/moonlight/drivers.md(2 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(4 hunks)misc/livescripts/E_PanTilt.sc(1 hunks)platformio.ini(3 hunks)src/MoonBase/Char.h(1 hunks)src/MoonBase/Module.h(5 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonBase/NodeManager.h(3 hunks)src/MoonLight/Modules/ModuleLightsControl.h(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MoonBase/Module.h
- docs/moonbase/devices.md
🧰 Additional context used
🧠 Learnings (6)
📚 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:
docs/develop/development.mddocs/develop/sveltekit.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Assume the Vite development server (`npm run dev`) is running during frontend development
Applied to files:
docs/develop/development.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Be aware of the integrated build process where the PlatformIO build automatically builds and embeds the SvelteKit frontend into the ESP32 firmware
Applied to files:
docs/develop/development.mddocs/develop/sveltekit.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
docs/develop/development.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/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/interface/svelte.config.js : Configure SvelteKit build and adapter settings in `interface/svelte.config.js`
Applied to files:
docs/develop/sveltekit.md
🧬 Code graph analysis (2)
src/MoonBase/Char.h (1)
src/MoonBase/NodeManager.h (2)
begin(36-74)void(78-78)
src/MoonBase/NodeManager.h (3)
src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)src/MoonBase/Nodes.h (9)
void(67-70)void(73-73)void(201-201)void(202-202)void(203-203)void(206-206)void(215-215)Node(48-163)Node(219-219)src/MoonBase/Char.h (2)
contains(115-115)contains(195-200)
🪛 Clang (14.0.6)
src/MoonBase/Char.h
[error] 14-14: 'Arduino.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
docs/develop/development.md
[style] ~40-~40: To form a complete sentence, be sure to include a subject.
Context: ...w components used in MoonLight modules. Might be rewriteen as MoonLight Module in the...
(MISSING_IT_THERE)
[grammar] ~40-~40: Ensure spelling is correct
Context: ...nts used in MoonLight modules. Might be rewriteen as MoonLight Module in the future. * ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~44-~44: Ensure spelling is correct
Context: ...* src folder for MoonBase and MoonLight vack-end * MoonLight Nodes: the easiest ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/develop/nodes.md
[style] ~17-~17: In American English, abbreviations like “etc.” require a period.
Context: ...pes (sliders/range, checkboxes, numbers etc). They are added with the addControl() ...
(ETC_PERIOD)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...e supports. They control when a physical to virtual mapping is recalculated *...
(QB_NEW_EN_HYPHEN)
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...upports. They control when a physical to virtual mapping is recalculated * **...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...support different lights e.g a mix of 15 channel lights and 32 channel lights etc...
(QB_NEW_EN_HYPHEN)
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...ts e.g a mix of 15 channel lights and 32 channel lights etc. You could set channe...
(QB_NEW_EN_HYPHEN)
[style] ~31-~31: Consider using polite language here.
Context: ...wheel could be used to really go crazy. Let us know when you have one of these setups 🚨 * Movin...
(INSERT_PLEASE)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...he LEDs / channels array and the virtual to physical layer mappings. * specif...
(QB_NEW_EN_HYPHEN)
[grammar] ~45-~45: Use a hyphen to join words.
Context: ...LEDs / channels array and the virtual to physical layer mappings. * specify w...
(QB_NEW_EN_HYPHEN)
[style] ~62-~62: In American English, abbreviations like “etc.” require a period.
Context: ...tPan, setTilt, setZoom etc. Also getRGB etc functions exists. ## Drivers ### Init...
(ETC_PERIOD)
docs/develop/standardsguidelines.md
[style] ~12-~12: Consider changing the order of words to improve your wording.
Context: ...ould contain compilable code and tested to not crash the system at minimal and support...
(TO_NOT_VB)
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...btle and unexpected consequences on our github repository. > For example, we regularly...
(GITHUB)
[style] ~52-~52: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... descriptions into English. AI bots are very good at understanding language. However, alw...
(EN_WEAK_ADJECTIVE)
[grammar] ~55-~55: Use a hyphen to join words.
Context: ... sure you understand exactly what the AI generated code does * best practice: a...
(QB_NEW_EN_HYPHEN)
[grammar] ~58-~58: Use a hyphen to join words.
Context: ...nts for correctness * always review AI generated source code * If the AI has ...
(QB_NEW_EN_HYPHEN)
[grammar] ~67-~67: Use a hyphen to join words.
Context: ...and Prettier for front-end files. Right click Format Document on each file you e...
(QB_NEW_EN_HYPHEN)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... what it does, including its arguments. Comments are also formatted using Clang-format a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/develop/nodes.md
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
62-62: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
78-78: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/develop/standardsguidelines.md
5-5: Images should have alternate text (alt text)
(MD045, no-alt-text)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
89-89: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
90-90: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/moonbase/inputoutput.md
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 4; Actual: 7
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
31-31: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
docs/develop/sveltekit.md (1)
67-67: Terminology standardization is correct and consistent with documentation.Line 67 updates link text labels from "Front End"/"Back End" to "Front-end"/"Back-end". Web search confirms that the MoonLight documentation pages use this hyphenated terminology consistently across
/sveltekit/,/structure/, and development pages. This change aligns with the broader project terminology standardization efforts.misc/livescripts/E_PanTilt.sc (1)
12-12: The 3-parameter checkbox signature is correct and consistent with codebase patterns.All checkbox controls in the codebase use exactly this signature:
addControl(&var, "name", "checkbox"). The implementation on line 12 aligns with established patterns across the project (see L_panels.sc, M_MoonLight.h, etc.), confirming this is the appropriate API for checkbox controls. No changes needed.src/MoonBase/Char.h (5)
22-28: LGTM!Constructors are well-designed with safe string handling via
strlcpy. The converting constructor template allows seamless size conversions with automatic truncation.
30-53: LGTM!Assignment operators correctly handle null checks for JSON types and use safe string copy operations throughout.
69-92: LGTM!Concatenation operators follow correct C++ idioms - non-mutating
operator+returns a copy whileoperator+=modifies in place. Buffer size for int conversion is adequate for 32-bit integers.
122-128: LGTM!The
formatmethod safely usesvsnprintfwhich handles buffer limits correctly.
156-162: LGTM!The non-member
operator+enables natural"prefix" + charObjsyntax with safe string operations.docs/gettingstarted/overview.md (1)
25-25: Sitemap wording / hyphenation looks consistent.The updated “Front-end / Back-end” wording matches the rest of the docs; no further changes needed.
docs/develop/installation.md (1)
41-41: Installation doc changes read clearly.The updated note about developing in branches from
mainand the front-end regeneration explanation are clear and accurate; no changes required.Also applies to: 113-118
platformio.ini (1)
36-36: Config and dependency tweaks look reasonable; please confirm on target boards.
- The “Parallel LED Driver” wording and APP_DATE bump are consistent with the rest of the PR.
- The HP_ALL_DRIVERS I2SClocklessLedDriver hash change should be verified on the usual test boards to confirm the intended SK6812 behavior and that no regressions were introduced.
Also applies to: 59-59, 215-216
src/MoonBase/NodeManager.h (1)
78-112: NodeManager UI/control wiring refactor looks consistent.
setupDefinition(const JsonArray& controls)now usesaddControl/addControlValueto definenodesand nestedcontrols, matching the new control model inNodes.h.- Passing a concrete
JsonObject controlintonodeClass->updateControl(control)andnodeClass->onUpdate(updatedItem.oldValue, control)in thenodes[i].controls[j].valuebranch is a good improvement over index-only updates and keeps node logic localized.Please just make sure all NodeManager subclasses override
addNodes(const JsonObject& control)andaddNode(..., const JsonArray& controls)with the new signatures.Also applies to: 253-268
docs/moonlight/drivers.md (1)
11-46: Driver documentation accurately updated.The terminology shift to "Parallel LED Driver" and the hardware peripheral details (I2S for ESP32-D0, LCD_CAM for ESP32-S3, ParLIO for ESP32-P4) are accurate and provide useful platform-specific guidance.
src/MoonLight/Modules/ModuleLightsControl.h (3)
28-29: LGTM: Consistent pin initialization.The rename from
pinRelaisBrightnesstopinRelayBrightness(fixing the spelling) and the addition ofpinToggleOnOffboth useUINT8_MAXfor consistent invalid-pin initialization.
72-89: LGTM: Pin configuration properly initialized.The
readPins()function correctly:
- Resets pins to
UINT8_MAXon each update- Configures
INPUT_PULLUPfor the button (line 83)- Handles the renamed
pin_Relay_Brightnessenum
92-137: LGTM: Clean migration to addControl API.The
setupDefinitionrewrite replaces manual JSON construction with the cleaneraddControl()/addControlValue()pattern, improving maintainability and consistency with the rest of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/MoonBase/Char.h (1)
102-110: Add missingend < begincheck insubstring().As noted in the previous review, the bounds check is incomplete. When
end < begin, line 107 computesend - begin + 1which underflows (both areuint16_t), producing a very large size value. Whilestrlcpyremains memory-safe due to its destination buffer limit, the method returns incorrect results.Apply this diff to add the missing check:
Char<N> substring(uint16_t begin, uint16_t end = sizeof(s) - 1) { Char<N> sub; - if (begin >= sizeof(s) || end >= sizeof(s)) + if (begin >= sizeof(s) || end >= sizeof(s) || end < begin) sub = ""; else { strlcpy(sub.s, s + begin, end - begin + 1); } return sub; }
🧹 Nitpick comments (2)
src/MoonBase/Char.h (1)
61-68: Remove commented-out code.The commented operator+ implementations should be removed to keep the codebase clean.
Apply this diff:
- // Char& operator+(const char* rhs) { - // strlcat(s, rhs, sizeof(s)); - // return *this; - // } - // Char& operator+(const String& rhs) { - // strlcat(s, rhs.c_str(), sizeof(s)); - // return *this; - // }interface/src/lib/components/moonbase/MultiRow.svelte (1)
51-60: Fragile dependency on derivedfilteredItemsstate.The reordering logic accesses
filteredItems[pos].originalIndex(line 55), coupling the callback to the current derived state. While Svelte's derived values are recalculated when dependencies change and stable during synchronous reads, this pattern is fragile if the filter or data changes during the drag operation. Additionally,originalIndexis included in thereorderedItemstype signature but never destructured or used, which is confusing.Consider one of these approaches for clarity and robustness:
Option 1: Build a stable index map before the loop
function handleReorder(reorderedItems: { item: any; originalIndex: number }[]) { console.log('handleReorder', property.name, reorderedItems); const full = [...data[property.name]]; + // Capture the current filteredItems mapping before any potential state changes + const indexMap = filteredItems.map(f => f.originalIndex); reorderedItems.forEach(({ item }, pos) => { - const originalIndex = filteredItems[pos].originalIndex; + const originalIndex = indexMap[pos]; full[originalIndex] = item; }); data[property.name] = full; onChange(); }Option 2: Pass the mapping from DraggableList if possible
IfDraggableListcan preserve metadata from the originalitemsprop, have it return the full structure so you can destructure directly:-reorderedItems.forEach(({ item }, pos) => { - const originalIndex = filteredItems[pos].originalIndex; +reorderedItems.forEach(({ item, originalIndex }, pos) => { + const targetIndex = filteredItems[pos].originalIndex; + full[targetIndex] = item;(This assumes
reorderedItemsincludes the stable mapping you need.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/develop/development.md(4 hunks)docs/develop/installation.md(3 hunks)docs/develop/standardsguidelines.md(1 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(5 hunks)src/MoonBase/Char.h(1 hunks)src/MoonBase/Utilities.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/develop/installation.md
🧰 Additional context used
🧠 Learnings (5)
📚 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:
docs/develop/development.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Assume the Vite development server (`npm run dev`) is running during frontend development
Applied to files:
docs/develop/development.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
docs/develop/development.mdsrc/MoonBase/Utilities.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/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
src/MoonBase/Utilities.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} : Use ArduinoJson for JSON serialization and deserialization in C++ backend code
Applied to files:
src/MoonBase/Utilities.h
🧬 Code graph analysis (1)
src/MoonBase/Char.h (1)
src/MoonBase/Module.h (3)
void(150-166)void(194-194)void(195-195)
🪛 Clang (14.0.6)
src/MoonBase/Char.h
[error] 14-14: 'Arduino.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
docs/develop/development.md
[style] ~40-~40: To form a complete sentence, be sure to include a subject.
Context: ...w components used in MoonLight modules. Might be rewritten as a MoonLight Module in t...
(MISSING_IT_THERE)
[style] ~93-~93: Consider using a different verb to strengthen your wording.
Context: ...nning on your computer. This very much speeds up UI development, as no flashing to the E...
(SPEED_UP_ACCELERATE)
docs/develop/standardsguidelines.md
[style] ~52-~52: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... descriptions into English. AI bots are very good at understanding language. However, alw...
(EN_WEAK_ADJECTIVE)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... what it does, including its arguments. Comments are also formatted using Clang-format a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/develop/development.md
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
docs/develop/standardsguidelines.md
41-41: Bare URL used
(MD034, no-bare-urls)
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
docs/develop/standardsguidelines.md (3)
6-12: Develop principles section is well-structured and aligned with PR objectives.The principles clearly codify the architectural direction (one firmware per board, documentation-with-code, upstream sync minimization, main-branch controls, modular src/lib/interface separation). These standards support the broader refactors evident in the PR.
14-23: PR description guidance is comprehensive and actionable.The section clearly sets expectations (avoid minimal descriptions), provides concrete examples of good coverage (what to achieve, how it works, testing, known limitations, areas for help), and reinforces the policy that documentation should accompany code. This aligns well with the PR workflow discipline.
44-60: AI-assisted code guidance is clear and sets appropriate expectations.The section appropriately acknowledges that AI-generated code is acceptable, reinforces the need for author understanding and verification, and establishes concrete best practices (comments for larger AI sections, review translations, verify refactors don't lose context). The emphasis on understanding and verification is constructive.
docs/develop/development.md (5)
11-11: Link to new standards guide improves navigation and reinforces contributor expectations.The addition of a clear link to the Standards and Guidelines document at the top of the workflow section is a good architectural choice. It promotes the standardized practices before diving into step-by-step instructions.
Please verify that the link
https://moonmodules.org/MoonLight/develop/standardsguidelines/is correct and accessible in the published documentation. The relative path in the markdown should resolve correctly in the final documentation build.
17-17: Branch-from-main clarification is helpful and aligns with develop principles.Adding the explicit instruction "Only branch from the main branch!" reinforces the policy documented in the new Standards and Guidelines and reduces ambiguity for contributors.
23-23: Terminology standardization (Front-end/Back-end) and grammar corrections are consistent and clear.The changes to use hyphenated "Front-end (UI)" and "Back-end (Server)" improve consistency with the new Standards and Guidelines document, and the grammar corrections ("are also made", "Might be rewritten as a MoonLight Module") align with feedback from prior reviews. The overall structure and readability are improved.
Also applies to: 36-36, 40-44
53-53: Formatting instruction is now clearer and more actionable.The rephrased instruction to "(right-click Format Document on each page you change)" is more direct and actionable than the previous version, improving clarity for contributors unfamiliar with IDE shortcuts.
70-70: Terminology and phrasing refinements improve consistency and readability.Corrections to "Additional info", updates to use "front-end" and "back-end" consistently, and rephrasing "Saving a UI file is enough to see the results" (removing the excessive exclamation marks and improving flow) align the document with the new Standards and Guidelines and enhance professional tone.
Also applies to: 91-93
src/MoonBase/Utilities.h (3)
18-18: Char utilities split into Char.h—include looks fineIncluding
Char.hhere keeps existingUtilities.hconsumers working after moving theCharutilities out; no issues from this change alone.
26-76: Compile-time log-level gating changes behavior of log argumentsThe new
CORE_DEBUG_LEVEL-based#ifblocks mean that when the level is below the threshold, theEXT_LOG*macros expand to no-ops and their arguments are not evaluated at all. That’s usually desirable, but it is a behavior change vs always-compiling calls, so it’s worth double‑checking that noEXT_LOG*call sites rely on side effects in their arguments (e.g., function calls doing work beyond formatting).
269-270: fastDiv255 inline helper is fine as a header utilityChanging to a single
inlinedefinition in the header (with an unsigned argument and shift) is safe and keeps the fast path available everywhere without linkage issues.src/MoonBase/Char.h (4)
1-16: LGTM!Header, licensing, and includes are appropriate for an Arduino utility header.
17-53: LGTM!Constructors and assignment operators are well-designed with safe string handling via
strlcpy. Null checks for JSON types and template support for different-sized Char instances are appropriate.
69-92: LGTM!Concatenation operators are correctly implemented. The
operator+returns by value (non-mutating), whileoperator+=modifies in place. All operations usestrlcatwith proper bounds, and integer formatting uses boundedsnprintf.
112-200: LGTM!The remaining utility methods demonstrate safe string handling:
- Bounded functions (
strnlen,strnstr,vsnprintf,strftime) prevent buffer overflows- The
split()method cleverly preserves the original string by saving and restoring state- Non-member
operator+enables natural"string" + Char<N>syntax- Utility functions include appropriate null checks
| * Only branch from the main branch! Press Branches, press New Branch, give it a name e.g. background-script and press Create new Branch, click on background-script | ||
|
|
||
| <img width="90" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" /> | ||
| <img width="90" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" /> |
There was a problem hiding this comment.
Add alt text to image for accessibility.
The image on line 19 is missing alt text, which is required for accessibility and is flagged by markdownlint.
Apply this diff:
- <img width="90" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" />
+ <img width="90" alt="Branch creation interface showing 'New Branch' button and 'background-script' branch name" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" />📝 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.
| <img width="90" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" /> | |
| <img width="90" alt="Branch creation interface showing 'New Branch' button and 'background-script' branch name" src="https://github.com/user-attachments/assets/588d0854-bac1-4b70-8931-ba6db4c94248" /> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
In docs/develop/development.md around line 19, the inline image tag lacks alt
text which fails markdownlint and reduces accessibility; update the <img>
element to include a concise, descriptive alt attribute (e.g., alt="brief
description of image") that conveys the image's purpose/context, ensuring the
src remains unchanged and the alt text accurately describes the image for screen
readers.
src/MoonBase/Char.h
Outdated
| bool operator==(const Char& rhs) const { return strcmp(s, rhs.s) == 0; } | ||
| bool operator!=(const char* rhs) const { return strcmp(s, rhs) != 0; } | ||
|
|
||
| char operator[](const uint16_t indexV) const { return s[indexV]; } |
There was a problem hiding this comment.
Add bounds check to prevent out-of-bounds access.
The index operator accesses s[indexV] without validating that indexV < N. This can cause out-of-bounds reads and undefined behavior.
Apply this diff to add bounds checking:
- char operator[](const uint16_t indexV) const { return s[indexV]; }
+ char operator[](const uint16_t indexV) const {
+ return (indexV < sizeof(s)) ? s[indexV] : '\0';
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char operator[](const uint16_t indexV) const { return s[indexV]; } | |
| char operator[](const uint16_t indexV) const { | |
| return (indexV < sizeof(s)) ? s[indexV] : '\0'; | |
| } |
🤖 Prompt for AI Agents
In src/MoonBase/Char.h around line 100, the const index operator accesses
s[indexV] without validating indexV < N; add a bounds check that if indexV >= N
throws a std::out_of_range (with a clear message including the invalid index and
N) to prevent undefined behavior, otherwise return s[indexV]; ensure <stdexcept>
is included if not already.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/MoonBase/Char.h (1)
93-100:substring()still has off‑by‑one truncation for single‑character ranges.
strlcpy(sub.s, s + begin, end - begin + 1);allocates space only forend - begincharacters plus the NUL terminator. Forsubstring(0, 0), the size argument becomes1, sostrlcpycopies0characters and you get an empty string instead of a single character. The newend < beginguard fixes the underflow, but the truncation bug remains.You can compute the intended length and pass
len + 1as the buffer size (clamped to the capacity):- Char<N> substring(uint16_t begin, uint16_t end = sizeof(s) - 1) { - Char<N> sub; - if (begin >= sizeof(s) || end >= sizeof(s) || end < begin) - sub = ""; - else { - strlcpy(sub.s, s + begin, end - begin + 1); - } - return sub; - } + Char<N> substring(uint16_t begin, uint16_t end = sizeof(s) - 1) { + Char<N> sub; + if (begin >= sizeof(s) || end >= sizeof(s) || end < begin) { + sub = ""; + } else { + size_t len = static_cast<size_t>(end) - static_cast<size_t>(begin) + 1; // characters to copy + if (len >= sizeof(sub.s)) { + len = sizeof(sub.s) - 1; // leave room for terminator + } + strlcpy(sub.s, s + begin, len + 1); + } + return sub; + }It would be good to have tests for
substring(0, 0), a multi‑char range (e.g.substring(1, 3)), and out‑of‑range inputs.
🧹 Nitpick comments (4)
docs/develop/standardsguidelines.md (2)
52-52: Consider a stronger descriptor than "very good".Line 52 uses "very good" which is somewhat weak phrasing. Consider replacing with a more direct descriptor such as "excellent," "skilled," or "adept" to strengthen the statement.
69-72: Reduce repetition of "Comments" at the start of consecutive sentences.Lines 71–72 begin consecutive sentences with "Comments," which breaks up the flow. Consider rewording one of these sentences to vary the opening. For example:
-Comments are used for technical documentation, always document your changes, each function must have a comment describing what it does, including its arguments. Comments are also formatted using Clang-format and Prettier. +Comments are used for technical documentation, always document your changes, and each function must have a comment describing what it does, including its arguments. The formatting of comments follows Clang-format and Prettier standards.src/MoonBase/Char.h (2)
19-21: Add a compile-time guard to prevent zero-capacityChar<0>.
CharwithN == 0would produce an invalid buffer type. It’s cheap and robust to enforce a positive capacity at compile time.You can add a static assertion inside the struct:
template <size_t N> struct Char { + static_assert(N > 0, "Char capacity N must be greater than 0"); char s[N] = "";
121-132:split()usesstrtok, which is non‑reentrant and globally stateful.Using
strtokonsis fine in a single‑threaded, controlled context, but it relies on global state and will interfere with any otherstrtokusage in the process. It also meanssplit()is not re‑entrant if called from nested contexts.If you anticipate concurrent or nested use, consider one of:
- Switching to
strtok_r(or equivalent re‑entrant variant) if available on your target.- Or implementing a small manual tokenizer that walks
swithstrchr/ pointer arithmetic, avoiding globalstrtokstate entirely.The current implementation is acceptable for many Arduino setups but worth revisiting if this utility becomes widely reused.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/develop/standardsguidelines.md(1 hunks)interface/src/lib/components/moonbase/MultiRow.svelte(5 hunks)src/MoonBase/Char.h(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
src/MoonBase/Char.h
[error] 14-14: 'Arduino.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
docs/develop/standardsguidelines.md
[style] ~41-~41: Try moving the adverb to make the sentence clearer.
Context: ... very useful tips and tricks here: [How to properly submit a PR](https://github.com/wled-dev/WLED/wiki/...
(SPLIT_INFINITIVE)
[style] ~52-~52: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... descriptions into English. AI bots are very good at understanding language. However, alw...
(EN_WEAK_ADJECTIVE)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... what it does, including its arguments. Comments are also formatted using Clang-format a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
docs/develop/standardsguidelines.md (1)
54-60: Verify list indentation meets markdown linting standards.The past review flagged list indentation issues on lines 55–59 (expected 0, actual 2). The current code shows these items at column 0 without leading spaces. To ensure this aligns with markdownlint expectations, please verify the rendered markdown list structure is correct and the indentation warnings no longer appear.
interface/src/lib/components/moonbase/MultiRow.svelte (4)
51-62: LGTM! Reordering logic correctly preserves filtered item positions.The indexMap approach correctly handles reordering of filtered items by mapping the new order to the original array positions. The shallow copy on line 53 is safe since line 58 assigns entire item references rather than mutating properties.
102-136: LGTM! Filtering correctly preserves original indices.The derived state correctly returns items wrapped with their original indices in both filtered and unfiltered cases, maintaining consistency with the
handleReordersignature. The slice to the first 3 properties (line 118) matches the display logic (line 207).
157-157: LGTM! CRUD checks now use consistent optional chaining.All CRUD permission checks have been updated to use optional chaining for both the
crudproperty access and theincludes()method call. This fully addresses the null-safety concerns raised in previous reviews.Also applies to: 198-198, 203-203, 240-240
194-199: LGTM! DraggableList props correctly wired.The
dragDisabledlogic correctly enables dragging when the sort permission ('s') is present or when CRUD is undefined. TheitemsandonReorderprops align with the new signature changes.
| Char() = default; // Keep default constructor | ||
| Char(const char* str) { strlcpy(s, str, sizeof(s)); } // Constructor to allow initialization from string literal | ||
| template <size_t M> | ||
| Char(const Char<M>& other) { // Converting constructor from different-sized Char | ||
| strlcpy(s, other.c_str(), sizeof(s)); | ||
| } | ||
|
|
||
| // assign operators | ||
| Char& operator=(const char* rhs) { | ||
| // if (strlen(rhs) >= N) EXT_LOGW("Char", "Truncating '%s' from %d to %d chars", rhs, strlen(rhs), N - 1); | ||
| strlcpy(s, rhs, sizeof(s)); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if there's documentation or usage of strlcpy in the repo
rg -i "strlcpy" --type h --type cpp -B 2 -A 2 | head -50Repository: MoonModules/MoonLight
Length of output: 2656
🏁 Script executed:
# Search for calls to the Char constructor and operator= with nullptr or optional values
rg "Char\s*\(" -t cpp -t h -A 1 | head -100Repository: MoonModules/MoonLight
Length of output: 513
🏁 Script executed:
# Look for any null safety checks in the codebase
rg "str\s*\?" --type cpp --type h | head -30Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for assignments or constructions of Char type
rg "Char\s*\w*\s*=.*;" -t cpp -A 0 | head -40Repository: MoonModules/MoonLight
Length of output: 191
🏁 Script executed:
# Look for JSON or config parsing that might return nullptr
rg "optional|nullptr|null|NULL" -t cpp --type h -B 2 -A 2 | grep -i "char\|str" | head -50Repository: MoonModules/MoonLight
Length of output: 5347
🏁 Script executed:
# Check if there are any defensive patterns already in use elsewhere
rg "\?\s*:\s*\"\"|str\s*\?\|safe" -t cpp -t h | head -30Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for function signatures that accept optional strings
rg "const char\*.*=\s*nullptr|optional.*char" -t cpp -t h -B 2 | head -50Repository: MoonModules/MoonLight
Length of output: 1622
🏁 Script executed:
# Check strlcpy man page or any documentation in the repo
man strlcpy 2>/dev/null | head -100 || echo "Man page not available, checking documentation"Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for Char instantiations or assignments from JSON/optional sources
rg "Char.*=.*json|Char.*=.*optional|Char.*\[\].*getString" -t cpp -t h -B 2 -A 2 | head -60Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Look for where Char might be assigned from config or external data
rg "setValue|setString|fromJson" -t cpp -A 3 | grep -i "char" | head -40Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check if there's a pattern of nullptr checks before strlcpy elsewhere
rg "if.*\?" -t cpp -B 1 -A 1 | grep -B 1 -A 1 "strcpy\|strncpy\|strlcpy" | head -30Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check what string functions are available and how they're used
rg "include.*string|include.*cstring" -t cpp -t h | head -20Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Look at the Char.h file more completely to understand its context
cat -n src/MoonBase/Char.h | head -80Repository: MoonModules/MoonLight
Length of output: 3253
🏁 Script executed:
# Check if strlcat also has the same vulnerability
rg "strlcat" -t cpp -t h -B 2 -A 2 src/MoonBase/Char.hRepository: MoonModules/MoonLight
Length of output: 694
Defensively handle null const char* inputs to avoid crash.
The Char(const char* str) constructor and operator=(const char* rhs) call strlcpy directly on the input pointer without null checking. If a caller passes nullptr, this will dereference address 0 and crash. Similarly, operator+=(const char* rhs) calls strlcat without null protection. Notably, the class already guards against null inputs in operator=(const JsonVariant) and operator=(const JsonString) (lines 37–48), but the base const char* paths remain unguarded—creating an inconsistency.
Normalize null pointers to an empty string in all three locations:
- Char(const char* str) { strlcpy(s, str, sizeof(s)); }
+ Char(const char* str) {
+ const char* safe = str ? str : "";
+ strlcpy(s, safe, sizeof(s));
+ }
Char& operator=(const char* rhs) {
- strlcpy(s, rhs, sizeof(s));
+ const char* safe = rhs ? rhs : "";
+ strlcpy(s, safe, sizeof(s));
return *this;
}
Char& operator+=(const char* rhs) {
- strlcat(s, rhs, sizeof(s));
+ const char* safe = rhs ? rhs : "";
+ strlcat(s, safe, sizeof(s));
return *this;
}This keeps the API consistent with the existing null-handling in the JSON variant operators and prevents crashes from nullable pointer sources.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonBase/Char.h around lines 24 to 36, the Char(const char*),
operator=(const char*), and related operator+=(const char*) paths call
strlcpy/strlcat directly on the incoming pointer and will crash if nullptr;
update these functions to defensively normalize a null const char* to an empty
string before calling strlcpy/strlcat (use the same null-check pattern as
operator=(const JsonVariant)/operator=(const JsonString) — e.g. treat input ==
nullptr ? "" : input), then proceed with strlcpy/strlcat and return *this.
Back end
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.