Adding DSP FFT and supporting integer math functions #248
Conversation
|
""" WalkthroughThe changes introduce support for multiple FFT backends and data types in the audio reactive module, enabling both floating-point and integer FFT processing with conditional compilation. All relevant buffer types and processing logic are updated accordingly. Additionally, a new bit-wise integer square root function is implemented and exported for general use. Changes
Sequence Diagram(s)sequenceDiagram
participant AudioSource
participant FFTProcessor
participant FFTBackend
AudioSource->>FFTProcessor: getSamples(FFTsampleType* buffer, num_samples)
FFTProcessor->>FFTBackend: processFFT(valFFT, backend-specific)
FFTBackend-->>FFTProcessor: FFT result (float/int)
FFTProcessor->>FFTProcessor: Post-process (magnitude, scaling, smoothing)
Poem
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 (
|
|
Test on C3: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
usermods/audioreactive/audio_source.h (1)
390-442: Verify conditional compilation logic and sample scaling.The implementation correctly handles different FFT sample types with conditional compilation. However, there are some considerations:
Sample scaling differences: Integer path skips sample scaling for performance on single-core MCUs, while float path applies
_sampleScale. Ensure this difference is intentional and documented.Complex conditional logic: The nested
#ifdefblocks could become difficult to maintain. Consider extracting the sample processing logic into separate methods.Consider refactoring the sample processing logic:
+ void processSampleFloat(float* buffer, I2S_datatype* samples, uint16_t num_samples) { + for (int i = 0; i < num_samples; i++) { + samples[i] = postProcessSample(samples[i]); +#ifdef I2S_SAMPLE_DOWNSCALE_TO_16BIT + float currSample = (float) samples[i] / 65536.0f; +#else + float currSample = (float) samples[i]; +#endif + buffer[i] = currSample * _sampleScale; + } + } + + void processSampleInteger(int16_t* buffer, I2S_datatype* samples, uint16_t num_samples) { + for (int i = 0; i < num_samples; i++) { + samples[i] = postProcessSample(samples[i]); +#ifdef I2S_SAMPLE_DOWNSCALE_TO_16BIT + int16_t currSample = samples[i] >> 16; +#else + int16_t currSample = samples[i]; +#endif + buffer[i] = currSample; + } + }usermods/audioreactive/audio_reactive.h (1)
659-662: Optimize window scaling by baking in correction factor.The TODO comment correctly identifies an optimization opportunity. Pre-scaling the window with the correction factor would save FFT_BIN_SCALE float multiplications per FFT cycle.
// convert float window to 16-bit int for (int i = 0; i < samplesFFT; i++) { - windowFFT[i] = (int16_t)(windowFloat[i] * 32767.0f); // TODO: bake-in the window correction factor and save a few float multiplications + // Bake in window correction factor and FFT_BIN_SCALE to reduce runtime calculations + windowFFT[i] = (int16_t)(windowFloat[i] * wc * 32767.0f / FFT_BIN_SCALE); }Note: This would require adjusting the FFT magnitude calculation accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/audioreactive/audio_reactive.h(19 hunks)usermods/audioreactive/audio_source.h(4 hunks)wled00/fcn_declare.h(1 hunks)wled00/wled_math.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
usermods/audioreactive/audio_reactive.h (1)
wled00/wled_math.cpp (2)
sqrt32_bw(114-135)sqrt32_bw(114-114)
🔇 Additional comments (8)
wled00/fcn_declare.h (1)
473-473: LGTM: Clean function declaration.The function declaration is properly placed in the math functions section and follows the existing naming conventions.
wled00/wled_math.cpp (1)
113-135: Approve sqrt32_bw integration—no issues foundThe bit-wise integer square root is correctly implemented and integrated. Verification confirms two call sites in usermods/audioreactive/audio_reactive.h and its declaration in wled00/fcn_declare.h. No overflow or misuse detected given the input ranges.
• Call sites verified:
– usermods/audioreactive/audio_reactive.h:
• return sqrt32_bw(result * (FFT_BIN_SCALE*FFT_BIN_SCALE) / (to - from + 1));
• valFFT[i] = sqrt32_bw(real_part * real_part + imag_part * imag_part);
• Declaration in wled00/fcn_declare.h matches implementation in wled00/wled_math.cpp.
• Edge-case behavior (x=0→0, x=1→1) is inherent in the algorithm.Optional enhancement: document or add a quick early-return for x < 2 to make the edge-case intent explicit.
usermods/audioreactive/audio_source.h (2)
1107-1107: Consistent type usage in I2SAdcSource.The update to use
FFTsampleType*maintains consistency with the base class interface changes.
161-161: FFTsampleType abstraction confirmed
I’ve verified thatFFTsampleTypeis defined inusermods/audioreactive/audio_reactive.has:
using FFTsampleType = float;whenUM_AUDIOREACTIVE_USE_INTEGER_FFTis not definedusing FFTsampleType = int16_t;whenUM_AUDIOREACTIVE_USE_INTEGER_FFTis definedThe change from
float*toFFTsampleType*ingetSamplesis correctly supported by these definitions. No further action required—the abstraction is sound and ready for use.usermods/audioreactive/audio_reactive.h (4)
1008-1032: Good implementation of window correction factor.The window correction factor is properly applied to all FFT bins during the mapping phase, which addresses the PR author's suggestion about pre-computing scaling factors.
443-469: Well-designed FFT bin averaging with proper type handling.The implementation correctly:
- Uses
FFTmathTypefor intermediate calculations to prevent overflow- Applies
FFT_BIN_SCALEappropriately for both linear and RMS averaging- Includes optimization for single-bin case
- Handles integer square root for integer FFT mode
1284-1284: Correct scaling adaptation for peak detection.Good use of
FFT_BIN_SCALE_RAWto ensure the peak detection threshold works correctly with both float and integer FFT implementations.
190-217: Successfully implements DSP FFT and integer math support.The implementation successfully addresses the PR objectives:
- ✅ ESP DSP FFT option is available for all ESP32 variants
- ✅ Integer math is properly implemented for S2/C3 by default
- ✅ Bit-wise square root (
sqrt32_bw) is integrated for integer magnitude calculations- ✅ Type abstraction (
FFTsampleType/FFTmathType) enables clean dual-mode supportThe conditional compilation approach provides good flexibility while maintaining compatibility.
Also applies to: 400-410
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
usermods/audioreactive/audio_reactive.h (3)
482-487: Memory leak issue remains unaddressed.The previous review comment about tracking raw pointers for proper cleanup is still valid. While the buffers are typically allocated once for the application lifetime, proper cleanup should still be implemented for completeness.
This is a duplicate of a previous concern that hasn't been resolved.
540-541: AC gain overflow concern remains unaddressed.The previous review comment about AC gain up to 2 at Nyquist frequency potentially causing overflow in integer math is still valid. While clamping prevents crashes, it may introduce distortion. Consider whether this gain characteristic is intentional for this filter.
This is a duplicate of a previous concern that hasn't been resolved.
827-827: Comment correction still needed.The previous review comment about the incorrect scaling factor comment is still valid. The code
(newZeroCrossingCount*341)>>9multiplies by approximately 2/3, not 1/3 as stated in the comment.This is a duplicate of a previous concern that hasn't been resolved.
🧹 Nitpick comments (1)
usermods/audioreactive/audio_reactive.h (1)
41-41: Suggest documenting the new FFT architecture.The extensive changes to support multiple FFT backends and data types represent a significant architectural improvement. Consider adding documentation (either in comments or separate docs) explaining:
- When each FFT backend is used
- Performance characteristics of float vs integer implementations
- How the scaling factors maintain consistency between implementations
This would help future maintainers understand the design decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/audioreactive/audio_reactive.h(19 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: MoonModules/WLED-MM#248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.189Z
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.
Learnt from: DedeHai
PR: MoonModules/WLED-MM#248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.954Z
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.
usermods/audioreactive/audio_reactive.h (3)
undefined
<retrieved_learning>
Learnt from: DedeHai
PR: #248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.189Z
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: DedeHai
PR: #248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.954Z
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: DedeHai
PR: #237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
</retrieved_learning>
🧬 Code Graph Analysis (1)
usermods/audioreactive/audio_reactive.h (1)
wled00/wled_math.cpp (2)
sqrt32_bw(114-135)sqrt32_bw(114-114)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_XL)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_M_debug)
- GitHub Check: Builds (esp32_16MB_M)
- GitHub Check: Builds (esp32_4MB_S)
- GitHub Check: Builds (esp32_4MB_M)
🔇 Additional comments (11)
usermods/audioreactive/audio_reactive.h (11)
190-218: LGTM: Well-structured FFT backend selection.The conditional compilation logic cleanly handles the different FFT implementations and correctly defaults to the most appropriate backend for each ESP32 variant. The type definitions provide good abstraction between float and integer FFT implementations.
400-409: LGTM: Appropriate scaling factors for different FFT implementations.The scaling constants are well-chosen to maintain consistent behavior between float and integer FFT implementations. The comment explaining the relationship between the scaling factors is helpful.
412-418: LGTM: Buffer declarations properly updated for type abstraction.The buffer type changes correctly use the new
FFTsampleTypeabstraction and handle the different windowing requirements between FFT backends appropriately.
444-469: LGTM: Well-implemented FFT averaging with proper scaling.The updates to move scaling into the averaging functions improve code organization. The use of
FFTmathTypemaintains type safety, and the integer square root implementation is appropriate. The single-sample optimization is a nice performance touch.
706-706: LGTM: Type mismatch in memory operations properly fixed.The updates to use
sizeof(FFTsampleType)instead ofsizeof(float)correctly address the previous review comment about type mismatches. This ensures the correct amount of data is copied regardless of whether the FFT implementation uses float or int16_t.Also applies to: 710-710, 798-798
1113-1177: LGTM: Excellent dual implementation with justified frequency differences.The filter implementation correctly provides both float and integer math versions. Based on the retrieved learnings, the higher 90Hz cutoff for integer math is intentional to preserve precision and avoid compounding errors in integer arithmetic. The fixed-point implementation is well-executed with proper scaling and bit operations.
629-663: LGTM: Well-implemented window function conversion for integer FFT.The approach of generating the window function in float for accuracy then converting to int16_t is sound. The proper scaling to the 16-bit range and cleanup of the temporary buffer demonstrate good memory management. The TODO comment about baking in the window correction factor is a reasonable future optimization.
944-972: LGTM: Comprehensive and well-implemented integer FFT pipeline.The integer FFT implementation is thorough and correctly handles all stages: windowing with fixed-point arithmetic, FFT computation using ESP-IDF DSP functions, magnitude calculation with integer square root, and peak detection. The scaling and type conversions are handled properly throughout.
1284-1284: LGTM: Proper scaling factor update for peak detection.The update to use
FFT_BIN_SCALE_RAWmaintains consistency with the new scaling approach and ensures peak detection works correctly with both float and integer FFT implementations.
307-307: LGTM: Function signature properly updated for type consistency.The update to use
FFTsampleType*in the function signature maintains type safety and allows the function to work correctly with both float and integer FFT implementations.
199-200: LGTM: sqrt macro redefinition is properly scoped and necessary.Based on the retrieved learnings, this sqrt macro redefinition is acceptable and necessary for ArduinoFFT performance optimization. It's properly scoped within the conditional compilation block and provides significant performance benefits (10-50% improvement on ESP32).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
platformio.ini (3)
2435-2437: Redundant duplication (see earlier comment)
Same two flags as above; once the refactor suggested for lines 1580-1582 is applied these lines can simply rely on the shared variable.
2461-2462: Redundant duplication (see earlier comment)
2489-2490: Redundant duplication (see earlier comment)
🧹 Nitpick comments (1)
platformio.ini (1)
1580-1582: Factor out repeatedWLED_DISABLE_PARTICLESYSTEM*flags into a shared blockThe same two
-D WLED_DISABLE_PARTICLESYSTEM{1D,2D}options are now copy-pasted into half-a-dozen environments.
Consider adding them once to a small helper group (e.g.${common_mm.build_disable_particle}) and referencing that variable from each env instead. This keeps the file shorter and avoids accidental drift when toggling the flags later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio.ini(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: MoonModules/WLED-MM#248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.189Z
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.
Learnt from: DedeHai
PR: MoonModules/WLED-MM#248
File: usermods/audioreactive/audio_reactive.h:1121-1121
Timestamp: 2025-06-30T17:58:27.954Z
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.
platformio.ini (2)
undefined
<retrieved_learning>
Learnt from: DedeHai
PR: #248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.189Z
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: DedeHai
PR: #237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
🔇 Additional comments (1)
platformio.ini (1)
2225-2227: Looks good – flags match the pattern used elsewhere
Nothing to add.
|
Sorry @DedeHai , I was trying to update your branch with my fixes on mdev to we could see the CI builds BUT I've messed up git a bit, just ignore my platformio.ini changes |
Ported from AC PR: wled#4750
Questions/Comments for @softhack007
runDCBlocker()has an AC gain > 1 which can be a problem in integer math samples. I found that usingrunMicFilter()with a low-cutoff of 100Hz followed by a simple DC removal (by subtracting the mean value from all samples) gives quite good results on an INMP441. In what circumstances is the runDCBlocker() function useful? Can it be dropped for simple setups that use integer samples i.e. on C3/S2?runMicFilter()cutoff frequency comments for high frequencies are wrong for a sample rate of 22kHz (none of the cutoffs can be any higher than 11kHz as that is the Nyquist-frequency)fftAddAvg()function as for integer samples it is faster and more accurate, so I did the same for the float version.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores