Open
Conversation
Backend ======= - Create DriverNode from Nodes - Create LiveScriptNode from Nodes - Module: .h to .cpp - NodeManager: .h to .cpp - add test_module.cpp
WalkthroughAdds new DriverNode and LiveScriptNode implementations, refactors Module/ModuleState and Nodes for centralized state and control management, introduces NodeManager, updates numerous module/socket access patterns to use SvelteKit, adds native ArduinoJson test dependency, and tweaks a Monitor UI preset constant. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Module as Module
participant ModuleState as ModuleState
participant NodeMgr as NodeManager
participant Node as Node
UI->>Module: onUpdate(UpdatedItem, originId)
Module->>ModuleState: compareRecursive/checkReOrderSwap
ModuleState-->>Module: UpdatedItem(s)
Module->>NodeMgr: onUpdate(UpdatedItem, originId)
NodeMgr->>NodeMgr: dispatch (name/on/control)
NodeMgr->>Node: create/update/configure node
Node-->>NodeMgr: acknowledge / requestMappings
NodeMgr-->>Module: node sync complete
sequenceDiagram
participant Script as Script File
participant LSN as LiveScriptNode
participant Parser as Parser
participant Runtime as Script Runtime
participant Driver as DriverNode
Script->>LSN: load animation path
LSN->>Parser: parseScript(code + wrappers)
Parser-->>LSN: compiled Executable
LSN->>Runtime: register host functions & sync primitives
Runtime->>Runtime: execute setup() (if present)
loop Frames
Runtime->>Driver: _setRGB / _setPan / _setTilt calls
Driver-->>Runtime: frame applied
Runtime->>Runtime: sync via semaphore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
lib/framework/WWWData.h (1)
4-5: Add required MoonLight markers on upstream SvelteKit asset updates.These hunks update embedded SvelteKit assets, but the required upstream marker comments are missing. Please tag these sections with
// 💫(MoonLight) next to the relevant asset-update comments/blocks.As per coding guidelines,
**/*.{cpp,h,hpp}files must mark upstream sveltekit changes with// 🌙(MoonBase) or// 💫(MoonLight).Also applies to: 34-36, 749-750, 763-779, 13741-13743, 14835-14837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/WWWData.h` around lines 4 - 5, The embedded SvelteKit asset blocks (e.g., the ESP_SVELTEKIT_DATA_0[] array and the other asset-update hunks referenced) are missing the required MoonLight marker; add the marker comment "// 💫" immediately next to each upstream SvelteKit asset-update comment/block (for example next to the ESP_SVELTEKIT_DATA_0[] declaration and the other indicated asset sections) so they clearly carry the MoonLight tag used by the codebase guidelines.platformio.ini (1)
321-321: Prefer a major-bounded ArduinoJson range for deterministic native builds.
ArduinoJson@>=7.0.0can silently pull a future major release and destabilize CI/test behavior.Patch
-lib_deps = ArduinoJson@>=7.0.0 +lib_deps = ArduinoJson@^7.0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 321, The lib_deps entry currently allows any future major release of ArduinoJson which can break native builds; update the ArduinoJson version specifier in platformio.ini (the lib_deps line referencing ArduinoJson) to a major-bounded range such as pinning to major version 7 (for example use a caret or explicit upper bound like "ArduinoJson@>=7.0.0,<8.0.0" or "ArduinoJson@^7.0.0") so CI and native builds remain deterministic.src/MoonBase/Module.h (1)
22-35: Add a small native test forUpdatedItemdefault sentinels.
UpdatedItem()now codifies important defaults (parent="",index=UINT8_MAX). A focused unit test would lock this behavior and prevent regressions in update routing.As per coding guidelines,
**/*.h: Add unit tests for simple, pure, side-effect-free functions and structs in the backend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Module.h` around lines 22 - 35, Add a small native unit test that includes Module.h, constructs an UpdatedItem with its default constructor, and asserts the sentinel defaults: both parent elements are empty strings (parent[0] and parent[1] compare equal to ""), and both index elements equal UINT8_MAX; keep the test focused, header-only/pure (no side effects), and name it to reflect UpdatedItem defaults so CI will catch regressions to the UpdatedItem() constructor behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/DriverNode.cpp`:
- Line 49: The power-estimation call casts raw channel data to CRGB* which
assumes 3-byte pixels and breaks for RGBW; modify the correctedBrightness
assignment so you only call calculate_max_brightness_for_power_mW when
layerP.lights.header.channelsPerLight == 3 and layerP.lights.header.nrOfLights
<= 8096, otherwise set correctedBrightness to the raw brightness value;
reference the existing correctedBrightness variable,
layerP.lights.header.nrOfLights, layerP.lights.header.channelsPerLight, and
calculate_max_brightness_for_power_mW to locate and guard the call.
In `@src/MoonBase/DriverNode.h`:
- Around line 27-33: The header currently defines a static ledsDriver instance
per TU (symbols: ledsDriver with types PhysicalDriverESP32S3,
PhysicalDriverESP32D0, or LedsDriver); remove the static definitions from the
header and replace them with an extern declaration for ledsDriver, then add a
single definition in DriverNode.cpp that instantiates the correct concrete type
depending on the same CONFIG_IDF_TARGET_* macros; keep the HP_ALL_DRIVERS path
as-is and ensure the implementation file includes the header so all TUs share
the one global ledsDriver instance.
In `@src/MoonBase/LiveScriptNode.cpp`:
- Around line 174-178: The compileAndRun() method is still executing
parse/compile synchronously on the caller thread even though the comment says it
should be deferred; wrap the heavy work into a lambda and push it onto the
deferred task queue (the runInAppTask vector) instead of running inline so
parsing/compilation happens in the main SvelteKit loop, e.g. create a
capture-safe lambda that captures the needed locals (animation, type, error) and
this (or weak/ptr if required) and push_back that lambda to runInAppTask, then
return immediately from compileAndRun; apply the same change for the similar
synchronous block around lines 205-223 to ensure both compile paths run in the
deferred main-loop task.
- Around line 22-39: The code uses the global gNode (and global
WaitAnimationSync) which gets overwritten when multiple LiveScript nodes exist;
replace the globals with per-instance state and thread-safe callbacks: remove
gNode and make the callbacks methods or lambda closures that capture/receive a
Node* (e.g., convert static functions like _addControl, _modifyPosition,
_fadeToBlackBy, _setRGB, _setPan/_setTilt and the WaitAnimationSync usage to
instance methods or register callbacks with a void* userData carrying the
Node*), or store the Node* inside the LiveScriptNode instance and pass it
explicitly into any C-style callback registration so each LiveScript node uses
its own pointer instead of the global gNode; also make WaitAnimationSync an
instance member (or keyed by node instance) to avoid cross-instance
synchronization conflicts.
- Around line 189-193: The boolean script-capability flags (hasSetupFunction,
hasLoopFunction, hasOnLayoutFunction, hasModifyFunction) are not reset before
rescanning scScript in compileAndRun(), which lets stale true values persist;
update compileAndRun() (or the start of the scan block) to explicitly set all
those flags to false before running the find(...) checks so each invocation
recalculates capabilities from the current scScript contents.
- Line 115: The addExternal registration for beat8 has the wrong signature:
update the registration call that references addExternal("float
beat8(uint8_t,uint32_t)", (void*)beat8) to match FastLED's function type by
changing the return type to uint8_t and the first parameter to uint16_t (i.e.,
register as "uint8_t beat8(uint16_t,uint32_t)") so the addExternal declaration
matches the actual beat8 function prototype.
In `@src/MoonBase/Module.cpp`:
- Line 313: The log call using EXT_LOGD in Module.cpp is using the wrong tag
(ML_TAG) for a MoonBase message; update the EXT_LOGD invocation in the
requestUIUpdate logging statement to use MB_TAG instead of ML_TAG (the line that
logs "requestUIUpdate %s" with _moduleName) so MoonBase logs use the MB_TAG
constant.
In `@src/MoonBase/NodeManager.cpp`:
- Around line 81-93: The onUpdate routing only handles controls[j].value and
always reapplies; update it to handle controls[j].default as well and avoid
no-op reapplications: in NodeManager::onUpdate, change the controls branch to
trigger for (updatedItem.name == "value" || updatedItem.name == "default") when
updatedItem.parent[1] == "controls", obtain JsonVariant control =
nodeState["controls"][updatedItem.index[1]] and if control[updatedItem.name] ==
updatedItem.value return early; otherwise call the appropriate handler (either
reuse handleNodeControlValueChange for "value" and add/introduce
handleNodeControlDefaultChange for "default", or merge into a single
handleNodeControlChange that accepts the property name) and apply the same
early-equality check logic to the similar control-handling block referenced
around lines 220-229 so both places skip no-op updates.
- Around line 179-183: Remove the explicit base-destructor call oldNode->~Node()
and let freeMBObject(oldNode) perform destruction to avoid bypassing virtual
dispatch and double-destruction; ensure Node has a virtual destructor so derived
destructors run when freeMBObject is invoked and simply call
freeMBObject(oldNode) in place of the explicit destructor call.
In `@src/MoonBase/Nodes.cpp`:
- Around line 73-80: updateControl currently uses a hardcoded 32-byte limit when
copying values for "text" and "selectFile" if `max` is unset, risking overflow;
change the copy logic in Node::updateControl to consult the metadata
`control["size"]` (set by setupControl) as the preferred limit, falling back to
`sizeofVar` and then the current 32-byte default only if `control["size"]` is
missing, and ensure the copy respects that computed limit and null-terminates
the destination. Locate the handling for control types "text" and "selectFile"
in updateControl and replace the fixed 32-byte behavior with using
`control["size"]` (and still honor `max` when present) so buffers sized smaller
than 32 bytes are not overflowed.
In `@src/MoonBase/Nodes.h`:
- Around line 167-169: The sharedData variable is declared with static in the
header (SharedData sharedData) which gives it internal linkage per translation
unit; change the header to declare it extern (extern SharedData sharedData;) and
remove the static definition from the header, then add a single definition in a
.cpp (e.g., in Nodes.cpp define SharedData sharedData;) so that SharedData and
sharedData (the struct type and the global instance) are a single process-wide
instance accessible to all nodes.
---
Nitpick comments:
In `@lib/framework/WWWData.h`:
- Around line 4-5: The embedded SvelteKit asset blocks (e.g., the
ESP_SVELTEKIT_DATA_0[] array and the other asset-update hunks referenced) are
missing the required MoonLight marker; add the marker comment "// 💫"
immediately next to each upstream SvelteKit asset-update comment/block (for
example next to the ESP_SVELTEKIT_DATA_0[] declaration and the other indicated
asset sections) so they clearly carry the MoonLight tag used by the codebase
guidelines.
In `@platformio.ini`:
- Line 321: The lib_deps entry currently allows any future major release of
ArduinoJson which can break native builds; update the ArduinoJson version
specifier in platformio.ini (the lib_deps line referencing ArduinoJson) to a
major-bounded range such as pinning to major version 7 (for example use a caret
or explicit upper bound like "ArduinoJson@>=7.0.0,<8.0.0" or
"ArduinoJson@^7.0.0") so CI and native builds remain deterministic.
In `@src/MoonBase/Module.h`:
- Around line 22-35: Add a small native unit test that includes Module.h,
constructs an UpdatedItem with its default constructor, and asserts the sentinel
defaults: both parent elements are empty strings (parent[0] and parent[1]
compare equal to ""), and both index elements equal UINT8_MAX; keep the test
focused, header-only/pure (no side effects), and name it to reflect UpdatedItem
defaults so CI will catch regressions to the UpdatedItem() constructor behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
interface/src/routes/moonbase/monitor/Monitor.sveltelib/framework/WWWData.hplatformio.inisrc/MoonBase/DriverNode.cppsrc/MoonBase/DriverNode.hsrc/MoonBase/LiveScriptNode.cppsrc/MoonBase/LiveScriptNode.hsrc/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/NodeManager.cppsrc/MoonBase/NodeManager.hsrc/MoonBase/Nodes.cppsrc/MoonBase/Nodes.htest/test_native/test_module.cpp
🔥 Files not summarized due to errors (1)
- lib/framework/WWWData.h: Error: Server error: no LLM provider could handle the message
|
|
||
| if (brightness != brightnessSaved || layerP.maxPower != maxPowerSaved) { | ||
| // Use FastLED for setMaxPowerInMilliWatts stuff, don't use if more then 8096 LEDs, decent power is assumed then! Also in case of Art-Net to HUB75 panels this calculation is not using the right mW per LED | ||
| uint8_t correctedBrightness = layerP.lights.header.nrOfLights > 8096 ? brightness : calculate_max_brightness_for_power_mW((CRGB*)&layerP.lights.channelsD, layerP.lights.header.nrOfLights, brightness, layerP.maxPower * 1000); // calculate_max_brightness_for_power_mW supports max 65K LEDs |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd DriverNode.cpp -t fRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
# Read context around line 49 in DriverNode.cpp
head -70 src/MoonBase/DriverNode.cpp | tail -35Repository: MoonModules/MoonLight
Length of output: 2568
🏁 Script executed:
# Check structure of lights and header
rg "struct.*{" src/MoonBase/DriverNode.cpp -A 20 | head -50Repository: MoonModules/MoonLight
Length of output: 146
🏁 Script executed:
# Check for channelsPerLight references and guards
rg "channelsPerLight" src/MoonBase/ -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 6605
🏁 Script executed:
# Find other CRGB casts on channel buffers
rg "\(CRGB\*\)" src/MoonBase/DriverNode.cpp -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 805
Guard FastLED power estimation against RGBW (4-byte) buffers.
Line 49 casts raw channel data to CRGB*, which assumes 3-byte pixel stride. When channelsPerLight=4 (RGBW presets), this misinterprets the buffer layout and corrupts power calculations. Gate the call to calculate_max_brightness_for_power_mW() to run only when channelsPerLight==3 and nrOfLights<=8096; otherwise return raw brightness.
Patch
- uint8_t correctedBrightness = layerP.lights.header.nrOfLights > 8096 ? brightness : calculate_max_brightness_for_power_mW((CRGB*)&layerP.lights.channelsD, layerP.lights.header.nrOfLights, brightness, layerP.maxPower * 1000); // calculate_max_brightness_for_power_mW supports max 65K LEDs
+ bool canUseFastLedPowerCalc = (header->channelsPerLight == 3) && (layerP.lights.header.nrOfLights <= 8096);
+ uint8_t correctedBrightness = canUseFastLedPowerCalc
+ ? calculate_max_brightness_for_power_mW(reinterpret_cast<CRGB*>(layerP.lights.channelsD),
+ layerP.lights.header.nrOfLights,
+ brightness,
+ layerP.maxPower * 1000)
+ : brightness;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/DriverNode.cpp` at line 49, The power-estimation call casts raw
channel data to CRGB* which assumes 3-byte pixels and breaks for RGBW; modify
the correctedBrightness assignment so you only call
calculate_max_brightness_for_power_mW when layerP.lights.header.channelsPerLight
== 3 and layerP.lights.header.nrOfLights <= 8096, otherwise set
correctedBrightness to the raw brightness value; reference the existing
correctedBrightness variable, layerP.lights.header.nrOfLights,
layerP.lights.header.channelsPerLight, and calculate_max_brightness_for_power_mW
to locate and guard the call.
Comment on lines
+27
to
+33
| #if defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2) | ||
| static PhysicalDriverESP32S3 ledsDriver; // sizeof(driver) = 1080K ! | ||
| #elif CONFIG_IDF_TARGET_ESP32 | ||
| static PhysicalDriverESP32D0 ledsDriver; // sizeof(driver) = 1080K ! | ||
| #else | ||
| static LedsDriver ledsDriver; // only the core driver, for setBrightness and setColorCorrection and LUT | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the DriverNode files
fd "DriverNode\.(h|cpp)" --type fRepository: MoonModules/MoonLight
Length of output: 118
🏁 Script executed:
# Check the overall file structure in src/MoonBase
git ls-files src/MoonBase/ | head -20Repository: MoonModules/MoonLight
Length of output: 673
🏁 Script executed:
# Read DriverNode.h around lines 27-33
head -40 src/MoonBase/DriverNode.h | tail -20Repository: MoonModules/MoonLight
Length of output: 1087
🏁 Script executed:
# Check the full file size first
wc -l src/MoonBase/DriverNode.hRepository: MoonModules/MoonLight
Length of output: 93
🏁 Script executed:
# Read DriverNode.h entirely (check size first)
cat -n src/MoonBase/DriverNode.hRepository: MoonModules/MoonLight
Length of output: 4580
🏁 Script executed:
# Read DriverNode.cpp to check for ledsDriver definitions
cat -n src/MoonBase/DriverNode.cpp | head -80Repository: MoonModules/MoonLight
Length of output: 4982
🏁 Script executed:
# Search for ledsDriver definitions in DriverNode.cpp
rg "ledsDriver" src/MoonBase/DriverNode.cpp -B2 -A2Repository: MoonModules/MoonLight
Length of output: 3201
Do not instantiate ledsDriver as static in a header.
Lines 27-33 create one driver object per translation unit (massive memory duplication and divergent state across TUs). Declare extern in the header and define once in DriverNode.cpp. Note: the HP_ALL_DRIVERS path (line 23) is already correct; the ESP32_LEDSDRIVER variants need the same fix.
Header-side fix
- `#if` defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2)
-static PhysicalDriverESP32S3 ledsDriver; // sizeof(driver) = 1080K !
- `#elif` CONFIG_IDF_TARGET_ESP32
-static PhysicalDriverESP32D0 ledsDriver; // sizeof(driver) = 1080K !
- `#else`
-static LedsDriver ledsDriver; // only the core driver, for setBrightness and setColorCorrection and LUT
- `#endif`
+ `#if` defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2)
+extern PhysicalDriverESP32S3 ledsDriver;
+ `#elif` defined(CONFIG_IDF_TARGET_ESP32)
+extern PhysicalDriverESP32D0 ledsDriver;
+ `#else`
+extern LedsDriver ledsDriver;
+ `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/DriverNode.h` around lines 27 - 33, The header currently defines
a static ledsDriver instance per TU (symbols: ledsDriver with types
PhysicalDriverESP32S3, PhysicalDriverESP32D0, or LedsDriver); remove the static
definitions from the header and replace them with an extern declaration for
ledsDriver, then add a single definition in DriverNode.cpp that instantiates the
correct concrete type depending on the same CONFIG_IDF_TARGET_* macros; keep the
HP_ALL_DRIVERS path as-is and ensure the implementation file includes the header
so all TUs share the one global ledsDriver instance.
Comment on lines
+22
to
+39
| Node* gNode = nullptr; | ||
|
|
||
| static void _addControl(uint8_t* var, char* name, char* type, uint8_t min = 0, uint8_t max = UINT8_MAX) { | ||
| EXT_LOGV(ML_TAG, "%s %s %d (%d-%d)", name, type, var, min, max); | ||
| gNode->addControl(*var, name, type, min, max); | ||
| } | ||
| static void _nextPin() { layerP.nextPin(); } | ||
| static void _addLight(uint8_t x, uint8_t y, uint8_t z) { layerP.addLight({x, y, z}); } | ||
|
|
||
| static void _modifySize() { gNode->modifySize(); } | ||
| static void _modifyPosition(Coord3D& position) { gNode->modifyPosition(position); } // need &position parameter | ||
| // static void _modifyXYZ() {gNode->modifyXYZ();}//need &position parameter | ||
|
|
||
| void _fadeToBlackBy(uint8_t fadeValue) { gNode->layer->fadeToBlackBy(fadeValue); } | ||
| static void _setRGB(uint16_t indexV, CRGB color) { gNode->layer->setRGB(indexV, color); } | ||
| static void _setRGBPal(uint16_t indexV, uint8_t index, uint8_t brightness) { gNode->layer->setRGB(indexV, ColorFromPalette(PartyColors_p, index, brightness)); } | ||
| static void _setPan(uint16_t indexV, uint8_t value) { gNode->layer->setPan(indexV, value); } | ||
| static void _setTilt(uint16_t indexV, uint8_t value) { gNode->layer->setTilt(indexV, value); } |
There was a problem hiding this comment.
Global runtime state is unsafe with multiple LiveScript nodes.
gNode and WaitAnimationSync are shared globally, and Line 211 overwrites gNode per compile. With >1 LiveScript node, callbacks/sync can target the wrong instance.
Also applies to: 41-42, 157-158, 211-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/LiveScriptNode.cpp` around lines 22 - 39, The code uses the
global gNode (and global WaitAnimationSync) which gets overwritten when multiple
LiveScript nodes exist; replace the globals with per-instance state and
thread-safe callbacks: remove gNode and make the callbacks methods or lambda
closures that capture/receive a Node* (e.g., convert static functions like
_addControl, _modifyPosition, _fadeToBlackBy, _setRGB, _setPan/_setTilt and the
WaitAnimationSync usage to instance methods or register callbacks with a void*
userData carrying the Node*), or store the Node* inside the LiveScriptNode
instance and pass it explicitly into any C-style callback registration so each
LiveScript node uses its own pointer instead of the global gNode; also make
WaitAnimationSync an instance member (or keyed by node instance) to avoid
cross-instance synchronization conflicts.
| addExternal("uint8_t inoise8(uint16_t,uint16_t,uint16_t)", (void*)(uint8_t (*)(uint16_t, uint16_t, uint16_t))inoise8); | ||
| addExternal("uint8_t beatsin8(uint16_t,uint8_t,uint8_t,uint32_t,uint8_t)", (void*)beatsin8); | ||
| addExternal("float hypot(float,float)", (void*)(float (*)(float, float))hypot); | ||
| addExternal("float beat8(uint8_t,uint32_t)", (void*)beat8); // saw wave |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/MoonBase/LiveScriptNode.cpp | sed -n '110,120p'Repository: MoonModules/MoonLight
Length of output: 877
🌐 Web query:
FastLED beat8 function C++ signature return type parameters
💡 Result:
beat8 (FastLED) C++ signature:
LIB8STATIC uint8_t beat8(accum88 beats_per_minute, uint32_t timebase = 0);- Return type:
uint8_t - Parameters:
accum88 beats_per_minute, optionaluint32_t timebase(default0) [1]
beat8 is registered with the wrong signature.
Line 115 declares beat8 as returning float, but FastLED beat8 returns uint8_t. Additionally, the first parameter is declared as uint8_t, but FastLED expects uint16_t (accum88). These mismatches will break runtime call/return handling in the script bridge.
Suggested fix
- addExternal("float beat8(uint8_t,uint32_t)", (void*)beat8); // saw wave
+ addExternal("uint8_t beat8(uint16_t,uint32_t)", (void*)(uint8_t (*)(uint16_t, uint32_t))beat8); // saw wave📝 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.
Suggested change
| addExternal("float beat8(uint8_t,uint32_t)", (void*)beat8); // saw wave | |
| addExternal("uint8_t beat8(uint16_t,uint32_t)", (void*)(uint8_t (*)(uint16_t, uint32_t))beat8); // saw wave |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/LiveScriptNode.cpp` at line 115, The addExternal registration
for beat8 has the wrong signature: update the registration call that references
addExternal("float beat8(uint8_t,uint32_t)", (void*)beat8) to match FastLED's
function type by changing the return type to uint8_t and the first parameter to
uint16_t (i.e., register as "uint8_t beat8(uint16_t,uint32_t)") so the
addExternal declaration matches the actual beat8 function prototype.
Comment on lines
+174
to
+178
| void LiveScriptNode::compileAndRun() { | ||
| // send UI spinner | ||
|
|
||
| // run the recompile not in httpd but in main loopTask (otherwise we run out of stack space) | ||
| // runInAppTask.push_back([&, animation, type, error] { |
There was a problem hiding this comment.
compileAndRun() is still synchronous on caller thread.
Line 177 says this should avoid httpd stack pressure, but the deferral wrapper is commented out, so parse/compile work still runs inline.
Based on learnings: ESP32-P4/httpd heavy onUpdate cascades can exhaust stack, and heavy work should be deferred to the SvelteKit loop.
Also applies to: 205-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/LiveScriptNode.cpp` around lines 174 - 178, The compileAndRun()
method is still executing parse/compile synchronously on the caller thread even
though the comment says it should be deferred; wrap the heavy work into a lambda
and push it onto the deferred task queue (the runInAppTask vector) instead of
running inline so parsing/compilation happens in the main SvelteKit loop, e.g.
create a capture-safe lambda that captures the needed locals (animation, type,
error) and this (or weak/ptr if required) and push_back that lambda to
runInAppTask, then return immediately from compileAndRun; apply the same change
for the similar synchronous block around lines 205-223 to ensure both compile
paths run in the deferred main-loop task.
Comment on lines
+81
to
+93
| void NodeManager::onUpdate(const UpdatedItem& updatedItem, const String& originId) { | ||
| // handle nodes | ||
| if (updatedItem.parent[0] == "nodes") { // onNodes | ||
| JsonVariant nodeState = _state.data["nodes"][updatedItem.index[0]]; | ||
|
|
||
| if (updatedItem.name == "name" && updatedItem.parent[1] == "") { // nodes[i].name | ||
| handleNodeNameChange(updatedItem, originId, nodeState); | ||
| } else if (updatedItem.name == "on" && updatedItem.parent[1] == "") { // nodes[i].on | ||
| handleNodeOnChange(updatedItem, originId, nodeState); | ||
| } else if (updatedItem.parent[1] == "controls" && updatedItem.name == "value" && updatedItem.index[1] < nodeState["controls"].size()) { // nodes[i].controls[j].value | ||
| handleNodeControlValueChange(updatedItem, originId, nodeState); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle controls.default too, and skip no-op control reapplications.
Current routing only handles value, and the handler always reapplies. That reintroduces spurious post-rename updates and unnecessary node callbacks.
Patch sketch
- } else if (updatedItem.parent[1] == "controls" && updatedItem.name == "value" && updatedItem.index[1] < nodeState["controls"].size()) { // nodes[i].controls[j].value
+ } else if (updatedItem.parent[1] == "controls" &&
+ (updatedItem.name == "value" || updatedItem.name == "default") &&
+ updatedItem.index[1] < nodeState["controls"].size()) { // nodes[i].controls[j].{value|default}
handleNodeControlValueChange(updatedItem, originId, nodeState);
} void NodeManager::handleNodeControlValueChange(const UpdatedItem& updatedItem, const String& originId, JsonVariant nodeState) {
JsonObject control = nodeState["controls"][updatedItem.index[1]];
+ if (control[updatedItem.name] == updatedItem.value) {
+ return; // avoid re-applying stale compareRecursive emissions
+ }Based on learnings: "Only value and default should drive nodeClass->updateControl/onUpdate; ... skip when the current control property already equals updatedItem.value."
Also applies to: 220-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/NodeManager.cpp` around lines 81 - 93, The onUpdate routing only
handles controls[j].value and always reapplies; update it to handle
controls[j].default as well and avoid no-op reapplications: in
NodeManager::onUpdate, change the controls branch to trigger for
(updatedItem.name == "value" || updatedItem.name == "default") when
updatedItem.parent[1] == "controls", obtain JsonVariant control =
nodeState["controls"][updatedItem.index[1]] and if control[updatedItem.name] ==
updatedItem.value return early; otherwise call the appropriate handler (either
reuse handleNodeControlValueChange for "value" and add/introduce
handleNodeControlDefaultChange for "default", or merge into a single
handleNodeControlChange that accepts the property name) and apply the same
early-equality check logic to the similar control-handling block referenced
around lines 220-229 so both places skip no-op updates.
Comment on lines
+179
to
+183
| EXT_LOGD(ML_TAG, "remove oldNode: %d p:%p", nodes->size(), oldNode); | ||
| oldNode->~Node(); | ||
|
|
||
| freeMBObject(oldNode); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify allocation/destruction contract for Node instances
echo "=== Searching for addNode calls ==="
rg -n -C4 '\baddNode\s*\(' src/MoonBase
echo -e "\n=== Searching for custom memory functions ==="
rg -n -C4 '\bnewMBObject\b|\bfreeMBObject\b|\bdestroyMBObject\b' src
echo -e "\n=== Searching for explicit destructor calls ==="
rg -n -C4 'oldNode->~Node|delete\s+oldNode' src/MoonBaseRepository: MoonModules/MoonLight
Length of output: 3109
🏁 Script executed:
#!/bin/bash
# Find Node class definition and derived types
echo "=== Searching for Node class definition ==="
rg -n 'class Node' src --type h
echo -e "\n=== Searching for derived Node types ==="
rg -n ':\s*public.*Node|:\s*private.*Node' src --type h
echo -e "\n=== Checking NodeManager.cpp context around line 179 ==="
sed -n '170,190p' src/MoonBase/NodeManager.cppRepository: MoonModules/MoonLight
Length of output: 11390
🏁 Script executed:
#!/bin/bash
# Examine the memory management implementation
echo "=== Looking for freeMBObject and newMBObject implementations ==="
rg -n -B2 -A8 'freeMBObject|newMBObject' src --type h | head -100
echo -e "\n=== Check if there's a custom destructor in Node hierarchy ==="
rg -n '~Node\(\)' src --type h -A3Repository: MoonModules/MoonLight
Length of output: 822
Remove the explicit base-destructor call before freeMBObject().
oldNode->~Node() at line 180 bypasses virtual dispatch, preventing derived destructors from executing their cleanup. This violates the resource cleanup contract for nodes. Additionally, calling the destructor explicitly and then via freeMBObject(oldNode) causes double deletion. Let freeMBObject() handle all destruction through the proper virtual mechanism.
Current code
EXT_LOGD(ML_TAG, "remove oldNode: %d p:%p", nodes->size(), oldNode);
oldNode->~Node();
freeMBObject(oldNode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/NodeManager.cpp` around lines 179 - 183, Remove the explicit
base-destructor call oldNode->~Node() and let freeMBObject(oldNode) perform
destruction to avoid bypassing virtual dispatch and double-destruction; ensure
Node has a virtual destructor so derived destructors run when freeMBObject is
invoked and simply call freeMBObject(oldNode) in place of the explicit
destructor call.
Comment on lines
+73
to
+80
| // set size based on control type and sizeCode | ||
| if (control["type"] == "slider" || control["type"] == "select" || control["type"] == "pin" || control["type"] == "number") { | ||
| if (sizeCode == 8 || sizeCode == 108 || sizeCode == 16 || sizeCode == 32 || sizeCode == 33 || sizeCode == 34) | ||
| control["size"] = sizeCode; | ||
| else | ||
| EXT_LOGE(ML_TAG, "size %d mismatch for %s", sizeCode, name); | ||
| } else if (control["type"] == "selectFile" || control["type"] == "text") { | ||
| control["size"] = sizeofVar; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the full context
fd -type f -name "Nodes.cpp" -path "*/MoonBase/*"Repository: MoonModules/MoonLight
Length of output: 235
🏁 Script executed:
# Get the full content of Nodes.cpp to understand the setupControl and updateControl methods
wc -l src/MoonBase/Nodes.cppRepository: MoonModules/MoonLight
Length of output: 91
🏁 Script executed:
# Read the Nodes.cpp file to understand the context
head -100 src/MoonBase/Nodes.cppRepository: MoonModules/MoonLight
Length of output: 3277
🏁 Script executed:
# Look at lines 73-80 specifically and surrounding context
sed -n '60,100p' src/MoonBase/Nodes.cppRepository: MoonModules/MoonLight
Length of output: 1587
🏁 Script executed:
# Search for updateControl method implementation
rg "Node::updateControl|void updateControl" -A 30 src/MoonBase/Repository: MoonModules/MoonLight
Length of output: 4588
🏁 Script executed:
# Search for strlcpy usage related to size
rg "strlcpy.*max\|bufSize\|maxLen" -B 5 -A 5 src/MoonBase/Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Get the complete updateControl method
sed -n '105,160p' src/MoonBase/Nodes.cppRepository: MoonModules/MoonLight
Length of output: 3220
🏁 Script executed:
# Search for text or selectFile handling in updateControl
rg -i "text|selectfile" -B 3 -A 10 src/MoonBase/Nodes.cppRepository: MoonModules/MoonLight
Length of output: 1312
🏁 Script executed:
# Search for strlcpy usage anywhere in the file
rg "strlcpy" src/MoonBase/Repository: MoonModules/MoonLight
Length of output: 1129
Use control size metadata when copying text/selectFile values to prevent buffer overflow.
setupControl() stores control["size"] for text/selectFile controls, but updateControl() ignores it and falls back to a hardcoded 32 bytes when max is unset. This causes overflow if the actual buffer is smaller than 32 bytes.
Fix in `Node::updateControl` (lines ~145–156)
} else if (control["type"] == "selectFile" || control["type"] == "text") {
char* valuePointer = (char*)pointer;
- size_t maxLen = control["max"].isNull() ? 32 : control["max"].as<size_t>();
+ size_t bufSize = control["size"].isNull() ? 32 : control["size"].as<size_t>();
+ if (!control["max"].isNull()) {
+ // `max` is content length; reserve one byte for '\0'
+ bufSize = MIN(bufSize, control["max"].as<size_t>() + 1);
+ }
const char* src = control["value"].as<const char*>();
- size_t bufSize = maxLen > 0 ? maxLen : 0;
if (bufSize > 0 && src) {
strlcpy(valuePointer, src, bufSize);
} else {
valuePointer[0] = '\0';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Nodes.cpp` around lines 73 - 80, updateControl currently uses a
hardcoded 32-byte limit when copying values for "text" and "selectFile" if `max`
is unset, risking overflow; change the copy logic in Node::updateControl to
consult the metadata `control["size"]` (set by setupControl) as the preferred
limit, falling back to `sizeofVar` and then the current 32-byte default only if
`control["size"]` is missing, and ensure the copy respects that computed limit
and null-terminates the destination. Locate the handling for control types
"text" and "selectFile" in updateControl and replace the fixed 32-byte behavior
with using `control["size"]` (and still honor `max` when present) so buffers
sized smaller than 32 bytes are not overflowed.
Comment on lines
+167
to
169
| /// Data shared between nodes (audio sync, status info, gravity, etc.). | ||
| /// Single static instance accessible by all effect/driver nodes. | ||
| static struct SharedData { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++ namespace scope static variable in header file translation unit linkage semantics
💡 Result:
In C++, a namespace-scope (i.e., “global”) variable defined in a header like:
// header.hpp
static int x = 42;has the following linkage / translation-unit semantics:
- Internal linkage:
staticat namespace scope gives the name internal linkage, meaning it is visible only within the current translation unit (TU). (en.cppreference.com) - One instance per TU: because the header is included into each TU that uses it, each TU gets its own separate
x(separate storage, separate address, separate state). This usually avoids “multiple definition” linker errors precisely because the definitions are not for the same linked entity. (en.cppreference.com) - Static storage duration: despite the keyword, at namespace scope the variable still has static storage duration (exists for the lifetime of the program);
statichere is about linkage, not lifetime. (en.cppreference.com)
Common consequences / gotchas
- If multiple
.cppfiles include the header, they will not share state throughx; modifyingxin one TU does not affect the others (because they’re different variables). (stackoverflow.com)
If you actually want “one shared variable program-wide”
Use one of these patterns instead:
- C++17+:
inlinevariable in the header (one entity program-wide, ODR-friendly).(// header.hpp inline int x = 42;
inlinevariables are designed to allow header definitions across TUs under ODR rules.) (blog.miguens.one) - Pre-C++17 (or still common):
externdeclaration in the header + single definition in one.cpp. (stackoverflow.com)
Citations:
- 1: https://en.cppreference.com/w/cpp/language/storage_duration.html?utm_source=openai
- 2: https://en.cppreference.com/w/cpp/language/storage_duration.html?utm_source=openai
- 3: https://en.cppreference.com/w/cpp/language/storage_duration.html?utm_source=openai
- 4: https://stackoverflow.com/questions/7847030/why-cant-a-non-static-variable-reside-in-a-header-file?utm_source=openai
- 5: https://blog.miguens.one/posts/2025/06/inline-variables-in-c-17-odr-safe-header-definitions/?utm_source=openai
- 6: https://stackoverflow.com/questions/7847030/why-cant-a-non-static-variable-reside-in-a-header-file?utm_source=openai
🏁 Script executed:
# Search for all references to sharedData in the codebase
rg -i "sharedData" --type cpp --type h -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 48943
static in header creates per-translation-unit instances, not a process-wide shared variable.
Line 169 declares sharedData as static at namespace scope in a header file. In C++, this gives the variable internal linkage, meaning each translation unit that includes the header gets its own separate instance. The comment states "Single static instance accessible by all effect/driver nodes," but the current implementation contradicts this—if these nodes are compiled separately or linked from different translation units, they will not see the same sharedData object.
To achieve true single-instance sharing across all nodes, move the definition to a .cpp file and use extern in the header:
Suggested fix
-static struct SharedData {
+struct SharedData {
// ...
-} sharedData;
+};
+extern SharedData sharedData;// src/MoonBase/Nodes.cpp
SharedData sharedData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Nodes.h` around lines 167 - 169, The sharedData variable is
declared with static in the header (SharedData sharedData) which gives it
internal linkage per translation unit; change the header to declare it extern
(extern SharedData sharedData;) and remove the static definition from the
header, then add a single definition in a .cpp (e.g., in Nodes.cpp define
SharedData sharedData;) so that SharedData and sharedData (the struct type and
the global instance) are a single process-wide instance accessible to all nodes.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Module.h (1)
20-143:⚠️ Potential issue | 🟡 MinorAdd
// 🌙markers for new MoonBase upstream additions in this header.The newly added MoonBase code in this range is missing the required upstream marker comments.
As per coding guidelines: "Mark upstream sveltekit changes with comments:
// 🌙for MoonBase additions or// 💫for MoonLight additions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Module.h` around lines 20 - 143, The header contains new MoonBase upstream additions that need the upstream marker comment; add "// 🌙" comments immediately above each new MoonBase block (e.g., before the UpdatedItem struct, before the extern JsonDocument* gModulesDoc declaration, before the ModuleState class definition and before the Module class definition) so each newly introduced symbol (UpdatedItem, gModulesDoc, ModuleState, Module, and any newly added methods like compareRecursive, checkReOrderSwap, read, update, processUpdatedItem, addControlValue, updatePin) is clearly annotated with the MoonBase upstream marker.
♻️ Duplicate comments (1)
src/MoonBase/Module.cpp (1)
312-312:⚠️ Potential issue | 🟡 MinorUse
MB_TAGfor the MoonBase log inloop20ms.Line 312 still uses
ML_TAGin a MoonBase source file.Patch
- EXT_LOGD(ML_TAG, "requestUIUpdate %s", _moduleName); + EXT_LOGD(MB_TAG, "requestUIUpdate %s", _moduleName);As per coding guidelines: "Use tag constants
ML_TAGfor MoonLight logging andMB_TAGfor MoonBase logging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Module.cpp` at line 312, Replace the incorrect ML_TAG usage with MB_TAG in the MoonBase logging call: locate the EXT_LOGD invocation that logs the module name (EXT_LOGD(ML_TAG, "requestUIUpdate %s", _moduleName);) in Module.cpp (inside loop20ms) and change the first argument from ML_TAG to MB_TAG so the MoonBase log uses the correct tag constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/Module.cpp`:
- Around line 18-50: The new MoonBase-specific blocks need the upstream
SvelteKit marker comment added: insert the comment // 🌙 at the start of the
MoonBase-specific implementations in ModuleState::ModuleState (the branch that
creates gModulesDoc and the psram-specific allocation line) and in
ModuleState::~ModuleState (the block deleting data from gModulesDoc), and also
add the same // 🌙 marker to the additional MoonBase code region referenced
(lines around 297-361) so all MoonBase additions are clearly marked.
In `@src/MoonBase/Module.h`:
- Around line 40-41: gModulesDoc is a global JsonDocument shared across modules
and must be protected by a cross-module lock to prevent concurrent
allocator/state corruption; add a global mutex (e.g., gModulesDocMutex) in
Module.h and acquire this mutex around every access/mutation of gModulesDoc
(including reads/writes performed inside StatefulService transaction code paths
and module methods that touch the shared document, e.g., ModuleDevices or any
module's state serialization/deserialization routines), and update
StatefulService transaction code to lock gModulesDocMutex for the duration of
transactions that touch the shared JsonDocument to ensure only one module
mutates the document at a time.
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Line 148: The migrated socket access call using
_sveltekit->getSocket()->getActiveClients() is missing the required MoonBase
SvelteKit marker; update the expression in ModuleDevices (where
_sveltekit->getSocket()->getActiveClients() is used) by adding the comment
marker // 🌙 on that line to indicate a MoonBase upstream SvelteKit change (keep
the existing logic unchanged, only append the marker comment to the line).
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 518-521: The socket emit calls for the MoonLight SvelteKit
migration are missing the required upstream marker; after the emitEvent calls
that use _sveltekit->getSocket()->emitEvent (the header send using
(char*)&layerP.lights.header with headerPrimeNumber and the channels send using
(char*)layerP.lights.channelsE with MIN(...) and _moduleName), add the MoonLight
marker comment "// 💫" at the end of each relevant line (also apply the same
addition for the similar emitEvent lines around layerP/lights at the later
533-534 location) so the changes are annotated per the coding guideline.
In `@src/MoonLight/Modules/ModuleLiveScripts.h`:
- Line 149: This change is missing the MoonLight SvelteKit marker: locate the
conditional using _sveltekit->getSocket()->getConnectedClients() in
ModuleLiveScripts (e.g., inside ModuleLiveScripts.h where the if statement
appears) and add the required MoonLight marker comment `// 💫` immediately
adjacent to that upstream SvelteKit socket migration line so the file follows
the `// 💫` convention for MoonLight edits.
---
Outside diff comments:
In `@src/MoonBase/Module.h`:
- Around line 20-143: The header contains new MoonBase upstream additions that
need the upstream marker comment; add "// 🌙" comments immediately above each
new MoonBase block (e.g., before the UpdatedItem struct, before the extern
JsonDocument* gModulesDoc declaration, before the ModuleState class definition
and before the Module class definition) so each newly introduced symbol
(UpdatedItem, gModulesDoc, ModuleState, Module, and any newly added methods like
compareRecursive, checkReOrderSwap, read, update, processUpdatedItem,
addControlValue, updatePin) is clearly annotated with the MoonBase upstream
marker.
---
Duplicate comments:
In `@src/MoonBase/Module.cpp`:
- Line 312: Replace the incorrect ML_TAG usage with MB_TAG in the MoonBase
logging call: locate the EXT_LOGD invocation that logs the module name
(EXT_LOGD(ML_TAG, "requestUIUpdate %s", _moduleName);) in Module.cpp (inside
loop20ms) and change the first argument from ML_TAG to MB_TAG so the MoonBase
log uses the correct tag constant.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Modules/ModuleTasks.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Modules/ModuleLiveScripts.h
💤 Files with no reviewable changes (1)
- src/MoonBase/Modules/ModuleIO.h
Comment on lines
+18
to
+50
| ModuleState::ModuleState() { | ||
| EXT_LOGD(MB_TAG, "ModuleState constructor"); | ||
|
|
||
| if (!gModulesDoc) { | ||
| EXT_LOGD(MB_TAG, "Creating doc"); | ||
| if (psramFound()) | ||
| gModulesDoc = new JsonDocument(JsonRAMAllocator::instance()); // crashed on non psram esp32-d0 | ||
| else | ||
| gModulesDoc = new JsonDocument(); | ||
| } | ||
| if (gModulesDoc) { | ||
| data = gModulesDoc->add<JsonObject>(); | ||
| } else { | ||
| EXT_LOGE(MB_TAG, "Failed to create doc"); | ||
| } | ||
| } | ||
|
|
||
| ModuleState::~ModuleState() { | ||
| EXT_LOGD(MB_TAG, "ModuleState destructor"); | ||
|
|
||
| // delete data from doc | ||
| if (gModulesDoc) { | ||
| JsonArray arr = gModulesDoc->as<JsonArray>(); | ||
| for (size_t i = 0; i < arr.size(); i++) { | ||
| JsonObject obj = arr[i]; | ||
| if (obj == data) { // same object (identity check) | ||
| EXT_LOGD(MB_TAG, "Deleting data from doc"); | ||
| arr.remove(i); | ||
| break; // optional, if only one match | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Mark new MoonBase implementation blocks with // 🌙.
These newly added MoonBase code paths are missing required upstream SvelteKit marker comments.
As per coding guidelines: "Mark upstream sveltekit changes with comments: // 🌙 for MoonBase additions or // 💫 for MoonLight additions".
Also applies to: 297-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Module.cpp` around lines 18 - 50, The new MoonBase-specific
blocks need the upstream SvelteKit marker comment added: insert the comment //
🌙 at the start of the MoonBase-specific implementations in
ModuleState::ModuleState (the branch that creates gModulesDoc and the
psram-specific allocation line) and in ModuleState::~ModuleState (the block
deleting data from gModulesDoc), and also add the same // 🌙 marker to the
additional MoonBase code region referenced (lines around 297-361) so all
MoonBase additions are clearly marked.
Comment on lines
+40
to
41
| /// Shared ArduinoJson document for all modules (saves RAM vs one doc per module). | ||
| extern JsonDocument* gModulesDoc; // shared document for all modules, to save RAM |
There was a problem hiding this comment.
Guard shared gModulesDoc with a cross-module lock.
Line 40 makes module state storage global, but StatefulService transaction locking is per module instance. Concurrent updates from different modules can mutate the same JsonDocument at once, which risks allocator/state corruption.
Based on learnings: "ModuleDevices deadlock caused by ... race conditions and JSON corruption."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Module.h` around lines 40 - 41, gModulesDoc is a global
JsonDocument shared across modules and must be protected by a cross-module lock
to prevent concurrent allocator/state corruption; add a global mutex (e.g.,
gModulesDocMutex) in Module.h and acquire this mutex around every
access/mutation of gModulesDoc (including reads/writes performed inside
StatefulService transaction code paths and module methods that touch the shared
document, e.g., ModuleDevices or any module's state
serialization/deserialization routines), and update StatefulService transaction
code to lock gModulesDocMutex for the duration of transactions that touch the
shared JsonDocument to ensure only one module mutates the document at a time.
| // set the doc | ||
| JsonDocument doc; | ||
| if (_socket->getActiveClients()) { // rebuild the devices array | ||
| if (_sveltekit->getSocket()->getActiveClients()) { // rebuild the devices array |
There was a problem hiding this comment.
Add the required MoonBase SvelteKit marker on this migrated socket access.
Line 148 switches to _sveltekit->getSocket() but is missing the required // 🌙 marker for upstream SvelteKit changes.
Suggested patch
- if (_sveltekit->getSocket()->getActiveClients()) { // rebuild the devices array
+ if (_sveltekit->getSocket()->getActiveClients()) { // 🌙 rebuild the devices arrayAs per coding guidelines: **/*.{cpp,h,hpp}: Mark upstream sveltekit changes with comments: // 🌙 for MoonBase additions or // 💫 for MoonLight additions.
📝 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.
Suggested change
| if (_sveltekit->getSocket()->getActiveClients()) { // rebuild the devices array | |
| if (_sveltekit->getSocket()->getActiveClients()) { // 🌙 rebuild the devices array |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonBase/Modules/ModuleDevices.h` at line 148, The migrated socket access
call using _sveltekit->getSocket()->getActiveClients() is missing the required
MoonBase SvelteKit marker; update the expression in ModuleDevices (where
_sveltekit->getSocket()->getActiveClients() is used) by adding the comment
marker // 🌙 on that line to indicate a MoonBase upstream SvelteKit change (keep
the existing logic unchanged, only append the marker comment to the line).
Comment on lines
+518
to
+521
| if (_sveltekit->getSocket()->getActiveClients() && _state.data["monitorOn"]) { | ||
| static_assert(sizeof(LightsHeader) > headerPrimeNumber, "LightsHeader size nog large enough for Monitor protocol"); | ||
| _socket->emitEvent("monitor", (char*)&layerP.lights.header, headerPrimeNumber, _moduleName); // send headerPrimeNumber bytes so Monitor.svelte can recognize this | ||
| _socket->emitEvent("monitor", (char*)layerP.lights.channelsE, MIN(layerP.lights.header.nrOfLights * 3, layerP.lights.maxChannels), _moduleName); //*3 is for 3 bytes position | ||
| _sveltekit->getSocket()->emitEvent("monitor", (char*)&layerP.lights.header, headerPrimeNumber, _moduleName); // send headerPrimeNumber bytes so Monitor.svelte can recognize this | ||
| _sveltekit->getSocket()->emitEvent("monitor", (char*)layerP.lights.channelsE, MIN(layerP.lights.header.nrOfLights * 3, layerP.lights.maxChannels), _moduleName); //*3 is for 3 bytes position |
There was a problem hiding this comment.
Please annotate these MoonLight SvelteKit socket migrations with // 💫.
The updated socket access in Lines 518-521 and Lines 533-534 follows the refactor correctly, but the required MoonLight upstream marker is missing.
Suggested patch
- if (_sveltekit->getSocket()->getActiveClients() && _state.data["monitorOn"]) {
+ if (_sveltekit->getSocket()->getActiveClients() && _state.data["monitorOn"]) { // 💫
...
- if (_sveltekit->getSocket()->getActiveClients() && _state.data["monitorOn"]) {
+ if (_sveltekit->getSocket()->getActiveClients() && _state.data["monitorOn"]) { // 💫As per coding guidelines: **/*.{cpp,h,hpp}: Mark upstream sveltekit changes with comments: // 🌙 for MoonBase additions or // 💫 for MoonLight additions.
Also applies to: 533-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Modules/ModuleLightsControl.h` around lines 518 - 521, The
socket emit calls for the MoonLight SvelteKit migration are missing the required
upstream marker; after the emitEvent calls that use
_sveltekit->getSocket()->emitEvent (the header send using
(char*)&layerP.lights.header with headerPrimeNumber and the channels send using
(char*)layerP.lights.channelsE with MIN(...) and _moduleName), add the MoonLight
marker comment "// 💫" at the end of each relevant line (also apply the same
addition for the similar emitEvent lines around layerP/lights at the later
533-534 location) so the changes are annotated per the coding guideline.
| // update scripts / read only values in the UI | ||
| void loop1s() override { | ||
| if (!_socket->getConnectedClients()) return; | ||
| if (!_sveltekit->getSocket()->getConnectedClients()) return; |
There was a problem hiding this comment.
Add the required MoonLight SvelteKit marker comment.
Line 149 is an upstream SvelteKit socket migration but is missing the // 💫 marker required for MoonLight files.
Suggested patch
- if (!_sveltekit->getSocket()->getConnectedClients()) return;
+ if (!_sveltekit->getSocket()->getConnectedClients()) return; // 💫As per coding guidelines: **/*.{cpp,h,hpp}: Mark upstream sveltekit changes with comments: // 🌙 for MoonBase additions or // 💫 for MoonLight additions.
📝 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.
Suggested change
| if (!_sveltekit->getSocket()->getConnectedClients()) return; | |
| if (!_sveltekit->getSocket()->getConnectedClients()) return; // 💫 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Modules/ModuleLiveScripts.h` at line 149, This change is
missing the MoonLight SvelteKit marker: locate the conditional using
_sveltekit->getSocket()->getConnectedClients() in ModuleLiveScripts (e.g.,
inside ModuleLiveScripts.h where the if statement appears) and add the required
MoonLight marker comment `// 💫` immediately adjacent to that upstream SvelteKit
socket migration line so the file follows the `// 💫` convention for MoonLight
edits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests