Add bidirectional ESP-NOW API#5737
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an ESP32 ESP-NOW transport and bidirectional JSON API with fragmentation, discovery, live streaming, state pushes, linked-remote validation, network rebinding, and connection handling. It also centralizes binary live LED payload construction for ESP-NOW and WebSocket consumers. ChangesESP-NOW protocol and transport
Fragmented JSON API
Network and synchronization integration
Shared live LED payloads
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Remote
participant ESPNowTransport
participant WLEDLoop
participant EspNowApi
participant WLEDState
Remote->>ESPNowTransport: Send fragmented REQUEST
ESPNowTransport->>WLEDLoop: Queue frame for loop processing
WLEDLoop->>EspNowApi: Reassemble and handle request
EspNowApi->>WLEDState: Apply command or read state
EspNowApi->>ESPNowTransport: Queue response, LIVE, or PUSH
ESPNowTransport->>Remote: Send ESP-NOW fragments
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/fcn_declare.h (1)
284-328: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winKeep
espNowGetDetectedRemotes()inside the ESP-NOW guard.When
WLED_DISABLE_ESPNOWis defined,EspNowDetectedRemoteis not declared but Line 328 still references it, breaking compilation.Proposed fix
void espNowApiOnSendResult(uint8_t* address, uint8_t status); +size_t espNowGetDetectedRemotes(EspNowDetectedRemote* remotes, size_t capacity); `#endif` - -size_t espNowGetDetectedRemotes(EspNowDetectedRemote* remotes, size_t capacity);🤖 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 `@wled00/fcn_declare.h` around lines 284 - 328, Ensure the declaration of espNowGetDetectedRemotes() remains within the `#ifndef` WLED_DISABLE_ESPNOW guard in the UDP declarations section, alongside EspNowDetectedRemote and the other ESP-NOW symbols, so it is omitted when ESP-NOW is disabled.
🤖 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/espnow-json-protocol.md`:
- Line 120: Update the four unlabeled code fences in the protocol example
section of espnow-json-protocol.md to use the text language tag, including the
fences at the referenced locations, so each opening fence is labeled as text.
- Around line 118-142: Update the request-response examples in the protocol
documentation: describe state-changing requests as receiving {"success":true}
followed by a PUSH frame instead of a verbose RESPONSE. Correct the LIVE frame
scheduling description to state that frames are sent every 100 ms, while
preserving the existing live payload reassembly details.
In `@wled00/espnow_api.cpp`:
- Around line 247-250: Move the apiReasmLast update in the fragment reassembly
flow to occur only after the apiReasmFlags duplicate check passes, so repeated
duplicate fragments cannot extend the reassembly lifetime. Preserve the existing
duplicate early return and update the timestamp only for newly accepted
fragments.
- Line 8: Replace the fixed ESPNOW_API_STRIPWAIT_TIMEOUT constant and its uses
in the ESP-NOW strip-wait logic with timing derived from strip.getFrameTime().
Ensure the wait duration tracks the current strip frame time so slower bus
updates complete before state changes proceed.
In `@wled00/espnow_transport.cpp`:
- Around line 72-95: Ensure every ESP-NOW send completion is consumed before
another transmission begins. Update onEspNowSent and the serviceTransmit flow to
prevent callbacks from being dropped or overwriting sentAddress/sentStatus while
sentEventPending is occupied; gate new sends until the pending completion is
processed, including the corresponding legacy and non-legacy paths.
In `@wled00/json.cpp`:
- Around line 1442-1444: Update the RGB channel assignments in the live-preview
conversion so the white-channel-mapped values are scaled by the global
brightness, matching the existing live JSON path. Preserve the zero output when
bri is zero and apply the same brightness handling consistently to R, G, and B
in the affected preview path.
In `@wled00/wled.cpp`:
- Around line 31-40: Update the ESP-NOW initialization branch around
Network.isConnected() to check Network.isEthernet() explicitly rather than
treating Ethernet as Wi-Fi STA connectivity. For Ethernet, ensure a valid Wi-Fi
interface and channel are established or fall back to the existing AP
initialization path before calling espNowTransportBegin; preserve the current
STA behavior for Wi-Fi connections.
- Around line 127-129: Reorder the main service loop so handleEspNowTransport()
runs before handleConnection(), allowing queued ESP-NOW activity to mark the
remote active before connection handling can stop or rebind the transport. Keep
handleRemote() and handleEspNowApi() in their existing relative order unless
required by this move.
---
Outside diff comments:
In `@wled00/fcn_declare.h`:
- Around line 284-328: Ensure the declaration of espNowGetDetectedRemotes()
remains within the `#ifndef` WLED_DISABLE_ESPNOW guard in the UDP declarations
section, alongside EspNowDetectedRemote and the other ESP-NOW symbols, so it is
omitted when ESP-NOW is disabled.
🪄 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: 47faf682-d3a2-4365-b226-0970436e6e01
📒 Files selected for processing (17)
docs/espnow-json-protocol.mdwled00/const.hwled00/data/settings_sync.htmwled00/data/settings_wifi.htmwled00/espnow_api.cppwled00/espnow_transport.cppwled00/fcn_declare.hwled00/json.cppwled00/led.cppwled00/remote.cppwled00/set.cppwled00/udp.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/ws.cppwled00/xml.cpp
💤 Files with no reviewable changes (1)
- wled00/wled.h
| buffer[pos++] = bri ? qadd8(w, r) : 0; //R, add white channel to RGB channels as a simple RGBW -> RGB map | ||
| buffer[pos++] = bri ? qadd8(w, g) : 0; //G | ||
| buffer[pos++] = bri ? qadd8(w, b) : 0; //B |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply global brightness to binary live-preview colors.
These channels are emitted at full intensity whenever bri is nonzero. This differs from the existing live JSON path and makes both WebSocket and ESP-NOW previews inaccurate at reduced brightness.
Proposed fix
- buffer[pos++] = bri ? qadd8(w, r) : 0;
- buffer[pos++] = bri ? qadd8(w, g) : 0;
- buffer[pos++] = bri ? qadd8(w, b) : 0;
+ const uint8_t brightness = strip.getBrightness();
+ buffer[pos++] = scale8(qadd8(w, r), brightness);
+ buffer[pos++] = scale8(qadd8(w, g), brightness);
+ buffer[pos++] = scale8(qadd8(w, b), brightness);📝 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.
| buffer[pos++] = bri ? qadd8(w, r) : 0; //R, add white channel to RGB channels as a simple RGBW -> RGB map | |
| buffer[pos++] = bri ? qadd8(w, g) : 0; //G | |
| buffer[pos++] = bri ? qadd8(w, b) : 0; //B | |
| const uint8_t brightness = strip.getBrightness(); | |
| buffer[pos++] = scale8(qadd8(w, r), brightness); //R, add white channel to RGB channels as a simple RGBW -> RGB map | |
| buffer[pos++] = scale8(qadd8(w, g), brightness); //G | |
| buffer[pos++] = scale8(qadd8(w, b), brightness); //B |
🤖 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 `@wled00/json.cpp` around lines 1442 - 1444, Update the RGB channel assignments
in the live-preview conversion so the white-channel-mapped values are scaled by
the global brightness, matching the existing live JSON path. Preserve the zero
output when bri is zero and apply the same brightness handling consistently to
R, G, and B in the affected preview path.
| if (Network.isConnected()) { | ||
| DEBUG_PRINTLN(F("ESP-NOW initing in STA mode.")); | ||
| espNowOK = espNowTransportBegin(WiFi.channel(), false); | ||
| espNowUsingAP = false; | ||
| } else if (apActive) { | ||
| DEBUG_PRINTLN(F("ESP-NOW initing in AP mode.")); | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| esp_wifi_set_bandwidth(WIFI_IF_AP, WIFI_BW_HT20); | ||
| #endif | ||
| espNowOK = espNowTransportBegin(apChannel, true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not treat Ethernet connectivity as Wi-Fi STA connectivity.
Network.isConnected() is also true for Ethernet, but this branch then uses WiFi.channel() and binds ESP-NOW to WIFI_IF_STA. Explicitly handle Network.isEthernet() so Ethernet builds establish a valid Wi-Fi interface/channel or fall back to AP before starting ESP-NOW.
🤖 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 `@wled00/wled.cpp` around lines 31 - 40, Update the ESP-NOW initialization
branch around Network.isConnected() to check Network.isEthernet() explicitly
rather than treating Ethernet as Wi-Fi STA connectivity. For Ethernet, ensure a
valid Wi-Fi interface and channel are established or fall back to the existing
AP initialization path before calling espNowTransportBegin; preserve the current
STA behavior for Wi-Fi connections.
|
thanks for contributing, there may be some useful features but I have a few concerns: |
|
Thanks. Yes, I’ve checked the full implementation (and tested it for several days on multiple WLED instances, no issues so far). I’m addressing the issues that CodeRabbit pointed out at the moment. I wasn’t aware of #5624 while developing this. This branch replaces QuickESPNow usage with its own native transport because it needs unicast peer management, queued fragmented transfers, etc. The old lib_deps entry for the QuickEspNow lib is indeed now unused and can be removed. |
…work detection, and transport servicing. Correct the protocol documentation and remove the unused QuickESPNow dependency.
|
I had a quick go at this, I like the new UI features but I think it needs to be trimmed down a little, like entering a custom mac manually I see no real usecase for. the huge chunk of additional ram use is a nogo and the penalty in flash size is also hard to justify. It all needs to be trimmed down to keep only the most broadly applicable features. Everything else needs to go in a usermod. I am not sure which features those are though. edit: just to put it into perspective, compared #5624 on an ESP8266 running this PR eats up 3k of RAM and 13k of flash. On ESP32 flash use is a bit less, more like 8k but RAM is about the same. |
These are valid concerns. I have made some changes (without affecting usability) that trimmed the implementation as much as possible:
All below values are differences against the PR’s main-branch base using the same build configs: ESP32
ESP8266
On ESP8266, the API itself occupies ~276 bytes of RAM and ~6.8 KB of flash. My current implementation enables the API by default on the ESP32 family and makes it opt-in on ESP8266 with If the new UI ESP-NOW features are still considered too heavy, reverting those changes will shave off about ~1.5 KB flash. That'd bring the approximate increase down to 4.8 KB on ESP32 and 1.5 KB on ESP8266 with the API disabled, with some additional savings possible by removing the supporting detected-device list code. Please let me know your preference on two points:
For ESP32, the complete implementation adds 6,420 B of flash (+0.49%) and 72 B of static RAM. I think that is a reasonable "cost" for a complete, plug-and-play ESP-NOW API. Please let me know what direction you think is best and I'll push the necessary changes. |
|
see the builds failing? that is the issue. |
|
I’ve completed a final reduction:
Compared with
The ESP01 build now uses 885,699/892,912 bytes, leaving 7,213 bytes available. All three of the failed ESP01 environments build successfully now. I tested runtime heap usage on a physical ESP32. Enabling ESP-NOW with a linked remote reduced the minimum free heap by 2,868 bytes and the largest contiguous block by 4,096 bytes. Current free heap remained around 110–112 KB. Running live preview caused no further reduction in either measurement, no progressiv heap loss observed either. At this point, this is the practical minimum for providing a useful ESP-NOW API for the ESP32. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/udp.cpp (2)
1069-1089: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject incomplete segment data before parsing.
Packet zero accepts payloads containing a partial segment. It may also declare more segments than were assembled, causing
parseNotifyPacket()to consume stale or uninitialized bytes fromudpIn.Require
(payloadLen - SEG_OFFSET) % UDP_SEG_SIZE == 0, and before parsing verifyudpIn[39] == segsReceivedandudpIn[40] == UDP_SEG_SIZE.As per path instructions, “ESP-NOW raw messages input” is an untrusted ingress point, so validation belongs here.
🤖 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 `@wled00/udp.cpp` around lines 1069 - 1089, Update the ESP-NOW sync-packet validation before the packet-zero memcpy and parsing flow to reject any payload where `(payloadLen - SEG_OFFSET)` is not divisible by `UDP_SEG_SIZE`. Before `parseNotifyPacket()` consumes `udpIn`, validate that `udpIn[39]` equals `segsReceived` and `udpIn[40]` equals `UDP_SEG_SIZE`; on failure, discard the assembled buffer and reset the receive state using the existing malformed-packet path.Source: Path instructions
1015-1038: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize persisted ESP-NOW MACs on load
cfg.cppstoreslinked_remotevalues verbatim, so legacy uppercase entries still fail the case-sensitivestrcmp()inudp.cpp. Lowercase them during deserialization; the UI-only normalization does not cover existing configs.🤖 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 `@wled00/udp.cpp` around lines 1015 - 1038, Normalize each persisted linked_remote MAC to lowercase during configuration deserialization in cfg.cpp, before the values are stored in linked_remotes. Preserve the existing MAC content while converting uppercase hexadecimal characters so udp.cpp’s knownRemote comparison against senderMac succeeds for legacy configurations.
🤖 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 `@wled00/udp.cpp`:
- Line 240: Update the packet-count calculation in the segment loop around
buffer.noOfPackets so it counts whole segments per packet rather than dividing
total segment bytes by bufferSize. Ensure the 18-segment case with a
first-packet capacity of 5 and subsequent capacity of 6 produces 4 packets,
preserving the existing first-packet handling.
---
Outside diff comments:
In `@wled00/udp.cpp`:
- Around line 1069-1089: Update the ESP-NOW sync-packet validation before the
packet-zero memcpy and parsing flow to reject any payload where `(payloadLen -
SEG_OFFSET)` is not divisible by `UDP_SEG_SIZE`. Before `parseNotifyPacket()`
consumes `udpIn`, validate that `udpIn[39]` equals `segsReceived` and
`udpIn[40]` equals `UDP_SEG_SIZE`; on failure, discard the assembled buffer and
reset the receive state using the existing malformed-packet path.
- Around line 1015-1038: Normalize each persisted linked_remote MAC to lowercase
during configuration deserialization in cfg.cpp, before the values are stored in
linked_remotes. Preserve the existing MAC content while converting uppercase
hexadecimal characters so udp.cpp’s knownRemote comparison against senderMac
succeeds for legacy configurations.
🪄 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: d726a1d8-09b5-4a66-9905-c136ace0053b
📒 Files selected for processing (11)
docs/espnow-json-protocol.mdwled00/const.hwled00/data/settings_wifi.htmwled00/espnow_api.cppwled00/espnow_transport.cppwled00/fcn_declare.hwled00/led.cppwled00/udp.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/led.cpp
- docs/espnow-json-protocol.md
- wled00/wled.cpp
- wled00/espnow_api.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/udp.cpp (1)
240-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd AI markers for the generated logic.
The packet count calculation correctly resolves the byte-division undercounting issue. Since this snippet was generated by an AI in a previous review, consider wrapping it with the required AI tracking markers. As per coding guidelines, AI-generated source code blocks must be marked with
// AI: below section was generated by an AIand// AI: endcomments.💡 Proposed optional refactor
+ // AI: below section was generated by an AI if (s > firstSegment) { const size_t segmentsPerPacket = bufferSize / UDP_SEG_SIZE; const size_t remaining = s - firstSegment; buffer.noOfPackets += (remaining + segmentsPerPacket - 1) / segmentsPerPacket; } + // AI: end🤖 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 `@wled00/udp.cpp` around lines 240 - 244, Wrap the generated packet-count calculation within the if (s > firstSegment) block with the required AI tracking comments: place the start marker immediately before the generated logic and the end marker immediately after it, preserving the existing calculation unchanged.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.
Nitpick comments:
In `@wled00/udp.cpp`:
- Around line 240-244: Wrap the generated packet-count calculation within the if
(s > firstSegment) block with the required AI tracking comments: place the start
marker immediately before the generated logic and the end marker immediately
after it, preserving the existing calculation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a0413f5-0a0a-4418-ab43-30874fc65ecb
📒 Files selected for processing (1)
wled00/udp.cpp
| // toggle visibility of ESP-NOW remote list based on checkbox state | ||
| function tE() { | ||
| // keep the hidden input with MAC addresses, only toggle visibility of the list UI | ||
| gId('rlc').style.display = d.Sf.RE.checked ? 'block' : 'none'; |
There was a problem hiding this comment.
Why did you remove the comment?
There was a problem hiding this comment.
I removed it as part of an earlier work on the web UI, which was later scrapped to preserve flash.
There was a problem hiding this comment.
Better to create a sub-folder like /docs/design and place this file into the sub-folder.
| | Value | Name | Direction | Payload | | ||
| |------:|------------|------------------|---------| | ||
| | 0x01 | `REQUEST` | remote → WLED | A JSON command (same as a `/json/state` body). `{"v":true}` requests current state without changing anything. `{"lv":true}` starts live LED peek frames (keepalive, see below); `{"lv":false}` stops them. | | ||
| | 0x02 | `RESPONSE` | WLED → remote | Reply to a `REQUEST`. `{"state":…,"info":…}` when verbose (the request set `"v":true`), otherwise `{"success":true}`. When the request **changed** state, a compact broadcast `PUSH` is imminent, so the direct reply is `{"success":true}` even if `"v":true` was set (same rule as the WebSocket API). On error, `{"error":<code>}` - see the error codes below. `msgId` echoes the request. | |
There was a problem hiding this comment.
Is it really worth to spend around 10% of the allowed esp-now payload size just for the word 'RESPONSE'?
There was a problem hiding this comment.
Did you read the code? Only 1 byte (0x02) is included in the payload. The word 'RESPONSE' is only used for debugging messages.
| l.appendChild(r); | ||
| rC++; | ||
| gId('+').style.display = gId("rml").childElementCount < 10 ? 'inline' : 'none'; // can't append to list anymore, hide button | ||
| mac = String(mac).toLowerCase(); |
There was a problem hiding this comment.
How about adding some comments to explain the code you added?
| Enable ESP-NOW: <input type="checkbox" name="RE" onchange="tE()"><br> | ||
| <i>Listen for events over ESP-NOW<br> | ||
| <i>Listen for events over ESP-NOW.<br> | ||
| Keep disabled if not using a remote or ESP-NOW sync, increases power consumption.<br></i> |
| // stuff as many segments in first packet as possible (normally up to 5) | ||
| for (size_t i = 0; packetSize < bufferSize && i < s; i++) { | ||
| memcpy(buffer.data + packetSize, &udpOut[41+i*UDP_SEG_SIZE], UDP_SEG_SIZE); | ||
| for (size_t i = 0; packetSize + UDP_SEG_SIZE <= bufferSize && i < s; i++) { |
There was a problem hiding this comment.
Same here: don't delete comments, and don't "beautify" unrelated code.
| if (s > s0) buffer.noOfPackets += 1 + ((s - s0) * UDP_SEG_SIZE) / bufferSize; // set number of packets | ||
| auto err = quickEspNow.send(ESPNOW_BROADCAST_ADDRESS, reinterpret_cast<const uint8_t*>(&buffer), packetSize+3); | ||
| if (!err && s0 < s) { | ||
| // send rest of the segments |
There was a problem hiding this comment.
Another lost comment. Please restore it.
| // and wait until at least one space is available in the buffer | ||
| for (size_t i = s0; i < s; i++) { | ||
| memcpy(buffer.data + packetSize, &udpOut[41+i*UDP_SEG_SIZE], UDP_SEG_SIZE); | ||
| for (size_t i = firstSegment; i < s; i++) { |
There was a problem hiding this comment.
Sorry but deleting comments and pretty-formatting unrelated code is becomg a no-go for your PR.
| if (enableESPNow) { | ||
| quickEspNow.onDataSent(espNowSentCB); // see udp.cpp | ||
| quickEspNow.onDataRcvd(espNowReceiveCB); // see udp.cpp | ||
| quickEspNow.onDataSent(espNowSentCB); |
There was a problem hiding this comment.
Another deleted comment. Please explain.
| espNowOK = quickEspNow.begin(apChannel, WIFI_IF_AP); | ||
| } else { | ||
| DEBUG_PRINTLN(F("ESP-NOW initing in STA mode.")); | ||
| espNowOK = quickEspNow.begin(); // Use no parameters to start ESP-NOW on same channel as WiFi, in STA mode |
There was a problem hiding this comment.
Aaaaaaaahhhhhh. I'm getting a comment-deleted-syndrome 😅
softhack007
added
the
AI
@figamore "checked and tested" is not the same as "reviewed and fully understood" (testing is always good). Please make sure to add AI markers to the code you haven't scrutinised personally. This helps maintainers to focus their attention on typical AI mistakes. |
|
@coderabbitai full review, including the AI code special criteria you find in your .coderabbit.yaml file. |
|
✅ Action performedFull review finished. |
|
@softhack007 thanks for looking through it, I only briefly glanced over it since it has an AI slop feel. There are some useful features but I fail to see the general usefulness of bi-directionality other than supporting the CYD remote which this appears to be tailored to. |
|
@DedeHai yeah, good point about "what's the added value". Its also not clear to me when bi-directional esp-now is a benefit over running on normal WiFi, especially when sender and receiver require a common WiFi channel to function reliably 🤔 Maybe I missed something ... if WiFi or AP connection must exists for stability reasons, why not use regular UDP ? |
@coderabbitai it looks like an error occurred, please try again. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
wled00/espnow_transport.cpp (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable ESP8266 transport branches.
The file-level guard requires
ARDUINO_ARCH_ESP32, so every internal#ifdef ESP8266branch is dead. Remove those branches or change the outer platform contract if ESP8266 compilation was intended.As per path instructions, check for and remove dead or disabled code.
Also applies to: 63-99, 124-142, 155-161, 178-182
🤖 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 `@wled00/espnow_transport.cpp` at line 2, Remove the unreachable ESP8266-specific branches and disabled code throughout espnow_transport.cpp, including the sections around the file guard and the referenced ranges. Keep the implementation consistently ESP32-only under the existing ARDUINO_ARCH_ESP32 contract; do not broaden the outer guard unless ESP8266 support is explicitly intended.Source: Path instructions
🤖 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 `@wled00/espnow_api.cpp`:
- Around line 369-377: Update the JSON serialization in the main-segment
response to emit the actual segment ID returned by strip.getMainSegment()
instead of hardcoding "mainseg":0. Include that same segment identifier in the
serialized segment object so companion remotes associate the controls with the
configured main segment.
- Around line 4-6: Wrap the generated implementation in wled00/espnow_api.cpp
from the indicated opening section through its closing guard with the prescribed
AI-generated source start and end comments. Apply the same markers around the
generated implementation in wled00/espnow_transport.cpp, covering the full
implementation through its closing guard.
In `@wled00/json.cpp`:
- Around line 1396-1447: Add the required AI-generation markers immediately
before and after the complete buildLiveLedsPayload function body, using the
exact “AI: below section was generated by an AI” and “AI: end” comment text. Do
not alter the function’s payload logic or surrounding code.
- Around line 1410-1435: Update the matrix sampling loop after the buffer header
setup to iterate over the advertised sampled width and height, using explicit
x/y coordinates and mapping each sample to source coordinates x*n and y*n.
Ensure odd widths with n > 1 emit exactly Segment::maxWidth/n samples per row,
matching the header dimensions, and remove the current linear i-based
row-skipping logic.
In `@wled00/udp.cpp`:
- Around line 23-91: Wrap each specified AI-generated block with the exact
markers "// AI: below section was generated by an AI" before the block and "//
AI: end" after it. Apply this to wled00/udp.cpp ranges 23-91, 993-1014, and
1073-1093; wled00/wled.cpp ranges 17-47 and 981-1041;
wled00/data/settings_wifi.htm range 160-189; and wled00/set.cpp range 137-150,
preserving the existing implementation.
- Around line 1073-1093: Update the packet validation in the ESP-NOW sync
handling before the packet-zero memcpy to reject payloads larger than
WLEDPACKETSIZE and require packet-zero’s segment portion to be aligned to
UDP_SEG_SIZE, while preserving the existing minimum SEG_OFFSET check. Replace
the udpIn malloc call with d_malloc() and keep the existing failure cleanup
behavior.
---
Nitpick comments:
In `@wled00/espnow_transport.cpp`:
- Line 2: Remove the unreachable ESP8266-specific branches and disabled code
throughout espnow_transport.cpp, including the sections around the file guard
and the referenced ranges. Keep the implementation consistently ESP32-only under
the existing ARDUINO_ARCH_ESP32 contract; do not broaden the outer guard unless
ESP8266 support is explicitly intended.
🪄 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: 12924bc2-850b-4971-b5fd-393604a11295
📒 Files selected for processing (17)
docs/espnow-json-protocol.mdplatformio_override.sample.iniwled00/const.hwled00/data/settings_sync.htmwled00/data/settings_wifi.htmwled00/espnow_api.cppwled00/espnow_transport.cppwled00/fcn_declare.hwled00/json.cppwled00/led.cppwled00/remote.cppwled00/set.cppwled00/udp.cppwled00/wled.cppwled00/wled.hwled00/ws.cppwled00/xml.cpp
💤 Files with no reviewable changes (1)
- platformio_override.sample.ini
| // Bidirectional JSON transport for linked ESP-NOW remotes. Frames are fragmented to fit | ||
| // the 250-byte ESP-NOW payload limit | ||
| // Received frames are reassembled in loop context before touching JSON, FS or LED state. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required AI-generated source markers to both implementations.
wled00/espnow_api.cpp#L4-L6: wrap the generated implementation through the closing guard.wled00/espnow_transport.cpp#L5-L7: wrap the generated implementation through the closing guard.
As per coding guidelines, AI-generated source blocks must use the prescribed start and end comments.
📍 Affects 2 files
wled00/espnow_api.cpp#L4-L6(this comment)wled00/espnow_transport.cpp#L5-L7
🤖 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 `@wled00/espnow_api.cpp` around lines 4 - 6, Wrap the generated implementation
in wled00/espnow_api.cpp from the indicated opening section through its closing
guard with the prescribed AI-generated source start and end comments. Apply the
same markers around the generated implementation in wled00/espnow_transport.cpp,
covering the full implementation through its closing guard.
Sources: Coding guidelines, Path instructions
| // Binary liveview payload shared by the WebSocket liveview and the ESP-NOW API: | ||
| // 'L', version (2 = 2D with width/height bytes), then sampled RGB triples. | ||
| // Call with buffer == nullptr to get the required buffer size, then again to fill it; | ||
| // returns 0 when there is nothing to serve or the buffer is too small. | ||
| size_t buildLiveLedsPayload(uint8_t* buffer, size_t bufSize, size_t maxLeds) | ||
| { | ||
| size_t used = strip.getLengthTotal(); | ||
| if (!used || !maxLeds) return 0; | ||
| size_t n = ((used -1)/maxLeds) +1; //only serve every n'th LED if count over maxLeds | ||
| size_t pos = 2; // start of data | ||
| size_t count = used/n; | ||
| #ifndef WLED_DISABLE_2D | ||
| if (strip.isMatrix) { | ||
| // ignore anything behind matrix (i.e. extra strip) | ||
| used = Segment::maxWidth*Segment::maxHeight; // always the size of matrix (more or less than strip.getLengthTotal()) | ||
| n = 1; | ||
| if (used > maxLeds) n = 2; | ||
| if (used > maxLeds*4) n = 4; | ||
| pos = 4; | ||
| count = (Segment::maxWidth/n) * (Segment::maxHeight/n); // matches the advertised dimensions | ||
| } | ||
| #endif | ||
| size_t needed = pos + count*3; | ||
| if (!buffer) return needed; | ||
| if (bufSize < needed) return 0; | ||
|
|
||
| buffer[0] = 'L'; | ||
| buffer[1] = 1; //version | ||
| #ifndef WLED_DISABLE_2D | ||
| if (strip.isMatrix) { | ||
| buffer[1] = 2; //version | ||
| buffer[2] = Segment::maxWidth/n; | ||
| buffer[3] = Segment::maxHeight/n; | ||
| } | ||
| #endif | ||
|
|
||
| for (size_t i = 0; pos < needed; i += n) | ||
| { | ||
| #ifndef WLED_DISABLE_2D | ||
| if (strip.isMatrix && n>1 && (i/Segment::maxWidth)%n) i += Segment::maxWidth * (n-1); | ||
| #endif | ||
| uint32_t c = strip.getPixelColor(i); // note: LEDs mapped outside of valid range are set to black | ||
| uint8_t r = R(c); | ||
| uint8_t g = G(c); | ||
| uint8_t b = B(c); | ||
| uint8_t w = W(c); | ||
| buffer[pos++] = bri ? qadd8(w, r) : 0; //R, add white channel to RGB channels as a simple RGBW -> RGB map | ||
| buffer[pos++] = bri ? qadd8(w, g) : 0; //G | ||
| buffer[pos++] = bri ? qadd8(w, b) : 0; //B | ||
| } | ||
| return needed; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required AI-generation markers around this block.
As per coding guidelines, “Embed larger AI-generated code blocks into // AI: below section was generated by an AI ... // AI: end comments.”
🤖 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 `@wled00/json.cpp` around lines 1396 - 1447, Add the required AI-generation
markers immediately before and after the complete buildLiveLedsPayload function
body, using the exact “AI: below section was generated by an AI” and “AI: end”
comment text. Do not alter the function’s payload logic or surrounding code.
Source: Coding guidelines
| used = Segment::maxWidth*Segment::maxHeight; // always the size of matrix (more or less than strip.getLengthTotal()) | ||
| n = 1; | ||
| if (used > maxLeds) n = 2; | ||
| if (used > maxLeds*4) n = 4; | ||
| pos = 4; | ||
| count = (Segment::maxWidth/n) * (Segment::maxHeight/n); // matches the advertised dimensions | ||
| } | ||
| #endif | ||
| size_t needed = pos + count*3; | ||
| if (!buffer) return needed; | ||
| if (bufSize < needed) return 0; | ||
|
|
||
| buffer[0] = 'L'; | ||
| buffer[1] = 1; //version | ||
| #ifndef WLED_DISABLE_2D | ||
| if (strip.isMatrix) { | ||
| buffer[1] = 2; //version | ||
| buffer[2] = Segment::maxWidth/n; | ||
| buffer[3] = Segment::maxHeight/n; | ||
| } | ||
| #endif | ||
|
|
||
| for (size_t i = 0; pos < needed; i += n) | ||
| { | ||
| #ifndef WLED_DISABLE_2D | ||
| if (strip.isMatrix && n>1 && (i/Segment::maxWidth)%n) i += Segment::maxWidth * (n-1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sample matrix pixels by explicit row and column.
When the width is odd and n > 1, the linear loop emits ceil(width / n) pixels per row while the header advertises floor(width / n). For example, width 17 with n = 2 emits nine first-row samples but advertises eight, shifting the remaining image.
Iterate over the advertised sampled width and height, mapping each (x, y) back to (x * n, y * n).
🤖 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 `@wled00/json.cpp` around lines 1410 - 1435, Update the matrix sampling loop
after the buffer header setup to iterate over the advertised sampled width and
height, using explicit x/y coordinates and mapping each sample to source
coordinates x*n and y*n. Ensure odd widths with n > 1 emit exactly
Segment::maxWidth/n samples per row, matching the header dimensions, and remove
the current linear i-based row-skipping logic.
| #if !defined(WLED_DISABLE_ESPNOW) && defined(ARDUINO_ARCH_ESP32) | ||
| namespace { | ||
| static constexpr size_t ESPNOW_SYNC_FIRST_SEGMENTS = (sizeof(partial_packet_t::data) - SEG_OFFSET) / UDP_SEG_SIZE; | ||
| static constexpr size_t ESPNOW_SYNC_NEXT_SEGMENTS = sizeof(partial_packet_t::data) / UDP_SEG_SIZE; | ||
|
|
||
| struct EspNowSyncTx { | ||
| uint8_t* payload = nullptr; | ||
| uint8_t segmentCount = 0; | ||
| uint8_t packetNext = 0; | ||
| uint8_t packetTotal = 0; | ||
| }; | ||
|
|
||
| static EspNowSyncTx espNowSyncTx; | ||
|
|
||
| static void resetEspNowSyncTx() { | ||
| free(espNowSyncTx.payload); | ||
| espNowSyncTx = EspNowSyncTx{}; | ||
| } | ||
|
|
||
| // Keep the complete notifier snapshot and generate one radio packet at a time. This avoids a | ||
| // permanent TX ring while preserving multi-packet sync for configurations with many segments. | ||
| static void serviceEspNowSyncTx() { | ||
| if (!espNowSyncTx.payload) return; | ||
| if (!enableESPNow || !useESPNowSync || statusESPNow != ESP_NOW_STATE_ON) { | ||
| resetEspNowSyncTx(); | ||
| return; | ||
| } | ||
| if (!espNowTransportReadyToSend()) return; | ||
|
|
||
| partial_packet_t packet = {'W', espNowSyncTx.packetNext, espNowSyncTx.packetTotal, {0}}; | ||
| size_t firstSegment = 0; | ||
| size_t segmentCount = 0; | ||
| size_t payloadLen = 0; | ||
| if (packet.packet == 0) { | ||
| memcpy(packet.data, espNowSyncTx.payload, SEG_OFFSET); | ||
| segmentCount = min<size_t>(espNowSyncTx.segmentCount, ESPNOW_SYNC_FIRST_SEGMENTS); | ||
| memcpy(packet.data + SEG_OFFSET, espNowSyncTx.payload + SEG_OFFSET, segmentCount * UDP_SEG_SIZE); | ||
| payloadLen = SEG_OFFSET + segmentCount * UDP_SEG_SIZE; | ||
| } else { | ||
| firstSegment = ESPNOW_SYNC_FIRST_SEGMENTS + size_t(packet.packet - 1) * ESPNOW_SYNC_NEXT_SEGMENTS; | ||
| segmentCount = min<size_t>(espNowSyncTx.segmentCount - firstSegment, ESPNOW_SYNC_NEXT_SEGMENTS); | ||
| memcpy(packet.data, espNowSyncTx.payload + SEG_OFFSET + firstSegment * UDP_SEG_SIZE, | ||
| segmentCount * UDP_SEG_SIZE); | ||
| payloadLen = segmentCount * UDP_SEG_SIZE; | ||
| } | ||
|
|
||
| if (espNowTransportSend(ESPNOW_BROADCAST_ADDRESS, reinterpret_cast<const uint8_t*>(&packet), payloadLen + 3)) { | ||
| DEBUG_PRINTLN(F("ESP-NOW sync send failed.")); | ||
| resetEspNowSyncTx(); | ||
| return; | ||
| } | ||
| if (++espNowSyncTx.packetNext >= espNowSyncTx.packetTotal) resetEspNowSyncTx(); | ||
| } | ||
|
|
||
| static void queueEspNowSyncTx(const uint8_t* payload, size_t segmentCount) { | ||
| const size_t payloadLen = SEG_OFFSET + segmentCount * UDP_SEG_SIZE; | ||
| uint8_t* copy = (uint8_t*)d_malloc(payloadLen); | ||
| if (!copy) { | ||
| DEBUG_PRINTLN(F("ESP-NOW sync allocation failed.")); | ||
| return; | ||
| } | ||
| memcpy(copy, payload, payloadLen); | ||
| resetEspNowSyncTx(); | ||
| espNowSyncTx.payload = copy; | ||
| espNowSyncTx.segmentCount = segmentCount; | ||
| const size_t remaining = segmentCount > ESPNOW_SYNC_FIRST_SEGMENTS ? segmentCount - ESPNOW_SYNC_FIRST_SEGMENTS : 0; | ||
| espNowSyncTx.packetTotal = 1 + (remaining + ESPNOW_SYNC_NEXT_SEGMENTS - 1) / ESPNOW_SYNC_NEXT_SEGMENTS; | ||
| serviceEspNowSyncTx(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required AI attribution markers around the new generated blocks.
wled00/udp.cpp#L23-L91: wrap the queued sync implementation.wled00/udp.cpp#L993-L1014: wrap the frame-classification helper.wled00/udp.cpp#L1073-L1093: wrap the reassembly validation changes.wled00/wled.cpp#L17-L47: wrap the network-aware lifecycle helper.wled00/wled.cpp#L981-L1041: wrap the reconnect-deferral logic.wled00/data/settings_wifi.htm#L160-L189: wrap the remote-list JavaScript.wled00/set.cpp#L137-L150: wrap the linked-MAC validation logic.
As per coding guidelines, larger AI-generated blocks must be enclosed by // AI: below section was generated by an AI and // AI: end.
📍 Affects 4 files
wled00/udp.cpp#L23-L91(this comment)wled00/udp.cpp#L993-L1014wled00/udp.cpp#L1073-L1093wled00/wled.cpp#L17-L47wled00/wled.cpp#L981-L1041wled00/data/settings_wifi.htm#L160-L189wled00/set.cpp#L137-L150
🤖 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 `@wled00/udp.cpp` around lines 23 - 91, Wrap each specified AI-generated block
with the exact markers "// AI: below section was generated by an AI" before the
block and "// AI: end" after it. Apply this to wled00/udp.cpp ranges 23-91,
993-1014, and 1073-1093; wled00/wled.cpp ranges 17-47 and 981-1041;
wled00/data/settings_wifi.htm range 160-189; and wled00/set.cpp range 137-150,
preserving the existing implementation.
Sources: Coding guidelines, Path instructions
|
I'm happy to keep working on this if there's genuine interest in the feature. Otherwise, just let me know and I'll close the PR so neither of us wastes any more time. |
|
@figamore can you name a few scenarios where the functionality added in the PR can be used? |
For my personal use case, I dislike having to pull my phone out (make sure it's on the same WiFi as my WLED lights, which is often not the case). Instead I just use the remote and I get everything right away. This also lets other persons in the house control the lights easily. The current ESP-NOW implementation is extremely limited. In my opinion, 6 KB of flash is nothing for a full-featured ESP-NOW implementation that can allow other developers to make all kinds of control devices. Ultimately, I submitted this PR because I find it extremely helpful for my own use and other people have expressed interest in all the extra features it provides. At the end of the day it's up to you if you wish to scrap this or not. |
|
@figamore I'm not against adding this PR, but maybe we need to get a better understanding of the use cases. From code quality and code maintenance perspective, I see some issues that often appear in AI generated code: overlooked corner cases, lost comments, unexplained code, unclear refactoring, wrong assuptions like "segment 0 is always the main segment" or "segment numbers are persistent". This is somewhat becoming "daily business" since people started to use AI for writing source code. I think it's worth to continue improving, because it will help maintainers to understand the code even a few years into the future. |
|
I’d rather keep the discussion centered on the implementation itself, not on assumptions about how the code was created. Many of the ESP-NOW principles in this implementation are part of personal projects I've created that have been tested and used in practice before being adapted for this PR. It's expected that there'll be edge cases to handle in such a large project (some of which I'm happy CodeRabbit was able to catch). I'll refine this further and we can go from there. |
I don't agree here. "AI generated code" seems to explain lots of code quality issues I see. But ignoring the code origin does not change any of these code quality/maintainabilty problems. We still need them addressed.
I agree. |
…alignment, and use WLED’s guarded allocator. Remove unreachable ESP8266 branches from the ESP32-only transport.
|
I've made another round of revisions to address the issues that Coderabbit identified, reverted the web UI to its original state, added all missing comments that were lost during development of this branch, and added more clear explanations where things may not have been clear. Tested again on ESP32 and everything is running smoothly. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/design/espnow-json-protocol.md`:
- Around line 64-71: The protocol documentation’s “Every REQUEST ... is
answered” guarantee conflicts with the implementation’s dropped-request paths.
Update the guarantee near the REQUEST description to state that requests may
time out when reassembly allocation fails or the inbox is full and must be
retried, rather than promising an error response for those cases.
- Around line 29-31: Make the security and discovery rules in
docs/design/espnow-json-protocol.md consistent: explicitly choose whether
DISCOVER is accepted from unpaired MACs or only allow-listed remotes. If
unpaired discovery is supported, document its rate limiting and clarify that
only discovery is permitted before pairing; otherwise remove or revise the flow
where DISCOVER teaches WLED an unknown remote MAC. Update the security statement
at lines 29-31, the discovery description at line 54, and the related
requirement at line 102.
🪄 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: acae2cb8-cdad-4dd6-985c-5eb3cd85c4dc
📒 Files selected for processing (10)
docs/design/espnow-json-protocol.mdplatformio_override.sample.iniwled00/espnow_api.cppwled00/espnow_transport.cppwled00/fcn_declare.hwled00/set.cppwled00/udp.cppwled00/wled.cppwled00/wled.hwled00/ws.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- wled00/ws.cpp
- wled00/fcn_declare.h
- wled00/espnow_transport.cpp
- platformio_override.sample.ini
- wled00/set.cpp
- wled00/wled.h
- wled00/espnow_api.cpp
- wled00/wled.cpp
- wled00/udp.cpp
3 participants
This PR adds a bidirectional ESP-NOW API for remote controllers while preserving the existing WiZ remote and WLED-to-WLED ESP-NOW sync behavior.
Linked remotes can leverage this API to:
/json/state-compatible behaviorA compatible remote implementation is available at:
https://github.com/figamore/wled-touch-remote
Implementation
deserializeState()path for parity with the HTTP and WebSocket JSON APIs.Pairing UI
The ESP-NOW section of the WiFi settings page has been enhanced:
Firmware-side validation also rejects malformed and duplicate MAC entries.
Compatibility and limitations
The complete protocol and remote implementation guidance are documented in
docs/espnow-json-protocol.md.Tests performed
npm testnpm run buildDocumentation
docs/espnow-json-protocol.mdcovering framing, fragmentation, discovery, catalogs, reliability, security assumptions, limits, and example messages.Summary by CodeRabbit
New Features
Bug Fixes
Documentation