Super sync part 1 #114
Super sync part 1 #114
Conversation
backend ======= - main: moduleLightsControl added as argument to ModuleDevices - Devices: extend UDPMessage with version, uptime, brightness, palette, preset, add LightsControl->addUpdateHandler sending UDP message when something changes, updateDevices: send new vars
WalkthroughModuleDevices was extended to carry and broadcast lights state (version, uptime, brightness, palette, preset), its constructor now accepts a ModuleLightsControl* dependency, UDP read/write and device JSON update paths were updated, and main loop timing/initialization adjusted to wire the new dependency. Changes
Sequence DiagramsequenceDiagram
participant MC as ModuleLightsControl
participant MD as ModuleDevices
participant UDP as UDP Broadcast
participant DJ as Device JSON / Devices Map
MC->>MD: notify lights update (brightness,palette,preset)
activate MD
MD->>MD: build UDPMessage (hostname,version,uptime,brightness,palette,preset)
MD->>UDP: broadcast UDPMessage
deactivate MD
UDP->>MD: incoming UDPMessage (from others)
activate MD
MD->>DJ: updateDevices(UDPMessage, IPAddress)
DJ->>DJ: create/update device JSON (name,version,uptime,brightness,palette,preset)
deactivate MD
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Fix all issues with AI agents
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Around line 47-54: The code is assigning the whole JSON object
state.data["preset"] to message.preset (and logging it), which when cast to
uint8_t becomes 0; instead extract the selected preset field (e.g., the numeric
property like "selected" or "value") from the JSON object and assign that to
message.preset, and log that numeric value rather than serializing the whole
object; update the lambda that handles ModuleState (references: ModuleState,
state.data["preset"], message.preset, serializeJson, EXT_LOGD, MB_TAG) to read
the correct subfield and convert it to uint8_t before assignment and logging
(apply the same change to the other occurrences around lines 188-191).
- Line 58: The assignment to message.uptime currently computes time(nullptr) -
millis()/1000 which yields the boot timestamp and can underflow; update the two
locations where message.uptime is set (the message.uptime assignments in
ModuleDevices.h) to assign uptime seconds directly as millis()/1000 if you
intend to store uptime, or if you intended to store the boot epoch rename the
field (e.g., bootTime) and then compute boot time as time(nullptr) -
(millis()/1000); also ensure any consumer code uses the new semantics/field
name.
- Around line 169-175: The UDPMessage struct is partially uninitialized when
packetSize < sizeof(UDPMessage); before memcpy into UDPMessage (symbol: message)
zero-initialize it (e.g., memset(&message, 0, sizeof(message)) or
value-initialize UDPMessage message) so any trailing fields like
version/brightness/palette/preset are deterministic, then perform deviceUDP.read
into buffer and memcpy(&message, buffer, min(packetSize, sizeof(message))).
Ensure you still use ::min(packetSize, sizeof(message)) for the copy length.
| [&](ModuleState& state) { | ||
| message.brightness = state.data["brightness"]; | ||
| message.palette = state.data["palette"]; | ||
| message.preset = state.data["preset"]; | ||
| String ddd; | ||
| serializeJson(state.data["preset"], ddd); | ||
| EXT_LOGD(MB_TAG, "pr %s", ddd.c_str()); | ||
| }, |
There was a problem hiding this comment.
Send the selected preset value, not the preset object.
state.data["preset"] is a JSON object; converting it to uint8_t yields 0. Use the selected preset field instead.
🔧 Suggested fix
- message.preset = state.data["preset"];
+ auto preset = state.data["preset"]["selected"];
+ if (preset.isNull()) preset = state.data["preset"]["select"];
+ message.preset = preset.as<uint8_t>();- message.preset = state.data["preset"];
+ auto preset = state.data["preset"]["selected"];
+ if (preset.isNull()) preset = state.data["preset"]["select"];
+ message.preset = preset.as<uint8_t>();Also applies to: 188-191
🤖 Prompt for AI Agents
In `@src/MoonBase/Modules/ModuleDevices.h` around lines 47 - 54, The code is
assigning the whole JSON object state.data["preset"] to message.preset (and
logging it), which when cast to uint8_t becomes 0; instead extract the selected
preset field (e.g., the numeric property like "selected" or "value") from the
JSON object and assign that to message.preset, and log that numeric value rather
than serializing the whole object; update the lambda that handles ModuleState
(references: ModuleState, state.data["preset"], message.preset, serializeJson,
EXT_LOGD, MB_TAG) to read the correct subfield and convert it to uint8_t before
assignment and logging (apply the same change to the other occurrences around
lines 188-191).
src/MoonBase/Modules/ModuleDevices.h
Outdated
| _moduleName); | ||
|
|
||
| EXT_LOGD(MB_TAG, "b: %d pa %d pr %d", message.brightness, message.palette, message.preset); | ||
| message.uptime = time(nullptr) - millis() / 1000; |
There was a problem hiding this comment.
Uptime calculation returns boot time (and can underflow).
time(nullptr) - millis()/1000 yields the boot timestamp, not uptime, and can underflow before time is set. If you want uptime seconds, use millis()/1000; if you want boot time, rename the field.
🔧 Suggested fix
- message.uptime = time(nullptr) - millis() / 1000;
+ message.uptime = millis() / 1000;- message.uptime = time(nullptr) - millis() / 1000;
+ message.uptime = millis() / 1000;Also applies to: 194-194
🤖 Prompt for AI Agents
In `@src/MoonBase/Modules/ModuleDevices.h` at line 58, The assignment to
message.uptime currently computes time(nullptr) - millis()/1000 which yields the
boot timestamp and can underflow; update the two locations where message.uptime
is set (the message.uptime assignments in ModuleDevices.h) to assign uptime
seconds directly as millis()/1000 if you intend to store uptime, or if you
intended to store the boot epoch rename the field (e.g., bootTime) and then
compute boot time as time(nullptr) - (millis()/1000); also ensure any consumer
code uses the new semantics/field name.
|
@coderabbitai, for some reason the package send in _moduleLightsControl->addUpdateHandler arrives very late (in the next 10s update?) in another device, which calls readUDP(), calling updateDevices. The 10s message (mostly... packages missed?) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24698 🏁 Script executed: Length of output: 4693 🏁 Script executed: Length of output: 23214 🏁 Script executed: Length of output: 766 🏁 Script executed: Length of output: 4475 🏁 Script executed: Length of output: 2683
Root CauseWhen However, void readUDP() {
size_t packetSize = deviceUDP.parsePacket(); // ← Only called ONCE
if (packetSize >= 38) {
// ... process one packet
updateDevices(message, deviceUDP.remoteIP());
}
}If 10 packets are queued, it takes 10 seconds to drain them all at 1 packet/second, which explains the "next 10s update" delay you're seeing. SolutionDrain all available packets in void readUDP() {
// Process all available UDP packets in the buffer
while (size_t packetSize = deviceUDP.parsePacket()) {
if (packetSize >= 38) { // WLED has 44, MM had ! 38
char buffer[packetSize];
UDPMessage message;
deviceUDP.read(buffer, packetSize);
memcpy(&message, buffer, ::min(packetSize, sizeof(message)));
// EXT_LOGD(ML_TAG, "UDP packet read from %d: %s (%d)", deviceUDP.remoteIP()[3], buffer + 6, packetSize);
updateDevices(message, deviceUDP.remoteIP());
}
}
}This will process all queued packets immediately instead of one per second, eliminating the delay. |
backend ======= - main: add last20ms and use for devices - devices: loop20 for direct response, readudp loops over received packets, use preset.selected
|
@coderabbitai, can you review latest commit: ced9a76 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7649
✅ Core Fix: readUDP() While LoopPerfect implementation of the suggested fix: while (size_t packetSize = deviceUDP.parsePacket()) {
if (packetSize >= 38) {
// ... process packet
}
}This drains all queued packets immediately instead of one per call, eliminating the backlog delay. 🚀 Major Improvement: 20ms Response TimeRenaming
📝 Other Improvements
|
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.