Add analog in support for AR on C3, S2, S3 #4761
Conversation
still needs some cleanup and testing but basically works.
WalkthroughThe changes unify and extend analog microphone input support across ESP32, ESP32S2, ESP32C3, and ESP32S3 targets. A new Changes
✨ 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: 1
🧹 Nitpick comments (4)
usermods/audioreactive/audio_source.h (3)
549-553: Consider removing commented-out code instead of keeping it.Since analog sound input support is now being added for these MCU variants, these warnings are obsolete. Consider removing the commented lines entirely for cleaner code.
-//#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 2, 0) -//#if !defined(SOC_I2S_SUPPORTS_ADC) && !defined(SOC_I2S_SUPPORTS_ADC_DAC) -// #warning this MCU does not support analog sound input -//#endif -//#endif
757-757: Address TODO: Use dynamic timeout calculation.The hardcoded
ADC_TIMEOUTof 30ms should be replaced with a dynamic calculation based onFFT_MIN_CYCLEas suggested in the TODO comment. This would ensure the timeout adapts to the configured FFT cycle time.Consider moving the
FFT_MIN_CYCLEdefinition before this file's inclusion in the cpp file, or pass the timeout as a parameter to the class.
855-859: Optimize DC offset removal for better performance.The DC offset removal is currently performed using floating-point arithmetic. As noted in the TODO comment, this could be optimized using integer arithmetic on ESP32-C3 and ESP32-S2 for better performance.
Would you like me to implement the integer-based DC offset removal optimization for C3 and S2 targets?
usermods/audioreactive/audio_reactive.cpp (1)
395-397: Clarify the rationale for excluding ADC sources from the delay.The comment mentions that the "delay trick" doesn't help for analog ADC, but it would be helpful to document why this is the case. Is this due to different timing characteristics of DMA-based ADC sampling versus I2S sampling?
Consider expanding the comment to explain the timing differences between I2S and ADC DMA sampling methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cpp(5 hunks)usermods/audioreactive/audio_source.h(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
usermods/audioreactive/audio_reactive.cpp (2)
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
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.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
🧬 Code Graph Analysis (1)
usermods/audioreactive/audio_reactive.cpp (1)
usermods/audioreactive/audio_source.h (6)
Type_Adc(587-587)Type_Adc(800-800)audioPin(589-645)audioPin(589-589)audioPin(802-824)audioPin(802-802)
⏰ 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). (19)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- 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 (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- 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 (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (6)
usermods/audioreactive/audio_source.h (2)
143-143: LGTM! Consistent naming for ADC sources.The rename from
Type_I2SAdctoType_Adcunifies the naming convention for all ADC audio sources, making the code more maintainable.
826-865: Robust error handling in sample acquisition.The implementation correctly handles:
- Partial frame returns from DMA
- Buffer overflow detection and recovery
- Invalid state recovery with ADC reset
Good defensive programming practice!
usermods/audioreactive/audio_reactive.cpp (4)
1176-1236: Well-structured platform-specific audio source selection.The code correctly:
- Falls through PDM case for S2/C3 targets (which don't support PDM)
- Uses
I2SAdcSourcefor classic ESP32- Uses
DMAadcSourcefor S2/C3/S3 targets- Applies band-pass filtering consistently for analog inputs
1614-1614: Correct usage of unified ADC type.The change to use
Type_Adcis consistent with the enum rename and properly identifies all ADC audio sources.
1849-1851: Good validation of platform capabilities.The code properly prevents selection of PDM mode on ESP32-S2 and ESP32-C3, which don't support PDM microphones. This prevents runtime errors from invalid configuration.
1886-1886: UI correctly reflects analog support for all ESP32 variants.Now that analog input is supported on all ESP32 variants, the UI appropriately shows the "Generic Analog" option universally.
| #if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3) | ||
| // ADC over I2S is only possible on "classic" ESP32 | ||
| DEBUGSR_PRINTLN(F("AR: Analog Microphone (left channel only).")); | ||
| audioSource = new I2SAdcSource(SAMPLE_RATE, BLOCK_SIZE); | ||
| delay(100); | ||
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | ||
| if (audioSource) audioSource->initialize(audioPin); | ||
| break; | ||
| #else | ||
| // use ADC DMA on ESP32S2, ESP32C3, ESP32S3 | ||
| DEBUGSR_PRINTLN(F("AR: Analog Microphone")); | ||
| audioSource = new DMAadcSource(SAMPLE_RATE, samplesFFT); | ||
| delay(10); // might help with proper initialization | ||
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | ||
| if (audioSource) audioSource->initialize(audioPin); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the analog button incompatibility.
According to the PR objectives, the DMA sampling approach is incompatible with analog buttons. This important limitation should be documented in the code comments to prevent user confusion.
case 0:
default:
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
// ADC over I2S is only possible on "classic" ESP32
DEBUGSR_PRINTLN(F("AR: Analog Microphone (left channel only)."));
audioSource = new I2SAdcSource(SAMPLE_RATE, BLOCK_SIZE);
delay(100);
useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog
if (audioSource) audioSource->initialize(audioPin);
break;
#else
// use ADC DMA on ESP32S2, ESP32C3, ESP32S3
+ // WARNING: DMA ADC sampling is incompatible with analog buttons due to continuous ADC usage
DEBUGSR_PRINTLN(F("AR: Analog Microphone"));
audioSource = new DMAadcSource(SAMPLE_RATE, samplesFFT);
delay(10); // might help with proper initialization
useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog
if (audioSource) audioSource->initialize(audioPin);
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3) | |
| // ADC over I2S is only possible on "classic" ESP32 | |
| DEBUGSR_PRINTLN(F("AR: Analog Microphone (left channel only).")); | |
| audioSource = new I2SAdcSource(SAMPLE_RATE, BLOCK_SIZE); | |
| delay(100); | |
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | |
| if (audioSource) audioSource->initialize(audioPin); | |
| break; | |
| #else | |
| // use ADC DMA on ESP32S2, ESP32C3, ESP32S3 | |
| DEBUGSR_PRINTLN(F("AR: Analog Microphone")); | |
| audioSource = new DMAadcSource(SAMPLE_RATE, samplesFFT); | |
| delay(10); // might help with proper initialization | |
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | |
| if (audioSource) audioSource->initialize(audioPin); | |
| break; | |
| #if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3) | |
| // ADC over I2S is only possible on "classic" ESP32 | |
| DEBUGSR_PRINTLN(F("AR: Analog Microphone (left channel only).")); | |
| audioSource = new I2SAdcSource(SAMPLE_RATE, BLOCK_SIZE); | |
| delay(100); | |
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | |
| if (audioSource) audioSource->initialize(audioPin); | |
| break; | |
| #else | |
| // use ADC DMA on ESP32S2, ESP32C3, ESP32S3 | |
| // WARNING: DMA ADC sampling is incompatible with analog buttons due to continuous ADC usage | |
| DEBUGSR_PRINTLN(F("AR: Analog Microphone")); | |
| audioSource = new DMAadcSource(SAMPLE_RATE, samplesFFT); | |
| delay(10); // might help with proper initialization | |
| useBandPassFilter = true; // PDM bandpass filter seems to help for bad quality analog | |
| if (audioSource) audioSource->initialize(audioPin); | |
| break; |
🤖 Prompt for AI Agents
In usermods/audioreactive/audio_reactive.cpp around lines 1221 to 1236, add a
comment explaining that the DMAadcSource sampling method used for ESP32S2,
ESP32C3, and ESP32S3 targets is incompatible with analog buttons. This
documentation should be placed near the conditional code block where
DMAadcSource is instantiated to clearly inform users of this limitation and
prevent confusion.
|
Not a criticism, just curious as to why you have started with a pr against here Vs MM, was it just that you had already written this code and so sharing here for the benefit of others who might want to pick up before the updated AR mod gets applied here? |
|
I have problems working with MM: I can't compile unless I base it directly off of MM, but then I can not do any commits. If I make a branch in my fork of AC, it will give me lots of compile errors when I set base to MM. So I did the development in AC and thought I might as well do a draft PR to see the diff and what the rabbit has to say. MM PR is next. |
|
@netmindz it might be in best interest of WLED to expedite migrating MM's version of AR into upstream. If that is desired at all and if there is someone that will do the migration. Otherwise the amount of differences will only grow with time making it more difficult to migrate with each commit. |
|
now that I have a better grasp on AR code and @softhack007 is not available, I can take a stab at it. How would one go about this? cherry pick all the commits or is there a better way of grabbing the current state of only the AR files and merging them in? I do however not approve of all features in MM AR, some are just too technical for the non-audio-people, like having 5 different FFT windows selectable at runtime for example. But that is easy to wrap in some |
|
I totally agree with you! KIS - keep it simple |
|
I would be happy to do a sweeping cleanup of that code, but I do not want to step on anyones toes. |
It's basically a 3 step process
That's why I don't want any extra "downstream" features added to the version in this repo or otherwise we are adding more steps to the process |
Let's try and jump on a call at some point and we can look at these issues together. I'm always swapping between the two |
fully agree that there should only be one "upstream" version that gets features added. |
|
I agree in principle that we should use git as much as possible, however I fear that the commits might not be clean enough, so while in theory we can cherry-pick the commits, I think we might face some significant challenges. This would of course be much simpler if we had out of tree usermods, should we perhaps split it out entirely making it a library rather than being in the source itself ? |
Theoretically, the current build scripts should already support this, though I haven't tested it fully: any |
|
Following up: I just tested it with a quick rip of the example usermod, seems like it works OK: ..builds for me with two usermods reported by the validation script. (I'm out of town this weekend and can't test an upload, though.) Hope this helps! |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
5 participants
DRAFT, only for reference, this needs to go upstream to MM first.
uses DMA sampling in the background, not compatible with analog buttons.
Summary by CodeRabbit
New Features
Improvements