Conversation
Two Home Assistant surfaces now work end-to-end on a projectMM device:
HA's WLED integration discovers it via zeroconf and controls it through
`/json` (a python-wled-compatible surface added alongside the existing
Android-app minimum), and MQTT-Discovery creates a light entity with the
correct single slug via `name: null`. A latent bug where
`MoonModule::readUint8` didn't accept `ControlType::Palette` silently
made every WLED colour readback resolve to Rainbow/white; fixing it
lit up the whole palette-representative colour path on the seg[0].col
surface.
KPI: PC tick 1916us (FPS:521, per scenario). ESP32 tick+FPS not
captured this session — devices are OTA-flashed, no serial monitor
attached to any of the four rigs. To be re-captured next commit that
touches ESP32-only paths.
- Core
- HttpServerModule: new /json route (serveWledDeviceJson) returning
the full python-wled-compatible blob HA's WLED integration fetches,
plus /presets.json (truthy-but-empty `{"0":{}}` so the config flow
doesn't 500 on the empty-presets guard). writeWledStateBody +
serveWledDeviceJson now emit seg[0].on / bri (255 = 100% of master,
per has_main_light=False maths) / col (palette-representative,
stable across effect animation, via new helper). applyWledState
handles POST /json/state's seg[0].col=[[r,g,b]] via
Palettes::nearestForRgb. ws:-1 in info tells python-wled 'WebSocket
unsupported in this build' so HA stops trying to open our /ws and
flooding its log with MissingField:filesystem parse errors.
- MqttModule: HA discovery config emits `name: null` per HA's docs
('the entity IS the device'), so the auto-created slug is
`light.<device>` rather than the doubled `light.<device>_<device>`
HA produces when the light name matches the device name.
- MoonModule: readUint8 now accepts ControlType::Palette alongside
Uint8 and Select. Previously the Palette omission meant
driversPalette() silently returned dflt=0 (Rainbow), which made
seg[0].col resolve to Rainbow's representative (white) regardless
of the palette actually picked — verified on the bench.
- Light domain
- Palette: two public helpers extracted for reuse. `nearestForRgb`
(RGB->HSV->nearest palette in one call, the entry HA's colour picker
hits via /json/state) and `representativeRgb` (palette index -> RGB
identity colour, HSV V=255 so the reported hue survives external
dimming, used by every WLED-shim response).
- Tests
- test_wled_json_shape.py: new golden-vector Python test that parses
a captured /json response through frenck/python-wled's real Device
dataclasses. Catches wire-format regressions before they reach a
live HA install. Runs in the existing `Test` workflow via the
`test/python/**` path filter.
Skipped commit gates (pre-existing failures on branch HEAD, verified
against a full working-tree stash — same failures without our diff):
- ctest: 18 test cases fail in unit_AudioModule_sync.cpp and adjacent
audio tests. Unrelated to any surface this commit touches.
- pytest: 5 failures in test_verify_version.py + test_installer_
manifests.py. Both test version/manifest drift against a git tag
that this branch has moved past. Unrelated to any surface this commit
touches.
- scenarios: 2 failures in scenario_perf_full / scenario_perf_light
('buffer non-zero after render' in MoonLive Stage-1 CONTROLS).
Unrelated (MoonLive is a light-domain scripting engine, no overlap
with the WLED shim).
Follow-up commit will address these.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Windows had 18 failing ctest and 2 failing scenarios inherited from an
arm64-only assumption in the MoonLive backend; a Winsock semantic mismatch
in UdpSocket::bind; and a pytest that silently missed its wled dep. All
green now, with the "runs dark on unsupported host" degradation clearly
gated instead of hand-waved. Home-automation docs reorganised device-side
first (the recipe most readers need) with the infra install moved to an
appendix; adds a full HA + Hyper-V + Mosquitto + HomeKit Bridge walkthrough.
KPI: 16384lights | PC:745KB | tick:3853/52/64/1656/5316/2461/6/209/92/66/4/1733/684/48us(FPS:259/19230/15625/603/188/406/166666/4784/10869/15151/250000/577/1461/20833)
ESP32 KPI absent: no ESP32 on serial this session (all devices on WiFi).
Core:
- moonlive_emit.h: new MM_MOONLIVE_HAS_HOST_JIT compile-time flag (1 on arm64, 0 on x86_64 SysV/Win64) — the emit blob + assembler are arm64-only today, so the flag lets tests and scenarios reflect the same graceful degradation the runtime already implements.
- platform_desktop.cpp: UdpSocket::bind no longer sets SO_REUSEADDR on Windows. Winsock semantics of that flag are the opposite of POSIX (allows overlapping bind), which was letting a second live socket claim an already-bound UDP port and breaking the audio-sync bind-backoff test's hog-then-module scenario.
Tests:
- unit_moonlive_fill / unit_moonlive_compiler / unit_moonlive_ir: gate compile-through-run tests on MM_MOONLIVE_HAS_HOST_JIT so the JIT-dependent assertions skip on x86_64 hosts. Degradation checks (reject too-small buffer, reject null, malformed input) stay unguarded.
- scenario_runner.cpp: honour a scenario-level "skip_on" allowlist of host targets. Keeps mm_scenarios in step with run_scenario.py so KPI collection (which calls the C++ runner directly) doesn't count MoonLive scenarios as failures on non-JIT hosts.
- test/scenarios/light/scenario_MoonLiveEffect_{controls,livescript}.json: opt out on pc-windows / pc-linux.
- test/scenarios/**: observed[pc-windows] blocks refreshed from this run.
Scripts / MoonDeck:
- run_scenario.py: honours the same skip_on field the C++ runner does.
- ci/verify_version.py: reconfigure stdout/stderr to UTF-8 so the "↔" in the OK summary doesn't UnicodeEncodeError on Windows Python 3.14 (cp1252 default).
Docs:
- docs/usecases/home-automation.md: reorganised device-side first — Adopt in Home Assistant, Adopt in Homebridge, Drive Hue lights, Other platforms — with "Set up the infrastructure" moved to an appendix (HAOS on Hyper-V walkthrough + Mosquitto add-on for HA; standalone Mosquitto + Homebridge for Pi/macOS/Windows). Adds a "Why keep both?" paragraph clarifying that MQTT and WLED discovery cover different transport / reachability / fleet-consistency needs, not different feature richness (WLED's traditional richer surface surfaces only projectMM's placeholder lists today).
- docs/moonmodules/core/services.md: MQTT reference updated — HA has two zero-config paths (MQTT auto-discovery + WLED integration), both link into the new walkthrough.
CI:
- .github/workflows/test.yml: add --with wled to the pytest job so test_wled_json_shape.py finds the python-wled library its Device.from_dict contract test parses through.
- test/python/test_installer_manifests.py: explicit encoding="utf-8" on the manifest/JSON reads so the Windows Python 3.14 cp1252 default doesn't UnicodeDecodeError on a non-ASCII byte in the JSON.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Collaborator
Author
|
@coderabbitai, cancel generating unit tests |
|
Failed to clone repository into sandbox. |
|
Tip For best results, initiate chat on the files or code changes.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/usecases/home-automation.md`:
- Around line 23-24: The setup guidance in the home automation doc currently
makes Mosquitto sound required for every path, but the WLED-only Home Assistant
flow does not need it. Update the onboarding text and the related infrastructure
section references so the Home Assistant/WLED path is described separately from
the MQTT-based Home Assistant and Homebridge paths, using the existing home
automation setup wording to clearly mark which flows require a broker and which
do not.
- Line 230: The HA WLED troubleshooting bullet points users at the wrong
endpoint and port, which can misdiagnose partial compatibility failures. Update
the relevant WLED troubleshooting text in the home-automation docs to tell users
to probe the device’s real HTTP port (default 8080) and include both the /json
and /presets.json endpoints. Keep the guidance tied to the HA WLED integration
check so readers can distinguish a missing presets response from an older
firmware issue.
In `@src/core/HttpServerModule.cpp`:
- Around line 18-20: The new Palette include in HttpServerModule.cpp conflicts
with the class documentation that says this module stays free of light-domain
includes. Update HttpServerModule.h’s class doc to explicitly acknowledge the
sanctioned exception, or refactor the RGB↔palette bridge behind a domain-neutral
virtual like firstOutputRgb() so the implementation matches the documented
invariant. Use the existing HttpServerModule, firstOutputRgb(), and
Palettes::nearestForHue symbols to locate the affected path.
- Around line 1105-1114: The name/MAC/IP identity-resolution logic is duplicated
in multiple handlers, including the current block and the existing serveWledInfo
and serveWledStateInfo paths. Extract this repeated `findModuleByName("System")`
/ `deviceName()` / `getMacAddress()` / `ethGetIPv4()` / `wifiStaGetIPv4()`
sequence into a small private helper such as `resolveWledIdentity(...)` in
HttpServerModule, then call that helper from all three handlers to keep the
behavior consistent and avoid further duplication.
- Around line 1116-1131: serveWledDeviceJson is hand-duplicating the exact state
JSON that writeWledStateBody already emits, which risks drift between the two
paths. Refactor the state block in serveWledDeviceJson to call
writeWledStateBody(sink) directly, just like serveWledStateInfo does, and remove
the local bri/pc/onStr re-derivation and repeated appendf literal. Keep the
state output identical by reusing the existing helper instead of maintaining two
copies of the same JSON structure.
In `@src/core/moonlive/moonlive_emit.h`:
- Around line 26-30: Move the __aarch64__-based MM_MOONLIVE_HAS_HOST_JIT
definition out of moonlive_emit.h and into the platform layer, ideally in
platform_config.h or the existing platform header used by src/platform/. Then
update the core header to consume that flag instead of performing its own
architecture-specific preprocessor check, keeping moonlive_emit.h
platform-independent and aligned with the existing platform branching pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 213b79f2-9f34-43ab-8843-00ec19984f38
📒 Files selected for processing (33)
.github/workflows/test.ymldocs/moonmodules/core/services.mddocs/usecases/home-automation.mdmoondeck/ci/verify_version.pymoondeck/scenario/run_scenario.pysrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/core/MqttModule.cppsrc/core/moonlive/moonlive_emit.hsrc/light/Palette.hsrc/platform/desktop/platform_desktop.cpptest/python/test_installer_manifests.pytest/python/test_wled_json_shape.pytest/scenario_runner.cpptest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layer_memory_1to1.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MoonLiveEffect_controls.jsontest/scenarios/light/scenario_MoonLiveEffect_livescript.jsontest/scenarios/light/scenario_MultiplyModifier_pipeline.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/core/unit_moonlive_compiler.cpptest/unit/core/unit_moonlive_fill.cpptest/unit/core/unit_moonlive_ir.cpp
Comment on lines
+18
to
+20
| #include "light/Palette.h" // Palettes::nearestForHue — maps HA's RGB colour picker onto our | ||
| // hue→palette convention (same core→light bridge MqttModule uses | ||
| // for hsv/set; see the note in MqttModule.cpp:7-14). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
New light-domain include contradicts this module's documented "no light-domain include" invariant.
HttpServerModule.h's class doc states the colour path is deliberately routed through the domain-neutral firstOutputRgb() virtual "keeping this module free of any light-domain include." This PR adds #include "light/Palette.h" directly into this core .cpp, which is now factually inconsistent with that documented design goal (even though the comment here notes MqttModule.cpp already does the same). Either the header doc needs updating to acknowledge this sanctioned exception, or the palette RGB↔index bridge should go through a domain-neutral virtual (mirroring firstOutputRgb()) so core stays free of light-domain includes as advertised.
As per coding guidelines, **/*.{md,cpp,h}: "Do not remove comments unless they are outdated or factually wrong" — the existing class-doc claim is now stale and should be corrected to match reality.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/HttpServerModule.cpp` around lines 18 - 20, The new Palette include
in HttpServerModule.cpp conflicts with the class documentation that says this
module stays free of light-domain includes. Update HttpServerModule.h’s class
doc to explicitly acknowledge the sanctioned exception, or refactor the
RGB↔palette bridge behind a domain-neutral virtual like firstOutputRgb() so the
implementation matches the documented invariant. Use the existing
HttpServerModule, firstOutputRgb(), and Palettes::nearestForHue symbols to
locate the affected path.
Source: Coding guidelines
Two orthogonal HA-integration threads land together with the CodeRabbit
review findings on the previous PR. First: HA WLED integration was flagging
a bogus "WLED 16.0.1 available" update against every projectMM device
because its update-check parses WLED tags as CalVer, so a projectMM semver
like 2.1.0-dev compared LOWER than 16.0.1 (2 < 16) and HA offered a WLED
.bin that would brick the device. Second: even after the WLED badge is
silenced, HA had no first-class way to surface a REAL projectMM release.
Both fixed by reporting a sentinel `99.0.0` in /json's info.ver (beats
any WLED tag in the CalVer regime), and by publishing a proper HA MQTT
update entity as a second discovery component alongside the existing
light. Alongside: the review's structural improvements to
HttpServerModule and the moonlive platform-boundary move. Verified on
three live devices (MM-P4, MM-S31, MM-LC16) — all three publish the
update entity to HA and no longer trigger the WLED nag.
KPI: 16384lights | PC:749KB | tick:3191/44/52/2003/4608/2001/7/178/110/48/9/1288/645/41us(FPS:313/22727/19230/499/217/499/142857/5617/9090/20833/111111/776/1550/24390)
ESP32 KPI absent: devices flashed OTA this session, none on serial.
Core:
- MqttModule: publishes a second HA-discovery component on `homeassistant/update/projectMM_<mac6>/config` alongside the existing light; state on `<prefix>/update/state` with installed_version = MM_VERSION and (for now) latest_version = installed_version; subscribes `<prefix>/update/set` and routes an install command through platform::http_fetch_to_ota with the release-artifact URL built from the payload version and kFirmwareName. `device_class:"firmware"` on the discovery config so HA labels the entity `<device> Firmware` (not colliding with the light's identical name). One haDiscovery gate, one MAC-stable device.ids, both components announced/retracted together. Discovery framing reuses the lazily-allocated discoveryBuf_/discoveryPayload_ (448+320 bytes) rather than an on-stack kSendBufLen buffer — the ~300-byte update payload overflowed the 256-byte stack scratch on the bench and set "error: update discovery too large" until the fix.
- HttpServerModule: /json's info.ver now reports "99.0.0" (sentinel), not the projectMM semver. HA's WLED integration parses WLED tags as CalVer (16.0.1 = year-16), so a projectMM semver 2.x.y is LOWER (2 < 16) and HA nags with a bogus "WLED 16.0.1 available" update. AwesomeVersion("99.0.0") > any WLED tag in the CalVer regime, so the WLED update-check stays silent. First tried mm::kVersion assuming SemVer parsing — bench P4 showed HA still flagging 16.0.1 after the flash because CalVer was the actual branch. Real projectMM version lives on the new MQTT update entity's installed_version, which is where "did projectMM ship a new release" belongs.
- HttpServerModule: extracted resolveWledIdentity(name, mac, ip) helper — one lookup, four call sites (serveWledInfo, serveWledStateInfo, serveWledDeviceJson, pushWledStateToWebSockets) that were hand-copying the same findModuleByName("System")/getMacAddress/eth+wifi lookup.
- HttpServerModule: serveWledDeviceJson now calls writeWledStateBody(sink) inside "state":, matching the serveWledStateInfo pattern — the state block is written by one authoritative helper instead of two.
- HttpServerModule.h: class doc acknowledges the light/Palette.h include as a sanctioned exception (same shape as MqttModule's), documented at the introduction site per Common patterns first.
- HttpServerModule.cpp: cleaned up a stale docstring on writeWledStateBody that still described the old first-LED behaviour — the palette-representative colour is the current + correct behaviour for every WLED consumer (native app, HA, Homebridge, /ws push).
- moonlive_emit.h: dropped its `#if defined(__aarch64__)` check. MM_MOONLIVE_HAS_HOST_JIT now lives in `src/platform/{desktop,esp32}/platform_config.h`, keeping the arch check behind the platform boundary per the platform-boundary rule.
Tests:
- test_wled_json_shape.py: golden vector updated — info.ver is now "99.0.0" (sentinel). Comment explains the CalVer-vs-SemVer version-comparison contract.
Docs / CI:
- docs/usecases/home-automation.md: onboarding split per-path — HA+MQTT vs HA-alone (WLED integration, no broker needed) vs Homebridge+MQTT; WLED troubleshooting now points at both /json AND /presets.json and calls out the desktop port (8080) alongside ESP32 port 80.
- docs/moonmodules/core/services.md: MQTT topic table gains `update/state` and `update/set` rows.
- docs/backlog/backlog-core.md: HA update entity item marked shipped for discovery + install command; only the device-side release check remains open (blocked on a platform::http_fetch_to_buffer seam + assets[] JSON walk), documented as the follow-up.
Reviews:
- 🐇 Prerequisites over-scoped MQTT as required for every HA path — split into per-path infra list (HA+MQTT / HA-alone / Homebridge+MQTT); fixed.
- 🐇 WLED troubleshooting bullet pointed at the wrong endpoint/port — now includes port 8080 (desktop), 80 (ESP32), and probes both /json and /presets.json with distinct diagnostics per failure mode; fixed.
- 🐇 HttpServerModule's #include "light/Palette.h" contradicted the class doc — updated the doc to acknowledge the same sanctioned exception MqttModule already documents (RGB↔palette conversion is inherently light-domain, module still drives palette via Scheduler::setControl); fixed by doc, per PO-accepted pattern.
- 🐇 Identity-resolution duplicated in four handlers — extracted resolveWledIdentity() helper; fixed.
- 🐇 serveWledDeviceJson hand-duplicated writeWledStateBody's state JSON — now calls writeWledStateBody() directly; fixed.
- 🐇 MM_MOONLIVE_HAS_HOST_JIT #if defined(__aarch64__) in core header violated the platform boundary — moved into `src/platform/{desktop,esp32}/platform_config.h`; fixed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/moonmodules/core/services.md (1)
205-207: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the HA discovery entity brightness-only.
The
haDiscoverypayload only advertises on/off + brightness; HSV/palette stays on the separate MQTT surface, not the auto-created HA light.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/core/services.md` around lines 205 - 207, The Home Assistant MQTT auto-discovery description is overstating the entity capabilities in the haDiscovery flow; update the docs around the MQTT auto-discovery and WLED integration entries to say the auto-created HA light is brightness-only with on/off, while HSV/palette remains available only through the separate MQTT surface. Keep the wording aligned with the haDiscovery payload and the projectMM_<mac6>/config discovery behavior.docs/usecases/home-automation.md (1)
54-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the HA discovery bullet to on/off + brightness. The discovery light only carries
stateandbrightness; HSV/palette control still goes through the rawhsv/settopic. Keep that split explicit so users don’t expect a color wheel in HA.🤖 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/usecases/home-automation.md` around lines 54 - 55, The MQTT auto-discovery description in the home-automation docs overstates Home Assistant control capabilities for the discovered light entity. Update the bullet that describes the `haDiscovery` path so it only lists on/off and brightness support for the auto-created entity, and make it explicit that HSV/palette control still uses the separate `hsv/set` topic; keep the wording aligned with the `MQTT auto-discovery` and `WLED integration` bullets so the split between HA discovery and raw topic control is clear.
🤖 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/usecases/home-automation.md`:
- Around line 24-26: The section links in the home-automation bullets point to
fragments that won’t match the generated Markdown slugs because the heading text
includes “+” and becomes doubled hyphens. Update the three link targets in this
list to use the exact anchor slugs produced by the corresponding headings, and
verify the references for “set up home assistant mosquitto” and “set up
standalone homebridge mosquitto” resolve correctly.
In `@src/core/MqttModule.cpp`:
- Around line 278-298: The empty-payload branch in
MqttModule::handleUpdateInstall currently returns early, which violates the
documented “install latest” behavior for update/set. Update the payload parsing
in handleUpdateInstall so an empty string falls back to the latest version
target (today that can be kVersion), while keeping the existing otaInFlight()
guard and version-string validation intact. Use the existing handleUpdateInstall
and g_otaStatus flow to preserve the current OTA path and failure reporting.
---
Outside diff comments:
In `@docs/moonmodules/core/services.md`:
- Around line 205-207: The Home Assistant MQTT auto-discovery description is
overstating the entity capabilities in the haDiscovery flow; update the docs
around the MQTT auto-discovery and WLED integration entries to say the
auto-created HA light is brightness-only with on/off, while HSV/palette remains
available only through the separate MQTT surface. Keep the wording aligned with
the haDiscovery payload and the projectMM_<mac6>/config discovery behavior.
In `@docs/usecases/home-automation.md`:
- Around line 54-55: The MQTT auto-discovery description in the home-automation
docs overstates Home Assistant control capabilities for the discovered light
entity. Update the bullet that describes the `haDiscovery` path so it only lists
on/off and brightness support for the auto-created entity, and make it explicit
that HSV/palette control still uses the separate `hsv/set` topic; keep the
wording aligned with the `MQTT auto-discovery` and `WLED integration` bullets so
the split between HA discovery and raw topic control is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2f3f69c-68af-4061-a45c-b3ca6a16880c
📒 Files selected for processing (24)
docs/backlog/backlog-core.mddocs/moonmodules/core/services.mddocs/usecases/home-automation.mdsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/core/moonlive/moonlive_emit.hsrc/platform/desktop/platform_config.hsrc/platform/esp32/platform_config.htest/python/test_wled_json_shape.pytest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/light/scenario_Audio_mutation.jsontest/scenarios/light/scenario_Driver_mutation.jsontest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_Layer_base_pipeline.jsontest/scenarios/light/scenario_Layer_memory_1to1.jsontest/scenarios/light/scenario_Layers_composition.jsontest/scenarios/light/scenario_Layouts_mutation.jsontest/scenarios/light/scenario_MultiplyModifier_pipeline.jsontest/scenarios/light/scenario_modifier_chain.jsontest/scenarios/light/scenario_modifier_swap.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.json
Comment on lines
+24
to
+26
| - **HA + Mosquitto broker add-on** — for MQTT auto-discovery (richer state + HomeKit Bridge). [Set up HA + Mosquitto](#set-up-home-assistant-mosquitto). | ||
| - **HA alone (no broker)** — for the WLED-integration path. Same HA install, just skip the Mosquitto add-on. | ||
| - **Homebridge + a Mosquitto broker** — for the standalone Apple-Home-only path. [Set up standalone Homebridge + Mosquitto](#set-up-standalone-homebridge-mosquitto). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the section links.
These headings include +, so the generated Markdown slugs use doubled hyphens here. The current fragments will not resolve correctly.
♻️ Proposed fix
- - **HA + Mosquitto broker add-on** — for MQTT auto-discovery (richer state + HomeKit Bridge). [Set up HA + Mosquitto](`#set-up-home-assistant-mosquitto`).
+ - **HA + Mosquitto broker add-on** — for MQTT auto-discovery (richer state + HomeKit Bridge). [Set up HA + Mosquitto](`#set-up-home-assistant--mosquitto`).
@@
- - **Homebridge + a Mosquitto broker** — for the standalone Apple-Home-only path. [Set up standalone Homebridge + Mosquitto](`#set-up-standalone-homebridge-mosquitto`).
+ - **Homebridge + a Mosquitto broker** — for the standalone Apple-Home-only path. [Set up standalone Homebridge + Mosquitto](`#set-up-standalone-homebridge--mosquitto`).📝 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
| - **HA + Mosquitto broker add-on** — for MQTT auto-discovery (richer state + HomeKit Bridge). [Set up HA + Mosquitto](#set-up-home-assistant-mosquitto). | |
| - **HA alone (no broker)** — for the WLED-integration path. Same HA install, just skip the Mosquitto add-on. | |
| - **Homebridge + a Mosquitto broker** — for the standalone Apple-Home-only path. [Set up standalone Homebridge + Mosquitto](#set-up-standalone-homebridge-mosquitto). | |
| - **HA + Mosquitto broker add-on** — for MQTT auto-discovery (richer state + HomeKit Bridge). [Set up HA + Mosquitto](`#set-up-home-assistant--mosquitto`). | |
| - **HA alone (no broker)** — for the WLED-integration path. Same HA install, just skip the Mosquitto add-on. | |
| - **Homebridge + a Mosquitto broker** — for the standalone Apple-Home-only path. [Set up standalone Homebridge + Mosquitto](`#set-up-standalone-homebridge--mosquitto`). |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Link fragments should be valid
(MD051, link-fragments)
[warning] 26-26: Link fragments should be valid
(MD051, link-fragments)
🤖 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/usecases/home-automation.md` around lines 24 - 26, The section links in
the home-automation bullets point to fragments that won’t match the generated
Markdown slugs because the heading text includes “+” and becomes doubled
hyphens. Update the three link targets in this list to use the exact anchor
slugs produced by the corresponding headings, and verify the references for “set
up home assistant mosquitto” and “set up standalone homebridge mosquitto”
resolve correctly.
Source: Linters/SAST tools
Comment on lines
+278
to
+298
| // HA's install command. The payload is the target version string (via HA's | ||
| // payload_install_template — defaults to `{{ latest_version }}`); an empty payload | ||
| // means "install latest". The device builds the download URL from the projectMM | ||
| // release-artifact convention: | ||
| // https://github.com/MoonModules/projectMM/releases/download/v<version>/firmware-<kFirmwareName>-v<version>.bin | ||
| // and hands it to platform::http_fetch_to_ota — the same OTA path POST /api/firmware/url | ||
| // takes. Guarded by otaInFlight() so a second install command mid-flash returns silently | ||
| // rather than corrupting the running OTA task. On desktop platform::http_fetch_to_ota is | ||
| // a stub returning false; the install command safely reports failure via g_otaStatus. | ||
| void MqttModule::handleUpdateInstall(const char* payload, size_t payloadLen) { | ||
| if (otaInFlight()) return; // matches the /api/firmware/url 409 guard's intent | ||
|
|
||
| // Copy the payload into a bounded local buffer for null-termination + shape checks. | ||
| // A malformed / oversized payload is refused; no partial URL reaches http_fetch_to_ota. | ||
| char version[32] = {}; | ||
| const size_t vlen = payloadLen < sizeof(version) - 1 ? payloadLen : sizeof(version) - 1; | ||
| std::memcpy(version, payload, vlen); | ||
| version[vlen] = '\0'; | ||
| // Strip an optional leading 'v' — HA's payload_install_template can send either shape. | ||
| const char* v = (version[0] == 'v') ? version + 1 : version; | ||
| if (v[0] == '\0') return; // empty install command with no default: nothing to install |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty update/set payload silently no-ops instead of "install latest".
The function's own comment and docs/moonmodules/core/services.md both state an empty payload means "install latest", but Line 298 just returns without acting. Since latest_version currently equals kVersion (installed), falling back to kVersion here would actually satisfy the documented contract (a harmless no-op reinstall today, and correctly meaningful once the release-check lands).
🔧 Proposed fix to honor the "install latest" contract
// Strip an optional leading 'v' — HA's payload_install_template can send either shape.
const char* v = (version[0] == 'v') ? version + 1 : version;
- if (v[0] == '\0') return; // empty install command with no default: nothing to install
+ if (v[0] == '\0') v = kVersion; // empty payload = "install latest"; latest == kVersion todayAs per path instructions, docs/moonmodules/core/services.md states: "accept command projectMM/<mac>/update/set where an empty string means "install latest"."
📝 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
| // HA's install command. The payload is the target version string (via HA's | |
| // payload_install_template — defaults to `{{ latest_version }}`); an empty payload | |
| // means "install latest". The device builds the download URL from the projectMM | |
| // release-artifact convention: | |
| // https://github.com/MoonModules/projectMM/releases/download/v<version>/firmware-<kFirmwareName>-v<version>.bin | |
| // and hands it to platform::http_fetch_to_ota — the same OTA path POST /api/firmware/url | |
| // takes. Guarded by otaInFlight() so a second install command mid-flash returns silently | |
| // rather than corrupting the running OTA task. On desktop platform::http_fetch_to_ota is | |
| // a stub returning false; the install command safely reports failure via g_otaStatus. | |
| void MqttModule::handleUpdateInstall(const char* payload, size_t payloadLen) { | |
| if (otaInFlight()) return; // matches the /api/firmware/url 409 guard's intent | |
| // Copy the payload into a bounded local buffer for null-termination + shape checks. | |
| // A malformed / oversized payload is refused; no partial URL reaches http_fetch_to_ota. | |
| char version[32] = {}; | |
| const size_t vlen = payloadLen < sizeof(version) - 1 ? payloadLen : sizeof(version) - 1; | |
| std::memcpy(version, payload, vlen); | |
| version[vlen] = '\0'; | |
| // Strip an optional leading 'v' — HA's payload_install_template can send either shape. | |
| const char* v = (version[0] == 'v') ? version + 1 : version; | |
| if (v[0] == '\0') return; // empty install command with no default: nothing to install | |
| // Strip an optional leading 'v' — HA's payload_install_template can send either shape. | |
| const char* v = (version[0] == 'v') ? version + 1 : version; | |
| if (v[0] == '\0') v = kVersion; // empty payload = "install latest"; latest == kVersion today |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 287-287: method 'handleUpdateInstall' can be made static
(readability-convert-member-functions-to-static)
[warning] 288-288: statement should be inside braces
(readability-braces-around-statements)
[warning] 292-292: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 297-297: variable name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 298-298: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.21.0)
[style] 289-289: The function 'addPin' is never used.
(unusedFunction)
[style] 291-291: The function 'lastFlags' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/MqttModule.cpp` around lines 278 - 298, The empty-payload branch in
MqttModule::handleUpdateInstall currently returns early, which violates the
documented “install latest” behavior for update/set. Update the payload parsing
in handleUpdateInstall so an empty string falls back to the latest version
target (today that can be kVersion), while keeping the existing otaInFlight()
guard and version-string validation intact. Use the existing handleUpdateInstall
and g_otaStatus flow to preserve the current OTA path and failure reporting.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits, one thread: bring projectMM into Home Assistant end-to-end (WLED integration + MQTT auto-discovery + HomeKit Bridge via HA), and green the ctest/scenario/pytest suite on Windows along the way.
/json+/presets.jsonshaped to whatfrenck/python-wled'sDevice.from_dictactually validates (state.seg[0].on/bri/col, info.fs, info.ws:-1), so HA's built-in WLED integration adopts the device with no manual config.col:[[r,g,b]]maps back throughPalettes::nearestForRgb; the representative colour lives in a sharedPalettes::representativeRgbhelper (used by both/jsonand MQTT). Golden-vector test (test/python/test_wled_json_shape.py) parses the captured JSON through python-wled's real dataclass models so a future breakage fails locally before HA sees it."name": nullso HA uses the device object's name and doesn't produce the doubled slug (light.mm_s31_mm_s31).MoonModule::readUint8acceptsControlType::Palette(it previously silently returned 0, making every palette read the Rainbow default → white).SO_REUSEADDRhas the opposite semantic of POSIX (allows overlapping bind), breaking the audio-sync bind-backoff test. NewMM_MOONLIVE_HAS_HOST_JITcompile-time flag gates the JIT-dependent tests;skip_onlist on the scenario JSONs (honoured by both the Python wrapper and the C++ runner so KPI stays honest);bind()drops SO_REUSEADDR on Windows.docs/usecases/home-automation.mdreorganised device-side first (Adopt in HA / Adopt in Homebridge as the recipes readers land on) with a Set up the infrastructure appendix at the end covering HAOS-on-Hyper-V, the Mosquitto add-on, and standalone Pi/macOS/Windows Homebridge. Adds a "Why keep both discovery paths on?" paragraph explaining that MQTT-discovery and WLED-integration cover different transport / reachability / fleet-consistency needs, not different feature richness.Test plan
ctest --test-dir build/windows -C Release— 620/620 pass (was 149 pass, 18 fail, 478 skipped)uv run moondeck/scenario/run_scenario.py— 20/20 pass (2 MoonLive scenarios cleanly skip via newskip_onfield on x86_64 hosts)uv run --with pytest --with pyserial --with markdown --with wled pytest test/python -q— 55/55 passuv run moondeck/check/check_specs.py— 82/82 modules okuv run moondeck/check/check_platform_boundary.py— PASSuv run moondeck/build/build_esp32.py— all 6 firmwares build (esp32, esp32-eth, esp32p4-eth, esp32s3-n8r8, esp32s3-n16r8, esp32s31)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/jsonsupport and improved RGB/palette handling for smoother app integration.Bug Fixes
Documentation
Tests