Replace QuickESPNow with local library#5624
Conversation
WalkthroughThis PR migrates WLED from the external QuickESPNow library to an internal WledEspNow abstraction. It adds a new lightweight ESP-NOW driver library with platform-agnostic callbacks and lifecycle management, replaces all QuickESPNow calls, and removes the external dependency from the build configuration. ChangesESP-NOW Abstraction Migration
🎯 3 (Moderate) | ⏱️ ~25 minutes 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)lib/wled_espnow/wled_espnow.cpplib/wled_espnow/wled_espnow.cpp:1:10: fatal error: 'wled.h' file not found ... [truncated 1083 characters] ... all/lib/clang/18/include" wled00/udp.cppIn file included from wled00/udp.cpp:1: ... [truncated 1060 characters] ... lugins/clang/install/lib/clang/18/include" wled00/wled.cppIn file included from wled00/wled.cpp:2: ... [truncated 1063 characters] ... ugins/clang/install/lib/clang/18/include" 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
3 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.
Summary by CodeRabbit
Refactor
Chores