Add analog in support for AR on C3, S2, S3#4761
Conversation
still needs some cleanup and testing but basically works.
WalkthroughAdjusted analog-microphone handling and ADC source naming across ESP32 variants; added a new DMA-based ADC audio source implementation and updated conditional compilation, runtime selection, configuration serialization, and UI labeling to use the unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 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
netmindz
removed
the
stale
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_source.h`:
- Around line 804-824: DMAadcSource::initialize() currently leaks the allocated
GPIO when later initialization fails; after successfully calling
PinManager::allocatePin(audioPin, ...), ensure you release that allocation on
all subsequent early-return paths (e.g., the incompatible channel branch and the
init_adc_continuous() != ESP_OK branch). Fix by calling the corresponding
PinManager release API (e.g., PinManager::freePin(_audioPin) or
PinManager::freePin(_audioPin, PinOwner::UM_Audioreactive) depending on the
project's API) before each of those returns, and optionally reset _audioPin
(e.g., to -1) after freeing to avoid stale state.
- Around line 828-867: getSamples() can reach the DC-removal and the final "if
(err == ESP_ERR_INVALID_STATE)" check when _initialized is false, leaving err
uninitialized; fix by handling the uninitialized case immediately: in
getSamples(), if (!_initialized) return (or explicitly set err = ESP_OK and zero
the buffer) before any DC-removal or checking err; update references in this
function (variables _initialized, err, reset_DMA_ADC, and the DC-removal loops)
so no code path reads err or processes buffer when initialization failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e35bd485-6c84-4ac7-866d-c4d91d309703
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.h
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | ||
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | ||
| _myADCchannel = 0x0F; | ||
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | ||
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | ||
| return; | ||
| } | ||
| _audioPin = audioPin; | ||
| // Determine Analog channel. Only Channels on ADC1 are supported | ||
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | ||
| if (channel > MAX_ADC1_CHANNEL) { | ||
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | ||
| return; | ||
| } else { | ||
| _myADCchannel = channel; | ||
| } | ||
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | ||
| adcpattern.channel = channel; // update pattern config | ||
| if (init_adc_continuous() != ESP_OK) | ||
| return; | ||
| adc_digi_start(); //start sampling |
There was a problem hiding this comment.
Pin allocation leaks on DMAadcSource::initialize() failure paths.
After PinManager::allocatePin() succeeds, failures at Line 814/822 currently return without releasing the pin. That can block reconfiguration/retry in the same runtime session.
♻️ Suggested fix
void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) {
DEBUGSR_PRINTLN(F("DMAadcSource::initialize()"));
_myADCchannel = 0x0F;
if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) {
DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin);
return;
}
_audioPin = audioPin;
// Determine Analog channel. Only Channels on ADC1 are supported
int8_t channel = digitalPinToAnalogChannel(_audioPin);
if (channel > MAX_ADC1_CHANNEL) {
DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin);
+ PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive);
+ _audioPin = I2S_PIN_NO_CHANGE;
return;
} else {
_myADCchannel = channel;
}
adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config
adcpattern.channel = channel; // update pattern config
- if (init_adc_continuous() != ESP_OK)
+ if (init_adc_continuous() != ESP_OK) {
+ PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive);
+ _audioPin = I2S_PIN_NO_CHANGE;
return;
- adc_digi_start(); //start sampling
+ }
+ esp_err_t err = adc_digi_start(); //start sampling
+ if (err != ESP_OK) {
+ DEBUGSR_PRINTF("Failed to start ADC DMA: %d\n", err);
+ adc_digi_deinitialize();
+ PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive);
+ _audioPin = I2S_PIN_NO_CHANGE;
+ return;
+ }
_initialized = true;
}📝 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.
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | |
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | |
| _myADCchannel = 0x0F; | |
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | |
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | |
| return; | |
| } | |
| _audioPin = audioPin; | |
| // Determine Analog channel. Only Channels on ADC1 are supported | |
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | |
| if (channel > MAX_ADC1_CHANNEL) { | |
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | |
| return; | |
| } else { | |
| _myADCchannel = channel; | |
| } | |
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | |
| adcpattern.channel = channel; // update pattern config | |
| if (init_adc_continuous() != ESP_OK) | |
| return; | |
| adc_digi_start(); //start sampling | |
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | |
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | |
| _myADCchannel = 0x0F; | |
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | |
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | |
| return; | |
| } | |
| _audioPin = audioPin; | |
| // Determine Analog channel. Only Channels on ADC1 are supported | |
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | |
| if (channel > MAX_ADC1_CHANNEL) { | |
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | |
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | |
| _audioPin = I2S_PIN_NO_CHANGE; | |
| return; | |
| } else { | |
| _myADCchannel = channel; | |
| } | |
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | |
| adcpattern.channel = channel; // update pattern config | |
| if (init_adc_continuous() != ESP_OK) { | |
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | |
| _audioPin = I2S_PIN_NO_CHANGE; | |
| return; | |
| } | |
| esp_err_t err = adc_digi_start(); //start sampling | |
| if (err != ESP_OK) { | |
| DEBUGSR_PRINTF("Failed to start ADC DMA: %d\n", err); | |
| adc_digi_deinitialize(); | |
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | |
| _audioPin = I2S_PIN_NO_CHANGE; | |
| return; | |
| } | |
| _initialized = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_source.h` around lines 804 - 824,
DMAadcSource::initialize() currently leaks the allocated GPIO when later
initialization fails; after successfully calling
PinManager::allocatePin(audioPin, ...), ensure you release that allocation on
all subsequent early-return paths (e.g., the incompatible channel branch and the
init_adc_continuous() != ESP_OK branch). Fix by calling the corresponding
PinManager release API (e.g., PinManager::freePin(_audioPin) or
PinManager::freePin(_audioPin, PinOwner::UM_Audioreactive) depending on the
project's API) before each of those returns, and optionally reset _audioPin
(e.g., to -1) after freeing to avoid stale state.
| void getSamples(float *buffer, uint16_t num_samples) { | ||
| int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes | ||
| uint8_t result[framesize]; // create a read buffer | ||
| uint32_t ret_num; | ||
| uint32_t totalbytes = 0; | ||
| uint32_t j = 0; | ||
| esp_err_t err; | ||
| if (_initialized) { | ||
| do { | ||
| err = adc_digi_read_bytes(result, framesize, &ret_num, ADC_TIMEOUT); // read samples | ||
| if ((err == ESP_OK || err == ESP_ERR_INVALID_STATE) && ret_num > 0) { // in invalid sate (internal buffer overrun), still read the last valid sample, then reset the ADC DMA afterwards (better than not having samples at all) | ||
| totalbytes += ret_num; // after an error, DMA buffer can be misaligned, returning partial frames. Found no solution to re-align or flush the buffers, seems to be yet another IDF4 bug | ||
|
|
||
| if (totalbytes > framesize) { // got too many bytes to fit sample buffer | ||
| ret_num -= totalbytes - framesize; // discard extra samples | ||
| } | ||
| for (int i = 0; i < ret_num; i += ADC_RESULT_BYTE) { | ||
| adc_digi_output_data_t *p = reinterpret_cast<adc_digi_output_data_t*>(&result[i]); | ||
| buffer[j++] = float((int(p->val & 0x0FFF))); // get the 12bit sample data and convert to float note: works on both format types | ||
| // TODO: for integer math: when scaling up to 16bit: compared to I2S mic the scaling seems about the same when not shifting at all, so need to divide by 16 after FFT if scaling up to 16bit | ||
| } | ||
| } else { // no samples or other error: usually ESP_ERR_TIMEOUT (if DMA has stopped for some reason) | ||
| reset_DMA_ADC(); | ||
| DEBUGSR_PRINTF("ADC ERROR!\n"); | ||
| return; // something went very wrong, just exit | ||
| } | ||
| } while (totalbytes < framesize); // read more samples if a partial frame was returned (data is still consistent in split frames) | ||
| } | ||
|
|
||
| // remove DC TODO: should really do this in int on C3 & S2... -> needs an update after PR #248 is merged | ||
| int32_t sum = 0; | ||
| for (int i = 0; i < num_samples; i++) sum += buffer[i]; | ||
| int32_t mean = sum / num_samples; | ||
| for (int i = 0; i < num_samples; i++) buffer[i] -= mean; //uses static mean, as it should not change too much over time, deducted above | ||
|
|
||
| if (err == ESP_ERR_INVALID_STATE) { // error reading data, error means buffer overrun, need to fully reset the DMA ADC to make it work again | ||
| DEBUGSR_PRINTF("ADC BFR OVERFLOW, RESETTING ADC\n"); | ||
| reset_DMA_ADC(); | ||
| } | ||
| } |
There was a problem hiding this comment.
getSamples() can run with uninitialized state and use undefined err.
If _initialized is false, the code still executes DC-removal loops and checks err at Line 863 even though err was never assigned. This is undefined behavior and can corrupt processing flow.
🛠️ Suggested fix
void getSamples(float *buffer, uint16_t num_samples) {
- int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes
+ if (!_initialized || buffer == nullptr || num_samples == 0) return;
+
+ int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes
uint8_t result[framesize]; // create a read buffer
uint32_t ret_num;
uint32_t totalbytes = 0;
uint32_t j = 0;
- esp_err_t err;
- if (_initialized) {
- do {
- err = adc_digi_read_bytes(result, framesize, &ret_num, ADC_TIMEOUT); // read samples
- if ((err == ESP_OK || err == ESP_ERR_INVALID_STATE) && ret_num > 0) { // in invalid sate (internal buffer overrun), still read the last valid sample, then reset the ADC DMA afterwards (better than not having samples at all)
- totalbytes += ret_num; // after an error, DMA buffer can be misaligned, returning partial frames. Found no solution to re-align or flush the buffers, seems to be yet another IDF4 bug
+ esp_err_t err = ESP_OK;
+ do {
+ err = adc_digi_read_bytes(result, framesize, &ret_num, ADC_TIMEOUT); // read samples
+ if ((err == ESP_OK || err == ESP_ERR_INVALID_STATE) && ret_num > 0) { // in invalid sate (internal buffer overrun), still read the last valid sample, then reset the ADC DMA afterwards (better than not having samples at all)
+ totalbytes += ret_num; // after an error, DMA buffer can be misaligned, returning partial frames. Found no solution to re-align or flush the buffers, seems to be yet another IDF4 bug
- if (totalbytes > framesize) { // got too many bytes to fit sample buffer
- ret_num -= totalbytes - framesize; // discard extra samples
- }
- for (int i = 0; i < ret_num; i += ADC_RESULT_BYTE) {
- adc_digi_output_data_t *p = reinterpret_cast<adc_digi_output_data_t*>(&result[i]);
- buffer[j++] = float((int(p->val & 0x0FFF))); // get the 12bit sample data and convert to float note: works on both format types
- // TODO: for integer math: when scaling up to 16bit: compared to I2S mic the scaling seems about the same when not shifting at all, so need to divide by 16 after FFT if scaling up to 16bit
- }
- } else { // no samples or other error: usually ESP_ERR_TIMEOUT (if DMA has stopped for some reason)
- reset_DMA_ADC();
- DEBUGSR_PRINTF("ADC ERROR!\n");
+ if (totalbytes > framesize) { // got too many bytes to fit sample buffer
+ ret_num -= totalbytes - framesize; // discard extra samples
+ }
+ for (int i = 0; i < ret_num; i += ADC_RESULT_BYTE) {
+ adc_digi_output_data_t *p = reinterpret_cast<adc_digi_output_data_t*>(&result[i]);
+ buffer[j++] = float((int(p->val & 0x0FFF))); // get the 12bit sample data and convert to float note: works on both format types
+ // TODO: for integer math: when scaling up to 16bit: compared to I2S mic the scaling seems about the same when not shifting at all, so need to divide by 16 after FFT if scaling up to 16bit
+ }
+ } else { // no samples or other error: usually ESP_ERR_TIMEOUT (if DMA has stopped for some reason)
+ reset_DMA_ADC();
+ DEBUGSR_PRINTF("ADC ERROR!\n");
- return; // something went very wrong, just exit
- }
- } while (totalbytes < framesize); // read more samples if a partial frame was returned (data is still consistent in split frames)
- }
+ return; // something went very wrong, just exit
+ }
+ } while (totalbytes < framesize); // read more samples if a partial frame was returned (data is still consistent in split frames)📝 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.
| void getSamples(float *buffer, uint16_t num_samples) { | |
| int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes | |
| uint8_t result[framesize]; // create a read buffer | |
| uint32_t ret_num; | |
| uint32_t totalbytes = 0; | |
| uint32_t j = 0; | |
| esp_err_t err; | |
| if (_initialized) { | |
| do { | |
| err = adc_digi_read_bytes(result, framesize, &ret_num, ADC_TIMEOUT); // read samples | |
| if ((err == ESP_OK || err == ESP_ERR_INVALID_STATE) && ret_num > 0) { // in invalid sate (internal buffer overrun), still read the last valid sample, then reset the ADC DMA afterwards (better than not having samples at all) | |
| totalbytes += ret_num; // after an error, DMA buffer can be misaligned, returning partial frames. Found no solution to re-align or flush the buffers, seems to be yet another IDF4 bug | |
| if (totalbytes > framesize) { // got too many bytes to fit sample buffer | |
| ret_num -= totalbytes - framesize; // discard extra samples | |
| } | |
| for (int i = 0; i < ret_num; i += ADC_RESULT_BYTE) { | |
| adc_digi_output_data_t *p = reinterpret_cast<adc_digi_output_data_t*>(&result[i]); | |
| buffer[j++] = float((int(p->val & 0x0FFF))); // get the 12bit sample data and convert to float note: works on both format types | |
| // TODO: for integer math: when scaling up to 16bit: compared to I2S mic the scaling seems about the same when not shifting at all, so need to divide by 16 after FFT if scaling up to 16bit | |
| } | |
| } else { // no samples or other error: usually ESP_ERR_TIMEOUT (if DMA has stopped for some reason) | |
| reset_DMA_ADC(); | |
| DEBUGSR_PRINTF("ADC ERROR!\n"); | |
| return; // something went very wrong, just exit | |
| } | |
| } while (totalbytes < framesize); // read more samples if a partial frame was returned (data is still consistent in split frames) | |
| } | |
| // remove DC TODO: should really do this in int on C3 & S2... -> needs an update after PR #248 is merged | |
| int32_t sum = 0; | |
| for (int i = 0; i < num_samples; i++) sum += buffer[i]; | |
| int32_t mean = sum / num_samples; | |
| for (int i = 0; i < num_samples; i++) buffer[i] -= mean; //uses static mean, as it should not change too much over time, deducted above | |
| if (err == ESP_ERR_INVALID_STATE) { // error reading data, error means buffer overrun, need to fully reset the DMA ADC to make it work again | |
| DEBUGSR_PRINTF("ADC BFR OVERFLOW, RESETTING ADC\n"); | |
| reset_DMA_ADC(); | |
| } | |
| } | |
| void getSamples(float *buffer, uint16_t num_samples) { | |
| if (!_initialized || buffer == nullptr || num_samples == 0) return; | |
| int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes | |
| uint8_t result[framesize]; // create a read buffer | |
| uint32_t ret_num; | |
| uint32_t totalbytes = 0; | |
| uint32_t j = 0; | |
| esp_err_t err = ESP_OK; | |
| do { | |
| err = adc_digi_read_bytes(result, framesize, &ret_num, ADC_TIMEOUT); // read samples | |
| if ((err == ESP_OK || err == ESP_ERR_INVALID_STATE) && ret_num > 0) { // in invalid sate (internal buffer overrun), still read the last valid sample, then reset the ADC DMA afterwards (better than not having samples at all) | |
| totalbytes += ret_num; // after an error, DMA buffer can be misaligned, returning partial frames. Found no solution to re-align or flush the buffers, seems to be yet another IDF4 bug | |
| if (totalbytes > framesize) { // got too many bytes to fit sample buffer | |
| ret_num -= totalbytes - framesize; // discard extra samples | |
| } | |
| for (int i = 0; i < ret_num; i += ADC_RESULT_BYTE) { | |
| adc_digi_output_data_t *p = reinterpret_cast<adc_digi_output_data_t*>(&result[i]); | |
| buffer[j++] = float((int(p->val & 0x0FFF))); // get the 12bit sample data and convert to float note: works on both format types | |
| // TODO: for integer math: when scaling up to 16bit: compared to I2S mic the scaling seems about the same when not shifting at all, so need to divide by 16 after FFT if scaling up to 16bit | |
| } | |
| } else { // no samples or other error: usually ESP_ERR_TIMEOUT (if DMA has stopped for some reason) | |
| reset_DMA_ADC(); | |
| DEBUGSR_PRINTF("ADC ERROR!\n"); | |
| return; // something went very wrong, just exit | |
| } | |
| } while (totalbytes < framesize); // read more samples if a partial frame was returned (data is still consistent in split frames) | |
| // remove DC TODO: should really do this in int on C3 & S2... -> needs an update after PR `#248` is merged | |
| int32_t sum = 0; | |
| for (int i = 0; i < num_samples; i++) sum += buffer[i]; | |
| int32_t mean = sum / num_samples; | |
| for (int i = 0; i < num_samples; i++) buffer[i] -= mean; //uses static mean, as it should not change too much over time, deducted above | |
| if (err == ESP_ERR_INVALID_STATE) { // error reading data, error means buffer overrun, need to fully reset the DMA ADC to make it work again | |
| DEBUGSR_PRINTF("ADC BFR OVERFLOW, RESETTING ADC\n"); | |
| reset_DMA_ADC(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_source.h` around lines 828 - 867, getSamples()
can reach the DC-removal and the final "if (err == ESP_ERR_INVALID_STATE)" check
when _initialized is false, leaving err uninitialized; fix by handling the
uninitialized case immediately: in getSamples(), if (!_initialized) return (or
explicitly set err = ESP_OK and zero the buffer) before any DC-removal or
checking err; update references in this function (variables _initialized, err,
reset_DMA_ADC, and the DC-removal loops) so no code path reads err or processes
buffer when initialization failed.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
usermods/audioreactive/audio_source.h (2)
551-555: Consider removing the commented-out code.Since DMAadcSource now provides analog mic support for S2/C3/S3, this warning block is obsolete. Consider removing it entirely rather than leaving it commented out, to reduce code clutter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_source.h` around lines 551 - 555, Remove the obsolete commented-out ESP_IDF warning block in audio_source.h (the lines surrounding the prior SOC_I2S_SUPPORTS_ADC checks) since DMAadcSource now provides analog mic support for S2/C3/S3; delete the entire commented block to reduce clutter and keep the file focused on current implementations (search for the commented "#if ESP_IDF_VERSION..." / "#warning this MCU does not support analog sound input" block and remove it).
904-910: Consider checking return values inreset_DMA_ADC().The reset function ignores return values from
init_adc_continuous(). If reinitialization fails, sampling will silently fail. Consider logging errors or setting_initialized = falseon failure.🛡️ Proposed fix to handle reinitialization failure
void reset_DMA_ADC(void) { adc_digi_stop(); adc_digi_deinitialize(); - //delay(1); // TODO: need any delay? seems to work fine without it - init_adc_continuous(); - adc_digi_start(); //start sampling + if (init_adc_continuous() != ESP_OK) { + DEBUGSR_PRINTF("ADC reinit failed!\n"); + _initialized = false; + return; + } + adc_digi_start(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_source.h` around lines 904 - 910, In reset_DMA_ADC(), check the return value of init_adc_continuous() and handle failures: call init_adc_continuous() into a status/result variable, and if it indicates failure, log an error (e.g., Serial.error or existing logger) and set the module state flag (e.g., _initialized = false) and skip calling adc_digi_start(); only call adc_digi_start() when init_adc_continuous() succeeded and set _initialized = true on success. Ensure you reference reset_DMA_ADC, init_adc_continuous, adc_digi_start, and the _initialized flag when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_source.h`:
- Around line 869-880: In deinitialize(), the GPIO pin (_audioPin) is being
released via PinManager::deallocatePin before the ADC is stopped; reverse the
cleanup order so the ADC is fully stopped and deinitialized first (call
adc_digi_stop() and adc_digi_deinitialize() and wait/delay as needed), then call
PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive), and finally
clear _initialized and _myADCchannel as currently done to avoid releasing the
pin while ADC still owns it.
- Around line 804-826: The initialize method allocates the audio pin via
PinManager::allocatePin but doesn't release it on subsequent failure paths;
ensure you call PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive)
(or the project's deallocation API) before returning when
digitalPinToAnalogChannel(_audioPin) indicates an incompatible channel and also
if init_adc_continuous() returns non-ESP_OK; keep the successful flow unchanged
(set _audioPin after allocate, update configs, start sampling, set _initialized
= true).
- Around line 828-834: The getSamples function currently creates a VLA `uint8_t
result[framesize]` (framesize based on num_samples) which can overflow the stack
for large num_samples; replace the VLA with a safe buffer allocation strategy
such as using a pre-allocated class member buffer (e.g., an audio read buffer on
the object) or allocate on the heap with malloc/free (or unique_ptr) and check
allocation success before use; update references to `result`, `framesize`, and
`num_samples` in getSamples to use the new buffer and ensure you free/reuse the
buffer appropriately to avoid leaks or stack overflow.
- Around line 779-784: The adcpattern initializer uses ADC_ATTEN_DB_11 which is
deprecated in IDF 5.x; update the atten field in the adcpattern struct (the
adcpattern variable/initializer block) to ADC_ATTEN_DB_12 to match the rest of
the codebase and avoid deprecation warnings, ensuring the same 0-2.5V input
range semantics are preserved.
---
Nitpick comments:
In `@usermods/audioreactive/audio_source.h`:
- Around line 551-555: Remove the obsolete commented-out ESP_IDF warning block
in audio_source.h (the lines surrounding the prior SOC_I2S_SUPPORTS_ADC checks)
since DMAadcSource now provides analog mic support for S2/C3/S3; delete the
entire commented block to reduce clutter and keep the file focused on current
implementations (search for the commented "#if ESP_IDF_VERSION..." / "#warning
this MCU does not support analog sound input" block and remove it).
- Around line 904-910: In reset_DMA_ADC(), check the return value of
init_adc_continuous() and handle failures: call init_adc_continuous() into a
status/result variable, and if it indicates failure, log an error (e.g.,
Serial.error or existing logger) and set the module state flag (e.g.,
_initialized = false) and skip calling adc_digi_start(); only call
adc_digi_start() when init_adc_continuous() succeeded and set _initialized =
true on success. Ensure you reference reset_DMA_ADC, init_adc_continuous,
adc_digi_start, and the _initialized flag when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e4e1df8-007d-4e03-bcbc-12364724c4b1
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.h
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | ||
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | ||
| _myADCchannel = 0x0F; | ||
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | ||
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | ||
| return; | ||
| } | ||
| _audioPin = audioPin; | ||
| // Determine Analog channel. Only Channels on ADC1 are supported | ||
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | ||
| if (channel > MAX_ADC1_CHANNEL) { | ||
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | ||
| return; | ||
| } else { | ||
| _myADCchannel = channel; | ||
| } | ||
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | ||
| adcpattern.channel = channel; // update pattern config | ||
| if (init_adc_continuous() != ESP_OK) | ||
| return; | ||
| adc_digi_start(); //start sampling | ||
| _initialized = true; | ||
| } |
There was a problem hiding this comment.
Missing cleanup on initialization failure paths.
If channel validation fails (line 815) or init_adc_continuous() fails (line 822), the allocated pin is not deallocated. This could leave pins locked.
🔧 Proposed fix to deallocate pin on failure
if (channel > MAX_ADC1_CHANNEL) {
DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin);
+ PinManager::deallocatePin(audioPin, PinOwner::UM_Audioreactive);
return;
} else {
_myADCchannel = channel;
}
adc_dma_config.adc1_chan_mask = (1 << channel);
adcpattern.channel = channel;
- if (init_adc_continuous() != ESP_OK)
+ if (init_adc_continuous() != ESP_OK) {
+ PinManager::deallocatePin(audioPin, PinOwner::UM_Audioreactive);
return;
+ }📝 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.
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | |
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | |
| _myADCchannel = 0x0F; | |
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | |
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | |
| return; | |
| } | |
| _audioPin = audioPin; | |
| // Determine Analog channel. Only Channels on ADC1 are supported | |
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | |
| if (channel > MAX_ADC1_CHANNEL) { | |
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | |
| return; | |
| } else { | |
| _myADCchannel = channel; | |
| } | |
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | |
| adcpattern.channel = channel; // update pattern config | |
| if (init_adc_continuous() != ESP_OK) | |
| return; | |
| adc_digi_start(); //start sampling | |
| _initialized = true; | |
| } | |
| void initialize(int8_t audioPin, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE, int8_t = I2S_PIN_NO_CHANGE) { | |
| DEBUGSR_PRINTLN(F("DMAadcSource::initialize()")); | |
| _myADCchannel = 0x0F; | |
| if(!PinManager::allocatePin(audioPin, false, PinOwner::UM_Audioreactive)) { | |
| DEBUGSR_PRINTF("failed to allocate GPIO for audio analog input: %d\n", audioPin); | |
| return; | |
| } | |
| _audioPin = audioPin; | |
| // Determine Analog channel. Only Channels on ADC1 are supported | |
| int8_t channel = digitalPinToAnalogChannel(_audioPin); | |
| if (channel > MAX_ADC1_CHANNEL) { | |
| DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin); | |
| PinManager::deallocatePin(audioPin, PinOwner::UM_Audioreactive); | |
| return; | |
| } else { | |
| _myADCchannel = channel; | |
| } | |
| adc_dma_config.adc1_chan_mask = (1 << channel); // update mask in DMA config | |
| adcpattern.channel = channel; // update pattern config | |
| if (init_adc_continuous() != ESP_OK) { | |
| PinManager::deallocatePin(audioPin, PinOwner::UM_Audioreactive); | |
| return; | |
| } | |
| adc_digi_start(); //start sampling | |
| _initialized = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_source.h` around lines 804 - 826, The initialize
method allocates the audio pin via PinManager::allocatePin but doesn't release
it on subsequent failure paths; ensure you call
PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive) (or the
project's deallocation API) before returning when
digitalPinToAnalogChannel(_audioPin) indicates an incompatible channel and also
if init_adc_continuous() returns non-ESP_OK; keep the successful flow unchanged
(set _audioPin after allocate, update configs, start sampling, set _initialized
= true).
| void getSamples(float *buffer, uint16_t num_samples) { | ||
| int32_t framesize = num_samples * ADC_RESULT_BYTE; // size of one sample frame in bytes | ||
| uint8_t result[framesize]; // create a read buffer | ||
| uint32_t ret_num; | ||
| uint32_t totalbytes = 0; | ||
| uint32_t j = 0; | ||
| esp_err_t err; |
There was a problem hiding this comment.
VLA allocation could cause stack overflow with large sample counts.
The VLA uint8_t result[framesize] allocates ~2KB on the stack (512 samples × 4 bytes). While this works for typical configurations, it's fragile if num_samples increases. Consider using a fixed-size buffer or heap allocation.
🛡️ Suggested fix using class member buffer
+ private:
+ static constexpr size_t MAX_FRAME_SIZE = 512 * ADC_RESULT_BYTE; // max expected frame
+ uint8_t _resultBuffer[MAX_FRAME_SIZE];
+
void getSamples(float *buffer, uint16_t num_samples) {
int32_t framesize = num_samples * ADC_RESULT_BYTE;
- uint8_t result[framesize];
+ if (framesize > MAX_FRAME_SIZE) framesize = MAX_FRAME_SIZE;
+ uint8_t* result = _resultBuffer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_source.h` around lines 828 - 834, The getSamples
function currently creates a VLA `uint8_t result[framesize]` (framesize based on
num_samples) which can overflow the stack for large num_samples; replace the VLA
with a safe buffer allocation strategy such as using a pre-allocated class
member buffer (e.g., an audio read buffer on the object) or allocate on the heap
with malloc/free (or unique_ptr) and check allocation success before use; update
references to `result`, `framesize`, and `num_samples` in getSamples to use the
new buffer and ensure you free/reuse the buffer appropriately to avoid leaks or
stack overflow.
| void deinitialize() { | ||
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | ||
| _initialized = false; | ||
| _myADCchannel = 0x0F; | ||
| esp_err_t err; | ||
| adc_digi_stop(); | ||
| delay(50); // just in case, give it some time | ||
| err = adc_digi_deinitialize(); | ||
| if (err != ESP_OK) { | ||
| DEBUGSR_PRINTF("Failed deinit ADC: %d\n", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pin deallocation should occur after stopping ADC.
The pin is deallocated on line 870 before stopping the ADC on line 874. This order should be reversed to ensure the ADC is fully stopped before releasing the pin.
🔧 Proposed fix for cleanup order
void deinitialize() {
- PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive);
_initialized = false;
_myADCchannel = 0x0F;
esp_err_t err;
adc_digi_stop();
delay(50); // just in case, give it some time
err = adc_digi_deinitialize();
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed deinit ADC: %d\n", err);
}
+ PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive);
}📝 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.
| void deinitialize() { | |
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | |
| _initialized = false; | |
| _myADCchannel = 0x0F; | |
| esp_err_t err; | |
| adc_digi_stop(); | |
| delay(50); // just in case, give it some time | |
| err = adc_digi_deinitialize(); | |
| if (err != ESP_OK) { | |
| DEBUGSR_PRINTF("Failed deinit ADC: %d\n", err); | |
| } | |
| } | |
| void deinitialize() { | |
| _initialized = false; | |
| _myADCchannel = 0x0F; | |
| esp_err_t err; | |
| adc_digi_stop(); | |
| delay(50); // just in case, give it some time | |
| err = adc_digi_deinitialize(); | |
| if (err != ESP_OK) { | |
| DEBUGSR_PRINTF("Failed deinit ADC: %d\n", err); | |
| } | |
| PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/audioreactive/audio_source.h` around lines 869 - 880, In
deinitialize(), the GPIO pin (_audioPin) is being released via
PinManager::deallocatePin before the ADC is stopped; reverse the cleanup order
so the ADC is fully stopped and deinitialized first (call adc_digi_stop() and
adc_digi_deinitialize() and wait/delay as needed), then call
PinManager::deallocatePin(_audioPin, PinOwner::UM_Audioreactive), and finally
clear _initialized and _myADCchannel as currently done to avoid releasing the
pin while ADC still owns it.
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