AR Sound receive: better management of outdated packets #314
Conversation
* rework of PR #231 * allow droping packets when - first connection - silence - re-connect after timeout - main loop is too slow
experimental
📝 WalkthroughWalkthroughAdds configurable per-packet UDP audio purge (audioSyncPurge) and read/flush constants; rewrites UDP receive to handle multiple packets per call with 8-bit rollover-safe sequencing, idle/resync and per-packet V1/V2 handling; annotates many public methods with Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as UDP Sender
participant Socket as Device UDP Socket
participant AR as AudioReactive
participant Decoder as FFT/Decoder
participant LEDs as LED Controller
Sender->>Socket: send audio packets (frames, seq)
Socket->>AR: receiveAudioData(maxSamples) invoked
AR->>AR: loop read packets (up to maxSamples), sequence check (8-bit rollover), idle/resync, audioSyncPurge decision
AR->>Decoder: deliver accumulated samples for FFT/decoding
Decoder->>LEDs: output levels to LED controller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
usermods/audioreactive/audio_reactive.h (4)
1762-1776: Consider cleaning up commented-out code before merging.The rollover-safe sequence check using
(int8_t)(cur - prev) > 0is correct and handles 8-bit counter wraparound properly. However, lines 1772-1773 contain commented-out alternative logic that could cause confusion.Since this is marked as WIP, consider either:
- Removing the commented alternative implementation
- Or documenting why it's kept for reference
1779-1779: Consider reverting toDEBUGSR_PRINTFbefore final merge.Using
USER_PRINTFinstead ofDEBUGSR_PRINTFwill output this message in production builds, potentially causing log spam if packets frequently arrive out of order. This may be intentional for debugging during WIP, but should be reverted toDEBUGSR_PRINTF(or wrapped in a conditional) before merging.♻️ Suggested fix
- USER_PRINTF("Skipping audio frame out of order or duplicated - %u vs %u\n", lastFrameCounter, receivedPacket.frameCounter); + DEBUGSR_PRINTF("Skipping audio frame out of order or duplicated - %u vs %u\n", lastFrameCounter, receivedPacket.frameCounter);
1858-1858: Consider documenting the magic value255for queue purge mode.The special value
255formaxSamplesmeaning "purge the queue" is used here and at call sites (lines 2362-2364), but it's not immediately obvious. Consider defining a named constant.♻️ Suggested improvement
+ static constexpr unsigned PURGE_UDP_QUEUE = 255; // special value to purge complete input queue + - bool receiveAudioData( unsigned maxSamples) { // maxSamples = 255 means "purge complete input queue" + bool receiveAudioData( unsigned maxSamples) { // maxSamples = PURGE_UDP_QUEUE means "purge complete input queue"
1899-1909: **Remove redundantreceivedFormatassignment.**Line 1901 assignsreceivedFormat = 2, but this value is immediately overwritten bydecodeAudioData()(lines 1757-1758), making it a dead store - a local variable that is assigned a value but is read by no following instruction, wasting processor time.♻️ Remove dead assignment
if (lastValidPacketSize > 0) { if (lastValidPacketSize == sizeof(audioSyncPacket) && (isValidUdpSyncVersion((const char *)fftUdpBuffer))) { - receivedFormat = 2; haveFreshData |= decodeAudioData(lastValidPacketSize, fftUdpBuffer); } else if (lastValidPacketSize == sizeof(audioSyncPacket_v1) && (isValidUdpSyncVersion_v1((const char *)fftUdpBuffer))) { decodeAudioData_v1(lastValidPacketSize, fftUdpBuffer);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
usermods/audioreactive/audio_reactive.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
🔇 Additional comments (2)
usermods/audioreactive/audio_reactive.h (2)
1192-1192: LGTM!Good addition of a named constant to document the expected UDP batching rate. This replaces magic numbers with a meaningful identifier.
2357-2366: LGTM! Well-designed bounded packet processing.The logic correctly:
- Estimates how many packets could have accumulated based on elapsed time
- Constrains to a reasonable upper bound (20 packets ≈ 460ms)
- Handles edge cases (timeout, new connection, silence) with purge mode
This should effectively prevent queue buildup while avoiding excessive processing in a single loop iteration.
remove early assignment receivedFormat = 2
experimental -> works removing old code
experimental -> works
* add "override" to all methods that oveeride the base usermod class * fixing a minor copy-paste mistake * ovoid shadowing class attributes * spelling, grammar and other nitpick
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (3)
usermods/audioreactive/audio_reactive.h (3)
1848-1904: Bug risk: malformed packet sizes can cause stale-buffer reprocessing; unknown packets can incorrectly forcereceivedFormat=0.
- If
packetSize == 5, you don’t read new data but still processlastValidPacketSizefrom a previous iteration.- Setting
receivedFormat = 0on an unknown packet can undo a valid decode from an earlier packet in the same call and trigger “new connection flush” behavior next loop.Proposed fix
bool receiveAudioData( unsigned maxSamples) { // maxSamples = AR_UDP_FLUSH_ALL (255) means "purge complete input queue" if (!udpSyncConnected) return false; bool haveFreshData = false; size_t packetSize = 0; static uint8_t fftUdpBuffer[UDPSOUND_MAX_PACKET + 1] = {0}; - size_t lastValidPacketSize = 0; // Loop to read available packets unsigned packetsReceived = 0; do { + size_t lastValidPacketSize = 0; // must be per-iteration to avoid re-processing stale buffers + #if __cpp_exceptions try { packetSize = fftUdp.parsePacket(); } catch (...) { packetSize = 0; #ifdef ARDUINO_ARCH_ESP32 fftUdp.flush(); #endif DEBUG_PRINTLN(F("receiveAudioData: parsePacket out of memory exception caught!")); USER_FLUSH(); } #else packetSize = fftUdp.parsePacket(); #endif #ifdef ARDUINO_ARCH_ESP32 if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) { fftUdp.flush(); continue; // Skip invalid packets -> next iteration } #endif if (packetSize == 0) break; // No more packets available --> exit loop - if ((packetSize > 5) && (packetSize <= UDPSOUND_MAX_PACKET)) { - fftUdp.read(fftUdpBuffer, packetSize); - lastValidPacketSize = packetSize; - } + // Always drain the packet; only decode known sizes. + const size_t toRead = min(packetSize, (size_t)UDPSOUND_MAX_PACKET); + const int readLen = fftUdp.read(fftUdpBuffer, toRead); + lastValidPacketSize = (readLen > 0) ? (size_t)readLen : 0; // Process each received packet: last value will persist, intermediate ones needed to update sequence counters if (lastValidPacketSize > 0) { if (lastValidPacketSize == sizeof(audioSyncPacket) && (isValidUdpSyncVersion((const char *)fftUdpBuffer))) { haveFreshData |= decodeAudioData(lastValidPacketSize, fftUdpBuffer); } else if (lastValidPacketSize == sizeof(audioSyncPacket_v1) && (isValidUdpSyncVersion_v1((const char *)fftUdpBuffer))) { decodeAudioData_v1(lastValidPacketSize, fftUdpBuffer); receivedFormat = 1; haveFreshData = true; } else { - receivedFormat = 0; // unknown format + // Ignore unknown packets; don't clobber last known-good format. } } packetsReceived++; } while ((packetSize > 0) && ((packetsReceived < maxSamples) || (maxSamples == AR_UDP_FLUSH_ALL)));
2832-2885: Config safety: clampaudioSyncPurgeto the supported range (0..2).
Right now any stored value will flow into theswitch(and default→auto), but it can also leak into UI/state and make behavior hard to reason about.Proposed fix
configComplete &= getJsonValue(top["sync"][F("port")], audioSyncPort); configComplete &= getJsonValue(top["sync"][F("mode")], audioSyncEnabled); configComplete &= getJsonValue(top["sync"][F("last_packet_only")], audioSyncPurge); configComplete &= getJsonValue(top["sync"][F("check_sequence")], audioSyncSequence); + + audioSyncPurge = constrain(audioSyncPurge, (uint8_t)0, (uint8_t)2);Also applies to: 2903-2969
3195-3204: Minor: user-facing text typo (“dropy”).
This string is in the settings UI path, so it’ll be visible.
🧹 Nitpick comments (3)
usermods/audioreactive/audio_reactive.h (3)
130-134:disableSoundProcessingis cross-task state;volatilehelps but doesn’t make it “thread-safe”.
If this flag is written from one task/core and read from another,volatilealone doesn’t provide atomicity/ordering guarantees (even thoughboolwrites are typically atomic). Consider usingstd::atomic<bool>(if available) or a small critical section around state transitions that must be coherent with other shared fields.
1192-1195: Preferstatic constexprover#definefor these private constants.
#define UDPSOUND_MAX_PACKET / AR_UDP_*are not scoped (despite being placed “inside” the class), which risks macro collisions across the build.Proposed refactor
- #define UDPSOUND_MAX_PACKET 96 // max packet size for audiosync, with a bit of "headroom" - #define AR_UDP_AVG_SEND_RATE 18 // 23ms = time for reading one new batch of samples @ 22kHz; minus 5ms margin for network overhead - #define AR_UDP_FLUSH_ALL 255 // tells receiveAudioData to purge the network input queue + static constexpr size_t UDPSOUND_MAX_PACKET = 96; // max packet size for audiosync, with a bit of "headroom" + static constexpr uint8_t AR_UDP_AVG_SEND_RATE = 18; // see comment above + static constexpr uint8_t AR_UDP_FLUSH_ALL = 255; // tells receiveAudioData to purge the network input queue
1755-1774: Sequence logic looks solid, but consider explicitly resettinglastFrameCounteron resync conditions.
Right now you “accept anything” on idle/first packets, but you still keep the oldlastFrameCountervalue. Resetting it when(millis() - last_UDPTime) >= AUDIOSYNC_IDLE_MS(or whenreceivedFormattransitions from 0→valid) makes the intent clearer and avoids edge-case interactions with the legacyframeCounter==0exception.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:58:27.987Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.987Z
Learning: In WLED AudioReactive usermod's runMicFilter function, the different cutoff frequencies between float (80Hz) and integer (90Hz) FFT implementations are intentional. The higher 90Hz cutoff for integer math helps preserve precision by avoiding overly aggressive filtering where integer arithmetic limitations could compound errors. The 10Hz difference between 80Hz and 90Hz is negligible from an audio performance standpoint but provides better precision for integer calculations.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-29T20:31:11.736Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.736Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
usermods/audioreactive/audio_reactive.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
🔇 Additional comments (1)
usermods/audioreactive/audio_reactive.h (1)
1579-1604:estimatePressure()bounds checks are a good hardening.
Early returns for min/max range avoid pathological log scaling and keep UI values stable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
usermods/audioreactive/audio_reactive.h (5)
1192-1194: Clarify constant naming and documentation.The constant
AR_UDP_AVG_SEND_RATEhas a value of18with a comment indicating it represents a time duration (23ms minus 5ms), but the name suggests it's a "rate" rather than a time interval. Consider renaming toAR_UDP_AVG_SEND_INTERVAL_MSor updating the comment to clarify that this represents a time value in milliseconds.📝 Suggested clarification
-#define AR_UDP_AVG_SEND_RATE 18 // 23ms = time for reading one new batch of samples @ 22kHz; minus 5ms margin for network overhead +#define AR_UDP_AVG_SEND_INTERVAL_MS 18 // 23ms = time for reading one new batch of samples @ 22kHz; minus 5ms margin for network overheadThen update usage at line 2351:
-unsigned maxReadSamples = timeElapsed / AR_UDP_AVG_SEND_RATE; // estimate how many packets arrived since last receive +unsigned maxReadSamples = timeElapsed / AR_UDP_AVG_SEND_INTERVAL_MS; // estimate how many packets arrived since last receive
678-683: Minor: Improve comment clarity and fix indentation.The comment on line 678 describes two distinct operations. Consider formatting it as a list for better readability. Also, line 682 appears to use a tab character which is inconsistent with the surrounding space-based indentation.
📝 Suggested improvements
- // find the highest sample in the batch, and count zero crossings + // Process FFT batch samples: + // - find the highest sample in the batch + // - count zero crossings float maxSample = 0.0f; // max sample from FFT batch uint_fast16_t newZeroCrossingCount = 0; for (int i=0; i < samplesFFT; i++) { - // pick our current mic sample - we take the max value from all samples that go into FFT + // pick our current mic sample - we take the max value from all samples that go into FFT if ((vReal[i] <= (INT16_MAX - 1024)) && (vReal[i] >= (INT16_MIN + 1024))) { //skip extreme values - normally these are artefacts
1868-1869: Remove confusing commented code.The commented-out
continuestatement with the note "don't skip to next iteration -> we are OOM" is confusing. If the intention is to not skip when out of memory, then the current code (which doesn't skip) is correct. The comment and dead code should be removed to avoid confusion.🧹 Proposed cleanup
DEBUG_PRINTLN(F("receiveAudioData: parsePacket out of memory exception caught!")); USER_FLUSH(); - //continue; // don't skip to next iteration -> we are OOM }
2349-2367: LGTM: Well-designed purge decision logic.The packet purge logic correctly implements the three modes described in the PR objectives:
- Mode 0: Process each packet sequentially
- Mode 1 (default): Auto-purge when silence detected or receiver is slower than sender
- Mode 2: Always purge to process only the latest packet
The special handling for new connections and timeouts ensures proper resynchronization. The constraint to a maximum of 20 packets (380ms) prevents excessive queue processing during prolonged delays.
One minor suggestion: The magic number
20Uon line 2352 could be defined as a named constant for clarity.
2883-2883: Consider renaming the config key for clarity.The JSON key
last_packet_onlyis somewhat misleading since a value of0means "process all packets" (not just the last one). Consider renaming topacket_purge_modeordrop_packetsfor better alignment with the actual behavior and the variable nameaudioSyncPurge.Current mapping:
last_packet_only: 0→ "Never" drop packetslast_packet_only: 1→ "Auto" drop packetslast_packet_only: 2→ "Always" drop packets (last only)The UI labels are clear, but the JSON key name could better reflect that it controls a mode rather than a boolean behavior.
Also applies to: 2967-2967, 3195-3199
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:58:27.987Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.987Z
Learning: In WLED AudioReactive usermod's runMicFilter function, the different cutoff frequencies between float (80Hz) and integer (90Hz) FFT implementations are intentional. The higher 90Hz cutoff for integer math helps preserve precision by avoiding overly aggressive filtering where integer arithmetic limitations could compound errors. The 10Hz difference between 80Hz and 90Hz is negligible from an audio performance standpoint but provides better precision for integer calculations.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-29T20:31:11.736Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.736Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
usermods/audioreactive/audio_reactive.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
🔇 Additional comments (5)
usermods/audioreactive/audio_reactive.h (5)
1579-1604: LGTM: Const correctness improvement.The addition of
constto theestimatePressure()function signature is appropriate since the method doesn't modify any member variables. The renamed constants (sampleRangeMin,sampleRangeMax) also improve code clarity.
1764-1768: LGTM: Robust rollover-safe sequence checking.The 8-bit rollover-safe sequence check using
(int8_t)(receivedPacket.frameCounter - lastFrameCounter) > 0correctly handles counter wraparound. The additional conditions for timeout-based resync and accepting the first V2 packet improve robustness when streams restart or connections are re-established.
1855-1903: LGTM: Multi-packet processing logic.The loop correctly processes multiple UDP packets based on the
maxSamplesparameter:
- Processes up to
maxSamplespackets, or all available packets whenmaxSamples == AR_UDP_FLUSH_ALL- Updates sequence counters for each packet to maintain proper tracking
- Accumulates fresh data status with OR operation
- Properly handles invalid packet sizes and continues to next packet
The logic enables flexible queue management as described in the PR objectives.
1923-1923: LGTM: Proper use of override specifiers.The addition of
overridespecifiers to all virtual method overrides is excellent practice. This provides compile-time verification that these methods actually override base class virtuals and will help catch errors if the base class interface changes in the future.Also applies to: 2176-2176, 2216-2216, 2450-2450, 2455-2455, 2464-2464, 2553-2553, 2576-2576, 2763-2763, 2778-2778, 2832-2832, 2903-2903, 2985-2985, 3267-3267
2973-2978: LGTM: Proper reboot requirement detection.The code correctly identifies when I2S pin configuration changes require a system reboot or power cycle, providing appropriate user feedback through error flags. This prevents configuration issues from causing mysterious failures.
@coderabbitIA i need a short and clear key name. The key will be shown in the UI (just underscores->spaces), so it should be clear for audioreactive users who usually don't know about the internal of UDP and networks. |
|
Tip For best results, initiate chat on the files or code changes. @softhack007, for a short and user-friendly name that works well in the UI, I'd suggest: Recommended option:
This phrasing is intuitive for audioreactive users—they understand they're receiving audio data and that older data can be skipped when newer data arrives. Alternative options:
I'd recommend 🧠 Learnings used |
* rename user option to "skip old data" * remove unneeded variable lastValidPacketSize * rename constant AR_UDP_AVG_SEND_RATE -> AR_UDP_AVG_SEND_RATE
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
usermods/audioreactive/audio_reactive.h (3)
1848-1907: Verify the OOM exception handling comment clarity.Line 1867's comment
//continue; // don't skip to next iteration -> we are OOMmight be confusing. The commented-outcontinuewith the explanation "don't skip to next iteration" could be misread. Consider rephrasing to make it clearer that we intentionally do NOT continue here because we're out of memory and should exit the loop.📝 Suggested comment improvement
- //continue; // don't skip to next iteration -> we are OOM + // Don't continue here - we're OOM and should exit the loop to avoid further issues
2352-2360: Consider tuning the silence threshold for auto-purge mode.Line 2359 uses
fabsf(volumeSmth) < 0.25fto detect silence and trigger packet purging in auto mode. This threshold might be too sensitive and could cause unnecessary packet drops during quiet musical passages. Consider whether this threshold aligns with the noise gate threshold used elsewhere in the code (e.g., line 2359 vs. thesoundSquelchparameter).Would it be more consistent to use the existing noise gate logic (checking against
soundSquelch) instead of a hardcoded 0.25f threshold?
709-709: Minor typo in comment.Comment says "compute mix/max again" but likely means "min/max" based on the variable names
datMinanddatMaxbeing computed below.📝 Typo fix
- // compute mix/max again after filtering - useful for filter debugging + // compute min/max again after filtering - useful for filter debugging
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:58:27.987Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.987Z
Learning: In WLED AudioReactive usermod's runMicFilter function, the different cutoff frequencies between float (80Hz) and integer (90Hz) FFT implementations are intentional. The higher 90Hz cutoff for integer math helps preserve precision by avoiding overly aggressive filtering where integer arithmetic limitations could compound errors. The 10Hz difference between 80Hz and 90Hz is negligible from an audio performance standpoint but provides better precision for integer calculations.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-29T20:31:11.736Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.736Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
usermods/audioreactive/audio_reactive.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
🔇 Additional comments (6)
usermods/audioreactive/audio_reactive.h (6)
2348-2350: Good implementation of adaptive packet reading.The logic to estimate
maxReadSamplesbased on elapsed time and the send interval is well-designed. Constraining to [1...20] packets (representing up to 380ms of data) provides a good balance between catching up on delayed packets and preventing excessive processing.
1764-1774: Excellent 8-bit rollover-safe sequence checking.The implementation correctly handles sequence number wraparound using
(int8_t)(receivedPacket.frameCounter - lastFrameCounter) > 0. The multiple resync conditions (idle timeout, first V2 packets, frameCounter==0) provide good robustness and backward compatibility.
1886-1898: Per-packet processing improves sequence tracking accuracy.Processing each packet individually (not just the last one) ensures that sequence counters remain accurate even when older packets are dropped. This is the correct approach for reliable stream synchronization.
3194-3197: Clear and user-friendly config option naming.The JSON key
skip_old_datawith UI labels "Never", "Auto (recommended)", and "Always" is clear and understandable for users. Good implementation of the PR objective to rename fromlast_packet_onlyto a more intuitive name.
1921-3265: Consistent use of override specifiers improves code safety.Adding
overridespecifiers to all virtual method implementations is excellent C++ practice. This helps catch errors at compile-time if the base class signature changes and improves code maintainability.
1192-1194: AR_UDP_SEND_INTERVAL_MS does not control transmit timing—revise the review premise.
AR_UDP_SEND_INTERVAL_MS = 18is used in the receive path (line 2349) to estimate incoming packet count (maxReadSamples = timeElapsed / AR_UDP_SEND_INTERVAL_MS), not to set transmit timing. The actual transmit interval is controlled by timing checks at lines 2434-2438:
- Standard mode:
millis() - lastTime > 20→ sends every 20ms- Fastpath mode:
millis() - lastTime > 24→ sends every ~25ms (or immediately whenhaveNewFFTResultflag is set)The constant's value (18) differs from both actual transmit intervals (20ms standard, ~25ms fastpath), and the comment's arithmetic (23ms − 5ms = 18ms) lacks connection to the actual transmit implementation. Clarify whether this constant's documentation should reflect its actual use (receive-side packet estimation) rather than suggesting it controls transmit timing.
Likely an incorrect or invalid review comment.
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.h (1)
1848-1902: Critical: some invalid packet sizes can be left unconsumed (risk: receiver stuck on the same bad UDP datagram).
packetSizeissize_t, butparsePacket()is typicallyint; if an error path returns a negative, it can wrap huge.- On ESP32 you flush when
packetSize < 5, but apacketSize == 5packet is neither flushed nor read.- On non-ESP32 there is no oversize/undersize discard path, and you also skip reading for
packetSize <= 5or> UDPSOUND_MAX_PACKET.
Net effect: malformed/oversize datagrams may never be drained, soparsePacket()can keep returning the same packet forever.Proposed fix (always drain/discard invalid packets; use signed `int` for size)
- size_t packetSize = 0; + int packetSize = 0; static uint8_t fftUdpBuffer[UDPSOUND_MAX_PACKET + 1] = {0}; ... - #ifdef ARDUINO_ARCH_ESP32 - if ((packetSize > 0) && ((packetSize < 5) || (packetSize > UDPSOUND_MAX_PACKET))) { - fftUdp.flush(); - continue; // Skip invalid packets -> next iteration - } - #endif - - if (packetSize == 0) break; // No more packets available --> exit loop - - if ((packetSize > 5) && (packetSize <= UDPSOUND_MAX_PACKET)) { - fftUdp.read(fftUdpBuffer, packetSize); - } + if (packetSize <= 0) break; // No more packets available --> exit loop + + // Always consume the UDP datagram, even if malformed/oversized. + if ((packetSize < 6) || (packetSize > (int)UDPSOUND_MAX_PACKET)) { + // drain/discard + while (packetSize-- > 0) (void)fftUdp.read(); + receivedFormat = 0; + packetsReceived++; + continue; + } + + // Read full packet payload (size is now known-good and within buffer limits) + fftUdp.read(fftUdpBuffer, packetSize);
🤖 Fix all issues with AI agents
In @usermods/audioreactive/audio_reactive.h:
- Around line 2878-2883: The config write currently uses "skip_old_data" but the
read path only reads the new key, causing upgrades to drop older settings;
update the config-read logic that assigns audioSyncPurge (and any other
locations reading this setting) to first check for "skip_old_data" and, if
absent, fall back to the legacy "last_packet_only" key (preserving
semantics/inverting if necessary), e.g., when parsing the "sync" JsonObject set
audioSyncPurge = valueOf("skip_old_data") ?: valueOf("last_packet_only"); apply
the same fallback wherever audioSyncPurge is loaded.
🧹 Nitpick comments (3)
usermods/audioreactive/audio_reactive.h (3)
1192-1195: Avoid#definemacros in headers for these constants (preferstatic constexpr).
UDPSOUND_MAX_PACKET,AR_UDP_READ_INTERVAL_MS,AR_UDP_FLUSH_ALL, andCYCLE_SAMPLEMAXare all unscoped macros and can leak/collide across translation units.Proposed refactor
- #define UDPSOUND_MAX_PACKET 96 // max packet size for audiosync, with a bit of "headroom" - #define AR_UDP_READ_INTERVAL_MS 18 // 23ms = time for reading one new batch of samples @ 22kHz; minus 5ms margin for network overhead - #define AR_UDP_FLUSH_ALL 255 // tells receiveAudioData to purge the network input queue + static constexpr size_t UDPSOUND_MAX_PACKET = 96; // max packet size for audiosync, with a bit of "headroom" + static constexpr unsigned AR_UDP_READ_INTERVAL_MS = 18; // 23ms batch @22kHz minus margin for network overhead + static constexpr unsigned AR_UDP_FLUSH_ALL = 255; // tells receiveAudioData to purge the network input queue- #define CYCLE_SAMPLEMAX 3500 // time window for measuring + static constexpr unsigned CYCLE_SAMPLEMAX = 3500; // time window for measuringAlso applies to: 1240-1240
1756-1774: Sequence/format state update happens even when you reject the packet.
receivedFormatis updated before the sequence check; when the packet is rejected (Line 1768-1771), the UI/state may still reflect “v2/v2+”, and the receiver logic that keys offreceivedFormatcan make different decisions than intended.One way to tighten this up
- int lastReceivedFormat = receivedFormat; - // add info for UI - if ((receivedPacket.frameCounter > 0) && (lastFrameCounter > 0)) receivedFormat = 3; // v2+ - else receivedFormat = 2; // v2 + int lastReceivedFormat = receivedFormat; + int nextReceivedFormat = ((receivedPacket.frameCounter > 0) && (lastFrameCounter > 0)) ? 3 : 2; // v2+ : v2 ... - if((sequenceOK == false) && (receivedPacket.frameCounter != 0)) { // always accept "0" as the legacy value + if((sequenceOK == false) && (receivedPacket.frameCounter != 0)) { // always accept "0" as the legacy value DEBUGSR_PRINTF("Skipping audio frame out of order or duplicated - %u vs %u\n", lastFrameCounter, receivedPacket.frameCounter); + // optionally: receivedFormat = lastReceivedFormat; // don't update UI/state on rejected packet return false; // reject out-of sequence frame } else { lastFrameCounter = receivedPacket.frameCounter; + receivedFormat = nextReceivedFormat; }
1579-1604: Pressure estimation changes look fine; consider scaling by 255 instead of 256 (optional).
Returningmin(max(256*scaled,0),255)effectively saturates the top end; if you want a true 0..255 mapping,255.0f * scaledvalueis slightly more intuitive.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2026-01-07T23:30:16.998Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:16.998Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-06-30T17:58:27.987Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.987Z
Learning: In WLED AudioReactive usermod's runMicFilter function, the different cutoff frequencies between float (80Hz) and integer (90Hz) FFT implementations are intentional. The higher 90Hz cutoff for integer math helps preserve precision by avoiding overly aggressive filtering where integer arithmetic limitations could compound errors. The 10Hz difference between 80Hz and 90Hz is negligible from an audio performance standpoint but provides better precision for integer calculations.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
usermods/audioreactive/audio_reactive.h
📚 Learning: 2025-12-29T20:31:11.736Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.736Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
usermods/audioreactive/audio_reactive.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (5)
usermods/audioreactive/audio_reactive.h (5)
130-135: Good: shared-task flag isvolatile, purge mode has a sane default.
disableSoundProcessingbeingvolatilematches the “shared between tasks” comment, and defaultingaudioSyncPurgeto1(Auto) is a reasonable UX default.
2348-2366: Purge/drop decision flow looks coherent; consider clampingaudioSyncPurgeto [0..2].
ThemaxReadSamplesderivation + mode switch is straightforward and should behave well under backlog/silence. I’d clampaudioSyncPurgeafter config load to avoid surprising “default:” behavior from arbitrary values.
1921-1921:overrideannotations improve safety/readability.
These changes reduce accidental signature mismatches and make the usermod interface contract clearer.Also applies to: 2174-2174, 2214-2214, 2453-2458, 2463-2522, 2551-2562, 2574-2575, 2761-2769, 2776-2793, 2830-2883, 2901-2980, 2983-3247, 3265-3268
320-340: Comment-only edits / wording tweaks.
Nothing to call out here beyond general readability improvements.Also applies to: 407-409, 678-690, 709-710, 753-755, 1299-1300, 1346-1353, 3261-3264
2448-2451: Confirmloop()double-invocation is intentional in FASTPATH builds.The
loop2()override is guarded by#if defined(_MoonModules_WLED_) && defined(WLEDMM_FASTPATH), so it only compiles in fastpath builds. Since bothusermods.loop()(wled.cpp:158) andusermods.loop2()(wled.cpp:235) are called in the main cycle, callingloop()fromloop2()causesloop()to execute twice per cycle in fastpath mode. Verify this double-invocation is intentional and thatloop()is designed to be idempotent when called twice, or if the fastpath path should call loop2 instead of loop.
Improvements for PR #231
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Polish
✏️ Tip: You can customize this high-level summary in your review settings.