Analog Microphone audio source shows ADC analog - quiet after enable (audioreactive usermod)
#4345
Comments
|
Hi @pjkerly Thanks for looking into this common bug, can you try the MoonModules fork of WLED to see if the same issues exist as this is the source of that userMod but the version in this repo is very old version Do we have the same ordering issue in MM @softhack007 ? |
|
Hi @pjkerly, The first thing to try - and this helped in many cases - is to install WLED from this site https://wled-install.github.io/ instead of installing from install.wled.me. My suspicion is that the default WLED bootloader (created by @Aircoookie many years ago) has some problems with initializing the I2S-ADC peripheral unit properly. There is an old PR that also goes into the same direction atuline#257 Update:
This reminds me of another important solution Caution After changing audioreactive analog settings, a power off-on cycle is required - normal reboot is not enough. I2S-ADC will only initialize properly when switching the power off/on. If you don't do this, then "ADC analog - quiet" is expected behaviour. |
|
About your other findings - please double-check each of them individually before making a PR.
Where does this insight come from? Can you point to some espressif documentation that supports this statement? Actually the I2S-ADC init sequence is based on examples from espressif, and it's the same sequence you find in other open source libs that use I2S-ADC for input. and the "modernized" one
(my conclusion from the "modernized" espressif code was - if you want to specify a sample rate, you need to use MASTER mode together with I2S_MODE_ADC_BUILT_IN)
This might be possible, however there are lots of user setups where analogue microphones work without any problems - including my own test setup (but i'm not using analogue mics since almost 2 years - I2S digital is just better). If the sequence was generally wrong, I would expect that no analog microphone had ever worked 🤔
Maybe you misunderstood the code? There is no down-sampling involved - the code simply scales the microphone sample (float) which was previously up-scaled by the I2S driver - needed because we are requesting 32bit samples. As this works in many setups, I doubt that the processing code is totally wrong. Please try if it works better in pure 16bit mode (add You might want to try these
Plus, try if using a "V4" buildenv - with newer arduino-esp32 version 2.0.9 instead if 1.0.6 - makes a difference. You can use this buildenv Line 483 in 076497e Or this one, if you add the AR build_flags and lib_deps WLED/platformio_override.sample.ini Line 250 in 076497e |
|
Also this page is a great help if you cannot get analog microphones to work: https://github.com/atuline/WLED/wiki/First-Time-Setup#analog-or-i2s-digital Plus sometimes its sufficient to use a different pin for the analog input. And it's always good to double-check that the microphone Vcc (3.3V) and GND connections are stable. Even when you can see reaction with a simple a |
Actually I never had problems that could be related to ordering of initializations. @pjkerly The MoonModules fork is the "master" for audioreactive - please try if your microphone works better in the WLED-MM fork. The MoonModules fork of WLED (WLED-MM) can be found here https://github.com/MoonModules/WLED |
softhack007
added
cannot reproduce
softhack007
changed the title
ADC analog - quiet after starting (audioreactive usermod)
softhack007
changed the title
ADC analog - quiet after starting (audioreactive usermod)ADC analog - quiet after enable (audioreactive usermod)
|
Is there a known hardware issue that you are aware of? The comment below makes me even MORE suspicious that the I2S ADC Mode isn't being setup/disabled correctly. As a hardware guy, there is a problem if you have to physically power off the device to get it back in to a know state in order to get the software to work. In my opinion,, either a hardware power reset nor a hardware reboot should not be required. So far with my changes, it has been consistently stable without power cycles or resets required. This reminds me of another important solution Caution |
|
I've seen the examples and a lot of code examples that have used th I2S_MODE_MASTER. With my changes, I've tried with it and without. And I saw issues when it was enabled. From the espressif documents, I2S_MASTER_MODE generates the required clocks: But in I2S ADC Mode, I'm not even using those pins. It apparently is needed for external digital I2S devices which is not the case in ADC Mode from what I can tell. I did wonder if somehow the ADC Mode might use those signals internally. But removing it seemed to resolve some issues. So I decided to not enable it. After making all my other changes, I did try enabling and disabling the I2S_MODE_MASTER and it definitely caused issues when enabled. I will again triple check. |
Well, there are known problems with espressif driver software that requires power cycling - see atuline#231 (comment). Unfortunately, the workaround suggested by another user (same thread) did not work. So until espressif fixes their stuff (will not happen) we are stuck with "power cycle needed".
|
|
I've used all of these build settings in debugging ... -D I2S_USE_16BIT_SAMPLES to force 16bit processing instead of 32bit As well as the FFT_SAMPLING_LOG They've been very useful. |
|
I'll comment on this option which is not currently the default. I've read the comments in the code and warnings of using this build setting particularly if you need to use analogRead() which "will lock up the device". -D I2S_GRAB_ADC1_COMPLETELY to enable continuous sampling In my opinion, this should actually be the default for all builds. Or at a minimum for the esp32dec_audioreactive build (where there are no other UserMods included. My reasoning is because the current implementation enables/disables just before and just after getting samples. But esp32dev is a context interruptible device. Without synchronization with other parts of the code that might use analogRead, this approach is just Russian roulette. It is more challenging tracking down an intermittent "device lock up" than one that happens every time (or more frequently). If someone wants to use other UserMods with analog functionality along with Audio Reactive, then they definitely should use a digital microphone given the hardware esp32dev resource limitations. |
Actually it was the default, but we had to change that because users reported that some (older) esp32 boards were locking up, and getting into a bootloop. |
|
@pjkerly please also try the audioreactive code from the MoonModules fork. My suggestion would be to have two Analog drivers
It will also enable us to do back-to-back testing. |
|
I was using it a year to more and thought it was. I assumed it was. My guess is that it was changed because someone ran into the analogRead issue. I didn't read through the whole thread but I get the jist is In this setup, I2S and analogread() drivers seem to get into conflict, and they both stop working. Yes, it's clear that there are only two ADC units in hardware. One for the WiFi and one other used for analog audio. If anyone wants to do an analogRead, that is a problem. I don't think of that as a espressif issue as a much as a software issue utilizing. Issues like this can be very intermittent. |
Explain that to users who "simply have an analog button and want to add a microphone" 😉 So we came to recommend "avoid all the trouble and pulling-out of hair - simply get a cheap digital microphone or digital line-in". And on top of this: Due to another espressif bug, ADC#2 cannot be used "during Wi-Fi activity". In other words - from WLED point of view - ADC#2 is dead on esp32. |
Did you check if the ADC sampling frequency is correctly applied when you remove I2S_MODE_MASTER? |
|
So I've looked at the MM implementation and it looks the same. Have there been any issues intermittently reported on analog audio for this fork? I can share my proposed changes without a Pull Request if anyone just wants to see them and review them.
..... I'm a hardware / software guy ... I don't see this as a bug. It's a hardware resource limitation. There are only two ADCs. Use them as you want. But you don't get to use 3 ADCs. So if you can multiplex the usage of one or both, then that's great. But it that approach would require implementing synchronization. I didn't see anything like that in the audio reactive user mode. For WiFi, that might be a challenge. For audio, that could work. But without synchronization, the current implementation is "hit or miss". Maybe lucky most of the time or maybe even all time but it is still a "potential" bug with slightly different timing or slight changes to code. Not an easy bug to track down. I've seen a number of issues reported that seem to get closed without resolution. That sounds like a tough bug to track down. And is always subject to "it works for me" especially if you are only testing it and not actually using it on a regular basis. |
|
I know I'm coming into the "new" and don't have the history. So apologies if I'm rehashing old issues. At least I'm not just saying I've got an issue. I'm at least proposing changes that (at least to me) makes sense and fixes the issues I've seen. I hope you will consider the changes after you've had a change to review them. Should I just post the one file instead of Pull Request? |
@pjkerly new ideas are welcome 😃 Its just that people like me can recall history that lead to the situation as it is today. If you can do it: option1) adding a new instance of If that's too much for you, then option2) post a link to your modified Just make sure that your If needed, you can do the complete I2S config in your own constructor, instead of extending the base class constructor in https://github.com/MoonModules/WLED/blob/972257a7ee8cf800b9ec3d445039727b30003eb4/usermods/audioreactive/audio_source.h#L970 . Option 2 might take some more time for me to integrate the code, as I'm already over-busy with other things. But it will not be forgotten. |
softhack007
added
enhancement
and removed
bug
cannot reproduce
|
MM Fork has the same order issue ... (not including the other two changes I've made elsewhere). My general approach is to configure the hardware first in order of usage ... i.e. GPIO first because it is used by ADC, then ADC because it is used by I2S, then I2S hardware before installing the driver, enabling, and starting. On de-initialization, we only stop, disable, and uninstall. These of course leaves the hardware configured. It's up to the next guy to configure the hardware as needed. This could be the reason why a power-off cycle is helps. But in my case, I'm making sure that I'm initializing all of the hardware as needed before usage. The suggestion that the hardware is "locked" after the I2S driver installation comes from the behavior of the ESP-IDF (Espressif IoT Development Framework) as described in its API Reference Manual and practical usage patterns. Specifically:
1). I renabled setting this ... adc1_config_width(ADC_WIDTH_BIT_12); 2.) I hosted this line above the install of the driver : Rationale: When configuring hardware, I typically start with the GPIO configuration (ie input or output) which is already done in the code. Then configure the adc hardware, Then configure I2S hardware. Then install the driver. Then I can enable. 3.) I've added i2s_start / i2s_stop calls. It doesn't seem to make a difference for ADC Mode but it is part of the I2S life-cycle and I'm not sure of the internal implementation. So I included it anyway. Never know if the underlying implementation changes and maybe it might be necessary in the future. Anyway, it works for me consistently. 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("I2SAdcSource:: 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 > 9) {
DEBUGSR_PRINTF("Incompatible GPIO used for analog audio input: %d\n", _audioPin);
return;
} else {
adc_gpio_init(ADC_UNIT_1, adc_channel_t(channel));
_myADCchannel = channel;
}
adc1_config_width(ADC_WIDTH_BIT_12); // ensure that ADC runs with 12bit resolution
// see example in https://github.com/espressif/arduino-esp32/blob/master/libraries/ESP32/examples/I2S/HiFreq_ADC/HiFreq_ADC.ino
adc1_config_channel_atten(adc1_channel_t(channel), ADC_ATTEN_DB_11); // configure ADC input amplification
// Bug Fix: 4345 - Re-ordering of configuration and enabling ADC and I2S
// Configure I2S mode of ADC
esp_err_t err = i2s_set_adc_mode(ADC_UNIT_1, adc1_channel_t(channel));
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed to set i2s adc mode: %d\n", err);
return;
}
// Install Driver
err = i2s_driver_install(I2S_NUM_0, &_config, 0, nullptr);
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed to install i2s driver: %d\n", err);
return;
}
#if defined(I2S_GRAB_ADC1_COMPLETELY)
// according to docs from espressif, the ADC needs to be started explicitly
// fingers crossed - i2s_start/i2s_stop aren't used (needed) for I2S ADC Mode
err = i2s_adc_enable(I2S_NUM_0);
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed to enable i2s adc: %d\n", err);
//return;
}
//Bug Fix: 4345 - Adding I2S start / Stop functionality
// Not clear if absolutiely necessary for I2S ADC Mode since it seems to work without it
// But it's not clear if there is some other unknown effect managing resources.
// Since it is part of the I2S lifecycle including to be on the safe side.
err = i2s_start(I2S_NUM_0);
if (err != ESP_OK) {
DEBUGSR_PRINTF("Failed to stop i2s: %d\n", err);
}
#endif
_initialized = true;
} |
|
BTW, the UI allows someone to pick any GPIO for Analog microphone. In fact, it is only limited to GPIO pins associated with ADC1. It seems like this should be limited to them in the list of selectable PINs. Otherwise, the initial must be more familiar with the hardware. |
|
@pjkerly Thanks, i'll see how to best integrate your changes 🥇 The point about doing ADC init before So it works for you even without other modifications to |
|
I definitely understand the "new guy" concern :-) ... And I share it. But hopefully my rationale both explains the potential intermittent behavior and how my recommendations can avoid the random hardware setting issue. My recommended changes are isolated to the I2SAdcSource functions except for the downing sampling code which is in common in I2SSource getSamples function. I haven't looked at the whole down sample code, who uses it, or if it is really used. But in the case of I2SAdcSource which all appears to be 32-bit sampling doesn't work. With Down Sampling as currently set, you can look at the MIC_LOGGER. It appears to work initially. But if you let it run for a a little while you can watch the volumeSmth value start from 255 and dive to 0 and stays at 0. Once that happens, the Peak in the UI with show quiet to never recover. So I don't know what happens with the digital implementation but the Down Sampling is absolutely broken as implemented for analog. |
|
Ok, so I'll try to add a separate instance for getSample() to
It does work for I2S digital, and its necessary to utilize the full 24bit resolution from the digital I2S input. |
|
No. I think the initial setup / configuration issue is the cause of intermittent issues depending on the hardware initial state. That's a separate issue. I also removed the I2S_MODE_MASTER. It may not be a source of any bug. But it will put signals out on to the two GPIO signaling pins (i.e. additional energy). But also, don't want GPIO pins being unexpectedly active if not necessary. The definite other bug (no if's or but's) is that the down sampling drives the volumeSmth down to 0. (see previous comment). And then WLED audio reactive. Do you have analog setup? Just enable MIC_LOGGER to see the affect of Down Sampling causing a consistent bug. Can enable and disable the define and see the difference. |
@pjkerly not an active setup, but I still have two MAX4466 lying around so I'll be able to test. "two years ago" (last time i used analog) I did not see this 'drops to zero and stuck' behavior, so it might be that your setup is a bit different from mine. |
|
I had to make the following changes to the DEFINES to handle the down sample issue.... But again, I don't know what else it might affect but fixes the issue for I2SAdcSource implementation. #ifdef I2S_USE_16BIT_SAMPLES
#define I2S_SAMPLE_RESOLUTION I2S_BITS_PER_SAMPLE_16BIT
#define I2S_datatype int16_t
#define I2S_unsigned_datatype uint16_t
#define I2S_data_size I2S_BITS_PER_CHAN_16BIT
// Bug Fix: 4345 - This was forceing a Downsample of 32-bit to 16-bi for I2S ADC Mode
// #undef I2S_SAMPLE_DOWNSCALE_TO_16BIT
#define I2S_SAMPLE_DOWNSCALE_TO_16BIT
#else
#define I2S_SAMPLE_RESOLUTION I2S_BITS_PER_SAMPLE_32BIT
//#define I2S_SAMPLE_RESOLUTION I2S_BITS_PER_SAMPLE_24BIT
#define I2S_datatype int32_t
#define I2S_unsigned_datatype uint32_t
#define I2S_data_size I2S_BITS_PER_CHAN_32BIT
// Bug Fix: 4345 - This was forceing a Downsample of 32-bit to 16-bi for I2S ADC Mode
//#define I2S_SAMPLE_DOWNSCALE_TO_16BIT
#undef I2S_SAMPLE_DOWNSCALE_TO_16BIT
#endifIn this code ... I2S_datatype postProcessSample(I2S_datatype sample_in) {
static I2S_datatype lastADCsample = 0; // last good sample
static unsigned int broken_samples_counter = 0; // number of consecutive broken (and fixed) ADC samples
I2S_datatype sample_out = 0;
// bring sample down down to 16bit unsigned
I2S_unsigned_datatype rawData = * reinterpret_cast<I2S_unsigned_datatype *> (&sample_in); // C++ acrobatics to get sample as "unsigned"
#ifndef I2S_USE_16BIT_SAMPLES
rawData = (rawData >> 16) & 0xFFFF; // scale input down from 32bit -> 16bit
I2S_datatype lastGoodSample = lastADCsample / 16384 ; // prepare "last good sample" accordingly (26bit-> 12bit with correct sign handling)
#else
rawData = rawData & 0xFFFF; // input is already in 16bit, just mask off possible junk
I2S_datatype lastGoodSample = lastADCsample * 4; // prepare "last good sample" accordingly (10bit-> 12bit)
#endif
// decode ADC sample data fields
uint16_t the_channel = (rawData >> 12) & 0x000F; // upper 4 bit = ADC channel
uint16_t the_sample = rawData & 0x0FFF; // lower 12bit -> ADC sample (unsigned)
I2S_datatype finalSample = (int(the_sample) - 2048); // convert unsigned sample to signed (centered at 0);
if ((the_channel != _myADCchannel) && (_myADCchannel != 0x0F)) { // 0x0F means "don't know what my channel is"
// fix bad sample
finalSample = lastGoodSample; // replace with last good ADC sample
broken_samples_counter ++;
if (broken_samples_counter > 256) _myADCchannel = 0x0F; // too many bad samples in a row -> disable sample corrections
//Serial.print("\n!ADC rogue sample 0x"); Serial.print(rawData, HEX); Serial.print("\tchannel:");Serial.println(the_channel);
} else broken_samples_counter = 0; // good sample - reset counter
// back to original resolution
#ifndef I2S_USE_16BIT_SAMPLES
finalSample = finalSample << 16; // scale up from 16bit -> 32bit;
#endif
finalSample = finalSample / 4; // mimic old analog driver behaviour (12bit -> 10bit)
sample_out = (3 * finalSample + lastADCsample) / 4; // apply low-pass filter (2-tap FIR)
//sample_out = (finalSample + lastADCsample) / 2; // apply stronger low-pass filter (2-tap FIR)
lastADCsample = sample_out; // update ADC last sample
return(sample_out);
} |
|
I'm using the last branch from Aircookie/WLED branch _15 per the documentation for doing a Pull Request. Here is what the MIC_LOGGER looks like and never really changes ... micReal:615.89 volumeSmth:0.00 DC_Level:611.46 |
OK, please make sure to allow "maintainer edits", and make sure to have the PR coming from a separate branch in your repository. |
|
One other comment ... not sure if it is a bug in timing or if it is an actual artifact of enabling and disabling I2S for the non-continuous case... the Sampling goes from sub-milliseconds for continuous sampling time to ~20-25 milli-seconds in the non-continuous case. Again, I think the right default should be continuous for performance as well as less glitchy sampling . |
|
OK, I've already got my fork and separate branch. I'll get to this in a day or two. |
Yes that's expected behaviour - I'm not really happy with it (lots of discontinuous samples that need filtering), but it was the only possibility to give certain users a software that will not lock up. As I said before - I'll probably separate your code (with continuous sampling at 16bit) into a new driver instance, so users can have the possibility to use the old code (if it worked) or use your new code (if they had problems before). |
|
In my opinion, I believe that's just making the situation worse. I think making them happy by not locking up consistently to one that is potentially intermittent (and potentially different from build to build, run to run) doesn't sound like the right answer. The real answer is if you want to use analogRead() (you are free to use digitalRead() no problem) for some other usage, then only an external digital I2S device / mic is supported in that way because of hardware limitations. |
|
One other comment ... does digital Microphone require use of GPIO 32 for some reason? I ask because it isn't clear why use a GPIO that is capable of read or write but when you could use a GPIO pin that is R/O already and leave an I/O capable pin for potentially some other usage that might require it. |
Well that's your opinion - i have understood what you said, and I think I've already explained how it will be. Let's not re-iterate our previous discussions please.
It does not require it, but we've found that using gpio32 is better for some users. I don't remember why that is, but I don't think the default needs to change. Users are free to change the SD pin to one of the read-only pins if needed. There are ready made boards available from community members, and 90% of them have SD routed to gpio32. I don't want to break legacy here. |
|
That's great ... I was just wondering if digital microphones require the I/O pins. |
|
Partly "yes" - clock pins (SCK, WS, MCLK) must be inOut as they are driven by the esp32 in master mode. But SD (serial data) is a pure input stream so it can be mapped to input-only pins, too. |
|
Yup ... understand the clock pins. And understand the history of PIN usage. Sorry if I'm rehashing and I understand that you don't want to change the default behavior. But users should be aware of and on the lookout for random maybe even rare device lock ups that this known bug that can cause. The solution just moved it from always happening to an intermittent and random hard to debug situation. It would be extremely difficult to isolate and debug. I suspect it will go under reported due to the intermittent and maybe even pretty rare occasions. If users/developers understood the limitations, full implications, and fully correctable alternative solution, they might make other choices. Is there a good place to document some of these considerations. I understand you don't want to change it. But at a minimum, I would think the users of non-continuous sampling should be aware of the potential issue if they are using other user mods. I would be willing to update a readme or such documentation. Or should I just include in the source code by the define statement for I2S_GRAB_ADC1_COMPLETELY? |
|
here is suggested wording I'll include in my PR ... // Uncomment the line below to utilize ADC1 exclusively for I2S analog sound input. |
|
I've created the Pull Request ... can you let me know if I've done it correctly (i.e. you can see it and edit it). Thank You! |
|
I thought I had created a Pull Request. It was listed the other day. But now I don't see anything. Let me know if some how I didn't finish the process of the Pull Request. I can try again. Thanks. |
What happened?
Analog Microphone audio source after initially staring goes to ADC analog - quiet after starting. The lights are no longer responsive to audio sound / music. This happens for both pre-compiled and well as self-build binaries (multiple versions) for audio reactive. I've already debugged and am working on a Pull Request to submit proposed changes, review, and hopefully incorporated.
To Reproduce Bug
Install esp32dev_audioreactive binary. Configure the hardware. I'm using GPIO 36 R/O (supposed on ADC1). Select Generic Analog and Enable. Refresh the Info screen and see the audio source to go quiet.
Expected Behavior
Expected ADC Audio Source to register Peak (varying in %) in reaction for audio.
Install Method
Binary from WLED.me
What version of WLED?
v0.14.4
Which microcontroller/board are you seeing the problem on?
ESP32
Relevant log/trace output
No response
Anything else?
There are several issues with the current source code. I will Submit a PR soon. Summary 1). I2S_MODE_MASTER is not a suitable mode for I2S ADC configuration. Thie mode is for controlling an external I2S device. 2). The order of initializing the I2S driver and the associated ADC is not created in correct order. Should configure the ADC completely before installing the driver and then enable the I2S and ADC on the driver. The current order seems to be intermittent failure/success. Correct order seems to eliminate this intermittent failure. 3). The I2SSource getSamples tries to down sample to 16 bits when it shouldn't. This consistently causes the symptom of going to quiet.
Code of Conduct
The text was updated successfully, but these errors were encountered: