Adding DSP FFT and integer math to AR #4750
Conversation
also added major peak and frequency bin calculation for DSP FFT
WalkthroughThe changes implement multiple FFT processing backends (ArduinoFFT, ESP-IDF DSP, and integer FFT) for the audioreactive usermod, with conditional compilation to select the appropriate variant. Buffer management, windowing, and filtering are adapted for each type. The audio sample acquisition interface is generalized to support both floating-point and integer sample buffers. The platformio.ini configuration is updated to enable the audioreactive usermod for the ESP32-C3 environment. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsusermods/audioreactive/audio_reactive.cpp (4)<retrieved_learning> <retrieved_learning> <retrieved_learning> <retrieved_learning> ⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (12)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
usermods/audioreactive/audio_source.h (1)
710-710: Critical: Method signature mismatch with base class.The
getSamplesmethod inI2SAdcSourcestill usesfloat*while the base classAudioSourcenow usesvoid*. This signature mismatch means the method won't properly override the virtual function, causing potential compilation errors or runtime issues.Apply this fix to match the base class signature:
- void getSamples(float *buffer, uint16_t num_samples) { + void getSamples(void *buffer, uint16_t num_samples) { + float *_buffer = static_cast<float*>(buffer);Then update line 725 to use
_bufferinstead ofbuffer:- I2SSource::getSamples(buffer, num_samples); + I2SSource::getSamples(_buffer, num_samples);
🧹 Nitpick comments (3)
usermods/audioreactive/audio_reactive.cpp (3)
371-373: Consider using separate buffers for clarityWhile the current approach works, reusing
valFFTas an int16_t buffer when it's declared as float* relies on type punning. Consider using a union or separate buffers for better type safety.
663-663: Remove redundant semicolon-sampleBuffer[i] = highFilteredSample_fp - (lowfilt_fp >> 15);; +sampleBuffer[i] = highFilteredSample_fp - (lowfilt_fp >> 15);
540-540: Fix comment formatting-// bins frequency range +// bins frequency range
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio.ini(1 hunks)usermods/audioreactive/audio_reactive.cpp(14 hunks)usermods/audioreactive/audio_source.h(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
platformio.ini (2)
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/audioreactive/audio_source.h (1)
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
usermods/audioreactive/audio_reactive.cpp (2)
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (11)
platformio.ini (1)
537-537: LGTM! Configuration aligns with PR objectives.Adding the audioreactive usermod to the ESP32-C3 build configuration is consistent with the PR's goal of enabling DSP FFT with integer math for C3 variants by default.
usermods/audioreactive/audio_source.h (3)
25-25: Correct removal of C3 from exclusion list.The removal of
CONFIG_IDF_TARGET_ESP32C3from the conditional compilation aligns with the PR objectives to support C3 variants with DSP FFT integer math implementation.
336-367: Implementation correctly handles dual buffer types.The conditional compilation properly splits the logic between float and integer FFT processing paths. The integer path optimization (no scaling, just bit shifting) is appropriate for performance-critical single-core MCUs like C3.
Note: The comment at line 358 mentions that scaling is not used for integer FFT to save ~0.4ms on C3, which is a reasonable performance optimization.
137-137: Breaking API change requires careful verification.Changing the virtual method signature from
float*tovoid*is a breaking change that affects all derived classes. While this enables flexibility for different buffer types, ensure all implementations are updated.Run the following script to verify all AudioSource implementations have updated signatures:
#!/bin/bash # Description: Find all AudioSource derived classes and check their getSamples signatures # Search for classes that inherit from AudioSource ast-grep --pattern 'class $_ : public AudioSource { $$$ }' # Search for getSamples method implementations rg -A 2 "getSamples\s*\(" --type cppusermods/audioreactive/audio_reactive.cpp (7)
27-68: Well-structured FFT variant selection with clear documentationThe conditional compilation logic and documentation effectively explain the FFT backend choices and their tradeoffs. The automatic selection of appropriate FFT implementations based on the target MCU is a good design decision.
70-86: Appropriate buffer management for different FFT variantsGood use of conditional typing and 16-byte alignment for SIMD optimization. The separation of real/imaginary buffers for ArduinoFFT vs interleaved format for DSP FFT is handled correctly.
217-221: Good function overloading for different sample typesThe overloaded
runMicFilterfunctions properly support both float and integer FFT paths.
275-289: Correct scaling between integer and float FFT resultsThe scaling factor of 32 properly compensates for the different magnitude scales between integer and float FFT implementations.
1399-1399: Appropriate band-pass filter configurationGood decision to enable the band-pass filter by default for noise reduction while allowing high-quality sources like ES8388 to bypass it.
Also applies to: 1448-1448
29-41: Excellent performance optimization strategyThe automatic selection of integer FFT for S2/C3 (no FPU) and float FFT for ESP32/S3 (with FPU) provides optimal performance for each platform as evidenced by the timing measurements in the PR description.
489-489: ```shell
#!/bin/bashLocate wled.h
wd=$(fd -e h wled.h)
echo "wled.h found at: $wd"Check if wled.h includes fcn_declare.h
grep -R "#include.*fcn_declare.h" -n "$wd" || echo "fcn_declare.h not included in wled.h"
Check for sqrt32_bw declaration directly in wled.h
grep -R "sqrt32_bw" -n "$wd" || echo "sqrt32_bw not directly declared in wled.h"
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
now also compiles on ESP8266
|
I would suggest you use a brilliant idea by @TripleWhy to define custom type instead of many Lines 1973 to 1993 in 66869f8 |
|
I initially wanted to do that but then decided to do it the classic way for easier debugging. I will check if and see if that approach results in cleaner code. |
|
I took a look at that approach and in some places it would work but IMHO it does not make the code cleaner or simpler. In most places different function calls are used depending on int/float so all those functions also need an alias, making the code harder to read as it gets obfuscated. |
…pplication post FFT band pass and IIR applied to samples are now separated: I found in testing that applying the sample filter helps with aliasing into base-bands, there is no need to hard-cut the lowest frequencies after FFT.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/audioreactive/audio_reactive.cpp (1)
503-507: Buffer clearing correctly handles different data types.The memset operations now properly account for the actual buffer data type, using
sizeof(float)for float buffers andsizeof(int16_t)for integer buffers. This addresses the buffer size mismatch issue identified in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.cpp(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
usermods/audioreactive/audio_reactive.cpp (2)
<retrieved_learning>
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
</retrieved_learning>
<retrieved_learning>
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (10)
usermods/audioreactive/audio_reactive.cpp (10)
27-27: Good addition for improved FFT accuracy.The
FFT_PREFER_EXACT_PEAKSmacro enables Blackman-Harris windowing for sharper frequency peaks and reduced spectral leakage, which is beneficial for audio analysis.
29-46: Well-documented FFT variant selection strategy.The comprehensive comments clearly explain the trade-offs between different FFT implementations and the automatic selection logic based on ESP32 variants. The default choices are sensible: ArduinoFFT for ESP32/S3 (with FPU) and integer DSP FFT for S2/C3 (without FPU).
148-163: Type-safe buffer declarations with conditional compilation.The buffer type declarations correctly adapt to the selected FFT variant. The use of
int16_t*for integer FFT andfloat*for floating-point variants is appropriate.
214-218: Function overloading provides clean type safety.The overloaded
runMicFilterfunctions for bothfloat*andint16_t*buffers provide type safety while maintaining a consistent interface. This is better than using void pointers or type casting.
273-285: Scaling factor in fftAddAvg requires validation.The integer version uses a scaling factor of 32 to match float values. Verify this scaling is mathematically correct across the entire processing chain.
The comment mentions "scaling value between float and int is 512, float version is scaled down by 16" but uses a factor of 32. Please confirm this scaling relationship is accurate for maintaining equivalent results between FFT variants.
1401-1403: Filter initialization logic is well-designed.The initialization of
useBandPassFilter(false) anduseMicFilter(true by default) provides sensible defaults. TheuseMicFilterhelps reduce noise floor and aliasing, which is beneficial for most microphone inputs.
1437-1437: Appropriate filter disabling for line-in sources.Disabling
useMicFilterfor I2S with Master Clock (typically used for line-in) is correct since line-in sources generally don't need the microphone-specific filtering.
1453-1453: Consistent filter handling for ES8388 source.Disabling
useMicFilterfor ES8388 source maintains consistency with other line-in type sources that don't require microphone-specific filtering.
126-146: I’d like to inspect the actual FFT‐execution block in audio_reactive.cpp to confirm that the dsp calls are correctly wrapped and that there is no unreachable or missing path. Can you run:sed -n '120,180p' usermods/audioreactive/audio_reactive.cppand share those lines?
604-668: runMicFilter integer implementation verifiedThe fixed-point version correctly mirrors the floating-point logic:
- BETA1_FP (55706) and BETA2_FP ((65536 − 55706)/2 = 4915) exactly encode β₁=0.85 and β₂=(1–β₁)/2 in 16.16 format, with the
>>16shift restoring the filtered sample to original units.- ALPHA_FP (737) matches α=0.0225 in 17.15 format, with the
>>15shift correctly converting back to sample units.- Worst-case intermediate products (≤≈2.147×10⁹) and low-pass accumulator (≤≈1.122×10⁹) stay within signed 32-bit range, avoiding overflow.
No changes required.
| // get a fresh batch of samples from I2S | ||
| if (audioSource) audioSource->getSamples(vReal, samplesFFT); | ||
| memset(vImag, 0, samplesFFT * sizeof(float)); // set imaginary parts to 0 | ||
| if (audioSource) audioSource->getSamples(valFFT, samplesFFT); // note: valFFT is used as a int16_t buffer on C3 and S2, could optimize RAM use by only allocating half the size (but makes code harder to read) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify buffer type compatibility in getSamples call.
The comment mentions that valFFT is used as an int16_t buffer on C3/S2 but the function signature suggests void*. Ensure the AudioSource implementation handles this correctly.
🏁 Script executed:
#!/bin/bash
# Check AudioSource getSamples method signature and implementation
ast-grep --pattern 'getSamples($_, $_) {
$$$
}'
# Also check the header file
rg "getSamples" usermods/audioreactive/audio_source.h -A 3 -B 3Length of output: 1265
🏁 Script executed:
#!/bin/bash
# Search for valFFT declaration and allocation in audio_reactive.cpp
rg "valFFT" -n -C 5 usermods/audioreactive/audio_reactive.cppLength of output: 11977
🏁 Script executed:
#!/bin/bash
# Locate I2SSource class definition and its getSamples signature
rg -n "class I2SSource" -C5 usermods/audioreactive/audio_source.h
rg -n "I2SSource::getSamples" -C5 usermods/audioreactive/audio_source.*Length of output: 1083
🏁 Script executed:
#!/bin/bash
# Locate audioSource declaration and initialization in audio_reactive.cpp
rg -n "audioSource" -C5 usermods/audioreactive/audio_reactive.cppLength of output: 8473
🏁 Script executed:
#!/bin/bash
# Print context around the inline getSamples implementations in audio_source.h
sed -n '680,760p' usermods/audioreactive/audio_source.hLength of output: 3711
Ensure getSamples buffer type matches valFFT under integer‐FFT mode
There’s a mismatch when UM_AUDIOREACTIVE_USE_INTEGER_FFT is enabled:
– In audio_reactive.cpp (line 370) you pass an int16_t* (valFFT) into
audioSource->getSamples(valFFT, samplesFFT).
– In usermods/audioreactive/audio_source.h, the analog‐ADC override of
getSamples(void*, uint16_t) always does
float* _buffer = static_cast<float*>(buffer);
(around line 700), so it assumes buffer is a float*.
Action items:
• Verify each AudioSource::getSamples override handles an int16_t* buffer when integer FFT is in use.
• Either:
– Change valFFT to remain a float* (and convert to integers before DSP), or
– Add an #ifdef UM_AUDIOREACTIVE_USE_INTEGER_FFT path in each getSamples override that casts to int16_t* and delivers raw 16-bit samples.
• Confirm no overruns or miscasts occur on C3/S2 targets.
🤖 Prompt for AI Agents
In usermods/audioreactive/audio_reactive.cpp at line 370, the buffer valFFT is
passed as int16_t* to getSamples, but the getSamples overrides in audio_source.h
assume a float* buffer, causing a type mismatch under
UM_AUDIOREACTIVE_USE_INTEGER_FFT. To fix this, review each
AudioSource::getSamples override and add conditional compilation (#ifdef
UM_AUDIOREACTIVE_USE_INTEGER_FFT) to handle int16_t* buffers correctly by
casting and providing raw 16-bit samples. Alternatively, keep valFFT as float*
and convert to int16_t after DSP. Ensure no buffer overruns or invalid casts
occur on C3/S2 targets during this adjustment.
better anti-aliasing at minimal loss of base frequency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
usermods/audioreactive/audio_reactive.cpp (2)
503-507: Duplicate comment: Buffer clearing fixed correctly.The memset calls now correctly account for different buffer types, resolving the previously identified buffer size mismatch issue.
370-370: Duplicate comment: Buffer type mismatch in getSamples call still exists.This issue was previously identified but remains unresolved. When
UM_AUDIOREACTIVE_USE_INTEGER_FFTis enabled,valFFTisint16_t*butgetSamplesimplementations in audio_source.h expectfloat*buffers.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (2)
309-348: Memory allocation complexity increases maintenance burden.The three different memory allocation strategies for each FFT variant significantly increase code complexity. While the aligned allocation for DSP variants is necessary for SIMD performance, the branching logic makes the code harder to maintain.
Consider extracting allocation logic into separate functions:
+static bool allocateArduinoFFTBuffers() { + if (valFFT == nullptr) valFFT = (float*) calloc(sizeof(float), samplesFFT); + if (vImag == nullptr) vImag = (float*) calloc(sizeof(float), samplesFFT); + return (valFFT != nullptr) && (vImag != nullptr); +} +static bool allocateFloatDSPBuffers() { + // Float DSP allocation logic here +} +static bool allocateIntegerDSPBuffers() { + // Integer DSP allocation logic here +}
1405-1457: Filter initialization logic is sensible but could be more explicit.The initialization of
useBandPassFilteranduseMicFilterbased on audio source type is logical, but the conditions could be more clearly documented. Specifically, why ES8388 and I2S with Master Clock sources skip filtering.Add clarifying comments:
-useMicFilter = false; // I2S with Master Clock is mostly used for line-in, skip sample filtering +useMicFilter = false; // I2S with Master Clock is typically used for line-in audio sources that don't need microphone-specific noise filtering-useMicFilter = false; +useMicFilter = false; // ES8388 provides its own built-in filtering, additional software filtering is unnecessary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.cpp(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
usermods/audioreactive/audio_reactive.cpp (2)
<retrieved_learning>
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
</retrieved_learning>
<retrieved_learning>
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Test cdata.js
🔇 Additional comments (4)
usermods/audioreactive/audio_reactive.cpp (4)
27-45: Well-documented FFT variant selection strategy.The comprehensive documentation and conditional compilation approach for selecting FFT variants based on platform capabilities is well-designed. The performance trade-offs between accuracy and speed are clearly explained.
411-496: FFT processing logic correctly handles three variants.The three distinct FFT processing paths are well-implemented with appropriate type handling, DC offset removal, windowing, and magnitude calculation for each variant. The integer FFT path includes proper fixed-point arithmetic.
604-672: Excellent fixed-point arithmetic implementation for integer filtering.The integer version of
runMicFilterproperly implements equivalent filtering logic using fixed-point arithmetic. The scaling factors and bit shifts are well-documented and mathematically correct.
126-163: Type consistency verified—no mismatches foundAll
valFFTusages are correctly guarded by the corresponding preprocessor checks:
- When
UM_AUDIOREACTIVE_USE_INTEGER_FFTis defined,valFFTis anint16_t*and only integer operations (e.g.,abs, integer sums, bit‐shifts) are performed.- When it’s not defined (including the Arduino‐FFT and float DSP paths),
valFFTis afloat*and only floating-point operations (e.g.,fabsf,sqrtf, division by16.0f) are used.No type safety issues remain.
| static float fftAddAvg(int from, int to) { | ||
| #if !defined(UM_AUDIOREACTIVE_USE_INTEGER_FFT) | ||
| float result = 0.0f; | ||
| for (int i = from; i <= to; i++) { | ||
| result += vReal[i]; | ||
| result += valFFT[i]; | ||
| } | ||
| return result / float(to - from + 1); | ||
| #else | ||
| int32_t result = 0; | ||
| for (int i = from; i <= to; i++) { | ||
| result += valFFT[i]; | ||
| } | ||
| result *= 32; // scale result to match float values. note: scaling value between float and int is 512, float version is scaled down by 16 | ||
| #endif | ||
| return float(result) / float(to - from + 1); // return average as float | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Integer FFT scaling logic needs verification.
The scaling factor of 32 for integer results (line 283) appears to be a magic number. The comment mentions "scaling value between float and int is 512, float version is scaled down by 16", suggesting 512/16 = 32, but this needs validation.
🏁 Script executed:
#!/bin/bash
# Search for scaling factor documentation or related constants
rg -n -A3 -B3 "(512|scaling)" usermods/audioreactive/audio_reactive.cppLength of output: 7274
Extract FFT integer‐to‐float scaling into named constants
The integer FFT path today uses a hard-coded 32 to compensate for a 512× scale (512/16 = 32) when averaging. Let’s clarify and centralize this logic:
• In usermods/audioreactive/audio_reactive.cpp (near the top or in an anonymous namespace), add:
constexpr int FFT_INT_SCALE = 512; // int→float magnitude scale
constexpr int FFT_FLOAT_DOWNSCALE = 16; // float path downscaling
constexpr int FFT_INT_AVG_SCALE = FFT_INT_SCALE / FFT_FLOAT_DOWNSCALE; // 512/16• Replace line 283:
- result *= 32; // scale result to match float values. note: scaling value between float and int is 512, float version is scaled down by 16
+ result *= FFT_INT_AVG_SCALE; // scale summed ints to float magnitude (512/16)• Update line 488 similarly:
- FFT_Magnitude_int = valFFT[i] * 512; // scale to match raw float value
+ FFT_Magnitude_int = valFFT[i] * FFT_INT_SCALE; // scale to match float magnitudesThis removes “magic” numbers, makes the 512/16 relationship explicit, and improves maintainability.
🤖 Prompt for AI Agents
In usermods/audioreactive/audio_reactive.cpp around lines 272 to 286, replace
the hard-coded integer scaling factor 32 with named constants for clarity and
maintainability. Define constexpr int constants near the top or in an anonymous
namespace: FFT_INT_SCALE as 512, FFT_FLOAT_DOWNSCALE as 16, and
FFT_INT_AVG_SCALE as their quotient (32). Then replace the literal 32 on line
283 with FFT_INT_AVG_SCALE. Also, apply the same replacement on line 488 to
remove magic numbers and clarify the scaling logic.
- moved scaling of FFT values into fftAddAvg() to use ferwer operations - added "using" for math types, removing some ifdefs and duplications
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/audioreactive/audio_reactive.cpp (1)
365-365: Buffer type compatibility issue persists.The
valFFTbuffer passed togetSamples()can be eitherfloat*orint16_t*depending on the FFT variant, but theAudioSourceimplementations may expect a specific type. This creates a type safety issue that should be resolved by either:
- Adding conditional compilation in
getSamples()implementations to handle both types- Keeping
valFFTasfloat*and converting to integers after sampling
🧹 Nitpick comments (1)
usermods/audioreactive/audio_reactive.cpp (1)
278-278: Extract scaling factors into named constants.The hardcoded scaling factor
32(and512on line 468) should be extracted into named constants to improve maintainability and make the scaling relationships explicit.+// FFT scaling constants for integer/float conversion +constexpr int32_t FFT_INT_SCALE = 512; // int→float magnitude scale +constexpr int32_t FFT_FLOAT_DOWNSCALE = 16; // float path downscaling +constexpr int32_t FFT_INT_AVG_SCALE = FFT_INT_SCALE / FFT_FLOAT_DOWNSCALE; // 32 - result *= 32; // scale result to match float values. note: raw scaling value between float and int is 512, float version is scaled down by 16 + result *= FFT_INT_AVG_SCALE; // scale result to match float valuesApply similar changes to line 468.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cpp(16 hunks)usermods/audioreactive/audio_source.h(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/audioreactive/audio_source.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
usermods/audioreactive/audio_reactive.cpp (4)
<retrieved_learning>
Learnt from: DedeHai
PR: #4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.622Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
</retrieved_learning>
<retrieved_learning>
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
</retrieved_learning>
<retrieved_learning>
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
</retrieved_learning>
<retrieved_learning>
Learnt from: netmindz
PR: #4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (3)
usermods/audioreactive/audio_reactive.cpp (3)
27-46: Excellent documentation and clear macro definition.The comprehensive comments explaining the three FFT variants (ArduinoFFT, ESP-IDF DSP float, ESP-IDF DSP integer) provide valuable context for understanding the conditional compilation strategy. The performance trade-offs and target-specific defaults are well documented.
149-165: Well-designed type abstraction for FFT variants.The typedef approach with
FFTsampleTypeandFFTmathTypecleanly abstracts the different data types used across FFT variants. This is much more maintainable than scattered conditional compilation throughout the codebase.
612-643: Excellent fixed-point filter implementation.The integer version of
runMicFilter()correctly translates the floating-point band-pass filter mathematics to fixed-point arithmetic. The choice of 17.15 and 16.16 fixed-point formats provides appropriate precision while optimizing performance on targets without FPU (ESP32-S2, C3).
|
Personally I would prefer if this change was against the MM version as version of audioreactive in this repo is out of date, so adding this to what in effect is downstream for this usermod will make it even harder to bring in the newer MM changes |
|
Now that the code is running and tested, I could port it to MM quite easily. But isnt MM still using IDF 3? |
Yeah I hoped that having done it once, then doing again hopefully wasn't going to too hard
I'm in the process of dropping any V3 builds, we have been focusing on the V4 builds for a long time
Yeah is roughly 18 months newer
So I asked @softhack007 about just bringing in the MM version to (as was) AC, but Frank felt there were some experimental features that were perhaps a bit much for vanilla WLED I would be keen to see everything on V4 and ESP-DSP and the vanilla WLED version basically based off WLED-MM but with the experimental stuff stripped out, it's something that really needs discussion with @softhack007 and @troyhacks |
I also think that once we simplify and drop a bunch of the legacy things (ESP8266 for example) that the firmware size should improve - or at least that's my hope. Also pushing towards V5 means a bunch of things get MUCH simpler to implement - like all the new RISC-V boards, easier I2S/FFT, maybe easier I2C+I2S audio codec support, etc - but I'm feeling like all this will also add some bloat to the firmware size. So the net outcome may be "about the same." 😁 |
|
Sounds like a good plan but also sounds like its not going to happen soon. I would love to see AR as a default on C3 in 0.16. |
netmindz
added
keep
|
For the moment, if we could focus on MoonModules#248 and then we can take a call at that stage if it's easier to take the MM version and strip/hide anything too experimental for the version here or finalize this "temporary" solution based on the changes made to MM. I really don't want to try and unpick any optimizations that might happen here when we do pull over the MM version |
netmindz
added
the
external
|
Yes, MM version supersedes this PR, I would like to keep this as a draft just to have a working AC version in case cherry picking MM results in trouble. |
|
I took the effort back in 2022 to re-code soundreactive into usermod. It took me less than two weeks (with a bit more polishing afterwards). |
useBandPassFilterapplied to all mic inputs: tested with INMP441 and sine-tones from speaker -> reduces aliasing to base bands and highest frequency bands and comes at minimal processing costOn the C3 the DSP FFT saves 3k of flash but uses 6k of heap, probably for FFT internal buffers.
On the S2 the DSP FFT uses 2.2k extra flash, did not check heap but most likely the same as on C3
Sampling time and FFT time on S2 and C3 are now just barely higher than on ESP32 or S3. Using the DSP FFT on ESP32 or S3 uses the highly optimized versions (roughly 2x as fast as Arduino FFT but at a high extra flash cost and probably also heap).
Some speed test results:
ESP32 Speed test with 512 samples:
Arduino FFT (float): 0.9ms
DSP FFT (float): 0.49ms
DSP FFT (16bit integer): 1.4ms
ESP32 S3 Speed test with 512 samples:
Arduino FFT (float): 0.90ms
DSP FFT (float): 0.51ms (0.68ms if using ANSI C version)
DSP FFT (16bit integer): 1.1ms
ESP32 S2 Speed test with 512 samples:
Arduino FFT (float): 8.5ms
DSP FFT (float): 7.2ms
DSP FFT (16bit integer): 1.3ms
ESP32 C3 Speed test with 512 samples:
Arduino FFT (float): 22.0ms
DSP FFT (float): 19.1ms
DSP FFT (16bit integer): 1.4ms
@softhack007 I moved the scaling of float version (/16.0) up into the FFT code as I did not see any reason it is not done there. Any issue with that?
I tested all versions with a simulated set of sine waves on all platforms, ArduinoFFT and DSP FFT produce identical results, integer version is of course slightly less accurate but produces very comparable results. I also compared ArduinoFFT and integer FFT side-by-side using PS GEQ 1D: while the ArduinoFFT sometimes picks up on low-volume sounds, the integer FFT is much more responsive thanks to faster processing. In general, all variants behave almost the same.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes