Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 16 additions & 30 deletions usermods/audioreactive/audio_reactive.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ void FFTcode(void * parameter)
#endif

xLastWakeTime = xTaskGetTickCount(); // update "last unblocked time" for vTaskDelay

// band pass filter - can reduce noise floor by a factor of 50
// downside: frequencies below 100Hz will be ignored
if (useBandPassFilter) runMicFilter(samplesFFT, vReal);
Expand Down Expand Up @@ -393,9 +392,8 @@ void FFTcode(void * parameter)
// run peak detection
autoResetPeak();
detectSamplePeak();

#if !defined(I2S_GRAB_ADC1_COMPLETELY)
if ((audioSource == nullptr) || (audioSource->getType() != AudioSource::Type_I2SAdc)) // the "delay trick" does not help for analog ADC
#if !defined(I2S_GRAB_ADC1_COMPLETELY) && !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
if ((audioSource == nullptr) || (audioSource->getType() != AudioSource::Type_Adc)) // the "delay trick" does not help for analog ADC
#endif
vTaskDelayUntil( &xLastWakeTime, xFrequency); // release CPU, and let I2S fill its buffers

Expand Down Expand Up @@ -672,9 +670,7 @@ class AudioReactive : public Usermod {
static const char _dynamics[];
static const char _frequency[];
static const char _inputLvl[];
#if defined(ARDUINO_ARCH_ESP32) && !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
static const char _analogmic[];
#endif
static const char _digitalmic[];
static const char _addPalettes[];
static const char UDP_SYNC_HEADER[];
Expand Down Expand Up @@ -1177,12 +1173,9 @@ class AudioReactive : public Usermod {
#endif

switch (dmType) {
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
// stub cases for not-yet-supported I2S modes on other ESP32 chips
case 0: //ADC analog
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
// stub case for not-yet-supported I2S mode
case 5: //PDM Microphone
#endif
#endif
case 1:
DEBUGSR_PRINT(F("AR: Generic I2S Microphone - ")); DEBUGSR_PRINTLN(F(I2S_MIC_CHANNEL_TEXT));
Expand Down Expand Up @@ -1223,17 +1216,24 @@ class AudioReactive : public Usermod {
delay(100);
if (audioSource) audioSource->initialize(i2swsPin, i2ssdPin, i2sckPin, mclkPin);
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
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
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;
Comment on lines +1216 to +1231

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
#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.

#endif
}
delay(250); // give microphone enough time to initialise
Expand Down Expand Up @@ -1611,7 +1611,7 @@ class AudioReactive : public Usermod {
// Analog or I2S digital input
if (audioSource && (audioSource->isInitialized())) {
// audio source successfully configured
if (audioSource->getType() == AudioSource::Type_I2SAdc) {
if (audioSource->getType() == AudioSource::Type_Adc) {
infoArr.add(F("ADC analog"));
} else {
infoArr.add(F("I2S digital"));
Expand Down Expand Up @@ -1787,10 +1787,8 @@ class AudioReactive : public Usermod {
top[FPSTR(_addPalettes)] = addPalettes;

#ifdef ARDUINO_ARCH_ESP32
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
JsonObject amic = top.createNestedObject(FPSTR(_analogmic));
amic["pin"] = audioPin;
#endif

JsonObject dmic = top.createNestedObject(FPSTR(_digitalmic));
dmic["type"] = dmType;
Expand Down Expand Up @@ -1846,18 +1844,10 @@ class AudioReactive : public Usermod {
configComplete &= getJsonValue(top[FPSTR(_addPalettes)], addPalettes);

#ifdef ARDUINO_ARCH_ESP32
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
configComplete &= getJsonValue(top[FPSTR(_analogmic)]["pin"], audioPin);
#else
audioPin = -1; // MCU does not support analog mic
#endif

configComplete &= getJsonValue(top[FPSTR(_digitalmic)]["type"], dmType);
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
if (dmType == 0) dmType = SR_DMTYPE; // MCU does not support analog
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
if (dmType == 5) dmType = SR_DMTYPE; // MCU does not support PDM
#endif
#endif

configComplete &= getJsonValue(top[FPSTR(_digitalmic)]["pin"][0], i2ssdPin);
Expand Down Expand Up @@ -1893,9 +1883,7 @@ class AudioReactive : public Usermod {
#ifdef ARDUINO_ARCH_ESP32
uiScript.print(F("uxp=ux+':digitalmic:pin[]';")); // uxp = shortcut for AudioReactive:digitalmic:pin[]
uiScript.print(F("dd=addDropdown(ux,'digitalmic:type');"));
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
uiScript.print(F("addOption(dd,'Generic Analog',0);"));
#endif
uiScript.print(F("addOption(dd,'Generic I2S',1);"));
uiScript.print(F("addOption(dd,'ES7243',2);"));
uiScript.print(F("addOption(dd,'SPH0654',3);"));
Expand Down Expand Up @@ -2059,9 +2047,7 @@ const char AudioReactive::_config[] PROGMEM = "config";
const char AudioReactive::_dynamics[] PROGMEM = "dynamics";
const char AudioReactive::_frequency[] PROGMEM = "frequency";
const char AudioReactive::_inputLvl[] PROGMEM = "inputLevel";
#if defined(ARDUINO_ARCH_ESP32) && !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
const char AudioReactive::_analogmic[] PROGMEM = "analogmic";
#endif
const char AudioReactive::_digitalmic[] PROGMEM = "digitalmic";
const char AudioReactive::_addPalettes[] PROGMEM = "add-palettes";
const char AudioReactive::UDP_SYNC_HEADER[] PROGMEM = "00002"; // new sync header version, as format no longer compatible with previous structure
Expand Down
181 changes: 173 additions & 8 deletions usermods/audioreactive/audio_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ class AudioSource {
virtual bool isInitialized(void) {return(_initialized);}

/* identify Audiosource type - I2S-ADC or I2S-digital */
typedef enum{Type_unknown=0, Type_I2SAdc=1, Type_I2SDigital=2} AudioSourceType;
typedef enum{Type_unknown=0, Type_Adc=1, Type_I2SDigital=2} AudioSourceType;
virtual AudioSourceType getType(void) {return(Type_I2SDigital);} // default is "I2S digital source" - ADC type overrides this method

protected:
Expand Down Expand Up @@ -546,11 +546,11 @@ class ES8388Source : public I2SSource {

};

#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
//#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

#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
// ADC over I2S is only availeable in "classic" ESP32
Expand Down Expand Up @@ -583,8 +583,8 @@ class I2SAdcSource : public I2SSource {
};
}

/* identify Audiosource type - I2S-ADC*/
AudioSourceType getType(void) {return(Type_I2SAdc);}
/* identify Audiosource type - ADC*/
AudioSourceType getType(void) {return(Type_Adc);}

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("I2SAdcSource:: initialize()."));
Expand Down Expand Up @@ -743,6 +743,170 @@ class I2SAdcSource : public I2SSource {
int8_t _audioPin;
int8_t _myADCchannel = 0x0F; // current ADC channel for analog input. 0x0F means "undefined"
};
#else

/* ADC sampling with DMA
This microphone is an ADC pin sampled via ADC1 in continuous mode
This allows to sample in the background with high sample rates and minimal CPU load
note: only ADC1 channels can be used (ADC2 is used for WiFi)
ESP32 is not implemented as it supports I2S for ADC sampling (see above)
*/

#include "driver/adc.h"
#include "hal/adc_types.h"
#define ADC_TIMEOUT 30 // Timout for one full frame of samples in ms (TODO: use (FFT_MIN_CYCLE + 5) but need to move the ifdefs before the include in the cpp file)
#define ADC_RESULT_BYTE SOC_ADC_DIGI_RESULT_BYTES //for C3 & S3 this is 4 bytes, S2 is 2 bytes, first 12bits is ADC result, see adc_digi_output_data_t
#ifdef CONFIG_IDF_TARGET_ESP32C3
#define MAX_ADC1_CHANNEL 4 // C3 has 5 channels (0-4)
#else
#define MAX_ADC1_CHANNEL 9 // ESP32, S2, S3 have 10 channels (0-9)
#endif

class DMAadcSource : public AudioSource {
public:
DMAadcSource(SRate_t sampleRate, int blockSize, float sampleScale = 1.0f) :
AudioSource(sampleRate, blockSize, sampleScale) {
// ADC continuous mode configuration
adc_dma_config = {
.max_store_buf_size = (unsigned)blockSize * ADC_RESULT_BYTE, // internal storage of DMA driver (in bytes, one sample is 4 bytes on C3&S3, 2bytes on S2 note: using 2x buffer size would reduce overflows but can add latency
.conv_num_each_intr = (unsigned)blockSize * ADC_RESULT_BYTE, // number of bytes per interrupt (or per frame, one sample contains 12bit of sample data)
.adc1_chan_mask = 0, // ADC1 channel mask (set to correct channel in initialize())
.adc2_chan_mask = 0, // dont use adc2 (used for wifi)
};

adcpattern = {
.atten = ADC_ATTEN_DB_11, // approx. 0-2.5V input range
.channel = 0, // channel mask (set to correct channel in initialize())
.unit = 0, // use ADC1
.bit_width = SOC_ADC_DIGI_MAX_BITWIDTH, // set to 12bit
};
Comment thread
DedeHai marked this conversation as resolved.

dig_cfg = {
.conv_limit_en = 0, // disable limit (does not work right if enabled)
.conv_limit_num = 255, // set to max just in case
.pattern_num = 1, // single channel sampling
.adc_pattern = &adcpattern, // Pattern configuration
.sample_freq_hz = sampleRate, // sample frequency in Hz
.conv_mode = ADC_CONV_SINGLE_UNIT_1, // use ADC1 only
#ifdef CONFIG_IDF_TARGET_ESP32S2
.format = ADC_DIGI_OUTPUT_FORMAT_TYPE1, // S2 and ESP32 use TYPE1 (there is an error about this for S2 in IDF example)
#else
.format = ADC_DIGI_OUTPUT_FORMAT_TYPE2, // C3 and S3 use TYPE2 format
#endif
};
}

/* identify Audiosource type - ADC*/
AudioSourceType getType(void) {return(Type_Adc);}

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
Comment on lines +804 to +824

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

_initialized = true;
}
Comment on lines +804 to +826

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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;
Comment on lines +828 to +834

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

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();
}
}
Comment on lines +828 to +867

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.


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);
}
}
Comment on lines +869 to +880

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


private:
adc_digi_init_config_t adc_dma_config;
adc_digi_pattern_config_t adcpattern;
adc_digi_configuration_t dig_cfg;
int8_t _audioPin;
int8_t _myADCchannel = 0x0F; // current ADC channel for analog input. 0x0F means "undefined"

// Initialize ADC continuous mode with stored settings
esp_err_t init_adc_continuous() {
esp_err_t err = adc_digi_initialize(&adc_dma_config);
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed init ADC DMA: %d\n", err);
return err;
}

err = adc_digi_controller_configure(&dig_cfg);
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed init ADC sampling: %d\n", err);
}
return err;
}

void reset_DMA_ADC(void) {
adc_digi_stop();
adc_digi_deinitialize();
//delay(1); // TODO: need any delay? seems to work fine without it and this code can be invoked at any time, so do not waste time here
init_adc_continuous();
adc_digi_start(); //start sampling
}
};
#endif

/* SPH0645 Microphone
Expand Down Expand Up @@ -771,3 +935,4 @@ class SPH0654 : public I2SSource {
}
};
#endif