Conversation
ModuleDevices: - Replace UDPMessage.rommel[6] (all-zeros) with proper UDPWLEDHeader (44 bytes) matching WLED/StarLight packet format (token=255, id=1, ip, name, type, version) - static_assert(sizeof(UDPWLEDHeader)==44) enforces layout at build time - lightsOn encoded in header.type bit 7, matching WLED convention - Add UDPControlMessage for port 65507 (MoonLight-only, WLED never sees this) - Move ALL control (broadcast + unicast) to port 65507; port 65506 carries discovery packets only — WLED can no longer receive MoonLight control commands - receiveUDP: size-based dispatch — 44 bytes = WLED, sizeof(UDPMessage) = MoonLight - Add updateDevicesWLED() for 44-byte WLED packets (name/ip/lightsOn available) - Track deviceControlUDPConnected separately; guard sendUDP(true) on it - partOfGroup comment: corrected "dots" to "hyphens" Docs: - Add docs/develop/devices.md: UDP protocol reference for developers - Update docs/moonbase/devices.md: end-user description including WLED coexistence - Add develop/devices.md to mkdocs.yml nav - Replace stale ModuleDevices section in docs/develop/network.md with pointer
WalkthroughAdds a dual-socket UDP protocol for ModuleDevices: WLED-compatible discovery on UDP/65506 and MoonLight-only control on UDP/65507. Introduces packed wire structs ( ChangesModuleDevices — UDP dual-socket protocol & device-table sync
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant MB as ModuleDevices
participant Sock65506 as UDP Socket (65506)
participant Sock65507 as UDP Socket (65507)
participant Device as Remote Device
Note over Sock65506,Sock65507: Discovery (65506) & Control (65507) split
UI->>MB: onUpdate(device, state)
alt target == local
MB->>MB: apply locally via _moduleControl->update(origin="unicast")
else
MB->>Sock65507: send UDPControlMessage -> targetIP:65507 (unicast) or broadcast (group, targetName="")
end
Note over Device,Sock65507: Group broadcast propagation
Sock65507->>Device: receive UDPControlMessage (group)
Device->>Sock65507: possibly re-broadcast unless originId == "group"
Note over Device,Sock65506: Discovery flows
Device->>Sock65506: broadcast UDPWLEDHeader (44) or UDPMessage (101)
Sock65506->>MB: receiveUDP()
alt size==44
MB->>MB: updateDevicesWLED(header, IP)
else size==101
MB->>MB: updateDevices(message, IP)
end
MB->>MB: finaliseDeviceUpdate()
MB->>UI: update device table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonBase/Modules/ModuleDevices.h (2)
149-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecouple the control socket lifecycle from the discovery socket.
deviceControlUDP.begin()is only retried whiledeviceUDPConnectedis false, andloop20ms()stops polling entirely when 65506 is down. That means one transient bind failure on 65507, or any ongoing failure on 65506, can disable MoonLight control even though the other socket is still usable.Suggested change
void loop20ms() override { Module::loop20ms(); if (!networkIsConnected()) return; - - if (!deviceUDPConnected) return; + if (!deviceUDPConnected && !deviceControlUDPConnected) return; receiveUDP(); // and updateDevices } void loop10s() override { if (!networkIsConnected()) return; if (!deviceUDPConnected) { deviceUDPConnected = deviceUDP.begin(deviceUDPPort); - deviceControlUDPConnected = deviceControlUDP.begin(deviceControlUDPPort); // Initialize control UDP on separate port + } + if (!deviceControlUDPConnected) { + deviceControlUDPConnected = deviceControlUDP.begin(deviceControlUDPPort); // Initialize control UDP on separate port EXT_LOGD(MB_TAG, "deviceUDP:%d p:%d, controlUDP:%d p:%d", deviceUDPConnected, deviceUDPPort, deviceControlUDPConnected, deviceControlUDPPort); if (!deviceControlUDPConnected) EXT_LOGW(MB_TAG, "Failed to bind control UDP on port %d", deviceControlUDPPort); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Modules/ModuleDevices.h` around lines 149 - 170, The control UDP socket lifecycle is currently tied to deviceUDPConnected causing control to stop if the discovery socket or its bind (deviceUDP.begin/deviceUDPConnected) fails; update loop10s() and loop20ms() so deviceControlUDP.begin(deviceControlUDPPort) is attempted and retried independently of deviceUDPConnected, use deviceControlUDPConnected as its own flag, and ensure loop20ms() still polls/receives from the control socket (and/or calls the control-specific receive path) even when deviceUDPConnected is false; keep existing logs (EXT_LOGD/EXT_LOGW) but reflect independent bind attempts for deviceControlUDPConnected and do not gate sendUDP/receive of control traffic behind deviceUDPConnected.
464-473:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a hyphen boundary when matching groups.
device.startsWith(base.substring(0, pos))will treatkitchen-1as part of the same group askitchenette-2,stage-1as matchingstagecoach-3, etc. Grouping here is hyphen-delimited, so the prefix match needs to respect that boundary.Suggested change
- return device.startsWith(base.substring(0, pos)); + String prefix = base.substring(0, pos); + return device == prefix || device.startsWith(prefix + "-"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Modules/ModuleDevices.h` around lines 464 - 473, The current suffix-group check uses device.startsWith(base.substring(0, pos)) which can falsely match names like "kitchenette-2"; update the final condition to enforce a hyphen boundary at the split point: after finding pos (index of the hyphen in base) require that device starts with base.substring(0, pos) AND that device has a hyphen at the same position (e.g., device.length() > pos and device.charAt(pos) == '-') so only true hyphen-delimited group members match; keep the existing exact-match return (base == device) path unchanged.
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleDevices.h (1)
41-61: ⚡ Quick winAdd compile-time size checks for the full wire packets.
receiveUDP()dispatches by exactsizeof(UDPMessage)/sizeof(UDPControlMessage), and the docs now call out fixed sizes. Right now onlyUDPWLEDHeaderis guarded, so any future change toChar<32>or packing will break interoperability silently.Suggested change
struct UDPMessage { UDPWLEDHeader header; // 44 bytes — WLED-compatible Char<32> versionStr; // human-readable version string char build[16]; // build date string uint32_t uptime; uint16_t packageSize; // sizeof(UDPMessage) — used by receiver to validate packet type uint8_t brightness; uint8_t palette; uint8_t preset; } __attribute__((packed)); // Force no padding +static_assert(sizeof(UDPMessage) == 101, "UDPMessage must be exactly 101 bytes"); // MoonLight-only control message sent exclusively on port 65507. // WLED does not listen on port 65507, so it never receives these packets. struct UDPControlMessage { UDPWLEDHeader header; // 44 bytes: sender identification char targetName[32]; // unicast: receiver hostname; group broadcast: empty string uint8_t brightness; uint8_t lightsOn; uint8_t palette; uint8_t preset; } __attribute__((packed)); // Force no padding +static_assert(sizeof(UDPControlMessage) == 80, "UDPControlMessage must be exactly 80 bytes");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Modules/ModuleDevices.h` around lines 41 - 61, Add compile-time size assertions for the full wire structs by inserting static_assert checks immediately after the UDPMessage and UDPControlMessage definitions that verify sizeof(UDPMessage) == 101 and sizeof(UDPControlMessage) == 80 (these totals come from UDPWLEDHeader == 44 plus the declared field sizes), and provide clear diagnostic messages so any accidental change to Char<32>, packing, or field sizes fails at compile time; reference the UDPMessage and UDPControlMessage symbols when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/develop/devices.md`:
- Around line 24-32: Update the three fenced code blocks that show the packet
layout to include a language identifier so markdownlint MD040 is satisfied: add
"text" after each opening triple-backtick for the block starting with "byte 0:
token — must be 255 (WLED validates this)" and the two blocks beginning with
"[UDPWLEDHeader — 44 bytes]". Ensure you change only the opening fence (``` ->
```text) for those blocks so rendering is unchanged.
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 293-328: Network-sourced fixed-size string fields must be
NUL-terminated before any strcmp/logging/JSON use: after reading packets in
receiveUDP (after deviceUDP.read / deviceControlUDP.read), explicitly set the
final byte to '\0' on any char arrays like UDPWLEDHeader::name,
UDPMessage::targetName (and any other fixed-size char buffers in UDPWLEDHeader,
UDPMessage, UDPControlMessage) so they cannot run past the buffer when used by
updateDevicesWLED, updateDevices, processControlMessage or logging. Ensure you
apply this for both discovery (UDPWLEDHeader/header and UDPMessage/message) and
control (UDPControlMessage/msg) reads immediately after read returns. />
---
Outside diff comments:
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 149-170: The control UDP socket lifecycle is currently tied to
deviceUDPConnected causing control to stop if the discovery socket or its bind
(deviceUDP.begin/deviceUDPConnected) fails; update loop10s() and loop20ms() so
deviceControlUDP.begin(deviceControlUDPPort) is attempted and retried
independently of deviceUDPConnected, use deviceControlUDPConnected as its own
flag, and ensure loop20ms() still polls/receives from the control socket (and/or
calls the control-specific receive path) even when deviceUDPConnected is false;
keep existing logs (EXT_LOGD/EXT_LOGW) but reflect independent bind attempts for
deviceControlUDPConnected and do not gate sendUDP/receive of control traffic
behind deviceUDPConnected.
- Around line 464-473: The current suffix-group check uses
device.startsWith(base.substring(0, pos)) which can falsely match names like
"kitchenette-2"; update the final condition to enforce a hyphen boundary at the
split point: after finding pos (index of the hyphen in base) require that device
starts with base.substring(0, pos) AND that device has a hyphen at the same
position (e.g., device.length() > pos and device.charAt(pos) == '-') so only
true hyphen-delimited group members match; keep the existing exact-match return
(base == device) path unchanged.
---
Nitpick comments:
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 41-61: Add compile-time size assertions for the full wire structs
by inserting static_assert checks immediately after the UDPMessage and
UDPControlMessage definitions that verify sizeof(UDPMessage) == 101 and
sizeof(UDPControlMessage) == 80 (these totals come from UDPWLEDHeader == 44 plus
the declared field sizes), and provide clear diagnostic messages so any
accidental change to Char<32>, packing, or field sizes fails at compile time;
reference the UDPMessage and UDPControlMessage symbols when adding these
assertions.
🪄 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: CHILL
Plan: Pro
Run ID: 19c69f48-24e8-4883-b44c-46138ebbdccd
📒 Files selected for processing (5)
docs/develop/devices.mddocs/develop/network.mddocs/moonbase/devices.mdmkdocs.ymlsrc/MoonBase/Modules/ModuleDevices.h
… match, static_asserts, doc fences - receiveUDP(): NUL-terminate all char arrays after udp.read() to guard against non-terminated network data - loop10s/loop20ms: bind discovery and control sockets independently; poll control even when discovery is unbound - partOfGroup(): require device.charAt(pos)=='-' to prevent "kitchenette" matching the "kitchen" group - Add static_assert for UDPMessage (101 bytes) and UDPControlMessage (80 bytes) - docs/develop/devices.md: add `text` language identifier to three fenced code blocks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 295-299: Unicast controls are being treated as module-origin group
updates because update(...) is called with _moduleName as the origin, which
causes addUpdateHandler to treat it as a remote-origin and call sendUDP(true);
change the origin argument so group broadcasts still use "group" but
unicast-controlled updates use a non-module origin (e.g. "unicast" or empty
string) instead of _moduleName: modify the call to
_moduleControl->update(newState, ModuleState::update, isGroupBroadcast ? "group"
: "unicast") so addUpdateHandler won't re-broadcast unicast as group; verify
related logic in addUpdateHandler/sendUDP still interprets "group" as
suppressing rebroadcast.
- Around line 281-283: The pointer stored in myName comes from a temporary
String returned by esp32sveltekit.getSystemHostname(), leaving myName dangling
when later used (e.g., in strcmp); fix this by capturing the returned String in
a persistent local variable (e.g., myNameStr) before taking c_str(), so myName
points into a String that remains alive for the strcmp/usage; update the code
around senderName, myName and getSystemHostname() in ModuleDevices.h
accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 8be2e0c1-033b-4334-955b-91d9116573c3
📒 Files selected for processing (2)
docs/develop/devices.mdsrc/MoonBase/Modules/ModuleDevices.h
✅ Files skipped from review due to trivial changes (1)
- docs/develop/devices.md
…teral pool, and status broadcast - processControlMessage: store getSystemHostname() in String (not const char*) to fix dangling pointer that prevented unicast control from applying - addUpdateHandler: always call sendUDP(false) so remote device tables update immediately on any control change; suppress sendUDP(true) for both "group" and "unicast" origins - Apply received unicast control with origin "unicast" (not _moduleName) so addUpdateHandler skips group re-broadcast - UDPMessage::versionStr: char[32] instead of Char<32> (POD, avoids GCC packed-struct warning); update all call sites to strlcpy/direct index - onUpdate() remote path: guard on deviceControlUDPConnected and log beginPacket failures - receiveUDP() control port: guard on deviceControlUDPConnected before parsePacket loop - esp32-d0-wrover: add -mtext-section-literals to fix l32r literal target out of range linker error
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MoonBase/Modules/ModuleDevices.h (1)
128-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat self-targeted row edits as unicast, not module-origin group control.
When
targetIP == activeIP, this calls_moduleControl->update(..., _moduleName). The handler on Lines 79-84 treats any origin other than"group"/"unicast"as a local control change and callssendUDP(true), so editing the local device row fans out to the whole group while the remote-row path stays targeted. Using"unicast"here keeps the self-target path consistent with the network unicast path and avoids accidental group propagation.Suggested fix
- _moduleControl->update(newState, ModuleState::update, _moduleName); // Do not add server in the originID as that blocks updates, see execOnUpdate + _moduleControl->update(newState, ModuleState::update, "unicast");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Modules/ModuleDevices.h` around lines 128 - 135, When handling self-targeted UDP control (where targetIP == activeIP), don't pass the module's name as the origin to _moduleControl->update (which causes the handler to treat it as a local/group-origin change); instead use the unicast origin identifier so the update is treated as a unicast/network-targeted change. Locate the block that calls messageToControlState(...) and _moduleControl->update(newState, ModuleState::update, _moduleName) and change the origin argument to the equivalent "unicast" origin string used elsewhere so the handler will not call sendUDP(true) and will keep the self-targeted edit as unicast.
🧹 Nitpick comments (1)
firmware/esp32-d0.ini (1)
53-59: ⚡ Quick winUncomment and update the platform version to ensure toolchain stability.
The recommendation to pin the platform is already noted in line 48, but line 49 remains commented. Since
-mtext-section-literalsis a compiler-specific Xtensa flag and now required for livescripts, uncomment the platform pin and update it toespressif32@6.13.0(or verify against the latest stable release) to prevent toolchain drift and ensure reproducible builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/esp32-d0.ini` around lines 53 - 59, Uncomment and set the Platform pin to a fixed espressif32 toolchain version to avoid toolchain drift: locate the existing commented platform pin near the build_flags block (the same env that uses -mtext-section-literals and build_flags) and replace the commented line with platform = espressif32@6.13.0 (or the latest verified stable), then save so the -mtext-section-literals flag and livescripts build_flags use a reproducible toolchain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 128-135: When handling self-targeted UDP control (where targetIP
== activeIP), don't pass the module's name as the origin to
_moduleControl->update (which causes the handler to treat it as a
local/group-origin change); instead use the unicast origin identifier so the
update is treated as a unicast/network-targeted change. Locate the block that
calls messageToControlState(...) and _moduleControl->update(newState,
ModuleState::update, _moduleName) and change the origin argument to the
equivalent "unicast" origin string used elsewhere so the handler will not call
sendUDP(true) and will keep the self-targeted edit as unicast.
---
Nitpick comments:
In `@firmware/esp32-d0.ini`:
- Around line 53-59: Uncomment and set the Platform pin to a fixed espressif32
toolchain version to avoid toolchain drift: locate the existing commented
platform pin near the build_flags block (the same env that uses
-mtext-section-literals and build_flags) and replace the commented line with
platform = espressif32@6.13.0 (or the latest verified stable), then save so the
-mtext-section-literals flag and livescripts build_flags use a reproducible
toolchain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57f56c83-40f3-41cf-9e9f-300409358fc8
📒 Files selected for processing (2)
firmware/esp32-d0.inisrc/MoonBase/Modules/ModuleDevices.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit