MoonLive: scripts on the filesystem, and a compiler that sizes itself to them - #65
Open
ewowi wants to merge 2 commits into
Open
MoonLive: scripts on the filesystem, and a compiler that sizes itself to them#65ewowi wants to merge 2 commits into
ewowi wants to merge 2 commits into
Conversation
A scripted module carried its script as a fixed 1 KB array, plus a second copy to notice edits — resident whether or not a script was loaded, so six modules held ~16 KB of a classic ESP32's 320 KB for text that was mostly empty. The script now lives in a file; the module holds its name, reads it into a right-sized buffer to compile, and frees it. Scripts are bounded by the filesystem instead of by an array nobody can grow. Performance: desktop 132 us/tick (7575 fps), esp32 2151 us/tick (464 fps). Light domain - A `script` control (~32 B) replaces the `source` textarea in all three bindings. The UI loads, edits and saves the file through the /api/file endpoints that already existed, so this needed no new backend surface. A fresh module reports "no script — set the script name" and renders nothing, rather than every new module compiling the same default. - The rebuild check is a 4-byte FNV-1a of the script text, not a second copy of it. It only ever answered "did this change". - Per-binding control-name pools are gone: the engine owns the names it publishes now, so three private copies of the same fact went with them. - /moonlive/ is created on demand — the write endpoint does not make parent directories, so a first save on a fresh device failed with nowhere obvious to look. Core - The engine copies declared control NAMES out of the source before returning. They pointed into the source text, which the caller is now free to release the moment compile() ends — and does. A control briefly appeared named "\x05" before this was found. - IrProgram's op array is heap-allocated and sized from a token count, RAII-owned (destructor frees, copy deleted). It was a ~2 KB stack member on a 12 KB main task, the same cost for a one-statement script as a full one — so growing it would have traded a compile limit for a stack overflow. SEVEN sequential statements used to fail; forty compile. kMaxIrOps 64 → 4096 is now a sanity bound, not the working limit. - Widening that count to uint16_t left four uint8_t loop counters iterating over it — three lowerers and IrProgram::hasInline — which wrapped at 256 ops and spun forever. On a device that is a watchdog reset from a script that merely got long. Bisected (60 statements fine, 80 hung); the regression test HANGS when the fix is reverted, which is how it was checked. - ParlioLedDriver asks the platform for its 65535-byte transfer cap rather than naming the number in the light domain, and an over-capacity frame reports the ceiling in lights per pin on both the reinit and tick paths — the KB figure was the one a user could not act on. Tests - A shared fixture writes each script to a file, so tests exercise the path that ships. It is thread-local: the concurrency test compiles from two threads, and a shared name buffer had them compiling each other's script. - Tests that relied on a built-in default script now name one. There is no default any more. Docs/CI - MIGRATING: `source` is gone, so a persisted script is an unknown key and ignored — the entry says where to find the text (/.config/Layouts.json as "N.source") and how to restore it. - The three module specs, and the plan's step 1 marked done with what actually shipped. Verified on the desktop: a 16x12 scripted grid layout with a scripted lines effect, both compiled from files written over the API, surviving a restart and reloading from persistence. Not yet run on hardware — the boards were unreachable; that is next. Flash: esp32 1762368, esp32s3-n16r8 1752992, esp32s31 2025600, esp32p4-eth 1603952, desktop 1138184. Tests: 1326 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughMoonLive modules now persist script filenames under ChangesMoonLive filesystem scripts
Expandable IR and compilation limits
Parlio transfer-budget reporting
Supporting updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MoonLiveModule
participant MoonLiveScriptFile
participant FileSystem
participant MoonLive
MoonLiveModule->>MoonLiveScriptFile: compileScriptFile(script filename)
MoonLiveScriptFile->>FileSystem: read /moonlive script
FileSystem-->>MoonLiveScriptFile: source text and content hash
MoonLiveScriptFile->>MoonLive: compile temporary source buffer
MoonLive-->>MoonLiveModule: compiled controls or error status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/MIGRATING.md`:
- Around line 23-39: Update the older migration guidance for layout users so it
no longer instructs them to edit the removed source control. Direct them to edit
the corresponding .mlv file through the File Manager, then set the module’s
script control to that filename, consistent with the current filesystem-based
behavior described in the migration document.
In `@src/core/moonlive/MoonLiveIr.h`:
- Line 6: Remove the platform dependency from IrProgram in MoonLiveIr.h by
replacing direct platform::alloc()/platform::free() usage with an injected
core-neutral allocation interface, or relocating runtime allocation ownership
outside src/core. Ensure src/core contains no platform includes and that
disasm.py no longer needs to link the desktop platform implementation solely for
IR storage.
In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 1868-1872: Update reportOverCapacity() to calculate the maximum
light count using the same padded, 64-byte-aligned frame size as
frameBytesFor(), while treating a zero DMA budget as unbounded. Ensure the
reported limit cannot allow a frame exceeding the configured budget, and
preserve the existing one-report-per-geometry behavior at the call site.
In `@src/light/moonlive/MoonLiveEffect.h`:
- Around line 35-49: Update MoonLiveEffect::affectsPrepare() to check for the
"script" control instead of "source", ensuring script filename changes trigger
prepare and recompilation. Add a control-system test that changes the script
control and verifies prepare is invoked.
In `@src/light/moonlive/MoonLiveLayout.h`:
- Around line 118-133: Invalidate the cached compilation when the registered
script control changes, since controls_.addText() updates script_ without
invoking setScript(). Update the relevant MoonLiveLayout control/change handling
so compiledHash_ and engine state cannot satisfy the early-return check for a
new filename, while preserving setScript() behavior. Add a test that changes the
registered script control and verifies the layout recompiles and uses the new
file.
In `@src/light/moonlive/MoonLiveScriptFile.h`:
- Around line 47-50: Update the validation in MoonLiveScriptFile’s script-name
handling before constructing path to accept only a basename with the supported
.mlv suffix. Reject any name containing '/' or '\' and reject traversal
components such as ".."; preserve the existing missing-name error behavior, then
build the path only after validation.
- Around line 47-70: Add a MoonLive operation that invalidates the currently
compiled code without clearing the control arena, then invoke it and reset
hashOut to zero on every failure path before engine.compile() in
MoonLiveScriptFile loading. Cover invalid names, missing/empty/oversized files,
allocation failure, and read failure while preserving existing error messages
and successful compilation behavior.
In `@src/platform/platform.h`:
- Around line 1168-1172: Update the documentation for parlioMaxTransferBytes()
to state that a return value of 0 means no transfer bound, not zero usable
bytes, while positive values represent the hardware’s maximum single-transfer
ceiling. Keep the existing declaration and surrounding allocation guidance
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b3ecfe3-901d-4e75-a6e2-57e8911ac97a
📒 Files selected for processing (29)
docs/MIGRATING.mddocs/history/plans/Plan-20260809 - MoonLive scales — right-sized IR, and the stack as the register overflow.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/light/MoonLiveEffect.mddocs/moonmodules/light/MoonLiveLayout.mddocs/moonmodules/light/MoonLiveModifier.mdmoondeck/moonlive/disasm.pysrc/core/moonlive/MoonLive.cppsrc/core/moonlive/MoonLive.hsrc/core/moonlive/MoonLiveBuiltins.hsrc/core/moonlive/MoonLiveCompiler.cppsrc/core/moonlive/MoonLiveIr.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/ParlioLedDriver.hsrc/light/moonlive/MoonLiveEffect.hsrc/light/moonlive/MoonLiveLayout.hsrc/light/moonlive/MoonLiveModifier.hsrc/light/moonlive/MoonLiveScriptFile.hsrc/platform/desktop/moonlive_lower_host.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/moonlive_lower_riscv.cppsrc/platform/esp32/moonlive_lower_xtensa.cppsrc/platform/esp32/platform_esp32_parlio.cppsrc/platform/platform.htest/unit/core/unit_moonlive_compiler.cpptest/unit/light/MoonLiveScriptFixture.htest/unit/light/unit_MoonLiveLayout.cpptest/unit/light/unit_MoonLiveModifier.cpp
💤 Files with no reviewable changes (1)
- src/core/moonlive/MoonLiveBuiltins.h
| #include <cstdint> | ||
| #include <cstddef> | ||
| #include "core/moonlive/MoonLiveBuiltins.h" // InlineOp (a neutral opcode tag) | ||
| #include "platform/platform.h" // alloc/free — the op array is sized to the script |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep the core layer independent from the platform layer.
MoonLiveIr.h now imports platform/platform.h, and IrProgram calls platform::alloc() and platform::free(). This breaks the src/core/** boundary. Inject a core-neutral allocation interface, or move the allocation owner outside src/core. The dependency also forces moondeck/moonlive/disasm.py to link the desktop platform implementation.
As per path instructions: “src/core/** … Must be platform-independent — no platform includes.” Based on learnings: “inject a core-neutral executable-code placement interface into MoonLive or relocate the runtime placement layer outside src/core.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/moonlive/MoonLiveIr.h` at line 6, Remove the platform dependency
from IrProgram in MoonLiveIr.h by replacing direct
platform::alloc()/platform::free() usage with an injected core-neutral
allocation interface, or relocating runtime allocation ownership outside
src/core. Ensure src/core contains no platform includes and that disasm.py no
longer needs to link the desktop platform implementation solely for IR storage.
Sources: Coding guidelines, Path instructions, Learnings
Comment on lines
+47
to
+70
| if (!name || !name[0]) { err = "no script — set the script name"; return false; } | ||
|
|
||
| char path[96]; | ||
| std::snprintf(path, sizeof(path), "%s/%s", kScriptDir, name); | ||
|
|
||
| const long size = platform::fsSize(path); | ||
| if (size < 0) { err = "script not found"; return false; } | ||
| if (size == 0) { err = "script is empty"; return false; } | ||
| if (size > kScriptFileMax) { err = "script too large"; return false; } | ||
|
|
||
| // +1 for the NUL the lexer reads as End. fsRead null-terminates on success, but the buffer has | ||
| // to have room for it. | ||
| char* text = static_cast<char*>(platform::alloc(static_cast<size_t>(size) + 1)); | ||
| if (!text) { err = "no memory for the script"; return false; } | ||
|
|
||
| const int read = platform::fsRead(path, text, static_cast<size_t>(size) + 1); | ||
| if (read <= 0) { platform::free(text); err = "script could not be read"; return false; } | ||
|
|
||
| if (hashOut) *hashOut = scriptHash(text, static_cast<size_t>(read)); | ||
| const bool ok = engine.compile(text, builtins, sysvars); | ||
| if (!ok) err = engine.error(); | ||
| // Freed on BOTH paths, before returning: the text has done its job either way, and a failed | ||
| // compile is exactly when a device can least afford to leak. | ||
| platform::free(text); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate prior code when file loading fails.
These failure paths return before engine.compile() runs. An existing program therefore remains ok(): an effect keeps rendering, a layout keeps placing old coordinates, and a modifier keeps applying its old mapping while the status reports the new file error.
Add a MoonLive operation that drops code while preserving the control arena. Call it on every pre-compile file failure and reset hashOut to zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/moonlive/MoonLiveScriptFile.h` around lines 47 - 70, Add a MoonLive
operation that invalidates the currently compiled code without clearing the
control arena, then invoke it and reset hashOut to zero on every failure path
before engine.compile() in MoonLiveScriptFile loading. Cover invalid names,
missing/empty/oversized files, allocation failure, and read failure while
preserving existing error messages and successful compilation behavior.
Hardware found what 1228 tests did not: naming a script never recompiled anything. The
effect still asked whether the "source" control had changed - a control renamed to
"script" - and the layout cached its compiled program behind a hash that a control write
never cleared. Both held a new filename while running the previous script.
Performance: desktop 127 us/tick (7874 fps), esp32 2151 us/tick (464 fps).
Light domain
- MoonLiveEffect::affectsPrepare tests "script". Found on a P4: the effect showed the new
name and dyn=0, having compiled nothing. The unit tests call prepare() directly, so the
control-change path had no coverage at all — which is why they passed.
- MoonLiveLayout invalidates its compiled hash when the script control is written.
addText binds the buffer directly, so a control write never reached setScript() and
compile()'s early-return kept the old program. Pinned by a test that fails without it.
- A script name is a BASENAME ending in .mlv, rejected otherwise. It was pasted straight
into the path, so `../.config/NetworkModule.json` would have read the device's saved
credentials as a script. The fixed directory is the boundary; now it holds.
- reportOverCapacity counts down through frameBytesFor instead of dividing. The frame is
64-byte rounded, so the division overshot by one: it reported 898 lights per lane, whose
frame rounds to 65536 against a 65535 cap. A limit that still fails is worse than none.
Core
- MoonLive::compile's staging buffer and each assembler's buf_ are heap-allocated, RAII
owned, with every write and both branch patchers guarded against a failed allocation.
That is ~4.1 KB off a compile chain sharing a 12 KB task — the plan named this ("buf_
inside the assembler, itself a stack local") and step 1 had only done IrProgram, while
raising kCodeCap 768 → 2048 grew what remained.
Scripts/MoonDeck
- The monitor opens its serial port before probing the network. raised_log_level contacts
every device in moondeck.json at a 3 s timeout each; with a dozen registered and most
powered off, that was half a minute before the first byte — losing the boot output it was
pointed at.
Docs/CI
- MIGRATING no longer tells a layout user to edit the `source` control it just removed.
- The Parlio ceilings are the corrected 897/673/442/332, and platform.h says plainly that a
0 transfer cap means NO bound rather than zero bytes.
- Backlog: MoonLive compiling watchdogs a classic ESP32 — `rst:0x8 (TG1WDT_SYS_RESET)`,
captured on serial while adding one layout. Not a panic and not the stack overflow I first
chased: the compile simply takes longer than the 12 s task watchdog allows while the
render task waits. The stack work above did not change it. The entry records the measured
signature, the ruled-out theories, and to measure before assuming which part is slow.
Verified on the P4: layout 256 lights 16x16 (268 B) and effect (988 B), both compiled from
files written over the API. The classic still resets, now with the watchdog signature.
Flash: esp32 1715008, esp32s3-n16r8 1753792, esp32s31 2025600, esp32p4-eth 1603920,
desktop 1138376. Tests: 1328 cases.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/light/moonlive/MoonLiveScriptFile.h (1)
51-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvalidate compiled code on every script-loader failure.
A failed file load returns before
MoonLive::compile()callsfreeCode(). The old program remains executable while the module reports an error.
src/light/moonlive/MoonLiveScriptFile.h#L51-L83: callengine.freeCode()and set*hashOutto zero, when provided, before every pre-compile failure return.src/light/moonlive/MoonLiveEffect.h#L72-L77: ensure a failed script load leavesengine_.ok()false sotick()renders no prior program.src/light/moonlive/MoonLiveLayout.h#L125-L140: ensure a failed script load leavesengine_.ok()false solightCount()andforEachCoord()do not run prior coordinates.test/unit/light/unit_MoonLiveLayout.cpp#L458-L470: compile a valid script first, then select an invalid name and assert zero lights.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/light/moonlive/MoonLiveScriptFile.h` around lines 51 - 83, Invalidate compiled state on every script-load failure: in src/light/moonlive/MoonLiveScriptFile.h:51-83, before each pre-compile failure return, call engine.freeCode() and zero hashOut when provided. In src/light/moonlive/MoonLiveEffect.h:72-77 and src/light/moonlive/MoonLiveLayout.h:125-140, ensure failed loads leave engine_.ok() false so prior programs and coordinates are not used. In test/unit/light/unit_MoonLiveLayout.cpp:458-470, first compile a valid script, then select an invalid name and assert zero lights.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/backlog/backlog-light.md`:
- Around line 293-298: Update the MoonLive watchdog entry’s causal wording to
state only that the compile path did not return before the twelve-second
task-watchdog deadline. Remove or qualify claims that CPU compilation itself
exceeded twelve seconds, while preserving the listed LittleFS and
platform::alloc blocking possibilities and the recommendation to measure
compileScriptFile.
In `@moondeck/run/monitor_esp32.py`:
- Around line 103-113: Update the monitoring setup around the serial handle and
the raised_log_level/open(LOG_FILE, "w") context managers so ser.close() is
performed by an outer finally covering context setup and the monitoring body.
Remove the inner-only cleanup and preserve the existing serial error handling
and monitoring behavior.
In `@src/core/moonlive/MoonLive.cpp`:
- Around line 51-56: Remove the direct platform::alloc and platform::free calls
from the Staging helper in MoonLive. Introduce and inject a core-neutral
memory/code-placement interface into MoonLive for staging allocation and
release, or relocate the runtime placement ownership to the platform layer,
while preserving Staging’s lifetime management and validity check.
In `@test/scenarios/light/scenario_MoonLive_pipeline.json`:
- Line 61: Update the MoonLive pipeline scenario to create isolated
/moonlive/*.mlv file fixtures and set every module’s script control to the
corresponding filename before recording the baseline. Add equivalent
filesystem-fixture support to the in-process runner so the scenario executes
consistently there. Remove any source-based setup or compatibility coverage.
---
Duplicate comments:
In `@src/light/moonlive/MoonLiveScriptFile.h`:
- Around line 51-83: Invalidate compiled state on every script-load failure: in
src/light/moonlive/MoonLiveScriptFile.h:51-83, before each pre-compile failure
return, call engine.freeCode() and zero hashOut when provided. In
src/light/moonlive/MoonLiveEffect.h:72-77 and
src/light/moonlive/MoonLiveLayout.h:125-140, ensure failed loads leave
engine_.ok() false so prior programs and coordinates are not used. In
test/unit/light/unit_MoonLiveLayout.cpp:458-470, first compile a valid script,
then select an invalid name and assert zero lights.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 39efedd1-79fd-4d30-8927-a304870451e6
📒 Files selected for processing (21)
docs/MIGRATING.mddocs/backlog/backlog-light.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/performance.mdmoondeck/run/monitor_esp32.pysrc/core/moonlive/MoonLive.cppsrc/light/drivers/ParallelLedDriver.hsrc/light/moonlive/MoonLiveEffect.hsrc/light/moonlive/MoonLiveLayout.hsrc/light/moonlive/MoonLiveScriptFile.hsrc/platform/desktop/moonlive_asm_host.cppsrc/platform/desktop/moonlive_asm_host.hsrc/platform/esp32/moonlive_asm_riscv.cppsrc/platform/esp32/moonlive_asm_riscv.hsrc/platform/esp32/moonlive_asm_xtensa.cppsrc/platform/esp32/moonlive_asm_xtensa.hsrc/platform/platform.htest/scenarios/light/scenario_MoonLive_pipeline.jsontest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/unit/light/unit_MoonLiveLayout.cpp
Comment on lines
+293
to
+298
| - **MoonLive compiling watchdogs a classic ESP32** (2026-08-12). Naming a script on an Olimex Gateway resets the board with `rst:0x8 (TG1WDT_SYS_RESET)` — the TASK watchdog at 12 s, not a panic: there is no `Guru Meditation`, no backtrace, and the last serial lines are ordinary ticks. So the compile is not crashing, it is taking longer than twelve seconds with the render task waiting on it, and the watchdog does its job. Bench-captured on serial while adding one `MoonLiveLayout` with `grid.mlv`; the P4 compiles the same script in well under a second. | ||
|
|
||
| **Not a stack overflow** — that was the earlier theory and it was wrong. ~4.1 KB was moved off the compile chain (`MoonLive::compile`'s staging buffer and each assembler's `buf_`, both now heap, RAII-owned) which was worth doing on its own merits (the plan named it) but did not change this: the board still resets, now with the watchdog signature rather than `Double exception`. The earlier `Double exception` runs came from a board carrying persisted WiFi credentials, a separate issue. | ||
|
|
||
| **Where to look:** the classic's ticks already read ~9 ms with `renderWait` ~8 ms BEFORE any compile, so the render loop has almost no slack. Either the compile is genuinely that slow on a 240 MHz single-issue Xtensa with no PSRAM, or something in the path blocks (the LittleFS read, `platform::alloc` under a fragmented heap). Measure first — instrument `compileScriptFile` with timings and run it on the classic — before assuming which. Moving the compile off the render task is the likely fix, but it is a scheduling change and wants its own cycle. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate the watchdog observation from the unverified cause.
The evidence shows that the compile path did not return before the 12-second task-watchdog deadline. It does not prove that CPU compilation itself exceeded 12 seconds because Line 297 still lists LittleFS and platform::alloc blocking as alternatives. Replace the causal wording with “the compile path did not return before twelve seconds.”
As per coding guidelines, **/*.md: “Documentation must describe the system as it currently exists; specs precede implementation, and breaking changes must be recorded in `docs/MIGRATING.md`.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/backlog/backlog-light.md` around lines 293 - 298, Update the MoonLive
watchdog entry’s causal wording to state only that the compile path did not
return before the twelve-second task-watchdog deadline. Remove or qualify claims
that CPU compilation itself exceeded twelve seconds, while preserving the listed
LittleFS and platform::alloc blocking possibilities and the recommendation to
measure compileScriptFile.
Source: Coding guidelines
Comment on lines
+103
to
+113
| # OPEN THE PORT FIRST. raised_log_level contacts every device in moondeck.json over HTTP at a | ||
| # 3 s timeout each — with a dozen registered and most powered off, that is half a minute of | ||
| # blocking before a single byte is read, and the boot output you were monitoring FOR is already | ||
| # gone. The log level is a nicety; the serial stream is the point. | ||
| try: | ||
| ser = serial.Serial(args.port, args.baud, timeout=1) | ||
| except serial.SerialException as e: | ||
| print(f"Cannot open {args.port}: {e}") | ||
| sys.exit(1) | ||
|
|
||
| with raised_log_level(active_device_ips(), LOG_INFO): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make serial cleanup cover context setup.
ser opens at Line 108, but ser.close() is only reached from the inner finally at Lines 183-188. If active_device_ips(), raised_log_level.__enter__(), or open(LOG_FILE, "w") raises, the monitoring body is never entered and the serial handle remains open. Move the existing close into an outer finally that covers both context managers.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 113-113: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(LOG_FILE, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moondeck/run/monitor_esp32.py` around lines 103 - 113, Update the monitoring
setup around the serial handle and the raised_log_level/open(LOG_FILE, "w")
context managers so ser.close() is performed by an outer finally covering
context setup and the monitoring body. Remove the inner-only cleanup and
preserve the existing serial error handling and monitoring behavior.
Comment on lines
+51
to
+56
| namespace { | ||
| struct Staging { | ||
| uint8_t* p = static_cast<uint8_t*>(platform::alloc(kCodeCap)); | ||
| ~Staging() { platform::free(p); } | ||
| explicit operator bool() const { return p != nullptr; } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move memory ownership behind a core-neutral interface.
Lines 53-54 add direct platform::alloc() and platform::free() calls in src/core. This breaks the required core/platform boundary.
Inject a core-neutral compiler-memory and executable-code-placement interface into MoonLive, or move the runtime placement layer into src/platform.
As per path instructions, src/core/** must be platform-independent. Based on learnings, MoonLive requires a single core/platform-boundary change for executable-memory ownership.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/moonlive/MoonLive.cpp` around lines 51 - 56, Remove the direct
platform::alloc and platform::free calls from the Staging helper in MoonLive.
Introduce and inject a core-neutral memory/code-placement interface into
MoonLive for staging allocation and release, or relocate the runtime placement
ownership to the platform layer, while preserving Staging’s lifetime management
and validity check.
Sources: Path instructions, Learnings
| "desktop-macos": { | ||
| "tick_us": [ | ||
| 5, | ||
| 4, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scenario files ---'
git ls-files 'test/scenarios/light/scenario_MoonLive_pipeline.json' 'test/scenarios/light/*MoonLive*' 'src/core/**' 'src/platform/**' 'docs/**' | sed -n '1,160p'
printf '%s\n' '--- pipeline scenario relevant fields ---'
rg -n -C 4 '"source"|"script|filename|file|baseline|observed|at"|/moonlive|MoonLive' test/scenarios/light/scenario_MoonLive_pipeline.json || true
printf '%s\n' '--- changed-file summary ---'
git diff --stat
git diff -- test/scenarios/light/scenario_MoonLive_pipeline.json | sed -n '1,220p'
printf '%s\n' '--- source and script-name references ---'
rg -n -C 3 'key:\s*["'\'']source|["'\'']source["'\'']|script.?name|filename|/moonlive|MoonLive' src test docs CMakeLists.txt --glob '!**/build/**' | sed -n '1,320p'Repository: MoonModules/projectMM
Length of output: 42733
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MoonLive control declarations and file loading ---'
rg -n -C 5 'script|source|moonlive|readFile|writeFile|FileManager|set_control' src/core/moonlive src/light/moonlive src/core test/scenarios test --glob '!test/doctest.h' | sed -n '1,420p'
printf '%s\n' '--- scenario runner operations and filesystem support ---'
fd -i -t f 'run_scenario.py|scenario.*(runner|README)|README.*scenario' . | sed -n '1,120p'
rg -n -C 5 'add_module|set_control|write_file|read_file|filesystem|/moonlive|fixture|operation|op' . --glob '*.py' --glob '*.json' --glob '*.md' --glob '!docs/assets/**' | sed -n '1,420p'
printf '%s\n' '--- scenarios that use script controls or MoonLive files ---'
rg -l '"key"[[:space:]]*:[[:space:]]*"script"|"key"[[:space:]]*:[[:space:]]*"source"|/moonlive/' test/scenarios | sort | sed -n '1,200p'Repository: MoonModules/projectMM
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MoonLive-related files ---'
fd -i -t f 'moonlive|MoonLive|scenario' src test moondeck | sed -n '1,240p'
printf '%s\n' '--- script/source references outside compiler comments ---'
rg -n 'source|script|setScript|scriptName|filename|moonlive' src/light src/core/moonlive src/core/Control* src/core/*Module* test/scenario_runner.cpp moondeck/scenario/run_scenario.py --glob '*.{h,hpp,cpp,py}' | sed -n '1,300p'
printf '%s\n' '--- scenario runner implementation ---'
wc -l moondeck/scenario/run_scenario.py test/scenario_runner.cpp
sed -n '1,260p' moondeck/scenario/run_scenario.py
sed -n '1,300p' test/scenario_runner.cpp
printf '%s\n' '--- migration contract ---'
sed -n '20,40p' docs/MIGRATING.mdRepository: MoonModules/projectMM
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file-backed script implementation ---'
wc -l src/light/moonlive/MoonLiveScriptFile.h src/light/moonlive/MoonLiveEffect.h src/light/moonlive/MoonLiveLayout.h src/light/moonlive/MoonLiveModifier.h
cat -n src/light/moonlive/MoonLiveScriptFile.h
rg -n -C 8 'script|source|MoonLiveScriptFile|compile|read|open|file' src/light/moonlive/MoonLiveEffect.h src/light/moonlive/MoonLiveLayout.h src/light/moonlive/MoonLiveModifier.h
printf '%s\n' '--- scenario operations ---'
rg -n -C 10 'set_control|add_module|remove_module|fixture|op ==|strcmp\(.*op|write|file|filesystem' test/scenario_runner.cpp | sed -n '1,360p'
printf '%s\n' '--- exact MoonLive scenario controls ---'
for f in test/scenarios/light/scenario_MoonLiveEffect_controls.json \
test/scenarios/light/scenario_MoonLiveEffect_livescript.json \
test/scenarios/light/scenario_MoonLive_pipeline.json; do
echo "--- $f ---"
rg -n -C 3 '"key"|"value"|/moonlive|script|source|fixture' "$f" | sed -n '1,260p'
doneRepository: MoonModules/projectMM
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository script files ---'
git ls-files | awk 'tolower($0) ~ /(^|\/)moonlive(\/|$)|\.mlv$/' | sed -n '1,200p'
printf '%s\n' '--- scenario runner dispatch and fixture setup ---'
rg -n 'set_control|add_module|remove_module|fixture|steps|op\b|applyControl|setControl|fs' test/scenario_runner.cpp | sed -n '1,240p'
sed -n '500,760p' test/scenario_runner.cpp
sed -n '760,940p' test/scenario_runner.cpp
printf '%s\n' '--- script-file test setup and platform filesystem API ---'
rg -n -C 6 'compileScriptFile|fsMkdir|fsRead|fsWrite|/moonlive|setScript|script_' test/unit test/scenario_runner.cpp src/platform/platform.h src/platform/desktop/platform_desktop.cpp | sed -n '1,360p'Repository: MoonModules/projectMM
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scenario runner filesystem initialization ---'
rg -n -C 8 'fsSetRoot|fsMount|fsMkdir|fsWrite|fsRead|working|temp|filesystem' test/scenario_runner.cpp test/unit/light/MoonLiveScriptFixture.h CMakeLists.txt test/CMakeLists.txt 2>/dev/null || true
sed -n '1,220p' test/unit/light/MoonLiveScriptFixture.h
printf '%s\n' '--- desktop filesystem path mapping ---'
rg -n -C 8 'toFsPath|fsSetRoot|fsMount|fsMkdir|fsRead\(' src/platform/desktop/platform_desktop.cpp src/platform/platform.h | sed -n '1,280p'
printf '%s\n' '--- build/package handling for moonlive scripts ---'
rg -n -C 5 'moonlive/|\.mlv|random-pixel|grid\.mlv|copy.*moon|file.*moonlive' CMakeLists.txt cmake test moondeck moonlive docs --glob '!**/*.png' --glob '!**/*.gif' | sed -n '1,300p'
printf '%s\n' '--- current pipeline scenario complete control steps ---'
sed -n '80,270p' test/scenarios/light/scenario_MoonLive_pipeline.jsonRepository: MoonModules/projectMM
Length of output: 50379
Make the pipeline scenario use file-backed scripts.
source is ignored. Add isolated /moonlive/*.mlv fixture setup, then set each module’s script control to its filename before recording the baseline. The in-process runner currently has no filesystem fixture operation, so add equivalent runner support. Do not test source compatibility; it is intentionally removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/scenarios/light/scenario_MoonLive_pipeline.json` at line 61, Update the
MoonLive pipeline scenario to create isolated /moonlive/*.mlv file fixtures and
set every module’s script control to the corresponding filename before recording
the baseline. Add equivalent filesystem-fixture support to the in-process runner
so the scenario executes consistently there. Remove any source-based setup or
compatibility coverage.
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.
Two steps of the MoonLive scalability plan: the compiler stops paying a fixed price per script, and scripts stop living in RAM.
Scripts live on the filesystem
A scripted module carried its script as a fixed 1 KB array, plus a second 1 KB copy to notice edits, plus a name pool — resident whether or not a script was loaded. Six modules held ~16 KB of a classic ESP32's 320 KB for text that was mostly empty.
Now the module holds a name (~32 B). The script is read into a right-sized buffer to compile and freed immediately, so nothing script-sized stays in RAM, and a script is bounded by the filesystem rather than by an array nobody can grow.
The UI loads, edits and saves the file through the
/api/fileendpoints that already existed — this needed no new backend surface. The rebuild check became a 4-byte FNV-1a hash; it only ever answered "did this change".The 7-statement wall is gone
IrProgram's op array was a ~2 KB stack member on a 12 KB main task — the same cost for a one-statement script as a full one. Growing it would have traded a compile limit for a stack overflow (this project has lost a P4 to a large stack frame before). It is now heap-allocated, sized from a token count, and RAII-owned.Seven sequential statements used to fail; forty compile.
kMaxIrOps64 → 4096 is a sanity bound now, not the working limit.Three bugs, each caught by verification rather than by reading
uint16_twrap I introduced. Wideningcountleft fouruint8_tloop counters iterating over it — three lowerers andIrProgram::hasInline— which wrapped at 256 ops and spun forever. On a device that is a watchdog reset from a script that merely got long. Bisected (60 statements fine, 80 hung). The regression test hangs when the fix is reverted, which is the only reason it is worth having.DeclaredControl::namepointed into the source text, which the new loader frees as soon as compiling ends. It surfaced as a control literally named\x05. The engine now copies the names it publishes — which also made three per-binding name pools redundant./moonlive/did not exist on a fresh device, and the write endpoint does not create parent directories, so the first script save returned a 500.Also
ParlioLedDriverasks the platform for its 65535-byte transfer cap instead of naming the number in the light domain, and an over-capacity frame now reports the ceiling in lights per pin on both the reinit and tick paths — the KB figure was the one a user could not act on.Breaking
sourceis gone, so a persisted script is an unknown key and is ignored. A MoonLive module boots with no script and renders nothing until one is named. MIGRATING says where to find the old text (/.config/Layouts.jsonas"N.source") and how to restore it as a file.Verification
1326 tests, 20 scenarios inside their contracts, GCC build clean, all 10 gates green.
Desktop-verified end to end: a 16×12 scripted grid layout with a scripted lines effect, both compiled from files written over the API, surviving a restart and reloading from persistence.
Not yet run on hardware — the boards were unreachable while this was written. That is the next step, and it matters here: this changes how every scripted module loads, on the platform the work is specifically aimed at.
Known limits
lines.mlvwith z-planes still does not compile on any backend — three sweeps with a nested loop name more live values than 14 registers hold, verified with a 64 KB code buffer so it is the register ceiling, not code size. That is step 3 of the plan: spilling to the stack, on its own branch.Summary by CodeRabbit
New Features
/moonlive/and selected through ascriptcontrol.Bug Fixes
Documentation