Adding ESPNOW connection to sync multiple hardware with sound reactive.#5637
Adding ESPNOW connection to sync multiple hardware with sound reactive.#5637Gallosauro wants to merge 6 commits into
Conversation
WalkthroughAdds an ESP-NOW transport for audioreactive sync, integrates transport selection and persistence in AudioReactive (transmit/receive/init/UI/info), refines UDP/realtime behavior, and updates FFT memory allocations and integer FFT scaling. ChangesAudio Sync Transport & FFT Improvements
Sequence Diagram(s)sequenceDiagram
participant Sender as AudioReactive(sender)
participant espnowAudioSync
participant Radio as ESPNowRadio
participant Receiver as AudioReactive(receiver)
Sender->>espnowAudioSync: send(44B payload) (stamp seq,timestamp)
espnowAudioSync->>Radio: transmit ENPacket x2 (duplicate flag)
Radio-->>Receiver: _onRecv callback with ENPacket bytes
Receiver->>espnowAudioSync: handleIncomingPacket(data,len)
espnowAudioSync->>espnowAudioSync: dedup/gap track & latch payload
Receiver->>espnowAudioSync: poll(out, deadline_us)
espnowAudioSync-->>Receiver: payload + render deadline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
usermods/audioreactive/audio_reactive.cpp (1)
321-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMemory leak if
dsps_fft2r_init_sc16()fails.If
dsps_fft2r_init_sc16(NULL, samplesFFT)at line 333 returns an error,windowFloathas been allocated but is not freed before returning. The cleanup at lines 327-331 handles allocation failures, but line 333's failure path just returns without freeingwindowFloat.Proposed fix
- if (dsps_fft2r_init_sc16(NULL, samplesFFT) != ESP_OK) return; // initialize FFT tables + if (dsps_fft2r_init_sc16(NULL, samplesFFT) != ESP_OK) { // initialize FFT tables + heap_caps_free(windowFloat); + heap_caps_free(windowFFT); windowFFT = nullptr; + heap_caps_free(valFFT); valFFT = nullptr; + return; + }🤖 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 `@usermods/audioreactive/audio_reactive.cpp` around lines 321 - 344, If dsps_fft2r_init_sc16(NULL, samplesFFT) returns an error we must free the temporary allocations to avoid a leak: before returning from the failing dsps_fft2r_init_sc16 call, call heap_caps_free on windowFloat, windowFFT and valFFT if they are non-null (and set those pointers to nullptr) so all buffers allocated earlier (windowFloat, windowFFT, valFFT) are cleaned up; ensure this cleanup mirrors the allocation-failure block and then return.
🧹 Nitpick comments (3)
usermods/audioreactive/espnow_audio_sync.h (2)
1-34: 💤 Low valueAdd AI attribution comments around this AI-generated code block.
Per coding guidelines, AI-generated code blocks should be wrapped with
// AI: below section was generated by an AI...// AI: endcomments. The PR description indicates this file was AI-generated.Proposed fix
+// AI: below section was generated by an AI // espnow_audio_sync.h // // Drop-in ESP-NOW transport for WLED audioreactive sync (v16.0.0 mainline).And at the end of the file (after the closing brace of the namespace):
} // namespace espnowAudioSync +// 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 `@usermods/audioreactive/espnow_audio_sync.h` around lines 1 - 34, The header is AI-generated and needs explicit AI-attribution comment markers; add a leading comment line "// AI: below section was generated by an AI" immediately before the start of the AI-generated block (i.e., before the current top comment block or whatever precedes the espnow_audio_sync.h content) and a trailing line "// AI: end" immediately after the end of the generated code (after the closing brace of the namespace used in this file), so that the entire generated region (including the public API comments and any namespace declarations/functions like begin, send, poll, handleIncomingPacket if present) is wrapped between those two markers.
72-97: ⚖️ Poor tradeoffHeader with static state risks ODR violations if included from multiple translation units.
The comment on line 73 acknowledges this must be included from exactly one .cpp file. However, this is fragile - accidental inclusion elsewhere would silently create duplicate, disconnected state copies. Consider moving state to a separate .cpp file or using inline variables (C++17) to enforce single definition.
Additionally, stats counters (
_stat_rx,_stat_dupes, etc.) are incremented insidehandleIncomingPacket()which can be called from the ESP-NOW receive callback (ISR context). While diagnostic-only, the read-modify-write increments are technically racy.🤖 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 `@usermods/audioreactive/espnow_audio_sync.h` around lines 72 - 97, The header currently defines many file-scope static variables (_channel, _isSender, _initialised, _hosted, _txSeq, _rxReady, _rxPayload, _rxMux, _stat_rx, _stat_dupes, _stat_gaps, _stat_tx, _stat_tx_err, etc.), which risks ODR violations if the header is ever included from multiple translation units; move these state definitions into a single .cpp (e.g., espnow_audio_sync.cpp) and leave only extern declarations in the header, or mark them as inline variables (C++17+) in the header to guarantee one definition. Also, protect the counter increments performed in handleIncomingPacket() (and any ESP-NOW callback) from ISR/race issues by making stats atomic types (std::atomic<uint32_t>) or by wrapping read-modify-write operations with the existing portMUX critical section (portENTER_CRITICAL/portEXIT_CRITICAL) using _rxMux or a dedicated mutex. Ensure you update declarations/uses of the variables and handlers accordingly so the receive callback and handleIncomingPacket() reference the single shared definitions.usermods/audioreactive/audio_reactive.cpp (1)
1311-1328: ⚡ Quick winConsider version-aware UDP buffer clearing.
Based on learnings,
WiFiUDP::flush()is deprecated and ineffective for RX draining in arduino-esp32 v3.x (ESP-IDF 5.x). For compatibility across versions:- if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) fftUdp.flush(); +#if ESP_IDF_VERSION_MAJOR >= 5 + if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) fftUdp.clear(); +#else + if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) fftUdp.flush(); +#endifBased on learnings:
WiFiUDP::flush()should not be used to clear/drain the UDP receive buffer on ESP-IDF 5.x; useWiFiUDP::clear()instead.🤖 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 `@usermods/audioreactive/audio_reactive.cpp` around lines 1311 - 1328, Replace the direct call to fftUdp.flush() in the ARDUINO_ARCH_ESP32 branch with a version-aware drain: call fftUdp.clear() when compiling against newer arduino-esp32/ESP-IDF 5.x (or when a clear() symbol is available) and fall back to fftUdp.flush() for older SDKs; update the conditional around the invalid-packet check that currently uses fftUdp.parsePacket() and fftUdp.flush() so it uses fftUdp.clear() when supported (keep the same size checks involving UDPSOUND_MAX_PACKET) and ensure compile-time detection uses a macro such as ESP_IDF_VERSION_MAJOR or a WIFI_UDP_HAS_CLEAR probe to pick the correct method.
🤖 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 `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1536-1541: In connected() avoid calling espnowAudioSync::begin()
when audioSyncTransport==0 (UDP); change the logic so you only compute
syncChannel/amSender and call espnowAudioSync::begin(syncChannel, amSender) if
audioSyncTransport != 0 and audioSyncEnabled indicates sender/receiver as
before, otherwise skip ESP-NOW init and leave the DEBUG_PRINTLN fallback logic
for when ESP-NOW is selected but begin() fails; reference
espnowAudioSync::begin, audioSyncTransport, audioSyncEnabled and the connected()
function when making this conditional gate.
---
Outside diff comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 321-344: If dsps_fft2r_init_sc16(NULL, samplesFFT) returns an
error we must free the temporary allocations to avoid a leak: before returning
from the failing dsps_fft2r_init_sc16 call, call heap_caps_free on windowFloat,
windowFFT and valFFT if they are non-null (and set those pointers to nullptr) so
all buffers allocated earlier (windowFloat, windowFFT, valFFT) are cleaned up;
ensure this cleanup mirrors the allocation-failure block and then return.
---
Nitpick comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1311-1328: Replace the direct call to fftUdp.flush() in the
ARDUINO_ARCH_ESP32 branch with a version-aware drain: call fftUdp.clear() when
compiling against newer arduino-esp32/ESP-IDF 5.x (or when a clear() symbol is
available) and fall back to fftUdp.flush() for older SDKs; update the
conditional around the invalid-packet check that currently uses
fftUdp.parsePacket() and fftUdp.flush() so it uses fftUdp.clear() when supported
(keep the same size checks involving UDPSOUND_MAX_PACKET) and ensure
compile-time detection uses a macro such as ESP_IDF_VERSION_MAJOR or a
WIFI_UDP_HAS_CLEAR probe to pick the correct method.
In `@usermods/audioreactive/espnow_audio_sync.h`:
- Around line 1-34: The header is AI-generated and needs explicit AI-attribution
comment markers; add a leading comment line "// AI: below section was generated
by an AI" immediately before the start of the AI-generated block (i.e., before
the current top comment block or whatever precedes the espnow_audio_sync.h
content) and a trailing line "// AI: end" immediately after the end of the
generated code (after the closing brace of the namespace used in this file), so
that the entire generated region (including the public API comments and any
namespace declarations/functions like begin, send, poll, handleIncomingPacket if
present) is wrapped between those two markers.
- Around line 72-97: The header currently defines many file-scope static
variables (_channel, _isSender, _initialised, _hosted, _txSeq, _rxReady,
_rxPayload, _rxMux, _stat_rx, _stat_dupes, _stat_gaps, _stat_tx, _stat_tx_err,
etc.), which risks ODR violations if the header is ever included from multiple
translation units; move these state definitions into a single .cpp (e.g.,
espnow_audio_sync.cpp) and leave only extern declarations in the header, or mark
them as inline variables (C++17+) in the header to guarantee one definition.
Also, protect the counter increments performed in handleIncomingPacket() (and
any ESP-NOW callback) from ISR/race issues by making stats atomic types
(std::atomic<uint32_t>) or by wrapping read-modify-write operations with the
existing portMUX critical section (portENTER_CRITICAL/portEXIT_CRITICAL) using
_rxMux or a dedicated mutex. Ensure you update declarations/uses of the
variables and handlers accordingly so the receive callback and
handleIncomingPacket() reference the single shared definitions.
🪄 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: ea437e91-8fcb-449d-978a-e33d3bbbafb4
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cppusermods/audioreactive/espnow_audio_sync.h
coderabbitai
Bot
added
external
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 (1)
usermods/audioreactive/espnow_audio_sync.h (1)
38-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard ESP-NOW header + fix one-shot init on reconnects.
usermods/audioreactive/audio_reactive.cppincludesusermods/audioreactive/espnow_audio_sync.hunconditionally, butespnow_audio_sync.hunconditionally includes ESP32/IDF headers (esp_now.h/esp_wifi.h/esp_idf_version.h)—so non-ESP32 builds can fail at compile time even though thebegin()call site is inside#ifdef ARDUINO_ARCH_ESP32.espnowAudioSync::begin()is one-shot (_initialisedearly return) whileaudio_reactive.cpp::connected()runs on every WiFi (re)connect, so STANDALONE mode can keep stale pinned channel/peer state after reconnect.Possible shape of the fix
+#ifdef ARDUINO_ARCH_ESP32 `#include` <Arduino.h> `#include` <WiFi.h> `#include` <esp_now.h> `#include` <esp_wifi.h> `#include` <esp_timer.h> `#include` <esp_idf_version.h> `#include` <string.h> + +namespace espnowAudioSync { + // current implementation +} +#else +#include <Arduino.h> + +namespace espnowAudioSync { + struct Stats { uint32_t rx, dupes, gaps, tx, tx_err; }; + inline bool begin(uint8_t, bool) { return false; } + inline bool send(const void*, size_t) { return false; } + inline bool poll(void*, size_t, uint32_t* = nullptr, uint32_t = 15000) { return false; } + inline bool handleIncomingPacket(const uint8_t*, int) { return false; } + inline bool isHosted() { return false; } + inline Stats stats() { return Stats{0, 0, 0, 0, 0}; } +} +#endif🤖 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 `@usermods/audioreactive/espnow_audio_sync.h` around lines 38 - 43, Wrap the ESP-IDF/ESP32-specific headers in espnow_audio_sync.h inside an `#ifdef` ARDUINO_ARCH_ESP32 guard so non-ESP32 builds don't include esp_now.h/esp_wifi.h/esp_idf_version.h, and modify espnowAudioSync::begin() so it is idempotent on reconnects instead of a one-shot: remove the unconditional early return on _initialised (or detect reconnects) and if already initialised perform a safe reinit sequence (call esp_now_deinit() or clear peers and re-run esp_now_init()/peer add/channel setup) before reconfiguring; this ensures audio_reactive.cpp::connected() can call begin() on every WiFi (re)connect (including STANDALONE mode) to refresh pinned channel/peer state while still setting _initialised at the end.
🤖 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 `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1541-1546: The ESP-NOW init failure logs a UDP fallback but never
actually flips the transport, so later send/receive still use ESP-NOW; in the
espnowAudioSync::begin() failure branch either set the transport back to UDP
(e.g. set audioSyncTransport = 0) or set a dedicated runtime-ready flag (e.g.
espnowReady = false) and update the later transport decision points to check
that flag (or call an espnowAudioSync::isReady()) before taking the ESP-NOW
branch; modify the code paths that use audioSyncTransport to gate on the
successful init flag (or the updated audioSyncTransport) so the module truly
falls back to UDP when begin() fails (refer to audioSyncTransport,
espnowAudioSync::begin, and the later send/receive transport branches).
In `@usermods/audioreactive/espnow_audio_sync.h`:
- Around line 97-98: The static flag _initialised makes begin() a no-op after
the first success so connected() can't refresh channel/peer state on reconnect;
change begin() (and any STANDALONE-mode init paths that check _initialised) to
detect channel or peer changes and reapply ESP-NOW configuration instead of
returning early, or add a clear/reset helper that forces reinitialisation and
re-registers the broadcast peer; specifically update the logic around
_initialised, begin(), and the reconnect hook connected() to either (1) compare
current Wi‑Fi channel/peer info and perform reconfiguration when different, or
(2) expose and call a reset/reinit function from connected() to unregister and
re-register peers and set the channel before reinitialising ESP-NOW.
---
Outside diff comments:
In `@usermods/audioreactive/espnow_audio_sync.h`:
- Around line 38-43: Wrap the ESP-IDF/ESP32-specific headers in
espnow_audio_sync.h inside an `#ifdef` ARDUINO_ARCH_ESP32 guard so non-ESP32
builds don't include esp_now.h/esp_wifi.h/esp_idf_version.h, and modify
espnowAudioSync::begin() so it is idempotent on reconnects instead of a
one-shot: remove the unconditional early return on _initialised (or detect
reconnects) and if already initialised perform a safe reinit sequence (call
esp_now_deinit() or clear peers and re-run esp_now_init()/peer add/channel
setup) before reconfiguring; this ensures audio_reactive.cpp::connected() can
call begin() on every WiFi (re)connect (including STANDALONE mode) to refresh
pinned channel/peer state while still setting _initialised at the end.
🪄 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: f8ddba42-2298-47e1-912a-9dd5dd7d3987
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cppusermods/audioreactive/espnow_audio_sync.h
| if (audioSyncTransport == 1) { | ||
| const uint8_t syncChannel = WiFi.channel(); | ||
| const bool amSender = (audioSyncEnabled & 0x01) != 0; | ||
| if (!espnowAudioSync::begin(syncChannel, amSender)) { | ||
| DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - falling back to UDP.")); | ||
| } |
There was a problem hiding this comment.
The logged UDP fallback never actually happens.
If espnowAudioSync::begin() fails here, audioSyncTransport stays 1, so later send/receive code keeps taking the ESP-NOW branch and sync stops instead of falling back. Flip a runtime-ready flag back to UDP here, or gate the later transport branches on successful ESP-NOW init.
Minimal localized fix
if (audioSyncTransport == 1) {
const uint8_t syncChannel = WiFi.channel();
const bool amSender = (audioSyncEnabled & 0x01) != 0;
if (!espnowAudioSync::begin(syncChannel, amSender)) {
- DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - falling back to UDP."));
+ audioSyncTransport = 0; // runtime fallback; persisted config stays unchanged until next save/reboot
+ DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - using UDP for this session."));
}
}📝 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.
| if (audioSyncTransport == 1) { | |
| const uint8_t syncChannel = WiFi.channel(); | |
| const bool amSender = (audioSyncEnabled & 0x01) != 0; | |
| if (!espnowAudioSync::begin(syncChannel, amSender)) { | |
| DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - falling back to UDP.")); | |
| } | |
| if (audioSyncTransport == 1) { | |
| const uint8_t syncChannel = WiFi.channel(); | |
| const bool amSender = (audioSyncEnabled & 0x01) != 0; | |
| if (!espnowAudioSync::begin(syncChannel, amSender)) { | |
| audioSyncTransport = 0; // runtime fallback; persisted config stays unchanged until next save/reboot | |
| DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - using UDP for this session.")); | |
| } | |
| } |
🤖 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 `@usermods/audioreactive/audio_reactive.cpp` around lines 1541 - 1546, The
ESP-NOW init failure logs a UDP fallback but never actually flips the transport,
so later send/receive still use ESP-NOW; in the espnowAudioSync::begin() failure
branch either set the transport back to UDP (e.g. set audioSyncTransport = 0) or
set a dedicated runtime-ready flag (e.g. espnowReady = false) and update the
later transport decision points to check that flag (or call an
espnowAudioSync::isReady()) before taking the ESP-NOW branch; modify the code
paths that use audioSyncTransport to gate on the successful init flag (or the
updated audioSyncTransport) so the module truly falls back to UDP when begin()
fails (refer to audioSyncTransport, espnowAudioSync::begin, and the later
send/receive transport branches).
| /** @brief True once begin() has succeeded. Subsequent begin() calls no-op. */ | ||
| static bool _initialised = false; |
There was a problem hiding this comment.
begin() cannot refresh channel/peer state after a reconnect.
connected() is a reconnect hook, but after the first success _initialised makes every later begin() a no-op. In STANDALONE mode that leaves the pinned channel and broadcast peer registration frozen to the original values, so a later Wi-Fi reconnect on a different channel can strand ESP-NOW on stale state. Consider allowing begin() to reapply channel/peer settings when they change, or add an explicit reset/reinit path.
Also applies to: 191-195, 215-221
🤖 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 `@usermods/audioreactive/espnow_audio_sync.h` around lines 97 - 98, The static
flag _initialised makes begin() a no-op after the first success so connected()
can't refresh channel/peer state on reconnect; change begin() (and any
STANDALONE-mode init paths that check _initialised) to detect channel or peer
changes and reapply ESP-NOW configuration instead of returning early, or add a
clear/reset helper that forces reinitialisation and re-registers the broadcast
peer; specifically update the logic around _initialised, begin(), and the
reconnect hook connected() to either (1) compare current Wi‑Fi channel/peer info
and perform reconfiguration when different, or (2) expose and call a
reset/reinit function from connected() to unregister and re-register peers and
set the channel before reinitialising ESP-NOW.
|
Thanks for contributing I did not yet look at the code but just from my experience with ESPNow I know it is a bit of a "diva" and unless you know what you are doing and testing all possible scenarios it can run into a lot of issues. So my question is: how much of the code is AI and did you review and try tounderstand every line of code? |
|
Hello, it is mostly AI-generated. I tested on the hardware, and it is working fine, though. |
|
I just now read the first few lines of changes and I have to say that this wont fly at all in its current state, all I see is AI slop. |
|
Are you experienced enough in C++ to clean this up? |
softhack007
added
the
AI
| float* raw_buffer = (float*)heap_caps_malloc((2 * samplesFFT * sizeof(float)) + 16, MALLOC_CAP_8BIT); | ||
| if ((raw_buffer == nullptr)) return; // something went wrong | ||
| valFFT = (float*)(((uintptr_t)raw_buffer + 15) & ~15); // SIMD requires aligned memory to 16-byte boundary. note in IDF5 there is MALLOC_CAP_SIMD available | ||
| valFFT = (float*)heap_caps_aligned_calloc(16, 2 * samplesFFT, sizeof(float), MALLOC_CAP_8BIT); // SIMD requires aligned memory to 16-byte boundary. note in IDF5 there is MALLOC_CAP_SIMD available |
There was a problem hiding this comment.
Why did you change this part? It has nothing to to with sound sync,
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
Hi @Gallosauro thanks for your contribution.
Before we can continue with code reviews, please align your code with the latest source code from main.
Currently your PR contains lots of deletions that revert some of our latest improvements. We cannot integrate your PR like this, and I don't want to spam with "please revert this change" comments 😉
oh-oh, this sounds like lots of work for reviewers 😉 |
Aehm ... this has nothing to do with Sound sync. Are you sure? And what does the "different pinout" remark want to say? |
Please clean up the PR description. Parts like above text are just confusing, at least for me. |
softhack007
added
needs_rebase
| #if defined(ARDUINO_ARCH_ESP32) && (defined(WLED_DEBUG) || defined(SR_DEBUG)) | ||
| #include <esp_timer.h> | ||
| #endif | ||
| #define ESPNOW_AUDIO_SYNC_SKIP_PACKET_DECL // audio_reactive.cpp already declares audioSyncPacket |
There was a problem hiding this comment.
Please don't re-define the audiosync packet structure.
There was a problem hiding this comment.
edit: looks like this define is not referenced, please remove it.
|
|
||
| infoArr = user.createNestedArray(F("ESP-NOW Mode")); | ||
| infoArr.add(espnowAudioSync::isHosted() ? F("hosted (sharing WLED stack)") : F("standalone")); | ||
| } |
There was a problem hiding this comment.
please reduce the statistics lines added here - add only 2-3 lines that are absolutely needed. If you want more infos, put them behind #ifdef WLED_DEBUG ... #endif
| // NEW: Audio Sync Transport selector (UDP vs ESP-NOW) | ||
| uiScript.print(F("dd=addDropdown(ux,'sync:transport');")); | ||
| uiScript.print(F("addOption(dd,'UDP (default)',0);")); | ||
| uiScript.print(F("addOption(dd,'ESP-NOW (low-latency)',1);")); |
There was a problem hiding this comment.
why do you think ESP-NOW is low-latency?
| #include <esp_wifi.h> | ||
| #include <esp_timer.h> | ||
| #include <esp_idf_version.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
too many includes, please clean up
| struct __attribute__((packed)) ENPacket { | ||
| uint16_t magic; ///< Equals EN_MAGIC (0xA53E). | ||
| uint8_t version; ///< Equals EN_VERSION (1). | ||
| uint8_t flags; ///< Bit 0: this is the duplicate copy. |
| uint8_t version; ///< Equals EN_VERSION (1). | ||
| uint8_t flags; ///< Bit 0: this is the duplicate copy. | ||
| uint16_t seq; ///< Monotonically increasing per sender. | ||
| uint16_t pad; ///< Alignment pad (set to 0). |
There was a problem hiding this comment.
padding in a packed struct? why?
| * time (HOSTED mode); false when this module owns the radio | ||
| * (STANDALONE mode). | ||
| */ | ||
| static bool _hosted = false; |
There was a problem hiding this comment.
can you explain why these two modes (hosted/self-hosted) are needed?
| if (outLen < EN_PAYLOAD) return false; | ||
|
|
||
| uint32_t arrLocalUs; | ||
| portENTER_CRITICAL(&_rxMux); |
There was a problem hiding this comment.
(major problem)
portENTER_CRITICAL() stalls everything, including LEDs drivers. If you need a mutex, please use a regular FreeRTOS semaphore instead.
| if (diff > 1) { _stat_gaps += (diff - 1); } | ||
| } | ||
|
|
||
| portENTER_CRITICAL(&_rxMux); |
There was a problem hiding this comment.
same as above - no portENTER_CRITICAL() please, use a semaphore if needed.
| */ | ||
| static void _onRecv(ESPNOW_RECV_CB_SIG) { | ||
| ESPNOW_RECV_CB_ARGS_UNUSED | ||
| handleIncomingPacket(data, len); |
There was a problem hiding this comment.
What if an incoming packet is not for audio sync, but for esp-now remote or for WLED sync? Will these be droped, or not?
|
Generic comment: please make sure that esp-now sound sync is only added when WLED_DISABLE_ESPNOW is not defined. This also implies the "self-hosted" part of your PR can be removed. If users build with |
There was a problem hiding this comment.
@coderabbitai please scan espnow_audio_sync.h for unused variables, unused struct members, unneeded getter/setter functions (=only called from the main object), and dead or unreachable code.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== fftResultMax usage ==="
rg -n "fftResultMax" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== enDeadline usage ==="
rg -n "enDeadline" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== agcSensitivity usage ==="
rg -n "agcSensitivity" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== userVar1 usage ==="
rg -n "userVar1" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== useMainSegmentOnly usage in audioreactive ==="
rg -n "useMainSegmentOnly" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== `#if` 0 block bounds ==="
rg -n "`#if` 0|`#endif`" usermods/audioreactive/audio_reactive.cpp | head -30
echo ""
echo "=== udpSamplePeak usage ==="
rg -n "udpSamplePeak" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== receivedFormat usage ==="
rg -n "receivedFormat" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== last_UDPTime usage ==="
rg -n "last_UDPTime" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== binNum maxVol deprecated check ==="
rg -n "binNum|maxVol" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== getUMData callers - looking for only-called-from-main pattern ==="
rg -n "getUMData\|removeAudioPalettes\|createAudioPalettes\|fillAudioPalettes\|getCRGBForBand" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== syncVolumeSmth scope ==="
rg -n "syncVolumeSmth" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== addPalettes usage ==="
rg -n "addPalettes" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== palettes (int8_t) usage ==="
rg -n "\bpalettes\b" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== updateIsRunning usage ==="
rg -n "updateIsRunning" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== raw_buffer (old variable, should be gone) ==="
rg -n "raw_buffer" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== haveDoneFFT usage ==="
rg -n "haveDoneFFT" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== audioSyncTransport in ESP-NOW receive condition (loop) ==="
rg -n "audioSyncTransport\|udpSyncConnected" usermods/audioreactive/audio_reactive.cpp | head -40
echo ""
echo "=== Check for audioSyncEnabled receive mode gating with ESP-NOW ==="
rg -n "audioSyncEnabled" usermods/audioreactive/audio_reactive.cpp | head -40
Length of output: 8485
🏁 Script executed:
#!/bin/bash
echo "=== Brace count verification around connected() ==="
# Extract lines from connected() through loop() to count braces
python3 - <<'EOF'
with open("usermods/audioreactive/audio_reactive.cpp", "r") as f:
lines = f.readlines()
# Find connected() start
start = None
for i, line in enumerate(lines):
if "void connected() override" in line:
start = i
break
if start is None:
print("connected() not found")
exit()
print(f"connected() starts at line {start+1}")
depth = 0
in_comment = False
in_string = False
found_open = False
for i in range(start, min(start+200, len(lines))):
line = lines[i].rstrip()
for j, ch in enumerate(line):
if in_comment:
if ch == '*' and j+1 < len(line) and line[j+1] == '/':
in_comment = False
continue
if not in_comment and ch == '/' and j+1 < len(line) and line[j+1] == '*':
in_comment = True
continue
if ch == '/' and j+1 < len(line) and line[j+1] == '/':
break # rest of line is comment
if ch == '{':
depth += 1
found_open = True
elif ch == '}':
depth -= 1
if found_open and depth == 0:
print(f"connected() closes at line {i+1}: {line}")
print(f"Next line ({i+2}): {lines[i+1].rstrip() if i+1 < len(lines) else 'EOF'}")
break
if i > start + 5 and found_open and "void loop() override" in line:
print(f"WARNING: found 'void loop() override' at line {i+1} with depth={depth} (not closed yet!)")
print(f" depth={depth}, context:")
for k in range(max(0,i-3), min(len(lines), i+3)):
print(f" {k+1}: {lines[k].rstrip()}")
break
EOF
echo ""
echo "=== Check `#if` 0 block location ==="
python3 - <<'EOF'
with open("usermods/audioreactive/audio_reactive.cpp", "r") as f:
lines = f.readlines()
for i, line in enumerate(lines):
if "`#if` 0" in line:
print(f"Line {i+1}: {line.rstrip()}")
# Show surrounding context
for k in range(i, min(len(lines), i+5)):
print(f" {k+1}: {lines[k].rstrip()}")
print()
EOF
echo ""
echo "=== fftResultMax - confirm no assignments ==="
rg -n "fftResultMax\s*\[" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Check getJsonValue 3-arg usage ==="
rg -n "getJsonValue.*uint8_t.*0" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Check for SR_DMTYPE_NETWORK_ONLY in loop() receive gate ==="
rg -n -A2 -B2 "audioSyncEnabled.*0x02.*udpSyncConnected" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== connectUDPSoundSync - does it open UDP even for ESPNOW? ==="
rg -n -A5 "connectUDPSoundSync" usermods/audioreactive/audio_reactive.cpp | head -40
Length of output: 2866
@softhack007, here are the findings from scanning the file. I'll start with the most critical item.
🔴 Critical: Missing closing } for connected() — compilation error
The diff removes the original closing brace of connected() (the - } line) when inserting the ESP-NOW begin() block, but never puts it back. The brace-depth counter confirms that void loop() override is reached at line 1558 while depth is still 1 (i.e., still inside connected()). This is a C++ syntax error and should prevent compilation.
The fix is simply inserting } to close connected() before the loop() comment:
if (audioSyncTransport == 1) {
const uint8_t syncChannel = WiFi.channel();
const bool amSender = (audioSyncEnabled & 0x01) != 0;
if (!espnowAudioSync::begin(syncChannel, amSender)) {
DEBUG_PRINTLN(F("[ENaudio] espnowAudioSync::begin() failed - falling back to UDP."));
}
}
} // ← missing closing brace for connected()
/*
* loop() is called continuously...
*/
void loop() override🟠 Unused variable: enDeadline in receiveAudioData()
Lines 1314–1315: enDeadline is declared, populated by espnowAudioSync::poll(), and then silently discarded — the returned timing deadline is never consumed:
uint32_t enDeadline = 0;
if (espnowAudioSync::poll(&enFrame, sizeof(enFrame), &enDeadline, /*renderDelayUs=*/15000)) {
decodeAudioData(...);
// enDeadline is never used hereIf the intent is to schedule rendering at a precise time (the whole point of the deadline), this feature is non-functional. Either use the value (e.g., for a delayMicroseconds-style render-timing nudge) or drop it and pass nullptr as the deadline argument.
🟡 Dead variable: fftResultMax[] (line 232, SR_DEBUG only)
Declared but never written to or read from anywhere in the file:
`#ifdef` SR_DEBUG
static float fftResultMax[NUM_GEQ_CHANNELS] = {0.0f}; // A table used for testing...
`#endif`No assignment, no reference. Safe to remove.
🟡 Dead code: #if 0 block — old FFT bin mapping (line 491)
"Andrew's updated mapping" for a 10240 Hz sample rate is permanently disabled via #if 0. It's been superseded by the softhack007 22050 Hz mapping in the #else branch and carries no informational value that the comment doesn't already convey. Worth removing to reduce maintenance surface.
There is a second, smaller #if 0 block at line 929 (a debug print loop inside FFT_SAMPLING_LOG) that is also dead.
🟡 Commented-out code that can be cleaned up
| Location | Item |
|---|---|
| Line 104 | //static float agcSensitivity = 128; — the getSensitivity() function it references was never implemented; safe to delete |
| Lines 1272, 1300 | //userVar1 = samplePeak; in both decodeAudioData() variants — userVar1 doesn't exist in the current WLED API; safe to delete |
🔵 Design note: UDP socket opened even in ESP-NOW transport mode
connectUDPSoundSync() and connected() unconditionally open a UDP multicast socket whenever audioSyncEnabled != 0, regardless of audioSyncTransport. In practice this means:
udpSyncConnectedistrueeven when ESP-NOW is the selected transport — which is why theloop()receive gateif ((audioSyncEnabled & 0x02) && udpSyncConnected)correctly reachesreceiveAudioData()in ESP-NOW mode (the function then early-returns with the ESP-NOW path). Not broken, but wastes a multicast socket and generates spuriousfftUdp.flush()calls. Worth skipping UDP socket setup whenaudioSyncTransport == 1.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5559
File: wled00/wled.h:990-998
Timestamp: 2026-05-03T11:37:43.177Z
Learning: In the WLED audioreactive usermod (usermods/audioreactive/*), when using DEBUGSR_PRINT, DEBUGSR_PRINTLN, or DEBUGSR_PRINTF, use the function-style macros DEBUGOUT(x), DEBUGOUTLN(x), and DEBUGOUTF(x...) instead of the old stream-style form (DEBUGOUT.print(...)/println(...)/printf(...)). This ensures consistent logging behavior and avoids deprecated syntax.
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, do not flag precedence issues for expressions that mix additive operators (`+`, `-`) with shift operators (`<<`, `>>`) based on the assumption that shifts bind tighter. Per C/C++ operator precedence, `+`/`-` have higher precedence than `<<`/`>>` (e.g., `x - edge0 << 8` parses as `(x - edge0) << 8`). When reviewing WLED (and other) fixed-point/bit-manipulation code, confirm the intended parse using cppreference before reporting a precedence bug for mixed `-`/`+` and `<<`/`>>` expressions.
Learnt from: softhack007
Repo: wled/WLED PR: 5599
File: usermods/audioreactive/audio_reactive.cpp:1303-1308
Timestamp: 2026-05-11T16:00:13.574Z
Learning: When targeting arduino-esp32 v3.x (ESP-IDF 5.x), treat `WiFiUDP::flush()` as deprecated and ineffective for RX draining; it should not be used to clear/drain the UDP receive buffer. Instead, use `WiFiUDP::clear()`, which was introduced in arduino-esp32 v3.0 and is marked as the deprecated replacement via `NetworkUdp.h`.
If the code must compile and behave correctly across both arduino-esp32 v2.x (ESP-IDF < 5) and v3.x (ESP-IDF >= 5), use conditional compilation:
- for `ESP_IDF_VERSION_MAJOR < 5`: use `udp.flush()`
- for `ESP_IDF_VERSION_MAJOR >= 5`: use `udp.clear()`
Do not flag `WiFiUDP::clear()` usages for ESP-IDF >= 5 as non-existent—this is the documented correct replacement.
There was a problem hiding this comment.
@coderabbitai please scan espnow_audio_sync.h for unused variables, unused struct members, unneeded getter/setter functions (=only called from the main object), and dead or unreachable code.
There was a problem hiding this comment.
🧩 Analysis chain
Good question — let me search the codebase to cross-reference every symbol from the header against its actual usage sites.
🏁 Script executed:
#!/bin/bash
echo "=== espnowAudioSync API calls in audio_reactive.cpp ==="
rg -n "espnowAudioSync::" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== espnowAudioSync API calls across the whole codebase ==="
rg -rn "espnowAudioSync::"
echo ""
echo "=== _rxSendUs usage ==="
rg -n "_rxSendUs" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== ENPacket::pad usage ==="
rg -n "\.pad" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== Stats struct field usage (dupes, gaps referenced from stats()) ==="
rg -n "stats\(\)" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== isHosted() usage ==="
rg -rn "isHosted"
echo ""
echo "=== _stat_dupes usage ==="
rg -n "_stat_dupes" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== renderDelayUs usage in poll() ==="
rg -n "renderDelayUs\|deadline_us\|poll(" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== EN_PREAMBLE constant usage ==="
rg -n "EN_PREAMBLE" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== BROADCAST_MAC const usage ==="
rg -n "BROADCAST_MAC" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== _channel usage ==="
rg -n "_channel" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== _hosted usage ==="
rg -n "_hosted" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== _rxSendUs: set vs read ==="
grep -n "_rxSendUs" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== deadline_us parameter usage in poll() and its callers ==="
rg -rn "deadline_us\|poll("
echo ""
echo "=== espnowChannel usage ==="
rg -rn "espnowChannel\|espnow_channel\|audioSyncTransport\|_syncTransport"
Length of output: 3579
Let me do one more targeted check before the final report.
🏁 Script executed:
#!/bin/bash
echo "=== How stats() fields are consumed in audio_reactive.cpp ==="
rg -n "s\.rx\|s\.dupes\|s\.gaps\|s\.tx\|stat_" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Context around stats() call (±15 lines) ==="
rg -n -A 20 "auto s = espnowAudioSync" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Check ep.pad and memset ordering in espnow_audio_sync.h ==="
grep -n "memset\|ep\.pad\|ep\.flags\|ep\.seq\|ep\.magic" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== Check _rxSendUs read sites (grep wider) ==="
grep -n "rxSendUs\|send_us" usermods/audioreactive/espnow_audio_sync.h
grep -rn "rxSendUs\|send_us" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Check renderDelayUs / 2 logic and how enDeadline is used ==="
rg -n "enDeadline\|deadline_us\|renderDelay" usermods/audioreactive/audio_reactive.cpp
echo ""
echo "=== Any direct read of ENPacket::pad on receiver side ==="
rg -n "ep->pad\|ep\.pad" usermods/audioreactive/espnow_audio_sync.h
echo ""
echo "=== Stats::dupes field reference outside of stats() ==="
rg -rn "\.dupes"
Length of output: 1823
@softhack007 Here is the full scan result. I found three confirmed dead-code items and one item to double-check in the caller.
1. _rxSendUs — Write-only dead variable ✂️
Lines 117 and 357.
static uint32_t _rxSendUs = 0; // line 117 — declared
// ...
_rxSendUs = ep->send_us; // line 357 — written in handleIncomingPacket()_rxSendUs is never read. poll() computes the deadline exclusively from _rxArrivedLocalUs; the sender timestamp that was carefully captured from ep->send_us is silently discarded. Either:
- Remove
_rxSendUsand the assignment on line 357 (simplest), or - Actually use it in
poll()to compute a network-latency-compensated deadline (arrLocalUs - (arrLocalUs - rxSendUs)gives round-trip offset) — which was presumably the original intent.
2. Stats::dupes field — Populated but never consumed ✂️
Lines 135, 348, 395.
static uint32_t _stat_dupes = 0; // declared
// ...
if (diff <= 0) { _stat_dupes++; return true; } // incremented
// ...
return Stats{_stat_rx, _stat_dupes, _stat_gaps, _stat_tx, _stat_tx_err}; // returnedIn audio_reactive.cpp, the stats() result is used as follows — s.dupes is the only field never referenced:
auto s = espnowAudioSync::stats();
infoArr.add(s.rx); // ✔ used
// s.gaps used in frame-loss formula ✔
infoArr.add(s.tx); // ✔ used
// s.tx_err checked (> 0) ✔
// s.dupes — never touched ✗Either add it to the info pane (e.g. as "ESP-NOW Dupes Dropped") or remove _stat_dupes, the increment in handleIncomingPacket(), and Stats::dupes.
3. ep.pad = 0 in send() — Redundant assignment ✂️
Lines 255 and 260.
memset(&ep, 0, sizeof(ep)); // line 255 — zeroes the entire struct
ep.magic = EN_MAGIC;
ep.version = EN_VERSION;
ep.flags = 0; // line 258 — redundant (already 0)
ep.seq = ++_txSeq;
ep.pad = 0; // line 260 — redundant (already 0)memset at line 255 already zeroes every byte of ep, so ep.pad = 0 (and ep.flags = 0) are no-ops. ep.flags = 0 is at least visually useful before being set to 1 for the duplicate copy; ep.pad = 0 contributes nothing at all. Remove line 260.
4. enDeadline in audio_reactive.cpp — Double-check the caller ⚠️
The poll() call on line 1315 passes &enDeadline:
if (espnowAudioSync::poll(&enFrame, sizeof(enFrame), &enDeadline, /*renderDelayUs=*/15000)) {However, the search for any subsequent use of enDeadline returned no results (the rg regex had a syntax error, so this is not conclusive). It is worth verifying that enDeadline is actually acted on after poll() returns — if it is declared but then never read, the entire deadline_us / renderDelayUs machinery in poll() is dead from the caller's perspective as well.
Non-issues (for completeness)
| Symbol | Status |
|---|---|
EN_PREAMBLE |
Only used to define EN_PACKET; a named intermediate constant — not harmful, fine to keep for readability. |
ENPacket::pad struct field |
Structurally needed for the static_assert layout; the explicit assignment is redundant (see item 3), but the field itself should stay. |
All other file-scope statics (_channel, _hosted, _rxReady, _rxArrivedLocalUs, _lastRxSeq, _hasLastRxSeq, _rxMux, _stat_rx, _stat_gaps, _stat_tx, _stat_tx_err) |
All legitimately used. |
begin(), send(), poll(), handleIncomingPacket(), isHosted(), stats() |
All called from audio_reactive.cpp. |
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, do not flag precedence issues for expressions that mix additive operators (`+`, `-`) with shift operators (`<<`, `>>`) based on the assumption that shifts bind tighter. Per C/C++ operator precedence, `+`/`-` have higher precedence than `<<`/`>>` (e.g., `x - edge0 << 8` parses as `(x - edge0) << 8`). When reviewing WLED (and other) fixed-point/bit-manipulation code, confirm the intended parse using cppreference before reporting a precedence bug for mixed `-`/`+` and `<<`/`>>` expressions.
missing `}` don't people try to compile before making a PR?
coderabbitai
Bot
added
the
external
softhack007
removed
the
external
| * before. | ||
| * | ||
| * Features: | ||
| * - ESP-NOW broadcast (~2-5 ms one-way vs ~8-30 ms UDP/Wi-Fi) |
There was a problem hiding this comment.
Just for explaination: the observed delays are only a minor problem. sound sync sends one packet each 22millis, and the listed network delays (wifi included) are both the same magnitude, so not going to add noticeable lagging.
"Wifi sleep" - if enabled - is actually the relevant "realtime killer". If just one client on your wifi allows sleep mode, the AP/router is allowed to send udp data in batches, with up to 500ms between batches.
| ep.flags = 0; | ||
| ep.seq = ++_txSeq; | ||
| ep.pad = 0; | ||
| ep.send_us = (uint32_t)(esp_timer_get_time() & 0xFFFFFFFFULL); |
There was a problem hiding this comment.
This is a microsecond timer, it will roll-over about once per hour. Are you sure?
| _stat_rx++; | ||
| memcpy(_rxPayload, ep->payload, EN_PAYLOAD); | ||
| _rxSendUs = ep->send_us; | ||
| _rxArrivedLocalUs = (uint32_t)(esp_timer_get_time() & 0xFFFFFFFFULL); |
There was a problem hiding this comment.
What do you want to measure here? Getting two esp32 into time sync on microseconds level is an impossible mission....
There was a problem hiding this comment.
not impossible but futile ;)
|
@softhack007 I did not really look into this code: is the general approach viable? From glancing over the code it looks like its a full re-implementation of ESPnow code and duplicates what is already implemented in the main code - I do know there is a hook for receiving messages, I do not know if there is one for sending them but that would be the proper way instead of making AR fully incompatible with any other ESPNow functionality. |
|
@DedeHai yes I agree, I also have doubts about the general approach.
My Conclusion
|
|
For completeness - I've found two PR that also suggest a way for adding esp-now in AR. They all need rework but can be examples for learning what's needed. |
|
Thanks, everyone, for the comments. I will work from that. No, I do not have enough experience in C++, but I will try to see from these comments if I can do anything. In my case solved the issue of connecting multiple cards, which failed with the UDP connection. This code implementation also works with the espnow normal function, and if a command is sent will receive it on the slave card. In the menu, by selecting ESPnow instead of UDP, it automatically deselects it to answer the question. Regarding the C3, no testing has been done yet. |
3 participants
Hello, first of all, I would like to thank everyone for the Wled. I have been using it for many years, and it is simply amazing. With the help of the AI I made this change that adds an option to the menu to connect multiple sound reactive devices with ESP now instead of UDP since I was having problems with the connection.
I built a custom-made hardware with an ESP32 Ue , PCM1808 and a Bluetooth receiver. The file compiles, and it works nicely so far. On the INFO menu, also display the frame loss and the sync status.
Recommended setup:
Master on send, Slave on receive, ESPNOW, and in case of different pinout : CheckboxSettingBrightness✅Color✅Effects✅Palette / Effect arguments✅Main segment only✅Segment options / Segment bounds / Segment data❌ untickedApply incoming sync✅
Known issue:
Every ESP32 has one radio. That radio can only be on one 2.4 GHz channel at a time. In a normal single-AP Wi-Fi setup, every WLED board associates to the same AP, so every radio ends up on the same channel — ESP-NOW broadcasts reach everyone. In a mesh, each mesh node (satellite) operates on its own channel, and your WLED boards roam to whichever node is closest. So board #1 ends up on channel 1, board #2 on channel 6, board #3 on channel 11 — and they literally can't hear each other's ESP-NOW packets even though they're on the same Wi-Fi network. WLED's state sync still works (it goes through the AP and onto the IP network), but ESP-NOW is peer-to-peer and channel-pinned.
Easiest fix: Lock the mesh to a single 2.4 GHz channel.
Audio Reactive ESP-NOW Patch — Complete Edit Recipe
This is the full explanation of the code from the AI:
Audio Reactive ESP-NOW Patch — Complete Edit Recipe
Canonical single-doc version. Applies to a clean copy of WLED v16.0.0's usermods/audioreactive/audio_reactive.cpp (2307 lines). Eleven edits total — overlapping ones from the original two-doc workflow are now merged.
Drop espnow_audio_sync.h into the same folder before starting.
How to apply each edit: in VSCode, Ctrl+F → click the small arrow to open Find & Replace → paste the FIND block → paste the REPLACE block → hit Replace once. Each FIND block is unique in the file, so there's no ambiguity.
Edit 0 — include the transport module
What it does: makes the ESP-NOW transport API available to the rest of the file.
FIND: (in the top include block, around line 12 — already done in your current file if you see this line)
#include <esp_timer.h>
REPLACE WITH:
#include <esp_timer.h>
#include "espnow_audio_sync.h"
Edit 1 — class members for the two GUI settings
What it does: adds two uint8_t members that hold the user's choices: which transport (UDP or ESP-NOW) and which channel (auto or forced 1–13).
FIND:
uint16_t audioSyncPort= 11988;// default port for UDP sound sync
REPLACE WITH:
uint16_t audioSyncPort= 11988;// default port for UDP sound sync
uint8_t audioSyncTransport = 0; // NEW: 0 = UDP (stock), 1 = ESP-NOW (low-latency)
uint8_t espnowChannel = 0; // NEW: 0 = auto (follow Wi-Fi), 1-13 = force channel
Edit 2 — config-key string declarations
What it does: declares the names that will appear as JSON keys in cfg.json under the sync section.
FIND:
static const char UDP_SYNC_HEADER[];
static const char UDP_SYNC_HEADER_v1[];
REPLACE WITH:
static const char UDP_SYNC_HEADER[];
static const char UDP_SYNC_HEADER_v1[];
static const char _syncTransport[];
static const char _espnowChannel[];
Edit 3 — config-key string definitions
What it does: assigns the actual JSON-key strings ("transport" and "espnow-channel") to the labels declared above. These are stored in PROGMEM to save RAM.
FIND:
const char AudioReactive::UDP_SYNC_HEADER_v1[] PROGMEM = "00001"; // old sync header version - need to add backwards-compatibility feature
REPLACE WITH:
const char AudioReactive::UDP_SYNC_HEADER_v1[] PROGMEM = "00001"; // old sync header version - need to add backwards-compatibility feature
const char AudioReactive::_syncTransport[] PROGMEM = "transport";
const char AudioReactive::_espnowChannel[] PROGMEM = "espnow-channel";
Edit 4 — initialise ESP-NOW when Wi-Fi connects
What it does: every time WLED's connected() fires (boot or Wi-Fi reconnect), this brings up the ESP-NOW transport. The configured channel overrides the auto-detected one if the user has chosen a fixed channel in the GUI. espnowAudioSync::begin() auto-detects whether to run hosted (alongside WLED's own ESP-NOW for WiZmote/state-sync) or standalone (we own the radio).
FIND:
void connected() override
{
if (udpSyncConnected) { // clean-up: if open, close old UDP sync connection
udpSyncConnected = false;
fftUdp.stop();
}
REPLACE WITH:
void connected() override
{
if (udpSyncConnected) { // clean-up: if open, close old UDP sync connection
udpSyncConnected = false;
fftUdp.stop();
}
Edit 5 — send over ESP-NOW (master)
What it does: in transmitAudioData(), if the user has selected the ESP-NOW transport, hand the packet to ESP-NOW. Otherwise keep doing the stock UDP multicast. Either path uses the same 44-byte V2 packet.
FIND:
if (fftUdp.beginMulticastPacket() != 0) { // beginMulticastPacket returns 0 in case of error
fftUdp.write(reinterpret_cast<uint8_t *>(&transmitData), sizeof(transmitData));
fftUdp.endPacket();
}
REPLACE WITH:
if (audioSyncTransport == 1) {
// NEW: ESP-NOW transport
espnowAudioSync::send(&transmitData, sizeof(transmitData));
} else {
// UDP transport (stock behaviour)
if (fftUdp.beginMulticastPacket() != 0) { // beginMulticastPacket returns 0 in case of error
fftUdp.write(reinterpret_cast<uint8_t *>(&transmitData), sizeof(transmitData));
fftUdp.endPacket();
}
}
Edit 6 — receive over ESP-NOW (slave)
What it does: in receiveAudioData(), if the user has selected the ESP-NOW transport, poll the addon for the latest packet and decode it through the same decodeAudioData() the UDP path uses. Otherwise the function falls through to the stock UDP code unchanged.
FIND:
bool receiveAudioData() // check & process new data. return TRUE in case that new audio data was received.
{
if (!udpSyncConnected) return false;
bool haveFreshData = false;
REPLACE WITH:
bool receiveAudioData() // check & process new data. return TRUE in case that new audio data was received.
{
if (audioSyncTransport == 1) {
// NEW: ESP-NOW transport
audioSyncPacket enFrame;
uint32_t enDeadline = 0;
if (espnowAudioSync::poll(&enFrame, sizeof(enFrame), &enDeadline, /renderDelayUs=/15000)) {
decodeAudioData(sizeof(enFrame), reinterpret_cast<uint8_t*>(&enFrame));
receivedFormat = 2;
last_UDPTime = millis();
return true;
}
return false;
}
if (!udpSyncConnected) return false;
bool haveFreshData = false;
Edit 7 — persist the new settings (save side)
What it does: adds both new settings to the sync JSON object that WLED writes to cfg.json whenever the user saves.
FIND:
JsonObject sync = top.createNestedObject("sync");
sync["port"] = audioSyncPort;
sync["mode"] = audioSyncEnabled;
}
REPLACE WITH:
JsonObject sync = top.createNestedObject("sync");
sync["port"] = audioSyncPort;
sync["mode"] = audioSyncEnabled;
sync[FPSTR(_syncTransport)] = audioSyncTransport;
sync[FPSTR(_espnowChannel)] = espnowChannel;
}
Edit 8 — persist the new settings (load side)
What it does: reads the two new settings back from cfg.json on boot. The (uint8_t)0 is the default used if the field is missing (first boot after flashing the patch, before the user has saved anything).
FIND:
configComplete &= getJsonValue(top["sync"]["port"], audioSyncPort);
configComplete &= getJsonValue(top["sync"]["mode"], audioSyncEnabled);
REPLACE WITH:
configComplete &= getJsonValue(top["sync"]["port"], audioSyncPort);
configComplete &= getJsonValue(top["sync"]["mode"], audioSyncEnabled);
configComplete &= getJsonValue(top["sync"][FPSTR(_syncTransport)], audioSyncTransport, (uint8_t)0);
configComplete &= getJsonValue(top["sync"][FPSTR(_espnowChannel)], espnowChannel, (uint8_t)0);
Edit 9 — add the two dropdowns to the Sound Settings page
What it does: emits the JavaScript that builds the Audio Sync Transport and espnow-channel dropdowns in WLED's web UI under Config → Sound Settings. Both appear right after the existing Sync (Off/Send/Receive) dropdown.
FIND:
uiScript.print(F("addOption(dd,'Receive',2);"));
REPLACE WITH:
uiScript.print(F("addOption(dd,'Receive',2);"));
// NEW: Audio Sync Transport selector (UDP vs ESP-NOW)
uiScript.print(F("dd=addDropdown(ux,'sync:transport');"));
uiScript.print(F("addOption(dd,'UDP (default)',0);"));
uiScript.print(F("addOption(dd,'ESP-NOW (low-latency)',1);"));
// NEW: ESP-NOW channel selector (Auto + 1..13)
uiScript.print(F("dd=addDropdown(ux,'sync:espnow-channel');"));
uiScript.print(F("addOption(dd,'Auto (follow Wi-Fi)',0);"));
uiScript.print(F("addOption(dd,'1',1);"));
uiScript.print(F("addOption(dd,'2',2);"));
uiScript.print(F("addOption(dd,'3',3);"));
uiScript.print(F("addOption(dd,'4',4);"));
uiScript.print(F("addOption(dd,'5',5);"));
uiScript.print(F("addOption(dd,'6',6);"));
uiScript.print(F("addOption(dd,'7',7);"));
uiScript.print(F("addOption(dd,'8',8);"));
uiScript.print(F("addOption(dd,'9',9);"));
uiScript.print(F("addOption(dd,'10',10);"));
uiScript.print(F("addOption(dd,'11',11);"));
uiScript.print(F("addOption(dd,'12',12);"));
uiScript.print(F("addOption(dd,'13',13);"));
Edit 10 — show ESP-NOW stats on the Info pane
What it does: when the user has switched to the ESP-NOW transport, the WLED home page's Info pane shows live counters: how many frames have been received, the rolling frame-loss percentage, how many packets the master has queued for transmit, any TX errors, and which mode the addon is running in (hosted / standalone). Healthy slaves show RX climbing fast and Frame Loss under 1 %.
FIND:
if (audioSyncEnabled && udpSyncConnected && (millis() - last_UDPTime < 2500)) {
if (receivedFormat == 1) infoArr.add(F(" v1"));
if (receivedFormat == 2) infoArr.add(F(" v2"));
}
REPLACE WITH:
if (audioSyncEnabled && udpSyncConnected && (millis() - last_UDPTime < 2500)) {
if (receivedFormat == 1) infoArr.add(F(" v1"));
if (receivedFormat == 2) infoArr.add(F(" v2"));
}
Edit 11 — coexistence hook with WLED's own ESP-NOW
What it does: WLED's core ESP-NOW dispatcher (in wled00/udp.cpp) calls UsermodManager::onEspNowMessage(...) on every incoming ESP-NOW packet before it does its own WiZmote / state-sync handling. By overriding this method on the AudioReactive class, we get first look at every packet. We claim audio-sync packets (their magic bytes match 0xA53E) and return true → WLED skips its own handling for that packet. For anything else (WiZmote button presses, ESP-NOW state sync, etc.) we return false and WLED proceeds normally. This is what lets the WiZmote remote and ESP-NOW state-sync features keep working alongside the audio sync. The whole thing is wrapped in #ifndef WLED_DISABLE_ESPNOW so the build still compiles if you ever disable ESP-NOW in build flags.
FIND: (this multi-line block is unique — copy it exactly, including the two blank lines between onStateChange and the addToConfig comment)
createAudioPalettes();
}
}
REPLACE WITH:
createAudioPalettes();
}
}
#ifndef WLED_DISABLE_ESPNOW
// ESP-NOW audio-sync coexistence hook. WLED's core ESP-NOW dispatcher
// (espNowReceiveCB in wled00/udp.cpp) calls this on every received
// packet BEFORE its own WiZmote/state-sync handling. We grab audio-sync
// packets and return true; WLED then skips the rest of its dispatch.
bool onEspNowMessage(uint8_t* sender, uint8_t* data, uint8_t len) override {
return espnowAudioSync::handleIncomingPacket(data, len);
}
#endif
Summary by CodeRabbit
New Features
Improvements