Add memory allocation, device discovery, heap modules, noise animation#1
Merged
Conversation
…s on panel First implementation commit. 128x128 grid with animated diagonal rainbow, sent as ArtNet packets to a hub75 panel. All MoonModules single-file .h, platform boundary clean, zero warnings, 7 test files passing. Also: MoonDeck improvements — Run/Stop toggle for long-running scripts, ThreadingHTTPServer, process detection on page load, script path fixes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviews against CLAUDE.md hard rules: platform boundary, hot path constraints, code style, and design principles. Path-based instructions for platform, core, light domain, tests, and scripts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds complete lighting runtime: CMake/build, platform abstractions (desktop/ESP32), core modules/scheduler, light pipeline (layouts/effects/modifiers/LUT/blend/drivers), HTTP/WebSocket server with embedded Web UI, scripts and scenario runners, extensive tests, and documentation. ChangesUnified lighting runtime, UI, tooling, and tests
Sequence Diagram(s)sequenceDiagram
participant DesktopApp
participant HttpServer
participant BrowserUI
participant Scheduler
participant Drivers
BrowserUI->>HttpServer: GET / (UI assets)
BrowserUI-->>HttpServer: GET /ws (upgrade)
HttpServer-->>BrowserUI: 101 Switching Protocols
BrowserUI->>HttpServer: POST /api/control {module,control,value}
HttpServer->>Scheduler: set control + rebuild()
Scheduler->>Drivers: tick() -> loop()
Drivers-->>HttpServer: PreviewFrame ready
HttpServer-->>BrowserUI: WS text state (1s cadence)
HttpServer-->>BrowserUI: WS binary preview frames (~fps)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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: 14
🧹 Nitpick comments (2)
src/platform/desktop/platform_desktop.cpp (1)
48-59: ⚡ Quick winVerify complete datagram transmission in send().
The code checks
sent >= 0but notsent == len. While UDPsendtotypically sends the entire datagram atomically or fails, returning the actual bytes sent would provide better diagnostics. Consider either documenting this assumption or returning sent byte count for verification by caller.♻️ Enhanced error handling
bool UdpSocket::send(const char* ip, uint16_t port, const uint8_t* data, size_t len) { if (fd_ < 0) return false; sockaddr_in addr{}; addr.sin_family = AF_INET; addr.sin_port = htons(port); if (inet_pton(AF_INET, ip, &addr.sin_addr) != 1) return false; auto sent = ::sendto(fd_, data, len, 0, reinterpret_cast<const sockaddr*>(&addr), sizeof(addr)); - return sent >= 0; + return sent >= 0 && static_cast<size_t>(sent) == len; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/desktop/platform_desktop.cpp` around lines 48 - 59, The UdpSocket::send function only checks sent >= 0 and doesn't verify the entire datagram was transmitted; update the implementation to verify that the return value from ::sendto equals the expected length (compare sent to static_cast<ssize_t>(len)) and return false if it differs (or alternatively change the API to return the sent byte count to the caller so they can validate); locate UdpSocket::send and replace the sent >= 0 check with an exact equality check (or adjust the function return signature and callers if opting to return the byte count).src/platform/platform.h (1)
14-28: 💤 Low valueConsider adding move semantics to UdpSocket.
The class currently has a user-declared destructor and deleted copy operations, which implicitly deletes move operations. If you anticipate scenarios where sockets need to be moved (e.g., returned from factory functions or stored in containers), explicitly defaulting move operations would enable those patterns.
♻️ Optional enhancement
UdpSocket(const UdpSocket&) = delete; UdpSocket& operator=(const UdpSocket&) = delete; + + UdpSocket(UdpSocket&& other) noexcept : fd_(other.fd_) { + other.fd_ = -1; + } + UdpSocket& operator=(UdpSocket&& other) noexcept { + if (this != &other) { + close(); + fd_ = other.fd_; + other.fd_ = -1; + } + return *this; + }🤖 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/platform.h` around lines 14 - 28, UdpSocket currently has a user-declared destructor and deleted copy ops which prevents moving; add explicit move semantics by implementing a move constructor and move assignment operator for class UdpSocket that transfer ownership of fd_ from the source to the destination and reset the source's fd_ to -1, and keep the destructor ~UdpSocket() as-is; reference the class name UdpSocket, the member fd_, and the destructor ~UdpSocket() when locating where to add these methods.
🤖 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/history/plan-01.md`:
- Around line 17-50: The fenced code block in docs/history/plan-01.md is missing
a language tag; update the opening fence from ``` to a tagged fence such as
```text (or ```plaintext) so the block beginning with "CMakeLists.txt
# Root: C++20, -Wall -Wextra -Werror, core + platform libs, test target" is
changed to start with ```text to follow markdown best practices and enable
consistent rendering.
In `@scripts/moondeck_config.json`:
- Around line 64-65: The config for the long-running script "monitor_esp32" is
missing the process tracking key; add a "process_name" property to its JSON
entry (e.g., "process_name": "monitor_esp32") so the UI can detect and restore
the running state via /api/running; update the object that currently contains
"needs_port": true and "long_running": true to include this "process_name"
field.
In `@scripts/moondeck.py`:
- Around line 67-77: The pkill/pgrep calls can raise FileNotFoundError on
non-Unix hosts; update usages (in scripts/moondeck.py functions around the pkill
call and is_process_running, and the similar call in scripts/run/run_desktop.py
at line noted) to first check shutil.which("pkill") or shutil.which("pgrep")
before calling subprocess.run, and invoke subprocess.run(..., check=False,
capture_output=True) so non-zero exits don't raise; for is_process_running
return False immediately when pgrep is unavailable and for the pkill path skip
calling pkill (or log a no-op) when pkill is not found.
In `@scripts/run/run_desktop.py`:
- Around line 18-19: The current pkill invocation uses subprocess.run(["pkill",
"-f", EXECUTABLE.name], ...), which can match any argv containing that token;
change it to match the process name exactly by replacing "-f" with "-x" and
passing the basename of EXECUTABLE.name (use os.path.basename(EXECUTABLE.name)
if EXECUTABLE.name may include a path). Update the subprocess.run call that
references EXECUTABLE.name to use ["pkill", "-x",
os.path.basename(EXECUTABLE.name)] (preserve capture_output=True).
In `@src/core/Scheduler.h`:
- Around line 12-15: addModule currently accepts null pointers which later
causes crashes when setup()/tick()/teardown() dereference entries; update
addModule(MoonModule* mod) to reject nullptr by checking if (mod == nullptr) and
returning (or logging/raising an error) before touching modules_ or
moduleCount_; retain the existing capacity check (moduleCount_ >=
modules_.size()) and only assign modules_[moduleCount_++] = mod when mod is
non-null to ensure no null entries are registered.
In `@src/light/ArtNetSendDriver.h`:
- Around line 39-41: The FPS limiter divides by fps without checking for zero,
which can cause undefined behavior; in the function that uses
platform::millis(), lastSendTime_, and fps, guard against fps == 0 before
computing interval (e.g., if fps == 0 return/skip send or set interval to a safe
large value) so you never perform 1000 / fps when fps is zero; update the code
paths that compute uint32_t interval = 1000 / fps to first check fps and handle
the zero case deterministically.
In `@src/light/Buffer.h`:
- Around line 40-43: The allocate method computes totalBytes =
static_cast<size_t>(nrOfLights) * cpl without checking for overflow; update
Buffer::allocate to guard the multiplication by converting operands to size_t
and checking if static_cast<size_t>(nrOfLights) > SIZE_MAX /
static_cast<size_t>(cpl) (or if cpl==0) before computing totalBytes, return
false (or handle error) when overflow would occur, then proceed to allocate
using the safe totalBytes; reference the allocate function, nrOfLightsType, cpl
and totalBytes when making the change.
In `@src/light/DriverGroup.h`:
- Around line 18-21: The addDriver method (DriverGroup::addDriver) currently
dereferences the incoming DriverBase* without null checks; guard against nullptr
by validating the driver parameter before calling driver->setParent(this) and
before indexing drivers_ using driverCount_. Update addDriver to check if driver
is null (and either return early or log/handle the error), then proceed to
setParent and assign drivers_[driverCount_++] only when driver is non-null and
there is capacity (using existing driverCount_ and drivers_.size()).
In `@src/light/LayoutGroup.h`:
- Around line 18-21: The addLayout method currently dereferences layout without
checking for null; update addLayout (in class LayoutGroup) to first check if the
incoming LayoutBase* layout is null and return (or handle error) before calling
layout->setParent(this), while keeping the existing bounds check on layoutCount_
against layouts_.size() and assignment to layouts_[layoutCount_++]; this
prevents a crash when a nullptr is passed.
In `@src/light/RainbowEffect.h`:
- Line 25: The phase accumulation uses 32-bit arithmetic and can overflow;
change the computation in the RainbowEffect code that sets uint32_t phase to
perform the multiply/divide in 64-bit (e.g., cast operands like elapsed() and
speed to uint64_t, compute uint64_t tmp = elapsed64 * speed64 * 256 / 60000)
then static_cast the final result back to uint32_t for phase; update the
expression that assigns to phase to use this 64-bit intermediate to avoid
overflow artifacts.
- Around line 20-40: The loop unconditionally writes three color bytes
(buf[offset + 0..2]) but channelsPerLight() (cpl) can be <3 causing OOB writes;
modify the write logic in the RainbowEffect render loop to only write up to cpl
channels by computing size_t offset = static_cast<size_t>(i) * cpl and then
conditionally writing the available channels (e.g., for n = 0; n < cpl && n < 3;
++n write the corresponding component from the RGB returned by hsvToRgb), or
compute a minChannels = std::min<size_t>(cpl, 3) and write only that many bytes,
ensuring no other code assumes fixed 3-channel stride.
In `@src/main.cpp`:
- Around line 51-53: The artnet module is being double-scheduled: it’s added
directly via scheduler.addModule(&artnet) while DriverGroup::loop() already
invokes each driver's loop; remove the explicit registration of artnet (the
scheduler.addModule(&artnet) call) so that artnet is only driven via
driverGroup, or alternatively stop DriverGroup from calling that specific
driver—locate the scheduler.addModule(&artnet) call and delete it (or change
DriverGroup::loop()/DriverGroup driver list accordingly) to eliminate duplicate
per-tick invocations.
- Around line 9-13: Replace the signal-communication variable declaration and
type used by signalHandler: change the static volatile bool running to static
volatile std::sig_atomic_t running and update any code that checks or sets
running accordingly; ensure <csignal> (or <signal.h>) is included so
std::sig_atomic_t is available and keep the signature static void
signalHandler(int) as-is so the handler writes the signal-safe running variable.
In `@test/test_pipeline.cpp`:
- Around line 35-36: The test schedules both driverGroup and artnet which allows
artnet to run twice; remove the direct scheduling of artnet and ensure artnet is
executed only via driverGroup (i.e., delete or comment out the
scheduler.addModule(&artnet) call and confirm artnet is registered/owned by
driverGroup or added via driverGroup.addModule/addDriver so only driverGroup is
passed to scheduler.addModule(&driverGroup)).
---
Nitpick comments:
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 48-59: The UdpSocket::send function only checks sent >= 0 and
doesn't verify the entire datagram was transmitted; update the implementation to
verify that the return value from ::sendto equals the expected length (compare
sent to static_cast<ssize_t>(len)) and return false if it differs (or
alternatively change the API to return the sent byte count to the caller so they
can validate); locate UdpSocket::send and replace the sent >= 0 check with an
exact equality check (or adjust the function return signature and callers if
opting to return the byte count).
In `@src/platform/platform.h`:
- Around line 14-28: UdpSocket currently has a user-declared destructor and
deleted copy ops which prevents moving; add explicit move semantics by
implementing a move constructor and move assignment operator for class UdpSocket
that transfer ownership of fd_ from the source to the destination and reset the
source's fd_ to -1, and keep the destructor ~UdpSocket() as-is; reference the
class name UdpSocket, the member fd_, and the destructor ~UdpSocket() when
locating where to add these methods.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 198eec92-a837-43f4-8459-fdfeb8d04c73
📒 Files selected for processing (38)
.coderabbit.yamlCMakeLists.txtdocs/history/plan-01.mdscripts/build/build_desktop.pyscripts/build/build_esp32.pyscripts/build/flash_esp32.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/style.cssscripts/run/monitor_esp32.pyscripts/run/run_desktop.pyscripts/test/test_desktop.pysrc/core/Control.hsrc/core/MoonModule.hsrc/core/Scheduler.hsrc/core/color.hsrc/core/types.hsrc/light/ArtNetSendDriver.hsrc/light/Buffer.hsrc/light/DriverGroup.hsrc/light/EffectBase.hsrc/light/GridLayout.hsrc/light/Layer.hsrc/light/LayoutGroup.hsrc/light/RainbowEffect.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/platform.htest/CMakeLists.txttest/doctest.htest/test_artnet_packet.cpptest/test_buffer.cpptest/test_color.cpptest/test_grid_layout.cpptest/test_moonmodule.cpptest/test_pipeline.cpptest/test_rainbow.cpp
Comment on lines
+17
to
+50
| ``` | ||
| CMakeLists.txt # Root: C++20, -Wall -Wextra -Werror, core + platform libs, test target | ||
| src/ | ||
| platform/ | ||
| platform.h # mm::platform API: millis, micros, alloc, free, UdpSocket | ||
| desktop/ | ||
| platform_desktop.cpp # std::chrono, std::malloc, BSD sockets | ||
| core/ | ||
| types.h # nrOfLightsType (uint32_t), lengthType (int16_t), CoordCallback | ||
| color.h # hsvToRgb, scale8 — constexpr, integer, no floats | ||
| Control.h # ControlDescriptor (<16B on ESP32), ControlList<N> | ||
| MoonModule.h # Base class: lifecycle, controls, name, parent | ||
| Scheduler.h # Module registry, tick(), elapsed(), loop/20ms/1s dispatch | ||
| light/ | ||
| Buffer.h # uint8_t* buffer, move-only, allocate/free/clear/span | ||
| LayoutGroup.h # Groups layouts, forEachCoord with index offset | ||
| GridLayout.h # width×height×depth grid, row-major coordinates | ||
| EffectBase.h # Thin accessors to parent Layer | ||
| Layer.h # Owns buffer + effects list, render = run effects in order | ||
| RainbowEffect.h # Diagonal rainbow, BPM speed control | ||
| DriverGroup.h # Groups drivers, passes layer buffer to each | ||
| ArtNetSendDriver.h # ArtNet OpDmx packets over UDP, universe splitting, FPS limit | ||
| main.cpp # Wire pipeline, run scheduler loop | ||
| test/ | ||
| CMakeLists.txt # Test executable | ||
| doctest.h # Vendored header-only test framework | ||
| test_color.cpp # hsvToRgb at cardinal hues, scale8 | ||
| test_buffer.cpp # Allocate, clear, move, double-free safety | ||
| test_moonmodule.cpp # Lifecycle, control binding | ||
| test_grid_layout.cpp # Coordinate iteration, row-major order, 3D | ||
| test_rainbow.cpp # Buffer contains expected hsvToRgb values | ||
| test_artnet_packet.cpp # Header format, byte order, universe splitting | ||
| test_pipeline.cpp # Full pipeline: grid→layer→rainbow→artnet packets | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block lacks a language identifier. Add "text" or "plaintext" for consistency with markdown best practices.
📝 Proposed fix
-```
+```text
CMakeLists.txt # Root: C++20, -Wall -Wextra -Werror, core + platform libs, test target🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plan-01.md` around lines 17 - 50, The fenced code block in
docs/history/plan-01.md is missing a language tag; update the opening fence from
``` to a tagged fence such as ```text (or ```plaintext) so the block beginning
with "CMakeLists.txt # Root: C++20, -Wall -Wextra
-Werror, core + platform libs, test target" is changed to start with ```text to
follow markdown best practices and enable consistent rendering.
Comment on lines
+67
to
+77
| # Clean up any orphaned processes (e.g. mmv3 after os.execv) | ||
| script_def = next((s for s in SCRIPTS if s["id"] == script_id), None) | ||
| pname = script_def.get("process_name") if script_def else None | ||
| if pname: | ||
| subprocess.run(["pkill", "-f", pname], capture_output=True) | ||
|
|
||
|
|
||
| def is_process_running(name: str) -> bool: | ||
| """Check if a process matching the name is running (via pgrep).""" | ||
| r = subprocess.run(["pgrep", "-f", name], capture_output=True) | ||
| return r.returncode == 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking external tool availability..."
python - <<'PY'
import shutil
print("pkill:", shutil.which("pkill"))
print("pgrep:", shutil.which("pgrep"))
PY
echo "Checking current unguarded invocations..."
rg -n 'subprocess\.run\(\["(pkill|pgrep)"' scripts/moondeck.pyRepository: ewowi/projectMM-v3
Length of output: 308
🏁 Script executed:
# Read the specific file to verify the code snippet and line numbers
head -100 scripts/moondeck.py | tail -40Repository: ewowi/projectMM-v3
Length of output: 1434
🏁 Script executed:
# Search for all pkill and pgrep calls in the entire repository to check for comprehensive coverage
rg -n 'pkill|pgrep' --type pyRepository: ewowi/projectMM-v3
Length of output: 441
🏁 Script executed:
# Test subprocess.run() behavior when binary doesn't exist to confirm the error type
python3 <<'EOF'
import subprocess
import sys
try:
result = subprocess.run(["nonexistent_binary_xyz"], capture_output=True)
except FileNotFoundError as e:
print(f"FileNotFoundError raised: {e}")
except Exception as e:
print(f"Other exception ({type(e).__name__}): {e}")
EOFRepository: ewowi/projectMM-v3
Length of output: 150
Guard pkill/pgrep usage to prevent crashes on non-Unix hosts.
These calls raise FileNotFoundError when the binaries are unavailable, which can break /api/run, /api/kill, and /api/running paths. Additionally, pkill is also unguarded in scripts/run/run_desktop.py:19.
Apply shutil.which() to check binary availability before calling subprocess.run() for both pkill and pgrep throughout the codebase. Use check=False to prevent exceptions on non-zero exit codes.
Suggested hardening for scripts/moondeck.py
+import shutil
+
def kill_script(script_id: str):
@@
if pname:
- subprocess.run(["pkill", "-f", pname], capture_output=True)
+ pkill = shutil.which("pkill")
+ if pkill:
+ subprocess.run([pkill, "-f", pname], capture_output=True, check=False)
@@
def is_process_running(name: str) -> bool:
"""Check if a process matching the name is running (via pgrep)."""
- r = subprocess.run(["pgrep", "-f", name], capture_output=True)
- return r.returncode == 0
+ pgrep = shutil.which("pgrep")
+ if not pgrep:
+ return False
+ r = subprocess.run([pgrep, "-f", name], capture_output=True, check=False)
+ return r.returncode == 0🧰 Tools
🪛 Ruff (0.15.13)
[error] 71-71: subprocess call: check for execution of untrusted input
(S603)
[error] 71-71: Starting a process with a partial executable path
(S607)
[error] 76-76: subprocess call: check for execution of untrusted input
(S603)
[error] 76-76: Starting a process with a partial executable path
(S607)
🤖 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 `@scripts/moondeck.py` around lines 67 - 77, The pkill/pgrep calls can raise
FileNotFoundError on non-Unix hosts; update usages (in scripts/moondeck.py
functions around the pkill call and is_process_running, and the similar call in
scripts/run/run_desktop.py at line noted) to first check shutil.which("pkill")
or shutil.which("pgrep") before calling subprocess.run, and invoke
subprocess.run(..., check=False, capture_output=True) so non-zero exits don't
raise; for is_process_running return False immediately when pgrep is unavailable
and for the pkill path skip calling pkill (or log a no-op) when pkill is not
found.
Comment on lines
+18
to
+19
| # Kill any already-running instance | ||
| subprocess.run(["pkill", "-f", EXECUTABLE.name], capture_output=True) |
There was a problem hiding this comment.
Narrow process matching before kill to avoid terminating unrelated processes.
Using pkill -f with only mmv3 can kill unrelated commands whose argv contains that token. Match the executable name exactly instead.
Suggested fix
- # Kill any already-running instance
- subprocess.run(["pkill", "-f", EXECUTABLE.name], capture_output=True)
+ # Kill any already-running instance of this exact executable name
+ subprocess.run(["pkill", "-x", EXECUTABLE.name], capture_output=True)📝 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
| # Kill any already-running instance | |
| subprocess.run(["pkill", "-f", EXECUTABLE.name], capture_output=True) | |
| # Kill any already-running instance of this exact executable name | |
| subprocess.run(["pkill", "-x", EXECUTABLE.name], capture_output=True) |
🧰 Tools
🪛 Ruff (0.15.13)
[error] 19-19: subprocess call: check for execution of untrusted input
(S603)
[error] 19-19: Starting a process with a partial executable path
(S607)
🤖 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 `@scripts/run/run_desktop.py` around lines 18 - 19, The current pkill
invocation uses subprocess.run(["pkill", "-f", EXECUTABLE.name], ...), which can
match any argv containing that token; change it to match the process name
exactly by replacing "-f" with "-x" and passing the basename of EXECUTABLE.name
(use os.path.basename(EXECUTABLE.name) if EXECUTABLE.name may include a path).
Update the subprocess.run call that references EXECUTABLE.name to use ["pkill",
"-x", os.path.basename(EXECUTABLE.name)] (preserve capture_output=True).
- Remove double-scheduling of artnet (DriverGroup::loop already calls it) - Fix RainbowEffect phase overflow with uint64_t intermediate - Guard RainbowEffect channel writes against cpl < 3 - Guard ArtNet fps==0 divide-by-zero - Add nullptr checks in addModule, addDriver, addLayout - Use std::sig_atomic_t for signal handler variable - MoonDeck: cross-platform process management (taskkill/tasklist on Windows, CREATE_NEW_PROCESS_GROUP, subprocess.run fallback for execv) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- JSON-driven scenario tests (test/scenarios/base-pipeline.json) with in-process replay runner — same format as projectMM v1, ready for HTTP-driven live testing when API is added - Parents propagate lifecycle to children (setup/onBuildControls/ onAllocateMemory/teardown) — only top-level modules registered with Scheduler - Remove test_pipeline.cpp (replaced by scenario) - Add docs/testing.md with full test inventory, anchored sections - Module specs link to their test sections - Architecture testing section: module tests, scenario tests, regression tests - MoonDeck: scenario in PC tab, group section headers in sidebar - plan.md: add System MoonModule to ESP32 deployment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@scripts/scenario/run_scenario.py`:
- Line 12: The code sets RUNNER = BUILD_DIR / "test" / "mm_scenarios" assuming a
Unix binary; on Windows the executable may be "mm_scenarios.exe". Update the
logic that constructs and checks the runner path (symbol RUNNER and any
subsequent existence checks that reference it) to try the Windows suffix: if
RUNNER.exists() is false and the platform is Windows (os.name == "nt" or
sys.platform startswith "win"), check RUNNER.with_suffix(".exe") (or test both
plain and ".exe") and use that path when it exists; apply the same suffix-aware
check/selection to the other runner/path usages referenced in the same block
(the occurrences around the RUNNER variable).
In `@test/scenario_runner.cpp`:
- Around line 285-289: Guard the FPS computation in the measurement block by
checking for zero elapsed duration before dividing: after computing elapsedUs =
mm::platform::micros() - startUs (and/or elapsedMs), if elapsedUs == 0 (or
msPerFrame == 0) set fps to 0 (or a safe sentinel) instead of performing the
divisions, otherwise compute msPerFrame = elapsedMs / MEASURE_FRAMES and fps =
1000.0f / msPerFrame; update the variables elapsedMs, msPerFrame, and fps
accordingly to avoid producing inf when elapsed time is zero.
- Around line 155-163: The current type-based wiring uses static_casts (in the
block handling "GridLayout", "RainbowEffect", "ArtNetSendDriver") which can
cause UB if the parent pointer isn't actually the expected runtime type; replace
those static_casts with runtime-checked casts (e.g. dynamic_cast to
mm::LayoutGroup*, mm::Layer*, mm::DriverGroup* respectively), verify the result
is non-null before calling addLayout/addEffect/addDriver, and in the failure
path log or return an error rather than proceeding; update the wiring code that
references parent and mod so it performs these safe checks and handles
mismatches gracefully.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac454ff5-e08a-4b73-837d-652df5f178c6
📒 Files selected for processing (29)
docs/architecture-light.mddocs/architecture.mddocs/moonmodules/core/Control.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/light/Buffer.mddocs/moonmodules/light/LightConfig.mddocs/moonmodules/light/drivers/ArtNetSendDriver.mddocs/moonmodules/light/effects/RainbowEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/plan.mddocs/testing.mdscripts/MoonDeck.mdscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/index.htmlscripts/moondeck_ui/style.cssscripts/run/run_desktop.pyscripts/scenario/run_scenario.pysrc/core/Scheduler.hsrc/light/ArtNetSendDriver.hsrc/light/DriverGroup.hsrc/light/Layer.hsrc/light/LayoutGroup.hsrc/light/RainbowEffect.hsrc/main.cpptest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/base-pipeline.json
✅ Files skipped from review due to trivial changes (13)
- docs/moonmodules/core/Control.md
- docs/moonmodules/light/layouts/GridLayout.md
- docs/moonmodules/core/MoonModule.md
- docs/plan.md
- docs/moonmodules/light/Buffer.md
- docs/moonmodules/light/LightConfig.md
- docs/moonmodules/light/effects/RainbowEffect.md
- docs/moonmodules/light/drivers/ArtNetSendDriver.md
- scripts/MoonDeck.md
- docs/architecture-light.md
- docs/architecture.md
- scripts/moondeck_ui/index.html
- docs/testing.md
🚧 Files skipped from review as they are similar to previous changes (7)
- src/light/RainbowEffect.h
- src/light/LayoutGroup.h
- src/light/DriverGroup.h
- test/CMakeLists.txt
- src/light/ArtNetSendDriver.h
- src/light/Layer.h
- src/core/Scheduler.h
Proves platform abstraction works — same pipeline (GridLayout → Rainbow → ArtNet) runs on both desktop and ESP32. 128x128 grid (16K lights) at 23+ FPS on ESP32 without PSRAM, 215KB free heap. Platform layer: - ESP32: esp_timer, heap_caps_malloc, lwIP sockets, vTaskDelay yield - platform_config.h per platform for compile-time constants (hasPsram) - platform::yield(), freeHeap(), maxAllocBlock() added to API - Refactored entry points: shared mm_main() + platform-specific main ESP-IDF project: - esp32/ wrapper with Olimex ESP32-Gateway Ethernet config (LAN8720) - Board-specific sdkconfig.defaults (Olimex Rev G, WT32-ETH01) - ESP-IDF auto-detection in build/flash/monitor scripts (no manual sourcing) MoonDeck improvements: - Status dots (green/red/orange) on script cards - Tab persistence across refresh - Serial monitor saves to esp32/monitor.log - Setup ESP-IDF and Clean buttons - uv run for all scripts (inline dependencies) - ConnectionResetError suppressed - ESP32 build shows flash/RAM usage summary Type fixes: - lengthType always int16_t (int8_t overflows at 128) - defaultGridSize = 128 unconditionally - .gitignore: anchor /build/ to root (was matching scripts/build/) CodeRabbit fixes: - Scenario runner: Windows .exe suffix, FPS div-by-zero guard Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/plan.md (1)
29-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove future roadmap items out of this document.
Line 29 through Line 36 add future-planning content, which conflicts with the repo’s documentation rule to keep docs current-state only.
Based on learnings: "Code, comments, and documentation must describe the system as it is now. No changelogs or roadmaps in docs — history lives in git commits, except for
docs/history/decisions.md."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plan.md` around lines 29 - 36, The lines introducing future roadmap items — e.g., the "Add embedded web UI with tree view..." paragraph and the "7. System MoonModule + Network MoonModules" section — must be removed from docs/plan.md and relocated to an appropriate future-planning file (such as a new docs/roadmap.md or an issue tracker); edit the document to keep only current-state descriptions, deleting those roadmap paragraphs and any references to MoonDeck device discovery and projectMM v2 scan_subnet() suggestions so the file conforms to the rule that docs describe the system as it is now.docs/history/plan-02.md (1)
1-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis planning doc location conflicts with current docs policy.
The file is a roadmap/plan artifact; current repo policy only allows the explicit exception file under
docs/history/decisions.md.Based on learnings: "Code, comments, and documentation must describe the system as it is now. No changelogs or roadmaps in docs — history lives in git commits, except for
docs/history/decisions.md."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/history/plan-02.md` around lines 1 - 136, The planning document "Plan: ESP32 Deployment" violates the docs policy by being a roadmap — remove or relocate it: delete this file and instead add a concise entry summarizing the decision and relevant outcomes into the allowed exceptions file (docs/history/decisions.md) using the document title "ESP32 Deployment" and a short decision-format note (what was decided, key steps, and outcome); ensure no roadmap/changelog language remains in the repo docs and update any references to point to the new decision entry.
🤖 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/architecture-light.md`:
- Around line 189-190: Remove the roadmap-style placeholders from the
architecture doc by deleting or replacing the exact lines containing "Mapping
LUT (1:0, 1:1, 1:N, rebuild on config change) — when implemented" and "Blend+map
pass (correct physical output from logical layers) — when implemented"; if those
features are already present in the codebase, instead replace each line with a
concise current-state statement (e.g., "Mapping LUT: implemented as ...",
"Blend+map pass: implemented as ...") referencing the implemented components,
otherwise simply remove the lines so the doc only describes the current system
state.
In `@docs/architecture.md`:
- Around line 71-76: Remove the roadmap-style "Not yet implemented" bullet list
from docs/architecture.md (specifically the entries titled "Threads", "LED
drivers", "HTTP/WebSocket", and "Filesystem") and replace it with only the
current, implemented architecture description; if you need to preserve planned
items, move them to a dedicated planning file (e.g., docs/history/decisions.md
or a new docs/roadmap.md) rather than the canonical architecture doc so the
architecture file documents the system as it exists now.
In `@docs/history/plan-02.md`:
- Line 17: The fenced code block starting with ``` on line 17 is missing a
language identifier which triggers MD040; update that opening fence to include a
language token (e.g., change ``` to ```text or ```bash depending on content) so
the markdown lint passes and the code block is properly annotated.
In `@esp32/main/main.cpp`:
- Line 39: Check the return of xEventGroupCreate() when assigning to
ethEventGroup and handle allocation failure before any use (e.g., before
xEventGroupSetBits/xEventGroupWaitBits calls); if xEventGroupCreate() returns
NULL, log an error via ESP_LOGE or processLogger, clean up/abort or return an
error code, and avoid dereferencing ethEventGroup. Update the allocation site
where ethEventGroup = xEventGroupCreate() is called and ensure all later uses
(references to ethEventGroup, xEventGroupSetBits, xEventGroupWaitBits) are
guarded by a non-null check or early exit.
In `@scripts/build/build_esp32.py`:
- Around line 126-132: The current logic skips running the set-target command
when build_dir (esp32/build) exists which can leave an old chip target in place;
change the code so the set-target command is invoked whenever the requested
target (args.env) might differ—either always run subprocess.run(cmd +
["set-target", args.env], cwd=ESP32_DIR, env=env) instead of guarding it with if
not build_dir.exists(), or add a check that reads the current target and only
skips when it matches args.env; update the block around build_dir, args.env, cmd
+ ["set-target", args.env], and ESP32_DIR to ensure set-target runs when --env
changes.
In `@scripts/moondeck_ui/app.js`:
- Around line 213-218: The current try/catch around JSON.parse in the message
handler calls resetBtn(0) on parse failure, which incorrectly marks the run as
successful; change the catch to log the parse error and call resetBtn with a
non-zero exit code (e.g., resetBtn(1)) instead of 0, and include the caught
error in the log so malformed payloads are treated as failures; update the block
around JSON.parse(e.data) that references resetBtn to implement this behavior.
- Around line 60-67: When restoring the saved tab (state.tab) ensure you handle
the case where the saved tab is missing: after clearing classes, look up const
btn = document.querySelector(`.tab[data-tab="${state.tab}"]`) and if it exists
add 'active' to btn and to the content element returned by
document.getElementById("tab-"+state.tab) only if that element exists; otherwise
fall back to a default tab by selecting the first .tab element (e.g.,
document.querySelector('.tab')) and its corresponding content element and mark
those active. Update the logic around state.tab, btn and
document.getElementById("tab-"+state.tab) to avoid null dereferences and to
activate a sensible default when the saved tab is stale.
In `@scripts/moondeck.py`:
- Around line 231-235: The code currently sets popen_kwargs["preexec_fn"] =
os.setsid when _IS_WIN is false before calling subprocess.Popen; replace that
assignment with popen_kwargs["start_new_session"] = True to avoid using
preexec_fn in multithreaded contexts (leave the Windows branch using
popen_kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP intact) so
subprocess.Popen(cmd, **popen_kwargs) achieves equivalent session isolation
without the deadlock risk from os.setsid.
In `@src/platform/desktop/main_desktop.cpp`:
- Around line 8-12: Replace the global flag type used for signal communication:
change the declaration of running from "static volatile bool running" to "static
volatile std::sig_atomic_t running" and update any code that sets or checks
running accordingly; ensure <csignal> (or <signal.h>) and <cstddef> are included
so std::sig_atomic_t is available, and keep the existing signalHandler(int)
function but have it assign 0 to running (or false-equivalent) to safely
communicate with the main loop.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 60-70: Update UdpSocket::send to use std::span<const uint8_t>
instead of the pointer+length pair: change the signature of UdpSocket::send to
accept std::span<const uint8_t> data, include the <span> header, and replace
usages of the old parameters inside the function (use data.data() and
data.size() when calling ::sendto and any length checks). Ensure callers are
updated to pass a std::span (or construct one from containers/arrays) so the
implementation in platform_desktop.cpp and any declarations match the new
signature.
In `@src/platform/platform.h`:
- Line 27: Change the pointer+length API to use std::span by replacing the
current declaration bool send(const char* ip, uint16_t port, const uint8_t*
data, size_t len); with a span-based signature (e.g., bool UdpSocket::send(const
char* ip, uint16_t port, std::span<const uint8_t> data)); add the <span> include
in the header, and update all corresponding implementations and call-sites to
accept/passthrough std::span<const uint8_t> instead of separate data/len
parameters (look for UdpSocket::send in platform.h and its definitions/usages).
---
Outside diff comments:
In `@docs/history/plan-02.md`:
- Around line 1-136: The planning document "Plan: ESP32 Deployment" violates the
docs policy by being a roadmap — remove or relocate it: delete this file and
instead add a concise entry summarizing the decision and relevant outcomes into
the allowed exceptions file (docs/history/decisions.md) using the document title
"ESP32 Deployment" and a short decision-format note (what was decided, key
steps, and outcome); ensure no roadmap/changelog language remains in the repo
docs and update any references to point to the new decision entry.
In `@docs/plan.md`:
- Around line 29-36: The lines introducing future roadmap items — e.g., the "Add
embedded web UI with tree view..." paragraph and the "7. System MoonModule +
Network MoonModules" section — must be removed from docs/plan.md and relocated
to an appropriate future-planning file (such as a new docs/roadmap.md or an
issue tracker); edit the document to keep only current-state descriptions,
deleting those roadmap paragraphs and any references to MoonDeck device
discovery and projectMM v2 scan_subnet() suggestions so the file conforms to the
rule that docs describe the system as it is now.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cf19893-65eb-45d5-b2dd-74f5e3713235
📒 Files selected for processing (36)
.gitignoreCLAUDE.mdCMakeLists.txtdocs/architecture-light.mddocs/architecture.mddocs/history/plan-02.mddocs/plan.mddocs/testing.mdesp32/CMakeLists.txtesp32/main/CMakeLists.txtesp32/main/main.cppesp32/main/wifi_credentials.example.hesp32/sdkconfig.defaultsesp32/sdkconfig.defaults.olimex_gwesp32/sdkconfig.defaults.wt32eth01scripts/MoonDeck.mdscripts/build/build_esp32.pyscripts/build/clean_esp32.pyscripts/build/flash_esp32.pyscripts/build/setup_esp_idf.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/style.cssscripts/run/monitor_esp32.pyscripts/scenario/run_scenario.pysrc/core/types.hsrc/light/ArtNetSendDriver.hsrc/main.cppsrc/platform/desktop/main_desktop.cppsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/scenario_runner.cpp
✅ Files skipped from review due to trivial changes (9)
- esp32/main/wifi_credentials.example.h
- esp32/CMakeLists.txt
- esp32/main/CMakeLists.txt
- esp32/sdkconfig.defaults.wt32eth01
- esp32/sdkconfig.defaults
- esp32/sdkconfig.defaults.olimex_gw
- CLAUDE.md
- scripts/build/clean_esp32.py
- scripts/MoonDeck.md
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- test/scenario_runner.cpp
- scripts/moondeck_config.json
- src/light/ArtNetSendDriver.h
|
|
||
| ## Files | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
Line 17 triggers MD040; add a language (for example, text) to keep markdown lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plan-02.md` at line 17, The fenced code block starting with ```
on line 17 is missing a language identifier which triggers MD040; update that
opening fence to include a language token (e.g., change ``` to ```text or
```bash depending on content) so the markdown lint passes and the code block is
properly annotated.
| } | ||
|
|
||
| static void eth_init() { | ||
| ethEventGroup = xEventGroupCreate(); |
There was a problem hiding this comment.
Check EventGroup allocation failure before use.
At Line 39, xEventGroupCreate() can return nullptr; later calls at Lines 76-77 would then dereference an invalid handle and crash.
Suggested fix
static void eth_init() {
ethEventGroup = xEventGroupCreate();
+ if (ethEventGroup == nullptr) {
+ ESP_LOGE(TAG, "Failed to create Ethernet event group");
+ return; // or abort/ESP_ERROR_CHECK style fail-fast
+ }📝 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
| ethEventGroup = xEventGroupCreate(); | |
| ethEventGroup = xEventGroupCreate(); | |
| if (ethEventGroup == nullptr) { | |
| ESP_LOGE(TAG, "Failed to create Ethernet event group"); | |
| return; // or abort/ESP_ERROR_CHECK style fail-fast | |
| } |
🤖 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 `@esp32/main/main.cpp` at line 39, Check the return of xEventGroupCreate() when
assigning to ethEventGroup and handle allocation failure before any use (e.g.,
before xEventGroupSetBits/xEventGroupWaitBits calls); if xEventGroupCreate()
returns NULL, log an error via ESP_LOGE or processLogger, clean up/abort or
return an error code, and avoid dereferencing ethEventGroup. Update the
allocation site where ethEventGroup = xEventGroupCreate() is called and ensure
all later uses (references to ethEventGroup, xEventGroupSetBits,
xEventGroupWaitBits) are guarded by a non-null check or early exit.
Comment on lines
+126
to
+132
| if not build_dir.exists(): | ||
| print(f"Setting target to {args.env}...") | ||
| r = subprocess.run(cmd + ["set-target", args.env], | ||
| cwd=ESP32_DIR, env=env) | ||
| if r.returncode != 0: | ||
| sys.exit(r.returncode) | ||
|
|
There was a problem hiding this comment.
Target can become stale when --env changes.
At Lines 126-132, skipping set-target whenever esp32/build exists can keep a previous chip target and produce incorrect builds for the requested --env.
Suggested fix
- if not build_dir.exists():
- print(f"Setting target to {args.env}...")
- r = subprocess.run(cmd + ["set-target", args.env],
- cwd=ESP32_DIR, env=env)
- if r.returncode != 0:
- sys.exit(r.returncode)
+ print(f"Setting target to {args.env}...")
+ r = subprocess.run([*cmd, "set-target", args.env], cwd=ESP32_DIR, env=env)
+ if r.returncode != 0:
+ sys.exit(r.returncode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not build_dir.exists(): | |
| print(f"Setting target to {args.env}...") | |
| r = subprocess.run(cmd + ["set-target", args.env], | |
| cwd=ESP32_DIR, env=env) | |
| if r.returncode != 0: | |
| sys.exit(r.returncode) | |
| print(f"Setting target to {args.env}...") | |
| r = subprocess.run([*cmd, "set-target", args.env], cwd=ESP32_DIR, env=env) | |
| if r.returncode != 0: | |
| sys.exit(r.returncode) |
🧰 Tools
🪛 Ruff (0.15.13)
[error] 128-128: subprocess call: check for execution of untrusted input
(S603)
[warning] 128-128: Consider [*cmd, "set-target", args.env] instead of concatenation
Replace with [*cmd, "set-target", args.env]
(RUF005)
🤖 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 `@scripts/build/build_esp32.py` around lines 126 - 132, The current logic skips
running the set-target command when build_dir (esp32/build) exists which can
leave an old chip target in place; change the code so the set-target command is
invoked whenever the requested target (args.env) might differ—either always run
subprocess.run(cmd + ["set-target", args.env], cwd=ESP32_DIR, env=env) instead
of guarding it with if not build_dir.exists(), or add a check that reads the
current target and only skips when it matches args.env; update the block around
build_dir, args.env, cmd + ["set-target", args.env], and ESP32_DIR to ensure
set-target runs when --env changes.
Comment on lines
+8
to
+12
| static volatile bool running = true; | ||
|
|
||
| static void signalHandler(int) { | ||
| running = false; | ||
| } |
There was a problem hiding this comment.
Use volatile std::sig_atomic_t for signal handler communication.
The C++ standard explicitly requires volatile std::sig_atomic_t (not volatile bool) for variables accessed in signal handlers. Using volatile bool results in undefined behavior.
🛡️ Proposed fix
-static volatile bool running = true;
+static volatile std::sig_atomic_t running = 1;
static void signalHandler(int) {
- running = false;
+ running = 0;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/main_desktop.cpp` around lines 8 - 12, Replace the
global flag type used for signal communication: change the declaration of
running from "static volatile bool running" to "static volatile
std::sig_atomic_t running" and update any code that sets or checks running
accordingly; ensure <csignal> (or <signal.h>) and <cstddef> are included so
std::sig_atomic_t is available, and keep the existing signalHandler(int)
function but have it assign 0 to running (or false-equivalent) to safely
communicate with the main loop.
Comment on lines
+60
to
+70
| bool UdpSocket::send(const char* ip, uint16_t port, const uint8_t* data, size_t len) { | ||
| if (fd_ < 0) return false; | ||
|
|
||
| sockaddr_in addr{}; | ||
| addr.sin_family = AF_INET; | ||
| addr.sin_port = htons(port); | ||
| if (inet_pton(AF_INET, ip, &addr.sin_addr) != 1) return false; | ||
|
|
||
| auto sent = ::sendto(fd_, data, len, 0, | ||
| reinterpret_cast<const sockaddr*>(&addr), sizeof(addr)); | ||
| return sent >= 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Replace pointer+length buffer parameters with std::span.
UdpSocket::send(const uint8_t* data, size_t len) violates the project API guideline; prefer std::span<const uint8_t> for safer, clearer bounds handling.
As per coding guidelines: src/**/*.{h,hpp,cpp}: Use std::span over pointer + length parameter pairs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/platform_desktop.cpp` around lines 60 - 70, Update
UdpSocket::send to use std::span<const uint8_t> instead of the pointer+length
pair: change the signature of UdpSocket::send to accept std::span<const uint8_t>
data, include the <span> header, and replace usages of the old parameters inside
the function (use data.data() and data.size() when calling ::sendto and any
length checks). Ensure callers are updated to pass a std::span (or construct one
from containers/arrays) so the implementation in platform_desktop.cpp and any
declarations match the new signature.
| UdpSocket& operator=(const UdpSocket&) = delete; | ||
|
|
||
| bool open(); | ||
| bool send(const char* ip, uint16_t port, const uint8_t* data, size_t len); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use std::span in UdpSocket::send API.
Line 27 introduces a pointer+length pair; the project guideline requires std::span for this pattern.
Proposed API change
+#include <span>
...
- bool send(const char* ip, uint16_t port, const uint8_t* data, size_t len);
+ bool send(const char* ip, uint16_t port, std::span<const uint8_t> data);As per coding guidelines: "src/**/*.{h,hpp,cpp}: Use std::span over pointer + length parameter pairs".
📝 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
| bool send(const char* ip, uint16_t port, const uint8_t* data, size_t len); | |
| bool send(const char* ip, uint16_t port, std::span<const uint8_t> data); |
🤖 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/platform.h` at line 27, Change the pointer+length API to use
std::span by replacing the current declaration bool send(const char* ip,
uint16_t port, const uint8_t* data, size_t len); with a span-based signature
(e.g., bool UdpSocket::send(const char* ip, uint16_t port, std::span<const
uint8_t> data)); add the <span> include in the header, and update all
corresponding implementations and call-sites to accept/passthrough
std::span<const uint8_t> instead of separate data/len parameters (look for
UdpSocket::send in platform.h and its definitions/usages).
Noise effect: hash-based value noise with bilinear interpolation, scale/speed controls, hue mapping via hsvToRgb. Mirror modifier: kaleidoscope 1:N mapping. Halves logical dimensions per mirrored axis, deduplicates centre-axis lights. Controls: mirrorX, mirrorY, mirrorZ. MappingLUT: CSR format (offsets + destinations arrays). oneToOneMapping fast path when no modifiers. forEachDestination for hot-path iteration. BlendMap: free function mapping logical buffer to physical via LUT. Additive blending with clamping for future multi-layer support. Layer: modifier support, rebuildLUT, logical vs physical dimensions. Effects see logical dimensions transparently. DriverGroup: allocates physical output buffer when LUT is active, calls blendMap before driver loops. Verified on ESP32 Olimex Gateway: 128x128 with mirror at 17 FPS, 128KB free heap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai, please review e462374 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20238 🏁 Script executed: Length of output: 793 🏁 Script executed: Length of output: 10820 🏁 Script executed: Length of output: 11222 🏁 Script executed: Length of output: 12901
SummaryThis commit introduces the NoiseEffect + MirrorModifier + MappingLUT pipeline (plan-03). The architecture is clean: 🐛 Bugs1.
|
| Area | Status |
|---|---|
lerp8 uint8 subtraction wrap |
🔴 Bug |
hash() signed overflow UB |
🔴 Bug |
NoiseEffect 3D indexing |
🟡 Latent (depth=1 only) |
test_blend_map clamping test |
🟡 Tests wrong behaviour |
mirrorZ default doc/code mismatch |
🟡 Doc error |
MappingLUT CSR structure |
✅ Clean |
MirrorModifier deduplication logic |
✅ Correct |
BlendMap hot-path (no heap alloc) |
✅ Correct |
Layer::rebuildLUT() |
✅ Correct for single modifier |
| Test coverage (4 new test files) | ✅ Good |
mirror.json scenario |
✅ Consistent with base-pipeline |
- architecture.md: remove "Not yet implemented" roadmap list - architecture-light.md: replace "when implemented" with links to tests - moondeck.py: preexec_fn → start_new_session (thread-safe) - app.js: resetBtn(1) on parse failure, null guard on tab restore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai, can you review 3e7dd09 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 888 🏁 Script executed: Length of output: 223 🏁 Script executed: Length of output: 3434
SummaryA clean, focused fix commit addressing four findings from the previous review. All changes are correct and appropriate. ✅ Changes reviewed
|
| Finding | Status |
|---|---|
preexec_fn thread-safety |
✅ Fixed |
resetBtn(0) false-success on parse error |
✅ Fixed (resetBtn(1)) |
| Tab restore null-safety | ✅ Fixed |
| Doc "when implemented" stubs | ✅ Fixed |
Unused err in catch (err) |
🟡 Minor lint nit |
HTTP server MoonModule with REST API (GET /api/state, POST /api/control)
and WebSocket state push. Web UI with module tree, sliders, checkboxes,
text inputs — all auto-rendered from MoonModule controls. Embedded UI
files for ESP32, disk serving for desktop development.
Core kept light-domain-free: generic childCount()/child() on MoonModule
replaces explicit LayoutGroup/Layer/DriverGroup knowledge in HTTP server.
Grid controls (width/height/depth) added. Control changes trigger full
pipeline rebuild. ESP32 port 80, desktop port 8080.
Also: KPI report script with lizard analysis, collect_kpi.py --commit
for standardized commit metrics. Bare catch {} for unused exceptions.
KPI: PC:91KB | FPS:9722/8252 | ESP32:325KB | FPS:15 | heap:125KB | src:27(2374) | test:11(1284) | lizard:4w
KPI Details:
Desktop:
Binary: 91 KB
[doctest] test cases: 50 | 50 passed | 0 failed | 0 skipped
FPS: 9722, 8252 (per scenario)
=== 2 scenario(s), 2 passed, 0 failed ===
Platform boundary: PASS
ESP32:
Image: 393,357 bytes (62% partition free)
Flash (code+data): 325 KB
DRAM: 19,376 / 180,736 (161,360 free)
FPS: 15 heap free: 128412
Code:
27 source files (2374 lines)
11 test files (1284 lines)
16 specs, 2 scenarios
Lizard: 4 warnings
mm::HttpServerModule::handleConnection@59-124@src/core/HttpServerModule.h
mm::HttpServerModule::handleSetControl@301-356@src/core/HttpServerModule.h
mm::MirrorModifier::mapToPhysical@26-54@src/light/MirrorModifier.h
mm_main@12-109@src/main.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (3)
esp32/main/main.cpp (1)
39-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard EventGroup allocation before any bit operations.
xEventGroupCreate()at Line 39 can returnnullptr; subsequent uses at Line 27, Line 34, and Line 76 will dereference an invalid handle.Suggested fix
static void eth_init() { ethEventGroup = xEventGroupCreate(); + if (ethEventGroup == nullptr) { + ESP_LOGE(TAG, "Failed to create Ethernet event group"); + return; + }Also applies to: 27-27, 34-34, 76-77
🤖 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 `@esp32/main/main.cpp` at line 39, Guard the EventGroup allocation: xEventGroupCreate() can return nullptr so check the returned ethEventGroup immediately after the call and handle allocation failure (log error and abort/return or retry) rather than proceeding; also add null-checks before any subsequent calls that dereference the handle (calls that use ethEventGroup such as xEventGroupSetBits / xEventGroupWaitBits / xEventGroupClearBits referenced around the other uses) to avoid dereferencing a null handle, and ensure functions that rely on ethEventGroup fail gracefully if the group wasn't created.scripts/moondeck.py (1)
59-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard OS process tools before invoking them.
Lines 62/65/90/94 assume these executables exist; if not,
subprocess.run(...)raises and the API path fails. This affects/api/killand/api/running.Suggested fix
import http.server import json import os +import shutil import signal @@ def _kill_process_by_name(name: str): """Kill processes matching name. Cross-platform.""" if _IS_WIN: - subprocess.run(["taskkill", "/F", "/IM", name + ".exe"], - capture_output=True) + taskkill = shutil.which("taskkill") + if taskkill: + subprocess.run([taskkill, "/F", "/IM", name + ".exe"], + capture_output=True, check=False) else: - subprocess.run(["pkill", "-f", name], capture_output=True) + pkill = shutil.which("pkill") + if pkill: + subprocess.run([pkill, "-f", name], capture_output=True, check=False) @@ def is_process_running(name: str) -> bool: """Check if a process matching name is running. Cross-platform.""" if _IS_WIN: - r = subprocess.run(["tasklist", "/FI", f"IMAGENAME eq {name}.exe"], - capture_output=True, text=True) + tasklist = shutil.which("tasklist") + if not tasklist: + return False + r = subprocess.run([tasklist, "/FI", f"IMAGENAME eq {name}.exe"], + capture_output=True, text=True, check=False) return name in r.stdout else: - r = subprocess.run(["pgrep", "-f", name], capture_output=True) + pgrep = shutil.which("pgrep") + if not pgrep: + return False + r = subprocess.run([pgrep, "-f", name], capture_output=True, check=False) return r.returncode == 0Also applies to: 87-95
🤖 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 `@scripts/moondeck.py` around lines 59 - 66, The _kill_process_by_name function (and other places invoking OS tools in this file) must check that the external executable exists before calling subprocess.run: use shutil.which to verify "taskkill" on Windows and "pkill" on POSIX, and if not found return a controlled error/log and avoid calling subprocess.run (or wrap the call in try/except to catch FileNotFoundError and log/raise a clear API-level error for the /api/kill and /api/running callers); update _kill_process_by_name to perform this guard and mirror the same guard/exception handling for the other subprocess.run invocations in the file.src/platform/platform.h (1)
27-27: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdopt
std::spanfor buffer APIs at the declaration boundary.These signatures still expose pointer+length pairs; please move them to
std::spanhere so both platform implementations and callers converge on one safer contract.As per coding guidelines: "
**/*.{h,hpp,cpp,cc}: Usestd::spanover pointer + length parameters".Also applies to: 50-51
🤖 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/platform.h` at line 27, Update the pointer+length buffer parameters to use std::span at the declaration boundary: replace send(const char* ip, uint16_t port, const uint8_t* data, size_t len) with a signature that takes a std::span<const uint8_t> (and similarly change the other two functions mentioned at lines 50-51) and add the necessary `#include` <span>; ensure the symbol names (send and the two other functions referenced) keep their semantics and const-correctness so platform implementations and callers switch to the std::span<const uint8_t> contract consistently.
🧹 Nitpick comments (1)
scripts/check/collect_kpi.py (1)
113-114: ⚡ Quick winDon’t silently swallow ESP32 KPI collection failures.
Line 113-114 hides real errors and can make KPI output look valid when ESP32 metrics actually failed to collect.
Suggested fix
- except Exception: - pass + except Exception as exc: + kpi["esp32_error"] = str(exc)🤖 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 `@scripts/check/collect_kpi.py` around lines 113 - 114, Replace the silent "except Exception: pass" in collect_kpi.py (the except Exception: pass block) with error handling that records the failure and logs the exception; specifically, catch Exception as e, call the module logger (e.g., logger.exception("Failed to collect ESP32 KPI")) to capture the stack trace, set the ESP32 KPI outputs/flags returned by the collect_kpi function to a clearly-failing value (None, empty dict, or a "failed" flag) so downstream code can detect the failure, and then continue (don’t swallow) so the KPI report reflects the collection error.
🤖 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/history/plan-04.md`:
- Around line 9-31: The fenced code block that contains the file tree listing
(the block starting with "src/platform/" and entries like platform.h,
Scheduler.h, HttpServerModule.h, LayoutGroup.h, Layer.h, DriverGroup.h,
index.html, app.js, src/main.cpp, test_http_server.cpp, CMakeLists.txt) is
missing a language and triggers MD040; change the opening fence from ``` to
```text (or another appropriate language) so the block is recognized as a code
block with a language and the markdownlint warning is resolved.
In `@src/core/HttpServerModule.h`:
- Around line 94-104: The code locates "Content-Length:" (clHeader) but discards
the length and uses headerEnd+4 as body pointer, which can yield an incomplete
POST payload before handleSetControl parses JSON; update the request handling to
parse the integer length from clHeader (use std::atoi(clHeader + 15) into a
variable), compute how many bytes of the body are already present after
headerEnd, and if fewer than Content-Length read the remaining bytes from the
socket into a buffer (or resize a std::string/ std::vector<char> and recv until
length satisfied) so that the full body is assembled before calling
handleSetControl (and apply the same fix for the other similar block around
lines 114-116); reference clHeader, headerEnd, body, and handleSetControl when
implementing this change.
- Around line 213-218: The lambda in buildStateJson updates pos with the full
snprintf return value which can exceed the remaining buffer and cause
out-of-bounds writes; change the append logic (and all similar snprintf usages
in buildStateJson) to compute remaining = bufSize - pos, early-return or skip
when remaining <= 0, call std::snprintf with remaining, and then increment pos
by the clipped number of characters actually written (i.e., if snprintf returns
n, add std::min(n, remaining - 1) when n > 0), and ignore/handle negative
snprintf returns without advancing pos so subsequent writes use a correct,
bounded offset.
- Around line 64-76: The inner retry loop inside the loop20ms handler (the
for-attempt loop that repeatedly calls conn.read on buf and uses totalRead) is
blocking and must be removed; instead convert request assembly to a non-blocking
per-connection state that does one read per tick. Concretely: remove the
for(...) retry loop in the loop20ms handler, add/use a per-connection struct
that stores the partial buffer (buf), totalRead, and a timestamp/attempt
counter, and change the handler to call conn.read once per tick (or until
EAGAIN) to append into buf, null-terminate and check std::strstr(... "\r\n\r\n")
after each single read, and only keep the connection open for subsequent ticks
until headers complete or timeout; preserve the existing peer-closed (n==0)
handling and convert the old timeout/attempt logic into a tick-based timeout on
that per-connection state.
In `@src/light/DriverGroup.h`:
- Around line 42-51: The driver source buffers are currently set after invoking
drivers' onAllocateMemory(), causing drivers that size resources from those
buffers to allocate incorrectly; call passBufferToDrivers() (which sets drivers'
source buffers) before the loop that invokes drivers_[i]->onAllocateMemory(),
i.e. move the passBufferToDrivers() call to precede the for-loop in
onAllocateMemory(), and make the same change in the analogous code block that
mirrors lines 81-87 (the other method that allocates/reallocates drivers),
ensuring outputBuffer_.allocate(...) still runs before passing buffers when
needed.
In `@src/light/Layer.h`:
- Around line 19-23: The addEffect method dereferences the incoming EffectBase*
without checking for null, which can crash; update addEffect (method name:
addEffect, class likely Layer, and members effectCount_ and effects_) to first
check if effect is nullptr and bail out (or assert/log and return) before
accessing effect->setParent/setLayer and assigning into effects_, ensuring you
still enforce the existing capacity guard (effectCount_ >= effects_.size()).
In `@src/light/ModifierBase.h`:
- Around line 19-22: Change ModifierBase::mapToPhysical to take
std::span<nrOfLightsType> outPhysicals instead of nrOfLightsType* outPhysicals
and nrOfLightsType maxOut, update the pure-virtual signature accordingly, and
include <span>; then update the MirrorModifier override to match the new
signature and use outPhysicals.size() for capacity checks and writes, adjust all
call sites in Layer (and any tests) to pass a std::span (e.g., span over the
output buffer) and leave nrOfLightsType& outCount unchanged; ensure bounds
checks use outPhysicals.size() and update any Buffer.h-style utilities if
applicable.
In `@src/light/NoiseEffect.h`:
- Around line 60-62: In lerp8, the subtraction (b - a) currently happens in
uint8_t space causing wraparound when b < a; change the computation to perform
the subtraction in signed 16-bit space by casting operands to int16_t before
subtracting (e.g., compute int16_t delta = int16_t(b) - int16_t(a)), multiply
that signed delta by t (cast t to int16_t), divide by 255, then add to
int16_t(a) and finally cast the result back to uint8_t; update the static
function lerp8 to use these signed intermediate values to avoid unsigned wrap.
- Around line 45-49: The hash function uses signed int32_t arithmetic which can
overflow; change the signature and all intermediate types in hash(int32_t x,
int32_t y, int32_t t) to use uint32_t (e.g., hash(uint32_t x, uint32_t y,
uint32_t t) and uint32_t h) so arithmetic wraps definedly, keep the final return
as static_cast<uint8_t>((h >> 16) & 0xFF) to preserve behavior; update any
callers (e.g., noise2d) if needed to pass/convert unsigned values to the new
hash signature.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 97-103: TcpConnection::write currently treats any non-positive
result from ::write as a hard failure; update it to handle transient conditions
by checking errno: if n < 0 and errno == EINTR continue the loop, if errno ==
EAGAIN or errno == EWOULDBLOCK then wait for the socket to become writable (use
poll/select on fd_) and then retry rather than returning false immediately, and
only return false for permanent errors; keep the sent accounting (sent += n) and
preserve the existing fd_ check and return semantics for fatal errors.
In `@src/ui/app.js`:
- Around line 67-71: Replace the non-interactive divs used for module navigation
with keyboard-operable elements: either change the created element in the nav
creation (currently `item = document.createElement("div")`) to a real <button>
or, if keeping the div, add `tabindex="0"`, `role="button"`, and a keydown
handler that calls `selectModule(mod.name)` when Enter or Space is pressed; keep
the existing click handler and maintain `item.className` and
`aria-current`/active state updates so `selectModule` and the
`"nav-item"`/`"active"` styling still work and the elements are focusable and
operable by keyboard.
- Around line 14-15: The WebSocket URL is hardcoded to "ws://" which breaks when
the page is served over HTTPS; update the code that constructs url (the const
url and the new WebSocket(url) usage in src/ui/app.js) to choose the scheme
based on location.protocol — use "wss" when location.protocol === "https:" and
"ws" otherwise (e.g., build the URL from the chosen scheme + '//' +
location.host + '/ws' or derive from location.origin by swapping the protocol)
so the WebSocket uses the secure wss:// scheme under HTTPS.
In `@src/ui/style.css`:
- Around line 126-128: The media query currently hides the entire nav on small
screens which removes the module-switch control; update the CSS in the `@media`
(max-width: 600px) rule so it does not hide the module-switch element — for
example, target specific nav children to hide (e.g., nav .secondary or nav
.full-menu) or explicitly keep the module-switch visible by excluding it (e.g.,
nav:not(.module-switch) or nav > :not(.module-switch) remain hidden) and ensure
the element with class or id used for module switching (e.g., .module-switch or
`#module-switch`) stays display:block (or visible) on small screens.
In `@test/scenario_runner.cpp`:
- Around line 281-286: The printed pipeline delta can underflow because the
expression heapBefore - heapAfter uses unsigned values; change the printf to
compute a signed delta (e.g., static_cast<long>(static_cast<long>(heapBefore) -
static_cast<long>(heapAfter)) or use ptrdiff_t) and use a matching signed format
specifier (%ld or %td) so the "pipeline" value reports negative numbers
correctly; update the std::printf call that references heapBefore and heapAfter
to cast the delta to a signed type and adjust the format string accordingly.
- Around line 182-193: The code unsafely static_casts module pointers from
modules[props["layoutGroup"].str] and modules[props["layer"].str] then
null-checks the result, which can produce UB if the ID exists but is the wrong
derived type; change the lookup to retrieve the base pointer from modules and
use a safe RTTI check (dynamic_cast) to mm::LayoutGroup* and mm::Layer*
respectively, verify the dynamic_cast result is non-null before calling
setLayoutGroup or setLayer, and preserve the existing props.has checks and
null-path behavior. Ensure you update the two sites that now call static_cast to
use dynamic_cast and conditional early-return or skip if the cast fails.
In `@test/test_blend_map.cpp`:
- Around line 67-88: The test never exercises additive clamping because the LUT
maps logical 0 to a single physical index once; update the mapping so logical 0
maps twice to the same physical index to force a double-add. Specifically,
change the LUT build to create two physical layers (call lut.build(1,2)), set
the mapping with an array of two entries pointing to the same physical index
(e.g., mm::nrOfLightsType map[] = {0,0}; lut.setMapping(0, map, 2)), keep src
value at 200, and assert that mm::blendMap produces a clamped result
(CHECK(dst.data()[0] == 255)); references: mm::MappingLUT::build,
mm::MappingLUT::setMapping, and mm::blendMap.
---
Duplicate comments:
In `@esp32/main/main.cpp`:
- Line 39: Guard the EventGroup allocation: xEventGroupCreate() can return
nullptr so check the returned ethEventGroup immediately after the call and
handle allocation failure (log error and abort/return or retry) rather than
proceeding; also add null-checks before any subsequent calls that dereference
the handle (calls that use ethEventGroup such as xEventGroupSetBits /
xEventGroupWaitBits / xEventGroupClearBits referenced around the other uses) to
avoid dereferencing a null handle, and ensure functions that rely on
ethEventGroup fail gracefully if the group wasn't created.
In `@scripts/moondeck.py`:
- Around line 59-66: The _kill_process_by_name function (and other places
invoking OS tools in this file) must check that the external executable exists
before calling subprocess.run: use shutil.which to verify "taskkill" on Windows
and "pkill" on POSIX, and if not found return a controlled error/log and avoid
calling subprocess.run (or wrap the call in try/except to catch
FileNotFoundError and log/raise a clear API-level error for the /api/kill and
/api/running callers); update _kill_process_by_name to perform this guard and
mirror the same guard/exception handling for the other subprocess.run
invocations in the file.
In `@src/platform/platform.h`:
- Line 27: Update the pointer+length buffer parameters to use std::span at the
declaration boundary: replace send(const char* ip, uint16_t port, const uint8_t*
data, size_t len) with a signature that takes a std::span<const uint8_t> (and
similarly change the other two functions mentioned at lines 50-51) and add the
necessary `#include` <span>; ensure the symbol names (send and the two other
functions referenced) keep their semantics and const-correctness so platform
implementations and callers switch to the std::span<const uint8_t> contract
consistently.
---
Nitpick comments:
In `@scripts/check/collect_kpi.py`:
- Around line 113-114: Replace the silent "except Exception: pass" in
collect_kpi.py (the except Exception: pass block) with error handling that
records the failure and logs the exception; specifically, catch Exception as e,
call the module logger (e.g., logger.exception("Failed to collect ESP32 KPI"))
to capture the stack trace, set the ESP32 KPI outputs/flags returned by the
collect_kpi function to a clearly-failing value (None, empty dict, or a "failed"
flag) so downstream code can detect the failure, and then continue (don’t
swallow) so the KPI report reflects the collection error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd7425a1-e60a-4299-8d8f-407c8acc95bb
📒 Files selected for processing (45)
.gitignoreCLAUDE.mdCMakeLists.txtdocs/architecture-light.mddocs/architecture.mddocs/history/plan-03.mddocs/history/plan-04.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/modifiers/MirrorModifier.mddocs/plan.mddocs/testing.mdesp32/main/CMakeLists.txtesp32/main/main.cppscripts/check/collect_kpi.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jssrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/core/Scheduler.hsrc/light/BlendMap.hsrc/light/DriverGroup.hsrc/light/GridLayout.hsrc/light/Layer.hsrc/light/LayoutGroup.hsrc/light/MappingLUT.hsrc/light/MirrorModifier.hsrc/light/ModifierBase.hsrc/light/NoiseEffect.hsrc/main.cppsrc/platform/desktop/main_desktop.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/embed_ui.cmakesrc/ui/index.htmlsrc/ui/style.csstest/CMakeLists.txttest/scenario_runner.cpptest/scenarios/mirror.jsontest/test_blend_map.cpptest/test_mapping_lut.cpptest/test_mirror.cpptest/test_noise.cpp
✅ Files skipped from review due to trivial changes (8)
- docs/moonmodules/light/effects/NoiseEffect.md
- src/ui/index.html
- test/test_mapping_lut.cpp
- CLAUDE.md
- src/ui/embed_ui.cmake
- docs/architecture-light.md
- docs/moonmodules/light/modifiers/MirrorModifier.md
- docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- src/platform/desktop/main_desktop.cpp
- esp32/main/CMakeLists.txt
- src/core/MoonModule.h
- src/core/Scheduler.h
- scripts/moondeck_config.json
- src/light/LayoutGroup.h
- scripts/moondeck_ui/app.js
Comment on lines
+9
to
+31
| ``` | ||
| src/platform/ | ||
| platform.h # MODIFY: add TcpServer + TcpConnection | ||
| desktop/ | ||
| platform_desktop.cpp # MODIFY: BSD socket implementations | ||
| esp32/ | ||
| platform_esp32.cpp # MODIFY: lwIP socket implementations (same API) | ||
| src/core/ | ||
| Scheduler.h # MODIFY: add moduleCount()/module(i) accessors | ||
| HttpServerModule.h # NEW: HTTP + WebSocket + REST API + JSON state | ||
| src/light/ | ||
| LayoutGroup.h # MODIFY: add layout(i) accessor | ||
| Layer.h # MODIFY: add effectCount/effect(i)/modifierCount/modifier(i) | ||
| DriverGroup.h # MODIFY: add driverCount/driver(i) | ||
| src/ui/ | ||
| index.html # NEW: minimal HTML5 with sidebar + cards | ||
| app.js # NEW: WebSocket, tree render, control render, debounce | ||
| style.css # NEW: dark theme per ui-spec | ||
| src/main.cpp # MODIFY: wire HttpServerModule | ||
| test/ | ||
| test_http_server.cpp # NEW: JSON state, control setter, HTTP parse | ||
| CMakeLists.txt # MODIFY: add test | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced block to satisfy markdownlint.
Line 9 uses a fenced block without a language, which triggers MD040 and keeps docs lint noisy.
Suggested fix
-```
+```text
src/platform/
platform.h # MODIFY: add TcpServer + TcpConnection
desktop/
@@
test/
test_http_server.cpp # NEW: JSON state, control setter, HTTP parse
CMakeLists.txt # MODIFY: add test</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/history/plan-04.md around lines 9 - 31, The fenced code block that
contains the file tree listing (the block starting with "src/platform/" and
entries like platform.h, Scheduler.h, HttpServerModule.h, LayoutGroup.h,
Layer.h, DriverGroup.h, index.html, app.js, src/main.cpp, test_http_server.cpp,
CMakeLists.txt) is missing a language and triggers MD040; change the opening
fence from totext (or another appropriate language) so the block is
recognized as a code block with a language and the markdownlint warning is
resolved.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+64
to
+76
| for (int attempt = 0; attempt < 50 && totalRead < static_cast<int>(sizeof(buf) - 1); attempt++) { | ||
| int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead); | ||
| if (n > 0) { | ||
| totalRead += n; | ||
| // Check if we have the full headers (double CRLF) | ||
| buf[totalRead] = 0; | ||
| if (std::strstr(reinterpret_cast<char*>(buf), "\r\n\r\n")) break; | ||
| } else if (n == 0) { | ||
| return; // peer closed | ||
| } | ||
| // n == -1: nothing yet, yield and retry | ||
| platform::yield(); | ||
| } |
There was a problem hiding this comment.
Avoid retry-wait loop inside loop20ms handler.
This loop can consume a full tick while waiting for socket data. Move request assembly to non-blocking staged reads across ticks so frame work stays predictable.
As per coding guidelines: "In the hot path (render loop): no blocking operations (delay, sleep, mutex.lock() — use try_lock)".
🤖 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.h` around lines 64 - 76, The inner retry loop
inside the loop20ms handler (the for-attempt loop that repeatedly calls
conn.read on buf and uses totalRead) is blocking and must be removed; instead
convert request assembly to a non-blocking per-connection state that does one
read per tick. Concretely: remove the for(...) retry loop in the loop20ms
handler, add/use a per-connection struct that stores the partial buffer (buf),
totalRead, and a timestamp/attempt counter, and change the handler to call
conn.read once per tick (or until EAGAIN) to append into buf, null-terminate and
check std::strstr(... "\r\n\r\n") after each single read, and only keep the
connection open for subsequent ticks until headers complete or timeout; preserve
the existing peer-closed (n==0) handling and convert the old timeout/attempt
logic into a tick-based timeout on that per-connection state.
Comment on lines
+67
to
+71
| const item = document.createElement("div"); | ||
| item.className = "nav-item" + (mod.name === selectedModule ? " active" : ""); | ||
| item.textContent = mod.name; | ||
| item.addEventListener("click", () => selectModule(mod.name)); | ||
| nav.appendChild(item); |
There was a problem hiding this comment.
Make module navigation keyboard-accessible.
At Lines 67-71, interactive div elements with click handlers are not keyboard-operable. This blocks non-pointer navigation.
Proposed fix
- const item = document.createElement("div");
+ const item = document.createElement("button");
+ item.type = "button";
item.className = "nav-item" + (mod.name === selectedModule ? " active" : "");
item.textContent = mod.name;
item.addEventListener("click", () => selectModule(mod.name));
nav.appendChild(item);📝 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 item = document.createElement("div"); | |
| item.className = "nav-item" + (mod.name === selectedModule ? " active" : ""); | |
| item.textContent = mod.name; | |
| item.addEventListener("click", () => selectModule(mod.name)); | |
| nav.appendChild(item); | |
| const item = document.createElement("button"); | |
| item.type = "button"; | |
| item.className = "nav-item" + (mod.name === selectedModule ? " active" : ""); | |
| item.textContent = mod.name; | |
| item.addEventListener("click", () => selectModule(mod.name)); | |
| nav.appendChild(item); |
🤖 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 67 - 71, Replace the non-interactive divs used
for module navigation with keyboard-operable elements: either change the created
element in the nav creation (currently `item = document.createElement("div")`)
to a real <button> or, if keeping the div, add `tabindex="0"`, `role="button"`,
and a keydown handler that calls `selectModule(mod.name)` when Enter or Space is
pressed; keep the existing click handler and maintain `item.className` and
`aria-current`/active state updates so `selectModule` and the
`"nav-item"`/`"active"` styling still work and the elements are focusable and
operable by keyboard.
Comment on lines
+126
to
+128
| @media (max-width: 600px) { | ||
| nav { display: none; } | ||
| } |
There was a problem hiding this comment.
Avoid removing the only module-switch control on small screens.
At Lines 126-128, hiding nav removes module selection on mobile unless another selector exists.
🤖 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/style.css` around lines 126 - 128, The media query currently hides the
entire nav on small screens which removes the module-switch control; update the
CSS in the `@media` (max-width: 600px) rule so it does not hide the module-switch
element — for example, target specific nav children to hide (e.g., nav
.secondary or nav .full-menu) or explicitly keep the module-switch visible by
excluding it (e.g., nav:not(.module-switch) or nav > :not(.module-switch) remain
hidden) and ensure the element with class or id used for module switching (e.g.,
.module-switch or `#module-switch`) stays display:block (or visible) on small
screens.
KPI: 16384lights | PC:91KB | FPS:9155/7951 | ESP32:325KB | src:27(2377) | test:11(1282) | lizard:4w
(see KPI Details at bottom)
Fixes:
- JSON builder snprintf overflow protection (bounds-check pos)
- DriverGroup: pass buffer to drivers before their onAllocateMemory
- Layer addEffect null check
- NoiseEffect lerp8 signed subtraction (prevents uint8_t wraparound)
- NoiseEffect hash uses uint32_t (avoids signed overflow UB)
- Scenario runner heap delta uses signed long
- BlendMap clamping test: maps pixel twice to actually test 200+200→255
Features:
- GridLayout exposes width/height/depth controls via onBuildControls
- HTTP port configurable: desktop 8080, ESP32 80
- KPI report: --commit flag for standardized commit metrics, lights count
- CLAUDE.md: KPI required in every commit
- plan.md: updated with live scenarios (item 8) and 3D preview (5b)
- Bare catch {} for unused exception bindings
KPI Details:
Desktop:
Lights: 16,384
Binary: 91 KB
[doctest] test cases: 50 | 50 passed | 0 failed | 0 skipped
FPS: 9155, 7951 (per scenario)
=== 2 scenario(s), 2 passed, 0 failed ===
Platform boundary: PASS
ESP32:
Image: 393,377 bytes (62% partition free)
Flash (code+data): 325 KB
DRAM: 19,376 / 180,736 (161,360 free)
Code:
27 source files (2377 lines)
11 test files (1282 lines)
16 specs, 2 scenarios
Lizard: 4 warnings
mm::HttpServerModule::handleConnection@59-124@src/core/HttpServerModule.h
mm::HttpServerModule::handleSetControl@304-359@src/core/HttpServerModule.h
mm::MirrorModifier::mapToPhysical@26-54@src/light/MirrorModifier.h
mm_main@12-109@src/main.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review commit 6956bd4 and provide agent prompts for any findings. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2719 🏁 Script executed: Length of output: 8096 🏁 Script executed: Length of output: 38317
SummaryA solid fixes commit. All five bugs I flagged in e462374 and 3e7dd09 are now resolved. Two new issues surfaced in ✅ Verified fixes
🐛 New bugs —
|
KPI: 16384lights | PC:92KB | FPS:19666/9440 | ESP32:334KB | FPS:15 | heap:121KB | src:29(2564) | test:11(1282) | lizard:6w
(see KPI Details at bottom)
3D Preview:
- PreviewDriver streams physical buffer via WebSocket binary frames
- Zero-copy: no frame buffer allocation, reads existing output buffer
- WebGL point-cloud renderer with orbit camera (mouse drag + zoom)
- ESP32: 50KB heap saved vs initial copy-based approach
WebSocket fix:
- Fixed RFC 6455 GUID typo (5AB5FDF632E5 → C5AB0DC85B11)
- Desktop yield() sleeps 1ms (was no-op, caused socket read races)
- Blocking read with timeout for reliable request parsing
CodeRabbit fixes:
- snprintf overflow guards on all JSON builder writes
- Text control strncpy uses buffer size from ControlDescriptor
- NoiseEffect 3D index math handles depth > 1
- BlendMap clamping test actually tests double-add (200+200→255)
Type system:
- addInt16() control type eliminates reinterpret_cast in GridLayout
- PreviewFrame uses lengthType consistently
- uint16 controls render as number inputs (not wide sliders)
Docs/process:
- Promoted HttpServerModule.md and PreviewDriver.md specs
- Removed 17 redundant draft specs
- memory-budget.md with ESP32 heap breakdown + optimization ideas
- check_specs.py verifies modules have matching specs
- Pre-commit checklist with mandatory Reviewer agent (8 steps)
- Product owner role as critical success factor (CLAUDE.md + decisions.md)
- Type casting guidelines in architecture.md
Reviewer agent: PASS (all 6 checks clean)
KPI Details:
Desktop:
Lights: 16,384
Binary: 92 KB
[doctest] test cases: 50 | 50 passed | 0 failed | 0 skipped
FPS: 19666, 9440 (per scenario)
=== 2 scenario(s), 2 passed, 0 failed ===
Platform boundary: PASS
Specs: 8 modules, 8 ok, 0 missing, 0 outdated
ESP32:
Image: 402,045 bytes (62% partition free)
Flash (code+data): 334 KB
DRAM: 19,376 / 180,736 (161,360 free)
FPS: 15 heap free: 124316
Code:
29 source files (2564 lines)
11 test files (1282 lines)
18 specs, 2 scenarios
Lizard: 6 warnings
mm::HttpServerModule::handleConnection@71-136
mm::HttpServerModule::writeModuleJson@249-275
mm::HttpServerModule::writeControls@277-310
mm::HttpServerModule::handleSetControl@316-373
mm::MirrorModifier::mapToPhysical@26-54
mm_main@13-120
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/light/GridLayout.h (1)
14-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate/clamp grid dimensions before using them in
lightCount().
width/height/depthare signed and currently unbounded, so negative values can yield invalid light counts after casting and break downstream buffer sizing/index assumptions.Suggested fix
nrOfLightsType lightCount() const override { - return static_cast<nrOfLightsType>(width) * height * depth; + const lengthType w = (width > 0) ? width : 0; + const lengthType h = (height > 0) ? height : 0; + const lengthType d = (depth > 0) ? depth : 0; + return static_cast<nrOfLightsType>(w) * h * d; }🤖 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/GridLayout.h` around lines 14 - 20, The lightCount() implementation uses signed members width/height/depth directly, allowing negative or excessively large values to produce invalid counts; update lightCount() to first validate and clamp width, height, and depth to a safe non-negative range (e.g., 0 to a defined MAX_DIMENSION) and perform multiplication in an unsigned or wider integer type before casting to nrOfLightsType to avoid overflow/underflow; reference the lightCount() function and the width/height/depth members and ensure the clamp logic is applied consistently wherever those dimensions are read (and consider adding a helper like clampDimension() if needed).
♻️ Duplicate comments (6)
src/platform/desktop/platform_desktop.cpp (1)
106-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry on
EINTRin TCP write loop.Interrupted syscalls currently return
false, causing unnecessary connection failures under signal interruptions.Proposed fix
- } else if (n < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { + } else if (n < 0 && errno == EINTR) { + continue; + } else if (n < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { // Wait briefly for send buffer space struct timespec ts = {0, 1000000}; // 1ms nanosleep(&ts, nullptr);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/desktop/platform_desktop.cpp` around lines 106 - 112, The write loop currently treats any interrupted syscall as a hard error; update the error handling around the condition checking errno (the block comparing errno to EAGAIN/EWOULDBLOCK) to also handle EINTR by continuing the loop instead of returning false. In practice, add a branch that if (n < 0 && errno == EINTR) { continue; } before the EAGAIN/EWOULDBLOCK handling so that transient signal interruptions are retried, leaving the existing nanosleep retry for EAGAIN/EWOULDBLOCK and the final return false only for genuine errors.test/scenario_runner.cpp (2)
161-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParent-child wiring still uses unchecked downcasts.
These
static_castpaths can still trigger UB on bad scenario graphs (type/parent_idmismatch). Please use runtime-checked casting and fail fast on mismatch.🤖 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 161 - 173, The wiring code in test/scenario_runner.cpp uses unchecked static_casts (type, parent, mod) which can cause UB on mismatched parent_id/type; replace each static_cast to mm::LayoutGroup/Layer/DriverGroup and mm::LayoutBase/EffectBase/ModifierBase/DriverBase with a runtime-checked dynamic_cast, verify the result is non-null, and fail fast (log error and return/throw) if the cast fails before calling addLayout/addEffect/addModifier/addDriver; ensure you check the specific combinations for "GridLayout", "RainbowEffect"/"NoiseEffect", "MirrorModifier", and "ArtNetSendDriver" so incorrect parent types are detected at runtime.
183-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProp reference casts remain unsafe.
modules[...]lookups are still directly downcast withstatic_cast; wrong IDs/types can pass non-null and still be invalid.🤖 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 183 - 193, The code unsafely uses static_cast on modules[...] entries (e.g., static_cast<mm::LayoutGroup*>(modules[props["layoutGroup"].str]) and static_cast<mm::Layer*>(modules[props["layer"].str])) which can yield a non-null pointer of the wrong type; change these to safe runtime-checked casts and null checks: retrieve the module entry (ensure modules contains the key), use dynamic_cast<mm::LayoutGroup*> or dynamic_cast<mm::Layer*> to perform the downcast, check the result for nullptr, and only then call layer.setLayoutGroup(lg) or driverGroup.setLayer(l); keep the existing props.has checks and guard against missing keys before accessing props["..."].str.src/core/HttpServerModule.h (3)
75-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking retry loop still runs inside tick path.
handleConnection()still performs up to 20 read attempts in oneloop20ms()tick, which can stall frame scheduling.As per coding guidelines: "Hot path (render loop): No blocking operations (
delay,sleep,mutex.lock()— usetry_lock)."🤖 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.h` around lines 75 - 77, handleConnection() currently does a blocking retry loop (for up to 20 attempts calling conn.read on buf/totalRead) inside loop20ms(), which can stall the hot tick; refactor this read into a non-blocking state machine: replace the 20-attempt for-loop with a single non-blocking read attempt per tick (call conn.read once, update totalRead and buffer state), store the connection state (bytes read, attempts/timeout deadline) in a per-connection context so subsequent loop20ms() invocations resume the read, and move any retry/timeout logic out of the immediate tick into that state (e.g., increment attempt counter or check deadline and close on expiration). Target symbols: handleConnection(), loop20ms(), conn.read, buf, totalRead.
355-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winText control write still relies on unsafe fallback length.
Using
c.max == 0 ? ... : ...with a hard fallback length still assumes destination capacity and can write out of bounds when descriptor metadata is missing or inconsistent.🤖 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.h` around lines 355 - 357, The code is using a hardcoded fallback length when c.max is missing, which can still overflow; update the write to first validate c.ptr and c.max, do nothing if c.ptr is null or c.max == 0, compute size_t space = c.max; copy at most space-1 bytes using std::min(space - 1, strlen(v)) into c.ptr, and always NUL-terminate at index copy_len (or set c.ptr[0]='\0' when space==1); replace the current strncpy logic around the variables c and v in HttpServerModule.h with this guarded, min-based copy to avoid out-of-bounds writes.
106-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPOST body can still be parsed before full payload arrives.
Content-Lengthis detected but not used to finish body assembly beforehandleSetControl(), so partial JSON payloads are still possible.Also applies to: 126-127
🤖 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.h` around lines 106 - 116, The code finds "Content-Length:" (clHeader) but discards the parsed length and sets body as soon as headers end, allowing partial POST JSON to be processed (see body, clHeader, headerEnd and handleSetControl). Fix by parsing and storing the Content-Length (e.g., contentLength = std::atoi(clHeader + 15)), then after locating headerEnd compute available payload bytes (bytes between headerEnd+4 and the end of the current request buffer) and only set body = headerEnd + 4 and proceed to handleSetControl() when available bytes >= contentLength; otherwise defer processing/return until more data arrives. Apply the same guarded check where body is used later (the region referenced around lines 126-127).
🧹 Nitpick comments (1)
src/ui/app.js (1)
173-174: ⚡ Quick winUse server-provided bounds for
uint16controls.This branch hardcodes
[0..65535]and ignoresctrl.min/ctrl.max, which can desync UI constraints from module control definitions.Proposed fix
- input.min = 0; - input.max = 65535; + input.min = ctrl.min !== undefined ? ctrl.min : 0; + input.max = ctrl.max !== undefined ? ctrl.max : 65535;🤖 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 173 - 174, The UI is hardcoding uint16 bounds with input.min = 0 and input.max = 65535, ignoring server-provided ctrl.min / ctrl.max; update the code that sets the input element for uint16 (look for the block assigning input.min/input.max in src/ui/app.js and any variable named input or ctrl) to use ctrl.min and ctrl.max when they are defined (fall back to 0 and 65535 only if ctrl.min/ctrl.max are undefined or null), and ensure values are coerced to numbers and clamped to the uint16 range so the UI constraints stay in sync with module control definitions.
🤖 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/history/plan-05.md`:
- Line 40: The markdown fenced code block in the document is missing a language
tag (MD040); update the fenced block (the triple-backtick block present in the
diff) to declare a language (for example ```text or ```bash) so the block begins
with a language identifier to satisfy markdownlint.
In `@scripts/check/collect_kpi.py`:
- Around line 58-59: The one-line try/except usages (e.g., the inline try around
fps_values.append(int(parts[i + 1])) and the similar occurrence at the other
location) violate Ruff E701; replace each inline try/except with a multi-line
try/except block: perform the int conversion and append inside a try: block and
handle ValueError in the except: block (keeping the behavior of silently
ignoring conversion errors). Update the statements that reference fps_values,
parts and index i so they are expanded into proper multiline try/except
constructs.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 167-169: The current code sets a per-connection blocking read
timeout on accepted sockets via setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO,
...), which can stall the render loop; instead remove the SO_RCVTIMEO call and
make the accepted socket non-blocking (use fcntl or equivalent on clientFd to
set O_NONBLOCK) and update the read/poll logic that consumes clientFd to handle
EAGAIN/EWOULDBLOCK (or use select/poll/epoll) so reads never block the hot
render loop; update any logic around clientFd reads to treat would-block as “no
data” and continue.
In `@src/ui/app.js`:
- Around line 364-370: Before parsing the frame, guard against short/corrupt
buffers by checking buf.byteLength first: ensure buf.byteLength >= 7 (header
size) before creating/reading the DataView and view.getUint8/view.getUint16
calls, then read w,h,d, validate they are positive reasonable integers, compute
count = w*h*d and verify buf.byteLength >= 7 + count before constructing pixels
= new Uint8Array(buf, 7); if any check fails, bail out gracefully to avoid
exceptions from view.getUint* or pixels access.
---
Outside diff comments:
In `@src/light/GridLayout.h`:
- Around line 14-20: The lightCount() implementation uses signed members
width/height/depth directly, allowing negative or excessively large values to
produce invalid counts; update lightCount() to first validate and clamp width,
height, and depth to a safe non-negative range (e.g., 0 to a defined
MAX_DIMENSION) and perform multiplication in an unsigned or wider integer type
before casting to nrOfLightsType to avoid overflow/underflow; reference the
lightCount() function and the width/height/depth members and ensure the clamp
logic is applied consistently wherever those dimensions are read (and consider
adding a helper like clampDimension() if needed).
---
Duplicate comments:
In `@src/core/HttpServerModule.h`:
- Around line 75-77: handleConnection() currently does a blocking retry loop
(for up to 20 attempts calling conn.read on buf/totalRead) inside loop20ms(),
which can stall the hot tick; refactor this read into a non-blocking state
machine: replace the 20-attempt for-loop with a single non-blocking read attempt
per tick (call conn.read once, update totalRead and buffer state), store the
connection state (bytes read, attempts/timeout deadline) in a per-connection
context so subsequent loop20ms() invocations resume the read, and move any
retry/timeout logic out of the immediate tick into that state (e.g., increment
attempt counter or check deadline and close on expiration). Target symbols:
handleConnection(), loop20ms(), conn.read, buf, totalRead.
- Around line 355-357: The code is using a hardcoded fallback length when c.max
is missing, which can still overflow; update the write to first validate c.ptr
and c.max, do nothing if c.ptr is null or c.max == 0, compute size_t space =
c.max; copy at most space-1 bytes using std::min(space - 1, strlen(v)) into
c.ptr, and always NUL-terminate at index copy_len (or set c.ptr[0]='\0' when
space==1); replace the current strncpy logic around the variables c and v in
HttpServerModule.h with this guarded, min-based copy to avoid out-of-bounds
writes.
- Around line 106-116: The code finds "Content-Length:" (clHeader) but discards
the parsed length and sets body as soon as headers end, allowing partial POST
JSON to be processed (see body, clHeader, headerEnd and handleSetControl). Fix
by parsing and storing the Content-Length (e.g., contentLength =
std::atoi(clHeader + 15)), then after locating headerEnd compute available
payload bytes (bytes between headerEnd+4 and the end of the current request
buffer) and only set body = headerEnd + 4 and proceed to handleSetControl() when
available bytes >= contentLength; otherwise defer processing/return until more
data arrives. Apply the same guarded check where body is used later (the region
referenced around lines 126-127).
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 106-112: The write loop currently treats any interrupted syscall
as a hard error; update the error handling around the condition checking errno
(the block comparing errno to EAGAIN/EWOULDBLOCK) to also handle EINTR by
continuing the loop instead of returning false. In practice, add a branch that
if (n < 0 && errno == EINTR) { continue; } before the EAGAIN/EWOULDBLOCK
handling so that transient signal interruptions are retried, leaving the
existing nanosleep retry for EAGAIN/EWOULDBLOCK and the final return false only
for genuine errors.
In `@test/scenario_runner.cpp`:
- Around line 161-173: The wiring code in test/scenario_runner.cpp uses
unchecked static_casts (type, parent, mod) which can cause UB on mismatched
parent_id/type; replace each static_cast to mm::LayoutGroup/Layer/DriverGroup
and mm::LayoutBase/EffectBase/ModifierBase/DriverBase with a runtime-checked
dynamic_cast, verify the result is non-null, and fail fast (log error and
return/throw) if the cast fails before calling
addLayout/addEffect/addModifier/addDriver; ensure you check the specific
combinations for "GridLayout", "RainbowEffect"/"NoiseEffect", "MirrorModifier",
and "ArtNetSendDriver" so incorrect parent types are detected at runtime.
- Around line 183-193: The code unsafely uses static_cast on modules[...]
entries (e.g., static_cast<mm::LayoutGroup*>(modules[props["layoutGroup"].str])
and static_cast<mm::Layer*>(modules[props["layer"].str])) which can yield a
non-null pointer of the wrong type; change these to safe runtime-checked casts
and null checks: retrieve the module entry (ensure modules contains the key),
use dynamic_cast<mm::LayoutGroup*> or dynamic_cast<mm::Layer*> to perform the
downcast, check the result for nullptr, and only then call
layer.setLayoutGroup(lg) or driverGroup.setLayer(l); keep the existing props.has
checks and guard against missing keys before accessing props["..."].str.
---
Nitpick comments:
In `@src/ui/app.js`:
- Around line 173-174: The UI is hardcoding uint16 bounds with input.min = 0 and
input.max = 65535, ignoring server-provided ctrl.min / ctrl.max; update the code
that sets the input element for uint16 (look for the block assigning
input.min/input.max in src/ui/app.js and any variable named input or ctrl) to
use ctrl.min and ctrl.max when they are defined (fall back to 0 and 65535 only
if ctrl.min/ctrl.max are undefined or null), and ensure values are coerced to
numbers and clamped to the uint16 range so the UI constraints stay in sync with
module control definitions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac5283a1-3808-4446-a1f1-00a25331cdf6
📒 Files selected for processing (43)
CLAUDE.mddocs/architecture.mddocs/history/decisions.mddocs/history/memory-budget.mddocs/history/plan-05.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules_draft/core/Control.mddocs/moonmodules_draft/core/HttpServerModule.mddocs/moonmodules_draft/core/MoonModule.mddocs/moonmodules_draft/core/Scheduler.mddocs/moonmodules_draft/light/BlendMap.mddocs/moonmodules_draft/light/Buffer.mddocs/moonmodules_draft/light/DriverGroup.mddocs/moonmodules_draft/light/EffectBase.mddocs/moonmodules_draft/light/Layer.mddocs/moonmodules_draft/light/LayoutGroup.mddocs/moonmodules_draft/light/LightConfig.mddocs/moonmodules_draft/light/MappingLUT.mddocs/moonmodules_draft/light/Noise.mddocs/moonmodules_draft/light/drivers/ArtNetSendDriver.mddocs/moonmodules_draft/light/drivers/PreviewDriver.mddocs/moonmodules_draft/light/effects/RainbowEffect.mddocs/moonmodules_draft/light/layouts/GridLayout.mdscripts/check/check_specs.pyscripts/check/collect_kpi.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jssrc/core/Control.hsrc/core/HttpServerModule.hsrc/core/PreviewFrame.hsrc/light/DriverGroup.hsrc/light/GridLayout.hsrc/light/Layer.hsrc/light/NoiseEffect.hsrc/light/PreviewDriver.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/ui/app.jssrc/ui/index.htmlsrc/ui/style.csstest/scenario_runner.cpptest/test_blend_map.cpp
💤 Files with no reviewable changes (17)
- docs/moonmodules_draft/light/Layer.md
- docs/moonmodules_draft/light/DriverGroup.md
- docs/moonmodules_draft/light/Buffer.md
- docs/moonmodules_draft/light/LightConfig.md
- docs/moonmodules_draft/light/layouts/GridLayout.md
- docs/moonmodules_draft/light/BlendMap.md
- docs/moonmodules_draft/core/MoonModule.md
- docs/moonmodules_draft/light/EffectBase.md
- docs/moonmodules_draft/light/drivers/PreviewDriver.md
- docs/moonmodules_draft/core/HttpServerModule.md
- docs/moonmodules_draft/light/effects/RainbowEffect.md
- docs/moonmodules_draft/core/Scheduler.md
- docs/moonmodules_draft/light/Noise.md
- docs/moonmodules_draft/light/drivers/ArtNetSendDriver.md
- docs/moonmodules_draft/light/MappingLUT.md
- docs/moonmodules_draft/light/LayoutGroup.md
- docs/moonmodules_draft/core/Control.md
✅ Files skipped from review due to trivial changes (4)
- docs/moonmodules/core/HttpServerModule.md
- docs/history/decisions.md
- docs/history/memory-budget.md
- docs/architecture.md
|
|
||
| ## Files | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify a language on fenced code block (MD040).
The fenced block should declare a language (for example ```text) to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plan-05.md` at line 40, The markdown fenced code block in the
document is missing a language tag (MD040); update the fenced block (the
triple-backtick block present in the diff) to declare a language (for example
```text or ```bash) so the block begins with a language identifier to satisfy
markdownlint.
Comment on lines
+167
to
+169
| // Set read timeout (2 seconds) instead of non-blocking | ||
| struct timeval tv = {2, 0}; | ||
| setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); |
There was a problem hiding this comment.
Avoid per-connection blocking read timeout on accepted sockets.
Setting SO_RCVTIMEO=2s can stall the loop when a read blocks, hurting frame responsiveness. Prefer non-blocking client sockets and keep read polling logic consistent.
Proposed fix
- // Set read timeout (2 seconds) instead of non-blocking
- struct timeval tv = {2, 0};
- setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
+ int flags = fcntl(clientFd, F_GETFL, 0);
+ if (flags >= 0) {
+ fcntl(clientFd, F_SETFL, flags | O_NONBLOCK);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/platform_desktop.cpp` around lines 167 - 169, The
current code sets a per-connection blocking read timeout on accepted sockets via
setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, ...), which can stall the render
loop; instead remove the SO_RCVTIMEO call and make the accepted socket
non-blocking (use fcntl or equivalent on clientFd to set O_NONBLOCK) and update
the read/poll logic that consumes clientFd to handle EAGAIN/EWOULDBLOCK (or use
select/poll/epoll) so reads never block the hot render loop; update any logic
around clientFd reads to treat would-block as “no data” and continue.
…ic arrays
KPI: 16384lights | PC:109KB | tick:53/106us(FPS:18867/9433) | ESP32:337KB | src:30(2768) | test:11(1277) | lizard:5w
Live scenario testing:
- Python HTTP runner replays scenario JSON against running devices
- Per-step FPS/heap measurement with baseline regression detection
- ModuleFactory for runtime module creation by type name
- HTTP endpoints: POST /api/modules, DELETE /api/modules/{name}, GET /api/system
- MoonDeck Live tab for device discovery and scenario execution
Per-module timing:
- tickTimeUs as primary metric, FPS derived from it
- Each MoonModule tracks loopTimeUs (averaged over 1-second window)
- Scheduler times top-level modules, containers time children recursively
- /api/system returns per-module timing tree
- Console, scenarios, KPI all use tick time as source of truth
Architecture improvements:
- Generic children array in MoonModule base (replaces typed arrays in Layer/DriverGroup/LayoutGroup)
- Dynamic grow-on-demand arrays for children and ControlList (no fixed limits)
- Default lifecycle propagation in MoonModule base (setup/teardown/onBuildControls/onAllocateMemory)
- Scheduler::rebuild() replaces duplicated pipeline rebuild loops
- EffectBase derives layer() from parent() instead of separate setLayer
KPI Details:
Desktop:
Lights: 16,384
Binary: 109 KB
50 tests, 242 assertions, all passed
tick: 53us, 106us (FPS: 18867, 9433) per scenario
3 scenarios, 3 passed
Platform boundary: PASS
Specs: 8 modules, 8 ok
ESP32:
Image: 405,005 bytes (61% partition free)
Flash (code+data): 337 KB
DRAM: 19,632 / 180,736 (161,104 free)
Code:
30 source files (2768 lines)
11 test files (1277 lines)
18 specs, 3 scenarios
Lizard: 5 warnings
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (4)
src/ui/style.css (1)
153-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep module switching available on small screens.
Hiding the entire
navremoves primary module navigation on mobile unless another visible selector exists.🤖 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/style.css` around lines 153 - 155, The current media query hides the entire nav on small screens which removes module switching; instead change the CSS so only non-essential nav portions are hidden while the module switcher remains visible — update the `@media` (max-width: 600px) rule to avoid targeting the whole nav selector and specifically hide only elements like .nav-extra or .nav-links, or explicitly force .module-switch (or the selector used for module navigation) to remain display:block/flex and accessible; if needed add a mobile-specific collapsible/hamburger behavior class (e.g., .nav-collapsed) and ensure .module-switch is excluded from being hidden so module navigation stays available on small screens.src/ui/app.js (2)
14-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse protocol-aware WebSocket URL (
ws/wss).Hardcoding
ws://breaks preview/control sync when the UI is served over HTTPS.Suggested fix
function connectWs() { - const url = `ws://${location.host}/ws`; + const scheme = location.protocol === "https:" ? "wss" : "ws"; + const url = `${scheme}://${location.host}/ws`; ws = new WebSocket(url);🤖 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 14 - 15, The WebSocket URL is hardcoded with "ws://" which breaks when the page is served over HTTPS; modify the code that builds the url (variable `url`) used to construct `ws = new WebSocket(url)` so it derives the scheme from `location.protocol` (use "wss" when `location.protocol === 'https:'`, otherwise "ws") and then build the URL with that scheme and `location.host` (e.g., "<scheme>//<host>/ws") before passing it to the `WebSocket` constructor to ensure protocol-aware connections.
72-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake module navigation keyboard-operable.
Clickable
divitems are not keyboard-accessible; this blocks non-pointer navigation.Suggested fix
- const item = document.createElement("div"); + const item = document.createElement("button"); + item.type = "button"; item.className = "nav-item" + (mod.name === selectedModule ? " active" : ""); item.textContent = mod.name; item.addEventListener("click", () => selectModule(mod.name));🤖 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 72 - 76, The nav items are created as non-interactive divs (see the element created in the loop and the selectModule handler) which are not keyboard-accessible; make them operable by turning each item into a semantic interactive control (e.g., create a <button> instead of a <div> or add tabindex="0", role="button", and keydown handling for Enter/Space) and ensure click and keyboard handlers both call selectModule(mod.name), also set aria-current (or aria-pressed) when mod.name === selectedModule and preserve the "nav-item" and "active" classes so styling and focus states remain consistent.docs/history/plan-05.md (1)
40-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the fenced code block.
This block triggers MD040; use a language hint (e.g.
text) on the opening fence.Suggested fix
-``` +```text src/light/PreviewDriver.h # NEW: builds binary preview frames src/core/HttpServerModule.h # MODIFY: add binary frame broadcast src/ui/app.js # MODIFY: add WebGL 3D renderer src/ui/style.css # MODIFY: add canvas styling src/main.cpp # MODIFY: wire PreviewDriver</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/history/plan-05.mdaround lines 40 - 46, The fenced code block listing
modified files is missing a language hint which triggers MD040; update the
opening fence fromto include a language (for example use `text`) so it readstext and leave the closing fence unchanged; locate the block showing
src/light/PreviewDriver.h, src/core/HttpServerModule.h, src/ui/app.js,
src/ui/style.css, and src/main.cpp and add the language hint to that opening
fence.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/history/plan-06.md:
- Around line 105-123: The fenced code block in docs/history/plan-06.md that
lists modified files is missing a language tag; update the opening
triple-backtick to include a language (e.g.,text ordiff) so the Markdown
linter (MD040) passes and rendering/highlighting is correct; locate the block
that contains the file list (the one showing src/core/MoonModule.h,
src/core/ModuleFactory.h, etc.) and change the fence accordingly.In
@scripts/scenario/run_live_scenario.py:
- Around line 149-153: The three print calls use f-strings without placeholders,
triggering Ruff F541; update the print statements in run_live_scenario.py (the
print(f"\n---"), print(f"PASSED"), and print(f"FAILED") lines near the
results["passed"] check) to use plain string literals (remove the leading "f")
so they become print("\n---"), print("PASSED"), and print("FAILED").- Around line 247-249: The current check using path.exists() only prints
"Scenario not found" and continues; change it to fail fast by raising an
exception or exiting non‑zero: replace the print+continue with a raised
FileNotFoundError(f"Scenario not found: {path}") (or call sys.exit(1) after
printing) so the process returns a non‑zero status; ensure you update any
surrounding try/except to let this propagate and add an import for sys if you
choose sys.exit.- Line 180: The loop uses zip(results.get("steps", []), base.get("steps", []))
which silently truncates when the two step lists differ; change the comparison
to use itertools.zip_longest (or explicitly compare lengths first) to detect
extra/missing steps, and if a None is seen for either side (or lengths differ)
raise or log a clear error including both lengths and the offending indices so
mismatches aren't silently ignored; update the loop that iterates over
results.get("steps", []) and base.get("steps", []) to iterate with zip_longest
and handle None cases accordingly.In
@src/core/Control.h:
- Around line 25-26: ControlList's destructor deletes the owned pointer
controls_ but copy semantics are implicit, risking double-free; update
ControlList to either delete the copy constructor and copy assignment operator
(prevent copying) and explicitly define a move constructor and move assignment
that transfer controls_ and null out the source, or implement a deep copy in
copy ctor/assignment; ensure the same fix is applied to the other owning class
shown around lines 58-70 (same pattern), referencing the ControlList type, its
controls_ member, destructor (~ControlList), and the copy/move special member
functions when making changes.In
@src/core/HttpServerModule.h:
- Around line 510-526: handleDeleteModule currently deletes a module object
(mod) after teardown but only removes it from its parent; if parent == nullptr
the module remains registered in Scheduler and later ticks will dereference
freed memory. Before deleting mod (or at minimum whenever mod is scheduled),
unregister it from the Scheduler storage: call the Scheduler's removal API
(e.g., Scheduler::instance().removeModule(mod) or whatever
unregister/removeModuleByName exists) for the found mod, doing this regardless
of whether parent() is non-null, then proceed to teardown and delete;
alternatively remove from Scheduler first, then parent->removeChild(mod),
mod->teardown(), and delete mod to ensure no dangling scheduler pointers.- Around line 418-435: The code advances pos using snprintf return values and
then uses jsonBuf_ + pos without ensuring pos stays within JSON_BUF_SIZE,
risking out-of-bounds writes; fix by checking snprintf/write return codes and
remaining buffer space before advancing pos and before any subsequent write:
after the initial std::snprintf to fill jsonBuf_ set a size_t rem = (pos >=
JSON_BUF_SIZE) ? 0 : JSON_BUF_SIZE - pos, clamp pos to at most JSON_BUF_SIZE,
and in the module loop only call writeModuleTimingJson when rem > 1 (or whatever
minimal space is required) and update rem/pos using the documented snprintf
return (treat negative as 0 and if return >= rem set pos to JSON_BUF_SIZE and
rem=0); finally when appending the trailing "]} " do the same bounds
check/update for n before incrementing pos. Reference jsonBuf_, JSON_BUF_SIZE,
pos, std::snprintf, writeModuleTimingJson, scheduler_->moduleCount(), and
scheduler_->module(i).- Around line 458-507: handleAddModule currently creates/setup/allocates a
module when parent_id is omitted but never attaches it, leaking and returning
success; fix by when parentId[0]==0 either (A) attach the new module as a
top-level/root module (e.g. register it with your module tree or call the
existing root addChild/registerRoot function) immediately after
ModuleFactory::create and before mod->setup(), ensuring scheduler_->rebuild() is
called, or (B) if no root registration exists, reject the request (delete mod
and send 400) instead of returning success; update handleAddModule to perform
one of these two flows (use the parentId, ModuleFactory::create, mod->setName,
mod->setup, and scheduler_->rebuild symbols to locate where to change the
logic).In
@src/core/ModuleFactory.h:
- Around line 13-23: The factory lacks null-argument guards: update registerType
to return false if typeName == nullptr or fn == nullptr before storing the entry
(symbols: registerType, CreateModuleFn, types_, count_), and update create to
check its input typeName != nullptr and to skip any registered entry whose
types_[i].name == nullptr or types_[i].create == nullptr, only calling
types_[i].create() when it is non-null and returning nullptr on failure; this
prevents dereferencing a null name in std::strcmp and calling a null constructor
pointer (MoonModule* create).In
@src/core/PreviewFrame.h:
- Around line 13-14: Replace the raw pointer+length in PreviewFrame with
std::span (replace data/dataLen with span), then update call
sites: in PreviewDriver::onPostLoop() assign frame_->data =
sourceBuffer_->span(); in HttpServerModule::broadcastPreviewFrame() replace null
checks with previewFrame_->data.empty(), replace dataLen uses with
previewFrame_->data.size(), and change sendWsBinaryFrameMulti(...) to pass
previewFrame_->data.data() and previewFrame_->data.size() (or iterate spans as
needed). Ensure headers include and adjust any ownership/comments to
reflect non-owning span semantics.In
@src/light/DriverGroup.h:
- Around line 54-55: The loop in passBufferToDrivers() unconditionally
static_casts child(i) to DriverBase which is unsafe; validate each child's
role/type before casting and calling setSourceBuffer. Update the loop to check
the child's actual type or role (e.g., use a runtime type check such as
dynamic_cast to DriverBase* or an existing isDriver()/role() method on the child
object) and only call static_cast<DriverBase*>(...)->setSourceBuffer(buf) when
that check succeeds; otherwise skip the child or handle the non-driver case
appropriately to avoid undefined behavior.- Line 7: DriverGroup.h currently includes platform/platform.h and directly uses
platform timing APIs (see the calls around lines 41-43); remove the direct
platform include and calls and instead depend on a non-platform abstraction
header (e.g., add or use light/PlatformTimer.h) that exposes the minimal timer
API (names you use in DriverGroup, e.g., startTimer/now/elapsed or whatever
symbols are referenced). Implement the real platform-specific timing functions
inside src/platform/ (implementing the PlatformTimer interface) and update
DriverGroup.h to include only the non-platform header and call the abstract
functions, keeping all#ifdefsand platform-specific includes inside
src/platform/.In
@src/light/GridLayout.h:
- Around line 14-16: The code is storing possibly-negative dimensions via
controls_.addInt16("width"/"height"/"depth") which later lets
GridLayout::lightCount() multiply raw negative values and cast to nrOfLightsType
causing huge/invalid counts; before calling controls_.addInt16 (and in the
equivalent places at 19-20) validate and clamp each dimension to a non-negative
range (e.g. max(0, value)) or reject with an error so that
GridLayout::lightCount() always receives non-negative ints and
allocations/iterations remain consistent; update any related setters or
constructors in GridLayout to enforce the same non-negative invariant.In
@src/light/LayoutGroup.h:
- Around line 19-20: The loops in LayoutGroup that call childCount()/child(i)
and then static_cast to LayoutBase (to call lightCount()) must guard the child's
type before downcasting to avoid UB; update the traversal in both occurrences
(the loop at lines ~19-20 and the similar loop at ~27-29) to check the child's
role/type or use a safe cast (e.g., dynamic_cast or an isLayout/isKindOf check)
and skip non-LayoutBase children, only calling static_cast<LayoutBase*>(...) or
invoking lightCount() after verifying the cast succeeded.In
@src/ui/app.js:
- Line 377: The line creating a new Float32Array per frame (const verts = new
Float32Array(count * 6)) causes GC churn; replace it by reusing a preallocated
buffer (e.g., a module- or class-scoped buffer like vertsBuffer or
this.vertsBuffer) and only reallocate when count * 6 exceeds the current buffer
length; update/clear the buffer in the render path and slice/subarray to the
active portion instead of allocating a new Float32Array each frame.- Around line 395-404: Guard against a zero-size canvas before computing aspect
and the MVP: ensure canvas.clientWidth and canvas.clientHeight are greater than
zero (or use a safe fallback) before setting canvas.width/height, calling
gl.viewport, or computing the mvp via buildMVP(cx, cy, cz, canvas.width /
canvas.height); if height is zero either skip/update the render pass early or
compute a safe aspect (e.g., Math.max(1, canvas.height)) so buildMVP and mvp are
never called with a zero denominator; locate this logic around the canvas
width/height assignments and the buildMVP call using the identifiers canvas,
gl.viewport, buildMVP, and mvp.- Around line 178-183: The input handler currently calls sendControl with
parseInt(input.value) which can produce NaN for empty/invalid input; update the
input.addEventListener callback to parse and validate the value before POSTing:
use Number(input.value) or parseInt then check
Number.isFinite()/Number.isInteger and that the value is between 0 and 65535
(uint16 range), and only call sendControl(moduleName, ctrl.name, value) when the
check passes; otherwise skip the send (or bail out) to avoid serializing
NaN/null. Reference symbols: input.addEventListener, parseInt(input.value),
sendControl(moduleName, ctrl.name, ...), ctrl.name, moduleName, dragTimers.
Duplicate comments:
In@docs/history/plan-05.md:
- Around line 40-46: The fenced code block listing modified files is missing a
language hint which triggers MD040; update the opening fence fromto include a language (for example use `text`) so it readstext and leave the closing
fence unchanged; locate the block showing src/light/PreviewDriver.h,
src/core/HttpServerModule.h, src/ui/app.js, src/ui/style.css, and src/main.cpp
and add the language hint to that opening fence.In
@src/ui/app.js:
- Around line 14-15: The WebSocket URL is hardcoded with "ws://" which breaks
when the page is served over HTTPS; modify the code that builds the url
(variableurl) used to constructws = new WebSocket(url)so it derives the
scheme fromlocation.protocol(use "wss" whenlocation.protocol === 'https:', otherwise "ws") and then build the URL with that scheme and
location.host(e.g., "///ws") before passing it to the
WebSocketconstructor to ensure protocol-aware connections.- Around line 72-76: The nav items are created as non-interactive divs (see the
element created in the loop and the selectModule handler) which are not
keyboard-accessible; make them operable by turning each item into a semantic
interactive control (e.g., create a instead of aor add
tabindex="0", role="button", and keydown handling for Enter/Space) and ensure
click and keyboard handlers both call selectModule(mod.name), also set
aria-current (or aria-pressed) when mod.name === selectedModule and preserve the
"nav-item" and "active" classes so styling and focus states remain consistent.In
@src/ui/style.css:
- Around line 153-155: The current media query hides the entire nav on small
screens which removes module switching; instead change the CSS so only
non-essential nav portions are hidden while the module switcher remains visible
— update the@media(max-width: 600px) rule to avoid targeting the whole nav
selector and specifically hide only elements like .nav-extra or .nav-links, or
explicitly force .module-switch (or the selector used for module navigation) to
remain display:block/flex and accessible; if needed add a mobile-specific
collapsible/hamburger behavior class (e.g., .nav-collapsed) and ensure
.module-switch is excluded from being hidden so module navigation stays
available on small screens.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `af3105a3-0ddf-43b7-914b-8b464dba1bc0` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4d9f3b67759bc0ac0b8d999812e8e2689d323510 and fbdb385021bc34ac43c56e314397597993b4a865. </details> <details> <summary>📒 Files selected for processing (59)</summary> * `CLAUDE.md` * `docs/architecture.md` * `docs/history/decisions.md` * `docs/history/memory-budget.md` * `docs/history/plan-05.md` * `docs/history/plan-06.md` * `docs/moonmodules/core/HttpServerModule.md` * `docs/moonmodules/core/MoonModule.md` * `docs/moonmodules/light/drivers/PreviewDriver.md` * `docs/moonmodules_draft/core/Control.md` * `docs/moonmodules_draft/core/HttpServerModule.md` * `docs/moonmodules_draft/core/MoonModule.md` * `docs/moonmodules_draft/core/Scheduler.md` * `docs/moonmodules_draft/light/BlendMap.md` * `docs/moonmodules_draft/light/Buffer.md` * `docs/moonmodules_draft/light/DriverGroup.md` * `docs/moonmodules_draft/light/EffectBase.md` * `docs/moonmodules_draft/light/Layer.md` * `docs/moonmodules_draft/light/LayoutGroup.md` * `docs/moonmodules_draft/light/LightConfig.md` * `docs/moonmodules_draft/light/MappingLUT.md` * `docs/moonmodules_draft/light/Noise.md` * `docs/moonmodules_draft/light/drivers/ArtNetSendDriver.md` * `docs/moonmodules_draft/light/drivers/PreviewDriver.md` * `docs/moonmodules_draft/light/effects/RainbowEffect.md` * `docs/moonmodules_draft/light/layouts/GridLayout.md` * `docs/plan.md` * `docs/testing.md` * `scripts/MoonDeck.md` * `scripts/check/check_specs.py` * `scripts/check/collect_kpi.py` * `scripts/moondeck_config.json` * `scripts/moondeck_ui/app.js` * `scripts/scenario/run_live_scenario.py` * `src/core/Control.h` * `src/core/HttpServerModule.h` * `src/core/ModuleFactory.h` * `src/core/MoonModule.h` * `src/core/PreviewFrame.h` * `src/core/Scheduler.h` * `src/light/DriverGroup.h` * `src/light/EffectBase.h` * `src/light/GridLayout.h` * `src/light/Layer.h` * `src/light/LayoutGroup.h` * `src/light/ModifierBase.h` * `src/light/NoiseEffect.h` * `src/light/PreviewDriver.h` * `src/main.cpp` * `src/platform/desktop/platform_desktop.cpp` * `src/ui/app.js` * `src/ui/index.html` * `src/ui/style.css` * `test/scenario_runner.cpp` * `test/scenarios/control-change.json` * `test/test_blend_map.cpp` * `test/test_grid_layout.cpp` * `test/test_noise.cpp` * `test/test_rainbow.cpp` </details> <details> <summary>💤 Files with no reviewable changes (17)</summary> * docs/moonmodules_draft/core/Scheduler.md * docs/moonmodules_draft/core/Control.md * docs/moonmodules_draft/light/BlendMap.md * docs/moonmodules_draft/light/effects/RainbowEffect.md * docs/moonmodules_draft/light/Buffer.md * docs/moonmodules_draft/light/drivers/PreviewDriver.md * docs/moonmodules_draft/light/drivers/ArtNetSendDriver.md * docs/moonmodules_draft/light/Noise.md * docs/moonmodules_draft/light/LightConfig.md * docs/moonmodules_draft/light/DriverGroup.md * docs/moonmodules_draft/light/MappingLUT.md * docs/moonmodules_draft/core/MoonModule.md * docs/moonmodules_draft/light/layouts/GridLayout.md * docs/moonmodules_draft/light/LayoutGroup.md * docs/moonmodules_draft/core/HttpServerModule.md * docs/moonmodules_draft/light/Layer.md * docs/moonmodules_draft/light/EffectBase.md </details> <details> <summary>✅ Files skipped from review due to trivial changes (7)</summary> * test/scenarios/control-change.json * docs/history/memory-budget.md * docs/moonmodules/light/drivers/PreviewDriver.md * docs/moonmodules/core/HttpServerModule.md * docs/history/decisions.md * docs/moonmodules/core/MoonModule.md * docs/architecture.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+105
to
+123
| ``` | ||
| src/core/MoonModule.h # MODIFY: add ModuleRole, addChild, removeChild | ||
| src/core/ModuleFactory.h # NEW: type registry, create by name | ||
| src/core/HttpServerModule.h # MODIFY: POST /api/modules, DELETE, GET /api/system | ||
| src/core/Scheduler.h # MODIFY: add fps tracking | ||
| src/light/EffectBase.h # MODIFY: role() returns Effect | ||
| src/light/ModifierBase.h # MODIFY: role() returns Modifier | ||
| src/light/DriverGroup.h # MODIFY: addChild/removeChild, role() | ||
| src/light/LayoutGroup.h # MODIFY: addChild/removeChild, role() | ||
| src/light/Layer.h # MODIFY: addChild/removeChild, role() | ||
| src/main.cpp # MODIFY: register module types with factory | ||
| scripts/scenario/run_live_scenario.py # NEW: Python HTTP scenario runner | ||
| scripts/moondeck_config.json # MODIFY: add Live tab entries | ||
| scripts/moondeck_ui/index.html # MODIFY: Live tab content | ||
| scripts/moondeck_ui/app.js # MODIFY: device discovery UI | ||
| test/scenarios/control-change.json # NEW: scenario with set_control steps | ||
| docs/moonmodules/core/HttpServerModule.md # MODIFY: new endpoints | ||
| docs/testing.md # MODIFY: live scenario section | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
The file list fence is missing a language tag (MD040).
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plan-06.md` around lines 105 - 123, The fenced code block in
docs/history/plan-06.md that lists modified files is missing a language tag;
update the opening triple-backtick to include a language (e.g., ```text or
```diff) so the Markdown linter (MD040) passes and rendering/highlighting is
correct; locate the block that contains the file list (the one showing
src/core/MoonModule.h, src/core/ModuleFactory.h, etc.) and change the fence
accordingly.
Comment on lines
+149
to
+153
| print(f"\n---") | ||
| if results["passed"]: | ||
| print(f"PASSED") | ||
| else: | ||
| print(f"FAILED") |
There was a problem hiding this comment.
Remove f-string prefixes from static strings to satisfy Ruff.
These lines use f"..." with no placeholders, which triggers F541.
🧰 Tools
🪛 Ruff (0.15.13)
[error] 149-149: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 151-151: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 153-153: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@scripts/scenario/run_live_scenario.py` around lines 149 - 153, The three
print calls use f-strings without placeholders, triggering Ruff F541; update the
print statements in run_live_scenario.py (the print(f"\n---"), print(f"PASSED"),
and print(f"FAILED") lines near the results["passed"] check) to use plain string
literals (remove the leading "f") so they become print("\n---"),
print("PASSED"), and print("FAILED").
| base = baseline[name] | ||
| regressions = [] | ||
|
|
||
| for step, base_step in zip(results.get("steps", []), base.get("steps", [])): |
There was a problem hiding this comment.
Avoid silent baseline/result mismatch truncation in comparison.
zip(results_steps, baseline_steps) ignores extra steps on either side, which can hide regressions.
Proposed fix
- for step, base_step in zip(results.get("steps", []), base.get("steps", [])):
+ result_steps = results.get("steps", [])
+ base_steps = base.get("steps", [])
+ if len(result_steps) != len(base_steps):
+ print(f" Step count mismatch for '{name}': current={len(result_steps)} baseline={len(base_steps)}")
+ return
+ for step, base_step in zip(result_steps, base_steps):📝 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
| for step, base_step in zip(results.get("steps", []), base.get("steps", [])): | |
| result_steps = results.get("steps", []) | |
| base_steps = base.get("steps", []) | |
| if len(result_steps) != len(base_steps): | |
| print(f" Step count mismatch for '{name}': current={len(result_steps)} baseline={len(base_steps)}") | |
| return | |
| for step, base_step in zip(result_steps, base_steps): |
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 180-180: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@scripts/scenario/run_live_scenario.py` at line 180, The loop uses
zip(results.get("steps", []), base.get("steps", [])) which silently truncates
when the two step lists differ; change the comparison to use
itertools.zip_longest (or explicitly compare lengths first) to detect
extra/missing steps, and if a None is seen for either side (or lengths differ)
raise or log a clear error including both lengths and the offending indices so
mismatches aren't silently ignored; update the loop that iterates over
results.get("steps", []) and base.get("steps", []) to iterate with zip_longest
and handle None cases accordingly.
Comment on lines
+247
to
+249
| if not path.exists(): | ||
| print(f"Scenario not found: {path}") | ||
| continue |
There was a problem hiding this comment.
Fail fast when a requested scenario file is missing.
Current flow prints “Scenario not found” and continues, which can still exit 0 with no executed scenarios.
Proposed fix
- if not path.exists():
- print(f"Scenario not found: {path}")
- continue
+ if not path.exists():
+ print(f"Scenario not found: {path}")
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not path.exists(): | |
| print(f"Scenario not found: {path}") | |
| continue | |
| if not path.exists(): | |
| print(f"Scenario not found: {path}") | |
| sys.exit(1) |
🤖 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 `@scripts/scenario/run_live_scenario.py` around lines 247 - 249, The current
check using path.exists() only prints "Scenario not found" and continues; change
it to fail fast by raising an exception or exiting non‑zero: replace the
print+continue with a raised FileNotFoundError(f"Scenario not found: {path}")
(or call sys.exit(1) after printing) so the process returns a non‑zero status;
ensure you update any surrounding try/except to let this propagate and add an
import for sys if you choose sys.exit.
Comment on lines
+14
to
+16
| controls_.addInt16("width", width); | ||
| controls_.addInt16("height", height); | ||
| controls_.addInt16("depth", depth); |
There was a problem hiding this comment.
Reject/clamp negative dimensions before computing lightCount().
addInt16 allows negative values, but lightCount() multiplies raw dimensions and casts to nrOfLightsType. That can yield huge counts and mismatched allocation vs iteration behavior.
Also applies to: 19-20
🤖 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/GridLayout.h` around lines 14 - 16, The code is storing
possibly-negative dimensions via controls_.addInt16("width"/"height"/"depth")
which later lets GridLayout::lightCount() multiply raw negative values and cast
to nrOfLightsType causing huge/invalid counts; before calling controls_.addInt16
(and in the equivalent places at 19-20) validate and clamp each dimension to a
non-negative range (e.g. max(0, value)) or reject with an error so that
GridLayout::lightCount() always receives non-negative ints and
allocations/iterations remain consistent; update any related setters or
constructors in GridLayout to enforce the same non-negative invariant.
Comment on lines
+19
to
+20
| for (uint8_t i = 0; i < childCount(); i++) { | ||
| total += static_cast<LayoutBase*>(child(i))->lightCount(); |
There was a problem hiding this comment.
Guard child type before downcasting in layout traversal.
Both loops assume every child is a LayoutBase; a non-layout child causes undefined behavior via static_cast. Add a role/type guard before casting.
Proposed fix
- for (uint8_t i = 0; i < childCount(); i++) {
- total += static_cast<LayoutBase*>(child(i))->lightCount();
+ for (uint8_t i = 0; i < childCount(); i++) {
+ MoonModule* m = child(i);
+ if (!m || m->role() != ModuleRole::Layout) continue;
+ total += static_cast<LayoutBase*>(m)->lightCount();
}
@@
- for (uint8_t i = 0; i < childCount(); i++) {
- auto* layout = static_cast<LayoutBase*>(child(i));
+ for (uint8_t i = 0; i < childCount(); i++) {
+ MoonModule* m = child(i);
+ if (!m || m->role() != ModuleRole::Layout) continue;
+ auto* layout = static_cast<LayoutBase*>(m);Also applies to: 27-29
🤖 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/LayoutGroup.h` around lines 19 - 20, The loops in LayoutGroup that
call childCount()/child(i) and then static_cast to LayoutBase (to call
lightCount()) must guard the child's type before downcasting to avoid UB; update
the traversal in both occurrences (the loop at lines ~19-20 and the similar loop
at ~27-29) to check the child's role/type or use a safe cast (e.g., dynamic_cast
or an isLayout/isKindOf check) and skip non-LayoutBase children, only calling
static_cast<LayoutBase*>(...) or invoking lightCount() after verifying the cast
succeeded.
Comment on lines
+178
to
+183
| input.addEventListener("input", () => { | ||
| dragTs[key] = Date.now(); | ||
| clearTimeout(dragTimers[key]); | ||
| dragTimers[key] = setTimeout(() => { | ||
| sendControl(moduleName, ctrl.name, parseInt(input.value)); | ||
| }, 500); |
There was a problem hiding this comment.
Guard uint16 input parsing before POSTing.
If the number box is temporarily empty/invalid, parseInt can produce NaN, which serializes to null in JSON and can corrupt control updates.
🤖 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 178 - 183, The input handler currently calls
sendControl with parseInt(input.value) which can produce NaN for empty/invalid
input; update the input.addEventListener callback to parse and validate the
value before POSTing: use Number(input.value) or parseInt then check
Number.isFinite()/Number.isInteger and that the value is between 0 and 65535
(uint16 range), and only call sendControl(moduleName, ctrl.name, value) when the
check passes; otherwise skip the send (or bail out) to avoid serializing
NaN/null. Reference symbols: input.addEventListener, parseInt(input.value),
sendControl(moduleName, ctrl.name, ...), ctrl.name, moduleName, dragTimers.
|
|
||
| // Build vertex data: [x,y,z,r,g,b] per point | ||
| const maxDim = Math.max(w, h, d); | ||
| const verts = new Float32Array(count * 6); |
There was a problem hiding this comment.
Avoid per-frame vertex allocation in the render hot path.
new Float32Array(count * 6) each frame will introduce avoidable GC churn/jank in preview rendering. Reuse a preallocated buffer and grow only when needed.
Based on learnings: Allocate buffers as single contiguous blocks outside the hot path. Never allocate small scattered objects in loops.
🤖 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` at line 377, The line creating a new Float32Array per frame
(const verts = new Float32Array(count * 6)) causes GC churn; replace it by
reusing a preallocated buffer (e.g., a module- or class-scoped buffer like
vertsBuffer or this.vertsBuffer) and only reallocate when count * 6 exceeds the
current buffer length; update/clear the buffer in the render path and
slice/subarray to the active portion instead of allocating a new Float32Array
each frame.
Comment on lines
+395
to
+404
| canvas.width = canvas.clientWidth; | ||
| canvas.height = canvas.clientHeight; | ||
| gl.viewport(0, 0, canvas.width, canvas.height); | ||
|
|
||
| // Camera | ||
| const cx = camDist * Math.cos(camPhi) * Math.sin(camTheta); | ||
| const cy = camDist * Math.sin(camPhi); | ||
| const cz = camDist * Math.cos(camPhi) * Math.cos(camTheta); | ||
| const mvp = buildMVP(cx, cy, cz, canvas.width / canvas.height); | ||
|
|
There was a problem hiding this comment.
Guard zero-size canvas before aspect/matrix calculation.
When clientHeight is 0 (initial hidden layout/resizes), canvas.width / canvas.height becomes invalid and can poison the MVP matrix.
🤖 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 395 - 404, Guard against a zero-size canvas
before computing aspect and the MVP: ensure canvas.clientWidth and
canvas.clientHeight are greater than zero (or use a safe fallback) before
setting canvas.width/height, calling gl.viewport, or computing the mvp via
buildMVP(cx, cy, cz, canvas.width / canvas.height); if height is zero either
skip/update the render pass early or compute a safe aspect (e.g., Math.max(1,
canvas.height)) so buildMVP and mvp are never called with a zero denominator;
locate this logic around the canvas width/height assignments and the buildMVP
call using the identifiers canvas, gl.viewport, buildMVP, and mvp.
Collaborator
Author
|
@coderabbitai, can you give a consolidated prompt for agents (like you normally do) for all the findings on last commit you listed above |
|
Tip For best results, initiate chat on the files or code changes.
|
…y, consistent naming
KPI: 16384lights | PC:111KB | tick:49/55/102/101us(FPS:20408/18181/9803/9900) | ESP32:339KB | src:30(2911) | test:11(1302) | lizard:5w
Memory reporting:
- classSize() via ModuleFactory template (no per-class override boilerplate)
- dynamicBytes tracking per module (Layer, DriverGroup report actual heap usage)
- MappingLUT::memoryUsed() and estimateBytes() for pre-flight estimation
- /api/system returns classSize + heap per module, freeInternalHeap for ESP32
- Console boot line shows sizeof, per-module timing includes heap
Adaptive allocation:
- Layer checks heap before LUT allocation, degrades to 1:1 identical if insufficient
- Layer halves buffer dimensions if buffer doesn't fit (minimum 8x8)
- HEAP_RESERVE (32KB) checked against internal heap (stack/HTTP/WiFi need internal RAM)
- canAllocate() helper eliminates duplicated budget logic
- Modifier maxMultiplier() for tighter LUT estimation (4x for XY mirror, not 8x)
Consistent naming:
- Mapping types: 1:1 identical, 1:1 shuffled, 1:0 unmapped, 1:N multimap
- isOneToOne() → hasLUT() (inverted, asks the right question)
- setOneToOne() → setIdentity()
- MoonModule name stored as char[24] buffer (fixes dangling pointer from HTTP module creation)
Live scenario improvements:
- Non-destructive: tracks created modules, deletes on cleanup
- Relative FPS bounds (min_pct) instead of absolute min — works on any device
- 3s settle time for ESP32 pipeline stabilization
- Scenario JSON ids match running module names
- Device discovery: subnet scan on ports 80+8080, refresh for online/offline status
- MoonDeck Live tab: checkboxes, status dots, persistent device list, remove button
New files:
- docs/performance.md — per-module timing + memory for desktop and ESP32
- test/scenarios/memory-1to1.json — verifies 1:1 identical uses no LUT/driver buffer
- test/scenarios/memory-lut.json — verifies modifier allocates LUT + driver buffer
- docs/history/plan-07.md — adaptive memory allocation plan
KPI Details:
Desktop:
Lights: 16,384
Binary: 111 KB
50 tests, 242 assertions, all passed
tick: 49us, 55us, 102us, 101us (FPS: 20408, 18181, 9803, 9900) per scenario
5 scenarios, 5 passed
Platform boundary: PASS
Specs: 8 modules, 8 ok
ESP32:
Image: 406,973 bytes (61% partition free)
Flash (code+data): 339 KB
DRAM: 19,760 / 180,736 (160,976 free)
Code:
30 source files (2911 lines)
11 test files (1302 lines)
18 specs, 5 scenarios
Lizard: 5 warnings
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/platform/esp32/platform_esp32.cpp (1)
101-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unbounded blocking retries in
TcpConnection::write.The EAGAIN path retries forever with
vTaskDelay(1), so a congested socket can stall the caller indefinitely.Based on learnings: "Hot path (render loop): No blocking calls (
delay,sleep,mutex.lock()— usetry_lockinstead)".🤖 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 101 - 107, TcpConnection::write currently loops on lwip_write when it returns EAGAIN/EWOULDBLOCK and uses vTaskDelay(1) which can block indefinitely; change this to a bounded retry/timeout strategy: detect EAGAIN/EWOULDBLOCK from lwip_write(fd_) and either (a) poll the socket with select/poll (or lwip equivalent) with a configurable short timeout before retrying, or (b) maintain a retry counter and/or elapsed-time timeout and then return an error/partial-write instead of spinning forever; ensure you update callers to handle a short timeout/partial write, remove the unbounded vTaskDelay(1) stall, and reference TcpConnection::write, lwip_write, fd_, errno (EAGAIN/EWOULDBLOCK) and vTaskDelay in your changes.src/light/MappingLUT.h (1)
49-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
logicalIdxbounds guard can corrupt LUT state.Line 52 writes
offsets_[logicalIdx]without validatinglogicalIdx < logicalCount_. A bad caller can write out of bounds.Proposed fix
void setMapping(nrOfLightsType logicalIdx, const nrOfLightsType* physicals, nrOfLightsType count) { - if (!offsets_ || !destinations_) return; + if (!offsets_ || !destinations_ || logicalIdx >= logicalCount_) 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/light/MappingLUT.h` around lines 49 - 55, The setMapping function writes offsets_[logicalIdx] without checking bounds; add a guard that validates logicalIdx < logicalCount_ (and that logicalCount_ > 0) before touching offsets_ or destinations_, returning early if the index is out of range to avoid corrupting the LUT; use the existing null checks for offsets_ and destinations_ and apply the new bounds check at the start of setMapping (function name: setMapping, symbols: offsets_, destinations_, destinationCount_, destinationCapacity_, logicalCount_) and keep the rest of the logic unchanged.test/scenario_runner.cpp (2)
252-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnknown module types should fail the scenario, not skip it.
Line 254 currently skips unsupported module types and keeps the run green. That can hide broken or stale scenario definitions.
Proposed fix
auto* mod = ctx.createModule(type); if (!mod) { - std::printf(" SKIP %s (unknown type: %s)\n", name, type); - continue; + std::printf(" FAIL %s (unknown type: %s)\n", name, type); + result.passed = false; + result.failures++; + continue; }🤖 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 252 - 256, The current logic treats unknown module types as a benign skip; instead, when ctx.createModule(type) returns nullptr (the mod variable), fail the scenario immediately: emit a clear error to stderr including name and type (use name and type variables), and terminate the run with a non-zero failure (e.g. return a failure status from the scenario runner, call std::exit(EXIT_FAILURE), or throw an exception) so the CI/test harness sees the broken/stale scenario; update the block around ctx.createModule(type) / mod accordingly to stop continuing and report failure.
267-270:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
set_controlis a no-op, so control scenarios can pass without applying changes.Line 269 makes control operations informational only. That gives false confidence for scenarios that depend on runtime control mutation.
🤖 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 267 - 270, The branch handling strcmp(op, "set_control") currently only prints and is a no-op, which lets control-dependent scenarios pass incorrectly; either implement applying the control change or make the runner fail when controls are requested. In scenario_runner.cpp where op and name are handled, replace the comment with a call to your control-application function (e.g., apply_control(name, value) or replay.applyControl(name, value) depending on your API) and handle/report errors, or if no implementation exists, log a clear error and return/abort the scenario (non-zero/false) so tests don't silently succeed.src/ui/app.js (1)
29-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-render when the module tree changes.
state = data; updateValues();only patches existing inputs. If/api/stateadds, removes, or renames modules/controls, the nav and cards stay stale even though the server is now pushing full replacement state. Detect structural changes and callrenderNav()/renderCards()before falling back toupdateValues().🤖 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 29 - 37, The WebSocket onmessage handler currently just replaces state and calls updateValues(), which misses structural changes; modify the ws.onmessage block to detect structural differences between the existing state and incoming data (e.g., compare module lists/IDs/names or Object.keys(state.modules) vs Object.keys(data.modules), or a shallow schema JSON compare) and if any modules/controls were added/removed/renamed, set state = data and call renderNav() and renderCards() (then return); otherwise set state = data and call updateValues(); keep the existing ArrayBuffer handling with renderPreviewFrame unchanged and use the same symbols ws.onmessage, renderPreviewFrame, renderNav, renderCards, updateValues, and state to locate the change.
♻️ Duplicate comments (6)
src/light/ModifierBase.h (1)
23-26: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse
std::spanfor output range instead of pointer + capacity pair.
mapToPhysicalcurrently exposes raw pointer/capacity parameters, which violates the project rule and weakens bounds safety for an interface used across modifiers/layer code.#!/bin/bash # Verify all mapToPhysical declarations/overrides/call sites that must be migrated. rg -n -C2 '\bmapToPhysical\s*\(' --type=cpp --type=h --type=ccAs per coding guidelines: "
**/*.{h,cpp,cc}: Usestd::spanover pointer + length parameters".🤖 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/ModifierBase.h` around lines 23 - 26, The mapToPhysical API exposes a raw pointer + capacity (outPhysicals, outCount, maxOut) which breaks the guideline—change the pure-virtual signature in ModifierBase (and all overrides) to accept a std::span<nrOfLightsType> outPhysicals and nrOfLightsType& outCount (remove maxOut), include <span>, and use outPhysicals.size() as the capacity; update every implementation of mapToPhysical to write no more than outPhysicals.size() elements and set outCount to the number written, and update all call sites to pass a std::span (e.g., std::span{buf, len} or std::span(vec)) instead of a raw pointer and maxOut.src/core/ModuleFactory.h (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd null guards before registration and factory dispatch.
typeName/fnand stored entries are dereferenced/called without validation, so null inputs can crash instd::strcmpor function invocation.Proposed fix
static bool registerType(const char* typeName, CreateModuleFn fn, size_t classSize = 0) { + if (!typeName || !fn) return false; if (count_ >= MAX_TYPES) return false; types_[count_] = {typeName, fn, classSize}; count_++; return true; } static MoonModule* create(const char* typeName) { + if (!typeName) return nullptr; for (uint8_t i = 0; i < count_; i++) { - if (std::strcmp(types_[i].name, typeName) == 0) { + if (types_[i].name && types_[i].create && + std::strcmp(types_[i].name, typeName) == 0) { auto* mod = types_[i].create(); if (mod) {Also applies to: 27-30
🤖 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/ModuleFactory.h` around lines 19 - 23, Add null guards to prevent dereferencing null pointers: in registerType(const char* typeName, CreateModuleFn fn, size_t classSize = 0) check that typeName != nullptr and fn != nullptr (and fail/return false) before accessing types_ or incrementing count_; similarly update the factory dispatch code that iterates types_ (the code around lines 27-30 that calls std::strcmp and invokes CreateModuleFn) to skip or treat entries with null typeName or null fn and avoid calling fn when it's null, returning a safe failure (false/nullptr) as appropriate; reference symbols: registerType, types_, count_, MAX_TYPES, CreateModuleFn and the factory dispatch function that compares typeName via std::strcmp and calls the stored CreateModuleFn.src/light/LayoutGroup.h (1)
19-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard child role before
static_cast<LayoutBase*>.Both traversals assume every child is a layout; a non-layout child makes the cast undefined behavior.
Proposed fix
for (uint8_t i = 0; i < childCount(); i++) { - total += static_cast<LayoutBase*>(child(i))->lightCount(); + MoonModule* module = child(i); + if (!module || module->role() != ModuleRole::Layout) continue; + total += static_cast<LayoutBase*>(module)->lightCount(); } @@ for (uint8_t i = 0; i < childCount(); i++) { - auto* layout = static_cast<LayoutBase*>(child(i)); + MoonModule* module = child(i); + if (!module || module->role() != ModuleRole::Layout) continue; + auto* layout = static_cast<LayoutBase*>(module);Also applies to: 27-29
🤖 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/LayoutGroup.h` around lines 19 - 20, The loops in LayoutGroup that iterate children and use static_cast<LayoutBase*> (calling child(i)->lightCount()) assume every child is a layout, which is unsafe; update both occurrences (the loop at the shown diff and the one at lines 27-29) to first fetch the child into a local (e.g. auto *c = child(i)), guard its role/type (e.g. if (!c || c->role() != Role::Layout) continue) or use a safe cast (e.g. dynamic_cast/qt-cast and check for null) and only then static_cast to LayoutBase* before calling lightCount(); skip non-layout children.src/platform/platform.h (1)
28-28: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSwitch
UdpSocket::sendtostd::span<const uint8_t>.This still violates the project API rule for pointer+length pairs.
As per coding guidelines: "
**/*.{h,cpp,cc}: Usestd::spanover pointer + length parameters".🤖 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/platform.h` at line 28, The declaration bool send(const char* ip, uint16_t port, const uint8_t* data, size_t len); in platform.h / UdpSocket::send should be changed to accept std::span<const uint8_t> for the payload (e.g., bool UdpSocket::send(const char* ip, uint16_t port, std::span<const uint8_t> data)); update the corresponding implementation of UdpSocket::send to use the span's data() and size(), add `#include` <span> where needed, and update all callers to pass a std::span (or construct one from containers/buffers) instead of pointer+length. Ensure signature changes are applied consistently in header and implementation.src/core/Control.h (1)
25-26:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
ControlListis copyable while owning raw memory (double-free risk).
~ControlList()freescontrols_, but copy ctor/assignment are implicit. Any copy aliases ownership and can crash on destruction.#!/bin/bash # Verify Rule-of-5 safety for ControlList rg -n "class ControlList|~ControlList|ControlList\\(const ControlList|operator=\\(const ControlList|ControlList\\(ControlList&&|operator=\\(ControlList&&" src/core/Control.h -C 2As per coding guidelines: code in
**/*.{h,cpp,cc}must remain robust and predictable; owning raw pointers without Rule-of-5 protections violates that expectation.Also applies to: 58-70
🤖 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/Control.h` around lines 25 - 26, ControlList currently owns raw memory (controls_) and defines a destructor (~ControlList) but has implicit copy/move operations, causing double-free risk; fix by making ControlList non-copyable or implementing the Rule of Five: either delete the copy constructor and copy assignment (operator=(const ControlList&)) and/or provide a safe move constructor and move assignment (ControlList(ControlList&&) and ControlList& operator=(ControlList&&)) or replace raw controls_ with an owning container/smart pointer (e.g., std::vector or std::unique_ptr<Control[]>). Update the class declarations (ControlList, controls_, ~ControlList) accordingly so ownership is unambiguous and destruction is safe.src/platform/desktop/platform_desktop.cpp (1)
171-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccepted TCP sockets should be non-blocking, not timeout-blocking.
Line 171–173 sets
SO_RCVTIMEO=2s, which allows blocking reads and can stall loop responsiveness under slow clients.#!/bin/bash # Verify accepted-socket mode and timeout usage rg -n "accept\\(|SO_RCVTIMEO|O_NONBLOCK|fcntl\\(" src/platform/desktop/platform_desktop.cpp -C 3Based on learnings: Hot path (render loop) must avoid blocking calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/desktop/platform_desktop.cpp` around lines 171 - 173, The accepted-socket is currently using SO_RCVTIMEO which allows blocking reads and can stall the render loop; instead make the accepted socket non-blocking: after accept(...) (where clientFd is created) remove the setsockopt SO_RCVTIMEO usage and call fcntl(clientFd, F_SETFL, flags | O_NONBLOCK) (retrieve existing flags with F_GETFL first) and handle EAGAIN/EWOULDBLOCK in your read paths; also check and log errors from fcntl to avoid silent failures.
🧹 Nitpick comments (3)
test/test_noise.cpp (1)
34-58: ⚡ Quick winAdd a 3D regression case (
depth > 1) forNoiseEffect.Current coverage only exercises
depth = 1, so index-math bugs in 3D traversal can pass undetected.Also applies to: 60-95
🤖 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/test_noise.cpp` around lines 34 - 58, Add a new test that exercises NoiseEffect with a 3D layout (set mm::GridLayout::depth > 1 and attach it to mm::LayoutGroup, create mm::Layer, setChannelsPerLight, add mm::NoiseEffect, call layer.onAllocateMemory() and layer.loop()) and then verify that values at different z slices or 3D corner positions are not all identical (compare RGB at index for e.g. first voxel and a voxel in a different z slice or opposite corner) to catch index-math bugs in 3D traversal; reuse the existing test structure (LayoutGroup, GridLayout, Layer, NoiseEffect, layer.buffer().data()) but with depth > 1 and appropriate index calculations for width/height/depth to assert variation across the third dimension.src/light/MappingLUT.h (1)
49-49: ⚡ Quick winUse
std::spaninstead of pointer+length insetMapping()API.The method signature violates the project guideline requiring
std::spanover pointer + length parameters. The implementation uses only index-based access and the single call site in Layer.h:135 can passstd::span(physicals, count).🤖 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/MappingLUT.h` at line 49, Change the setMapping API to accept a std::span<const nrOfLightsType> instead of (const nrOfLightsType* physicals, nrOfLightsType count): update the declaration and implementation of MappingLUT::setMapping to take std::span<const nrOfLightsType> mapping, include <span>, and use mapping[i] (or mapping.size()) where the code currently indexes the pointer/uses count; then update the call site that passes physicals/count to call setMapping(std::span(physicals, count)) (e.g., the caller in Layer), preserving the nrOfLightsType element type and constness.scripts/check/check_specs.py (1)
42-50: ⚡ Quick winUse source-directory mapping when resolving spec files.
find_spec()currently matches by stem across alldocs/moonmodules/**, which can pick the wrong file if names collide. UseSOURCE_DIRS+ module parent directory to constrain lookup.Proposed refactor
def find_spec(module_path): """Find the matching spec .md file for a source .h file.""" name = module_path.stem # e.g. "NoiseEffect" - - # Search all spec directories - for md in SPECS.rglob("*.md"): - if md.stem == name: - return md + rel = module_path.relative_to(SRC) + top = rel.parts[0] if rel.parts else "" + spec_dir = SOURCE_DIRS.get(top) + if spec_dir: + candidate = SPECS / spec_dir / f"{name}.md" + if candidate.exists(): + return candidate + # Fallback for legacy placements + for md in SPECS.rglob("*.md"): + if md.stem == name: + return md return None🤖 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 `@scripts/check/check_specs.py` around lines 42 - 50, find_spec currently scans all SPECS by stem and can pick wrong collisions; instead determine which entry in SOURCE_DIRS contains module_path.parent (or the immediate source subdir for the module), map that index to the corresponding spec subdirectory under SPECS, and only search that constrained spec directory for md files matching module_path.stem; implement this in find_spec by locating the matching source dir (using SOURCE_DIRS), computing the relative module folder, building the candidate spec-directory path under SPECS, searching that path for md.stem == module_path.stem, and fall back to the previous global search only if no matching source-dir mapping is found.
🤖 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/history/plan-07.md`:
- Around line 137-141: Two fenced code blocks currently use unlabeled
triple-backtick fences (the block starting with "PREDICT Layer buffer..." and
the block listing "src/core/MoonModule.h ..." ) which triggers markdownlint
MD040; fix by adding a language identifier (use text) to both opening fences
(i.e., replace ``` with ```text for both blocks) so the blocks become fenced as
```text ... ``` and markdownlint will pass.
In `@docs/performance.md`:
- Around line 39-46: The table in docs/performance.md has a column mismatch: the
header defines four columns ("| Module | Time (us) | % of tick | Notes |") but
the "ArtNet (97 UDP packets)" row only contains three pipe-separated cells;
update that row to include the missing fourth cell (e.g., add a Notes cell or an
explicit empty cell) so every row matches the header format and MD056 is
resolved.
In `@scripts/moondeck.py`:
- Around line 81-96: The current scan spawns one thread per probe (probe
function) which creates ~509 threads; replace this with a bounded worker pool
(e.g., concurrent.futures.ThreadPoolExecutor or a Queue + fixed worker threads)
so you submit probe(ip, port) tasks instead of creating a Thread per probe; use
a fixed max_workers (e.g., 20-50) to limit concurrency, submit tasks for the
subnet ports and localhost:8080, collect futures and use
future.result(timeout=3.0) or as_completed with timeouts to enforce the
per-probe join behavior and time limits; update the loop that builds threads and
the final join block to work with the executor/futures and ensure daemon
behavior/clean shutdown.
In `@scripts/scenario/run_live_scenario.py`:
- Around line 107-111: The code currently treats op == "add_module" as a warning
and does not set results["passed"] = False, allowing module-creation failures to
be treated as passed; update both places that print warnings (the blocks using
op, step_name, msg, and results) so that when op == "add_module" you print the
failure message the same as other ops and set results["passed"] = False (i.e.,
remove the special-case that downgrades "add_module" to a warning and ensure
results["passed"] is set to False on any non-successful op).
In `@src/core/HttpServerModule.h`:
- Around line 242-243: buildStateJson may truncate when jsonBuf_ fills but the
code always replies 200 with the partial buffer; change serveState (and the
other call sites that call buildStateJson around the 247-268 range) to detect
truncation by checking the returned int from buildStateJson and treat a return
indicating the buffer limit (e.g., len >= JSON_BUF_SIZE or other sentinel
returned by buildStateJson) as an error: do not call sendResponse with a
truncated jsonBuf_, instead send a proper error response (500 or 413) and/or a
small valid error JSON body; update both the initial call at the shown diff and
the other calls in that block to follow the same check and error path.
In `@src/core/MoonModule.h`:
- Line 13: MoonModule owns a raw pointer children_ but still allows implicit
copy/move which causes double-free; explicitly disable copying and moving by
deleting the copy constructor, copy assignment operator, move constructor, and
move assignment operator on the MoonModule class (so any use of MoonModule
cannot be copied or moved), and ensure the destructor (virtual ~MoonModule())
remains unchanged; apply the same deletion pattern to the other classes/sections
noted (the occurrences referenced around the other similar definitions).
- Around line 58-64: The removeChild method currently shifts children and
decrements childCount_ but does not clear the removed child's parent pointer;
update removeChild(MoonModule* child) so that before shifting/removing you
capture the matched child (children_[i]) and set its parent pointer (e.g.,
parent_ or setParent(nullptr) depending on the class API) to nullptr, then
perform the shift and decrement childCount_; reference removeChild, children_,
childCount_, and the module's parent_ (or setParent) symbol when making the
change.
In `@src/light/Layer.h`:
- Around line 173-188: The loop uses canAllocate(...) as a prediction but then
calls buffer_.allocate(...) without checking for failure; update the Layer
allocation path (around buffer_.allocate, canAllocate, setDynamicBytes, and
lut_.memoryUsed) to detect allocation failure after buffer_.allocate returns (or
by checking a post-condition like buffer_.isAllocated() or buffer_.bytes()>0),
and if allocation fails continue degrading (retry with smaller
width_/height_/depth_), and only call setDynamicBytes(...) when allocation
actually succeeded; if you reach the minimum and allocation still fails, log an
error and set dynamic bytes to zero (or abort cleanly) instead of assuming
allocation succeeded.
In `@src/light/NoiseEffect.h`:
- Around line 31-37: The loop that computes x and y from i ignores the depth
slice so every z gets identical noise; compute a z coordinate from i (e.g.
lengthType z = static_cast<lengthType>((i / wh) % depth) or lengthType z =
static_cast<lengthType>(i / wh) when depth == count/wh) and pass it into the
noise sampler instead of calling noise2d(x, y, t) — either call the 3D noise
function (noise3d(x, y, z, t)) or the appropriate noise2d overload that accepts
a z/seed parameter, then use that returned value for HSV->RGB (keep using RGB,
hsvToRgb and t as before). Ensure you reference the same types used in the loop
(nrOfLightsType, lengthType) and preserve existing casts.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 111-114: TcpConnection::write must not call nanosleep on
EAGAIN/EWOULDBLOCK because sleeping on the render/hot path introduces jitter;
remove the nanosleep call (the block handling errno == EAGAIN || errno ==
EWOULDBLOCK) and instead return immediately with a distinct signal that the
socket would block (e.g., propagate -EAGAIN, 0, or a boolean/written length
convention used by your caller) so the caller can retry on the next tick or wait
for writability via the event/poll mechanism; keep the existing errno check and
ensure the function documents/propagates the would-block result so the render
loop can handle retries off the render path.
---
Outside diff comments:
In `@src/light/MappingLUT.h`:
- Around line 49-55: The setMapping function writes offsets_[logicalIdx] without
checking bounds; add a guard that validates logicalIdx < logicalCount_ (and that
logicalCount_ > 0) before touching offsets_ or destinations_, returning early if
the index is out of range to avoid corrupting the LUT; use the existing null
checks for offsets_ and destinations_ and apply the new bounds check at the
start of setMapping (function name: setMapping, symbols: offsets_,
destinations_, destinationCount_, destinationCapacity_, logicalCount_) and keep
the rest of the logic unchanged.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 101-107: TcpConnection::write currently loops on lwip_write when
it returns EAGAIN/EWOULDBLOCK and uses vTaskDelay(1) which can block
indefinitely; change this to a bounded retry/timeout strategy: detect
EAGAIN/EWOULDBLOCK from lwip_write(fd_) and either (a) poll the socket with
select/poll (or lwip equivalent) with a configurable short timeout before
retrying, or (b) maintain a retry counter and/or elapsed-time timeout and then
return an error/partial-write instead of spinning forever; ensure you update
callers to handle a short timeout/partial write, remove the unbounded
vTaskDelay(1) stall, and reference TcpConnection::write, lwip_write, fd_, errno
(EAGAIN/EWOULDBLOCK) and vTaskDelay in your changes.
In `@src/ui/app.js`:
- Around line 29-37: The WebSocket onmessage handler currently just replaces
state and calls updateValues(), which misses structural changes; modify the
ws.onmessage block to detect structural differences between the existing state
and incoming data (e.g., compare module lists/IDs/names or
Object.keys(state.modules) vs Object.keys(data.modules), or a shallow schema
JSON compare) and if any modules/controls were added/removed/renamed, set state
= data and call renderNav() and renderCards() (then return); otherwise set state
= data and call updateValues(); keep the existing ArrayBuffer handling with
renderPreviewFrame unchanged and use the same symbols ws.onmessage,
renderPreviewFrame, renderNav, renderCards, updateValues, and state to locate
the change.
In `@test/scenario_runner.cpp`:
- Around line 252-256: The current logic treats unknown module types as a benign
skip; instead, when ctx.createModule(type) returns nullptr (the mod variable),
fail the scenario immediately: emit a clear error to stderr including name and
type (use name and type variables), and terminate the run with a non-zero
failure (e.g. return a failure status from the scenario runner, call
std::exit(EXIT_FAILURE), or throw an exception) so the CI/test harness sees the
broken/stale scenario; update the block around ctx.createModule(type) / mod
accordingly to stop continuing and report failure.
- Around line 267-270: The branch handling strcmp(op, "set_control") currently
only prints and is a no-op, which lets control-dependent scenarios pass
incorrectly; either implement applying the control change or make the runner
fail when controls are requested. In scenario_runner.cpp where op and name are
handled, replace the comment with a call to your control-application function
(e.g., apply_control(name, value) or replay.applyControl(name, value) depending
on your API) and handle/report errors, or if no implementation exists, log a
clear error and return/abort the scenario (non-zero/false) so tests don't
silently succeed.
---
Duplicate comments:
In `@src/core/Control.h`:
- Around line 25-26: ControlList currently owns raw memory (controls_) and
defines a destructor (~ControlList) but has implicit copy/move operations,
causing double-free risk; fix by making ControlList non-copyable or implementing
the Rule of Five: either delete the copy constructor and copy assignment
(operator=(const ControlList&)) and/or provide a safe move constructor and move
assignment (ControlList(ControlList&&) and ControlList&
operator=(ControlList&&)) or replace raw controls_ with an owning
container/smart pointer (e.g., std::vector or std::unique_ptr<Control[]>).
Update the class declarations (ControlList, controls_, ~ControlList) accordingly
so ownership is unambiguous and destruction is safe.
In `@src/core/ModuleFactory.h`:
- Around line 19-23: Add null guards to prevent dereferencing null pointers: in
registerType(const char* typeName, CreateModuleFn fn, size_t classSize = 0)
check that typeName != nullptr and fn != nullptr (and fail/return false) before
accessing types_ or incrementing count_; similarly update the factory dispatch
code that iterates types_ (the code around lines 27-30 that calls std::strcmp
and invokes CreateModuleFn) to skip or treat entries with null typeName or null
fn and avoid calling fn when it's null, returning a safe failure (false/nullptr)
as appropriate; reference symbols: registerType, types_, count_, MAX_TYPES,
CreateModuleFn and the factory dispatch function that compares typeName via
std::strcmp and calls the stored CreateModuleFn.
In `@src/light/LayoutGroup.h`:
- Around line 19-20: The loops in LayoutGroup that iterate children and use
static_cast<LayoutBase*> (calling child(i)->lightCount()) assume every child is
a layout, which is unsafe; update both occurrences (the loop at the shown diff
and the one at lines 27-29) to first fetch the child into a local (e.g. auto *c
= child(i)), guard its role/type (e.g. if (!c || c->role() != Role::Layout)
continue) or use a safe cast (e.g. dynamic_cast/qt-cast and check for null) and
only then static_cast to LayoutBase* before calling lightCount(); skip
non-layout children.
In `@src/light/ModifierBase.h`:
- Around line 23-26: The mapToPhysical API exposes a raw pointer + capacity
(outPhysicals, outCount, maxOut) which breaks the guideline—change the
pure-virtual signature in ModifierBase (and all overrides) to accept a
std::span<nrOfLightsType> outPhysicals and nrOfLightsType& outCount (remove
maxOut), include <span>, and use outPhysicals.size() as the capacity; update
every implementation of mapToPhysical to write no more than outPhysicals.size()
elements and set outCount to the number written, and update all call sites to
pass a std::span (e.g., std::span{buf, len} or std::span(vec)) instead of a raw
pointer and maxOut.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 171-173: The accepted-socket is currently using SO_RCVTIMEO which
allows blocking reads and can stall the render loop; instead make the accepted
socket non-blocking: after accept(...) (where clientFd is created) remove the
setsockopt SO_RCVTIMEO usage and call fcntl(clientFd, F_SETFL, flags |
O_NONBLOCK) (retrieve existing flags with F_GETFL first) and handle
EAGAIN/EWOULDBLOCK in your read paths; also check and log errors from fcntl to
avoid silent failures.
In `@src/platform/platform.h`:
- Line 28: The declaration bool send(const char* ip, uint16_t port, const
uint8_t* data, size_t len); in platform.h / UdpSocket::send should be changed to
accept std::span<const uint8_t> for the payload (e.g., bool
UdpSocket::send(const char* ip, uint16_t port, std::span<const uint8_t> data));
update the corresponding implementation of UdpSocket::send to use the span's
data() and size(), add `#include` <span> where needed, and update all callers to
pass a std::span (or construct one from containers/buffers) instead of
pointer+length. Ensure signature changes are applied consistently in header and
implementation.
---
Nitpick comments:
In `@scripts/check/check_specs.py`:
- Around line 42-50: find_spec currently scans all SPECS by stem and can pick
wrong collisions; instead determine which entry in SOURCE_DIRS contains
module_path.parent (or the immediate source subdir for the module), map that
index to the corresponding spec subdirectory under SPECS, and only search that
constrained spec directory for md files matching module_path.stem; implement
this in find_spec by locating the matching source dir (using SOURCE_DIRS),
computing the relative module folder, building the candidate spec-directory path
under SPECS, searching that path for md.stem == module_path.stem, and fall back
to the previous global search only if no matching source-dir mapping is found.
In `@src/light/MappingLUT.h`:
- Line 49: Change the setMapping API to accept a std::span<const nrOfLightsType>
instead of (const nrOfLightsType* physicals, nrOfLightsType count): update the
declaration and implementation of MappingLUT::setMapping to take std::span<const
nrOfLightsType> mapping, include <span>, and use mapping[i] (or mapping.size())
where the code currently indexes the pointer/uses count; then update the call
site that passes physicals/count to call setMapping(std::span(physicals, count))
(e.g., the caller in Layer), preserving the nrOfLightsType element type and
constness.
In `@test/test_noise.cpp`:
- Around line 34-58: Add a new test that exercises NoiseEffect with a 3D layout
(set mm::GridLayout::depth > 1 and attach it to mm::LayoutGroup, create
mm::Layer, setChannelsPerLight, add mm::NoiseEffect, call
layer.onAllocateMemory() and layer.loop()) and then verify that values at
different z slices or 3D corner positions are not all identical (compare RGB at
index for e.g. first voxel and a voxel in a different z slice or opposite
corner) to catch index-math bugs in 3D traversal; reuse the existing test
structure (LayoutGroup, GridLayout, Layer, NoiseEffect, layer.buffer().data())
but with depth > 1 and appropriate index calculations for width/height/depth to
assert variation across the third dimension.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7d2f321-ecac-474c-932f-2e30bfcd0899
📒 Files selected for processing (78)
CLAUDE.mddocs/architecture-light.mddocs/architecture.mddocs/history/decisions.mddocs/history/memory-budget.mddocs/history/plan-05.mddocs/history/plan-06.mddocs/history/plan-07.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/light/MappingLUT.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules_draft/core/Control.mddocs/moonmodules_draft/core/HttpServerModule.mddocs/moonmodules_draft/core/MoonModule.mddocs/moonmodules_draft/core/Scheduler.mddocs/moonmodules_draft/light/BlendMap.mddocs/moonmodules_draft/light/Buffer.mddocs/moonmodules_draft/light/DriverGroup.mddocs/moonmodules_draft/light/EffectBase.mddocs/moonmodules_draft/light/Layer.mddocs/moonmodules_draft/light/LayoutGroup.mddocs/moonmodules_draft/light/LightConfig.mddocs/moonmodules_draft/light/MappingLUT.mddocs/moonmodules_draft/light/Noise.mddocs/moonmodules_draft/light/drivers/ArtNetSendDriver.mddocs/moonmodules_draft/light/drivers/PreviewDriver.mddocs/moonmodules_draft/light/effects/RainbowEffect.mddocs/moonmodules_draft/light/layouts/GridLayout.mddocs/performance.mddocs/plan.mddocs/testing.mdscripts/MoonDeck.mdscripts/check/check_specs.pyscripts/check/collect_kpi.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/index.htmlscripts/moondeck_ui/style.cssscripts/scenario/run_live_scenario.pysrc/core/Control.hsrc/core/HttpServerModule.hsrc/core/ModuleFactory.hsrc/core/MoonModule.hsrc/core/PreviewFrame.hsrc/core/Scheduler.hsrc/core/types.hsrc/light/BlendMap.hsrc/light/DriverGroup.hsrc/light/EffectBase.hsrc/light/GridLayout.hsrc/light/Layer.hsrc/light/LayoutGroup.hsrc/light/MappingLUT.hsrc/light/MirrorModifier.hsrc/light/ModifierBase.hsrc/light/NoiseEffect.hsrc/light/PreviewDriver.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/index.htmlsrc/ui/style.csstest/scenario_runner.cpptest/scenarios/base-pipeline.jsontest/scenarios/control-change.jsontest/scenarios/memory-1to1.jsontest/scenarios/memory-lut.jsontest/scenarios/mirror.jsontest/test_blend_map.cpptest/test_grid_layout.cpptest/test_mapping_lut.cpptest/test_moonmodule.cpptest/test_noise.cpptest/test_rainbow.cpp
💤 Files with no reviewable changes (17)
- docs/moonmodules_draft/light/Layer.md
- docs/moonmodules_draft/core/Scheduler.md
- docs/moonmodules_draft/light/LightConfig.md
- docs/moonmodules_draft/light/drivers/ArtNetSendDriver.md
- docs/moonmodules_draft/light/MappingLUT.md
- docs/moonmodules_draft/light/effects/RainbowEffect.md
- docs/moonmodules_draft/core/Control.md
- docs/moonmodules_draft/light/Buffer.md
- docs/moonmodules_draft/light/Noise.md
- docs/moonmodules_draft/light/drivers/PreviewDriver.md
- docs/moonmodules_draft/light/layouts/GridLayout.md
- docs/moonmodules_draft/core/HttpServerModule.md
- docs/moonmodules_draft/light/DriverGroup.md
- docs/moonmodules_draft/light/BlendMap.md
- docs/moonmodules_draft/core/MoonModule.md
- docs/moonmodules_draft/light/LayoutGroup.md
- docs/moonmodules_draft/light/EffectBase.md
✅ Files skipped from review due to trivial changes (9)
- src/ui/index.html
- docs/moonmodules/light/drivers/PreviewDriver.md
- docs/moonmodules/light/MappingLUT.md
- src/ui/style.css
- scripts/MoonDeck.md
- docs/history/memory-budget.md
- docs/architecture.md
- docs/history/decisions.md
- docs/moonmodules/core/HttpServerModule.md
Comment on lines
+81
to
+96
| # Scan .1 to .254 on both port 80 (ESP32) and 8080 (desktop) | ||
| for i in range(1, 255): | ||
| ip = f"{subnet}.{i}" | ||
| for port in [80, 8080]: | ||
| t = threading.Thread(target=probe, args=(ip, port), daemon=True) | ||
| threads.append(t) | ||
| t.start() | ||
|
|
||
| # Also check localhost:8080 (desktop dev) | ||
| t = threading.Thread(target=lambda: probe("localhost", 8080), daemon=True) | ||
| threads.append(t) | ||
| t.start() | ||
|
|
||
| for t in threads: | ||
| t.join(timeout=3.0) | ||
|
|
There was a problem hiding this comment.
Cap discovery concurrency.
This launches 509 threads per scan (254 hosts * 2 ports + localhost). On slower machines that can saturate threads/sockets and make MoonDeck unresponsive for a best-effort feature. Use a bounded worker pool or queue instead of one thread per probe.
🤖 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 `@scripts/moondeck.py` around lines 81 - 96, The current scan spawns one thread
per probe (probe function) which creates ~509 threads; replace this with a
bounded worker pool (e.g., concurrent.futures.ThreadPoolExecutor or a Queue +
fixed worker threads) so you submit probe(ip, port) tasks instead of creating a
Thread per probe; use a fixed max_workers (e.g., 20-50) to limit concurrency,
submit tasks for the subnet ports and localhost:8080, collect futures and use
future.result(timeout=3.0) or as_completed with timeouts to enforce the
per-probe join behavior and time limits; update the loop that builds threads and
the final join block to work with the executor/futures and ensure daemon
behavior/clean shutdown.
Comment on lines
+107
to
+111
| if op == "add_module": | ||
| print(f" WARN {step_name}: {msg}") | ||
| else: | ||
| print(f" FAIL {step_name}: {msg}") | ||
| results["passed"] = False |
There was a problem hiding this comment.
Fail scenario on add_module HTTP errors.
Line 107 and Line 115 currently downgrade add_module exceptions to warnings, so a broken module-creation step can still end as PASSED. That makes live scenario results unreliable.
Proposed fix
except urllib.error.HTTPError as e:
@@
step_result["status"] = "error"
step_result["error"] = msg
- if op == "add_module":
- print(f" WARN {step_name}: {msg}")
- else:
- print(f" FAIL {step_name}: {msg}")
- results["passed"] = False
+ print(f" FAIL {step_name}: {msg}")
+ results["passed"] = False
except Exception as e:
@@
step_result["status"] = "error"
step_result["error"] = str(e)
- if op == "add_module":
- print(f" WARN {step_name}: {e}")
- else:
- print(f" FAIL {step_name}: {e}")
- results["passed"] = False
+ print(f" FAIL {step_name}: {e}")
+ results["passed"] = FalseAlso applies to: 115-119
🤖 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 `@scripts/scenario/run_live_scenario.py` around lines 107 - 111, The code
currently treats op == "add_module" as a warning and does not set
results["passed"] = False, allowing module-creation failures to be treated as
passed; update both places that print warnings (the blocks using op, step_name,
msg, and results) so that when op == "add_module" you print the failure message
the same as other ops and set results["passed"] = False (i.e., remove the
special-case that downgrades "add_module" to a warning and ensure
results["passed"] is set to False on any non-successful op).
Comment on lines
+242
to
+243
| int len = buildStateJson(jsonBuf_, JSON_BUF_SIZE); | ||
| sendResponse(conn, 200, "application/json", jsonBuf_); |
There was a problem hiding this comment.
Detect JSON truncation before replying.
buildStateJson() stops appending when jsonBuf_ fills, but serveState() still returns whatever partial buffer is left as 200 OK. Once the module tree grows past 4 KB, /api/state degrades into malformed JSON instead of a clear error path.
Also applies to: 247-268
🤖 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.h` around lines 242 - 243, buildStateJson may
truncate when jsonBuf_ fills but the code always replies 200 with the partial
buffer; change serveState (and the other call sites that call buildStateJson
around the 247-268 range) to detect truncation by checking the returned int from
buildStateJson and treat a return indicating the buffer limit (e.g., len >=
JSON_BUF_SIZE or other sentinel returned by buildStateJson) as an error: do not
call sendResponse with a truncated jsonBuf_, instead send a proper error
response (500 or 413) and/or a small valid error JSON body; update both the
initial call at the shown diff and the other calls in that block to follow the
same check and error path.
Comment on lines
+173
to
+188
| if (canAllocate(needed)) { | ||
| buffer_.allocate(count, channelsPerLight_); | ||
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); | ||
| return; | ||
| } | ||
| // Halve: reduce to sqrt of count (halve each dimension) | ||
| width_ = width_ > 1 ? width_ / 2 : 1; | ||
| height_ = height_ > 1 ? height_ / 2 : 1; | ||
| depth_ = depth_ > 1 ? depth_ / 2 : 1; | ||
| count = static_cast<nrOfLightsType>(width_) * height_ * depth_; | ||
| std::printf(" DEGRADE buffer too large, reducing to %dx%dx%d\n", | ||
| static_cast<int>(width_), static_cast<int>(height_), static_cast<int>(depth_)); | ||
| if (width_ <= 8 && height_ <= 8) break; // minimum | ||
| } | ||
| buffer_.allocate(count, channelsPerLight_); | ||
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); |
There was a problem hiding this comment.
Handle Buffer::allocate failure explicitly.
canAllocate() is only a prediction; if allocation fails anyway, the module continues with invalid runtime assumptions and still publishes dynamic-bytes as if allocation succeeded.
Suggested fix
if (canAllocate(needed)) {
- buffer_.allocate(count, channelsPerLight_);
+ if (!buffer_.allocate(count, channelsPerLight_)) {
+ width_ = height_ = depth_ = 0;
+ setDynamicBytes(lut_.memoryUsed());
+ return;
+ }
setDynamicBytes(buffer_.bytes() + lut_.memoryUsed());
return;
}
@@
- buffer_.allocate(count, channelsPerLight_);
+ if (!buffer_.allocate(count, channelsPerLight_)) {
+ width_ = height_ = depth_ = 0;
+ setDynamicBytes(lut_.memoryUsed());
+ return;
+ }
setDynamicBytes(buffer_.bytes() + lut_.memoryUsed());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (canAllocate(needed)) { | |
| buffer_.allocate(count, channelsPerLight_); | |
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); | |
| return; | |
| } | |
| // Halve: reduce to sqrt of count (halve each dimension) | |
| width_ = width_ > 1 ? width_ / 2 : 1; | |
| height_ = height_ > 1 ? height_ / 2 : 1; | |
| depth_ = depth_ > 1 ? depth_ / 2 : 1; | |
| count = static_cast<nrOfLightsType>(width_) * height_ * depth_; | |
| std::printf(" DEGRADE buffer too large, reducing to %dx%dx%d\n", | |
| static_cast<int>(width_), static_cast<int>(height_), static_cast<int>(depth_)); | |
| if (width_ <= 8 && height_ <= 8) break; // minimum | |
| } | |
| buffer_.allocate(count, channelsPerLight_); | |
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); | |
| if (canAllocate(needed)) { | |
| if (!buffer_.allocate(count, channelsPerLight_)) { | |
| width_ = height_ = depth_ = 0; | |
| setDynamicBytes(lut_.memoryUsed()); | |
| return; | |
| } | |
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); | |
| return; | |
| } | |
| // Halve: reduce to sqrt of count (halve each dimension) | |
| width_ = width_ > 1 ? width_ / 2 : 1; | |
| height_ = height_ > 1 ? height_ / 2 : 1; | |
| depth_ = depth_ > 1 ? depth_ / 2 : 1; | |
| count = static_cast<nrOfLightsType>(width_) * height_ * depth_; | |
| std::printf(" DEGRADE buffer too large, reducing to %dx%dx%d\n", | |
| static_cast<int>(width_), static_cast<int>(height_), static_cast<int>(depth_)); | |
| if (width_ <= 8 && height_ <= 8) break; // minimum | |
| } | |
| if (!buffer_.allocate(count, channelsPerLight_)) { | |
| width_ = height_ = depth_ = 0; | |
| setDynamicBytes(lut_.memoryUsed()); | |
| return; | |
| } | |
| setDynamicBytes(buffer_.bytes() + lut_.memoryUsed()); |
🤖 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/Layer.h` around lines 173 - 188, The loop uses canAllocate(...) as
a prediction but then calls buffer_.allocate(...) without checking for failure;
update the Layer allocation path (around buffer_.allocate, canAllocate,
setDynamicBytes, and lut_.memoryUsed) to detect allocation failure after
buffer_.allocate returns (or by checking a post-condition like
buffer_.isAllocated() or buffer_.bytes()>0), and if allocation fails continue
degrading (retry with smaller width_/height_/depth_), and only call
setDynamicBytes(...) when allocation actually succeeded; if you reach the
minimum and allocation still fails, log an error and set dynamic bytes to zero
(or abort cleanly) instead of assuming allocation succeeded.
Comment on lines
+31
to
+37
| for (nrOfLightsType i = 0; i < count; i++) { | ||
| nrOfLightsType rem = i % wh; | ||
| lengthType x = static_cast<lengthType>(rem % w); | ||
| lengthType y = static_cast<lengthType>(rem / w); | ||
|
|
||
| uint8_t n = noise2d(x, y, t); | ||
| RGB c = hsvToRgb(n, 200, 255); |
There was a problem hiding this comment.
Depth dimension is still ignored during noise sampling.
Line 31–37 computes x/y from i, but z is never used, so every depth slice gets identical noise for depth > 1. This breaks expected 3D behavior.
Proposed fix
- for (nrOfLightsType i = 0; i < count; i++) {
- nrOfLightsType rem = i % wh;
- lengthType x = static_cast<lengthType>(rem % w);
- lengthType y = static_cast<lengthType>(rem / w);
-
- uint8_t n = noise2d(x, y, t);
+ for (nrOfLightsType i = 0; i < count; i++) {
+ nrOfLightsType z = wh > 0 ? (i / wh) : 0;
+ nrOfLightsType rem = wh > 0 ? (i % wh) : 0;
+ lengthType x = static_cast<lengthType>(rem % w);
+ lengthType y = static_cast<lengthType>(rem / w);
+
+ uint8_t n = noise2d(x, y, t + static_cast<uint32_t>(z) * 31u);Based on learnings: Hot path correctness and 3D pipeline behavior are required, and this path currently collapses depth into repeated 2D output.
🤖 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/NoiseEffect.h` around lines 31 - 37, The loop that computes x and y
from i ignores the depth slice so every z gets identical noise; compute a z
coordinate from i (e.g. lengthType z = static_cast<lengthType>((i / wh) % depth)
or lengthType z = static_cast<lengthType>(i / wh) when depth == count/wh) and
pass it into the noise sampler instead of calling noise2d(x, y, t) — either call
the 3D noise function (noise3d(x, y, z, t)) or the appropriate noise2d overload
that accepts a z/seed parameter, then use that returned value for HSV->RGB (keep
using RGB, hsvToRgb and t as before). Ensure you reference the same types used
in the loop (nrOfLightsType, lengthType) and preserve existing casts.
Comment on lines
+111
to
+114
| } else if (n < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { | ||
| struct timespec ts = {0, 1000000}; // 1ms | ||
| nanosleep(&ts, nullptr); | ||
| } else { |
There was a problem hiding this comment.
nanosleep retry in TcpConnection::write introduces avoidable blocking.
Line 112–113 sleeps on EAGAIN/EWOULDBLOCK; this can accumulate frame jitter under backpressure. Prefer returning and retrying next tick (or poll-based writable wait off the render path).
Based on learnings: Hot path (render loop) must avoid blocking calls (sleep/delay).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/desktop/platform_desktop.cpp` around lines 111 - 114,
TcpConnection::write must not call nanosleep on EAGAIN/EWOULDBLOCK because
sleeping on the render/hot path introduces jitter; remove the nanosleep call
(the block handling errno == EAGAIN || errno == EWOULDBLOCK) and instead return
immediately with a distinct signal that the socket would block (e.g., propagate
-EAGAIN, 0, or a boolean/written length convention used by your caller) so the
caller can retry on the next tick or wait for writability via the event/poll
mechanism; keep the existing errno check and ensure the function
documents/propagates the would-block result so the render loop can handle
retries off the render path.
- MoonModule/ControlList: delete copy/move (Rule of Five, owns raw pointers) - MoonModule::removeChild: clear child's parent pointer - Layer::allocateBuffer: check buffer_.allocate() return value - MappingLUT::setMapping: bounds-check logicalIdx - ModuleFactory: null-check typeName and fn parameters - Docs: fix markdown fences and table column mismatch - Add 10 lessons to decisions.md from this branch - Archive plan-01 through plan-07 to docs/history/archive/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- All modules heap-allocated via factory or new (no more stack allocation) - MoonModule::operator new/delete uses platform::alloc for PSRAM on ESP32 - Scheduler::teardown deletes module tree recursively - HTTP DELETE works on any module (no stack vs heap distinction) - NoiseEffect: BPM control (not speed), resolution-independent animation, phase accumulation (no jump on BPM change), smooth coordinate scrolling - BPM animation guidelines promoted to EffectBase.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ewowi
changed the title
This was referenced May 20, 2026
ewowi
added a commit
that referenced
this pull request
Jun 3, 2026
Moondeck.json restructures from a flat device list + single port into named
networks, each holding its own devices, last-used serial port, and WiFi
credentials. Header network bar auto-selects from host's current subnet on
load; manual override pins. Improv WiFi reads the active network's creds —
wifi_credentials.json retired. Existing moondeck.json migrates in place.
KPI: n/a (no functional src/ changes — only comments in scenario_runner.cpp)
Tests
- scenario_runner.cpp: KEEP IN SYNC comment pointing at run_live_scenario.py tolerance defaults.
- unit_ArtNetSendDriver_no_alloc_in_loop.cpp: present-tense ("would need" → "needs", "grow happens" → "grow runs"). [CodeRabbit]
- unit_FilesystemModule_persistence.cpp: present-tense ("would have caught" → "catches"). [CodeRabbit]
- scenario_PreviewDriver_detail.json: present-tense descriptions ("must still hit" → "still hits", "must not affect" → "does not affect"). [CodeRabbit]
- All scenario JSONs: routine observed-range widenings from gate sweeps.
Scripts / MoonDeck
- moondeck.py: load_state() detects legacy flat shape and runs _migrate_to_networks (buckets by /24 subnet, largest bucket "Home"); save_state() strips volatile per-device fields including conditionally board when it equals the deduced value; new _active_network / _auto_select_network / _deduce_board helpers; /api/state GET auto-selects on host subnet, /api/discover attributes found devices to matching network, /api/refresh refreshes single network's devices; flash-event breadcrumb still links last_port. NAMING COLLISION breadcrumb added to _deduce_board docstring pointing at docs/plan.md. KEEP IN SYNC note between _deduce_board and the JS board picker list. contextlib.suppress for three inline try/except sites. [CodeRabbit #5]
- moondeck_ui/app.js: getActiveNetwork() helper; renderNetworkBar() + setupNetworkBar() build the dropdown, Rename/Add buttons, WiFi panel; applyNetworkBarVisibility() hides the bar on PC tab; every state.devices / state.port site re-routed through the active network; cross-ref comment at the hardware-board picker pointing at moondeck.py::_deduce_board.
- moondeck_ui/index.html: network-bar div above the per-tab content in the sidebar.
- moondeck_ui/style.css: .network-bar + .network-wifi styling; .device-board picker layout fix.
- build/host_wifi.py: primary credentials source is the active network's wifi block in moondeck.json; OS auto-detect fallback stays.
- check/collect_kpi.py: 18 FPS banner → derived from MIN_ESP32_FPS_LED_PRODUCT (currently 10). [CodeRabbit #3]
- scenario/run_live_scenario.py: max_alloc_block 0 fails when contract demands >0 (was silently passing). [CodeRabbit #6]; KEEP IN SYNC comment about tolerance defaults mirroring scenario_runner.cpp.
- scenario/_observed.py: shared widen-only range update for observed.<target> blocks (factored from both runners).
- docs/screenshot_modules.py: --extras-only flag + _ExtrasOnlyDone sentinel; lets MoonDeck UI re-shoots run without projectMM.
- build/flash_esp32.py: writes scripts/.last_flash.json breadcrumb on success; MoonDeck consumes it to attribute last_port.
Deleted
- scripts/build/wifi_credentials.example.json — the live source moved into moondeck.json's network records.
- .gitignore entry for scripts/build/wifi_credentials.json.
Docs
- testing.md: new Live tab screenshot; observed-range model wording.
- building.md: MoonDeck PC tab + ESP32 tab screenshots placed in their sections.
- MoonDeck.md: Network bar UI Features bullet; improv_provision section updated for the new active-network credentials flow.
- docs/plan.md: "Board vs firmware separation, runtime board presets (multi-commit, started)" roadmap.
- docs/tests/unit-tests.md + scenario-tests.md: regenerated (esp32 row now included in PreviewDriver tables from stale-doc fix). [CodeRabbit #1]
- assets/screenshots/moondeck_{pc,esp32,live}.png: re-captured with the network bar.
Reviews
- 🐰 CodeRabbit: 6 fixed (#1 esp32 row from stale doc → regen; #2 unit-test tense; #3 18 FPS → 10 FPS; #5 contextlib.suppress; #6 max_alloc_block 0 fails; #7 scenario descriptions present-tense). 1 skipped: #4 (show tolerated contract values in perf table) — conflates contract (promise) with tolerance (measurement noise); reviewer agent agreed.
- 👾 Reviewer (Opus, branch diff): ship with changes; 3 architectural calls accepted (sync comments for tolerance defaults, board naming-collision breadcrumb, hardware-board catalog cross-ref) + 1 minor refactor (FPS helper docstring extended to name shared core).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 4, 2026
Adds the LOLIN S3 N16R8 board entry to the catalog and the full chain
of fixes it needed: a writable WiFi-TX-power cap to prevent the
on-module LDO brown-out, USB-Serial-JTAG support in the Improv
listener so the device responds on native-USB S3 boards, a CORS
preflight handler so cross-origin POSTs from the web installer
actually reach `/api/control`, and a generic per-board controls
fan-out across MoonDeck and the installer paths so future per-board
fields land without code changes. Web installer also gains a
Monitor button + retry affordance, and `maxBlock` now reports
internal-RAM block (useful) instead of PSRAM block (always ~8MB).
KPI: tick:153831us(FPS:6) on esp32s3-n16r8 with txPower capped to
8 dBm (LOLIN brown-out fix). Slower than full-power ArtNet because
8 dBm cuts radio TX margin — known hardware trade-off, not a code
regression.
Core:
- NetworkModule: added writable `txPowerSetting` (0..21 dBm, 0=no
override) re-applied on change via syncTxPower(). Setting back to 0
actively pushes 80 quarter-dBm to undo any prior cap (no IDF
"reset to default" API exists; sticky-cap was the previous behavior).
- BoardModule: ASCII-printable validation rationale documented at
setBoard() (JSON encoding, UI rendering, C-string round-trip).
- HttpServerModule: added OPTIONS preflight handler returning 204 +
CORS headers. Root cause of every "cross-origin POST silently
blocked" symptom — the orchestrator's HTTP inject from preview now
lands properly. Also documented path-agnostic preflight choice.
- SystemModule: maxBlock display switched to maxInternalAllocBlock;
added comment explaining how PSRAM is detected (heap-size derivation,
not a flag).
Platform:
- platform.h / platform_esp32.cpp / platform_desktop.cpp: new
`maxInternalAllocBlock()` API alongside existing `maxAllocBlock`.
Internal-only block is the memory-pressure KPI; the all-memory
variant reports ~8 MB on PSRAM boards and tells you nothing.
Layer::canAllocate keeps the all-memory call (light buffers may
live in PSRAM); every diagnostic site (SystemModule, main.cpp tick
log, HttpServerModule /api/state, scenario_runner) now uses the
internal-only variant.
- platform.h / platform_esp32.cpp / platform_desktop.cpp: new
`wifiSetTxPower(int8_t quarterDbm)` setter wrapping
esp_wifi_set_max_tx_power. Clamps into ESP-IDF's 8..84 range.
- platform_esp32_improv.cpp: USB-Serial-JTAG read+write path added
alongside UART0 (guarded by SOC_USB_SERIAL_JTAG_SUPPORTED). LOLIN
S3 N16R8 and other native-USB S3 boards expose USB-C through
USB-Serial-JTAG, not UART0 — without this the Improv task was deaf
to host writes on those boards. Both transports drained
symmetrically each loop with a single 10 ms yield when both are
empty; ESP_LOGW for driver-install warnings + compound
`"listening (uart unavailable)"` status so partial-transport
failures stay visible.
UI:
- src/ui/install-picker.js: added `installRowExtras` slot so the
installer page can slot the Erase-chip checkbox between firmware
dropdown and Install button without the on-device OTA UI inheriting
installer-specific affordances.
- src/ui/app.js: sendControl() now logs non-ok HTTP + network errors
to console.warn (no retry — design intent for the single-shot
`?board=` consume path).
Scripts / MoonDeck:
- MoonDeck `_push_board_to_device` now fans out the full
`controls.<Module>.<control>` block from boards.json on every push
(was Board.board only). Falls back to bare-board for catalog-
missing names; no-op on empty board. Generic — any future per-board
control in boards.json lands via MoonDeck without code changes.
- docs/install/boards.json: new "LOLIN S3 N16R8" entry injecting
`Network.txPowerSetting: 8` (8 dBm — well below the ~13 dBm
threshold ESPHome documents for the brown-out symptom).
- docs/install/index.html: rich installer-UI changes — Monitor
button + serial-monitor modal (Reset/Clear/Close, autoscroll,
UTF-8 decode, RTS-pulse reset), Retry button on the "Improv not
detected" dialog with stale-port probe + 250 ms post-close wait
for SDK lock release, lazy-render WiFi-creds form (avoids macOS
iCloud Passwords prompt on page load), Skip-creds path exits
cleanly via onSuccess({url:""}) rather than failing in provision(),
HTTP injection runs on the Improv-success path too (was needsIp-
only — fixes the "txPower not applied after Improv install"
symptom), 2 s → 3 s post-flash port-reopen wait (boot race against
Improv task init on S3), monitor button auto-disabled during
install (Web Serial port-mutex enforcement), pendingBoard fallback
now covers Improv-success HTTP fail.
- docs/install/install-orchestrator.js: stale-prePickedPort probe
with fallback to requestPort() in both start() and eraseOnly();
isTransientImprovError helper extracted next to isImprovNotDetected
so SDK error-string drift has one update site; uiShowNeedsIpRetrying
contract reconciled with actual behavior.
- docs/install/devices.js: pendingBoard explicitly cleared on
successful inject (was leaking across re-installs); erase confirm
text rewritten to reflect the orchestrator-driven flow (EWT
branding removed).
Docs / CI:
- docs/install/README.md, scripts/MoonDeck.md: replaced "ESP Web
Tools installer page" branding with the orchestrator-driven
description (present tense, no backward-looking change-log talk).
- docs/moonmodules/core/BoardModule.md: MoonDeck section updated to
reflect full controls fan-out; "Pre-WiFi limitation" paragraph
trimmed to a one-line pointer; full timing-constraint rationale
moved to docs/history/decisions.md as the lesson worth carrying
forward.
- docs/moonmodules/core/NetworkModule.md: txPowerSetting bullet
added (trimmed to wire contract + intended use).
- docs/history/decisions.md: new "Board-injection pipeline timing
constraint" entry — names the controls that wouldn't tolerate the
HTTP-after-WiFi fan-out (country code, antenna selector, pre-
association TX-power) and the two escape hatches (new vendor RPC,
sdkconfig bake). Warns against extending SET_BOARD's wire format.
Tests:
- test/scenario_runner.cpp: switched to maxInternalAllocBlock for
the regression KPI; updated rationale to explain why (PSRAM
boards previously masked internal-heap fragmentation).
- test/unit/core/unit_BoardModule.cpp: added c.readonly assertion
on the board control — regression guard against accidentally
making the BoardModule.board field user-editable.
Reviews:
- 👾 Reviewer (on-demand pre-commit): 0 blockers, 10 should-fix, 12
nits — all 22 addressed in this commit. Highlights: #1
syncTxPower=0 now actively lifts the cap (was sticky until reboot);
#2 pendingBoard fallback covers the Improv-success HTTP-fail path
the LOLIN feature most needs to defend; #4 Monitor button now
actually disabled during install (matched the documented mutex);
#5 Improv driver-install warnings now logged via ESP_LOGW and
carried in compound status (were silently overwritten by
"listening"); #7 JTAG TX timeout 50 ms → 0 (truly non-blocking);
#8 UART/JTAG polling made symmetric. The remaining items are
comment trims, sentinel-vs-tagged-union docstring clarifications,
and the 32-vs-34-quarter-dBm example fix in platform.h.
- 🐇 CodeRabbit (prior round): 10 findings processed earlier in the
branch; 2 rejected with reason (HttpServerModule single-line `if
(c.readonly)` brace fix — no clang-tidy config in the repo, and
the codebase has many single-line ifs; consumePendingBoardParam
retry-token — contradicts the documented "no retry" single-shot
design).
KPI Details:
Desktop:
Lights: 16,384
Binary: 375 KB
[doctest] test cases: 207 | 207 passed | 0 failed | 0 skipped
tick: 352us, 99us, 304us, 100us, 99us, 100us, 45us, 35us, 35us
(FPS: 2840, 10101, 3289, 10000, 10101, 10000, 22222, 28571, 28571)
=== 10 scenario(s), 10 passed, 0 failed ===
Platform boundary: PASS
Specs: 26 modules, 26 ok, 0 missing, 0 outdated
ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm):
Image: 1,307,517 bytes (69% partition free)
Flash (code+data): 1171 KB
tick: 153831us (FPS: 6)
free heap: 8354995 maxBlock (internal): 163840 (160 KB)
ArtNetSend: ~105 ms/tick (LOLIN at 8 dBm — known trade-off)
Code:
62 source files (10491 lines)
38 test files (5410 lines)
39 specs, 10 scenarios
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 4, 2026
Four CodeRabbit findings folded in (three with leaner shapes than proposed), one deferred with reason. Most user-visible: syncTxPower() now runs immediately after every successful wifiStaInit / wifiApInit, so the LOLIN brown-out cap lands BEFORE the first association burst rather than up to 1 s after — closes the window the cap exists to defend. Also: Improv replies now go only on the transport that asked, the needs-ip dialog catches whitespace-only input via the browser's own validation tooltip, and the BoardModule.h ASCII-validation asymmetry between SET_BOARD-over-Improv (validates) and HTTP /api/control (doesn't) is documented explicitly. Plus an unrelated docs addition: the NanoDLA v1.3 logic analyzer is named as the project's reference fx2lafw device with wiring + capture instructions for measuring ESP32 WS2812 output. KPI: tick:138506us(FPS:7) on esp32s3-n16r8 at txPower=8 dBm. Within noise of last commit's 141ms/FPS:7; the syncTxPower-on-init change doesn't measurably affect the tick. Core: - NetworkModule: syncTxPower() called immediately after each of the 5 wifiStaInit / wifiApInit success sites (setWifiCredentials, setup, the two ethernet-cascade-to-WiFi paths in loop1s, and startAP). The comment at setWifiCredentials's call is the canonical one; the rest point back to it. Closes the up-to-1 s gap between WiFi-up and the next loop1s tick during which the radio was at full power. - BoardModule: documented the HTTP-write-bypasses-ASCII-validation asymmetry on setBoard()'s comment. The HTTP path uses the generic Text-control write at applyControlValue (Control.cpp) which does no printable-ASCII check. Acceptable today because HTTP-write callers (MoonDeck, web installer's HTTP inject) source values from boards.json (project-controlled). If the threat model grows, the right fix is a per-control validator hook on ControlDescriptor — named in the comment for the next reader. Platform: - platform_esp32_improv.cpp: per-transport reply routing. New static `g_replySource` set at the top of improvFeedByte's FrameReady branch and read inside improvSend; replies go ONLY on the transport that received the request rather than broadcast to both. The Improv task is single-threaded so a single static is sufficient; resets to ImprovSource::Both after dispatch so any unsolicited send (status broadcasts) keeps the prior shape. Lighter than threading an enum through every signature — same correctness outcome. UI: - docs/install/index.html: uiWaitForIp's submit handler now rejects whitespace-only input via setCustomValidity + reportValidity. Surfaces the browser's existing field-validation tooltip on the same field rather than letting normalizeDeviceUrl() return empty and the orchestrator silently re-prompt. The `required` attribute catches empty submits but not " " — trim first, then flag. Docs / CI: - docs/moonmodules/light/leddriver-analysis-top-down.md: named the NanoDLA v1.3 as the project's reference fx2lafw-class logic analyzer (replacing the generic "$10-15 fx2lafw clone" mention in the TL;DR and recommended-suite table) and added a new §5.3.1 "Wiring the NanoDLA v1.3 to an ESP32 and capturing one frame" — common-ground / signal-tap / power-isolation / strand-power wiring, voltage-level reasoning (3.3→3.3 direct, no shifter needed), sample-rate math for typical strands, copy-pasteable sigrok-cli capture + decode sequence, and common gotchas in the order they'll appear. Tests: - test/scenarios/light/scenario_PreviewDriver_detail.json: drift- tracking observed[pc-macos].tick_us[0] updated from 297→296. Normal CI noise persisted by scripts/scenario/run_scenario.py per the docs/testing.md drift-visibility contract. Reviews: - 🐇 CodeRabbit round 2: 4 findings; 3 fixed, 1 deferred. (a) #1 empty-IP validation: chose host-side setCustomValidity instead of proposed onValidationError callback (smaller, equivalent UX, no new API surface). (b) Outside #improv-routing: chose static- g_replySource instead of proposed signature-threading (same correctness, ~30 fewer touched call sites). (c) #4 syncTxPower- after-wifi-init: implemented as-proposed. (d) #2 unified validation between HTTP and Improv board-name paths: deferred — both clean fixes are architectural (per-control validator hook on ControlDescriptor, or per-module HTTP dispatch exception); the asymmetry is documented with the right fix-shape named, to be picked up when the threat model warrants it. KPI Details: Desktop: Lights: 16,384 Binary: 375 KB [doctest] test cases: 207 | 207 passed | 0 failed | 0 skipped tick: 354us, 100us, 303us, 99us, 99us, 98us, 45us, 35us, 34us (FPS: 2824, 10000, 3300, 10101, 10101, 10204, 22222, 28571, 29411) === 10 scenario(s), 10 passed, 0 failed === Platform boundary: PASS Specs: 26 modules, 26 ok, 0 missing, 0 outdated ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm): Image: 1,307,661 bytes (69% partition free) Flash (code+data): 1171 KB tick: 138506us (FPS: 7) free heap: 8358927 maxBlock (internal): 163840 (160 KB) ArtNetSend: ~93 ms/tick (LOLIN at 8 dBm — known trade-off) Code: 62 source files (10592 lines) 38 test files (5410 lines) 39 specs, 10 scenarios Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 4, 2026
PR-merge review pass turned up 12 items; 5 fixed pre-merge per the Reviewer's "quick wins worth landing before merge" recommendation, 2 deferred to docs/plan.md as concrete follow-ups, 5 deferred as nits. Plus the gate-3 (carry-forward lessons) and gate-7 (live perf snapshot to docs/performance.md) outputs that the PR-merge checklist mandates whenever the branch touches tick-path code. KPI: tick:138506us(FPS:7) on esp32s3-n16r8 — unchanged from commit 6212fc0 (this round's only src/ change is a desktop stub return- value tweak, no ESP32 behavioral delta). Core: - platform_desktop.cpp: wifiSetTxPower(0) now returns true to match the documented "0 = no-override = success" contract — same shape the MM_NO_WIFI ESP32 stub already uses. Non-zero still returns false (no radio to set). Without this, NetworkModule's syncTxPower on desktop never updated appliedTxPowerSetting_ and retried on every loop1s tick — harmless but inconsistent. Docs / CI: - docs/install/index.html: user-visible browser-warning text "ESP Web Tools needs Web Serial" → "Web Serial is required". EWT is gone; the warning lives on past it. - docs/install/README.md: same string at the bottom of the local- preview docs. - docs/moonmodules/core/BoardModule.md: rewrote the past-tense "This commit replaced ESP Web Tools' install button..." paragraph as a present-tense statement about what the orchestrator does today. Per § Principles, module spec pages describe the system as-is; changelog-style narrative lives in history. - docs/moonmodules/core/NetworkModule.md: "Board-specific GPIO config ... will be handled by a future IO module" → present-tense ("is hardcoded per board via compile-time defines in sdkconfig.defaults.<board>"). Forward-looking promises belong in plan.md. - docs/history/decisions.md: three new "Lessons from the LOLIN S3 N16R8 enablement branch" entries — USB-Serial-JTAG vs UART0 on S3 (the dual-stdio mirror that made the wrong inference cheap), CORS preflight silent-failure (the root cause behind every "inject silently fails" symptom), and cache-staleness on hardware-event invalidation (the appliedTxPowerSetting_ pattern). Plus the ESP-Web-Tools replacement rationale moved out of BoardModule.md. - docs/plan.md: two new open-follow-up entries under § Board injection — per-control validator hook on ControlDescriptor (the right fix for the HTTP-vs-Improv board-name validation asymmetry), and shared JS helpers across device-UI and web-installer (defer until a second cross-context helper earns the 5-file build-glue cost). - docs/performance.md: new "ESP32-S3 — LOLIN S3 N16R8" section covering tick budget at txPower=8 dBm (~138 ms / FPS 7), the ArtNet-slow-at-low-TX explanation, memory layout (Layer in PSRAM, maxBlock as internal-RAM-only KPI), and the use-case framing (LOLIN fits "lots of PSRAM, accept WiFi compromise"; Ethernet boards win on FPS). Reviews: - 👾 PR-merge Reviewer (whole branch diff, 7 commits): 0 blockers, 7 should-fix, 5 nits. (a) #1-3 (stale EWT copy, past-tense spec, forward-looking spec) → fixed above. (b) #10 (desktop stub contract) → fixed above. (c) #4 (validation hook) + #6 (shared helpers) → documented as follow-ups in plan.md. (d) #5, #7-9, #11-12 → deferred per the Reviewer's own "future-work, not merge blocker / low priority / tiny / minor / nit" framing. KPI Details: Desktop: Lights: 16,384 Binary: 375 KB [doctest] test cases: 207 | 207 passed | 0 failed | 0 skipped tick: 354us, 100us, 303us, 99us, 99us, 98us, 45us, 35us, 34us (FPS: 2824, 10000, 3300, 10101, 10101, 10204, 22222, 28571, 29411) === 10 scenario(s), 10 passed, 0 failed === Platform boundary: PASS Specs: 26 modules, 26 ok, 0 missing, 0 outdated ESP32 (esp32s3-n16r8 on LOLIN S3 N16R8 at txPower=8 dBm): tick: 138506us (FPS: 7) [unchanged from 6212fc0 — desktop- only src/ change this round, no ESP32 behavioral delta] Code: 62 source files (10591 lines) 38 test files (5410 lines) 39 specs, 10 scenarios Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
ewowi
added a commit
that referenced
this pull request
Jun 12, 2026
… fixes Processes the Event-2 (pre-merge) Reviewer pass over the branch. The headline is a structural deduplication of the three LED drivers: the parallel WS2812 drivers (S3 LCD_CAM i80, P4 Parlio) were ~250 of ~370 lines byte-for-byte identical, now one shared CRTP base. Also fixes four stale doc/comment items, makes the AudioModule enabled-toggle actually stop its FFT cost, and removes two dead platform flags. No control names or output behaviour change; the LED hot path is unchanged (measured zero overhead, see below). KPI: 16384lights | PC:340KB | tick:116/95/118/115/56/1/37/20/113/332us(FPS:8620/10526/8474/8695/17857/1000000/27027/50000/8849/3012) | ESP32:1178KB | tick:17341us(FPS:57) | heap:33068KB | src:88(16543) | test:53(8518) | lizard:65w Light domain (the dedup, per the No-duplication rule): - ParallelLedDriver.h (new): a CRTP base ParallelLedDriver<Derived> holding the shared body of the parallel drivers — the pins/ledsPerPin/loopback controls, parseConfig, the per-ROW fused correct+encode loop(), reinit/deinit, the loopback self-test. Binding is CRTP (static polymorphism), NOT a virtual second hierarchy: the base calls derived()->busX() resolved at compile time, no vtable, no per-light indirection — so it stays inside the data-over-objects / hot-path rules and "the one deliberate class hierarchy is the module tree" rule. - LcdLedDriver.h: 378 → 92 lines, now a derived shell supplying only the i80-specific pieces (clockPin/dcPin, the exactly-8-pins rule via kExactLaneCount, the platform::lcdWs2812* calls). - ParlioLedDriver.h: 362 → 82 lines, the simpler shell (no clock/dc, kExactLaneCount=false, kClockHz). - Drivers.h (DriverBase): absorbs the status-string lifecycle (configErr_/failBuf_, setConfigErr/clearConfigErr/failBufEnsure/clearFailBuf) that was triplicated verbatim across all three drivers (RMT too). - RmtLedDriver.h: loses its status-lifecycle copy (now inherited); symbol-per-light model stays standalone (genuinely different from the parallel drivers — concrete-first boundary). Core: - AudioModule.h: removed respectsEnabled()=false so the Scheduler honours `enabled` — disabling the module now skips loop() entirely, stopping the FFT (the real per-tick cost). Verified on hardware: 524us enabled -> 0us disabled. The FFT is never gated while enabled (it is the capability audio effects consume, not an optional cost). - platform_config.h (esp32 + desktop): removed the dead isEsp32/isEsp32S3 family flags (no users anywhere); kept isEsp32P4 (drives ethPins + hasWifiCoprocessor). Desktop now mirrors with isEsp32P4=false. Docs / CI: - architecture.md: § Firmware vs board now lists the two P4 firmwares (esp32p4-eth, esp32p4-eth-wifi); § Drivers updated for the LCD_CAM/Parlio drivers and the four-driver shared Correction pointer. (👾 MUST-FIX 1, 2) - sdkconfig.defaults.esp32p4-eth-wifi: corrected the comment that described an esp_hosted bring-up prelude the code deliberately does NOT do (it was removed; esp_hosted self-inits at boot). Following the stale comment would reintroduce the bench-fixed SDIO teardown. (👾 MUST-FIX 3) - platform_esp32_lcd.cpp: the header + #else/#endif comments now name the correct SOC_LCDCAM_I80_LCD_SUPPORTED macro (they said SOC_LCD_I80_SUPPORTED — the exact macro confusion that caused the classic-ESP32 boot loop). (👾 MUST-FIX 4) - check_specs.py: skip CRTP template bases (template<...> class X : public DriverBase) — shared infrastructure, not a registered module; the concrete derived classes still carry the docs. - backlog.md: the LED-driver duplication item updated (driver-logic dedup landed via the CRTP base; the platform loopback capture+verify extraction remains as a follow-up). Reviews — 👾 Reviewer (Fable 5, over git diff main...HEAD; verdict "merge with noted fixes"): - Fixed all 4 MUST-FIX (stale docs/comments, above). - SHOULD-CONSIDER #1/#2 (Lcd/Parlio driver-logic + status duplication): FIXED via the CRTP base + DriverBase status lifecycle (chose to do it now per the No-duplication rule rather than backlog). The remaining platform loopback capture+verify duplication is the tracked follow-up. - SHOULD-CONSIDER #4 (AudioModule enabled toggle couldn't stop the FFT): FIXED (above); the FFT itself is must-have, only the disabled-still-runs gap was the bug. - SHOULD-CONSIDER #5 (unused isEsp32/isEsp32S3 flags): FIXED (removed). - SHOULD-CONSIDER #3 (audio headers in src/light/ giving a core->light arrow): accepted as-is — documented producer/consumer placement, boundary check passes. Verification: 56 driver/encoder unit tests pass, desktop build zero-warning, spec + platform-boundary clean. All three ESP32 targets (esp32, esp32s3-n16r8, esp32p4-eth-wifi) build clean under -Werror, including both CRTP instantiations. On hardware: S3 LcdLedDriver loopback bit-perfect over a 13->12 jumper (1536/1536 symbols captured, 0-bits 15..15 / 1-bits 30..30 ticks, zero jitter); P4 ParlioLedDriver renders the S3's audio spectrum live over the DDP path. Hot-path A/B (same S3, same config): pre-dedup ~3421-3442us/frame vs CRTP ~3425-3436us/frame — statistically identical, zero overhead (CRTP is static dispatch; the per-light loop is unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
added a commit
that referenced
this pull request
Jun 21, 2026
Follow-up to the "Improv = REST over serial" work on this branch: processes the CodeRabbit review of the prior commit and adds the host-side + device-side test coverage that makes config-push over serial provable without hardware. The Improv frame format (built three times — device C++, Python, installer JS) and the device's APPLY_OP chunk reassembly + sequence guard now have automated tests; the installer's frame builders were extracted into a shared, dependency-free module so the JS is testable in node at all. Also restores 18 historical plans under the corrected Plan-YYYYMMDD (ISO-8601) naming. KPI: 16384lights | PC:365KB | tick:111/87/110/9/1/312/37/15/19/111/11us(FPS:9009/11494/9090/111111/1000000/3205/27027/66666/52631/9009/90909) | ESP32:1249KB | tick:5155us(FPS:193) | heap:8335KB | src:97(19754) | test:68(10118) | lizard:77w (ESP32 tick 5155us is a fresh live capture from the S3 running the full testbench config — Grid + AudioSpectrum + RandomMap + RmtLed + NetworkSend broadcasting Art-Net. Higher than the old idle-ish 3.6ms because NetworkSend is active; this change touches no render-path code, so the tick is unchanged for a like-for-like config.) Core: - ImprovOpReassembler (new): extracted the APPLY_OP chunk-reassembly + out-of-order/duplicate sequence guard out of the ESP32 handler into a pure, header-only core state machine (the ImprovFrame.h precedent — algorithm in core, UART in platform). Returns a typed Continue/Ready/Error. This makes the heart of "REST over serial" desktop-unit-testable and shrinks the platform handler (removed two g_improv fields). - HttpServerModule: split OpResult::NotFound into ModuleNotFound + ControlNotFound so the HTTP /api/control handler reports the two distinct 404 bodies again (CodeRabbit #1); added OpResult::AlreadyExists so an idempotent add reports {"ok":true,"note":"already exists"} again (#5); documented the serial "parent" vs HTTP "parent_id" key difference at the applyOp site (#6). - ImprovProvisioningModule: a non-Ok APPLY_OP result is now surfaced — logged over serial and parked in provision_status — so a silently-misconfigured device is visible on a monitor and via /api/state instead of looking like a clean install (#2). Ok/AlreadyExists are both treated as success. Platform: - platform_esp32_improv.cpp: improvHandleApplyOp now delegates chunk merge + seq guard to mm::ImprovOpReassembler, keeping only the serial I/O (busy guard, ack/error sends). No behaviour change; the logic is now the unit-tested core path. UI: - improv-frame.js (new): the pure Improv frame builders (buildImprovFrame, encodeApplyOpFrames, the command IDs) extracted from install-orchestrator.js so node:test can import them without the orchestrator's top-level unpkg imports. The orchestrator imports them back — no behaviour change. - install-orchestrator.js: APPLY_OP inter-op pacing 30ms -> 120ms to cover the worst-case single-buffer consume window with headroom (CodeRabbit #3; the closed-loop read-back-ack upgrade is backlogged, ops are idempotent). Tests: - unit_ImprovOpReassembler (new, 12 cases): in-order multi-chunk reassembly + NUL-termination, duplicate-chunk rejection, out-of-order/skipped-seq rejection, overflow at the buffer-minus-NUL boundary, exact-fit boundary, mid-stream seq-0 reset, empty final chunk, recovery after every error. - test/js/improv-frame.test.mjs + test/python/test_improv_frame.py (new): assert the SAME golden frame (buildImprovFrame(0x03,[0x01]) == 49 4d 50 52 4f 56 01 03 01 01 e3) so the device C++, Python, and JS frame builders provably agree; the JS suite also pins APPLY_OP chunking (seq/last, the 125-byte boundary). Encode (JS) + reassemble (C++) prove APPLY_OP end to end without hardware. - unit_HttpServerModule_apply: updated for the ModuleNotFound/ControlNotFound/AlreadyExists split. Scripts / CI: - .github/workflows/test.yml (new): PR-triggered pytest (test/python) + node --test (test/js) — the first PR-level test gate (none existed). Python tests carry deps in a PEP-723 inline block (repo convention, no central pyproject). Docs / CI: - testing.md: new "Host-side tests (Python + JS)" tier with a per-test inventory of exactly what the Python/JS frame suites pin, plus the C++ reassembler coverage. test/python + test/js added to the file layout. - CLAUDE.md: added commit gate 10 (host-side Python/JS unit tests); corrected the plan naming convention to Plan-YYYYMMDD (ISO-8601, sorts chronologically) and clarified plans are product-owner reference (agents write, don't auto-read). - ImprovProvisioningModule.md: documented the serial-vs-HTTP parent key difference. - docs/history/plans: restored 18 historical plans (plan-01..18, deleted in an old merge) under Plan-YYYYMMDD names; renamed the Improv-as-REST plan to the corrected ISO date. - backlog: removed the shipped "board injection / general data injector" item (it IS APPLY_OP now); added the closed-loop-pacing follow-up; renamed stale boards.json -> deviceModels.json references. Reviews: - 🐇 #1 module-vs-control 404 specificity — fixed (OpResult split). - 🐇 #2 dropped APPLY_OP result — fixed (logged + provision_status). - 🐇 #3 open-loop 30ms pacing — fixed (bumped to 120ms; ack-loop backlogged). - 🐇 #4 perf-bound "regression" — accepted: the cited tick_us are observed.* telemetry (0 bounds, 21 observed), which only widens on a slow run; no render-path change, not a regression. - 🐇 #5 dropped "already exists" note — fixed (AlreadyExists OpResult). - 🐇 #6 parent vs parent_id — fixed (documented at code site + spec). - 🐇 #7 NUL-guard off-by-one — no action (CodeRabbit confirmed correct). - 🐇 #8 plan date format — fixed (YYYYDDMM -> ISO YYYYMMDD, all plans renamed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 24, 2026
Merged
This was referenced Jul 5, 2026
ewowi
added a commit
that referenced
this pull request
Jul 9, 2026
…A, testbench catalog Ships pins top-down increment #1: a read-only System module that walks the live module tree and shows which module owns each GPIO, for what role — the device's pin ownership map, keyed by physical GPIO like a Device Manager. Also fixes the firmware OTA push abort on MB-scale images, finalizes the Parlio loopback fix and the MM-testbench catalog, and processes the Reviewer findings. KPI: tick:131/91/121/15/2/281/59/17/21/160/120/17/1/71us (PC) · 4352us/FPS229 (ESP32) Core - PinsModule: new fixed, wired-by-code System child (Tasks/I2cScan pattern). Walks the tree, collects every ControlType::Pin (>=0) plus each LED-driver "pins" CSV, exposes a GPIO-keyed read-only list — owner + name-derived role (sckPin→BCLK, pins→LED lane N, sda/scl→I²C, ethMdcGpio→MDC, …). A GPIO claimed twice stays visible (both owners in the row detail) — read-only surfacing, not enforcement. Reads live on loop1s, holds no state; owner AND role are copied per-claim so a claim survives its module being deleted. Inverts MoonLight's central ModuleIO: each module owns its pins, this only observes. Hardware-tested on P4 + S3. - HttpServerModule: firmware push-OTA gets its own 5-min whole-request ceiling (kFirmwareUploadHardMs) instead of the file path's 60s anti-slowloris cap, which aborted MB-scale firmware uploads mid-flash (~87%). The idle guard and file-upload bounds are unchanged. Verified end-to-end on S3. - NetworkModule: ethPhyAddr is a PHY MDIO address (0..31), not a GPIO — bind it with addUint8 instead of addPin, so the pin map doesn't report it as a false GPIO claim. - platform_esp32_rmt: the loopback continuity log is kept as a permanent bench HAL diagnostic (reframed from a TEMP comment) — it pinned the MHC-WLED P4 shield loopback. Light domain - ParallelLedDriver: cap the loopback self-test frame to kLoopbackTestLights=256 so a large operational grid can't exceed the P4 Parlio single-transfer bit limit or the RMT-RX capture buffer. Verified PASS on MM-P4 (32/33, 8×8 and 128×128), S3 RMT (13/12), Olimex RMT (33/34). - CarLightsLayout / TorontoBarGourdsLayout: attribute the original authors (Eric Marciniak; troyhacks). UI / Installer - deviceModels.json + web-installer: MM-testbench catalog (P4/S3/olimex) — audio line-in, LED + loopback pins, images; renamed "projectMM testbench X" → "MM testbench X". Tests - unit_PinsModule: 7 cases — collects set pins + skips -1; parses the pins CSV into per-lane claims; GPIO-ordered rows; a double-claim shows both owners in detail; child modules walked depth-first; a claim survives its owner being destroyed (no use-after-free); an out-of-range CSV pin is skipped, not wrapped. - config-ops.test.mjs: "MM testbench S3" rename. Docs - pins-analysis-top-down (new, PO-approved) + bottom-up (added the MoonLight ModuleIO field survey); Plan-20260709 PinsModule saved; system.md Pins card + #pins anchor. - layouts.md: author column for the two attributed layouts. Reviews - 🐇 5 CodeRabbit findings processed (earlier this cycle). - 👾 Reviewer: FIXED a use-after-free in PinsModule (a claim now copies its owner name, not a pointer into module storage a delete frees — the UI serializes right after a delete, so it was a live read); FIXED ethPhyAddr addPin→addUint8 (a phantom GPIO claim the map surfaced); FIXED a broken markdown table, a CSV pin-range wrap, and a duplicated comment. Accepted (not changed): "MM testbench classic olimex" naming (touches the untracked bench registry; "classic" is intentional). Co-Authored-By: Claude Opus 4.8 <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
Test plan
🤖 Generated with Claude Code