next-iteration: child-policy + type-boundary + SphereLayout/sparse-preview + GameOfLife/gzip#11
Merged
Conversation
The UI's add / delete / replace / drag affordances were gated on a
hardcoded light-domain list (acceptsNewChildren / rolesAcceptedBy:
"Layers"|"Layer"|"Drivers"|"Layouts") plus a `role === "effect" ||
"modifier"` check — which both violated the architecture's stated rule
(architecture.md: "The UI is MoonModule-driven... no hard-coded
knowledge of specific effects, layouts, or drivers") and, as a bug,
left Drivers' and Layouts' children with no remove/replace/drag even
though they were addable. Both policies now live on the C++ side and
flow to the UI through the existing /api/types + /api/state endpoints.
Two MoonModule virtuals, mirroring the existing role()/tags()/
respectsEnabled() pattern:
- acceptsChildRoles() — on the PARENT, comma-separated roles it
accepts as user-added children ("" = none, the default). Drives
the "+ add child" affordance + picker role filter.
- userEditable() — on the CHILD, whether the user may delete/replace
it (true default). A load-bearing child opts out; the child owns
this because the child knows whether it's safe to remove.
KPI: tick:141674us(FPS:7) on esp32s3-n16r8 — within noise of prior
s3 numbers. The change is registration-time + HTTP-serve-time only,
nothing on the render hot path.
Core:
- MoonModule: added the two virtuals with full rationale comments.
- ModuleFactory: TypeEntry gains acceptsChildRoles (probed at
register time like role/tags/dim); new typeAcceptsChildRoles(i)
accessor; hand-built registerType overload defaults it to "".
- HttpServerModule: /api/types emits acceptsChildRoles per type;
/api/state emits userEditable:false per instance ONLY when a module
opts out (omitted = editable, same byte-saving convention as the
control hidden/readonly flags).
Light domain:
- Layers → "layer", Layer → "effect,modifier", Drivers → "driver",
Layouts → "layout" (one-line acceptsChildRoles override each).
- PreviewDriver → userEditable() false: deleting it would kill the
live 3D preview, so it stays fixed-shape in the UI (persistence /
MoonDeck can still remove it; this only hides the UI affordance).
UI:
- app.js: deleted the hardcoded container-type list and the
role === effect||modifier gate. rolesAcceptedBy() now reads
acceptsChildRoles from /api/types; acceptsNewChildren() derives
from it; new isUserEditableChild() gates delete/replace/drag on
"role is one some container accepts AND userEditable !== false".
Net: the light-type strings are gone from app.js; a new container
type or fixed child needs zero UI edits.
Docs:
- architecture.md § Web UI: the "zero UI changes" guarantee now
explicitly covers tree-mutation affordances (acceptsChildRoles +
userEditable), so the long-standing principle is true rather than
aspirational.
- MoonModule.md: documented the two virtuals and which side owns
which policy (parent: add; child: delete/replace).
- ui.md: updated the add/delete/replace/drag descriptions and the
/api/types + /api/state wire contract for the two new fields;
removed the stale "role→child mapping is derived in the UI" line.
Verified on hardware: the flashed s3-n16r8 serves
userEditable:false for PreviewDriver and acceptsChildRoles for all
four containers via the real ESP32 /api/state + /api/types, not just
the desktop binary.
KPI Details:
Desktop:
Lights: 16,384
Binary: 375 KB
[doctest] 207 cases, 207 passed
tick: 351us, 98us, 300us, 98us, 98us, 100us, 45us, 34us, 35us
(FPS: 2849, 10204, 3333, 10204, 10204, 10000, 22222, 29411, 28571)
=== 10 scenario(s), 10 passed, 0 failed ===
Platform boundary: PASS
Specs: 26 modules, 26 ok
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm):
tick: 141674us (FPS: 7)
Image: 1,309,641 bytes
Code:
62 source files (10650 lines)
38 test files (5410 lines)
Co-Authored-By: Claude Opus 4.8 (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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/HttpServerModule.cpp`:
- Around line 799-804: Ensure server-side validation of role compatibility using
the type metadata's "acceptsChildRoles" before applying mutations: in
handleAddModule and handleReplaceModule, fetch the module type metadata (the
field "acceptsChildRoles" published by /api/types), compute the parent role and
the candidate child's role(s), and if the parent's acceptsChildRoles does not
permit the child's role, reject the request with a proper error response (e.g.,
400/validation error) instead of mutating the tree; do this check prior to any
tree modification and include clear error text identifying the parent type/role
and the incompatible child type/role so callers can correct their request.
In `@src/core/MoonModule.h`:
- Around line 202-210: The comment should document that the virtual method
acceptsChildRoles() returns a pointer whose lifetime must be static because
ModuleFactory::registerType<T>() stores that pointer in static registry state;
update the doc on acceptsChildRoles() (the virtual const char*
acceptsChildRoles() const) to state overrides must return static-lifetime
storage (e.g., string literals or static const char arrays) and must not return
pointers to temporary or stack-allocated buffers to avoid dangling pointers when
ModuleFactory::registerType<T>() saves the value.
In `@src/ui/app.js`:
- Around line 886-890: isUserEditableChild currently uses global
allAcceptedChildRoles(), causing children to be editable if any container
accepts their role; change it to scope checks to the actual parent: update the
function (isUserEditableChild) to accept or resolve the immediate
parent/container of mod and check the parent's accepted-child roles (e.g.,
parent.acceptedChildRoles or parent.acceptedRoles set) for mod.role instead of
allAcceptedChildRoles(); keep the existing depth > 0 and mod.userEditable !==
false checks and fall back to the global allAcceptedChildRoles() only when the
parent cannot be determined.
🪄 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
Run ID: a00284b6-ca20-439b-b68e-a761b51750ba
📒 Files selected for processing (12)
docs/architecture.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/core/ui.mdsrc/core/HttpServerModule.cppsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/light/drivers/Drivers.hsrc/light/drivers/PreviewDriver.hsrc/light/layers/Layer.hsrc/light/layers/Layers.hsrc/light/layouts/Layouts.hsrc/ui/app.js
Comment on lines
+799
to
+804
| "\"docPath\":\"%s\",\"tags\":\"%s\",\"dim\":%u," | ||
| "\"acceptsChildRoles\":\"%s\",\"defaults\":{", | ||
| first ? "" : ",", name, displayName, roleStr, | ||
| docPath ? docPath : "", tags ? tags : "", | ||
| static_cast<unsigned>(dim)); | ||
| static_cast<unsigned>(dim), | ||
| childRoles ? childRoles : ""); |
There was a problem hiding this comment.
Enforce acceptsChildRoles in mutation handlers, not only in metadata output.
Now that /api/types publishes acceptsChildRoles, POST /api/modules and /replace should reject parent/child role mismatches server-side. Right now, API clients can bypass UI filtering and persist invalid tree shapes.
Suggested enforcement sketch
+static bool roleAcceptedByParent(const MoonModule* parent, ModuleRole childRole) {
+ if (!parent) return false;
+ const char* csv = parent->acceptsChildRoles();
+ if (!csv || !csv[0]) return false;
+ const char* wanted = roleName(childRole);
+ // token match on comma-separated values
+ const size_t n = std::strlen(wanted);
+ const char* p = csv;
+ while (*p) {
+ while (*p == ' ' || *p == ',') ++p;
+ const char* s = p;
+ while (*p && *p != ',') ++p;
+ const size_t len = static_cast<size_t>(p - s);
+ if (len == n && std::strncmp(s, wanted, n) == 0) return true;
+ if (*p == ',') ++p;
+ }
+ return false;
+}Apply this check in handleAddModule and handleReplaceModule before mutating the tree.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 802-802: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
[warning] 802-802: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
[warning] 804-804: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
🤖 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 799 - 804, Ensure server-side
validation of role compatibility using the type metadata's "acceptsChildRoles"
before applying mutations: in handleAddModule and handleReplaceModule, fetch the
module type metadata (the field "acceptsChildRoles" published by /api/types),
compute the parent role and the candidate child's role(s), and if the parent's
acceptsChildRoles does not permit the child's role, reject the request with a
proper error response (e.g., 400/validation error) instead of mutating the tree;
do this check prior to any tree modification and include clear error text
identifying the parent type/role and the incompatible child type/role so callers
can correct their request.
Comment on lines
+202
to
+210
| // Comma-separated role names this module accepts as user-added children | ||
| // (e.g. "effect,modifier"). "" = accepts none — the default, covering | ||
| // leaf modules and fixed-shape containers. A container overrides this to | ||
| // tell the UI's "+ add child" picker what to offer. Comma-separated | ||
| // string (not a bitmask) so it serialises straight into /api/types and a | ||
| // multi-role parent (Layer → "effect,modifier") needs no enum-set type. | ||
| // This is what makes the UI domain-neutral: it reads the accepted roles | ||
| // from here instead of hardcoding which module types are containers. | ||
| virtual const char* acceptsChildRoles() const { return ""; } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Clarify the lifetime contract for acceptsChildRoles() return values.
ModuleFactory::registerType<T>() stores this pointer in static registry state. Please document here that overrides must return static-lifetime storage (e.g., string literals), otherwise a transient buffer can dangle.
Proposed doc-only fix
- // Comma-separated role names this module accepts as user-added children
+ // Comma-separated role names this module accepts as user-added children
// (e.g. "effect,modifier"). "" = accepts none — the default, covering
// leaf modules and fixed-shape containers. A container overrides this to
// tell the UI's "+ add child" picker what to offer. Comma-separated
// string (not a bitmask) so it serialises straight into /api/types and a
// multi-role parent (Layer → "effect,modifier") needs no enum-set type.
// This is what makes the UI domain-neutral: it reads the accepted roles
// from here instead of hardcoding which module types are containers.
+ // Return value must have static lifetime (string literal / static storage).
virtual const char* acceptsChildRoles() const { return ""; }📝 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
| // Comma-separated role names this module accepts as user-added children | |
| // (e.g. "effect,modifier"). "" = accepts none — the default, covering | |
| // leaf modules and fixed-shape containers. A container overrides this to | |
| // tell the UI's "+ add child" picker what to offer. Comma-separated | |
| // string (not a bitmask) so it serialises straight into /api/types and a | |
| // multi-role parent (Layer → "effect,modifier") needs no enum-set type. | |
| // This is what makes the UI domain-neutral: it reads the accepted roles | |
| // from here instead of hardcoding which module types are containers. | |
| virtual const char* acceptsChildRoles() const { return ""; } | |
| // Comma-separated role names this module accepts as user-added children | |
| // (e.g. "effect,modifier"). "" = accepts none — the default, covering | |
| // leaf modules and fixed-shape containers. A container overrides this to | |
| // tell the UI's "+ add child" picker what to offer. Comma-separated | |
| // string (not a bitmask) so it serialises straight into /api/types and a | |
| // multi-role parent (Layer → "effect,modifier") needs no enum-set type. | |
| // This is what makes the UI domain-neutral: it reads the accepted roles | |
| // from here instead of hardcoding which module types are containers. | |
| // Return value must have static lifetime (string literal / static storage). | |
| virtual const char* acceptsChildRoles() const { 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/MoonModule.h` around lines 202 - 210, The comment should document
that the virtual method acceptsChildRoles() returns a pointer whose lifetime
must be static because ModuleFactory::registerType<T>() stores that pointer in
static registry state; update the doc on acceptsChildRoles() (the virtual const
char* acceptsChildRoles() const) to state overrides must return static-lifetime
storage (e.g., string literals or static const char arrays) and must not return
pointers to temporary or stack-allocated buffers to avoid dangling pointers when
ModuleFactory::registerType<T>() saves the value.
Comment on lines
+886
to
890
| function isUserEditableChild(mod, depth) { | ||
| return depth > 0 | ||
| && mod.userEditable !== false | ||
| && allAcceptedChildRoles().has(mod.role); | ||
| } |
There was a problem hiding this comment.
Scope editability to the actual parent’s accepted roles.
isUserEditableChild() uses a global union of accepted roles. That marks a child editable when some container accepts its role, even if its own parent does not. This can show delete/replace/drag affordances on the wrong subtree.
Targeted fix
function isUserEditableChild(mod, depth) {
- return depth > 0
- && mod.userEditable !== false
- && allAcceptedChildRoles().has(mod.role);
+ if (depth <= 0 || mod.userEditable === false) return false;
+ const parent = findParent(mod.name);
+ if (!parent) return false;
+ return rolesAcceptedBy(parent).includes(mod.role);
}🤖 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/ui/app.js` around lines 886 - 890, isUserEditableChild currently uses
global allAcceptedChildRoles(), causing children to be editable if any container
accepts their role; change it to scope checks to the actual parent: update the
function (isUserEditableChild) to accept or resolve the immediate
parent/container of mod and check the parent's accepted-child roles (e.g.,
parent.acceptedChildRoles or parent.acceptedRoles set) for mod.role instead of
allAcceptedChildRoles(); keep the existing depth > 0 and mod.userEditable !==
false checks and fall back to the global allAcceptedChildRoles() only when the
parent cannot be determined.
…fault
Removes the last light-domain types from core (deleting core/types.h
entirely), surfaces the release channel on the device version, drops the
default grid to 16x16x1, and sharpens the project principles. The boundary
work is pure decoupling — same behaviour, fewer cross-domain dependencies.
KPI: tick:141556us(FPS:7) on esp32s3-n16r8 — within noise of prior s3
readings; every change here is off the render hot path (registration,
HTTP-serve, version string, type homes).
Core:
- Introduced BinaryBroadcaster (core interface): "broadcast these bytes to
all WS clients", domain-neutral. HttpServerModule implements it via
broadcastBinary() — the old broadcastPreviewFrame body minus all preview
specifics. Core no longer knows PreviewFrame, lengthType, or the wire
format; the preview producer pushes bytes to the interface instead of the
server polling a shared struct.
- Deleted core/types.h. Its symbols moved to their owners: lengthType +
nrOfLightsType + Dim → light/light_types.h (foundational light vocabulary,
no single owner); HEAP_RESERVE → platform.h (a platform memory constraint,
not Layer's); defaultGridSize → GridLayout.h; CoordCallback → Layouts.h;
PreviewFrame → light/PreviewFrame.h then folded into PreviewDriver.h.
- ModuleFactory: the dimensions() probe no longer names Dim — loosened to a
return-type-agnostic static_cast<uint8_t> check, so core captures a
module's dimensionality without depending on the light enum.
- platform.h now includes platform_config.h (it documents `if constexpr
(platform::hasOta)` in its own contract but relied on a transitive include).
- SystemModule: version control shows "semver (channel)" when the build
pipeline supplies a release tag (MM_RELEASE), e.g. "1.0.0-rc2 (latest)";
bare semver on local/dev builds. version buffer 16→32 to fit.
- MoonModule: documented that acceptsChildRoles() overrides must return
static-lifetime storage (the registry stores the pointer). [CodeRabbit]
Light domain:
- GridLayout: default grid 128x128 → 16x16x1 (defaultGridSize, now owned
here). A fresh device shows a manageable grid; users scale up.
- PreviewDriver: owns PreviewFrame + the 13-byte preview header; builds the
frame and pushes it to the BinaryBroadcaster (set in main.cpp). Replaces
the PreviewFrame::ready poll (flag removed).
- mm_main lost its gridW/gridH parameters — both callers passed
defaultGridSize over a field that already defaulted to it. Dead pass-through
removed; the composition roots no longer reference any light type.
Scripts / build:
- build_esp32.py: new --release flag → -DMM_RELEASE; generate_build_info.py
emits the MM_RELEASE #ifndef fallback + kRelease; esp32 CMakeLists forwards
it to the compile line.
- release.yml: build job resolves the channel tag (main→latest, vX.Y.Z→tag)
and passes --release; plus the latest-release-recreate step so a moving
`latest` build gets a fresh publishedAt.
UI:
- app.js: isUserEditableChild documents the role→container 1:1 assumption
behind the global accepted-roles check (vs parent-scoped). [CodeRabbit]
Tests:
- scenario_runner: honor width/height/depth in GridLayout add_module props —
they were silently ignored, so scenarios secretly ran at the construct
default. With the 16x16 default this surfaced as unmeasurable sub-microsecond
ticks; the fix makes props mean what they say.
- scenarios: the two that relied on the grid default (Layer_base_pipeline,
MirrorModifier_pipeline) now set 128x128 explicitly so the desktop tick is
above microsecond resolution; Layer_buildup keeps its deliberate 16x16
buildup but drops the fps>=1 floor on the sub-microsecond 16x16 steps
(unmeasurable on desktop; tick-contract + heap-delta remain the checks).
- unit_PreviewDriver: a RecordingBroadcaster verifies frames are produced AND
pushed (replacing the removed ready flag).
Docs / CI:
- CLAUDE.md § Principles: split "minimalism" into self-explaining rules —
"Core grows slower than the domain", "Default to subtraction", "No
duplication in code or docs"; scoped "Data over objects" to the hot path
with its rationale; removed "Build one capability at a time" (the history
shows commits land clusters).
- architecture.md: the data-exchange section now describes both shapes
(shared-struct pull, push-to-sink) domain-neutrally; the light instances
moved to § The pipeline.
- HttpServerModule.md / PreviewDriver.md / SystemModule.md / GridLayout.md /
coding-standards.md: updated for broadcastBinary, the preview-push wiring,
the version channel, the 16x16 default + corrected control ranges, and the
deleted PreviewFrame.h.
- decisions.md: carry-forward note on the boundary cleanup — "every tie was
incidental; check essential vs incidental before declaring core depends on X".
Reviews:
- 🐇 acceptsChildRoles() return-lifetime: fixed — documented the static-storage
contract (matches tags()); no bug today, all overrides return literals.
- 🐇 isUserEditableChild parent-scoping: accepted as-is — the role→container
mapping is 1:1, so the global accepted-roles check is exact for any tree the
engine produces; documented the assumption.
- 🐇 server-side acceptsChildRoles validation in add/replace handlers: declined
— the HTTP API is a trusted same-device surface and addChild already rejects
bad placement; acceptsChildRoles is a UI affordance by design, not an
engine-enforced constraint.
KPI Details:
Desktop:
Lights: 16,384
Binary: 376 KB
[doctest] 207 cases, 207 passed
tick: 359/101/315/1/103/103/44/34us (FPS: 2785/9900/3174/1000000/9708/9708/22727/29411)
=== 10 scenario(s), 10 passed, 0 failed ===
Platform boundary: PASS
Specs: 26 modules, 26 ok
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm):
Image: 1,310,285 bytes (69% partition free)
tick: 141556us (FPS: 7) heap free: 8367991
Code:
62 source files (10733 lines)
38 test files (5437 lines)
Lizard: 40 warnings
Co-Authored-By: Claude Opus 4.8 (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 (2)
src/core/HttpServerModule.cpp (1)
666-681:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
userEditable()in delete/replace handlers.
userEditable:falseis currently UI-only; API clients can still delete/replace protected children directly.Proposed fix
void HttpServerModule::handleDeleteModule(platform::TcpConnection& conn, const char* moduleName) { auto* mod = findModuleByName(moduleName); if (!mod) { sendResponse(conn, 404, "application/json", "{\"error\":\"module not found\"}"); return; } + if (!mod->userEditable()) { + sendResponse(conn, 400, "application/json", "{\"error\":\"module is not user-editable\"}"); + return; + } auto* parent = mod->parent(); if (!parent) { sendResponse(conn, 400, "application/json", "{\"error\":\"cannot delete top-level module\"}"); return; @@ void HttpServerModule::handleReplaceModule(platform::TcpConnection& conn, const char* moduleName, const char* body) { auto* mod = findModuleByName(moduleName); if (!mod) { sendResponse(conn, 404, "application/json", "{\"error\":\"module not found\"}"); return; } + if (!mod->userEditable()) { + sendResponse(conn, 400, "application/json", "{\"error\":\"module is not user-editable\"}"); + return; + } auto* parent = mod->parent(); if (!parent) { sendResponse(conn, 400, "application/json", "{\"error\":\"top-level modules cannot be replaced\"}"); return; }Also applies to: 704-714
🤖 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 666 - 681, In handleDeleteModule (HttpServerModule::handleDeleteModule) and the corresponding replace handler (HttpServerModule::handleReplaceModule), enforce the module's userEditable flag by checking mod->userEditable() before allowing delete/replace and return an appropriate error (e.g., 403 with JSON payload) if it is false; locate the checks around parent() handling and add the userEditable() guard there so protected modules cannot be removed or replaced via the API.docs/moonmodules/core/HttpServerModule.md (1)
16-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new mutation-policy fields in REST wire shapes.
The REST contract list still omits the new fields introduced by this PR:
acceptsChildRolesin/api/typesand optionaluserEditable:falsein/api/state.Suggested doc patch
GET /api/state → full module tree JSON: each entry carries name, type, role, enabled, loopTimeUs, classSize, dynamicBytes, controls[], status + severity (only when set by the - module; severity ∈ status/warning/error) + module; severity ∈ status/warning/error), + plus optional userEditable:false when + a module opts out of UI delete/replace GET /api/types → {types:[{name, displayName, role, - docPath, tags, dim, defaults}]} + docPath, tags, dim, + acceptsChildRoles, defaults}]}Based on learnings: “Module specs (
docs/moonmodules/*.md) … include wire contracts (REST URLs, JSON shapes, …).”🤖 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/moonmodules/core/HttpServerModule.md` around lines 16 - 24, Update the REST wire contract documentation to include the two new mutation-policy fields: add "acceptsChildRoles" to the /api/types response shape inside each type object (alongside name, displayName, role, docPath, tags, dim, defaults) and document the optional "userEditable" boolean on entries returned by /api/state (noting it may be omitted and can be false); ensure the JSON shapes/examples and any descriptions mention the field names, types, and that userEditable defaults to true when absent and can explicitly be false, and add these fields to any example payloads shown for GET /api/types and GET /api/state so consumers see the expected wire format.
🤖 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 @.github/workflows/release.yml:
- Around line 92-103: The "Resolve release tag" step (id: tag) duplicates
tag-resolution logic; extract the if/elif/else logic that reads INPUT_TAG,
REF_NAME and IS_MAIN into a single shared reusable workflow or composite action
that emits an output "tag", then replace this step with a call to that reusable
component (or uses: ./path/to/action) and wire its output to the rest of the
job; ensure the shared component sets the "tag" output the same way and
preserves the environment variables INPUT_TAG, REF_NAME and IS_MAIN so callers
(previously relying on step id: tag) continue to receive the same output without
duplicated logic.
In `@src/light/drivers/PreviewDriver.h`:
- Around line 180-182: pushFrame() currently dereferences frame_ without
null-check which can cause a crash if called before setPreviewFrame(); add a
guard at the start of pushFrame() to return early when frame_ is null (in
addition to the existing checks for broadcaster_, frame_->data, and
frame_->dataLen) so the method safely exits if frame_ is not yet set; update the
condition that currently checks broadcaster_ and frame_->data/Len to also check
frame_ (or add an explicit if (!frame_) return) to avoid dereferencing frame_.
In `@src/light/light_types.h`:
- Around line 23-26: The comment for nrOfLightsType is misleading about
MappingLUT not existing; update the comment to clarify that MappingLUT exists
but the size optimization (using smaller coordinate/index types to reduce
MappingLUT memory) is not yet implemented. Reference platform::hasPsram and
nrOfLightsType in the updated comment and explicitly state that with PSRAM we
use uint32_t, otherwise uint16_t to keep MappingLUT smaller until the
smaller-coordinate optimization is implemented in MappingLUT.
In `@test/scenario_runner.cpp`:
- Around line 278-287: The code writes JSON numbers from props directly into
GridLayout dimensions (mod cast to mm::GridLayout, assigning to
width/height/depth of type mm::lengthType) without validation; add checks before
each assignment: for each props.has("width"/"height"/"depth") ensure the JSON
value is numeric, within std::numeric_limits<mm::lengthType>::min()/max(), and
represents an integer if mm::lengthType is integral (e.g., compare floor/ceil),
then perform the static_cast to mm::lengthType; on invalid input emit a clear
error (throw std::runtime_error or assert/fail the test) instead of silently
assigning malformed values.
In `@test/scenarios/light/scenario_Layer_base_pipeline.json`:
- Around line 22-30: The scenario description text for the Grid module is
inconsistent with the configured workload: the module with id "Grid" and type
"GridLayout" is configured with props width:128 and height:128 but the
human-readable description and the referenced "add-artnet" FPS context still
mention a 16x16 grid; update the description (and any "add-artnet" text that
refers to a 16x16 FPS context) to reflect the 128x128 grid so the description
matches the configured workload, or alternatively change the GridLayout props
back to width:16 and height:16 if you intend to keep the 16x16 description —
ensure edits target the "description" field for the "Grid" module and any
"add-artnet" scenario text that references the grid size.
In `@test/scenarios/light/scenario_Layer_buildup.json`:
- Line 55: The description string in scenario_Layer_buildup.json currently
claims "the tick contract + heap delta are the meaningful checks" but the step
uses measure-minimum which does not define any heap bound; update the
"description" field for this step to accurately reflect the assertions (e.g.,
remove the "heap delta" claim or state that only the tick contract is checked),
or alternatively modify the measure-minimum configuration to include an explicit
heap bound if you intend to assert heap delta; locate the JSON step's
"description" key and adjust the text or the measure-minimum block accordingly.
In `@test/unit/light/unit_PreviewDriver.cpp`:
- Around line 18-29: The test currently only verifies
RecordingBroadcaster::calls > 0; extend the assertions in the tests that use
RecordingBroadcaster by checking the wire-shape: assert that the broadcastBinary
invocation produced exactly two chunks (chunkCount == 2) and that
RecordingBroadcaster::lastTotalLen equals the expected header+data size (13 +
frame.dataLen). Update the expectations wherever RecordingBroadcaster is used
(e.g., the tests that rely on calls/lastTotalLen) so they validate both
chunkCount and lastTotalLen against the frame's dataLen to catch header/chunk
regressions.
---
Outside diff comments:
In `@docs/moonmodules/core/HttpServerModule.md`:
- Around line 16-24: Update the REST wire contract documentation to include the
two new mutation-policy fields: add "acceptsChildRoles" to the /api/types
response shape inside each type object (alongside name, displayName, role,
docPath, tags, dim, defaults) and document the optional "userEditable" boolean
on entries returned by /api/state (noting it may be omitted and can be false);
ensure the JSON shapes/examples and any descriptions mention the field names,
types, and that userEditable defaults to true when absent and can explicitly be
false, and add these fields to any example payloads shown for GET /api/types and
GET /api/state so consumers see the expected wire format.
In `@src/core/HttpServerModule.cpp`:
- Around line 666-681: In handleDeleteModule
(HttpServerModule::handleDeleteModule) and the corresponding replace handler
(HttpServerModule::handleReplaceModule), enforce the module's userEditable flag
by checking mod->userEditable() before allowing delete/replace and return an
appropriate error (e.g., 403 with JSON payload) if it is false; locate the
checks around parent() handling and add the userEditable() guard there so
protected modules cannot be removed or replaced via the API.
🪄 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
Run ID: 84931871-bc34-4867-a243-6d0237bab2d1
⛔ Files ignored due to path filters (2)
scripts/build/build_esp32.pyis excluded by!**/build/**scripts/build/generate_build_info.pyis excluded by!**/build/**
📒 Files selected for processing (41)
.github/workflows/release.ymlCLAUDE.mddocs/architecture.mddocs/coding-standards.mddocs/history/decisions.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/layouts/GridLayout.mdesp32/main/CMakeLists.txtesp32/main/main.cppsrc/core/BinaryBroadcaster.hsrc/core/Control.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/PreviewFrame.hsrc/core/SystemModule.hsrc/core/types.hsrc/light/drivers/PreviewDriver.hsrc/light/effects/EffectBase.hsrc/light/layers/Buffer.hsrc/light/layers/Layer.hsrc/light/layers/MappingLUT.hsrc/light/layouts/GridLayout.hsrc/light/layouts/Layouts.hsrc/light/light_types.hsrc/light/modifiers/ModifierBase.hsrc/main.cppsrc/platform/desktop/main_desktop.cppsrc/platform/platform.hsrc/ui/app.jstest/scenario_runner.cpptest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layer_buildup.jsontest/scenarios/light/scenario_Layer_memory_1to1.jsontest/scenarios/light/scenario_MirrorModifier_memory_lut.jsontest/scenarios/light/scenario_MirrorModifier_pipeline.jsontest/unit/light/unit_PreviewDriver.cpp
💤 Files with no reviewable changes (2)
- src/core/PreviewFrame.h
- src/core/types.h
Comment on lines
+92
to
+103
| - name: Resolve release tag | ||
| id: tag | ||
| env: | ||
| INPUT_TAG: ${{ inputs.tag }} | ||
| REF_NAME: ${{ github.ref_name }} | ||
| IS_MAIN: ${{ github.ref == 'refs/heads/main' }} | ||
| run: | | ||
| set -euo pipefail | ||
| if [ -n "$INPUT_TAG" ]; then echo "tag=$INPUT_TAG" >> "$GITHUB_OUTPUT" | ||
| elif [ "$IS_MAIN" = "true" ]; then echo "tag=latest" >> "$GITHUB_OUTPUT" | ||
| else echo "tag=$REF_NAME" >> "$GITHUB_OUTPUT"; fi | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Deduplicate release-tag resolution to one source of truth.
Line 92-Line 103 duplicates the tag-resolution policy already implemented in the release job. Keep this policy in one shared script/composite action to prevent drift between the tag burned into firmware and the tag used for publishing.
Based on learnings: "No duplication in code or docs; same logic in two places belongs in one shared function; same fact in two docs belongs in one place with links from elsewhere".
🤖 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 @.github/workflows/release.yml around lines 92 - 103, The "Resolve release
tag" step (id: tag) duplicates tag-resolution logic; extract the if/elif/else
logic that reads INPUT_TAG, REF_NAME and IS_MAIN into a single shared reusable
workflow or composite action that emits an output "tag", then replace this step
with a call to that reusable component (or uses: ./path/to/action) and wire its
output to the rest of the job; ensure the shared component sets the "tag" output
the same way and preserves the environment variables INPUT_TAG, REF_NAME and
IS_MAIN so callers (previously relying on step id: tag) continue to receive the
same output without duplicated logic.
Comment on lines
+278
to
+287
| } else if (std::strcmp(type, "GridLayout") == 0) { | ||
| // Grid dimensions set at construct time (the fixture phase runs | ||
| // before the scheduler starts, so set_control can't apply them | ||
| // yet). Without this, props.width/height were silently ignored | ||
| // and the grid stayed at GridLayout's default — masking the real | ||
| // scenario size. | ||
| auto* grid = static_cast<mm::GridLayout*>(mod); | ||
| if (props.has("width")) grid->width = static_cast<mm::lengthType>(props["width"].num); | ||
| if (props.has("height")) grid->height = static_cast<mm::lengthType>(props["height"].num); | ||
| if (props.has("depth")) grid->depth = static_cast<mm::lengthType>(props["depth"].num); |
There was a problem hiding this comment.
Validate GridLayout props before casting to lengthType.
Line 285-Line 287 writes raw JSON numbers straight into dimensions with no type/range/integer checks. Malformed fixture values can silently change workload size and invalidate scenario measurements instead of failing fast.
Proposed fix
} else if (std::strcmp(type, "GridLayout") == 0) {
// Grid dimensions set at construct time (the fixture phase runs
// before the scheduler starts, so set_control can't apply them
// yet). Without this, props.width/height were silently ignored
// and the grid stayed at GridLayout's default — masking the real
// scenario size.
auto* grid = static_cast<mm::GridLayout*>(mod);
- if (props.has("width")) grid->width = static_cast<mm::lengthType>(props["width"].num);
- if (props.has("height")) grid->height = static_cast<mm::lengthType>(props["height"].num);
- if (props.has("depth")) grid->depth = static_cast<mm::lengthType>(props["depth"].num);
+ auto parseDim = [&](const char* key, mm::lengthType& out) -> bool {
+ if (!props.has(key)) return true;
+ const JsonVal& v = props[key];
+ if (v.type != JsonVal::Number) return false;
+ const int dim = static_cast<int>(v.num);
+ if (dim < 1 || dim > 512 || static_cast<double>(dim) != v.num) return false;
+ out = static_cast<mm::lengthType>(dim);
+ return true;
+ };
+ if (!parseDim("width", grid->width) ||
+ !parseDim("height", grid->height) ||
+ !parseDim("depth", grid->depth)) {
+ std::printf(" SKIP %s (invalid GridLayout props)\n", id);
+ return;
+ }
}🧰 Tools
🪛 Clang (14.0.6)
[note] 278-278: +1, nesting level increased to 2
(clang)
[note] 285-285: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 286-286: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 287-287: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[warning] 284-284: do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead
(cppcoreguidelines-pro-type-static-cast-downcast)
[warning] 285-285: statement should be inside braces
(readability-braces-around-statements)
[warning] 286-286: statement should be inside braces
(readability-braces-around-statements)
[warning] 287-287: statement should be inside braces
(readability-braces-around-statements)
🤖 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/scenario_runner.cpp` around lines 278 - 287, The code writes JSON
numbers from props directly into GridLayout dimensions (mod cast to
mm::GridLayout, assigning to width/height/depth of type mm::lengthType) without
validation; add checks before each assignment: for each
props.has("width"/"height"/"depth") ensure the JSON value is numeric, within
std::numeric_limits<mm::lengthType>::min()/max(), and represents an integer if
mm::lengthType is integral (e.g., compare floor/ceil), then perform the
static_cast to mm::lengthType; on invalid input emit a clear error (throw
std::runtime_error or assert/fail the test) instead of silently assigning
malformed values.
… only)
Adds a hollow SphereLayout and reworks the light pipeline so a sparse layout
drives only its REAL lights — not its dense bounding box — through the driver
buffer (ArtNet), and shows them at their true 3D positions in the preview. The
preview becomes a point list (coordinates sent once, RGB per frame) instead of
a dense voxel grid, so spheres/rings/arbitrary maps render in their real shape
and per-frame data shrinks to the lights that exist. Also surfaces and fixes a
latent out-of-bounds write and a preview-drops-the-WS bug, both found on
hardware.
KPI: tick:105064us(FPS:9) on esp32s3-n16r8 at 128x128 + ArtNet + an active
preview client — the heavy case. The sparse-LUT rework is grid-short-circuited
(dense grids keep the memcpy fast path), so the render hot path is unchanged.
Light domain:
- SphereLayout (new): a hollow lattice sphere — lights on the surface shell
only (radius control, 1-64). lightCount() and forEachCoord() share one
squared-integer band predicate so they never disagree.
- Layer::rebuildLUT: the MappingLUT now ALWAYS maps grid cell -> driver index
for sparse layouts (no-modifier and modifier paths), so the driver/output
buffer holds exactly the real lights (a radius-4 sphere => 210, not its 729
box). Dense grids short-circuit to setIdentity + memcpy unchanged. New
helpers buildBoxToDriver / buildSparseIdentityLUT (cold-path, scratch freed).
This also fixes a latent out-of-bounds write: a sphere + modifier previously
emitted box indices (0..728) into a 210-light buffer.
- PreviewDriver: rewritten to a point-list protocol. Two WS messages — 0x03
coordinate table (sent on every LUT rebuild + ~1Hz keepalive; positions
1 byte/axis, scaled to fit when a box exceeds 255/axis) and 0x02 per-frame
RGB indexed by driver light. Reads the sparse driver buffer flat by index;
index-downsamples (stride) to fit the send budget. Removed the PreviewFrame
struct and the detail/decompress controls.
- light_types.h: nrOfLightsType comment clarified (MappingLUT exists; the
index-width optimisation is what it documents). [CodeRabbit]
Core:
- platform_esp32 TcpConnection::writeChunks: on a partial write, drain the
unsent tail with a bounded retry (~8ms cap) instead of dropping the
connection. Fixes the preview WebSocket closing ~0.8s after connect at
128x128 — a ~5KB frame partial-wrote under ArtNet backpressure and the old
code closed on any partial. lwIP exposes no free-TX-space query, so finishing
the write is the way to keep the stream intact. Verified: WS stable, no FPS
regression (drain completes in us, not the cap).
UI:
- app.js: parses the 0x03 coordinate table into cached point positions and
colours each point from the 0x02 RGB stream — true-shape point cloud, no
dense-grid math, no decompress.
- Header: show largest internal-RAM block alongside free heap (🧠 free / 🧱
block); hamburger hidden on wide screens where the nav is a static column
(style.css), shown only as the <820px drawer toggle.
Main / wiring:
- main.cpp: PreviewDriver pushes to the HTTP server's BinaryBroadcaster (no
external frame). ArtNet + Preview drivers markWiredByCode — persistence was
replacing the broadcaster-wired PreviewDriver with a fresh instance (verified
on device: no broadcaster => blank preview). Same protection Board/Improv use.
Scripts / tests:
- scenario_runner: new remove_module / replace_module ops (mirror the HTTP
delete/replace handlers); GridLayout width/height/depth props honoured on
add_module (were silently ignored, masking the real scenario size).
- unit_SphereLayout (shell geometry, hollow, count==iterator, symmetry),
unit_Layouts_mutation (add/replace/remove/multiple layouts),
unit_Layer_sparse_mapping (driver buffer = real lights, LUT indices < N —
pins the overflow fix), unit_PreviewDriver reshaped to the point-list wire
format. scenario_Layouts_mutation (live add/replace/remove). Removed
scenario_PreviewDriver_detail (detail/decompress controls gone).
- Two scenarios that relied on the grid default now set 128x128 explicitly so
the desktop tick is above microsecond resolution.
Docs:
- SphereLayout.md (new); PreviewDriver.md rewritten for the two-message
protocol + MoonLight prior art; architecture.md data-exchange section notes
the push-to-sink shape; plan.md backlog updated (per-layout offsets,
full-density interpolated preview) and the implemented preview item removed.
Reviews:
- 👾 Reviewer (Opus, whole diff): no blockers; sparse-LUT correctness + hot
path verified clean. One should-fix applied — clampAxis silently flattened
coordinates >255; replaced with scaleAxis (aspect-preserving scale so large
grids preview at true proportions). Three nits accepted as no-action.
- 🐇 light_types.h MappingLUT comment: fixed (above).
- 🐇 scenario_Layer_base_pipeline / Layer_buildup descriptions: fixed to match
the 128x128 grid and the actual asserted bounds.
- 🐇 PreviewDriver pushFrame null-check / RecordingBroadcaster wire-shape:
stale — that code was replaced by the point-list rewrite; the new
CaptureBroadcaster already asserts the wire shape.
- 🐇 scenario_runner GridLayout prop numeric validation: declined — test code
reading author-written scenario JSON (trusted); a bad size fails the assertion.
- 🐇 release.yml duplicate tag-resolution -> composite action: declined — ~5
lines across two jobs; a composite action is more machinery than it saves.
- 🐇 HttpServerModule.md document acceptsChildRoles/userEditable: declined —
already documented in ui.md (the canonical wire-contract doc).
- 🐇 handleDelete/Replace enforce userEditable (403): declined — userEditable is
a UI affordance by design (PreviewDriver's own comment says so); the API is
the trusted same-device surface.
KPI Details:
Desktop:
Lights: 16,384
Binary: 377 KB
[doctest] 219 cases, 219 passed (4945 assertions)
tick: 359/103/57/1/105/103/45/35us (per scenario)
=== 9 scenario(s), 9 passed, 0 failed ===
Platform boundary: PASS
Specs: 27 modules, 27 ok
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8, 128x128 + ArtNet + live preview):
tick: 105064us (FPS: 9) — preview WS stable, HttpServer ~950us (drain not blocking)
All 4 firmwares build clean.
Code:
63 source files (10944 lines)
41 test files (5775 lines)
Lizard: 42 warnings
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/scenario_runner.cpp (1)
280-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate GridLayout
propsbefore casting tomm::lengthType.Line 280-Line 282 writes raw JSON numeric values directly into dimensions with no integer/range/type validation. Malformed fixture values can be silently truncated and invalidate scenario intent instead of failing fast.
Proposed fix
} else if (std::strcmp(type, "GridLayout") == 0) { auto* grid = static_cast<mm::GridLayout*>(mod); - if (props.has("width")) grid->width = static_cast<mm::lengthType>(props["width"].num); - if (props.has("height")) grid->height = static_cast<mm::lengthType>(props["height"].num); - if (props.has("depth")) grid->depth = static_cast<mm::lengthType>(props["depth"].num); + auto parseDim = [&](const char* key, mm::lengthType& out) -> bool { + if (!props.has(key)) return true; + const JsonVal& v = props[key]; + if (v.type != JsonVal::Number) return false; + const int dim = static_cast<int>(v.num); + if (dim < 1 || dim > 512 || static_cast<double>(dim) != v.num) return false; + out = static_cast<mm::lengthType>(dim); + return true; + }; + if (!parseDim("width", grid->width) || + !parseDim("height", grid->height) || + !parseDim("depth", grid->depth)) { + std::printf(" SKIP %s (invalid GridLayout props)\n", id); + return; + } }As per coding guidelines,
GridLayoutcontrols have documented integer bounds (width/height/depthrange 1–512).🤖 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/scenario_runner.cpp` around lines 280 - 282, The code writes raw JSON numerics from props into grid->width/height/depth using static_cast to mm::lengthType without validation; update the assignment sites that touch grid->width, grid->height, and grid->depth to validate props["..."] is numeric and an integer, then check the value is within the documented bounds (1–512) before casting and assigning to mm::lengthType; if validation fails, fail fast (throw an exception or return an error) with a clear message including the prop name and invalid value so malformed fixture values don't silently truncate or corrupt the GridLayout.
🤖 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/platform/esp32/platform_esp32.cpp`:
- Around line 691-731: The writeChunks() implementation currently performs a
bounded retry with vTaskDelay, turning the documented non-blocking write into a
blocking multi-ms operation; instead, remove the retry/sleep logic from
writeChunks() and have it return WouldBlock (or Partial as per platform.h)
immediately when lwip_write() would block, leaving tail-drain work to a separate
mechanism (e.g., a per-connection pending buffer and a drain task or the
connection's poll/tx-ready handler). Concretely, modify writeChunks() (in
platform_esp32::writeChunks or the function that uses fd_) to stop looping and
sleeping on EAGAIN/EWOULDBLOCK and return the non-blocking indicator, then
implement a connection-local pending buffer and a drain routine invoked from the
existing socket-ready/poll path (or a background drain task) to finish sending
the remaining bytes; ensure HttpServerModule::broadcastBinary() and platform.h
semantics are preserved by keeping writeChunks() non-blocking.
In `@test/scenarios/light/scenario_Layer_base_pipeline.json`:
- Line 63: Update the step description string so it uses non-gating language for
the min_pct metric—change wording like "must stay at >=80% of the rated FPS" to
something like "target" or "expected" (e.g., "expected to stay at >=80% of the
rated FPS") to reflect that bounds.fps.min_pct is warning-only; locate the step
in scenario_Layer_base_pipeline.json that mentions min_pct and modify the
description accordingly.
In `@test/unit/light/unit_Layouts_mutation.cpp`:
- Line 49: Update the misleading comment on the SphereLayout radius line: where
s.radius = 1 currently comments "18-light shell, indices 4..21", change it to
reflect that SphereLayout with radius=1 emits exactly the 6 axis neighbors
(6-light shell) — e.g., "6-light shell (axis neighbors), indices ...", and
ensure the comment references SphereLayout and s.radius so reviewers can verify
the expected 6 points.
In `@test/unit/light/unit_SphereLayout.cpp`:
- Around line 57-64: The test currently asserts 18 points for mm::SphereLayout
with s.radius = 1 but the spec says radius=1 should emit only the 6 axis
neighbors; update the unit test (unit_SphereLayout.cpp's TEST_CASE using
mm::SphereLayout and collectPoints) to expect 6 points instead of 18 and adjust
the comment/assertions to verify only the 6 axis neighbors are present (and that
the centre (1,1,1) is not emitted); if after changing the test it fails, modify
the SphereLayout implementation to ensure radius=1 yields exactly the six
axis-neighbor points.
---
Duplicate comments:
In `@test/scenario_runner.cpp`:
- Around line 280-282: The code writes raw JSON numerics from props into
grid->width/height/depth using static_cast to mm::lengthType without validation;
update the assignment sites that touch grid->width, grid->height, and
grid->depth to validate props["..."] is numeric and an integer, then check the
value is within the documented bounds (1–512) before casting and assigning to
mm::lengthType; if validation fails, fail fast (throw an exception or return an
error) with a clear message including the prop name and invalid value so
malformed fixture values don't silently truncate or corrupt the GridLayout.
🪄 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
Run ID: ae5963fa-d5d4-4591-b163-ea88bdeb232b
📒 Files selected for processing (24)
docs/architecture.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/layouts/SphereLayout.mddocs/plan.mdsrc/light/drivers/PreviewDriver.hsrc/light/layers/Layer.hsrc/light/layouts/SphereLayout.hsrc/light/light_types.hsrc/main.cppsrc/platform/esp32/platform_esp32.cppsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_GridLayout_grid_sizes.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layer_buildup.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_PreviewDriver_detail.jsontest/unit/light/unit_Layer_sparse_mapping.cpptest/unit/light/unit_Layouts_mutation.cpptest/unit/light/unit_PreviewDriver.cpptest/unit/light/unit_SphereLayout.cpp
💤 Files with no reviewable changes (1)
- test/scenarios/light/scenario_PreviewDriver_detail.json
👮 Files not reviewed due to content moderation or server errors (8)
- src/light/layers/Layer.h
- docs/architecture.md
- src/ui/app.js
- src/light/drivers/PreviewDriver.h
- src/main.cpp
- docs/moonmodules/light/drivers/PreviewDriver.md
- docs/plan.md
- src/ui/style.css
Comment on lines
+691
to
+731
| // Partial: some bytes of this WS frame went out, so we MUST finish the rest | ||
| // — a half-sent frame corrupts the stream and the caller would otherwise | ||
| // drop the connection. This happens under backpressure when the link is | ||
| // saturated (e.g. ArtNet + a large preview frame on a slow 128×128 tick): | ||
| // the lwIP send buffer can't take the whole frame at once but drains in | ||
| // microseconds. Drain the unsent tail with a bounded retry loop; only if it | ||
| // still can't complete (a genuinely stuck socket) do we report Partial so | ||
| // the caller closes. lwIP exposes no free-TX-space query (SO_SNDBUF is | ||
| // unimplemented), so a pre-check isn't possible — finishing the write is the | ||
| // way to keep the stream intact without dropping. | ||
| size_t sent = static_cast<size_t>(n); | ||
| // Small cap: a partial tail (a few KB) drains in 1-2 ms as TCP ACKs arrive; | ||
| // 8 ms is plenty for a transient. A genuinely saturated link blows past it — | ||
| // then we give up (report Partial → caller closes, browser reconnects), | ||
| // rather than stall the render tick. Bounds the worst-case tick hit to ~8 ms. | ||
| constexpr int kMaxDrainTries = 8; // each yields up to 1ms | ||
| for (int tries = 0; sent < total && tries < kMaxDrainTries; ) { | ||
| // Locate the chunk + offset where `sent` lands, write its remaining tail. | ||
| size_t acc = 0; | ||
| const uint8_t* p = nullptr; size_t remain = 0; | ||
| for (int i = 0; i < count; i++) { | ||
| if (sent < acc + chunks[i].len) { | ||
| size_t off = sent - acc; | ||
| p = chunks[i].data + off; | ||
| remain = chunks[i].len - off; | ||
| break; | ||
| } | ||
| acc += chunks[i].len; | ||
| } | ||
| if (!p) break; // shouldn't happen (sent < total) | ||
| ssize_t w = lwip_write(fd_, p, remain); | ||
| if (w > 0) { | ||
| sent += static_cast<size_t>(w); | ||
| } else if (w < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { | ||
| vTaskDelay(pdMS_TO_TICKS(1)); // buffer full — let it drain | ||
| tries++; | ||
| } else { | ||
| return WriteResult::Error; // real socket error | ||
| } | ||
| } | ||
| return (sent == total) ? WriteResult::Complete : WriteResult::Partial; |
There was a problem hiding this comment.
Keep writeChunks() non-blocking.
This retry loop turns a documented non-blocking one-shot write into a potentially multi-millisecond blocking path. HttpServerModule::broadcastBinary() relies on WouldBlock to skip a frame so a slow browser cannot stall the preview/render path; sleeping here instead pushes that backpressure directly onto the hot path and breaks the platform.h contract. The fix needs to move tail completion out of writeChunks() (for example, a connection-local pending buffer drained elsewhere) rather than waiting in this call.
As per coding guidelines, "In the render loop and anything it calls: no blocking" and "src/platform/**: Verify implementations match the API declared in platform.h`."
🧰 Tools
🪛 Clang (14.0.6)
[warning] 701-701: use auto when initializing with a cast to avoid duplicating the type name
(modernize-use-auto)
[warning] 710-710: variable name 'p' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 720-720: implicit conversion 'const uint8_t *' (aka 'const unsigned char *') -> bool
(readability-implicit-bool-conversion)
[warning] 720-720: statement should be inside braces
(readability-braces-around-statements)
[warning] 721-721: variable name 'w' is too short, expected at least 3 characters
(readability-identifier-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/platform/esp32/platform_esp32.cpp` around lines 691 - 731, The
writeChunks() implementation currently performs a bounded retry with vTaskDelay,
turning the documented non-blocking write into a blocking multi-ms operation;
instead, remove the retry/sleep logic from writeChunks() and have it return
WouldBlock (or Partial as per platform.h) immediately when lwip_write() would
block, leaving tail-drain work to a separate mechanism (e.g., a per-connection
pending buffer and a drain task or the connection's poll/tx-ready handler).
Concretely, modify writeChunks() (in platform_esp32::writeChunks or the function
that uses fd_) to stop looping and sleeping on EAGAIN/EWOULDBLOCK and return the
non-blocking indicator, then implement a connection-local pending buffer and a
drain routine invoked from the existing socket-ready/poll path (or a background
drain task) to finish sending the remaining bytes; ensure
HttpServerModule::broadcastBinary() and platform.h semantics are preserved by
keeping writeChunks() non-blocking.
| mm::GridLayout g; | ||
| g.width = 4; g.height = 1; g.depth = 1; // 4 lights, indices 0..3 | ||
| mm::SphereLayout s; | ||
| s.radius = 1; // 18-light shell, indices 4..21 |
There was a problem hiding this comment.
Correct the radius=1 SphereLayout light-count comment.
Line 49 says 18-light shell, but the spec for radius=1 is 6 shell points. The test code is fine; this comment is misleading and should be corrected.
As per coding guidelines: for radius=1, SphereLayout emits exactly the 6 axis neighbors of the centre.
🤖 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/light/unit_Layouts_mutation.cpp` at line 49, Update the misleading
comment on the SphereLayout radius line: where s.radius = 1 currently comments
"18-light shell, indices 4..21", change it to reflect that SphereLayout with
radius=1 emits exactly the 6 axis neighbors (6-light shell) — e.g., "6-light
shell (axis neighbors), indices ...", and ensure the comment references
SphereLayout and s.radius so reviewers can verify the expected 6 points.
Comment on lines
+57
to
+64
| // radius = 1 is the smallest hollow sphere: the 6 axis neighbours (d^2=1) plus | ||
| // the 12 edge points (d^2=2) of the centre — 18 lights, no centre. | ||
| TEST_CASE("SphereLayout radius 1 is the 18-point base shell") { | ||
| mm::SphereLayout s; | ||
| s.radius = 1; | ||
| auto pts = collectPoints(s); | ||
| CHECK(pts.size() == 18); | ||
| // All within the 3x3x3 box (coords 0..2), none at the centre (1,1,1). |
There was a problem hiding this comment.
Align the radius=1 base-case expectation with the SphereLayout spec.
This test asserts 18 points for radius=1, but the documented contract says radius=1 emits exactly the 6 axis neighbors (no centre/interior points). This mismatch can lock in incorrect behavior.
Proposed fix
-// radius = 1 is the smallest hollow sphere: the 6 axis neighbours (d^2=1) plus
-// the 12 edge points (d^2=2) of the centre — 18 lights, no centre.
+// radius = 1 is the smallest hollow sphere: exactly the 6 axis neighbours of
+// the centre, with no centre/interior points.
TEST_CASE("SphereLayout radius 1 is the 18-point base shell") {
mm::SphereLayout s;
s.radius = 1;
auto pts = collectPoints(s);
- CHECK(pts.size() == 18);
+ CHECK(pts.size() == 6);As per coding guidelines, docs/moonmodules/light/layouts/SphereLayout.md states: “For radius=1, the emitted shell points are exactly the 6 axis neighbors of the centre.”
📝 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
| // radius = 1 is the smallest hollow sphere: the 6 axis neighbours (d^2=1) plus | |
| // the 12 edge points (d^2=2) of the centre — 18 lights, no centre. | |
| TEST_CASE("SphereLayout radius 1 is the 18-point base shell") { | |
| mm::SphereLayout s; | |
| s.radius = 1; | |
| auto pts = collectPoints(s); | |
| CHECK(pts.size() == 18); | |
| // All within the 3x3x3 box (coords 0..2), none at the centre (1,1,1). | |
| // radius = 1 is the smallest hollow sphere: exactly the 6 axis neighbours of | |
| // the centre, with no centre/interior points. | |
| TEST_CASE("SphereLayout radius 1 is the 18-point base shell") { | |
| mm::SphereLayout s; | |
| s.radius = 1; | |
| auto pts = collectPoints(s); | |
| CHECK(pts.size() == 6); | |
| // All within the 3x3x3 box (coords 0..2), none at the centre (1,1,1). |
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 59-59: The function 'DOCTEST_ANON_FUNC_17' is never used.
(unusedFunction)
[style] 59-59: The function 'DOCTEST_ANON_FUNC_2' 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/light/unit_SphereLayout.cpp` around lines 57 - 64, The test
currently asserts 18 points for mm::SphereLayout with s.radius = 1 but the spec
says radius=1 should emit only the 6 axis neighbors; update the unit test
(unit_SphereLayout.cpp's TEST_CASE using mm::SphereLayout and collectPoints) to
expect 6 points instead of 18 and adjust the comment/assertions to verify only
the 6 axis neighbors are present (and that the centre (1,1,1) is not emitted);
if after changing the test it fails, modify the SphereLayout implementation to
ensure radius=1 yields exactly the six axis-neighbor points.
… fixes
Adds a Conway Game of Life effect, gzips the embedded web UI so the firmware
carries it ~3.5x smaller (browser inflates natively), splits the 3D WebGL
preview out of app.js into its own module, and applies a round of review
fixes. Four self-contained changes landed together.
KPI: ESP32 tick:24576us(FPS:40) on esp32s3-n16r8 — measured at the device's
current grid running Metaballs, NOT the 128x128 heavy case the previous commit
reported (FPS 9). Different measured configuration, not a regression; well
above the FPS×lights floor. The render hot path is otherwise unchanged.
Light domain:
- GameOfLifeEffect (new): Conway B3/S23 on the XY plane (D2; extrude fills z).
Two width×height byte grids (cur/nxt), PSRAM-first via platform::alloc like
FireEffect, (re)allocated in onBuildState, freed in teardown. Controls:
seed, wraparound, hue, bpm. bpm time-gates the generation rate (≈bpm/8
gen/s) so speed is independent of frame rate; the grid is re-painted EVERY
frame (the Layer clears the buffer per frame, so a non-step frame must still
draw or it goes black). A random soup always decays to a near-frozen field,
so it re-seeds on extinction, sub-3% density, or ~32 stagnant generations to
keep gliders/chaos coming (the minimal version of MoonLight's pentomino +
CRC approach). Re-seeds continue the PRNG stream so each revival differs.
- SphereLayout: corrected the radius=1 comment — the shell band holds 18
lights (6 axis-neighbours at d²=1 + 12 edge-neighbours at d²=2), not just
the 6 axis-neighbours. Geometry unchanged; only the comment was wrong.
Core:
- HttpServerModule::serveFile: serves the embedded text assets with
Content-Encoding: gzip (a per-file gzipped flag; the PNG stays raw). The
disk path (desktop live-edit) is untouched and still serves raw.
UI:
- preview3d.js (new): the 3D WebGL preview extracted from app.js into a
self-contained ES module (same pattern as install-picker.js). It owns its GL
context, camera, and geometry; app.js wires it at four points
(init/setupShrink/onBinaryMessage/resetCamera) and talks to it only through
the DOM and localStorage. app.js drops ~345 lines.
- embed_ui.cmake: gzips the 5 text UI files at build time (python3 stdlib,
guaranteed on every build host) before hex-encoding; the PNG stays raw.
Firmware UI shrinks ~149KB→43KB (measured on S3: .rodata 385KB→280KB, .bin
−105KB). Added embed_ui.cmake + the logo to the embed DEPENDS so edits
re-trigger it.
Scripts / tests:
- unit_GameOfLifeEffect (new): B3/S23 via blinker oscillation + block
still-life, lone-cell death, wraparound, alloc/free, reallocation, 0×0
survival, bpm rate, and renders-every-frame (pins the black-frame fix).
- unit_Layouts_mutation: corrected the SphereLayout radius=1 comment to 18
lights with the d² rationale.
- scenario_Layer_base_pipeline.json: reworded the min_pct step description to
non-gating language (min_pct needs a live baseline — gates only on hardware,
skipped with a WARN in the desktop runner).
Docs:
- GameOfLifeEffect.md (new, promoted from draft): controls, the Buffer-style
allocation, the keep-it-lively re-seed logic, bpm, and the extend-later
(ruleset/palette) note. Draft removed.
- SphereLayout.md: radius=1 prose corrected to 18 lights with the d² split.
Reviews:
- 👾 SphereLayout radius=1 "6 vs 18": REJECTED the suggestion to change the
test to 6 / alter the geometry. The documented half-open band [r−0.5,r+0.5)
for r=1 is 1≤4d²<9 → d²∈{1,2} → 6 axis + 12 edge = 18, which is what the code
emits and the test asserts. The "6" came from inaccurate prose; fixed the
prose/comments instead (above). Test stays at 18.
- 👾 writeChunks blocking-retry → non-blocking + pending-buffer/drain-task:
DEFERRED. This is a core redesign (per-connection buffer, drain/poll
integration), not a bug fix; the current bounded-drain was a deliberate,
documented, on-device-verified choice (lwIP has no free-TX query; 8ms cap;
WS stable, no FPS regression). Nothing shows it broken — declined per
default-to-subtraction / core-grows-slower.
- 🐇 scenario_runner GridLayout prop numeric validation (1–512): DECLINED —
scenario_runner reads trusted author-written fixture JSON; a bad size fails
the scenario's own assertions. Validation is machinery against input that
isn't untrusted here.
KPI Details:
Desktop:
Lights: 16,384
Binary: 281 KB
[doctest] 229 cases, 229 passed (4979 assertions)
tick: 432/123/66/2/122/123/53/42us (per scenario)
=== 10 scenario(s), 10 passed, 0 failed ===
Platform boundary: PASS
Specs: 28 modules, 28 ok
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8, current grid + Metaballs):
tick: 24576us (FPS: 40) heap free: 8526939 maxBlock: 180224
All 4 firmwares build clean (esp32s3-n16r8, esp32, esp32-eth, esp32-eth-wifi).
Code:
64 source files (11223 lines)
42 test files (5964 lines)
Lizard: 45 warnings
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/moonmodules/light/effects/GameOfLifeEffect.md`:
- Around line 41-47: Rewrite the "Extending" section (header "Extending") to
describe only current extension seams in present tense: state that the
simulation step and colouring are decoupled (colour is a single render line),
that the ruleset is implemented as the B3/S23 predicate and can be swapped for a
different birth/survival mask, and that colour conversion currently uses
hsvToRgb but can be replaced by a palette lookup; remove any future/roadmap
language and move any planning notes to docs/plan.md or docs/history as
appropriate.
In `@src/light/effects/GameOfLifeEffect.h`:
- Around line 182-185: The stagnation delta currently narrows the difference
between alive and lastAlive_ to uint16_t causing truncation on large grids;
change delta to be of type nrOfLightsType (use nrOfLightsType delta = (alive >
lastAlive_) ? static_cast<nrOfLightsType>(alive - lastAlive_) :
static_cast<nrOfLightsType>(lastAlive_ - alive)) and keep the subsequent
comparison against (cellCount_ / 256 + 1) unchanged so the full-width arithmetic
prevents false reseed triggers in GameOfLifeEffect.
- Around line 83-85: The first-frame dt calculation uses lastElapsed_ as a
stale/zero baseline causing a huge initial dt; fix by bootstrapping lastElapsed_
when it's uninitialized: compute now = elapsed(); if lastElapsed_ == 0 (or
lastElapsed_ > now) then set lastElapsed_ = now and dt = 0 (or skip stepping)
before the normal now - lastElapsed_ assignment; apply the same change to the
other occurrence covering lines ~250-255 so both code paths use the bootstrapped
lastElapsed_ to avoid an initial catch-up burst.
In `@src/ui/preview3d.js`:
- Around line 244-245: When vi === 0 the code returns without updating lastVerts
so stale lit points remain; change the early-return path to clear the cached
vertices before returning (e.g. set lastVerts to an empty subarray derived from
vertsBuf or otherwise zero-length) and then return, and apply the same change to
the other occurrence around the block referencing vi/lastVerts (the code at
~281-285) so both places clear lastVerts when there are no vertices.
- Around line 16-21: Guard parsing of the persisted "mm_cam" value so a
malformed JSON doesn't throw during module init: wrap
JSON.parse(localStorage.getItem("mm_cam") || "null") in a try/catch (or use a
safe parse helper) and validate the result has numeric properties t, p, d before
using them to set camTheta, camPhi, camDist and camAutoFit; on parse/validation
failure fall back to the original defaults (Math.PI, 0.4, 2.5) and ensure
saveCam still writes a valid object back to localStorage.
🪄 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
Run ID: 6be3140d-272d-47f9-a7e0-9c456c9e2dde
📒 Files selected for processing (18)
CMakeLists.txtdocs/moonmodules/light/effects/GameOfLifeEffect.mddocs/moonmodules/light/layouts/SphereLayout.mddocs/moonmodules_draft/light/effects/GameOfLifeEffect.mdesp32/main/CMakeLists.txtsrc/core/HttpServerModule.cppsrc/light/effects/GameOfLifeEffect.hsrc/light/layouts/SphereLayout.hsrc/main.cppsrc/ui/app.jssrc/ui/embed_ui.cmakesrc/ui/preview3d.jstest/CMakeLists.txttest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_GridLayout_grid_sizes.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/unit/light/unit_GameOfLifeEffect.cpptest/unit/light/unit_Layouts_mutation.cpp
💤 Files with no reviewable changes (1)
- docs/moonmodules_draft/light/effects/GameOfLifeEffect.md
Comment on lines
+41
to
+47
| ## Extending | ||
|
|
||
| The simulation step and the colouring are decoupled (colour is one render | ||
| line). A future ruleset control swaps the hardcoded B3/S23 predicate for a | ||
| birth/survival bitmask; a future palette feature replaces the `hsvToRgb` call | ||
| with a palette lookup. Neither exists yet and neither is stubbed — see the | ||
| *concrete first* principle in [architecture.md](../../../architecture.md). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Keep this section present-tense and non-roadmap.
This section currently reads as future-planning text. Reword it to describe current extension seams only, and move roadmap intent to docs/plan.md/history if needed.
Proposed wording
-## Extending
-
-The simulation step and the colouring are decoupled (colour is one render
-line). A future ruleset control swaps the hardcoded B3/S23 predicate for a
-birth/survival bitmask; a future palette feature replaces the `hsvToRgb` call
-with a palette lookup. Neither exists yet and neither is stubbed — see the
-*concrete first* principle in [architecture.md](../../../architecture.md).
+## Extending
+
+The simulation step and colouring are decoupled (colour is one render line).
+The current implementation keeps B3/S23 and direct `hsvToRgb` rendering as the
+active behavior, and the split keeps ruleset/palette changes local when they
+are implemented.🤖 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/moonmodules/light/effects/GameOfLifeEffect.md` around lines 41 - 47,
Rewrite the "Extending" section (header "Extending") to describe only current
extension seams in present tense: state that the simulation step and colouring
are decoupled (colour is a single render line), that the ruleset is implemented
as the B3/S23 predicate and can be swapped for a different birth/survival mask,
and that colour conversion currently uses hsvToRgb but can be replaced by a
palette lookup; remove any future/roadmap language and move any planning notes
to docs/plan.md or docs/history as appropriate.
Comment on lines
+83
to
+85
| uint32_t now = elapsed(); | ||
| uint32_t dt = now - lastElapsed_; | ||
| lastElapsed_ = now; |
There was a problem hiding this comment.
Bootstrap pacing state to avoid first-frame catch-up bursts.
Line 84 computes dt from a zero/stale baseline, so the first frame after startup/rebuild can step several generations immediately (budget-capped), which breaks expected bpm pacing.
Proposed fix
void loop() override {
if (!cur_ || !nxt_) return;
@@
uint32_t now = elapsed();
+ if (lastElapsed_ == 0) {
+ lastElapsed_ = now; // first frame after (re)build: no catch-up burst
+ }
uint32_t dt = now - lastElapsed_;
lastElapsed_ = now;
@@
void releaseGrids() {
if (cur_) { platform::free(cur_); cur_ = nullptr; }
if (nxt_) { platform::free(nxt_); nxt_ = nullptr; }
cellCount_ = 0;
seeded_ = false; // a fresh grid re-derives the seed
+ lastElapsed_ = 0;
+ stepAccum_ = 0;
}Also applies to: 250-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/effects/GameOfLifeEffect.h` around lines 83 - 85, The first-frame
dt calculation uses lastElapsed_ as a stale/zero baseline causing a huge initial
dt; fix by bootstrapping lastElapsed_ when it's uninitialized: compute now =
elapsed(); if lastElapsed_ == 0 (or lastElapsed_ > now) then set lastElapsed_ =
now and dt = 0 (or skip stepping) before the normal now - lastElapsed_
assignment; apply the same change to the other occurrence covering lines
~250-255 so both code paths use the bootstrapped lastElapsed_ to avoid an
initial catch-up burst.
Comment on lines
+182
to
+185
| uint16_t delta = (alive > lastAlive_) | ||
| ? static_cast<uint16_t>(alive - lastAlive_) | ||
| : static_cast<uint16_t>(lastAlive_ - alive); | ||
| if (delta <= (cellCount_ / 256 + 1)) { // <0.4% change this generation |
There was a problem hiding this comment.
Use full-width arithmetic for stagnation delta.
Lines 182-185 narrow alive deltas to uint16_t. On larger grids this truncates and can trigger false stagnation reseeds. Keep delta in nrOfLightsType.
Proposed fix
- uint16_t delta = (alive > lastAlive_)
- ? static_cast<uint16_t>(alive - lastAlive_)
- : static_cast<uint16_t>(lastAlive_ - alive);
+ nrOfLightsType delta = (alive > lastAlive_)
+ ? (alive - lastAlive_)
+ : (lastAlive_ - alive);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/effects/GameOfLifeEffect.h` around lines 182 - 185, The stagnation
delta currently narrows the difference between alive and lastAlive_ to uint16_t
causing truncation on large grids; change delta to be of type nrOfLightsType
(use nrOfLightsType delta = (alive > lastAlive_) ?
static_cast<nrOfLightsType>(alive - lastAlive_) :
static_cast<nrOfLightsType>(lastAlive_ - alive)) and keep the subsequent
comparison against (cellCount_ / 256 + 1) unchanged so the full-width arithmetic
prevents false reseed triggers in GameOfLifeEffect.
Comment on lines
+16
to
+21
| const _cam = JSON.parse(localStorage.getItem("mm_cam") || "null"); | ||
| let camTheta = _cam ? _cam.t : Math.PI; | ||
| let camPhi = _cam ? _cam.p : 0.4; | ||
| let camDist = _cam ? _cam.d : 2.5; | ||
| let camAutoFit = !_cam; // fit on first frame when no saved position | ||
| function saveCam() { localStorage.setItem("mm_cam", JSON.stringify({t: camTheta, p: camPhi, d: camDist})); } |
There was a problem hiding this comment.
Guard persisted camera JSON parsing.
A malformed mm_cam value in localStorage throws during module initialization and can break preview startup for the whole page.
Suggested fix
-const _cam = JSON.parse(localStorage.getItem("mm_cam") || "null");
+let _cam = null;
+try {
+ const rawCam = localStorage.getItem("mm_cam");
+ _cam = rawCam ? JSON.parse(rawCam) : null;
+} catch {
+ _cam = null;
+}📝 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
| const _cam = JSON.parse(localStorage.getItem("mm_cam") || "null"); | |
| let camTheta = _cam ? _cam.t : Math.PI; | |
| let camPhi = _cam ? _cam.p : 0.4; | |
| let camDist = _cam ? _cam.d : 2.5; | |
| let camAutoFit = !_cam; // fit on first frame when no saved position | |
| function saveCam() { localStorage.setItem("mm_cam", JSON.stringify({t: camTheta, p: camPhi, d: camDist})); } | |
| let _cam = null; | |
| try { | |
| const rawCam = localStorage.getItem("mm_cam"); | |
| _cam = rawCam ? JSON.parse(rawCam) : null; | |
| } catch { | |
| _cam = null; | |
| } | |
| let camTheta = _cam ? _cam.t : Math.PI; | |
| let camPhi = _cam ? _cam.p : 0.4; | |
| let camDist = _cam ? _cam.d : 2.5; | |
| let camAutoFit = !_cam; // fit on first frame when no saved position | |
| function saveCam() { localStorage.setItem("mm_cam", JSON.stringify({t: camTheta, p: camPhi, d: camDist})); } |
🤖 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/ui/preview3d.js` around lines 16 - 21, Guard parsing of the persisted
"mm_cam" value so a malformed JSON doesn't throw during module init: wrap
JSON.parse(localStorage.getItem("mm_cam") || "null") in a try/catch (or use a
safe parse helper) and validate the result has numeric properties t, p, d before
using them to set camTheta, camPhi, camDist and camAutoFit; on parse/validation
failure fall back to the original defaults (Math.PI, 0.4, 2.5) and ensure
saveCam still writes a valid object back to localStorage.
Comment on lines
+244
to
+245
| if (vi === 0) return; // all-dark frame — keep the last geometry, let rAF idle | ||
| lastVerts = vertsBuf.subarray(0, vi); |
There was a problem hiding this comment.
All-dark frames keep stale lit points visible.
When vi === 0, the function returns without updating cached vertices, so the render loop keeps drawing the previous non-dark frame.
Suggested fix
- if (vi === 0) return; // all-dark frame — keep the last geometry, let rAF idle
+ if (vi === 0) {
+ // Clear preview on all-dark frames instead of re-drawing stale points.
+ lastVerts = new Float32Array(0);
+ lastVertCount = 0;
+ lastMaxDim = previewMaxDim_;
+ if (!glLoopRunning) startRenderLoop();
+ return;
+ }
@@
function loop() {
if (!lastVerts) { glLoopRunning = false; return; }
drawVerts();
+ if (lastVertCount === 0) { glLoopRunning = false; return; }
requestAnimationFrame(loop);
}Also applies to: 281-285
🤖 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/ui/preview3d.js` around lines 244 - 245, When vi === 0 the code returns
without updating lastVerts so stale lit points remain; change the early-return
path to clear the cached vertices before returning (e.g. set lastVerts to an
empty subarray derived from vertsBuf or otherwise zero-length) and then return,
and apply the same change to the other occurrence around the block referencing
vi/lastVerts (the code at ~281-285) so both places clear lastVerts when there
are no vertices.
Follow-up to the GameOfLifeEffect/gzip/preview3d commit, gathering the PR-merge gate work: three review fixes, the regenerated test inventory, and the branch's carry-forward lesson — so main gets them with the code that proved them. Light domain: - GameOfLifeEffect: first-frame dt bootstrap (lastElapsed_ starts at 0, so a raw now-0 was a huge initial dt that pinned the step accumulator at max rate forever, ignoring bpm). Widened the stagnation delta from uint16_t to nrOfLightsType (at the 512x512 max grid on a PSRAM board that is 262144 cells, which truncated uint16_t and triggered false re-seeds). UI: - preview3d.js: guard the persisted mm_cam parse — a corrupt localStorage value no longer throws during module init; falls back to the camera defaults. Docs / tests: - decisions.md: carry-forward lesson — a time-gated effect must still repaint every frame (Layer clears the buffer per frame), plus the two adjacent traps (first-frame dt, delta width). Lands with the GameOfLifeEffect code. - GameOfLifeEffect.md: rewrote the future-tense "Extending" section to present-tense "Extension seams"; added a Tests cross-link. - tests/unit-tests.md, tests/scenario-tests.md: regenerated so the inventory includes GameOfLifeEffect and the refreshed esp32s3-n16r8 observations. Reviews: - 👾 Reviewer (Opus, whole branch): APPROVE-WITH-NITS. One non-blocking nit (PreviewDriver rgb_ lazy-allocates on the first frame instead of in onBuildState; bounded + allocation-free in steady state) — accepted, no action. - 👾 GameOfLife "renders every frame" black-screen, dt burst, delta width: fixed (above). - 👾 preview3d.js vi===0 keeps last geometry on an all-dark frame: accepted as intentional anti-flicker (commented); clearing would flash effects that go briefly dark. No change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
changed the title
This was referenced Jun 6, 2026
ewowi
added a commit
that referenced
this pull request
Jun 12, 2026
…bit review Renames the audio module from MicModule to AudioModule (it does audio acquisition plus level/FFT analysis, and will gain line-in / USB sources beyond the I²S mic, so the name now reflects the module, not one source). Makes the primary-source references in the docs clickable (datasheets, Espressif API pages, vendor sites) per the spec-and-test principle. Processes the CodeRabbit review on the prior commit: six findings fixed, six skipped with a stated reason. KPI: 16384lights | PC:340KB | tick:121/89/119/122/56/2/37/21/117/342us(FPS:8264/11235/8403/8196/17857/500000/27027/47619/8547/2923) | tick:21177us(FPS:47) | heap:33066KB | src:87(16753) | test:53(8518) | lizard:68w Core: - MicModule.h → AudioModule.h (git mv, history preserved): class MicModule → AudioModule, registration string "MicModule" → "AudioModule", the latestFrame() static, the main.cpp wiring. The mic-source platform seam (hasI2sMic, audioMicRead, AudioMicHandle) is deliberately NOT renamed: it correctly names today's actual source; it broadens when line-in lands (concrete-first). Verified live on the S3: AudioModule registers and runs (AudioModule:523us in the tick breakdown), no boot-loop. Light domain: - AudioLevel.h: clamp the DcBlocker IIR output to the int32 range before narrowing (a settling transient could push the float past int32 bounds, which is UB on the cast). 🐇 #1. - AudioSpectrumEffect.h: fix a height-gradient off-by-one — the gradient divided by (h-1) but the spectrum spans specH rows; when the bottom row is the level meter (specH == h-1) the top spectrum row never reached full red. Now divides by (specH-1). 🐇 #4. - AudioVolumeEffect.h: defensively zero any per-light channels beyond RGB (buffers are RGB/cpl=3 today; this keeps the write correct for any cpl and leaves no stale bytes). 🐇 #3. Scripts / build: - idf_component.yml: pin esp_wifi_remote ~1.6.1 and esp_hosted ~2.12.9 (the P4-NANO-validated versions) instead of "*", so the P4 build can't silently drift to a new minor; also corrected the stale "esp_hosted bring-up prelude" comment (that prelude was removed). 🐇 #2. Verified the pins resolve and the P4 builds + boots. Docs / CI: - AudioModule.md (was MicModule.md): retitled; opening reframed as "acquires an audio source ... named for what it does, not one source" (present-tense, no claim line-in exists yet). Added primary-source links: INMP441 datasheet, esp-dsp, ESP-IDF I2S API, Hann window. First fully em-dash-free module spec. - RmtLedDriver.md / LcdLedDriver.md / ParlioLedDriver.md: linked WS2812B datasheet, ESP-IDF RMT v2 / esp_lcd / Parlio API pages, the v6.0 migration guide (legacy-RMT removal), ESP32-P4 product page. (Caught + fixed an agent-guessed Parlio URL that 404'd; all links verified live.) - SystemModule.md / building.md: linked ESP32-C6, esp_hosted, esp_wifi_remote, Waveshare ESP32-P4-NANO. - decisions.md: reworded the "designed fresh" audio lesson so it no longer reads as "don't look at prior art at all" (which fought the study-with-respect principle); now "reference proven behaviour, don't trace structure," with the flat-mic / no-correction-table call as the example. 🐇 #6. - AudioVolumeEffect.md: em-dashes removed (touched alongside the effect). 🐇 #7 (partial). - AudioSpectrumEffect.md / AudioFrame.h / test files: AudioModule rename propagated (@module annotations, references). Reviews — 🐇 CodeRabbit, prior commit (b79d54f): - Fixed #1 (DcBlocker int32 clamp), #2 (component version pins), #3 (AudioVolumeEffect extra-channel clear), #4 (AudioSpectrumEffect specH off-by-one), #6 (decisions.md prior-art wording), #7 (AudioVolumeEffect.md em-dashes). - Skipped #5 (architecture.md Buffer*/Correction* "pin at wiring time"): describes the identity-mapping fast path where the pointer is genuinely stable; the suggestion proposes a different architecture, not a doc fix; paragraph untouched this branch. - Skipped #7 (README.md / backlog.md bulk em-dash sweep): the rule is "as files are touched, not a single sweep"; a ~176-dash reformat is its own task. - Skipped #8 (AudioModule stale frame): already handled — latestFrame() returns the static silent frame when active_ is null (teardown nulls it); reinit keeps the same active mic and refreshes within one loop. - Skipped #9 (AudioModule platform boundary): not a violation — the boundary check passes and the neutral platform.h facade is included by ~10 core modules; the rule forbids platform-specific code outside src/platform/, not the abstract interface. An IAudioSource indirection is exactly the bespoke abstraction the architecture avoids. - Skipped #10 (desktop improv signature): decl and definition match exactly; the desktop build passes, which proves it. - Skipped #11/#12 (platform_esp32.cpp fcntl error-handling / include): pre-existing code not changed this branch; expanding into untouched code violates minimal-change scope. Hardware verified this commit: S3 boots clean at 225 FPS with AudioModule live; P4 boots clean at 60 FPS on Ethernet with the pinned components resolved and wifiCoproc reporting "not detected" (the known C6-slave-firmware blocker, unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four commits of feature + architecture work on
next-iteration. Each landed with its own commit gates green; this is the combined record of what merges intomain.151cd43)8db9ddf)89cf681)a212300)1. Child add/edit policy: device-declared, not hardcoded in the UI
Moves the UI's child add / delete / replace / drag policy out of a hardcoded light-domain list in
app.jsonto the C++ side, declared per-module and reported through/api/types+/api/state.role === "effect" || "modifier", so only Layer children got the buttons."Layers"|"Layer"|"Drivers"|"Layouts", honouring architecture.md's "the UI contains no hard-coded knowledge of specific modules."Two
MoonModulevirtuals (mirroringrole()/tags()):acceptsChildRoles()on the parent (what it accepts),userEditable()on the child (whether it may be deleted/replaced — a load-bearing child opts out).2. Core/light type boundary + release channel + grid default
src/core/types.h. It had become a junk-drawer of light types. Each symbol moved to its owner: light types →light/light_types.h,HEAP_RESERVE→platform.h,defaultGridSize→GridLayout.h,CoordCallback→Layouts.h.core/now names zero light types.PreviewFrameleft core.HttpServerModuleno longer knows the preview wire format; it implements a domain-neutralBinaryBroadcaster(~6-line core interface) thatPreviewDriverpushes bytes to. (Carried forward indecisions.md: incidental vs essential dependency.)MM_RELEASEso SystemModule version reportssemver (channel)(e.g.1.0.0-rc2 (latest)).3. SphereLayout + true-shape sparse preview
SphereLayout— a hollow lattice sphere (surface shell only,radius1–64).lightCount()andforEachCoord()share one squared-integer band predicate so they can't disagree.setIdentity+ memcpy fast path.0x03), RGB per frame (0x02), positions 1 byte/axis. Sparse layouts render in their real shape; per-frame data shrinks to real lights.writeChunksdrain-on-partial (platform_esp32) — on a partial write, drain the unsent tail with a bounded retry instead of dropping the connection; fixes the preview WebSocket closing ~0.8 s after connect at 128×128 under ArtNet backpressure.4. GameOfLifeEffect + gzip UI + preview3d.js extraction + review fixes
GameOfLifeEffect— Conway B3/S23 on the XY plane (D2). Two width×height byte grids (cur/nxt), PSRAM-first, (re)allocated inonBuildState, freed inteardown. Controls:seed,wraparound,hue,bpm.bpmtime-gates the generation rate independent of frame rate; the grid repaints every frame (the Layer clears the buffer per-frame). Re-seeds on extinction / sub-3% density / ~32 stagnant generations so it stays lively.python3stdlib) and served withContent-Encoding: gzip; the browser inflates natively. Firmware UI shrinks ~149 KB → 43 KB (measured on S3:.rodata385 KB → 280 KB,.bin−105 KB). PNG stays raw.app.jsinto a self-contained ES module (same pattern asinstall-picker.js);app.jsdrops ~345 lines. Wired at four points (init/setupShrink/onBinaryMessage/resetCamera).deltawidened tonrOfLightsType(no truncation at 512×512 on PSRAM); GoL first-framedtbootstrap (no initial catch-up burst);mm_camparse guarded against corrupt localStorage; spec "Extending" section rewritten present-tense.Test plan (whole branch)
-Werror, zero warnings)ctest/mm_tests— 229 cases, 4979 assertions passmm_scenarios— 10/10 passesp32,esp32-eth,esp32-eth-wifi,esp32s3-n16r8all cleantick:24576us(FPS:40)on s3-n16r8 (current grid + Metaballs; not the 128×128 heavy case — different configuration, not a regression)rgb_lazy-alloc on first frame vsonBuildState)docs/tests/*.md); carry-forward lesson added (decisions.md)Content-Encoding: gzip), sparse sphere preview,userEditable/acceptsChildRolesover real/api/state+/api/typesbpm— pending product-owner🤖 Generated with Claude Code