Add ControlModule: presets on a control surface - #62
Open
ewowi wants to merge 2 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
+45
to
+52
| # A plain `cmake --build build` writes here rather than into the per-host dir, so this path | ||
| # is often the NEWER binary. Both are considered and the freshest wins below: picking the | ||
| # first that merely exists served a stale build whose changes appeared to be no-ops. | ||
| ROOT / "build" / "projectMM", | ||
| ] | ||
| for c in candidates: | ||
| if c.exists(): | ||
| return c | ||
| existing = [c for c in candidates if c.exists()] | ||
| if existing: | ||
| return max(existing, key=lambda c: c.stat().st_mtime) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include the Windows suffix for the root-build candidate.
Line 48 always checks ROOT / "build" / "projectMM". A Windows executable uses the .exe suffix, so ROOT/build/projectMM.exe is skipped. The launcher can then select an older executable or report that no executable exists.
Proposed fix
- ROOT / "build" / "projectMM",
+ ROOT / "build" / (
+ "projectMM.exe" if sys.platform == "win32" else "projectMM"
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # A plain `cmake --build build` writes here rather than into the per-host dir, so this path | |
| # is often the NEWER binary. Both are considered and the freshest wins below: picking the | |
| # first that merely exists served a stale build whose changes appeared to be no-ops. | |
| ROOT / "build" / "projectMM", | |
| ] | |
| for c in candidates: | |
| if c.exists(): | |
| return c | |
| existing = [c for c in candidates if c.exists()] | |
| if existing: | |
| return max(existing, key=lambda c: c.stat().st_mtime) | |
| # A plain `cmake --build build` writes here rather than into the per-host dir, so this path | |
| # is often the NEWER binary. Both are considered and the freshest wins below: picking the | |
| # first that merely exists served a stale build whose changes appeared to be no-ops. | |
| ROOT / "build" / ( | |
| "projectMM.exe" if sys.platform == "win32" else "projectMM" | |
| ), | |
| ] | |
| existing = [c for c in candidates if c.exists()] | |
| if existing: | |
| return max(existing, key=lambda c: c.stat().st_mtime) |
🤖 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 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.
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.
Comment on lines
77
to
90
| bool MqttModule::ensureDiscoveryBuffers() { | ||
| if (discoveryBuf_ && discoveryPayload_) return true; | ||
| if (!discoveryBuf_) discoveryBuf_ = static_cast<uint8_t*>(platform::alloc(kDiscoveryBufLen)); | ||
| if (!discoveryPayload_) discoveryPayload_ = static_cast<char*>(platform::alloc(kDiscoveryPayloadLen)); | ||
| const size_t effects = haEffectListBytes(); | ||
| const size_t wantPayload = kDiscoveryPayloadBase + effects; | ||
| const size_t wantBuf = kDiscoveryBufBase + effects; | ||
| if (discoveryBuf_ && discoveryPayload_ && | ||
| discoveryPayloadLen_ == wantPayload && discoveryBufLen_ == wantBuf) return true; | ||
| freeDiscoveryBuffers(); | ||
| discoveryBuf_ = static_cast<uint8_t*>(platform::alloc(wantBuf)); | ||
| discoveryPayload_ = static_cast<char*>(platform::alloc(wantPayload)); | ||
| if (!discoveryBuf_ || !discoveryPayload_) { freeDiscoveryBuffers(); return false; } | ||
| setDynamicBytes(kDiscoveryBufLen + kDiscoveryPayloadLen); | ||
| discoveryPayloadLen_ = wantPayload; | ||
| discoveryBufLen_ = wantBuf; | ||
| setDynamicBytes(wantBuf + wantPayload); | ||
| return true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Re-publish discovery when the preset set changes.
ensureDiscoveryBuffers() resizes only when publishDiscovery(true) runs. A preset save, rename, or delete calls ControlModule::rescan(), but the supplied MQTT paths do not re-announce the retained discovery config. Home Assistant therefore keeps an obsolete fx_list until reconnect or until haDiscovery is toggled.
Expose a monotonic preset revision from ControlModule. Track it in MqttModule::tick1s(). When it changes while discovery is enabled and connected, call publishDiscovery(true). Add coverage for live save, rename, and delete.
🤖 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/MqttModule.cpp` around lines 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.
Comment on lines
+186
to
+204
| // The looks this device offers, as HA's effect list. Built into a scratch region of the payload | ||
| // buffer first so its true length is known before the config is assembled around it. | ||
| char* fxScratch = discoveryPayload_ + kDiscoveryPayloadBase / 2; | ||
| const size_t fxLen = writeHaEffectList(fxScratch, discoveryPayloadLen_ - kDiscoveryPayloadBase / 2); | ||
| char fxKey[24] = ""; | ||
| if (fxLen) std::snprintf(fxKey, sizeof(fxKey), "\"effect\":true,"); | ||
|
|
||
| const int pn = std::snprintf(discoveryPayload_, discoveryPayloadLen_, | ||
| "{\"schema\":\"json\",\"name\":null,\"uniq_id\":\"%s\",\"cmd_t\":\"%s\"," | ||
| "\"stat_t\":\"%s\",\"avty_t\":\"%s\",\"brightness\":true," | ||
| "\"stat_t\":\"%s\",\"avty_t\":\"%s\",\"brightness\":true,%s%s%s%s" | ||
| "\"dev\":{\"ids\":[\"%s\"],\"name\":\"%s\",\"mf\":\"MoonModules\",\"mdl\":\"projectMM\"}}", | ||
| id, cmd, stat, avty, id, dnEsc); | ||
| if (pn <= 0 || static_cast<size_t>(pn) >= kDiscoveryPayloadLen) return; // truncated → don't send a broken config | ||
| id, cmd, stat, avty, | ||
| fxKey, | ||
| fxLen ? "\"fx_list\":[" : "", fxLen ? fxScratch : "", fxLen ? "]," : "", | ||
| id, dnEsc); | ||
| if (pn <= 0 || static_cast<size_t>(pn) >= discoveryPayloadLen_) return; // truncated → don't send a broken config | ||
|
|
||
| const size_t n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(discoveryPayload_), | ||
| static_cast<size_t>(pn), discoveryBuf_, kDiscoveryBufLen, | ||
| static_cast<size_t>(pn), discoveryBuf_, discoveryBufLen_, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not use discoveryPayload_ as both snprintf input and output.
Line 188 stores fxScratch inside discoveryPayload_. Line 193 then writes the final JSON to the same buffer while %s reads fxScratch. The ranges overlap. This can corrupt the fx_list or invoke undefined behavior.
Store the effect list in non-overlapping scratch storage before formatting the payload. discoveryBuf_ is available until buildMqttPublish() starts.
Proposed fix
- char* fxScratch = discoveryPayload_ + kDiscoveryPayloadBase / 2;
- const size_t fxLen = writeHaEffectList(fxScratch, discoveryPayloadLen_ - kDiscoveryPayloadBase / 2);
+ char* fxScratch = reinterpret_cast<char*>(discoveryBuf_);
+ const size_t fxLen = writeHaEffectList(fxScratch, discoveryBufLen_);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The looks this device offers, as HA's effect list. Built into a scratch region of the payload | |
| // buffer first so its true length is known before the config is assembled around it. | |
| char* fxScratch = discoveryPayload_ + kDiscoveryPayloadBase / 2; | |
| const size_t fxLen = writeHaEffectList(fxScratch, discoveryPayloadLen_ - kDiscoveryPayloadBase / 2); | |
| char fxKey[24] = ""; | |
| if (fxLen) std::snprintf(fxKey, sizeof(fxKey), "\"effect\":true,"); | |
| const int pn = std::snprintf(discoveryPayload_, discoveryPayloadLen_, | |
| "{\"schema\":\"json\",\"name\":null,\"uniq_id\":\"%s\",\"cmd_t\":\"%s\"," | |
| "\"stat_t\":\"%s\",\"avty_t\":\"%s\",\"brightness\":true," | |
| "\"stat_t\":\"%s\",\"avty_t\":\"%s\",\"brightness\":true,%s%s%s%s" | |
| "\"dev\":{\"ids\":[\"%s\"],\"name\":\"%s\",\"mf\":\"MoonModules\",\"mdl\":\"projectMM\"}}", | |
| id, cmd, stat, avty, id, dnEsc); | |
| if (pn <= 0 || static_cast<size_t>(pn) >= kDiscoveryPayloadLen) return; // truncated → don't send a broken config | |
| id, cmd, stat, avty, | |
| fxKey, | |
| fxLen ? "\"fx_list\":[" : "", fxLen ? fxScratch : "", fxLen ? "]," : "", | |
| id, dnEsc); | |
| if (pn <= 0 || static_cast<size_t>(pn) >= discoveryPayloadLen_) return; // truncated → don't send a broken config | |
| const size_t n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(discoveryPayload_), | |
| static_cast<size_t>(pn), discoveryBuf_, kDiscoveryBufLen, | |
| static_cast<size_t>(pn), discoveryBuf_, discoveryBufLen_, | |
| // The looks this device offers, as HA's effect list. Built into a scratch region of the payload | |
| // buffer first so its true length is known before the config is assembled around it. | |
| char* fxScratch = reinterpret_cast<char*>(discoveryBuf_); | |
| const size_t fxLen = writeHaEffectList(fxScratch, discoveryBufLen_); | |
| char fxKey[24] = ""; | |
| if (fxLen) std::snprintf(fxKey, sizeof(fxKey), "\"effect\":true,"); | |
| const int pn = std::snprintf(discoveryPayload_, discoveryPayloadLen_, | |
| "{\"schema\":\"json\",\"name\":null,\"uniq_id\":\"%s\",\"cmd_t\":\"%s\"," | |
| "\"stat_t\":\"%s\",\"avty_t\":\"%s\",\"brightness\":true,%s%s%s%s" | |
| "\"dev\":{\"ids\":[\"%s\"],\"name\":\"%s\",\"mf\":\"MoonModules\",\"mdl\":\"projectMM\"}}", | |
| id, cmd, stat, avty, | |
| fxKey, | |
| fxLen ? "\"fx_list\":[" : "", fxLen ? fxScratch : "", fxLen ? "]," : "", | |
| id, dnEsc); | |
| if (pn <= 0 || static_cast<size_t>(pn) >= discoveryPayloadLen_) return; // truncated → don't send a broken config | |
| const size_t n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(discoveryPayload_), | |
| static_cast<size_t>(pn), discoveryBuf_, discoveryBufLen_, |
🤖 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/MqttModule.cpp` around lines 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.
Comment on lines
+826
to
+834
| // The applied look rides along, so HA's dropdown shows what is actually on -- including when | ||
| // the change came from the device's own pad grid rather than from HA. | ||
| char fxEsc[72] = ""; | ||
| const char* look = controlModule_ ? controlModule_->currentLook() : nullptr; | ||
| if (look && look[0]) jsonEscape(look, fxEsc, sizeof(fxEsc)); | ||
| char haState[136]; | ||
| std::snprintf(haState, sizeof(haState), "{\"state\":\"%s\",\"brightness\":%u%s%s%s}", | ||
| on ? "ON" : "OFF", static_cast<unsigned>(bri), | ||
| fxEsc[0] ? ",\"effect\":\"" : "", fxEsc, fxEsc[0] ? "\"" : ""); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the active look in MQTT state change detection.
publishState(false) returns before this block when on, brightness, and palette are unchanged. Applying a look-only preset changes ControlModule::currentLook() without necessarily changing those Drivers values. The retained ha/state message then continues to report the previous effect.
Track a signature of currentLook() with lastOn_, lastBri_, and lastPalette_. Publish and commit it only after all state sends succeed. Add a regression test that applies two look-only presets with unchanged Drivers values.
🤖 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/MqttModule.cpp` around lines 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.
Comment on lines
+75
to
+92
| int main(int argc, char** argv) { | ||
| // --port N: the HTTP port. Defaults to 8080 because ports below 1024 need root on POSIX, but | ||
| // Home Assistant's WLED integration hardcodes port 80 with no way to specify another, so testing | ||
| // that path on desktop needs `sudo projectMM --port 80`. | ||
| uint16_t httpPort = 8080; | ||
| for (int i = 1; i < argc; i++) { | ||
| const bool isPort = std::strcmp(argv[i], "--port") == 0; | ||
| if (isPort && i + 1 < argc) { | ||
| const long v = std::strtol(argv[++i], nullptr, 10); | ||
| if (v > 0 && v <= 65535) httpPort = static_cast<uint16_t>(v); | ||
| else { std::printf("--port must be 1..65535\n"); return 1; } | ||
| } else if (isPort) { | ||
| std::printf("--port needs a value\n"); return 1; | ||
| } else if (std::strcmp(argv[i], "--help") == 0) { | ||
| std::printf("usage: projectMM [--port N] (default 8080; 80 needs root)\n"); | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n \
--glob 'test/**' \
--glob 'moondeck/**/*.py' \
--glob 'src/platform/desktop/*.cpp' \
-e 'projectMM' -e '--port' -e 'httpPort' .Repository: MoonModules/projectMM
Length of output: 25479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== main_desktop.cpp relevant section ==\n'
sed -n '70,95p' src/platform/desktop/main_desktop.cpp
printf '\n== run_desktop.py arg parsing relevant section ==\n'
sed -n '85,105p' moondeck/run/run_desktop.py
printf '\n== command-line tests relevant section ==\n'
sed -n '1,80p' test/scenario_runner.cpp
rg -n --glob 'test/**/*.cpp' --glob 'test/scenario_runner.cpp' -e 'projectMM|mm_scenarios|--port|unknown argument|--help' .
printf '\n== strtol behavior probe ==\n'
python3 - <<'PY'
import ctypes, sys, os
csu = {'linux':'libc.so.6','darwin':'libc.dylib'}.get(sys.platform,'libc.so.6')
try:
lib=ctypes.CDLL(csu)
except Exception as e:
print('libc probe unavailable:', e)
raise SystemExit(0)
strtoll = lib.strtoll
for s in ['8080','8080oops','80a','',None]:
end=ctypes.c_char_p()
err=ctypes.c_int()
try:
ctypes.memset(ctypes.byref(err), 0, ctypes.sizeof(err))
ctypes.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_char_p), ctypes.c_int]
ctypes.restype = ctypes.c_longlong
ctypes.c_errno.value = 0
ret=strtoll((s or '').encode() if hasattr(ctypes.c_char_p,'value') and s is not None else None, ctypes.byref(end), 10)
print(repr(s), ret, ctypes.string_at(end.ctypes), ctypes.c_errno.value)
except Exception as e:
print(repr(s), type(e).__name__, str(e))
PYRepository: MoonModules/projectMM
Length of output: 13372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for desktop CLI tests/scenarios ==\n'
fd -e cpp -e cc -e c -e h -e json 'desktop|scenarios?|main' test src | sed -n '1,120p'
rg -n -e 'Run the desktop build|--port|projectMM is running|projectMM log' moondeck/run test src/platform desktop scenarios scenarios.json 2>/dev/null | sed -n '1,220p'
printf '\n== strtoll/end-pointer probe via system library (if available) ==\n'
python3 - <<'PY'
import ctypes, os, sys
libc = ctypes.CDLL('libc.so.6')
for s in ['8080','8080oops','80abc','abc80','']:
p = ctypes.c_char_p((s or '').encode())
end = ctypes.c_char_p()
libc.strtoll.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_char_p), ctypes.c_int]
libc.strtoll.restype = ctypes.c_int64
err = errno = 0 # C++ strtol uses errno, not separate return
ret = libc.strtoll(p, ctypes.byref(end), 10)
print(repr(s), ret, repr(ctypes.string_at(end)), os.errno, errno)
PYRepository: MoonModules/projectMM
Length of output: 2797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== strtol end-pointer behavior probe ==\n'
python3 - <<'PY'
import ctypes, errno
libc = ctypes.CDLL('libc.so.6')
for s in ['8080','8080oops','80abc','abc80','01','']:
p = ctypes.c_char_p((s or '').encode())
end = ctypes.c_char_p()
libc.strtoul.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_char_p), ctypes.c_int]
libc.strtoul.restype = ctypes.c_uint
libc.errno.argtypes = []
libc.errno.restype = ctypes.POINTER(ctypes.c_int)
libc.errno.value = 0
ret = libc.strtoul(p, ctypes.byref(end), 10)
print(repr(s), ret, repr(s and ctypes.string_at(end) or b''), libc.errno.value)
PY
printf '\n== run_desktop.py unknown argument behavior ==\n'
python3 - <<'PY'
import subprocess
code='''import argparse, sys
ap = argparse.ArgumentParser(description="x")
ap.add_argument('--port', type=int, default=None, help="x")
args = ap.parse_args()
'''
try:
subprocess.check_output([sys.executable, '-c', code, '--unknown'], stderr=subprocess.STDOUT, text=True)
print('argparse accepted unknown argument')
except subprocess.CalledProcessError as e:
print('returns', e.returncode)
print(e.output.splitlines()[0])
PYRepository: MoonModules/projectMM
Length of output: 466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import subprocess, sys
code='''import argparse, sys
ap = argparse.ArgumentParser(description="x")
ap.add_argument("--port", type=int, default=None, help="x")
args = ap.parse_args()'''
try:
subprocess.check_output([sys.executable, '-c', code, '--unknown'], stderr=subprocess.STDOUT, text=True)
print('argparse accepted unknown argument')
except subprocess.CalledProcessError as e:
print('returns', e.returncode)
print(e.output.splitlines()[0])
PYRepository: MoonModules/projectMM
Length of output: 198
Reject malformed and unknown command-line arguments.
projectMM --port 8080oops currently parses as port 8080 because std::strtol() accepts the valid numeric prefix and ignores trailing characters. Unknown arguments are also accepted and ignored.
Use an end pointer and reject unconsumed characters. Return an error for unknown arguments. Add regression coverage for valid, missing, non-numeric, trailing-character, and out-of-range port values.
Proposed fix
+// Add <cerrno> with the other includes.
+
- const long v = std::strtol(argv[++i], nullptr, 10);
- if (v > 0 && v <= 65535) httpPort = static_cast<uint16_t>(v);
+ const char* raw = argv[++i];
+ char* end = nullptr;
+ errno = 0;
+ const long v = std::strtol(raw, &end, 10);
+ if (errno == 0 && end != raw && *end == '\0' &&
+ v >= 1 && v <= 65535) {
+ httpPort = static_cast<uint16_t>(v);
+ }
else { std::printf("--port must be 1..65535\n"); return 1; }
} else if (isPort) {
std::printf("--port needs a value\n");
std::printf("unknown argument: %s\n", argv[i]);
return 1;
} else if (std::strcmp(argv[i], "--help") == 0) {
std::printf("usage: projectMM [--port N] (default 8080; 80 needs root)\n");
return 0;🤖 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/platform/desktop/main_desktop.cpp` around lines 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.
Sources: Coding guidelines, Path instructions
Comment on lines
+806
to
+826
| TEST_CASE("ControlModule stamps a new revision whenever the preset set changes") { | ||
| Device d; | ||
| d.add(d.layers, "Layer"); | ||
|
|
||
| d.setCapture("Layers"); | ||
| d.setText("name", "first"); | ||
| d.press("save"); | ||
| const uint32_t afterFirst = d.control->presetsModifiedS(); | ||
|
|
||
| // Saving another preset must move the stamp, or a consumer caching on it never re-reads. | ||
| mm::platform::delayMs(1100); // the stamp has second resolution | ||
| d.setText("name", "second"); | ||
| d.press("save"); | ||
| CHECK(d.control->presetsModifiedS() > afterFirst); | ||
|
|
||
| // So must a delete: a removed preset has to disappear from a cached list too. | ||
| const uint32_t afterSecond = d.control->presetsModifiedS(); | ||
| mm::platform::delayMs(1100); | ||
| REQUIRE(d.control->deleteListRow(d.firstRowId())); | ||
| CHECK(d.control->presetsModifiedS() > afterSecond); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for an existing test-only clock/millis override in the platform layer.
fd -e h -e cpp . src/platform | xargs rg -n -i 'millis|ForTest|mock|override' -C 2Repository: MoonModules/projectMM
Length of output: 21094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate control module definitions/usages =="
fd -e h -e cpp 'ControlModule|control' . | sed -n '1,80p'
echo
echo "== Relevant ControlModule snippets =="
rg -n "presetsModifiedS|presetsModifiedS_|rescan\\(" test unit src test/unit/core -g '*ControlModule*' -g '*control*' -g '*.cpp' -g '*.h' -C 3
echo
echo "== Current test around cited lines =="
sed -n '760,840p' test/unit/core/unit_ControlModule.cppRepository: MoonModules/projectMM
Length of output: 9308
Use setTestNowMs() instead of delaying in this test.
The platform layer already provides mm::platform::setTestNowMs() to drive millis() deterministically. Remove the two delayMs(1100) calls and use test-controlled time for the stamp-change expectations, so this timing-independent unit test does not spend ~2.2 seconds in real clock time. Restore setTestNowMs(0) after the case to keep tests independent.
🤖 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_ControlModule.cpp` around lines 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.
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