Replace QuickESPNow with local library#5624
Conversation
WalkthroughAdds an internal ChangesESP-NOW Abstraction Migration
Sequence Diagram(s)sequenceDiagram
participant App as WLED App
participant EspNow as wled::EspNow
participant SDK as ESP-NOW SDK
App->>EspNow: begin(channel, iface)
EspNow->>SDK: esp_now_init / register send/recv callbacks / add broadcast peer
App->>EspNow: send(addr, data, len)
EspNow->>EspNow: check _inFlight throttle / optional retry
EspNow->>SDK: esp_now_send (broadcast)
SDK-->>EspNow: send callback (status -> _sentCB)
SDK-->>EspNow: recv callback (data, rx_ctrl -> _rcvdCB)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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. 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
added
the
connectivity
![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
🧹 Nitpick comments (1)
wled00/src/dependencies/espnow_wled/espnow_wled.cpp (1)
230-230: ⚡ Quick winUse debug macros instead of unconditional
Serial.printf.Line 230 bypasses
WLED_DEBUGgating and adds serial I/O in normal builds.As per coding guidelines: "Use `DEBUG_PRINTF()` / `DEBUG_PRINTLN()` for debug output (compiled out unless `-D WLED_DEBUG`)."Suggested fix
- if (err != 0 && isretransmit) Serial.printf("ESP-NOW send failed with error %d, inflight=%d\n", err, (int)espNow._inFlight); + if (err != 0 && isretransmit) DEBUG_PRINTF_P(PSTR("ESP-NOW send failed with error %d, inflight=%d\n"), err, (int)espNow._inFlight);🤖 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/src/dependencies/espnow_wled/espnow_wled.cpp` at line 230, Replace the unconditional Serial.printf in espnow_wled.cpp (the ESP-NOW send failure log in the send callback/handler around the if (err != 0 && isretransmit) check) with the project debug macro so it is compiled out unless WLED_DEBUG is defined; use DEBUG_PRINTF (or DEBUG_PRINTLN if preferred) to emit the same formatted message ("ESP-NOW send failed with error %d, inflight=%d\n") and preserve the (int)espNow._inFlight cast and err variable so the debug output remains identical but gated by WLED_DEBUG.
🤖 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/src/dependencies/espnow_wled/espnow_wled.cpp`:
- Around line 221-228: The retry branch that calls esp_now_send(...) when
isretransmit is set does not increment the in-flight counter, causing _inFlight
to go out of sync; update the block that contains the
esp_now_send(const_cast<uint8_t*>(BCAST), const_cast<uint8_t*>(data), len) call
so that after a successful send (check err == ESP_OK) you increment _inFlight,
e.g., modify the retry path around variables _inFlight and isretransmit to
++_inFlight when the send returns success to keep the in-flight counter
accurate.
- Around line 186-190: The ESP8266 branch currently ignores return values from
esp_now_register_recv_cb(_espnowRecvCB),
esp_now_register_send_cb(_espnowSentCB), and
esp_now_add_peer(const_cast<uint8_t*>(BCAST), ESP_NOW_ROLE_COMBO, channel,
nullptr, 0) and sets _running = true unconditionally; change this to check each
call's return code (like the ESP32 path), log or handle failures, avoid setting
_running when any registration/peer-add fails, and return/clean up appropriately
(e.g., deinit or unregister) on error so subsequent send/stop operations behave
consistently.
In `@wled00/src/dependencies/espnow_wled/espnow_wled.h`:
- Around line 7-11: The conditional include for ESP-NOW uses a generic `#else`
which can incorrectly include <esp_now.h> on non-target platforms; update the
guard to explicitly check for the ESP32 architecture by replacing the `#else` with
`#elif` defined(ARDUINO_ARCH_ESP32) so the block reads `#ifdef` ESP8266 / `#elif`
defined(ARDUINO_ARCH_ESP32) / `#endif` and include <espnow.h> for ESP8266 and
<esp_now.h> for ESP32 (references: macros ESP8266, ARDUINO_ARCH_ESP32 and the
include directives <espnow.h> and <esp_now.h>).
---
Nitpick comments:
In `@wled00/src/dependencies/espnow_wled/espnow_wled.cpp`:
- Line 230: Replace the unconditional Serial.printf in espnow_wled.cpp (the
ESP-NOW send failure log in the send callback/handler around the if (err != 0 &&
isretransmit) check) with the project debug macro so it is compiled out unless
WLED_DEBUG is defined; use DEBUG_PRINTF (or DEBUG_PRINTLN if preferred) to emit
the same formatted message ("ESP-NOW send failed with error %d, inflight=%d\n")
and preserve the (int)espNow._inFlight cast and err variable so the debug output
remains identical but gated by WLED_DEBUG.
🪄 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: 09fa15a1-2b61-4741-a209-3454a4ae2277
📒 Files selected for processing (6)
platformio.iniwled00/src/dependencies/espnow_wled/espnow_wled.cppwled00/src/dependencies/espnow_wled/espnow_wled.hwled00/udp.cppwled00/wled.cppwled00/wled.h
💤 Files with no reviewable changes (1)
- platformio.ini
|
I have some pending changes, the actual bug the rabbit did not even mention... |
|
@willmmiles question about folder and file structure: I put this in src/dependencies but it feels kind of out of place since it is not an external dependency. I could put it in the main code folder but it is cluttered as it is. Any suggestion on a better folder structure to put such "local library" files? |
I absolutely agree that If it was really up to me, though ... I'd pull ESP-NOW support out of the core entirely and move it to |
|
If you do go with |
|
I think making core libraries a usermod puts too much strain on the concept, at least for now. |
Oh for sure - there are still some key APIs missing. I do think that moving things in to modules instead of |
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 (2)
lib/wled_espnow/wled_espnow.cpp (2)
47-50:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSynchronize
_inFlightupdates betweenEspNow::send()and the ESP-NOW sent callbacks.
_inFlightis declared asvolatile int8_tinlib/wled_espnow/wled_espnow.h:60, decremented in_espnowSentCB(lib/wled_espnow/wled_espnow.cpp:48and:130) and incremented/read inEspNow::send()(lib/wled_espnow/wled_espnow.cpp:219-232).volatiledoesn’t make++/--atomic, so callback/send interleaving can corrupt the in-flight counter and break the send throttling used by burst ESP-NOW calls inwled00/udp.cpp:168,:178,:185. Use atomic operations or an appropriate synchronization mechanism that is safe with callback context.🤖 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 `@lib/wled_espnow/wled_espnow.cpp` around lines 47 - 50, The _inFlight counter is updated non-atomically in _espnowSentCB and in EspNow::send(), risking races; change espNow._inFlight from volatile int8_t to an atomic type (e.g., std::atomic<int8_t> or std::atomic<int>) and replace ++/-- and reads with atomic operations (fetch_add/fetch_sub/load) using an appropriate memory_order (relaxed is fine for a simple counter) so updates from the ESP-NOW callback (_espnowSentCB) and EspNow::send() are synchronized and safe in the callback context; ensure all sites that increment, decrement or check _inFlight (including _espnowSentCB, EspNow::send(), and any other reads) use the new atomic API.
183-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ESP8266 ESP-NOW init to fail cleanly when peer/callback setup fails
Inlib/wled_espnow/wled_espnow.cpp’s ESP8266EspNow::begin()branch, return values fromesp_now_set_self_role,esp_now_register_recv_cb,esp_now_register_send_cb, andesp_now_add_peerare ignored, but_runningis set totrueand the function returnstrueunconditionally. That letswled00/wled.cppsetstatusESPNow = ESP_NOW_STATE_ON, andwled00/udp.cppbegin sending ESPNOW sync packets viawled::espNow.send()even if the peer/callbacks were never installed.
Mirror the ESP32 path: check each step’s return code, unwind (esp_now_unregister_*/esp_now_deinit/ peer removal as appropriate), leave_runningunset, and returnfalseon any failure.🤖 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 `@lib/wled_espnow/wled_espnow.cpp` around lines 183 - 193, In EspNow::begin() for the ESP8266 branch, check the return values of esp_now_set_self_role, esp_now_register_recv_cb, esp_now_register_send_cb and esp_now_add_peer (the calls that currently ignore errors) and if any fail, undo any previously successful setup (call esp_now_del_peer for the broadcast peer if added, esp_now_unregister_recv_cb and esp_now_unregister_send_cb if registered, and esp_now_deinit) leave _running false and return false; only set _running = true and return true after all steps succeed. Use the existing callback symbols (_espnowRecvCB, _espnowSentCB) and mirror the error/unwind pattern used in the ESP32 path to ensure partial initialization is rolled back cleanly.
🧹 Nitpick comments (1)
lib/wled_espnow/wled_espnow.h (1)
66-123: ⚡ Quick winDelete the commented-out
EspNowBroadcastAPI until it's real.This block is disabled, duplicated again in
lib/wled_espnow/wled_espnow.cpp, and explicitly labeled unreviewed/untested. Keeping a dormant public API in the header adds noise to the migration and makes it easier to accidentally revive code that has never been validated. Please either remove it from this PR or move the porting notes to an issue/ADR. As per coding guidelines, "Remove dead/unused code — justify or delete it" and "Mark AI-generated code blocks with // AI: below section was generated by an AI and // 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 `@lib/wled_espnow/wled_espnow.h` around lines 66 - 123, Remove the entire commented-out EspNowBroadcast API block (the multi-line comment that defines class EspNowBroadcast and its methods) and the commented extern for espnowBroadcast so no dormant, duplicate, AI-generated API remains; if you need to preserve the migration notes, move them to an issue/ADR or add explicit AI markers (// AI: ... // AI: end) in a non-compiling doc instead of leaving the code commented in the header.
🤖 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 `@lib/wled_espnow/wled_espnow.cpp`:
- Around line 47-50: The _inFlight counter is updated non-atomically in
_espnowSentCB and in EspNow::send(), risking races; change espNow._inFlight from
volatile int8_t to an atomic type (e.g., std::atomic<int8_t> or
std::atomic<int>) and replace ++/-- and reads with atomic operations
(fetch_add/fetch_sub/load) using an appropriate memory_order (relaxed is fine
for a simple counter) so updates from the ESP-NOW callback (_espnowSentCB) and
EspNow::send() are synchronized and safe in the callback context; ensure all
sites that increment, decrement or check _inFlight (including _espnowSentCB,
EspNow::send(), and any other reads) use the new atomic API.
- Around line 183-193: In EspNow::begin() for the ESP8266 branch, check the
return values of esp_now_set_self_role, esp_now_register_recv_cb,
esp_now_register_send_cb and esp_now_add_peer (the calls that currently ignore
errors) and if any fail, undo any previously successful setup (call
esp_now_del_peer for the broadcast peer if added, esp_now_unregister_recv_cb and
esp_now_unregister_send_cb if registered, and esp_now_deinit) leave _running
false and return false; only set _running = true and return true after all steps
succeed. Use the existing callback symbols (_espnowRecvCB, _espnowSentCB) and
mirror the error/unwind pattern used in the ESP32 path to ensure partial
initialization is rolled back cleanly.
---
Nitpick comments:
In `@lib/wled_espnow/wled_espnow.h`:
- Around line 66-123: Remove the entire commented-out EspNowBroadcast API block
(the multi-line comment that defines class EspNowBroadcast and its methods) and
the commented extern for espnowBroadcast so no dormant, duplicate, AI-generated
API remains; if you need to preserve the migration notes, move them to an
issue/ADR or add explicit AI markers (// AI: ... // AI: end) in a non-compiling
doc instead of leaving the code commented in the header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 502c10e9-3cf6-4d1b-808e-cb1be401b667
📒 Files selected for processing (6)
lib/wled_espnow/library.jsonlib/wled_espnow/wled_espnow.cpplib/wled_espnow/wled_espnow.hwled00/udp.cppwled00/wled.cppwled00/wled.h
✅ Files skipped from review due to trivial changes (1)
- lib/wled_espnow/library.json
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/wled.cpp
- wled00/wled.h
- wled00/udp.cpp
|
Sounds like a very good idea |
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 (1)
lib/wled_espnow/wled_espnow.h (1)
66-120: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove the commented-out AI scaffold from the header.
Keeping this inactive block in a public header adds maintenance noise and drifts out of sync.
Suggested change
-/* -// start AI code, unreviewed, untested -// --- EspNowBroadcast ----------------------------------------------- -// Thin facade matching the ESPNOWBroadcast interface from WLEDtubes so the -// Tubes usermod can be ported upstream with minimal modifications. -// Delegates actual sending to espNow; state is derived from statusESPNow. -... -// end AI code, unreviewed, untested -*/As per coding guidelines: “Unused or dead code must be justified or removed to keep the codebase clean, maintainable, and readable.”
🤖 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 `@lib/wled_espnow/wled_espnow.h` around lines 66 - 120, Remove the entire commented-out AI scaffold block in lib/wled_espnow/wled_espnow.h (the multi-line comment that defines the EspNowBroadcast class and related preprocessor symbols like WLED_ESPNOW_MAX_REGISTERED_CALLBACKS and WLED_MAX_USERMODS); simply delete that inactive block so the header only contains the active, maintained declarations, and ensure no other code depends on those symbols—if any code uses EspNowBroadcast, move the real implementation into the appropriate source/header and reference it there instead of keeping the commented scaffold.
🧹 Nitpick comments (1)
lib/wled_espnow/wled_espnow.h (1)
17-17: ⚡ Quick winUse
constexprinstead of a macro forESPNOW_MAX_INFLIGHT.This constant is typed and compile-time; replacing
#defineavoids macro leakage from a public header.Suggested change
-#define ESPNOW_MAX_INFLIGHT 6 // note: ESP32 have a larger buffer (16 work no problem) but ESP8266 struggles receiving more than 6 without any pause, also depends on msg length (longer=better) +constexpr uint8_t ESPNOW_MAX_INFLIGHT = 6; // note: ESP32 have a larger buffer (16 work no problem) but ESP8266 struggles receiving more than 6 without any pause, also depends on msg length (longer=better)As per coding guidelines: “Prefer constexpr over
#definefor compile-time constants when possible.”🤖 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 `@lib/wled_espnow/wled_espnow.h` at line 17, Replace the preprocessor macro ESPNOW_MAX_INFLIGHT with a typed, compile-time constant (e.g. use "constexpr int ESPNOW_MAX_INFLIGHT = 6;" or "static constexpr std::size_t ESPNOW_MAX_INFLIGHT = 6;") in the header; update any code that referenced the macro to use the new constant name (ESPNOW_MAX_INFLIGHT) and remove the `#define` to prevent macro leakage. Ensure the chosen type fits usages (int/size_t/uint8_t) and that headers compile without extra includes.
🤖 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 `@lib/wled_espnow/wled_espnow.h`:
- Around line 66-120: Remove the entire commented-out AI scaffold block in
lib/wled_espnow/wled_espnow.h (the multi-line comment that defines the
EspNowBroadcast class and related preprocessor symbols like
WLED_ESPNOW_MAX_REGISTERED_CALLBACKS and WLED_MAX_USERMODS); simply delete that
inactive block so the header only contains the active, maintained declarations,
and ensure no other code depends on those symbols—if any code uses
EspNowBroadcast, move the real implementation into the appropriate source/header
and reference it there instead of keeping the commented scaffold.
---
Nitpick comments:
In `@lib/wled_espnow/wled_espnow.h`:
- Line 17: Replace the preprocessor macro ESPNOW_MAX_INFLIGHT with a typed,
compile-time constant (e.g. use "constexpr int ESPNOW_MAX_INFLIGHT = 6;" or
"static constexpr std::size_t ESPNOW_MAX_INFLIGHT = 6;") in the header; update
any code that referenced the macro to use the new constant name
(ESPNOW_MAX_INFLIGHT) and remove the `#define` to prevent macro leakage. Ensure
the chosen type fits usages (int/size_t/uint8_t) and that headers compile
without extra includes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1078399e-0b87-45ed-b778-7ee778192adc
📒 Files selected for processing (2)
lib/wled_espnow/wled_espnow.cpplib/wled_espnow/wled_espnow.h
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/wled_espnow/wled_espnow.cpp
4 participants
QuickESPNow is not well suited as it is focused on compatibility rather than speed and resources.
This PR replaces it with local functions directly based on esp API calls.
From my tests it is faster and more light weight:
I tested this with a send and receive function both with large packets and bursts of small packets. While large packets work quite well, small (5 bytes) packets tend to get lost on ESP8266 when sent in bursts longer than 6 packets.
The changes need testing in real applications but bench-top tests are very promising.
edit:
it also saves 2k of flash
Summary by CodeRabbit
Refactor
Chores