Add ControlModule: presets on a control surface - #62
Open
ewowi wants to merge 3 commits into
Open
Conversation
Save the device's state as a named preset and bring it back with one click. A preset is a file, so it can be uploaded, downloaded and shared; each one records which parts of the setup it carries, so a look saved on one board applies to a board with different hardware. The presets sit on an 8x8 pad grid with encoders above and faders below, laid out like a Mackie control desk so a MIDI surface maps onto it later without a translation layer. Flash esp32s3-n16r8 1,651 KB (+22 KB), esp32s31 1,887 KB (+7 KB), desktop 960 KB (+36 KB); desktop tick 132 us (+5 us). Core: - ControlModule: a new top-level module, peer of Layouts/Layers/Drivers rather than a child of Services, since it reaches across them. Hosts presets now and external control (MIDI, IR) later. - FilesystemModule gains two seams: saveSubtreeTo writes a subtree into a caller's sink, applySubtree puts one back onto a LIVE tree. saveSubtree now calls the former, so there is exactly one serializer. - applySubtree guards on the prefix being present: without it applyNode reads "no children in JSON" as "delete every child", so a truncated preset would wipe the live look. - ListSource::persistsList: a list whose rows are re-derived at setup is no longer written to flash. The preset list was serialized on every save and discarded on load, since nothing restores it. - Preset names are validated as printable ASCII without / \ or . -- the name becomes a file name, and ESP32's fsTranslate does no path normalization (desktop's does), so an unguarded name could escape the preset folder on device via delete, rename or save. - Control.h: fader/encoder/faderTarget descriptor flags and the pad-grid ListSource hooks, all presentation-only and domain-neutral. Light domain: - Unchanged. ControlModule resolves subtrees generically through typeName(), so core carries no light-specific knowledge. UI: - Pad grid, rotary encoders and faders share one column track, so the three banks line up and still follow the pane as it is resized. - Pads are tinted by the roles they carry (layout/layer/driver/service), mixing hues when a preset carries several. Applying a preset claims only the roles it carries, so a layout preset and a layer preset stay lit at once. - Fixed: the seven-segment readouts and knob dials never redrew on a WebSocket patch (which fires neither input nor change), and were built before the input had a value or bounds. One redrawRangeDecorations call now owns the seam. - Power-on demo sweep, marked as one removable block plus a single call site. View-only: it never sends a value to the device. Tests: - 21 ControlModule tests, 7 FilesystemModule subtree tests. - Mutation-tested: removing the per-role rule fails 3 assertions, removing the path-traversal guard fails 7. - No scenario test for the preset round trip: the scenario runner has no op that can apply a preset (it speaks /api/control, applying needs /api/list/). Extending the runner is separate work. Docs/CI: - docs/moonmodules/core/control.md: catalog card plus the rules no header owns (what a preset carries, one-active-preset-per-role, applying is a rebuild). Reviews: - Path traversal via preset name -> fixed, validator on the control so every write path runs it; pinned and mutation-tested. - Preset list persisted then discarded -> fixed at the core seam (ListSource::persistsList) rather than locally, so Pins/Tasks can use it. - Save wrote the file without its slot then moved it, a second whole-folder rewrite that could displace an unrelated preset -> fixed by writing the slot up front. Exposed a real bug: an unaimed save landed on pad 1; added kNoSlot. - Duplicated fader binding -> driveFader now parses faderTarget, so the popup and the action cannot disagree. - Stale `order` naming in three comments -> renamed to `slot`. - "Apply runs on the HTTP thread, not the render tick" -> not applied. HttpServerModule::tick20ms is MM_NONBLOCKING and drains inside Scheduler::tick, so the docstring is correct. - 192-byte header read, insertion-sort struct copies -> deferred to the dynamic presets rework, along with kMaxPresets 36->64 (accepted by the PO for now). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 23
🤖 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 `@src/core/Control.h`:
- Around line 398-409: Update addText and addTextArea to use designated
initializers for the Control descriptors, explicitly naming the relevant members
such as var, name, type, bufSize, and validate. Remove the positional
false/nullptr values and rely on default initialization for unused aggregate
fields, while preserving each method’s existing behavior.
In `@src/core/ControlModule.h`:
- Around line 522-534: Update applyPreset around applySubtree and prepareTree to
persist the applied preset by marking each mutated module dirty and calling
FilesystemModule::noteDirty(). After the batch completes, trigger the existing
MoonModule schema-changed signal/hook so HttpServerModule performs
requestFullResync(), without coupling ControlModule directly to
HttpServerModule; preserve the current status reporting and return behavior.
- Around line 63-69: Update the grid-rendering comment above kGridCols,
kGridRows, and kMaxPresets to describe all kMaxPresets cells, or otherwise
reference kGridCols * kGridRows instead of the stale literal 36.
- Around line 109-110: Make the “slot” control non-persistable so saveSlot_
remains transient and kNoSlot is never written to or restored from flash, using
the existing transient-control mechanism. In savePreset, treat any value outside
the valid preset range as kNoSlot before selecting the target slot, preserving
assignFreeSlots for no-pad selections and preventing invalid API values from
becoming a real slot.
- Around line 537-555: Update renamePreset to detect whether the destination
preset already exists before calling fsWriteAtomic, and reject the operation
with the existing collision-reporting behavior instead of overwriting it.
Preserve the current rename flow for unused destination names, and follow the
collision handling principle already used by moveListRow.
- Around line 345-357: Update ControlModule::onEntry to skip preset files whose
stem length is at least sizeof(p.name), rather than truncating the name into
p.name. Only create a preset row when the complete filename stem fits,
preserving pathFor compatibility for all discovered entries.
- Around line 397-427: Update the slot persistence flow so a reorder writes only
presets whose slot values changed, rather than calling writeSlots for every
preset. In moveListRow, identify the moved preset and swapped occupant, then
persist each affected preset individually using the existing serialization and
atomic-write behavior; leave unchanged preset files untouched. Refactor
writeSlots or extract a single-preset helper as needed, preserving slot metadata
rewriting and cleanup.
In `@src/core/FilesystemModule.cpp`:
- Around line 350-357: The namespaced branch of FilesystemModule::saveSubtreeTo
must pass firstField=true to writeNode, matching the bare branch, because
savePreset already emits the sole separator before each subtree. In
src/core/ControlModule.h lines 467-475, retain the existing sink.append(",")
separator and add a strict JSON parsing test for saved preset output; no
separator change is needed there.
In `@src/ui/app.js`:
- Around line 2316-2338: Update the knob drag handlers around the pointerdown
listener so the existing up teardown also runs for pointercancel and
lostpointercapture. Ensure all termination paths remove the pointermove
listener, clear knob-turning, release capture when applicable, unregister the
end listeners, and dispatch the final change event only once.
- Around line 1505-1535: Extract the duplicated target-popup construction and
contextmenu/long-press listener setup from the encoder and fader branches into a
shared helper near this control-building logic. Have the helper accept the input
and control name/target context, then call it from both branches while
preserving the existing popup text and event behavior.
- Around line 2436-2459: Update the empty-cell creation logic in the
fixed/item-null branch to use a button element instead of a div, preserving its
existing drop-target behavior and styling. Add a primary click handler plus
keyboard activation for Enter and Space that call openPadEditor with the same
moduleName, ctrlName, null item, and slot index; retain context-menu and
long-press behavior as appropriate.
- Around line 2121-2141: Update the popup teardown around close, away, and the
document listeners so close() removes the popup and detaches both mousedown and
keydown listeners, matching away’s cleanup behavior. Ensure openPadEditor’s
save, overwrite, and delete paths use this shared teardown without leaving
listeners attached.
- Around line 2487-2493: The action payloads used by the pad click handler and
generic list button in src/ui/app.js (lines 2487-2493 and 2705-2720) must match
the corresponding test expectations in test/unit/core/unit_ControlModule.cpp.
Update both UI handlers and the tests to use the same payload, using "{}" if
that is the intended activate/apply body, while preserving the existing
listSetField flow.
- Around line 689-747: Add an early prefers-reduced-motion check at the
beginning of startSurfaceDemo, before checking or mutating surfaceDemoShownFor
or starting the animation, using the existing matchMedia browser API to return
immediately when reduced motion is requested. Preserve the current sweep
behavior for users who do not request reduced motion.
In `@src/ui/style.css`:
- Around line 940-947: Fix the Stylelint declaration-empty-line-before errors by
inserting a blank line before the padding declaration in .list-pad and before
the background declaration in .list-pad-active. Preserve all existing CSS values
and formatting otherwise.
- Around line 852-857: Update the .encoder-input styling to expose focus
feedback on its associated knob, using the existing :has() pattern because the
input follows the knob in the DOM. Add a visible focus-ring rule that activates
when the hidden encoder input is focused, while preserving its focusability and
current layout behavior.
- Around line 1665-1668: Consolidate the cursor declarations for the .knob
selector with its existing rules, removing the later duplicate cursor: grab
declaration so the intended ns-resize cursor is preserved. Keep the
.knob.knob-turning grabbing state, and group the drag/hover cursor styles with
the other .knob rules.
- Around line 1034-1055: Ensure the empty-cell hover styling in
.list-pad-empty:hover overrides the later generic .list-pad:hover rule by moving
it after the generic rule or increasing its specificity to
.list-pad.list-pad-empty:hover; preserve the quieter empty-cell background and
border colors.
In `@test/scenarios/light/scenario_peripheral_switch.json`:
- Line 257: Resolve the unsupported performance claim for the measure-i80-double
step by rerunning it alongside measure-i80-single on identical targets and runs,
then either add the appropriate supported performance or relative-bound
assertion or update the description near the measure-i80 scenario to state only
the non-freeze/normal-tick regression guard. Ensure the recorded values and
expectation are consistent.
In `@test/unit/core/unit_ControlModule.cpp`:
- Around line 37-62: Both fixtures derive temporary roots from
mm::platform::millis() and lack cleanup. In
test/unit/core/unit_ControlModule.cpp:37-62, update Device to use a monotonic
counter for unique roots and add a destructor that deletes the module tree and
removes the root directory; apply the same changes to Tree in
test/unit/core/unit_FilesystemModule_subtree.cpp:40-62, or use a shared helper
for both fixtures.
- Around line 73-82: Update the comment above setText to state that it sets the
named control's text value only, without claiming that it fires the change hook;
preserve the implementation and note that hook invocation is handled separately
by the tests.
- Around line 214-236: Update the fixture in “ControlModule skips a capture this
build does not have” so the capture names a module type that no build registers,
rather than “Drivers,” which the fixture/device provides. Keep the existing
assertions and valid Layers subtree, ensuring applyPreset reaches the
missing-module !m branch while still applying NoiseEffect and reporting the
skipped capture.
In `@test/unit/core/unit_FilesystemModule_subtree.cpp`:
- Around line 189-194: Update the applySubtree calls in
unit_FilesystemModule_subtree.cpp, including the cases at lines 121, 142, 158,
175, 189, 194, and 219, to assert their returned bool according to whether each
body is expected to be accepted or rejected. Preserve the existing tree-state
assertions while making corrupt and empty-body cases explicitly verify the
rejection result, following the corresponding ControlModule test.
🪄 Autofix (Beta)
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: a5f03ff1-34f5-469d-9dd5-0c0ae42db909
⛔ Files ignored due to path filters (1)
docs/assets/core/ControlModule.pngis excluded by!**/*.png
📒 Files selected for processing (28)
docs/history/plans/Plan-20260731 - ControlModule and presets.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmkdocs.ymlsrc/core/Control.cppsrc/core/Control.hsrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/main.cppsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_GridBlacks_blackpixel.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/scenarios/light/scenario_peripheral_switch.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_FilesystemModule_subtree.cpp
| "tick_us": [ | ||
| 4, | ||
| 15 | ||
| 31 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)scenario_peripheral_switch\.json$|test/scenarios|docs/moonmodules' || true
echo
echo "== file stats =="
wc -l test/scenarios/light/scenario_peripheral_switch.json
echo
echo "== relevant lines =="
sed -n '220,275p' test/scenarios/light/scenario_peripheral_switch.json | cat -n -v
echo
echo "== lines around 150-180 =="
sed -n '150,180p' test/scenarios/light/scenario_peripheral_switch.json | cat -n -v
echo
echo "== timing references =="
rg -n "tick_us|double-buffer|single|baseline|peripheral_switch\\)|observed\\.|at" test/scenarios/light/scenario_peripheral_switch.jsonRepository: MoonModules/projectMM
Length of output: 8932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all measure steps names =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for i, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and "name" in step:
print(f"{i}: {step['name']}: {step.get('description', '')[:120]}")
PY
echo
echo "== measure-i80-single and measure-i80-double observed values =="
python3 - <<'PY'
import json
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for idx, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and step.get("name") in ("measure-i80-single", "measure-i80-double"):
print(f"step {idx}: {step['name']}")
for platform, obs in step.get("observed", {}).items():
ticks.obs = obs.get("tick_us")
print(f" {platform}: tick_us={obs.get('tick_us')}, at={obs.get('at')}")
PY
echo
echo "== scenario runner handling for measure expectations =="
rg -n '"description": "i80 with double-buffer: the encode overlaps the wire, so the tick should be at or below the single-buffer baseline\."|tick should be at or below|baseline|measure": true|observed|run scenario|scenario runner|scenarios' test scenario test/scenarios --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' | head -200
echo
echo "== nearby moonmodules docs =="
sed -n '1,120p' docs/moonmodules/core/system.md | cat -n -v || trueRepository: MoonModules/projectMM
Length of output: 956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== measure-i80-single and measure-i80-double observed values =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path("test/scenarios/light/scenario_peripheral_switch.json")
tree = json.loads(p.read_text())
for idx, step in enumerate(tree.get("steps", []), 1):
if step.get("op") == "measure" and step.get("name") in ("measure-i80-single", "measure-i80-double"):
print(f"step {idx}: {step['name']}")
print(f" observed tick_us:")
for platform, obs in step.get("observed", {}).items():
print(f" {platform}: {obs.get('tick_us')}, at={obs.get('at')}")
PY
echo
echo "== benchmark invariant wording and nearby tests =="
rg -n "i80 with double-buffer: the encode overlaps the wire|tick should be at or below the single-buffer baseline|measure-i80-double|measure-i80-single" test docs --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' || true
echo
echo "== scenario runner handling for measure expectations =="
rg -n "scenario_peripheral_switch|measure-i80-double|baseline|tick should be at or below|run scenario|scenarios" scenario test test/scenarios docs --glob '*.ts' --glob '*.js' --glob '*.json' --glob '*.md' || trueRepository: MoonModules/projectMM
Length of output: 50377
Resolve the i80 double-buffer performance claim before publication.
measure-i80-double currently records 8,925 µs for esp32s3-n16r8 and 31 µs for desktop-macos, which are above the measure-i80-single baselines in this JSON. Rerun both steps on the same target and run; if this bound is required, add a supported performance contract or relative bound. If not, update the description at line 231 to match the non-freeze/normal-tick regression guard.
🤖 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_peripheral_switch.json` at line 257, Resolve
the unsupported performance claim for the measure-i80-double step by rerunning
it alongside measure-i80-single on identical targets and runs, then either add
the appropriate supported performance or relative-bound assertion or update the
description near the measure-i80 scenario to state only the
non-freeze/normal-tick regression guard. Ensure the recorded values and
expectation are consistent.
Sources: Coding guidelines, Learnings
A preset now captures exactly one thing: a look, or a geometry, or a hardware
setup, or a service configuration. Never a combination. Looks also reach Home
Assistant, where they appear in its own preset dropdown and can be applied from
the UI, a voice assistant or an automation.
Flash esp32s3-n16r8 1,656 KB (+6 KB), desktop 976 KB (+17 KB). Desktop tick reads
337 us but the sample was taken with an HTTP client attached; ControlModule has
no tick method and the hot-path gate passes.
Core:
- ControlModule: the four capture toggles become one `captures` Select, so the 16
representable combinations become 4 and the invalid ones are unrepresentable.
Defaults to Layers. Applying claims one role and leaves the other three, so a
layout preset and a look stay lit together.
- A preset file naming several subtrees (written by the previous build) is listed
but refused with a reason, so it can be seen and deleted rather than silently
vanishing.
- ControlModule stamps a revision whenever the preset set changes, exposed as the
WLED shim's `info.fs.pmt`. Home Assistant caches the preset list and re-fetches
only when that value moves; a constant left HA showing the list it read at setup
forever.
- Preset names are validated (printable ASCII, no / \ or .) — the name becomes a
file name, and ESP32's fsTranslate does no path normalization, so an unguarded
name could escape the preset folder via save, delete or rename.
- applySubtree marks the tree dirty: an applied preset rendered correctly and was
then lost on reboot, because the boot loader restored the config the apply never
updated.
- saveSubtreeTo passed firstField=false for a namespaced subtree while the caller
also emitted a separator, so every preset carrying more than one capture was
written as invalid JSON (",,"). Our own first-match reader tolerated it; a real
parser would not.
- ListSource::persistsList: a list whose rows are re-derived at setup is no longer
written to flash. The preset list was serialized on every save and discarded on
load.
- A preset filename longer than the name buffer was truncated, so pathFor then
addressed a different file — reachable by uploading through the File Manager.
Renaming onto an existing preset overwrote it and deleted the source.
- Save writes its slot into the file rather than fixing it up afterwards, which
removed a second whole-folder rewrite that could displace an unrelated preset.
Light domain:
- Drivers: `multicore` and `renderWait` are expert-only. Tuning knobs, not
settings.
UI:
- The capture checkboxes become a radio group; pad tint is a single role hue.
- Popup teardown detached only on click-away, so every save/delete leaked a
mousedown+keydown pair. A pointercancel left the knob turning after the gesture
ended. Empty pads were divs, so a keyboard user could not create a preset.
- The demo sweep respects prefers-reduced-motion and runs 1s rather than 3s.
Scripts/MoonDeck:
- run_desktop.py takes --port. Home Assistant's WLED integration connects on port
80 only (its host field rejects a port), so testing that path on desktop needs
`sudo uv run moondeck/run/run_desktop.py --port 80`.
- run_desktop.py picked the first executable path that existed, which served a
build a day older than `cmake --build build` produces; it now picks the newest.
Tests:
- 28 ControlModule tests. Three mutation-tested this session: the per-role rule,
the path-traversal guard, and the presets revision stamp.
- No scenario test for the preset round trip: the scenario runner speaks
/api/control only, and applying a preset needs /api/list/.
Docs/CI:
- control.md covers one-role presets and both Home Assistant paths, including
that the WLED integration is HA's native preset support while MQTT publishes the
same looks as effects (HA has no MQTT preset concept).
Reviews:
- CodeRabbit, 24 findings: fixed the invalid-JSON separator, the lost-on-reboot
apply, the filename truncation, the rename collision, the persisted-then-
discarded list, the slot clamp, the popup and pointer leaks, the keyboard
accessibility, four CSS ordering/duplication issues, and the designated
initializers. Declined one: the activate payload already matches (the UI passes
the value, the tests pass the body). Deferred the 192-byte header read and the
insertion-sort copies to the dynamic-presets rework.
- One finding exposed a vacuous test: "skips a capture this build does not have"
named a module the fixture provides, so it never reached the missing-module
branch.
SKIPPED GATE: "ESP32 firmware up to date" fails — no boards connected this
session, so the ESP32-affecting changes (HttpServerModule, MqttModule,
ControlModule, Drivers) are verified on desktop only. Needs a hardware check
before merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/core/ControlModule.h (2)
500-534: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA new save can land on an already-occupied pad, putting two presets on one grid cell.
savePreset()writessaveSlot_into the new preset's file (Line 521) without checking whether another preset already holds that slot.moveListRowexplicitly swaps to avoid this class of collision (Line 337: "swap rather than overwrite"), butsavePreset()has no equivalent guard.This is reachable without any UI action:
slot,name,captures, andsaveare ordinary hidden controls, and the class doc states they are settable "from the popup, the API and persistence." A client that setsslotto an occupied value and then triggerssavecreates a second file claiming the same grid cell.assignFreeSlots()only reassigns presets withhasSlot == false(Line 393), so it does not detect or resolve two presets that both already declare the sameslot.🐛 Proposed fix — refuse a save onto an occupied slot held by a different preset
+ if (saveSlot_ < kMaxPresets) { + for (uint8_t i = 0; i < presetCount_; i++) { + if (presets_[i].slot == saveSlot_ && std::strcmp(presets_[i].name, name_) != 0) { + setStatusf(Severity::Warning, "slot %u is occupied by %s", + static_cast<unsigned>(saveSlot_), presets_[i].name); + return; + } + } + } if (captureRole_ >= kCaptureCount) { setStatusf(Severity::Warning, "choose what to capture"); return; }🤖 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/ControlModule.h` around lines 500 - 534, Update savePreset() to check whether saveSlot_ is already assigned to a different existing preset before writing the new file. If the slot is occupied, refuse the save and report an appropriate status instead of persisting a duplicate slot; preserve the current behavior for unassigned slots and slots that are not selected.
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
kCaptureCountagainst drifting fromkCapturable/kCaptureRole.
kCaptureCountis a hand-maintained constant separate from thekCapturableandkCaptureRolearray literals. If either array ever grows without updatingkCaptureCount, every loop bounded bykCaptureCount(role lookup, capture serialization,writeListRow's role list) silently ignores the extra entries instead of failing to compile. The file already uses astatic_assertfor this exact class of risk at Lines 81-82 (kLayersRole).♻️ Proposed fix
static constexpr uint8_t kCaptureCount = 4; + static_assert(sizeof(kCapturable) / sizeof(kCapturable[0]) == kCaptureCount && + sizeof(kCaptureRole) / sizeof(kCaptureRole[0]) == kCaptureCount, + "kCaptureCount must match kCapturable/kCaptureRole length");🤖 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/ControlModule.h` around lines 73 - 78, Replace the hand-maintained kCaptureCount value with a compile-time size derived from kCapturable, and add a static_assert alongside the existing kLayersRole check to verify kCapturable and kCaptureRole have equal lengths. Keep the resulting count usable by the existing loops and role lookup code.src/core/FilesystemModule.cpp (2)
350-368: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the block comment to match the split between
saveSubtreeToandsaveSubtree.Lines 351-352 state "Returns true only when the file was written," but
saveSubtreeTo(Line 356) never touches a file — it writes into the caller'sJsonSinkand returnsfalseonly on an allocation failure (Line 366). The file-write contract belongs tosaveSubtree(Line 369). Leaving the two comments merged risks a future reader assumingsaveSubtreeTo's return value reflects a completed write.📝 Proposed fix
// ---- Save ---- -// Returns true only when the file was written. On failure (path/overflow/write -// error) the caller must keep the subtree dirty so the change isn't lost. // Serialize a subtree into a caller's sink. The write half of saveSubtree, split out so a caller // storing the bytes elsewhere (a named preset file) produces the SAME format the loader reads, // rather than a second serializer that could drift from this one. See the header. +// Returns false only on an allocation failure (sink.overflowed()); this function never touches a file. bool FilesystemModule::saveSubtreeTo(MoonModule* m, JsonSink& sink, const char* prefix) {🤖 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/FilesystemModule.cpp` around lines 350 - 368, Update the comment immediately before saveSubtreeTo to describe serializing into the caller-provided JsonSink and returning false only when the sink overflows; move the file-written/dirty-subtree contract to the saveSubtree comment near that method. Keep the existing format and loader-compatibility documentation attached to saveSubtreeTo.
196-223: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersistence is now fixed; the WS resync gap from the same past comment remains open.
applySubtreenow callsm->markDirty()andnoteDirty()(Lines 220-221), so an applied preset survives a reboot. This resolves the persistence half of the earlier "An applied preset is never persisted" finding.The other half of that same finding is not addressed here.
applyNode(called at Line 209) creates, replaces, and removes children to match the JSON — a structural mutation of the live tree. The past comment noted that every other structural mutator inHttpServerModule(applyAddModule,handleDeleteModule,handleReplaceModule,handleMoveModule) ends by callingrequestFullResync()so connected WS clients do not patch against a stale leaf-hash baseline.applySubtreeperforms the same class of mutation but has no equivalent signal here, and none ofControlModule.h's callers (applyPreset) add one either.#!/bin/bash # Confirm whether applySubtree's structural changes reach HttpServerModule's resync signal. set -euo pipefail rg -n 'requestFullResync|setSchemaChangedHook|onSchemaChanged' -C4 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/FilesystemModule.cpp` around lines 196 - 223, Update the applySubtree flow in FilesystemModule::applySubtree to notify HttpServerModule after applyNode performs structural subtree changes, using the existing requestFullResync or schema-change hook mechanism rather than adding a separate signaling path. Ensure preset callers such as ControlModule::applyPreset result in a full WS resync while preserving the existing markDirty and noteDirty persistence behavior.
♻️ Duplicate comments (2)
src/core/ControlModule.h (2)
464-494: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winA single pad drag still rewrites every preset file.
moveListRow(Line 338) still callswriteSlots(), which loops over allpresetCount_presets and performs a read, a heap allocation, and anfsWriteAtomicfor each one. This code is unchanged from the prior review round: only the moved preset and the swapped occupant actually changed slot, so a full-grid drag still costs up to 64 file rewrites (128 flash operations viafsWriteAtomic's temp-file-and-rename) on a cold path that already blocks the render tick.♻️ Proposed fix — write only the presets whose slot changed
- void writeSlots() { - for (uint8_t i = 0; i < presetCount_; i++) { - char path[128]; - pathFor(presets_[i].name, path, sizeof(path)); + void writeSlot(const Preset& p) { + char path[128]; + pathFor(p.name, path, sizeof(path)); const long size = platform::fsSize(path); - if (size <= 0) continue; + if (size <= 0) return; char* body = static_cast<char*>(platform::alloc(static_cast<size_t>(size) + 1)); - if (!body) continue; + if (!body) return; const int n = platform::fsRead(path, body, static_cast<size_t>(size) + 1); if (n > 0) { body[n] = '\0'; JsonSink sink; - sink.appendf("{\"slot\":%u,", static_cast<unsigned>(presets_[i].slot)); + sink.appendf("{\"slot\":%u,", static_cast<unsigned>(p.slot)); // … unchanged … } platform::free(body); - } }Then in
moveListRow, replace the whole-folder rewrite:const uint8_t from = moving->slot; moving->slot = to; if (occupant) occupant->slot = from; // swap rather than overwrite - writeSlots(); + writeSlot(*moving); + if (occupant) writeSlot(*occupant); sortBySlot();🤖 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/ControlModule.h` around lines 464 - 494, Update the moveListRow flow and writeSlots implementation so a reorder persists only presets whose slot value changed, rather than rewriting every preset file. Track the moved preset and swapped occupant, then invoke the existing file-writing logic only for those affected presets while preserving slot metadata cleanup and atomic writes.
598-610: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA rename can still overwrite an existing zero-byte preset file.
The collision check uses
platform::fsSize(dst) > 0(Line 607). If a preset file atdstexists but is empty (0 bytes), this check does not detect it as "already exists," and the subsequent write silently replaces it. The intent stated in the adjacent comment ("Never overwrite another preset") calls for detecting existence, not just non-empty content.🐛 Proposed fix
- if (platform::fsSize(dst) > 0) { + if (platform::fsSize(dst) >= 0) { setStatusf(Severity::Warning, "%s already exists", to); return false; }🤖 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/ControlModule.h` around lines 598 - 610, Update the collision check in renamePreset to detect whether the destination preset file exists, including zero-byte files, instead of relying on platform::fsSize(dst) > 0. Preserve the existing warning status and early return for any existing destination, so renaming never overwrites another preset.
🤖 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 `@moondeck/run/run_desktop.py`:
- Around line 45-52: Update the root-build candidate list in the executable
selection logic to include the Windows-suffixed path ROOT / "build" /
"projectMM.exe" alongside the existing unsuffixed candidate, so Windows builds
are considered when selecting the freshest executable.
In `@src/core/HttpServerModule.cpp`:
- Around line 1495-1497: Add a monotonically increasing preset-set revision to
ControlModule, incrementing it after every successful preset-set mutation,
including saves, deletes, and renames. Update the pmt assignment in the
HttpServerModule handler to report this revision instead of presetsModifiedS(),
while preserving the nonzero behavior. Add a regression test covering two
mutations performed within the same second.
In `@src/core/MqttModule.cpp`:
- Around line 77-90: Expose a monotonic preset revision from ControlModule and
increment it whenever presets are saved, renamed, or deleted, including the
existing rescan flow. In MqttModule::tick1s(), retain the last observed revision
and, when discovery is enabled, connected, and the revision changes, invoke
publishDiscovery(true) so buffers and retained discovery are refreshed. Add
coverage for live preset save, rename, and delete updates.
- Around line 826-834: Update publishState(false) to include a currentLook()
signature in its change-detection state alongside lastOn_, lastBri_, and
lastPalette_, so look-only changes publish updated ha/state effects. Capture the
look signature before the early-return check, and update it only after all MQTT
state publishes succeed; add a regression test applying two look-only presets
while Drivers values remain unchanged.
- Around line 186-204: Move the effect-list scratch storage out of
discoveryPayload_ and into non-overlapping storage such as discoveryBuf_ before
the final snprintf. Update the fxScratch capacity and writeHaEffectList call
accordingly, while keeping buildMqttPublish’s use of discoveryBuf_ safe by
ensuring the temporary effect data is consumed before that call.
In `@src/platform/desktop/main_desktop.cpp`:
- Around line 75-92: Update the argument parsing in main to use strtol’s end
pointer and reject any non-numeric trailing characters, while preserving
validation for ports outside 1..65535 and missing values. Reject unknown
arguments with an error instead of ignoring them, and add regression coverage
for valid, missing, non-numeric, trailing-character, and out-of-range --port
values.
In `@test/unit/core/unit_ControlModule.cpp`:
- Around line 806-826: Update the test case “ControlModule stamps a new revision
whenever the preset set changes” to replace both delayMs(1100) calls with
deterministic mm::platform::setTestNowMs() advances before each save/delete
expectation. Restore the test clock with setTestNowMs(0) after the case,
including on failure if the test framework supports cleanup.
---
Outside diff comments:
In `@src/core/ControlModule.h`:
- Around line 500-534: Update savePreset() to check whether saveSlot_ is already
assigned to a different existing preset before writing the new file. If the slot
is occupied, refuse the save and report an appropriate status instead of
persisting a duplicate slot; preserve the current behavior for unassigned slots
and slots that are not selected.
- Around line 73-78: Replace the hand-maintained kCaptureCount value with a
compile-time size derived from kCapturable, and add a static_assert alongside
the existing kLayersRole check to verify kCapturable and kCaptureRole have equal
lengths. Keep the resulting count usable by the existing loops and role lookup
code.
In `@src/core/FilesystemModule.cpp`:
- Around line 350-368: Update the comment immediately before saveSubtreeTo to
describe serializing into the caller-provided JsonSink and returning false only
when the sink overflows; move the file-written/dirty-subtree contract to the
saveSubtree comment near that method. Keep the existing format and
loader-compatibility documentation attached to saveSubtreeTo.
- Around line 196-223: Update the applySubtree flow in
FilesystemModule::applySubtree to notify HttpServerModule after applyNode
performs structural subtree changes, using the existing requestFullResync or
schema-change hook mechanism rather than adding a separate signaling path.
Ensure preset callers such as ControlModule::applyPreset result in a full WS
resync while preserving the existing markDirty and noteDirty persistence
behavior.
---
Duplicate comments:
In `@src/core/ControlModule.h`:
- Around line 464-494: Update the moveListRow flow and writeSlots implementation
so a reorder persists only presets whose slot value changed, rather than
rewriting every preset file. Track the moved preset and swapped occupant, then
invoke the existing file-writing logic only for those affected presets while
preserving slot metadata cleanup and atomic writes.
- Around line 598-610: Update the collision check in renamePreset to detect
whether the destination preset file exists, including zero-byte files, instead
of relying on platform::fsSize(dst) > 0. Preserve the existing warning status
and early return for any existing destination, so renaming never overwrites
another preset.
🪄 Autofix (Beta)
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: 71613d6f-210b-42bc-a616-20576761a37a
📒 Files selected for processing (18)
docs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/core/Control.hsrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/light/drivers/Drivers.hsrc/main.cppsrc/platform/desktop/main_desktop.cppsrc/ui/app.jssrc/ui/style.csstest/unit/core/unit_ControlModule.cpptest/unit/core/unit_FilesystemModule_subtree.cpp
Comment on lines
+1495
to
+1497
| unsigned pmt = 1; | ||
| if (auto* control = static_cast<ControlModule*>(findModuleByName("Control"))) | ||
| pmt = static_cast<unsigned>(control->presetsModifiedS()) + 1; // +1: never report 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a monotonic preset revision for pmt.
ControlModule::rescan() stores platform::millis() / 1000u. Two saves, deletes, or renames in the same second produce the same value. Home Assistant then keeps its previous /presets.json result.
Add a monotonically increasing preset-set revision in ControlModule. Increment it after each successful preset-set mutation. Report that revision here instead of the second-resolution timestamp. Add a regression test with two mutations in one second.
🤖 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/HttpServerModule.cpp` around lines 1495 - 1497, Add a monotonically
increasing preset-set revision to ControlModule, incrementing it after every
successful preset-set mutation, including saves, deletes, and renames. Update
the pmt assignment in the HttpServerModule handler to report this revision
instead of presetsModifiedS(), while preserving the nonzero behavior. Add a
regression test covering two mutations performed within the same second.
Home Assistant now sees a preset saved, renamed or deleted while it is connected, instead of keeping the list it read at setup. Preset pads refuse to overwrite each other, and a preset applied from HA reaches every open browser. Separately, the first power-function groundwork lands: a 16-bit math tier and a golden-frame harness that pins what the effects render today, so the coming migration can prove it changes nothing. Flash desktop 982 KB (+6 KB); desktop tick 125 us (the 337 us in the previous commit was sampled with an HTTP client attached, not a regression). Core: - ControlModule reports a monotonic preset revision instead of a seconds-resolution stamp: two changes inside one second were indistinguishable, so a consumer caching on it missed the second one. Drives both the WLED shim's info.fs.pmt and MQTT's re-announce. - MqttModule re-announces discovery when that revision moves, so a mid-session preset reaches Home Assistant without a reconnect; and the ha/state change gate now includes the applied look, which alters neither on, brightness nor palette and so never published. - The HA effect-list scratch moved out of discoveryPayload_: it sat at a fixed offset inside the buffer snprintf was writing, and the fixed prefix can grow past that offset and trample the list mid-format. - Saving a different preset onto an occupied pad is refused with the holder's name; saving over the same name is unchanged. Rename now treats a zero-byte destination as a collision (fsSize returns 0 for an existing empty file, -1 for a missing one). - A reorder rewrites only the presets whose slot changed, not every file. - applySubtree fires the existing schema-changed hook, so a preset applied with no HTTP request in flight (HA over MQTT or the WLED shim) still reaches open browsers. - ModuleFactory::registerType is idempotent by name. It never deduped, so each test fixture construction re-registered its types until the uint8_t capacity saturated and every later registration in the run failed. - New core/math16.h: the 16-bit contract tier for the power functions -- sin16/cos16, map32, and BeatPhase (the BPM accumulator nine effects hand-roll, which freezes when the frame time rounds to zero). sin16 uses a 130-byte quarter-wave table: interpolating the existing 8-bit LUT was implemented first and rejected on measurement at 1.1% error, worse than the 0.69% it was meant to beat; the table measures 0.031%. Light domain: - Drivers: multicore and renderWait are expert-only. Tuning knobs, not settings. Scripts/MoonDeck: - run_desktop.py takes --port (Home Assistant's WLED integration hardcodes port 80 and its host field rejects a port, so testing that path on desktop needs `sudo ... --port 80`), and now picks the newest executable rather than the first path that exists -- it was serving a build a day older than `cmake --build build` produces. - The desktop binary rejects a non-numeric or trailing-garbage --port and unknown arguments, instead of silently running on the default. Tests: - Golden-frame harness: renders an effect at a fixed clock and hashes the frame, so "renders exactly the same" is proved rather than asserted. Ten baselines captured from the current code and verified reproducible across runs; hashes, not frame blobs, so repo size stays flat. - unit_math16: sin16 smoothness between LUT entries (the property large fixtures need), the 0.5% accuracy bound, map32's fencepost, and BeatPhase under sub-millisecond frames and the millis wrap. - New MQTT rig covering live preset save and look-only state publishes. Docs/CI: - Power-function analysis, bottom-up and top-down: the primitive catalog with its prior art, and the build spec (homes, types, migration order, resource accounting, eleven showcase effects). A canon survey found one structural gap -- no way to read the framebuffer as a texture at a transformed coordinate, which is about a third of the classic effect canon. - architecture.md drops "concrete first, abstract later" (removed from CLAUDE.md earlier); the four backlog files citing it now stand on their own rationale. - ADRs are documented as immutable except the status line, so a superseded decision gets a dated pointer instead of the convention being folklore. Reviews: - CodeRabbit, 11 findings: fixed the scratch-buffer overlap, the revision resolution, the MQTT re-announce and state gate, the slot and rename collisions, the per-preset slot write, the resync hook, --port validation and the Windows path. The delayMs-based revision test was replaced by a counter, which made the suggested test-clock fix unnecessary. - Skipped: regression tests for --port parsing -- main() is not linkable into the unit binary, and the parsing is ten lines validated by inspection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/core/HttpServerModule.cpp (1)
1557-1567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccept standalone
pscommands on the WLED WebSocket path.
applyWledStatenow supportsps, butpollWledStateFromWebSockets()calls it only when the frame containsonorbri. A WLED client that sends{"ps":N}alone drops the preset request.Include
psin the WebSocket ingress predicate. Add a regression test for a masked WebSocket frame that contains onlyps.🤖 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/HttpServerModule.cpp` around lines 1557 - 1567, The WebSocket ingress predicate in pollWledStateFromWebSockets must invoke applyWledState for frames containing only ps, not just on or bri. Extend that predicate to recognize ps and add a regression test covering a masked WebSocket frame with a standalone ps command.src/core/ControlModule.h (3)
639-644: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report a rename after source deletion fails.
platform::fsRemove(src)is ignored. If it fails afterfsWriteAtomic(dst, ...)succeeds, both preset files remain but this method returnstrue. The HTTP list operation then reports success for a rename that created a copy.Check the source removal result. If it fails, remove the new destination when possible and return failure.
🤖 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/ControlModule.h` around lines 639 - 644, Update the rename flow around the source-removal call in ControlModule so it checks the result of platform::fsRemove(src) before setting ok to true. If source deletion fails after fsWriteAtomic succeeds, attempt to remove the newly created destination when possible, keep the operation failed, and preserve the existing cleanup and rescan behavior.
382-391: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRefresh the preset list after File Manager changes.
The preset grid is rebuilt only at setup and after ControlModule operations. A preset uploaded through the File Manager does not appear until another ControlModule operation or a reboot. This conflicts with the documented upload behavior.
src/core/ControlModule.h#L382-L391: add a core-neutral filesystem-change notification or an explicit live refresh path that callsrescan()after preset-folder uploads, deletes, and renames.docs/moonmodules/core/control.md#L27-L29: retain this statement only after the live refresh behavior exists. Otherwise document the required refresh step.🤖 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/ControlModule.h` around lines 382 - 391, The preset list must refresh immediately after File Manager uploads, deletes, and renames. Add a core-neutral filesystem-change notification or explicit live refresh path connected to ControlModule::rescan() for changes in the preset folder; update docs/moonmodules/core/control.md lines 27-29 to retain the documented behavior only if live refresh is implemented, otherwise document the required manual refresh step.
278-287: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep active-role state consistent with preset file mutations.
current_stores preset names. Deleting or renaming an active preset leaves its old name active. MQTT can then publish an effect that no longer exists, and WLED cannot resolve the active preset slot.
src/core/ControlModule.h#L278-L287: after a successful delete, clear everycurrent_entry equal to the deleted name.src/core/ControlModule.h#L619-L645: after a successful rename, replace everycurrent_entry equal tofromwithto.🤖 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/ControlModule.h` around lines 278 - 287, Keep active-role state synchronized with preset mutations in ControlModule: in deleteListRow, after a successful fsRemove, clear every current_ entry matching the deleted preset name; in the rename flow around lines 619-645, after a successful rename, replace every current_ entry matching from with to. Apply the required changes at both listed sites in src/core/ControlModule.h (278-287 and 619-645).moondeck/run/run_desktop.py (1)
49-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict executable candidates to the current host.
The resolver now selects the newest path from both
.exeand unsuffixed candidates. If a stale artifact from another host remains inbuild, the launcher can select an incompatible binary and fail to start or run the wrong build. Filter candidates by the host-specific suffix before callingmax(...).Proposed fix
- existing = [c for c in candidates if c.exists()] + suffix = ".exe" if sys.platform == "win32" else "" + existing = [ + c for c in candidates + if c.suffix == suffix and c.is_file() + ]🤖 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/run_desktop.py` around lines 49 - 53, Update the executable candidate selection in the resolver to retain only paths matching the current host’s executable suffix before evaluating existence and calling max. Preserve the newest-existing-candidate behavior while excluding incompatible .exe or unsuffixed artifacts from other hosts.
🤖 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/power-functions-analysis-bottom-up.md`:
- Line 3: Update the introductory paragraph in the power-functions analysis
document to remove the stale “to be written” wording and state that the top-down
companion already exists as the implementation specification, preserving the
surrounding description and link context.
- Line 138: Clarify the Stage 2 scope for fillTriangle across the candidate
table, gather-gap entry, filled-polygons cut line, and VectorBallsEffect
showcase reference. If triangles are included, list fillTriangle explicitly and
distinguish it from deferred general polygon fill; otherwise remove the showcase
reference and keep the deferred-scope statements consistent.
- Line 16: Reconcile the documented scope counts in the TL;DR, synthesis, and
repeated summary: update the stale “~30-function” and “eight families”
references to consistently reflect the defined ~34 functions across families
1–9, including Projection. Ensure all affected statements use the same totals.
In `@docs/backlog/power-functions-analysis-top-down.md`:
- Line 44: Update the particle API example’s code fence in the documentation to
specify the cpp language tag, changing the untyped opening fence to a cpp-tagged
fence so Markdownlint MD040 passes.
In `@src/core/ControlModule.h`:
- Around line 341-345: Update the reorder method surrounding writeSlot(*moving)
and writeSlot(*occupant) so writeSlot returns whether each file write persisted
successfully. Only increment presetsRevision_ and report success after every
affected write succeeds; on failure, restore the original in-memory slot
assignments and use a recoverable two-file swap strategy that avoids duplicate
slot claims after a partial write.
In `@src/core/math16.h`:
- Around line 82-88: Update map32 to widen operands before subtracting, avoiding
int32_t overflow, and replace the intermediate int64_t multiplication with an
overflow-safe multiply-divide approach that handles full-width 32-bit input and
output spans. Preserve clamping and zero-span behavior, and add regression
coverage for INT32_MIN/INT32_MAX combinations across input and output ranges;
invalid or unrepresentable cases must degrade visibly rather than crash.
- Around line 52-88: Mark the `sin16` and `map32` function declarations as
`constexpr` so their existing integer-only implementations can be evaluated at
compile time under C++20. Leave `cos16` unchanged since it already delegates to
`sin16`.
In `@test/unit/core/unit_math16.cpp`:
- Around line 13-14: Update unit_math16.cpp to include <algorithm> for std::max
and replace the implementation-defined M_PI usage with the portable C++20
std::numbers::pi_v<double> from <numbers> (or an equivalent local constexpr
value), including the repeated usage around the referenced lines.
In `@test/unit/core/unit_MqttModule.cpp`:
- Around line 311-333: Update PresetRig’s destructor to reset the platform
filesystem-root override before or while tearing down the fixture, then remove
root_. Ensure later tests no longer retain platform::fsSetRoot(root_) after
PresetRig ends.
---
Outside diff comments:
In `@moondeck/run/run_desktop.py`:
- Around line 49-53: Update the executable candidate selection in the resolver
to retain only paths matching the current host’s executable suffix before
evaluating existence and calling max. Preserve the newest-existing-candidate
behavior while excluding incompatible .exe or unsuffixed artifacts from other
hosts.
In `@src/core/ControlModule.h`:
- Around line 639-644: Update the rename flow around the source-removal call in
ControlModule so it checks the result of platform::fsRemove(src) before setting
ok to true. If source deletion fails after fsWriteAtomic succeeds, attempt to
remove the newly created destination when possible, keep the operation failed,
and preserve the existing cleanup and rescan behavior.
- Around line 382-391: The preset list must refresh immediately after File
Manager uploads, deletes, and renames. Add a core-neutral filesystem-change
notification or explicit live refresh path connected to ControlModule::rescan()
for changes in the preset folder; update docs/moonmodules/core/control.md lines
27-29 to retain the documented behavior only if live refresh is implemented,
otherwise document the required manual refresh step.
- Around line 278-287: Keep active-role state synchronized with preset mutations
in ControlModule: in deleteListRow, after a successful fsRemove, clear every
current_ entry matching the deleted preset name; in the rename flow around lines
619-645, after a successful rename, replace every current_ entry matching from
with to. Apply the required changes at both listed sites in
src/core/ControlModule.h (278-287 and 619-645).
In `@src/core/HttpServerModule.cpp`:
- Around line 1557-1567: The WebSocket ingress predicate in
pollWledStateFromWebSockets must invoke applyWledState for frames containing
only ps, not just on or bri. Extend that predicate to recognize ps and add a
regression test covering a masked WebSocket frame with a standalone ps command.
🪄 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: 22131fa5-8b3c-4797-8ac3-85d896892434
📒 Files selected for processing (28)
CLAUDE.mddocs/adr/README.mddocs/architecture.mddocs/backlog/backlog-core.mddocs/backlog/power-functions-analysis-bottom-up.mddocs/backlog/power-functions-analysis-top-down.mddocs/backlog/rename-to-moonlight.mddocs/backlog/system-modules.mddocs/backlog/ui-extensibility-analysis-bottom-up.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/core/control.mdmoondeck/run/run_desktop.pysrc/core/ControlModule.hsrc/core/FilesystemModule.cppsrc/core/HttpServerModule.cppsrc/core/ModuleFactory.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/core/math16.hsrc/platform/desktop/main_desktop.cpptest/CMakeLists.txttest/scenarios/light/scenario_peripheral_grid_sweep.jsontest/unit/core/unit_ControlModule.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_math16.cpptest/unit/light/golden_frame.htest/unit/light/unit_Effects_golden.cpp
| @@ -0,0 +1,190 @@ | |||
| # Power functions — bottom-up analysis | |||
|
|
|||
| > **Forward-looking research document — exception to CLAUDE.md present-tense rule.** This is a Stage-1 bottom-up survey of *power functions*: the shared primitives (drawing, fields, physics, color, time) that LED effects are really made of, which MoonLive should expose as built-ins so scripts stay compact. It inventories three sources read on **2026-08-05**: (a) our own 39 compiled effects and the helpers they share, (b) WLED / the WLED Particle System / FastLED as prior art, (c) the industry-standard algorithm canon with originators. The **top-down** companion (to be written) turns the catalog into an implementation spec. Modelled on [livescripts-analysis-bottom-up.md](livescripts-analysis-bottom-up.md). Source citations are `file:line` against this repo, or repo-relative paths for external sources; usage counts come from reading every effect header and grepping the cloned externals. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale “to be written” reference.
Line 3 says the top-down companion is still to be written, but docs/backlog/power-functions-analysis-top-down.md is included in this change and already provides that specification. Update the sentence to describe the current repository state.
🤖 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/power-functions-analysis-bottom-up.md` at line 3, Update the
introductory paragraph in the power-functions analysis document to remove the
stale “to be written” wording and state that the top-down companion already
exists as the implementation specification, preserving the surrounding
description and link context.
| - **Modern additions the classic canon lacks.** Two primitives from the shader world earn a place on CPU: **2D signed distance functions** (circle/box/segment + smooth-min; Quilez) — anti-aliased shapes, outlines, glow and metaball-morphing from a few fixed-point ops — and **cosine gradient palettes** (12 constants = a whole palette, bakeable to a LUT). Plus the **Wu sub-pixel splat**, which WLED treats as the difference between 8-bit-console and modern motion on a coarse matrix. | ||
| - **Fixed-point is settled policy, and the conventions already exist in-repo.** Coding standards mandate integer-first ([coding-standards § numeric types](../coding-standards.md)); effects document the working idioms: uint8 angle (256 = full turn), palette index mod-256, noise coords 16.0 fixed, the uint64 BPM phase numerator divided late, 12.4 particle positions. Power functions adopt these, not float. The known trap to design around: naive signed right-shift rounds asymmetrically (−1>>1 = −1) — WLED-PS documents the sign-corrected form. | ||
| - **Prior art is cataloged, credited, and not ported.** Standing rule ([no-WLED-MM-derivation](../../CLAUDE.md)) plus license reality: WLED and the PS are EUPL v1.2. This document takes *concepts, measurements and API shapes*; implementations come fresh from the textbook sources named per primitive (Bresenham 1965, Wu 1991, Blinn 1982/1996, Reynolds 1987, Penner 2002, Kriegsman's fire2012, Elias's ripple, Quilez's articles). | ||
| - **Recommendation for the top-down doc: a ~30-function core in eight families** (§ The candidate set), dimension-generic per the PO decision, each function: one canonical algorithm, integer form, one home in core or light. The three MoonLive-side constraints that must be lifted for scripts to use any of this: the 16-entry builtin table, the one-arg-in/one-out host-call ABI, and a grammar with no variables, loops, or coordinate/time symbols ([MoonLiveBuiltins.h:40-54](../../src/core/moonlive/MoonLiveBuiltins.h), [MoonLiveCompiler.h:11-15](../../src/core/moonlive/MoonLiveCompiler.h)). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the family and function counts.
The TL;DR says “~30-function core in eight families”. The synthesis defines “~34 functions” and families 1–9, including Projection at Line 144. Line 181 repeats “all eight families”. Update the stale counts so the scope is consistent.
Also applies to: 132-145, 181-181
🤖 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/power-functions-analysis-bottom-up.md` at line 16, Reconcile the
documented scope counts in the TL;DR, synthesis, and repeated summary: update
the stale “~30-function” and “eight families” references to consistently reflect
the defined ~34 functions across families 1–9, including Projection. Ensure all
affected statements use the same totals.
| |---|---|---|---| | ||
| | 1 | **Frame ops** | `fill` *(have)*, `fade` *(have)*, `blur` *(have — already dimension-generic)*, `scroll(axis, delta, wrap)` | WLED #6/#8/#9; FreqMatrix's hand-rolled shift | | ||
| | 2 | **Pixel ops** | `pixel`/`get`/`addPixel`/`blendPixel` *(have)*, **`splat(fx, fy, c)`** — the Wu sub-pixel writer, 12.4 or 16.16 coords | WLED-PS renderer; ParticlesEffect's private 12.4 math; "modern motion" on coarse matrices | | ||
| | 3 | **Geometry** | `line` *(have)*, `lineAA` (Wu 1991), `circle`/`fillCircle` (midpoint), `rect`/`fillRect`/`bar` (the audio-meter staple), `text` *(have)*; **SDF trio** `sdCircle/sdBox/sdSegment` + `smin` + coverage-AA | 4 effects hand-roll bars; SDFs subsume metaballs/glow/outline | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify whether fillTriangle is in scope.
The candidate table at Line 138 omits fillTriangle, the gather gap adds it at Line 162, and Line 172 places filled polygons below the cut. The companion top-down spec uses fillTriangle in VectorBallsEffect at Line 102. If triangles are in Stage 2, list them in the candidate set and distinguish them from deferred general polygon fill. Otherwise, remove the top-down showcase reference.
Also applies to: 162-162, 172-172
🤖 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/power-functions-analysis-bottom-up.md` at line 138, Clarify the
Stage 2 scope for fillTriangle across the candidate table, gather-gap entry,
filled-polygons cut line, and VectorBallsEffect showcase reference. If triangles
are included, list fillTriangle explicitly and distinguish it from deferred
general polygon fill; otherwise remove the showcase reference and keep the
deferred-scope statements consistent.
|
|
||
| ## 3. The particle kernel | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the code fence.
The particle API example begins with an untyped fence at Line 44. Change it to ```cpp so Markdownlint MD040 passes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/power-functions-analysis-top-down.md` at line 44, Update the
particle API example’s code fence in the documentation to specify the cpp
language tag, changing the untyped opening fence to a cpp-tagged fence so
Markdownlint MD040 passes.
Source: Linters/SAST tools
Comment on lines
+341
to
+345
| // Only the one or two presets whose slot changed get rewritten: a reorder used to rewrite | ||
| // EVERY file on the device, which is flash wear proportional to the collection for a | ||
| // two-preset change. | ||
| writeSlot(*moving); | ||
| if (occupant) writeSlot(*occupant); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make slot moves durable before reporting success.
writeSlot does not report write failure. This method changes the in-memory order and returns success even if a slot rewrite fails. A reboot can restore the old order or a half-written swap.
A successful reorder also changes the WLED preset-slot mapping. Increment presetsRevision_ only after all affected files persist, so Home Assistant refreshes its cached /presets.json mapping.
Make writeSlot return success. Roll back the in-memory swap and return failure when either write fails. Use a recoverable strategy for the two-file swap so one failed write does not leave duplicate slot claims.
🤖 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/ControlModule.h` around lines 341 - 345, Update the reorder method
surrounding writeSlot(*moving) and writeSlot(*occupant) so writeSlot returns
whether each file write persisted successfully. Only increment presetsRevision_
and report success after every affected write succeeds; on failure, restore the
original in-memory slot assignments and use a recoverable two-file swap strategy
that avoids duplicate slot claims after a partial write.
Comment on lines
+52
to
+88
| inline uint16_t sin16(angle16 theta) { | ||
| const uint8_t quadrant = static_cast<uint8_t>(theta >> 14); // 0..3 | ||
| const uint16_t pos = static_cast<uint16_t>(theta & 0x3FFF); // position within the quadrant | ||
| // Odd quadrants run the quarter wave backwards (sin descends from the peak). | ||
| const uint16_t walk = (quadrant & 1) ? static_cast<uint16_t>(0x4000 - pos) : pos; | ||
| // uint16 for the index, NOT uint8: walk reaches 0x4000 exactly at a quadrant boundary in an odd | ||
| // quadrant, and >>8 is then 64 — which a uint8 holds fine but only by luck of the cast order. | ||
| // Keeping it wide makes the "table has 65 entries so idx==64 is valid" invariant explicit. | ||
| const uint16_t idx = static_cast<uint16_t>(walk >> 8); // 0..64 | ||
| const uint8_t frac = static_cast<uint8_t>(walk & 0xFF); | ||
| const int32_t a = sin16_quarter[idx]; | ||
| // At idx==64 the wave is exactly at the peak and frac is 0, so the b term contributes nothing; | ||
| // clamping keeps the read inside the 65-entry table without a branch on the hot path. | ||
| const int32_t b = sin16_quarter[idx < 64 ? idx + 1 : 64]; | ||
| int32_t v = a + (((b - a) * frac) >> 8); | ||
| if (quadrant >= 2) v = -v; // lower half of the circle | ||
| return static_cast<uint16_t>(v + 32768); | ||
| } | ||
|
|
||
| /// Cosine: a quarter turn ahead of sine. | ||
| inline uint16_t cos16(angle16 theta) { return sin16(static_cast<angle16>(theta + 16384)); } | ||
|
|
||
| // ---- Range mapping --------------------------------------------------------- | ||
|
|
||
| /// Map `v` from [inLo,inHi] to [outLo,outHi] in 64-bit intermediate, clamped to the input range. | ||
| /// | ||
| /// The fencepost lives HERE, once: six effects hand-rolled this and each carried its own comment | ||
| /// about the off-by-one (mapping to `n-1` vs `n` when the output is a grid extent). Callers map to | ||
| /// the extent (`0..w`) and the result is clamped to `w-1` by the pixel writer, which is the form | ||
| /// that does not lose the last column. | ||
| inline int32_t map32(int32_t v, int32_t inLo, int32_t inHi, int32_t outLo, int32_t outHi) { | ||
| if (inHi == inLo) return outLo; // zero span: no meaningful ratio | ||
| if (inHi > inLo) { if (v <= inLo) return outLo; if (v >= inHi) return outHi; } | ||
| else { if (v >= inLo) return outLo; if (v <= inHi) return outHi; } | ||
| const int64_t num = static_cast<int64_t>(v - inLo) * (outHi - outLo); | ||
| return static_cast<int32_t>(outLo + num / (inHi - inLo)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^CMakeLists\.txt$' -x rg -n 'CXX_STANDARD|cxx_std_' {}
rg -n -C 2 'inline (constexpr )?uint16_t (sin16|cos16)|inline (constexpr )?int32_t map32' src/core/math16.hRepository: MoonModules/projectMM
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- math16 constants ---\n'
fd -a '^math16\.h$' -x sh -c 'echo "### $1"; sed -n "1,140p" "$1"' sh {}
printf '\n--- C++ standard references ---\n'
rg -n -C 3 'set\(CMAKE_CXX_STANDARD|CMAKE_CXX_STANDARD|cxx_std_|CXX_STANDARD_REQUIRED' --glob 'CMakeLists.txt' .
printf '\n--- uses of sin16/cos16/map32 ---\n'
rg -n -C 2 '\b(sin16|cos16|map32)\b' src/core src || true
printf '\n--- constexpr-like definitions near math16 ---\n'
rg -n -C 2 'constexpr|consteval|sin16_quarter|angle16' src/core/math16.h src/core || trueRepository: MoonModules/projectMM
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- math16.h excerpt ---'
fd -a '^math16\.h$' -x sh -c 'echo "### $1"; sed -n "1,160p" "$1"' sh {}
echo
echo '--- C++ standard references ---'
rg -n -C 3 'set\(CMAKE_CXX_STANDARD|CMAKE_CXX_STANDARD|cxx_std_|CXX_STANDARD_REQUIRED' --glob 'CMakeLists.txt' . || true
echo
echo '--- core references to sin16/cos16/map32 ---'
rg -n -C 2 '\b(sin16|cos16|map32)\b' src/core src || true
echo
echo '--- constexpr/angle16/sin16_quarter references ---'
rg -n -C 2 'constexpr|consteval|sin16_quarter|angle16' src/core/math16.h src/core || trueRepository: MoonModules/projectMM
Length of output: 50377
Mark sin16 and map32 constexpr.
src/core/math16.h has C++20 enabled, so make these pure integer fixed-point helpers usable at compile time. Keep cos16 as is because it delegates to sin16.
🤖 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/math16.h` around lines 52 - 88, Mark the `sin16` and `map32`
function declarations as `constexpr` so their existing integer-only
implementations can be evaluated at compile time under C++20. Leave `cos16`
unchanged since it already delegates to `sin16`.
Source: Path instructions
Comment on lines
+82
to
+88
| inline int32_t map32(int32_t v, int32_t inLo, int32_t inHi, int32_t outLo, int32_t outHi) { | ||
| if (inHi == inLo) return outLo; // zero span: no meaningful ratio | ||
| if (inHi > inLo) { if (v <= inLo) return outLo; if (v >= inHi) return outHi; } | ||
| else { if (v >= inLo) return outLo; if (v <= inHi) return outHi; } | ||
| const int64_t num = static_cast<int64_t>(v - inLo) * (outHi - outLo); | ||
| return static_cast<int32_t>(outLo + num / (inHi - inLo)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent signed overflow in map32.
Line 86 subtracts int32_t values before the cast to int64_t. For example, v == INT32_MAX and inLo == INT32_MIN invokes signed overflow.
Widen operands before subtraction. Also use a safe multiply-divide implementation for full-width ranges, because two valid 32-bit spans can exceed int64_t when multiplied. Add INT32_MIN and INT32_MAX regression cases.
As per path instructions, “For any input, order, or size, degrade visibly rather than crash.”
🤖 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/math16.h` around lines 82 - 88, Update map32 to widen operands
before subtracting, avoiding int32_t overflow, and replace the intermediate
int64_t multiplication with an overflow-safe multiply-divide approach that
handles full-width 32-bit input and output spans. Preserve clamping and
zero-span behavior, and add regression coverage for INT32_MIN/INT32_MAX
combinations across input and output ranges; invalid or unrepresentable cases
must degrade visibly rather than crash.
Source: Path instructions
Comment on lines
+13
to
+14
| #include <cmath> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^CMakeLists\.txt$' -x rg -n 'CXX_STANDARD|cxx_std_' {}
rg -n -C 2 '`#include` <algorithm>|std::max|M_PI|std::numbers' test/unit/core/unit_math16.cppRepository: MoonModules/projectMM
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n test/unit/core/unit_math16.cpp | sed -n '1,70p'
printf '\nProject standard references:\n'
grep -nE 'CMAKE_CXX_STANDARD|cxx_std_|set\(.*CXX|target_compile_features|std::numbers|M_PI|std::max|algorithm' -R --exclude-dir=.git .Repository: MoonModules/projectMM
Length of output: 23389
Make unit_math16.cpp self-contained and portable.
std::max requires <algorithm>, and M_PI is implementation-defined. This target is C++20, so use <numbers> and std::numbers::pi_v<double> instead, or use a local constexpr pi value.
Also applies to: 41-44
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 13-13: The function 'hsvToRgb' is never used.
(unusedFunction)
🤖 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/unit/core/unit_math16.cpp` around lines 13 - 14, Update unit_math16.cpp
to include <algorithm> for std::max and replace the implementation-defined M_PI
usage with the portable C++20 std::numbers::pi_v<double> from <numbers> (or an
equivalent local constexpr value), including the repeated usage around the
referenced lines.
Comment on lines
+311
to
+333
| PresetRig() { | ||
| static unsigned seq = 0; | ||
| std::snprintf(root_, sizeof(root_), "/tmp/mm_mqtt_preset_%u", ++seq); | ||
| std::filesystem::remove_all(root_); | ||
| platform::fsSetRoot(root_); | ||
|
|
||
| ModuleFactory::registerType<Layers>("Layers"); | ||
| ModuleFactory::registerType<Layer>("Layer"); | ||
| ModuleFactory::registerType<NoiseEffect>("NoiseEffect"); | ||
| ModuleFactory::registerType<ControlModule>("ControlModule"); | ||
|
|
||
| fs = new FilesystemModule(); | ||
| fs->setTypeName("FilesystemModule"); | ||
| fs->setScheduler(&scheduler); | ||
| layers = ModuleFactory::create("Layers"); | ||
| control = static_cast<ControlModule*>(ModuleFactory::create("ControlModule")); | ||
| scheduler.addModule(fs); | ||
| scheduler.addModule(layers); | ||
| scheduler.addModule(control); | ||
| fs->setup(); layers->setup(); control->defineControls(); control->setup(); | ||
| mqtt->setControlModule(control); | ||
| } | ||
| ~PresetRig() { std::filesystem::remove_all(root_); } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the filesystem-root override during fixture teardown.
PresetRig removes root_ but leaves platform::fsSetRoot(root_) active. A later test can use a deleted filesystem root and depend on this fixture's state.
Reset the test-only filesystem override before the fixture ends.
As per path instructions, “Tests must reset test-only platform overrides and isolate filesystem roots between cases.”
🤖 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/unit/core/unit_MqttModule.cpp` around lines 311 - 333, Update
PresetRig’s destructor to reset the platform filesystem-root override before or
while tearing down the fixture, then remove root_. Ensure later tests no longer
retain platform::fsSetRoot(root_) after PresetRig ends.
Source: Path instructions
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.
Save the device's state as a named preset and bring it back with one click.
A preset is a file, so it can be uploaded, downloaded and shared. Each one records which parts of the setup it carries, so a look saved on one board applies to a board with completely different hardware. The presets sit on an 8x8 pad grid with rotary encoders above and faders below, laid out like a Mackie control desk (X-Touch, QCon Pro G2) so a MIDI surface maps onto it later without a translation layer.
Why a core module
ControlModuleis a top-level module, a peer of Layouts/Layers/Drivers rather than a child of Services, because it reaches across the top-level modules and cannot sit inside one. Presets are its first capability; external control (MIDI, IR, a hardware panel) is what it exists to host next.MoonLight solved presets inside
ModuleLightsControl, effects-only. This generalises it: a preset can carry any part of the tree, and the file records which.What a preset carries
{ "slot": 12, "captures": "Layouts,Layers", "Layouts.enabled": true, "Layouts.0.type": "GridLayout", "Layouts.0.width": 128, "Layers.enabled": true, "Layers.0.type": "Layer", "Layers.0.0.type": "NoiseEffect" }Each captured subtree is exactly the bytes the persistence engine already writes, namespaced under a
<TypeName>.prefix. Save and restore reuse the engine that already reconciles a tree against JSON rather than a second serializer that could drift from it.Layersalone is a portable look. AddingDriversmakes it a device snapshot carrying pin maps. Because the file records the set, applying a preset is never a surprise.One active preset per role
Each capturable subtree holds a role: layout, layer, driver, service. Applying a preset claims every role it carries and leaves the others alone, so a layout preset and a layer preset are both lit at once, and a new layer preset replaces only the layer.
Pads are tinted by their roles, mixing hues when a preset carries several. This is why mixed presets need no special case: a mixed preset owns several roles rather than being a different kind of pad, and one rule both colours it and decides when it is superseded.
Core changes
FilesystemModule::saveSubtreeTo/applySubtree— two new seams.saveSubtreenow calls the former, so there is exactly one serializer.applySubtreeguards on the prefix being present: without itapplyNodereads "no children in JSON" as "delete every child", so a truncated preset file would wipe the live look. Pinned by two tests.ListSource::persistsList— a list whose rows are re-derived at setup is no longer written to flash. The preset list was being serialized on every save and discarded on load. Added at the core seam so Pins/Tasks can use it too./,\or.. The name becomes a file name, and ESP32'sfsTranslatedoes no path normalization (the desktop one does), so an unguarded name could escape the preset folder on device through save, delete or rename.UI
The pad grid, encoders and faders share one column track, so the three banks line up and still follow the pane as it is resized.
Two real bugs fixed along the way:
inputnorchange), and were built before the input had a value or bounds. OneredrawRangeDecorationscall now owns that seam, so any decorated control added later stays in sync.Testing
Gap, stated plainly: no scenario test for the preset round trip. The scenario runner has no op that can apply a preset — it speaks
/api/control, and applying a preset needs/api/list/. Extending the runner is separate work, so the round trip is covered by unit tests only.Review
Findings fixed: path traversal via preset name; the persisted-then-discarded list; a redundant whole-folder rewrite on save that could displace an unrelated preset (fixing it exposed a real bug where an unaimed save landed on pad 1); a duplicated fader binding; stale
ordernaming in three comments.One finding not applied: "applying a preset runs on the HTTP thread, not the render tick".
HttpServerModule::tick20msisMM_NONBLOCKINGand drains synchronously insideScheduler::tick, so the docstring as written is correct.Deferred to the dynamic-presets rework: the 192-byte header read, the insertion-sort struct copies, and
kMaxPresets36 -> 64 (accepted for now).Not in this PR
Playlists, apply-on-boot, and the external-control bindings the encoders and faders 2-8 are waiting for. Each gets its own plan.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation