S31 mic + I2cScan, DevicesModule UDP discovery + WLED native-app interop#31
Merged
Conversation
Wires up the ESP32-S31 onboard microphone path (ES8311 codec over I2C), adds a reusable I2C bus-scan diagnostic module, and gives every driver an explicit start/count window into the shared light buffer so an onboard LED and a strip can run as two independent drivers. The S31 codec is reachable on I2C (bench-confirmed) but its open still needs a hardware MCLK check, so the mic fails gracefully until then; everything else is hardware-verified on S31 + S3.
tick:62us(FPS:16129) [PC] | ESP32:1297KB | S31 ~915us(FPS~1090) | S3 ~14481us(FPS69)
Core
- platform.h / platform_esp32_es8311.cpp: ES8311 codec seam (CodecType / AudioCodecPins / audioCodecInit) over a new I2C master bus via esp_codec_dev; codec is reachable (ACK 0x18) but es8311_open's clock verify still fails — needs MCLK-on-GPIO52 hardware confirmation, captured as a follow-up.
- platform_esp32_i2s.cpp / AudioModule.h: thread the codec MCLK pin through audioMicInit; AudioModule brings up I2S before the codec and fails gracefully ("codec init failed") on a codec error, no crash.
- platform_esp32_i2c.cpp / platform.h: new domain-neutral platform::i2cScan seam (i2cdetect-style; opens a temporary bus, probes 0x01-0x77, tears down) with a desktop stub.
- I2cScanModule.h: new System-peripheral diagnostic (sda/scl pins default GPIO21/22, scan button, result read-out) mirroring DevicesModule; factory-registered in main.cpp.
- Control.h / esp32 CMakeLists: MM_MAX_GPIO derived from the IDF's CONFIG_SOC_GPIO_PIN_COUNT (S31=61) so pin controls accept the chip's full GPIO range, with a header fallback to keep Control.h core.
Light domain
- Drivers.h (DriverBase): shared start/count source-buffer window (windowSlice/setWindow/isWindowControl) — every driver outputs a contiguous [start, start+count) slice (count 0 = to end), making light distribution explicit and order-independent.
- RmtLedDriver / ParallelLedDriver (Lcd/Parlio): apply the window in parseConfig (ledsPerPin distributes over the slice) and the read loop (offset by the window start); start/count trigger a live reconfig.
- NetworkSendDriver: replace the old light_count control with the shared start/count window (universe_start stays the separate protocol offset). NOTE: persistence break — a saved light_count value is dropped on load (acceptable pre-1.0).
Tests
- unit_RmtLedDriver_pins: four window cases (size-1 onboard slice, count-0-rest, distribute-over-window, past-end-empty-clamp).
Docs / CI
- deviceModels.json: S31 AudioModule + I2cScanModule (sda 51 / scl 50); I2cScanModule on both P4 boards (sda 7 / scl 8); testbench S3 onboard-LED driver (pin 48, window [0,1)) mirroring grid light 0; S31 planned-features list expanded.
- docs/reference/esp32-s31-coreboard.md: new durable S31 hardware reference (audio/eth/RGB pin maps, datasheet-verified; SDA/SCL are GPIO51/50, opposite the schematic net labels).
- Drivers.md / NetworkSendDriver.md / I2cScanModule.md: document the source window and the scan module; backlog per-driver-window item removed (shipped).
- Plan-20260628 saved.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/light/drivers/NetworkSendDriver.h (1)
121-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWindowed sends still reserve a full-frame correction buffer.
This loop now sends only
nLights, butresizeCorrected()still allocatescorrected_forsourceBuffer_->count(). With multiple narrow window senders, each instance keeps a full-frame buffer; on larger outputs that can exhaust RAM and push this driver into raw passthrough, so network colors diverge from the corrected LED outputs.🤖 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/drivers/NetworkSendDriver.h` around lines 121 - 154, The windowed send path in NetworkSendDriver still allocates corrected_ for the full sourceBuffer_->count() even though sendPacket now only uses the sink’s window slice. Update resizeCorrected() (and any related sizing logic used by the correction_ path) to size corrected_ for the active window length nLights rather than the full frame, using the same windowing helpers as sendPacket/windowSlice so each sender only reserves what it actually transmits. Keep the defensive checks in the send loop aligned with the new window-sized buffer so corrected_ count and stride still match outChannels.
🤖 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/plans/Plan-20260628` - S31 hardware ref + microphone.md:
- Around line 37-38: The plan document is out of sync with the implemented
hardware/microphone flow, so update the affected sections to reflect what
actually landed. In the history notes around the hardware pins and
initialization order, revise the references tied to audioCodecInit(),
audioMicInit(), and the microphone mclkPin so they match the PR’s
implementation, and make sure the reference-doc location is described accurately
rather than implying docs/history is the source of truth. If these sections are
meant to preserve the old behavior, explicitly label them as superseded
implementation deltas so future readers can tell what changed.
In `@docs/install/deviceModels.json`:
- Around line 917-927: The device catalog is marking S31 audio as supported too
early, which causes AudioModule to be exposed before the codec/GPIO52 MCLK
follow-up is complete. Update the relevant device entry in deviceModels.json so
the S31 board does not advertise "Audio" as supported yet and does not auto-add
AudioModule until the ES8311 path is actually working. Use the board-specific
support list and module mapping in this JSON to locate and adjust the S31
configuration consistently.
In `@docs/reference/esp32-s31-coreboard.md`:
- Around line 86-88: The S31 capability summary in the markdown is written as
future/absence narration, but it should describe the modules that exist now.
Update the paragraph in the esp32-s31-coreboard docs to present the currently
exposed projectMM/S31 capabilities, referencing the wired modules like
AudioModule and I2cScanModule alongside the existing LED and Wi‑Fi 6 support,
and avoid “not yet wired” wording. If relevant, mention the current
mic-validation status in present tense rather than listing missing or planned
features.
In `@src/core/AudioModule.h`:
- Around line 227-240: AudioModule is reaching directly into
platform::audioCodecType and platform::audioCodecPins, which leaks
board-specific wiring policy into core code. Update the AudioModule
initialization flow to use a single neutral platform API that encapsulates MCLK
selection and codec sequencing, and keep all codec/pin decisions inside the
platform layer instead of src/core/AudioModule.h. Preserve the existing init
path by routing the current audioMicInit/audioCodecInit behavior through that
platform seam.
In `@src/core/I2cScanModule.h`:
- Around line 19-20: The I2cScanModule header is exposing a platform dependency
through platform/platform.h and platform::i2cScan(), which breaks the core layer
boundary. Move the scan seam behind a core-neutral abstraction in
I2cScanModule/MoonModule so src/core/** no longer includes platform headers
directly; update the scan call site to depend on an injected interface or
forward-declared adapter instead of platform APIs.
- Around line 66-68: The I2C scan path in I2cScanModule::scan is creating a new
master bus via platform::i2cScan(), which conflicts with the ES8311-owned
controller on S31 when AudioModule is active. Update the scan logic to reuse the
existing active I2C bus handle instead of initializing I2C_NUM_0 again, or
propagate a distinct scan/setup error from platform::i2cScan() so the caller can
tell “bus in use” apart from “no devices found.”
In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 46-50: The `NetworkSendDriver::onBuildControls()` change replaces
the persisted `light_count` setting with `count`, which breaks existing saved
configs by loading `count` as 0 and changing send behavior. Add
backward-compatible migration in the `NetworkSendDriver` settings/load path so
old `light_count` values are read into `count` (or preserved as an alias) before
controls are built, and keep the `count` control synchronized with existing
devices.
In `@src/light/drivers/RmtLedDriver.h`:
- Around line 285-286: The windowed RMT path in RmtLedDriver still allocates
symbols for the full source buffer instead of the active window, causing
oversized allocations for small onboard slices. Update resizeSymbols() and the
related symbol sizing logic to derive the required symbol count from winStart_
and winLen_ (the clamped window) rather than sourceBuffer_->count(), and make
sure any callers that encode or reserve the buffer use the same windowed bounds.
Check the RmtLedDriver symbols resize/encode flow so the allocated symbols_
capacity matches only the lights this driver actually drives.
In `@src/platform/esp32/platform_esp32_es8311.cpp`:
- Around line 74-83: The I2C initialization in the ES8311 platform code is
creating a new master bus on I2C_NUM_0 instead of reusing the active bus, which
causes i2cScan() to fail and appear as “0 devices found.” Update the
platform_esp32_es8311 setup path so the codec shares the existing live I2C bus
(or otherwise skips reopening the port), and make sure bus initialization
failures are reported separately from an empty scan result in the same code path
around i2c_new_master_bus and i2cScan.
In `@src/platform/esp32/platform_esp32_i2c.cpp`:
- Around line 35-46: The I2C scan path in the ESP32 platform code is hard-wired
to I2C_NUM_0, which collides with the ES8311 control bus allocation from
audioCodecInit(). Update the bus setup in the I2C scan logic to avoid always
claiming the codec-owned controller, and instead use a shared-bus path or select
an available I2C controller dynamically. Make sure the scan code references the
existing bus configuration flow around i2c_new_master_bus and does not
unconditionally bind to the same port used by platform_esp32_es8311.cpp.
In `@src/platform/platform.h`:
- Around line 559-568: The public contract in platform.h is outdated: the
comment for audioCodecInit() still says it is called before audioMicInit(), but
AudioModule now initializes the mic first to ensure MCLK is active before codec
bring-up. Update the documentation near audioCodecInit() to describe the new
order accurately, and keep the rest of the behavior notes intact so callers and
implementers follow the sequence used by AudioModule.
---
Outside diff comments:
In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 121-154: The windowed send path in NetworkSendDriver still
allocates corrected_ for the full sourceBuffer_->count() even though sendPacket
now only uses the sink’s window slice. Update resizeCorrected() (and any related
sizing logic used by the correction_ path) to size corrected_ for the active
window length nLights rather than the full frame, using the same windowing
helpers as sendPacket/windowSlice so each sender only reserves what it actually
transmits. Keep the defensive checks in the send loop aligned with the new
window-sized buffer so corrected_ count and stride still match outChannels.
🪄 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: e1f36c49-5633-4e07-9456-9d0dd0d24466
📒 Files selected for processing (25)
docs/backlog/backlog-light.mddocs/history/plans/Plan-20260628 - S31 hardware ref + microphone.mddocs/install/deviceModels.jsondocs/moonmodules/core/I2cScanModule.mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/reference/esp32-s31-coreboard.mdesp32/main/CMakeLists.txtesp32/main/idf_component.ymlsrc/core/AudioModule.hsrc/core/Control.hsrc/core/I2cScanModule.hsrc/light/drivers/Drivers.hsrc/light/drivers/NetworkSendDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/RmtLedDriver.hsrc/main.cppsrc/platform/desktop/platform_config.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_config.hsrc/platform/esp32/platform_esp32_es8311.cppsrc/platform/esp32/platform_esp32_i2c.cppsrc/platform/esp32/platform_esp32_i2s.cppsrc/platform/platform.htest/unit/light/unit_RmtLedDriver_pins.cpp
💤 Files with no reviewable changes (1)
- docs/backlog/backlog-light.md
Comment on lines
+37
to
+38
| | ESP_I2C_SDA | GPIO50 | codec control bus | | ||
| | ESP_I2C_SCL | GPIO51 | codec control bus | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the plan to match what actually landed.
This history file still says the codec bus is GPIO50/GPIO51, that audioCodecInit() runs before audioMicInit(), that audioMicInit() has no mclkPin, and that the reference doc lives under docs/history/. The implementation in this PR went the other way, so future readers will not know which details were superseded unless these sections are synced or explicitly called out as implementation deltas.
Also applies to: 96-122
🤖 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/plans/Plan-20260628` - S31 hardware ref + microphone.md around
lines 37 - 38, The plan document is out of sync with the implemented
hardware/microphone flow, so update the affected sections to reflect what
actually landed. In the history notes around the hardware pins and
initialization order, revise the references tied to audioCodecInit(),
audioMicInit(), and the microphone mclkPin so they match the PR’s
implementation, and make sure the reference-doc location is described accurately
rather than implying docs/history is the source of truth. If these sections are
meant to preserve the old behavior, explicitly label them as superseded
implementation deltas so future readers can tell what changed.
Comment on lines
+227
to
+240
| const int16_t mclkPin = platform::audioCodecType == platform::CodecType::None | ||
| ? -1 : static_cast<int16_t>(platform::audioCodecPins.mclk); | ||
| inited_ = platform::audioMicInit(mic_, static_cast<uint16_t>(wsPin), | ||
| static_cast<uint16_t>(sdPin), | ||
| static_cast<uint16_t>(sckPin), sampleRate()); | ||
| static_cast<uint16_t>(sckPin), mclkPin, sampleRate()); | ||
| if (!inited_) { | ||
| setStatus(kInitFailMsg, Severity::Error); | ||
| return; | ||
| } | ||
| // Now configure the codec over I2C (MCLK is running). A no-op returning true | ||
| // on direct-mic boards, so the call is uniform. The codec then streams its ADC | ||
| // onto the I2S bus the read above drains. | ||
| if (!platform::audioCodecInit(platform::audioCodecType, platform::audioCodecPins, | ||
| sampleRate())) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep codec wiring details behind the platform seam.
AudioModule now reaches into platform::audioCodecType and platform::audioCodecPins to derive MCLK and own codec sequencing. That leaks per-target hardware policy into src/core/**; this layer should call one neutral platform entry point instead of reading board config directly. As per path instructions, "src/core/**: Core infrastructure ... Must be platform-independent — no platform includes."
🤖 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/AudioModule.h` around lines 227 - 240, AudioModule is reaching
directly into platform::audioCodecType and platform::audioCodecPins, which leaks
board-specific wiring policy into core code. Update the AudioModule
initialization flow to use a single neutral platform API that encapsulates MCLK
selection and codec sequencing, and keep all codec/pin decisions inside the
platform layer instead of src/core/AudioModule.h. Preserve the existing init
path by routing the current audioMicInit/audioCodecInit behavior through that
platform seam.
Source: Path instructions
Comment on lines
+19
to
+20
| #include "core/MoonModule.h" | ||
| #include "platform/platform.h" // i2cScan |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move the scan seam out of src/core.
This module includes platform/platform.h and calls platform::i2cScan() directly, which hard-couples the core layer to platform APIs. Please hide this behind a core-neutral boundary instead of exposing platform headers in src/core/**. As per path instructions, "src/core/**: Core infrastructure ... Must be platform-independent — no platform includes."
Also applies to: 66-68
🤖 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/I2cScanModule.h` around lines 19 - 20, The I2cScanModule header is
exposing a platform dependency through platform/platform.h and
platform::i2cScan(), which breaks the core layer boundary. Move the scan seam
behind a core-neutral abstraction in I2cScanModule/MoonModule so src/core/** no
longer includes platform headers directly; update the scan call site to depend
on an injected interface or forward-declared adapter instead of platform APIs.
Source: Path instructions
Comment on lines
46
to
+50
| void onBuildControls() override { | ||
| controls_.addSelect("protocol", protocol, kProtocolOptions, kProtocolCount); | ||
| controls_.addIPv4("ip", ip); | ||
| controls_.addUint16("universe_start", universeStart); | ||
| controls_.addUint16("light_count", lightCount); | ||
| addWindowControls(); // start / count — the slice of the shared buffer this sink sends |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve or migrate light_count on upgrade.
Replacing the persisted light_count control with count means existing configs now load count = 0, which expands the sender from “first N lights” to “entire buffer from start”. That is a silent behavior change for already-provisioned devices.
🤖 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/drivers/NetworkSendDriver.h` around lines 46 - 50, The
`NetworkSendDriver::onBuildControls()` change replaces the persisted
`light_count` setting with `count`, which breaks existing saved configs by
loading `count` as 0 and changing send behavior. Add backward-compatible
migration in the `NetworkSendDriver` settings/load path so old `light_count`
values are read into `count` (or preserved as an alias) before controls are
built, and keep the `count` control synchronized with existing devices.
Addresses CodeRabbit findings on the S31/driver-window work (window-sized buffers, an I2C bus-in-use signal, doc accuracy) and lands the no-code groundwork for the eventual projectMM→MoonLight rename plus a saved plan for the DevicesModule UDP-presence refactor. Code changes are small and verified; the bulk is docs and one transition script. tick:62us(FPS:16129) [PC] | ESP32:1268KB | S31/S3/classic all build clean Core - I2cScanModule: distinguish a bus that's in use (e.g. the ES8311 codec holds I2C_NUM_0 while AudioModule is active) from an empty scan — surfaces "bus in use" instead of a misleading "0 devices found". - platform.h / platform_esp32_i2c.cpp: add the kI2cBusUnavailable sentinel so i2cScan reports a bus-open failure separately from a 0-device result; fix the audioCodecInit doc comment to say it runs AFTER audioMicInit (MCLK must be live first). Light domain - RmtLedDriver: size the symbol buffer for the driver's window slice, not the whole source buffer — a window driver (onboard LED, 1 light) no longer over-allocates the full grid's symbols. - NetworkSendDriver: size the corrected_ buffer for the window length too, matching what sendPacket transmits. Tests - (no test changes; existing window + lifecycle suites cover the resized-buffer paths, all 513 cases green) Docs / CI - deviceModels.json: S31 Audio moved supported→planned and the auto-added AudioModule removed (the codec path is implemented but not yet mic-validated — don't advertise it as working); esp32-s31-coreboard.md rewritten present-tense (names the wired LEDs/WiFi/I2cScanModule, no "not yet wired"). - rename-to-moonlight.md (new): phased projectMM→MoonLight plan — predecessor links repointed to the ewowi/MoonLight fork, naming decisions recorded (keep MM-/projectMM until the switch), a MoSCoW of feature gaps (live scripting is NOT a gap — MoonLive overrules it), single-owner repo move, and scripts/rename/rename_to_moonlight.py (a dry-run-by-default sweep, 542 hits verified, MoonLive/predecessor-MoonLight provably untouched). - ~24 prior-art permalinks (architecture, module specs, README lineage) repointed MoonModules/MoonLight → ewowi/MoonLight; backlog-core gains an "offer as a library" seed; decisions.md notes the rename-vs-library reasoning. - Plan-20260629 (new): DevicesModule UDP-presence refactor — replace the slow blocking HTTP sweep + mDNS browse with a UDP presence beacon (lossy-OK discovery), REST for must-arrive commands, and a WLED-compat patch-in; appear in WLED devices (UDP) and the native WLED iOS/Android/Desktop apps (one _wled._tcp mDNS advertise). Reviews - 🐇 RmtLedDriver/NetworkSendDriver oversized buffers — fixed (size to the window, not the full frame). - 🐇 i2cScan vs ES8311 bus conflict — fixed via the kI2cBusUnavailable signal (the scan opening its own transient bus is correct; the fix is reporting "in use" distinctly). - 🐇 deviceModels S31 Audio advertised too early — fixed (moved to planned, module not auto-added). - 🐇 esp32-s31-coreboard absence-narration / platform.h stale ordering comment — fixed (present-tense; mic-first order documented). - 🐇 AudioModule/I2cScanModule "platform boundary leak" — accepted as-is: check_platform_boundary passes and all core modules use the neutral platform:: seam by design (not a violation). - 🐇 NetworkSendDriver light_count migration alias — declined: intentional pre-1.0 break, absent key safely defaults count to 0 (whole buffer); an alias is the kept-for-compat friction the standards forbid. - 🐇 Plan-20260628 "sync to implementation" — declined: saved plans are the design-intent record, deliberately not kept in sync with the code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/platform/esp32/platform_esp32_i2c.cpp (1)
66-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
kI2cBusUnavailablehere.platform.handI2cScanModuleboth use that sentinel to mean “no bus,” so0makes an unavailable controller look like an empty scan.🤖 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_i2c.cpp` around lines 66 - 74, The inert ESP32 I2C stub in i2cScan currently returns 0, which is indistinguishable from an empty device scan. Update mm::platform::i2cScan in platform_esp32_i2c.cpp to return the kI2cBusUnavailable sentinel instead so platform.h and I2cScanModule can correctly detect that no I2C bus is available.docs/moonmodules/light/Drivers.md (1)
34-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNarrow the contract to window-aware output drivers.
“Every driver child” overstates the exposed controls; the supplied scope covers LED drivers and
NetworkSendDriver. Use narrower wording so integrators do not expectstart/counton every DriverBase child. As per path instructions, “Module specs indocs/moonmodules/*.mdmust be end-user/API-integrator documentation that explains contracts”.Proposed wording
-Every driver reads the **same** shared source buffer and outputs a contiguous slice of it — its *window*. Two controls on `DriverBase`, shared by every driver child (the LED drivers, the network sink): +Window-aware output drivers read the **same** shared source buffer and output a contiguous slice of it — their *window*. LED drivers and `NetworkSendDriver` expose two shared `DriverBase` controls:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/moonmodules/light/Drivers.md` around lines 34 - 43, The wording in this section overstates the contract by implying every DriverBase child exposes the window controls; tighten the language in Drivers.md around DriverBase, `start`, and `count` so it explicitly applies only to window-aware output drivers such as the LED drivers and `NetworkSendDriver`, and avoid phrasing that suggests all subclasses must support these controls. Keep the rest of the explanation about slice semantics and order-independent distribution, but make the scope of the contract unambiguous for integrators.Source: Path instructions
src/light/drivers/NetworkSendDriver.h (1)
46-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrigger rebuilds when
startorcountchanges.
resizeCorrected()only runs from setters/build hooks, but the loop immediately uses the new window. Ifcountgrows past the currentcorrected_allocation, the guard falls back to passthrough and silently drops brightness/order correction until another rebuild.Proposed fix
void onBuildControls() override { controls_.addSelect("protocol", protocol, kProtocolOptions, kProtocolCount); controls_.addIPv4("ip", ip); controls_.addUint16("universe_start", universeStart); addWindowControls(); // start / count — the slice of the shared buffer this sink sends controls_.addUint8("fps", fps, 1, 120); } + + bool controlChangeTriggersBuildState(const char* name) const override { + return isWindowControl(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/light/drivers/NetworkSendDriver.h` around lines 46 - 51, `NetworkSendDriver::onBuildControls` exposes `start`/`count` via `addWindowControls`, but changes to that window do not force a rebuild of the corrected buffer path. Update the `start`/`count` handling in `NetworkSendDriver` (and any related setters/build hooks such as `resizeCorrected()`) so that changing either value triggers the same reallocation/reinitialization as other configuration changes, ensuring the driver does not silently fall back to passthrough when the window exceeds `corrected_`.docs/install/deviceModels.json (1)
553-562: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSplit the S3 strip window away from the onboard LED
RmtLedstill defaults tostart=0,count=0, so it also drives light 0 and overlapsOnboardLed’s[0,1)slice. Set the strip driver to start after the onboard pixel if that LED is meant to stay separate.🤖 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/install/deviceModels.json` around lines 553 - 562, The RmtLedDriver configuration for the S3 strip and the OnboardLed currently overlaps because RmtLed still starts at 0 with count 0 semantics, so both can address pixel 0. Update the deviceModels.json entries for the RmtLedDriver and OnboardLed definitions so the strip window begins after the onboard LED’s single-pixel slice, keeping OnboardLed’s [0,1) range separate from the strip controls.
♻️ Duplicate comments (2)
docs/reference/esp32-s31-coreboard.md (1)
86-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove remaining forward-looking and absence-narration phrasing.
The rewrite improved by describing what exists now (I2cScanModule wired, codec reachable), but "so Audio sits in the entry's planned list until that lands" and "The remaining board capabilities … are in
plannedtoo" still use future-looking and absence-narration phrasing. As per coding guidelines, describe what exists now rather than what will happen later or what is missing.Suggested rewrite:
-The S31 catalog entry drives **LEDs** (RMT on GPIO60) and **Wi-Fi 6**, and wires an -[I2cScanModule](../moonmodules/core/I2cScanModule.md) on the codec bus (SDA 51 / SCL 50) for -I2C bring-up. The ES8311 audio path is implemented (codec seam + `AudioModule`); the codec is -reachable on I2C, and validating the mic end-to-end depends on confirming MCLK at GPIO52 — so -**Audio** sits in the entry's `planned` list until that lands. The remaining board capabilities -(Ethernet, Bluetooth, SD, USB host, …) are in `planned` too; see the S31 entry in -`docs/install/deviceModels.json`. +The S31 catalog entry drives **LEDs** (RMT on GPIO60), **Wi-Fi 6**, and an +[I2cScanModule](../moonmodules/core/I2cScanModule.md) on the codec bus (SDA 51 / SCL 50). The +ES8311 codec seam and `AudioModule` are wired; the codec responds on I2C, but the mic path +remains inactive pending validation of MCLK at GPIO52. Ethernet, Bluetooth, SD, and USB host are +not yet enabled; see the S31 entry in `docs/install/deviceModels.json`.🤖 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/reference/esp32-s31-coreboard.md` around lines 86 - 92, The S31 catalog description still uses future-looking and absence-focused wording in the audio and capability summary. Update the narrative in the ESP32 S31 coreboard docs to describe the current supported state using the existing symbols and concepts already mentioned (I2cScanModule, AudioModule, ES8311, codec bus, deviceModels entry) instead of saying Audio is “planned” or listing remaining capabilities as absent; rewrite those sentences to state what the board currently includes and how it is wired.Source: Coding guidelines
src/light/drivers/RmtLedDriver.h (1)
360-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSize the symbol buffer to the encoded light count.
resizeSymbols()still allocates for the full window, whileloop()only encodesmin(txLightCount_, winLen_). A default/full window withledsPerPinset to a smaller strand can still reserve symbols for the whole grid.Proposed fix
- nrOfLightsType winStart, n; - windowSlice(sourceBuffer_->count(), winStart, n); + nrOfLightsType winStart, winLen; + windowSlice(sourceBuffer_->count(), winStart, winLen); + const nrOfLightsType n = txLightCount_ < winLen ? txLightCount_ : winLen;🤖 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/drivers/RmtLedDriver.h` around lines 360 - 365, `resizeSymbols()` is still sizing the symbol buffer from the full window instead of the actual encoded light count used by `loop()`. Update the allocation logic in `resizeSymbols()` (and any related sizing helper) to use the same `min(txLightCount_, winLen_)` / encoded-light count that `loop()` encodes, so the buffer reflects the strand size rather than the whole grid; use the existing `windowSlice()` and `txLightCount_` / `winLen_` symbols to locate the fix.
🤖 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.
Outside diff comments:
In `@docs/install/deviceModels.json`:
- Around line 553-562: The RmtLedDriver configuration for the S3 strip and the
OnboardLed currently overlaps because RmtLed still starts at 0 with count 0
semantics, so both can address pixel 0. Update the deviceModels.json entries for
the RmtLedDriver and OnboardLed definitions so the strip window begins after the
onboard LED’s single-pixel slice, keeping OnboardLed’s [0,1) range separate from
the strip controls.
In `@docs/moonmodules/light/Drivers.md`:
- Around line 34-43: The wording in this section overstates the contract by
implying every DriverBase child exposes the window controls; tighten the
language in Drivers.md around DriverBase, `start`, and `count` so it explicitly
applies only to window-aware output drivers such as the LED drivers and
`NetworkSendDriver`, and avoid phrasing that suggests all subclasses must
support these controls. Keep the rest of the explanation about slice semantics
and order-independent distribution, but make the scope of the contract
unambiguous for integrators.
In `@src/light/drivers/NetworkSendDriver.h`:
- Around line 46-51: `NetworkSendDriver::onBuildControls` exposes
`start`/`count` via `addWindowControls`, but changes to that window do not force
a rebuild of the corrected buffer path. Update the `start`/`count` handling in
`NetworkSendDriver` (and any related setters/build hooks such as
`resizeCorrected()`) so that changing either value triggers the same
reallocation/reinitialization as other configuration changes, ensuring the
driver does not silently fall back to passthrough when the window exceeds
`corrected_`.
In `@src/platform/esp32/platform_esp32_i2c.cpp`:
- Around line 66-74: The inert ESP32 I2C stub in i2cScan currently returns 0,
which is indistinguishable from an empty device scan. Update
mm::platform::i2cScan in platform_esp32_i2c.cpp to return the kI2cBusUnavailable
sentinel instead so platform.h and I2cScanModule can correctly detect that no
I2C bus is available.
---
Duplicate comments:
In `@docs/reference/esp32-s31-coreboard.md`:
- Around line 86-92: The S31 catalog description still uses future-looking and
absence-focused wording in the audio and capability summary. Update the
narrative in the ESP32 S31 coreboard docs to describe the current supported
state using the existing symbols and concepts already mentioned (I2cScanModule,
AudioModule, ES8311, codec bus, deviceModels entry) instead of saying Audio is
“planned” or listing remaining capabilities as absent; rewrite those sentences
to state what the board currently includes and how it is wired.
In `@src/light/drivers/RmtLedDriver.h`:
- Around line 360-365: `resizeSymbols()` is still sizing the symbol buffer from
the full window instead of the actual encoded light count used by `loop()`.
Update the allocation logic in `resizeSymbols()` (and any related sizing helper)
to use the same `min(txLightCount_, winLen_)` / encoded-light count that
`loop()` encodes, so the buffer reflects the strand size rather than the whole
grid; use the existing `windowSlice()` and `txLightCount_` / `winLen_` symbols
to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3534bfb2-d180-4e10-9b73-4271ad0e6754
📒 Files selected for processing (34)
README.mddocs/architecture.mddocs/backlog/README.mddocs/backlog/backlog-core.mddocs/backlog/rename-to-moonlight.mddocs/history/decisions.mddocs/history/plans/Plan-20260629 - DevicesModule UDP-presence refactor.mddocs/install/deviceModels.jsondocs/moonmodules/core/Control.mddocs/moonmodules/core/MoonModule.mddocs/moonmodules/light/Buffer.mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/EffectBase.mddocs/moonmodules/light/Layer.mddocs/moonmodules/light/Layers.mddocs/moonmodules/light/LightConfig.mddocs/moonmodules/light/MappingLUT.mddocs/moonmodules/light/ModifierBase.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/effects/GameOfLifeEffect.mddocs/moonmodules/light/effects/NetworkReceiveEffect.mddocs/moonmodules/light/effects/NoiseEffect.mddocs/moonmodules/light/effects/RipplesEffect.mddocs/moonmodules/light/layouts/GridLayout.mddocs/moonmodules/light/layouts/SphereLayout.mddocs/moonmodules/light/modifiers/CheckerboardModifier.mddocs/moonmodules/light/modifiers/MultiplyModifier.mddocs/reference/esp32-s31-coreboard.mdscripts/rename/rename_to_moonlight.pysrc/core/I2cScanModule.hsrc/light/drivers/NetworkSendDriver.hsrc/light/drivers/RmtLedDriver.hsrc/platform/esp32/platform_esp32_i2c.cppsrc/platform/platform.h
Replaces DevicesModule's slow blocking HTTP subnet sweep with the standard mDNS-SD pattern — devices announce, we passively listen — behind a DevicePlugin adapter seam so foreign ecosystems hook in as one file each. Ships projectMM + WLED plugins, a WLED-compatible /json/info shim so projectMM appears in the native WLED apps, and net removes ~177 lines. WLED discovery works on hardware; projectMM-peer discovery works but a board's own mDNS advertisement doesn't yet propagate reliably to peers (documented as a focused follow-up, not half-patched here).
tick:71us(FPS:14084) [classic ESP32] | ESP32:928KB | PC tick:62us
Core
- DevicePlugin.h (new): the interop seam — a plugin claims an mDNS service type and classifies a hit into a device kind. projectMM (_http._tcp + mm=1) and WLED (_wled._tcp) plugins; the command half + hub-shaped plugins (Hue) are reserved extensions.
- DevicesModule: rewritten around a non-blocking mDNS listener feeding the plugins on a slow round-robin cadence; keeps the Device list / ListSource / persistence / age-out / self-row. Drops the blocking sweep, the blocking mDNS browse, the scan button + progress control, the via/speaks bitmasks, and the HTTP-body classifier. A peer rename propagates live (name comes from the mDNS announcement) with a type/name authority rule so a _wled hit can't downgrade or rename a projectMM device. A test seam (injectMdnsHitForTest) drives the full classify→upsert path on the host.
- DeviceIdentify.h: shrunk to the DevType enum + label (the HTTP classifier moved into the plugins).
- platform mdnsListenPoll: a bounded mDNS query for the listener, driven on a slow cadence — querying every tick exhausted the IDF mDNS task's fixed pool ("Cannot allocate memory" with megabytes free) and destabilised the network; throttled. Also advertises _wled._tcp so projectMM shows in the WLED app ecosystem + Home Assistant.
- HttpServerModule: serve a minimal WLED-compatible /json/info (brand:projectMM, real name/ip/mac) so the WLED apps validate + list a projectMM device — the interop cross-check for the _wled._tcp advertise.
Light domain
- NetworkSendDriver: a start/count window change now triggers the onBuildState sweep so corrected_ re-sizes for the new slice (was silently falling back to passthrough). 🐇
- platform_esp32_i2c: the no-I2C stub returns kI2cBusUnavailable, not 0, so an I2C-less target is distinguishable from an empty scan. 🐇
Tests
- unit_DevicesModule_discovery (new): the full mDNS pipeline on the host — classification, projectMM>WLED priority, no name/IP cross-contamination, peer rename, no-clobber-by-lower-authority.
- unit_DeviceIdentify: repurposed to test the plugins' classify(); unit_DevicesModule_ageout: updated for the new kStaleMs window.
- Note: the first ctest invocation reported a transient failure that did not reproduce — the known FilesystemModule .config write/test-isolation artifact (decisions.md); the suite passes 509/509, 8279 assertions, verified twice.
Docs / CI
- Plan-20260629 (renamed from the UDP-presence draft): the reasoned mDNS-discovery + plugin design — three-transport split (mDNS discover / REST config / UDP sync), the multi-ecosystem hub positioning (WLED/ESPHome/Hue, incl. the S31 Thread/Zigbee radio angle), latency analysis, and live-peer-state-via-REST as a future.
- mdns-advertise-instability.md (new, indexed): the open bench finding — symptom (one board advertises solid, three don't; WLED solid), what's ruled out (network, chip-limitation, query side, UI all eliminated with evidence), leading hypotheses (self-query-vs-self-advertise interference; missing re-announce), next steps, plus the WLED-friendly-name-needs-REST and projectMM-shown-as-WLED symptoms.
- DevicesModule.md / Drivers.md / esp32-s31-coreboard.md / backlog-core.md / decisions.md: discovery rewrite, window-control scope tightened to opt-in output drivers, S31 audio present-tense, backlog stance replaced, the mDNS-rate-limit + read-serial-first lessons recorded.
Reviews
- 🐇 NetworkSendDriver window change didn't trigger a rebuild — fixed (controlChangeTriggersBuildState on the window controls).
- 🐇 i2cScan no-I2C stub returned 0 — fixed (returns kI2cBusUnavailable).
- 🐇 Drivers.md overstated the window contract — fixed (window controls are opt-in, not every DriverBase subclass).
- 🐇 esp32-s31-coreboard absence-narration — fixed (present-tense about what's wired).
- 🐇 S3 strip/OnboardLed window overlap — accepted as-is: the overlap is the deliberate "onboard mirrors grid pixel 0" design the product owner chose.
- 🐇 resizeSymbols sizing — declined: it already sizes from the window slice (the safe upper bound); the suggested min(txLightCount_, winLen_) would under-allocate on a live pin reconfigure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai, can you thoroughly review commit 2cf2e58 As you can find in the changed code, I was not successful in getting mdns broadcasting on esp-idf 6.1 pre release working fine so try to find the fix for that. deliver your review in a copy pastable prompt for agents |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="2cf2e585ef22fc644975875466d7f6052702a899"
echo "=== Commit metadata ==="
git show --stat --summary --oneline "$commit"
echo
echo "=== Changed files in commit ==="
git diff-tree --no-commit-id --name-only -r "$commit"
echo
echo "=== Search repo for mdns references ==="
rg -n -C 3 --iglob '*.{c,cc,cpp,cxx,h,hpp,md,txt,yml,yaml,json}' '\bmdns\b|_http\._tcp|_wled\._tcp|nsd|bonjour|zeroconf' .
echo
echo "=== Candidate files by name ==="
fd -i 'mdns|device|network|presence|udp' .
echo
echo "=== Commit diff filtered for likely networking/discovery files ==="
git show --unified=80 --format=medium "$commit" -- \
'src/**' 'docs/**' 'esp32/**' ':!docs/history/**' | \
sed -n '/mdns\|_http\._tcp\|_wled\._tcp\|DevicesModule\|UDP\|presence\|discover\|browse/I,+120p'
echo
echo "=== AST outlines for likely files if present ==="
for f in $(fd -i 'DevicesModule|platform_esp32.*mdns|mdns|network' src esp32 docs || true); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
doneLength of output: 172053 🤖 web_search executed:
Length of output: 17972 🤖 web_search executed:
Length of output: 17971 🤖 web_search executed:
Length of output: 9395 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Find mdns lifecycle call sites ==="
rg -n -C 4 '\bmdnsInit\s*\(|\bmdnsStop\s*\(|\bmdnsShutdown\s*\(|\bmdnsListenPoll\s*\(' src esp32
echo
echo "=== Outline NetworkModule ==="
ast-grep outline src/core/NetworkModule.h --view expanded || true
echo
echo "=== NetworkModule relevant slice ==="
sed -n '1,260p' src/core/NetworkModule.h
echo
echo "=== ESP32 platform mDNS slice ==="
fd -i 'platform_esp32*.cpp' src/platform/esp32 esp32 | while read -r f; do
if rg -n '\bmdnsInit\s*\(|\bmdnsStop\s*\(|\bmdnsShutdown\s*\(|\bmdnsBrowse\s*\(|\bmdnsListenPoll\s*\(' "$f" >/dev/null; then
echo "--- $f ---"
rg -n -C 20 '\bmdnsInit\s*\(|\bmdnsStop\s*\(|\bmdnsShutdown\s*\(|\bmdnsBrowse\s*\(|\bmdnsListenPoll\s*\(' "$f"
fi
done
echo
echo "=== mdns dependency / version references ==="
rg -n -C 3 'espressif/mdns|mdns:' esp32 src .github docs --glob '!docs/assets/**'Length of output: 41401
Given Also, your current re-init path only does Here’s a copy-pastable prompt for agents: My ranking of likely fixes is:
If you want, I can next turn this into a shorter implementation-only agent prompt or a review checklist prompt. |
projectMM devices now discover each other (and WLED devices) over passive UDP 65506 broadcast instead of querying mDNS, and a projectMM device shows up in — and is controllable from — the native WLED apps + Home Assistant. mDNS shrinks to advertise-only, which makes the self-query-disturbs-advertise instability structurally impossible. Bench-verified on all 4 boards: each sees all 4 projectMM peers + both WLEDs and appears in WLED's own instances list and the native WLED app.
ESP32 tick:774us(FPS:1291) [S31 live]. (PC KPI half blocked by a stale build/macos decoy binary that hangs the collector — ESP32 captured live instead.)
Core
- DevicesModule: discovery is now passive UDP presence — bind one UdpSocket on the plugins' discovery port, drain it non-blocking each tick through the plugins, broadcast our own WLED-valid presence packet every ~10s. Dropped the mDNS-query path entirely. A cached (restored-from-persistence, never-re-heard) device gets a short probation window instead of the full 24h, so a long-gone device can't survive forever across reboots (its persisted clock is "boot", not real last-seen); a live packet promotes it to the 24h window.
- WledPacket.h (new): build/parse the 44-byte WLED-compatible presence packet (token=255,id=1, ip, name, board type) with a projectMM marker stamped into the version field (a region no WLED validator reads) so a peer types us projectMM while a WLED app still lists us. Wire format reverse-engineered from a live WLED + MoonLight's ModuleDevices.h, written fresh.
- DevicePlugin: reshaped the seam from mDNS-shaped (service()/classify(MdnsHost)) to transport-agnostic (discoveryPort()/classifyPacket(data,len,srcIp)); MmPlugin claims a marked packet, WledPlugin claims an unmarked valid one. discoveryPort() is load-bearing — ensureListener derives the bind port from the plugins, not a constant.
- HttpServerModule: WLED-compatibility shim — serve /json/info (the minimal field set the WLED-Android app's Moshi model requires: name + leds{} + wifi{} + a real mac), /json/state and /json/si, and push a WLED {state,info} frame over the existing /ws alongside our own UI frame (the app reads live state over the WebSocket, not an HTTP GET). Inbound /ws frames are read + applied (the app's slider/toggle send {on,bri} over /ws) so brightness/on-off control is bidirectional. The card colour is the live first-LED RGB, falling back to projectMM purple when black.
- MoonModule::firstLedRgb: domain-neutral seam (core declares returning false; light's Drivers overrides to read pixel 0 of the driven buffer) so the shim tints the WLED card with the live colour without a light-domain include.
- platform: removed the mdnsListenPoll/mdnsBrowse/MdnsHost query seam (mDNS is advertise-only now); mdnsInit keeps announcing _http._tcp+mm=1 and _wled._tcp+mac= so foreign apps find us. Fixed a UdpSocket fd-leak in DevicesModule's ensureListener (close on bind-fail, else a retry leaks one fd per second).
Light domain
- Drivers::firstLedRgb override — reads pixel 0 of the composited output buffer (or the single enabled layer's buffer on the zero-copy fast path); RGB in the buffer's logical channel order.
UI
- app.js: the WS onmessage handler ignores frames without a `modules` array, so the WLED {state,info} frame sharing /ws no longer clobbers the module view (it blanked until refresh). A device-detail `url` value renders as a clickable link opening in a new tab (generic for any ListSource URL).
Tests
- unit_WledPacket (new): the 44-byte wire format — build/parse round-trip, name truncation, marker presence + WLED-validity, short/garbage/null rejection.
- unit_Drivers_firstLedRgb (new): pixel-0 read, black-as-is, no-buffer → false, the MoonModule default → false.
- unit_DevicesModule_discovery + unit_DeviceIdentify + unit_DevicesModule_ageout: rewritten for classifyPacket (synthetic datagrams, marker priority, contamination, rename) + cached-probation vs live-24h age-out. Registered unit_DevicesModule_discovery in CMake (it was never built — the discovery-pipeline tests had been silently dead).
Docs / CI
- DevicesModule.md / HttpServerModule.md: UDP discovery + the WLED shim wire contract, with three bench screenshots (device card, WLED instances list, native WLED app). decisions.md: the discovery-transport lesson rewritten (UDP over mDNS, transport-per-plugin, advertise-only). README Credits section: softhack007 (Frank), troyhacks, hpwit, Moustachauve, Improv. Removed the resolved mdns-advertise-instability investigation; UDP plan marked (shipped), the mDNS plan (superseded). Backlog pruned of shipped discovery items + the resolved mDNS-toggle evaluation.
Reviews
- 🐇 (earlier round, carried) WLED /json/info, NetworkSend window rebuild, i2cScan sentinel, Drivers.md scope, S31 doc present-tense — all landed before this work.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/platform.h (1)
168-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the mDNS comments match the advertise-only contract.
Line 168 still says browse queries work, but the browse seam was removed; lines 173-177 also use absence-narration phrasing. Reword this as the current advertise + UDP-presence contract.
As per coding guidelines, “Do not remove comments unless they are outdated or factually wrong” and “Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later.”
Proposed wording
-// Stop advertising (clears the hostname) but keep the mDNS stack up so browse -// queries still work. mdnsShutdown() does the full mdns_free — call at teardown. +// Stop advertising and clear the hostname while keeping the mDNS stack allocated. +// mdnsShutdown() does the full mdns_free — call at teardown. void mdnsStop(); void mdnsShutdown(); -// mDNS is ADVERTISE-ONLY (see mdnsInit below): a projectMM device announces `_http._tcp` -// + `_wled._tcp` so the WLED native app / Home Assistant discover it. It does NOT query -// mDNS to find peers — that's UDP presence discovery now (DevicesModule + WledPacket), -// which a PTR query for a service we also host would destabilise (it exhausts the IDF -// mDNS pool — see docs/history/decisions.md). No browse/query seam here. +// mDNS advertises this device: mdnsInit publishes `_http._tcp` + `_wled._tcp` +// so the WLED native app / Home Assistant discover it. Peer discovery uses UDP +// presence (DevicesModule + WledPacket), avoiding PTR queries for services this +// firmware also hosts; those exhaust the IDF mDNS pool (see 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 `@src/platform/platform.h` around lines 168 - 177, Update the mDNS comments in platform.h to match the current advertise-only behavior: remove the mention that browse queries still work after mdnsStop() and rewrite the surrounding block to describe the active contract of mdnsInit(), mdnsStop(), and mdnsShutdown() in present-tense terms. Keep the explanation focused on what mdnsStop() and mdnsShutdown() do now, and rephrase the mdnsInit comment to state that projectMM advertises `_http._tcp` and `_wled._tcp` while DevicesModule and WledPacket handle UDP presence discovery, avoiding absence-narration and references to removed browse/query behavior.Source: Coding guidelines
🧹 Nitpick comments (5)
docs/history/plans/Plan-20260629 - UDP device discovery + mDNS advertise-only (shipped).md (2)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRephrase absence-narration.
Line 33 ("no core edit") and line 35 ("with no network") describe capabilities by negation. Rephrase positively, e.g., "Adding a new system requires only a new plugin file" and "host-unit-tested without requiring a network stack." As per coding guidelines, avoid absence-narration phrasing in module specs.
🤖 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/plans/Plan-20260629` - UDP device discovery + mDNS advertise-only (shipped).md around lines 33 - 35, Rephrase the spec text to state capabilities positively instead of describing absences. Update the planning notes around the new UDP-discovery seam and the host-unit-testing guidance so they say what is supported (for example, adding a new system via a plugin file and unit-testing the host without a network stack), and remove the negated wording currently used in the plan document.Source: Coding guidelines
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRephrase absence-narration in persistence section.
Line 63 ("The self entry is not restored from the cache") describes behavior by negation. Rephrase positively: "The self entry is rebuilt live each boot (
upsertSelf) because its IP may have changed; only peer devices restore from cache." As per coding guidelines, avoid absence-narration phrasing in module specs.🤖 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/plans/Plan-20260629` - UDP device discovery + mDNS advertise-only (shipped).md around lines 59 - 63, Reword the persistence spec in the DevicesModule section to describe the self entry positively instead of by negation. Update the sentence around upsertSelf so it says the self entry is rebuilt live on each boot because its IP may change, while peer devices are restored from cache. Keep the wording aligned with DevicesModule, upsertSelf, and the cache restore flow.Source: Coding guidelines
src/platform/esp32/platform_esp32.cpp (1)
1001-1005: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite the comments around the advertise-only contract.
Line 1078 still says keeping the stack up preserves “browse”, but this layer now uses UDP presence. The new comments also lean on “does NOT / no browse” absence narration; describe the current contract instead.
Suggested comment-only cleanup
-// Advertising is gated by the user's mDNS toggle; the stack init is NOT — mdnsStop() -// removes the services + hostname but keeps the stack up, so toggling mDNS back on -// re-advertises without a full re-init. mdns_init is safe to call when already running -// (returns an already-init error we treat as fine). Discovery does NOT use mDNS (it's UDP -// presence — see DevicesModule); mDNS here is advertise-only. +// The user's mDNS toggle gates service publication. mdnsStop() removes services + +// hostname while keeping the responder initialized, so toggling mDNS back on can +// publish fresh records quickly. Peer discovery uses UDP presence (DevicesModule); +// mDNS publishes local service records for app/integration discovery. ... - // Tearing the stack down would also kill browse. The toggle-off path wants to stop - // ADVERTISING, not lose discovery — so keep the stack but drop BOTH advertised + // Toggle-off stops ADVERTISING while keeping the initialized responder available + // for fast re-advertise, so drop BOTH advertised ... -// mDNS is advertise-only (mdnsInit). There is no browse/query here: discovery uses UDP -// presence (DevicesModule + WledPacket), because a PTR query for a service this device -// also hosts destabilises our own advertise — see docs/history/decisions.md. +// Peer discovery uses UDP presence (DevicesModule + WledPacket). Keeping mDNS scoped +// to publication avoids PTR queries against a service this device also hosts; see +// docs/history/decisions.md.As per coding guidelines,
**/*.{md,cpp,cxx,cc,h,hpp,hh}should “Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later,” and comments should not be removed unless outdated or factually wrong.Also applies to: 1078-1083, 1101-1103
🤖 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 1001 - 1005, Rewrite the comments in the ESP32 platform code around the advertise-only mDNS contract so they describe the current behavior directly: the mDNS stack stays initialized while advertising is toggled, and discovery/presence is handled by UDP in DevicesModule. Update the relevant comments near the mDNS init/stop logic and the section that mentions “browse” so they no longer rely on absence-narration (“does NOT”, “no browse”, “removed”, “will”) and instead clearly state what the code currently does in the functions handling mDNS advertisement.Source: Coding guidelines
src/core/DevicePlugin.h (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the future-state narration from this header.
These blocks describe systems and APIs that do not exist yet (
ESPHome / Tasmota / Hue, reservedcommand()support) instead of the interface that exists today. In a public header, that makes the current discovery contract harder to scan. As per coding guidelines,**/*.{md,cpp,cxx,cc,h,hpp,hh}: Avoid forward-looking or absence-narration phrasing; describe what exists now rather than what was removed or what will happen later, except in backlog, history, and decision-contrast contexts.Also applies to: 67-69
🤖 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/DevicePlugin.h` around lines 23 - 27, The comment block in DevicePlugin should be rewritten to describe only the current interface and behavior, removing future-state narration about planned command support and systems that do not exist yet. Update the documentation near the DevicePlugin declaration to focus on the present discovery contract and the actual concrete plugins, and avoid mentioning reserved `command()` extensions or “later” API changes. Keep the header public-facing and scan-friendly by using current-state wording only, aligned with the existing DevicePlugin and DevicesModule design.Source: Coding guidelines
src/core/MoonModule.h (1)
144-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a domain-neutral name for this core hook.
MoonModuleis the cross-domain boundary, sofirstLedRgbleaks light-specific terminology into the shared API. A neutral name such asfirstOutputRgbkeeps the same contract without baking the light domain into core. As per coding guidelines,**/*.{h,hpp,hh,cpp,cxx,cc,md}: Use domain-neutral naming when core and light concerns must mix, so the code can be separated later if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/MoonModule.h` around lines 144 - 148, Rename the core hook in MoonModule from firstLedRgb to a domain-neutral name such as firstOutputRgb, and update its declaration and any overrides/call sites accordingly. Keep the same contract and behavior in the shared API, but avoid light-specific terminology in the MoonModule boundary so the cross-domain interface stays neutral.Source: Coding guidelines
🤖 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/plans/Plan-20260629` - UDP device discovery + mDNS
advertise-only (shipped).md:
- Around line 15-23: Rephrase the discovery-plan text in the plan document to
describe the current behavior positively instead of listing removed or forbidden
paths. Update the wording around the UDP/mDNS transport split so it states what
DevicesModule and the platform do now, using the existing concepts like passive
UDP receive, bound UdpSocket, loop1s/loop20ms draining, and mDNS advertise-only,
without phrases like “no”, “never”, or “removed.”
- Line 72: The history note in the plan document inaccurately claims “symmetric
stop” was shipped; update the entry to reflect that mdnsStop and mdnsInit are
asymmetric, with mdnsStop only removing _http._tcp while mdnsInit advertises
both _http._tcp and _wled._tcp. Correct or prune the sentence around the
mdnsInit/mdnsStop note so it no longer states the bug was shipped, and make the
wording match the reviewed behavior of the mDNS lifecycle.
- Around line 71-75: The prior-art note in the plan is outdated and describes
the wrong discovery model. Update the DevicesModule summary to say it uses
MoonLight-style UDP-presence discovery (broadcast + listen on 65506) with mDNS
kept advertise-only for cross-ecosystem visibility, and remove any wording that
implies standard mDNS announce/listen or a bespoke UDP beacon mismatch. Use the
DevicesModule and UDP-presence references in this section to keep the
description aligned with the shipped architecture.
In `@src/core/DeviceIdentify.h`:
- Around line 5-10: Update the header comment in DeviceIdentify.h so it matches
the current discovery contract in DevicePlugin.h: replace the old mDNS
service/TXT-record description with the passive UDP presence-packet
classification flow. Keep the note that this header only defines the shared
DevType vocabulary used by DevicesModule, persistence, UI, and interop plugins,
but remove any wording that implies HTTP probing or mDNS-based typing.
In `@src/core/DevicePlugin.h`:
- Around line 56-58: Update the `discoveryPort()` documentation in
`DevicePlugin` so it matches the current `DevicesModule` contract rather than
describing per-port listener fan-out. Replace the outdated comment with one that
states plugins are expected to share the same discovery port and that
`DevicesModule` enforces that invariant, referencing the `discoveryPort()` seam
and the behavior implemented in `DevicesModule` so the runtime contract is clear
without overpromising unsupported behavior.
In `@src/core/DevicesModule.h`:
- Around line 189-190: Update the stale comments in DevicesModule to describe
the current UDP discovery flow instead of mDNS, _http._tcp, or _wled._tcp
terminology. Keep the intent of the existing notes around lastSeenMs, age-out,
and device freshness, but rewrite them to reference DevicePlugin-classified UDP
presence packets and the current discovery rationale so the comments match the
code path in DevicesModule.h and the related comment block around the
DevicePlugin handling section.
- Around line 149-150: The self-registration flow in DevicesModule is making the
first local row permanent because selfListed_ blocks later upsertSelf() calls
and ageOut() keeps all self-marked rows fresh forever. Update the self-row
handling so local IP/interface changes can replace or migrate the existing self
entry: revise the logic around upsertSelf(), selfListed_, and ageOut() so only
the current local address is preserved and stale self rows can expire or be
updated when the local URL changes. Also apply the same fix anywhere else the
self-listing path is used so the current address is re-registered instead of
leaving the old one immortal.
- Around line 299-335: In upsertDevice(), avoid triggering persistence on every
presence packet when only transient fields like lastSeenMs change; the
serialized device list only depends on name, ip, type, and self. Update the
logic around refreshStatus() so it runs only when those persisted fields
actually change, and keep using sortByName()/refreshStatus() only after a real
insert or a meaningful field update. Apply the same change to the matching
device-update path referenced by the comment so filesystem writes are only armed
when the saved JSON would differ.
In `@src/core/HttpServerModule.cpp`:
- Around line 1334-1355: The current pollWledStateFromWebSockets() logic treats
each ws.read() result as a full WebSocket message, which can drop fragmented
payloads or ignore additional frames in the same chunk. Update
HttpServerModule::pollWledStateFromWebSockets() to accumulate per-client RX
bytes, parse complete WebSocket frames incrementally, and only call
applyWledState() once a full text frame payload is assembled. Add a regression
test that covers both fragmented and coalesced frames so later {on,bri} updates
are not lost.
- Around line 55-58: `pollWledStateFromWebSockets()` is being called from the 20
ms loop but the underlying `platform::TcpConnection::read()` can still block on
`recv()`, stalling the transport loop per client. Update the `/ws` read path so
the `HttpServerModule::pollWledStateFromWebSockets` flow uses a non-blocking
receive strategy (or temporarily non-blocking socket mode) and returns
immediately when no data is available, instead of waiting on `SO_RCVTIMEO`.
In `@src/core/WledPacket.h`:
- Around line 48-49: `WledPacket::isValid()` is too permissive because it
accepts any datagram larger than the discovery header; tighten the size check so
it only returns true for the exact `kSize` packet length while still validating
the token and id bytes. Update the `isValid(const uint8_t* data, size_t len)`
guard accordingly so the WLED/projectMM discovery path only treats true 44-byte
packets as presence messages before `DevicesModule` upserts the sender.
In `@src/platform/desktop/platform_desktop.cpp`:
- Around line 471-473: Update the discovery comment in the desktop platform code
to state the supported behavior as the active contract rather than describing
what is not present. In the platform_desktop.cpp discovery section, rewrite the
comment around the UdpSocket-based path to emphasize that desktop discovery uses
UDP presence and is unit-testable with loopback datagrams via
DevicesModule::injectPacketForTest or a bound socket, avoiding “No mDNS on
desktop” or other absence-narration phrasing.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1072-1073: The final mDNS summary log in platform_esp32.cpp is
hardcoded to show successful _http._tcp and _wled._tcp advertising even when
txtErr, wledErr, or wledTxtErr indicate failures. Update the ESP_LOGI call in
the mDNS startup summary to report the actual status of each advertised service
using the existing error/result symbols from the setup block, so the message
reflects non-fatal failures accurately and stays useful for debugging.
In `@test/unit/core/unit_DevicesModule_ageout.cpp`:
- Around line 3-14: The file header in unit_DevicesModule_ageout.cpp is outdated
and still describes the removed mDNS discovery flow. Update the comment near the
top of the test to match the current passive UDP presence age-out behavior
exercised by DevicesModule::loop1s(), restoreCachedList(), and listRowCount(),
keeping the narrative accurate without deleting the header.
- Around line 93-109: The current test only verifies restore/serialization and
does not cover the downgrade-prevention path in upsertDevice. Update the
DevicesModule test to first restore the cached projectMM row, then simulate a
later plain-WLED discovery/update through the public list path that reaches
upsertDevice, and finally assert the stored row still serializes as projectMM.
Use the existing DevicesModule, restoreList, writeListRow, and upsertDevice flow
to ensure the test proves a WLED packet cannot overwrite the higher-priority
projectMM type.
- Around line 44-69: Make the online precondition explicit in aPresentAfter()
before calling DevicesModule::loop1s(), since ageOut() only runs when the module
is marked online. Set up the test so the module is definitely online through the
appropriate DevicesModule test hook or a live packet for the device under test,
rather than relying on hidden platform-stub state. Update the affected helpers
in unit_DevicesModule_ageout.cpp (including the other similar case noted in the
review) so they intentionally drive the online path before asserting age-out
behavior.
In `@test/unit/core/unit_DevicesModule_discovery.cpp`:
- Around line 97-105: The regression test in DevicesModule::discovery currently
repeats a projectMM sighting instead of exercising the downgrade scenario, so it
does not verify that upsertDevice() preserves the original type. Update the
second inject() in the “DevicesModule: a projectMM device is not downgraded on a
later sighting” test to use a plain WLED sighting (mm=false) while keeping the
same device/address, so the test specifically guards against projectMM being
downgraded to WLED on re-sighting.
---
Outside diff comments:
In `@src/platform/platform.h`:
- Around line 168-177: Update the mDNS comments in platform.h to match the
current advertise-only behavior: remove the mention that browse queries still
work after mdnsStop() and rewrite the surrounding block to describe the active
contract of mdnsInit(), mdnsStop(), and mdnsShutdown() in present-tense terms.
Keep the explanation focused on what mdnsStop() and mdnsShutdown() do now, and
rephrase the mdnsInit comment to state that projectMM advertises `_http._tcp`
and `_wled._tcp` while DevicesModule and WledPacket handle UDP presence
discovery, avoiding absence-narration and references to removed browse/query
behavior.
---
Nitpick comments:
In `@docs/history/plans/Plan-20260629` - UDP device discovery + mDNS
advertise-only (shipped).md:
- Around line 33-35: Rephrase the spec text to state capabilities positively
instead of describing absences. Update the planning notes around the new
UDP-discovery seam and the host-unit-testing guidance so they say what is
supported (for example, adding a new system via a plugin file and unit-testing
the host without a network stack), and remove the negated wording currently used
in the plan document.
- Around line 59-63: Reword the persistence spec in the DevicesModule section to
describe the self entry positively instead of by negation. Update the sentence
around upsertSelf so it says the self entry is rebuilt live on each boot because
its IP may change, while peer devices are restored from cache. Keep the wording
aligned with DevicesModule, upsertSelf, and the cache restore flow.
In `@src/core/DevicePlugin.h`:
- Around line 23-27: The comment block in DevicePlugin should be rewritten to
describe only the current interface and behavior, removing future-state
narration about planned command support and systems that do not exist yet.
Update the documentation near the DevicePlugin declaration to focus on the
present discovery contract and the actual concrete plugins, and avoid mentioning
reserved `command()` extensions or “later” API changes. Keep the header
public-facing and scan-friendly by using current-state wording only, aligned
with the existing DevicePlugin and DevicesModule design.
In `@src/core/MoonModule.h`:
- Around line 144-148: Rename the core hook in MoonModule from firstLedRgb to a
domain-neutral name such as firstOutputRgb, and update its declaration and any
overrides/call sites accordingly. Keep the same contract and behavior in the
shared API, but avoid light-specific terminology in the MoonModule boundary so
the cross-domain interface stays neutral.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1001-1005: Rewrite the comments in the ESP32 platform code around
the advertise-only mDNS contract so they describe the current behavior directly:
the mDNS stack stays initialized while advertising is toggled, and
discovery/presence is handled by UDP in DevicesModule. Update the relevant
comments near the mDNS init/stop logic and the section that mentions “browse” so
they no longer rely on absence-narration (“does NOT”, “no browse”, “removed”,
“will”) and instead clearly state what the code currently does in the functions
handling mDNS advertisement.
🪄 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: 6d89a26f-a95a-4bd3-b3be-954927f4d47c
⛔ Files ignored due to path filters (3)
docs/assets/screenshots/Devices module.pngis excluded by!**/*.pngdocs/assets/screenshots/WLED Native discovers projectMM.jpegis excluded by!**/*.jpegdocs/assets/screenshots/Wled discovers projectMM.pngis excluded by!**/*.png
📒 Files selected for processing (32)
README.mddocs/backlog/README.mddocs/backlog/backlog-core.mddocs/history/decisions.mddocs/history/plans/Plan-20260629 - DevicesModule mDNS discovery + plugin interop (attempted, superseded by UDP).mddocs/history/plans/Plan-20260629 - UDP device discovery + mDNS advertise-only (shipped).mddocs/moonmodules/core/DevicesModule.mddocs/moonmodules/core/HttpServerModule.mddocs/moonmodules/light/Drivers.mddocs/reference/esp32-s31-coreboard.mdsrc/core/DeviceIdentify.hsrc/core/DevicePlugin.hsrc/core/DevicesModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/core/WledPacket.hsrc/light/drivers/Drivers.hsrc/light/drivers/NetworkSendDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_i2c.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenarios/light/scenario_modifier_chain.jsontest/unit/core/unit_DeviceIdentify.cpptest/unit/core/unit_DevicesModule_ageout.cpptest/unit/core/unit_DevicesModule_discovery.cpptest/unit/core/unit_WledPacket.cpptest/unit/light/unit_Drivers_firstLedRgb.cpp
✅ Files skipped from review due to trivial changes (8)
- test/scenarios/light/scenario_modifier_chain.json
- docs/history/plans/Plan-20260629 - DevicesModule mDNS discovery + plugin interop (attempted, superseded by UDP).md
- docs/history/decisions.md
- docs/moonmodules/core/HttpServerModule.md
- docs/reference/esp32-s31-coreboard.md
- docs/moonmodules/light/Drivers.md
- docs/backlog/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/light/drivers/Drivers.h
- src/light/drivers/NetworkSendDriver.h
- src/platform/esp32/platform_esp32_i2c.cpp
Comment on lines
+15
to
+23
|
|
||
| - **Common patterns first / Industry standards, our own code:** UDP broadcast presence is the textbook LAN-discovery-without-infrastructure pattern; WLED's 65506 packet is a documented wire format we *observe and re-implement fresh* (not copy — per [[no-wled-mm-derivation]] and the MoonLight `ModuleDevices.h` reference we read for the byte layout). mDNS-for-advertise is the standard service-announce. | ||
| - **Default to subtraction / Complexity lives in core:** removes the mDNS-query path from DevicesModule + the platform; the UDP receive primitive is a small core seam each plugin leans on. | ||
| - **Robust to any input:** a malformed/short datagram is dropped, never crashes (the robustness contract); the plugin classify stays defensive. | ||
| - **Hot path discipline:** the UDP listen is a non-blocking `recvFrom` drained on `loop1s`/`loop20ms`, off the render path — same shape as the current mDNS poll. | ||
|
|
||
| ## The transport split (the end state) | ||
|
|
||
| | Plugin | Discovers peers via | Makes us discoverable via | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rephrase absence-narration to describe what exists.
Lines 17–22 describe the discovery mechanism by listing what was removed ("No subnet sweep, no per-host probe, no mDNS query", "never queries mDNS"). Per coding guidelines, avoid absence-narration phrasing; describe what exists now rather than what was removed. Rephrase positively, e.g.:
"Discovery is passive UDP receive only: devices announce themselves via broadcast, and this module listens on a bound
UdpSocketdrained non-blocking each tick."
Similarly, rephrase line 22 to state what mDNS does ("mDNS advertises only") rather than what it doesn't do. As per coding guidelines, module specs should describe what exists now rather than what was removed or what will happen later.
🤖 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/plans/Plan-20260629` - UDP device discovery + mDNS
advertise-only (shipped).md around lines 15 - 23, Rephrase the discovery-plan
text in the plan document to describe the current behavior positively instead of
listing removed or forbidden paths. Update the wording around the UDP/mDNS
transport split so it states what DevicesModule and the platform do now, using
the existing concepts like passive UDP receive, bound UdpSocket, loop1s/loop20ms
draining, and mDNS advertise-only, without phrases like “no”, “never”, or
“removed.”
Source: Coding guidelines
Comment on lines
+71
to
+75
|
|
||
| - `mdnsInit` unchanged (keep the `_http`+`mm=1`, `_wled`+`mac=`, re-advertise, symmetric stop — all shipped). | ||
| - **Remove** `mdnsListenPoll` (platform) + its decl + the desktop stub + the DevicesModule caller. mDNS no longer queries anything. | ||
| - `MdnsHost` struct + `mdnsBrowse` may stay if still used elsewhere; if not, remove (subtraction). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Prior art bullet contradicts current architecture.
Line 73 states DevicesModule "uses the standard mDNS announce/listen instead of a bespoke UDP beacon," but the rewritten architecture is explicitly UDP-based discovery with mDNS advertise-only. The module now does use a UDP beacon (WLED-compatible presence packet on 65506). This prior art description is stale and misrepresents the current design. Rewrite to accurately reflect that DevicesModule adopted MoonLight's UDP-presence pattern (broadcast + listen) while keeping mDNS for cross-ecosystem visibility only.
🤖 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/plans/Plan-20260629` - UDP device discovery + mDNS
advertise-only (shipped).md around lines 71 - 75, The prior-art note in the plan
is outdated and describes the wrong discovery model. Update the DevicesModule
summary to say it uses MoonLight-style UDP-presence discovery (broadcast +
listen on 65506) with mDNS kept advertise-only for cross-ecosystem visibility,
and remove any wording that implies standard mDNS announce/listen or a bespoke
UDP beacon mismatch. Use the DevicesModule and UDP-presence references in this
section to keep the description aligned with the shipped architecture.
|
|
||
| ### 5. mDNS: advertise-only | ||
|
|
||
| - `mdnsInit` unchanged (keep the `_http`+`mm=1`, `_wled`+`mac=`, re-advertise, symmetric stop — all shipped). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Inaccurate claim: symmetric stop was not shipped.
Line 72 states "symmetric stop — all shipped," but the PR review identified that mdnsStop() removes only _http._tcp while mdnsInit() advertises both _http._tcp and _wled._tcp. This asymmetric lifecycle is explicitly called "the most concrete bug" in the review. Please correct this plan document to accurately reflect that symmetric teardown was identified as a bug, not a shipped feature. As per coding guidelines, history entries must be pruned or corrected when they contain inaccurate claims.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/history/plans/Plan-20260629` - UDP device discovery + mDNS
advertise-only (shipped).md at line 72, The history note in the plan document
inaccurately claims “symmetric stop” was shipped; update the entry to reflect
that mdnsStop and mdnsInit are asymmetric, with mdnsStop only removing
_http._tcp while mdnsInit advertises both _http._tcp and _wled._tcp. Correct or
prune the sentence around the mdnsInit/mdnsStop note so it no longer states the
bug was shipped, and make the wording match the reviewed behavior of the mDNS
lifecycle.
Addresses the CodeRabbit review on the UDP-discovery commit (8 findings fixed, the rest skipped with reasons), fixes a pre-existing desktop blocking-socket stall that made the web UI sluggish and capped FPS, and corrects a few scripts/docs that referenced the wrong desktop build directory.
PC tick worst-case 388us(FPS:2577); ESP32 S31 live tick:784us(FPS:1275). (ctest flags the pre-existing NetworkReceiveEffect port-contention flake; a direct mm_tests run is 8330/8330 SUCCESS.)
Core
- DevicesModule: the self row now migrates with our IP — upsertSelf runs every tick (idempotent), demotes a stale self row left at an old address, and ageOut keys "this is us, keep fresh" on the current local IP rather than the self flag, so a DHCP renew / eth↔WiFi switch can't leave an immortal ghost at the old IP (removed the one-shot selfListed_ guard). upsertDevice now arms persistence only when a SAVED field (name/ip/type/self) actually changes, so a re-sighting that only refreshes lastSeenMs/cached no longer triggers a flash write on every ~10 s presence broadcast. 🐇
- WledPacket: isValid() requires the exact 44-byte length (was >=), so a longer datagram on the port isn't treated as a presence packet. 🐇
- HttpServerModule: pollWledStateFromWebSockets walks every complete masked text frame in a read instead of only the first, so a fast slider drag that coalesces multiple {on,bri} frames into one chunk doesn't drop the intermediate updates. 🐇
- MoonModule::firstLedRgb renamed to firstOutputRgb — a domain-neutral name at the core boundary (Drivers override + the WLED shim call site + the unit test renamed to match). 🐇
Light domain
- Drivers::firstOutputRgb — the renamed override (behaviour unchanged: reads pixel 0 of the driven buffer).
Platform
- platform_desktop: the accepted client socket is persistently non-blocking instead of carrying a 2 s SO_RCVTIMEO. The HTTP/WS server runs inside the single render loop, so a blocking recv() froze the whole loop for up to 2 s whenever a request's bytes hadn't landed the instant accept() returned — the "sluggish UI" + the ~18 FPS cap (a fixed per-connection stall, unrelated to grid size). writeSome no longer toggles the socket back to blocking. handleConnection reads non-blocking with a short bounded wait. PC UI latency ~2 s → ~13 ms, FPS now tracks real render speed. Pre-existing (present on main). 🐇 (non-blocking-read finding)
- platform_esp32: the mDNS startup summary log reports each service's actual result (ok/fail) instead of claiming success unconditionally. 🐇
UI
- (no functional change) — covered by the platform read fix above.
Tests
- unit_DevicesModule_discovery: the "not downgraded" case now injects a plain WLED packet (mm=false) on the second sighting, actually exercising the type-only-raises path. 🐇
- unit_DevicesModule_ageout: header rewritten for UDP presence; the downgrade case now drives a real WLED packet through injectPacketForTest after restore (was a no-op restore→serialize). 🐇
- unit_WledPacket / unit_Drivers_firstOutputRgb: the firstOutputRgb rename.
Docs / CI
- DeviceIdentify.h, DevicePlugin.h, platform.h, platform_esp32.cpp comments rewritten present-tense around UDP-presence discovery + advertise-only mDNS (no absence-narration, no stale "browse"). 🐇
- MoonDeck.md, screenshot_modules.py, testing.md: point desktop build/test commands at build/<host>/ (macos/linux/windows), the canonical per-host dir MoonDeck and build_desktop.py use — was the cause of a stale-binary divergence between MoonDeck and ad-hoc builds.
Reviews
- 🐇 mdnsStop "asymmetric" — skipped: it already removes BOTH _http + _wled (CR reviewed stale code); the plan's "symmetric stop" note is accurate.
- 🐇 WS non-blocking read — covered by the platform_desktop socket fix above.
- 🐇 explicit online-precondition in the ageout test — skipped: passes via the desktop stub's host IP, now documented in the header; no test hook exists and the value is low.
- 🐇 plan.md prior-art/wording — skipped: the cited lines are in the SUPERSEDED mDNS plan (correctly marked attempted/abandoned, where the old-design wording belongs), not the shipped UDP plan, which is accurate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ewowi
changed the title
Collaborator
Author
|
@coderabbitai, can you review a8ef250 and share your results in a copy passable prompt for agents |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="a8ef250d4c9adb22a27fcdfd389fc46a29465644"
echo "=== Commit metadata ==="
git show --stat --summary --oneline "$commit"
echo
echo "=== Changed files ==="
git diff-tree --no-commit-id --name-only -r "$commit"
echo
echo "=== Full diff (first 1200 lines) ==="
git show --unified=80 --format=medium "$commit" | sed -n '1,1200p'
echo
echo "=== Candidate file outlines ==="
git diff-tree --no-commit-id --name-only -r "$commit" | while read -r f; do
echo "--- OUTLINE: $f ---"
ast-grep outline "$f" --view expanded || true
done
echo
echo "=== Focused searches for touched symbols / risks ==="
rg -n -C 4 \
'mdnsInit|mdnsStop|mdns_service|_http\._tcp|_wled\._tcp|WledPacket|classifyPacket|injectPacketForTest|firstLedRgb|windowSlice|audioCodecInit|audioMicInit|i2cScan|MM_MAX_GPIO' \
src esp32 test docs || true
echo
echo "=== Relevant slices from likely changed implementation files ==="
for f in \
src/platform/esp32/platform_esp32.cpp \
src/core/DevicesModule.h \
src/core/DevicePlugin.h \
src/core/WledPacket.h \
src/core/HttpServerModule.cpp \
src/light/drivers/Drivers.h \
src/light/drivers/NetworkSendDriver.h \
src/light/drivers/RmtLedDriver.h \
src/light/drivers/ParallelLedDriver.h \
src/core/AudioModule.h \
src/platform/platform.h \
src/platform/esp32/platform_esp32_es8311.cpp \
src/platform/esp32/platform_esp32_i2c.cpp \
src/platform/esp32/platform_esp32_i2s.cpp
do
if [ -f "$f" ]; then
echo "--- FILE: $f ---"
sed -n '1,260p' "$f"
echo
fi
doneLength of output: 129657 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="a8ef250d4c9adb22a27fcdfd389fc46a29465644"
echo "=== Commit metadata ==="
git show --no-patch --format=fuller "$commit"
echo
echo "=== Changed files ==="
git diff-tree --no-commit-id --name-status -r "$commit"
echo
echo "=== Patch for this commit only ==="
git show --stat=200 --unified=40 --format=medium "$commit"
echo
echo "=== Per-file focused context ==="
while read -r status path; do
[ -n "${path:-}" ] || continue
echo "--- FILE: $path ---"
ast-grep outline "$path" --view expanded || true
echo "--- DIFF HUNKS: $path ---"
git diff "${commit}^" "$commit" --unified=40 -- "$path"
echo
done < <(git diff-tree --no-commit-id --name-status -r "$commit")Length of output: 153007 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== DevicesModule serialization / persistence slices ==="
sed -n '1,260p' src/core/DevicesModule.h
echo
sed -n '260,520p' src/core/DevicesModule.h
echo
echo "=== HttpServerModule body handling slices ==="
sed -n '1,220p' src/core/HttpServerModule.cpp
echo
rg -n -C 6 'handleWledState|handleSetControl|handleAddModule|json::hasKey|parse' src/core/HttpServerModule.cpp src/core/JsonUtil.h src/core/JsonUtil_parse.cpp || trueLength of output: 73268
Overall: this looks like a good cleanup pass. The strongest fixes in it are real:
The main thing I’d still ask an agent to check/fix is a likely regression in Here’s a copy-pastable prompt for agents: My ranking of findings from this commit:
If you want, I can also turn this into a shorter implementation-only prompt. |
…tion Processes the PR-merge Opus reviewer pass (👾) and a final CodeRabbit round (🐇) on the device-discovery branch before it merges to main. The substantive fix is a real regression from the non-blocking-socket change: a POST body that arrives a TCP segment after its headers could be read truncated and routed into the permissive JSON parsers. The rest is subtraction (an orphaned discovery path) and doc/consistency corrections. PC worst-scenario tick:475us; ESP32 tick:1039us(FPS:962). Core - HttpServerModule: the POST body read now waits for a late-arriving body (the same bounded ~50 ms retry as the header phase) instead of breaking on the first non-blocking -1, then rejects with 400 if the declared Content-Length still hasn't arrived — so a partial body never reaches handleSetControl / handleWledState / add/replace / firmware-url. Live-verified: a body sent 40 ms after the headers now returns 200 (was truncating). 🐇 - DevicesModule: upsertSelf now refreshes status + arms persistence — but only when the self row actually changed (insert or IP migration), not on every idempotent tick (same don't-persist-a-no-op rule as upsertDevice). 👾🐇 Platform - Removed platform::httpGet entirely — the HTTP-sweep discovery it served was replaced by UDP presence, leaving it with zero callers: the decl + its stale doc block (platform.h), the ESP32 impl (platform_esp32_httpget.cpp + its CMake entry), and the desktop impl. The build then flagged make_blocking as unused (the non-blocking-socket change removed its last caller) — removed both definitions too. Pure subtraction. 👾 - platform_desktop i2cScan: a no-I2C host now returns kI2cBusUnavailable (matching the I2C-less ESP32 stub), so "no bus" reads distinctly from "scanned, found nothing". 👾 Docs / CI - DevicesModule.md: corrected the age-out wording (kStaleMs = 24 h live, kCachedGraceMs = 60 s cached probation — was conflated as 60 s) and the prior-art note (now describes the shipped UDP-presence beacon; it had said "mDNS instead of a UDP beacon", the opposite of what shipped). 👾 - I2cScanModule.md: the no-I2C target reports "bus unavailable", not "0 devices". 👾 - decisions.md: carried forward the branch's hard-won lesson — a blocking socket op on the single render loop freezes everything, and an intermittent freeze hides as the wrong cause (the desktop-UI-stall debugging). Reviews - 👾 reviewer verdict: architecture sound, no blockers — DevicePlugin seam, WledPacket, per-driver window, firstOutputRgb, WLED shim, non-blocking socket, I2cScan all verified clean; findings were the orphaned httpGet + the doc/consistency items above, all fixed. - 🐇 truncated POST body after the non-blocking switch — fixed (above). - 🐇 WLED exact-length presence validation, multi-frame WS parsing, mDNS summary-log accuracy — confirmed already correct in the prior commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/testing.md`:
- Line 14: Update the Unit tests description in docs/testing.md so the prose
label uses macOS instead of desktop, while keeping the build path token as
macos; adjust the explanatory text in the testing section only, preserving the
reference to test/unit/{core,light}/unit_*.cpp, the // `@module` annotation, and
the ctest / ./build/<host>/test/mm_tests invocation.
In `@scripts/docs/screenshot_modules.py`:
- Around line 362-364: The three print statements in the screenshot_modules.py
help text are using f-strings without any interpolated values, which triggers
Ruff F541. Remove the unused f prefixes from those print calls in the relevant
help/output block so the strings are plain literals while keeping the text
unchanged.
In `@src/core/DevicesModule.h`:
- Around line 334-340: The name-refresh logic in DevicesModule.h still treats
every DevType::Wled packet as authoritative, which can let a plain WLED packet
rename an existing ProjectMM row. Update the authoritative check in the block
around found.name handling so only truly higher-priority matches can overwrite
d->name, preserving the precedence enforced by newType and the ProjectMM/Wled
rules already used in this path.
In `@src/core/HttpServerModule.cpp`:
- Around line 72-89: The bounded-wait logic in
HttpServerModule::handleConnection only covers the initial header read, so POST
requests can still be truncated when the body arrives in a later packet. Extend
the same patience/accumulation behavior to the request body parsing path after
the headers are detected, and keep reading until the expected POST payload is
complete or the same short timeout is exceeded. Make the fix in the
request-handling flow around conn.read, the header/body split, and the dispatch
for /api/control, /api/modules, and /json/state so packetization does not affect
valid JSON requests.
In `@test/unit/light/unit_Drivers_firstOutputRgb.cpp`:
- Around line 38-78: Add a test case that exercises Drivers::firstOutputRgb
through the composed outputBuffer_ path instead of only the single-layer
zero-copy buffer path. Create a setup with multiple layers or a LUT/composition
scenario so firstOutputRgb reads from the composed buffer, then assert the
returned RGB values match the expected first output pixel. Keep the existing
first pixel and black-pixel coverage, but extend the suite to verify the branch
in Drivers::firstOutputRgb that prefers outputBuffer_ over layer.buffer().
🪄 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: a9e980d1-c8d0-4adf-9a05-042809195f19
📒 Files selected for processing (18)
docs/moonmodules/core/HttpServerModule.mddocs/testing.mdscripts/MoonDeck.mdscripts/docs/screenshot_modules.pysrc/core/DeviceIdentify.hsrc/core/DevicePlugin.hsrc/core/DevicesModule.hsrc/core/HttpServerModule.cppsrc/core/MoonModule.hsrc/core/WledPacket.hsrc/light/drivers/Drivers.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/CMakeLists.txttest/unit/core/unit_DevicesModule_ageout.cpptest/unit/core/unit_DevicesModule_discovery.cpptest/unit/light/unit_Drivers_firstOutputRgb.cpp
| Three test categories, each with a clear purpose: | ||
|
|
||
| - **Unit tests** (desktop, `test/unit/{core,light}/unit_*.cpp`) — exercise individual MoonModules in isolation with doctest. Each test file declares `// @module <Name>` so it's categorised under that module in the generated inventory. Run via `ctest` or `./build/test/mm_tests`. Verify a module's API, edge cases, and output independent of how it's wired into a pipeline. | ||
| - **Unit tests** (desktop, `test/unit/{core,light}/unit_*.cpp`) — exercise individual MoonModules in isolation with doctest. Each test file declares `// @module <Name>` so it's categorised under that module in the generated inventory. Run via `ctest` or `./build/<host>/test/mm_tests` (`<host>` = `macos`/`linux`/`windows`, the per-host build dir). Verify a module's API, edge cases, and output independent of how it's wired into a pipeline. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use macOS in the prose label.
Keep the directory token as macos if that is the real build folder, but spell the platform name as macOS in the explanatory text.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: The operating system from Apple is written “macOS”.
Context: ...build//test/mm_tests (=macos/linux/windows`, the per-host build ...
(MAC_OS)
🤖 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/testing.md` at line 14, Update the Unit tests description in
docs/testing.md so the prose label uses macOS instead of desktop, while keeping
the build path token as macos; adjust the explanatory text in the testing
section only, preserving the reference to test/unit/{core,light}/unit_*.cpp, the
// `@module` annotation, and the ctest / ./build/<host>/test/mm_tests invocation.
Source: Linters/SAST tools
Comment on lines
+362
to
+364
| print(f" second projectMM (a stale build/<host>/projectMM) is still on :8080.") | ||
| print(f" Fix: pkill -f projectMM then rebuild + run ONE server:") | ||
| print(f" uv run scripts/build/build_desktop.py && ./build/<host>/projectMM") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the unused f prefixes.
These print(...) calls have no placeholders, so Ruff will keep flagging F541 on this hunk.
Suggested fix
- print(f" second projectMM (a stale build/<host>/projectMM) is still on :8080.")
- print(f" Fix: pkill -f projectMM then rebuild + run ONE server:")
- print(f" uv run scripts/build/build_desktop.py && ./build/<host>/projectMM")
+ print(" second projectMM (a stale build/<host>/projectMM) is still on :8080.")
+ print(" Fix: pkill -f projectMM then rebuild + run ONE server:")
+ print(" uv run scripts/build/build_desktop.py && ./build/<host>/projectMM")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print(f" second projectMM (a stale build/<host>/projectMM) is still on :8080.") | |
| print(f" Fix: pkill -f projectMM then rebuild + run ONE server:") | |
| print(f" uv run scripts/build/build_desktop.py && ./build/<host>/projectMM") | |
| print(" second projectMM (a stale build/<host>/projectMM) is still on :8080.") | |
| print(" Fix: pkill -f projectMM then rebuild + run ONE server:") | |
| print(" uv run scripts/build/build_desktop.py && ./build/<host>/projectMM") |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 362-362: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 363-363: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 364-364: 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/docs/screenshot_modules.py` around lines 362 - 364, The three print
statements in the screenshot_modules.py help text are using f-strings without
any interpolated values, which triggers Ruff F541. Remove the unused f prefixes
from those print calls in the relevant help/output block so the strings are
plain literals while keeping the text unchanged.
Source: Linters/SAST tools
Comment on lines
+334
to
+340
| const bool authoritative = | ||
| (found.type == DevType::ProjectMM && d->type == DevType::ProjectMM) || | ||
| (found.type == DevType::Wled); | ||
| if (found.name[0] && (!d->name[0] || isIpPlaceholder(d->name, ip) || authoritative) | ||
| && std::strcmp(d->name, found.name) != 0) { | ||
| std::snprintf(d->name, sizeof(d->name), "%s", found.name); | ||
| persistChanged = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't let plain WLED packets rename a projectMM row.
This authoritative check is still true for every found.type == DevType::Wled, even after newType kept d->type == DevType::ProjectMM. A later unmarked packet from the same IP can therefore overwrite a projectMM peer's name, which contradicts the precedence rule in the comment above.
Suggested fix
- const bool authoritative =
- (found.type == DevType::ProjectMM && d->type == DevType::ProjectMM) ||
- (found.type == DevType::Wled);
+ const bool authoritative =
+ (found.type == DevType::ProjectMM && d->type == DevType::ProjectMM) ||
+ (found.type == DevType::Wled && d->type == DevType::Wled);📝 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 bool authoritative = | |
| (found.type == DevType::ProjectMM && d->type == DevType::ProjectMM) || | |
| (found.type == DevType::Wled); | |
| if (found.name[0] && (!d->name[0] || isIpPlaceholder(d->name, ip) || authoritative) | |
| && std::strcmp(d->name, found.name) != 0) { | |
| std::snprintf(d->name, sizeof(d->name), "%s", found.name); | |
| persistChanged = true; | |
| const bool authoritative = | |
| (found.type == DevType::ProjectMM && d->type == DevType::ProjectMM) || | |
| (found.type == DevType::Wled && d->type == DevType::Wled); | |
| if (found.name[0] && (!d->name[0] || isIpPlaceholder(d->name, ip) || authoritative) | |
| && std::strcmp(d->name, found.name) != 0) { | |
| std::snprintf(d->name, sizeof(d->name), "%s", found.name); | |
| persistChanged = true; |
🤖 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/DevicesModule.h` around lines 334 - 340, The name-refresh logic in
DevicesModule.h still treats every DevType::Wled packet as authoritative, which
can let a plain WLED packet rename an existing ProjectMM row. Update the
authoritative check in the block around found.name handling so only truly
higher-priority matches can overwrite d->name, preserving the precedence
enforced by newType and the ProjectMM/Wled rules already used in this path.
Comment on lines
+72
to
+89
| // Read the request. read() is non-blocking (-1 = nothing pending yet), so the render | ||
| // loop is never stalled waiting for bytes (a blocking socket timeout used to freeze the | ||
| // whole loop). A just-accepted connection's request normally lands in the same read; if | ||
| // not, allow a SHORT bounded wait (≤ ~5 ms total) for it, then bail — an idle/half-open | ||
| // connection costs at most that, and the steady-state (nothing pending) costs ~0. | ||
| for (int empties = 0; totalRead < static_cast<int>(sizeof(buf) - 1);) { | ||
| int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead); | ||
| if (n > 0) { | ||
| totalRead += n; | ||
| buf[totalRead] = 0; | ||
| if (std::strstr(reinterpret_cast<char*>(buf), "\r\n\r\n")) break; | ||
| empties = 0; // got data — reset the patience counter | ||
| } else if (n == 0) { | ||
| return; // peer closed | ||
| } else { | ||
| break; // timeout or error | ||
| return; // peer closed | ||
| } else { // -1 = nothing pending yet | ||
| if (totalRead > 0) break; // had a partial then nothing more — process it | ||
| if (++empties > 5) break; // fresh conn, no bytes after ~5 ms — give up | ||
| platform::delayMs(1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Extend the bounded wait to the POST body as well.
This new patience loop only protects the header read. If a POST arrives as headers then body in a later packet, the code below still stops on the first read() == -1 and dispatches a truncated JSON body. That makes /api/control, /api/modules, and /json/state fail based on packetization rather than payload validity.
💡 Minimal fix
- while (totalRead < bodyNeeded && totalRead < static_cast<int>(sizeof(buf) - 1)) {
- int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead);
- if (n > 0) totalRead += n;
- else break;
- }
+ for (int empties = 0;
+ totalRead < bodyNeeded && totalRead < static_cast<int>(sizeof(buf) - 1);) {
+ int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead);
+ if (n > 0) {
+ totalRead += n;
+ empties = 0;
+ } else if (n == 0) {
+ return;
+ } else {
+ if (++empties > 5) break;
+ platform::delayMs(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
| // Read the request. read() is non-blocking (-1 = nothing pending yet), so the render | |
| // loop is never stalled waiting for bytes (a blocking socket timeout used to freeze the | |
| // whole loop). A just-accepted connection's request normally lands in the same read; if | |
| // not, allow a SHORT bounded wait (≤ ~5 ms total) for it, then bail — an idle/half-open | |
| // connection costs at most that, and the steady-state (nothing pending) costs ~0. | |
| for (int empties = 0; totalRead < static_cast<int>(sizeof(buf) - 1);) { | |
| int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead); | |
| if (n > 0) { | |
| totalRead += n; | |
| buf[totalRead] = 0; | |
| if (std::strstr(reinterpret_cast<char*>(buf), "\r\n\r\n")) break; | |
| empties = 0; // got data — reset the patience counter | |
| } else if (n == 0) { | |
| return; // peer closed | |
| } else { | |
| break; // timeout or error | |
| return; // peer closed | |
| } else { // -1 = nothing pending yet | |
| if (totalRead > 0) break; // had a partial then nothing more — process it | |
| if (++empties > 5) break; // fresh conn, no bytes after ~5 ms — give up | |
| platform::delayMs(1); | |
| for (int empties = 0; | |
| totalRead < bodyNeeded && totalRead < static_cast<int>(sizeof(buf) - 1);) { | |
| int n = conn.read(buf + totalRead, sizeof(buf) - 1 - totalRead); | |
| if (n > 0) { | |
| totalRead += n; | |
| empties = 0; | |
| } else if (n == 0) { | |
| return; | |
| } else { | |
| if (++empties > 5) break; | |
| platform::delayMs(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 `@src/core/HttpServerModule.cpp` around lines 72 - 89, The bounded-wait logic
in HttpServerModule::handleConnection only covers the initial header read, so
POST requests can still be truncated when the body arrives in a later packet.
Extend the same patience/accumulation behavior to the request body parsing path
after the headers are detected, and keep reading until the expected POST payload
is complete or the same short timeout is exceeded. Make the fix in the
request-handling flow around conn.read, the header/body split, and the dispatch
for /api/control, /api/modules, and /json/state so packetization does not affect
valid JSON requests.
Comment on lines
+38
to
+78
| TEST_CASE("Drivers::firstOutputRgb reads pixel 0 of the driven buffer") { | ||
| Rig r; | ||
| uint8_t* buf = r.layer.buffer().data(); | ||
| REQUIRE(buf != nullptr); | ||
| buf[0] = 10; buf[1] = 200; buf[2] = 60; // pixel 0 = R,G,B (logical channel order) | ||
|
|
||
| uint8_t rgb[3] = {0, 0, 0}; | ||
| REQUIRE(r.drivers.firstOutputRgb(rgb)); | ||
| CHECK(rgb[0] == 10); | ||
| CHECK(rgb[1] == 200); | ||
| CHECK(rgb[2] == 60); | ||
| } | ||
|
|
||
| TEST_CASE("Drivers::firstOutputRgb reports black pixel 0 as-is (caller substitutes the default)") { | ||
| Rig r; | ||
| uint8_t* buf = r.layer.buffer().data(); | ||
| REQUIRE(buf != nullptr); | ||
| buf[0] = 0; buf[1] = 0; buf[2] = 0; // black — firstOutputRgb returns true with 0,0,0 | ||
|
|
||
| uint8_t rgb[3] = {1, 2, 3}; | ||
| REQUIRE(r.drivers.firstOutputRgb(rgb)); // true: there IS an output, it's just black | ||
| CHECK(rgb[0] == 0); | ||
| CHECK(rgb[1] == 0); | ||
| CHECK(rgb[2] == 0); | ||
| // (The WLED shim is what maps an all-black read to projectMM purple — pinned in the | ||
| // HTTP shim's own logic, not here; this seam just reports the raw pixel.) | ||
| } | ||
|
|
||
| TEST_CASE("Drivers::firstOutputRgb returns false when there is no driven buffer") { | ||
| mm::Drivers drivers; // no layer pinned, no buffer | ||
| uint8_t rgb[3] = {9, 9, 9}; | ||
| CHECK_FALSE(drivers.firstOutputRgb(rgb)); | ||
| } | ||
|
|
||
| TEST_CASE("MoonModule::firstOutputRgb defaults to false (no output module)") { | ||
| // A plain module that doesn't drive output never claims a first LED — the seam's | ||
| // safe default, so the shim falls back to purple for a device with no Drivers. | ||
| mm::MoonModule m; | ||
| uint8_t rgb[3] = {}; | ||
| CHECK_FALSE(m.firstOutputRgb(rgb)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a case for the composed outputBuffer_ path.
All of these tests keep Drivers on the single-layer zero-copy path, so firstOutputRgb() never exercises the preferred outputBuffer_ branch used when multiple layers are enabled or a LUT forces composition. That leaves the new WLED-tint source untested on the branch most likely to diverge from the layer buffer.
As per path instructions, test/**: Verify tests cover edge cases and match the specifications in docs/moonmodules/.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 49-49: The function 'formatDottedQuad' is never used.
(unusedFunction)
[style] 62-62: The function 'sanitizeHostname' is never used.
(unusedFunction)
[style] 68-68: The function 'apply' is never used.
(unusedFunction)
[style] 59-59: The function 'teardown' is never used.
(unusedFunction)
[style] 53-53: The function 'layouts' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/light/unit_Drivers_firstOutputRgb.cpp` around lines 38 - 78, Add a
test case that exercises Drivers::firstOutputRgb through the composed
outputBuffer_ path instead of only the single-layer zero-copy buffer path.
Create a setup with multiple layers or a LUT/composition scenario so
firstOutputRgb reads from the composed buffer, then assert the returned RGB
values match the expected first output pixel. Keep the existing first pixel and
black-pixel coverage, but extend the suite to verify the branch in
Drivers::firstOutputRgb that prefers outputBuffer_ over layer.buffer().
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.
Five commits across one cycle: ESP32-S31 onboard-mic infrastructure, then a full reshape of device discovery — from a slow HTTP/mDNS approach to passive UDP presence, with WLED native-app interoperability (projectMM devices list, show live colour, and are controllable from the WLED apps + appear in WLED's own instances list). Plus a CodeRabbit-review pass and a pre-existing desktop-UI stall fix found along the way.
ESP32 S31 live
tick:784us(FPS:1275); PC worst-scenario tick ~388us.What landed
ESP32-S31 microphone + I2C (commit 1)
CodecType/AudioCodecPins/audioCodecInit) over the platform's first I2C master bus;AudioModulebrings up the codec before the I2S mic.I2cScanModule— a reusablei2cdetect-style bus-scan diagnostic (sda/scl/scan/result controls).start/countwindow into the shared light buffer (an onboard LED + a strip as two driver instances on one buffer).DevicesModule: UDP presence discovery + plugin interop (commits 2–4)
WledPacket, with a projectMM marker distinguishing a peer from a plain WLED). This makes the prior mDNS self-query-disturbs-advertise instability structurally impossible — mDNS is now advertise-only.DevicePluginseam is transport-agnostic (discoveryPort()/classifyPacket()); projectMM + WLED plugins ship./json/info+/json/state+/json/sishim and a WLED{state,info}push/poll over the existing/ws, so a projectMM device lists in the native WLED apps + Home Assistant, shows the live first-LED colour, and has a working bidirectional brightness/on-off control. projectMM devices also appear in WLED's own "instances" sync list.MoonModule::firstOutputRgb— a domain-neutral core seam (Drivers reads pixel 0) so the shim tints the WLED card without a light-domain include in core.Review processing + desktop stall fix (commit 5)
SO_RCVTIMEO, and since the HTTP/WS server runs inside the single render loop, a blockingrecv()froze the whole loop — the sluggish web UI + a ~18-FPS cap independent of grid size. Now non-blocking: ~2 s → ~13 ms, FPS tracks real render speed.build/<host>/desktop build dir.Verification
Desktop build (zero warnings), full suite 8330 assertions pass, scenarios pass, all 4 ESP32 variants build clean, spec/boundary/devices checks green. (ctest flags the known NetworkReceiveEffect port-contention flake; a direct run is 8330/8330 SUCCESS.)
Prior art credited: Christophe Gagnier (@Moustachauve, WLED-Android — the interop contract), MoonLight (the UDP 65506 packet shape), softhack007/troyhacks/hpwit/Improv (README credits).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests