Open milindmovasha opened 1 year ago
felixcollins
commented
1 year ago I've just found the exact same thing. I've arrived at my calculation of 1.2195 independently so it seem there is a real bug here. Interestingly the data captured does seem complete. I fed a sine wave in and it looks correct when plotted. There must be a bug in the clock configuration for the ADC.
felixcollins
commented
1 year ago Relevant code is here: C:\Users\felix\esp5\esp-idf\components\hal\adc_hal.c
/**
* For esp32s2 and later chips
* - Set ADC digital controller clock division factor. The clock is divided from `APLL` or `APB` clock.
* Expression: controller_clk = APLL/APB * (div_num + div_a / div_b + 1).
* - Enable clock and select clock source for ADC digital controller.
* For esp32, use I2S clock
*/
static void adc_hal_digi_sample_freq_config(adc_hal_dma_ctx_t *hal, uint32_t freq)
{
#if !CONFIG_IDF_TARGET_ESP32
uint32_t interval = APB_CLK_FREQ / (ADC_LL_CLKM_DIV_NUM_DEFAULT + ADC_LL_CLKM_DIV_A_DEFAULT / ADC_LL_CLKM_DIV_B_DEFAULT + 1) / 2 / freq;
//set sample interval
adc_ll_digi_set_trigger_interval(interval);
//Here we set the clock divider factor to make the digital clock to 5M Hz
adc_ll_digi_controller_clk_div(ADC_LL_CLKM_DIV_NUM_DEFAULT, ADC_LL_CLKM_DIV_B_DEFAULT, ADC_LL_CLKM_DIV_A_DEFAULT);
adc_ll_digi_clk_sel(0); //use APB
#else
i2s_ll_rx_clk_set_src(hal->dev, I2S_CLK_SRC_DEFAULT); /*!< Clock from PLL_D2_CLK(160M)*/
uint32_t bclk_div = 16;
uint32_t bclk = freq * 2;
uint32_t mclk = bclk * bclk_div;
uint32_t mclk_div = I2S_BASE_CLK / mclk;
i2s_ll_rx_set_mclk(hal->dev, I2S_BASE_CLK, mclk, mclk_div);
i2s_ll_rx_set_bck_div_num(hal->dev, bclk_div);
#endif
}
lychee512
commented
1 year ago I have found the same issue on an ESP32, using a modified continuous_read_main.c that prints the sample waveforms to the output, allowing me to capture the waveforms. I sent in a 10 KHz triangle wave, and got this:
From this data, I found that the adc_continuous_read driver skips 2 samples every 11 samples, which is where the observed sampling frequency discrepancy comes from (11/9 = 1.22).
Modifying the sampling time data to correct for these skipped samples, we can get the correct measurements of the waveform (and get the correct frequency).
This behaviour is only apparent when using adc_continuous_read(), using the i2s adc driver doesn't show this behaviour.
higaski
commented
1 year ago 6 months have passed since I've last posted something related to the ongoing ADC continuous mode problems here #8874. I've re-visited the issue now and I still have no clue how this driver is supposed to behave/work...
I have taken the initial example from this issue and removed code for "non-S3" chips. I've setup the ADC to take 100.000 samples @ 10kHz. My highly advanced math skills tell me that it should take roughly 10s to take those, yet the loop which collects it finishes within 2.5s. So I'm not off by a factor of 1.22 but a factor of 4.
The ESP-IDF version I'm using is v5.2-dev-350-g56123c52aa
Any ideas, suggestions...?
#include <esp_adc/adc_continuous.h>
#include <esp_task.h>
#include <esp_timer.h>
#include <freertos/FreeRTOS.h>
#include <freertos/task.h>
#include <array>
#include <cstdio>
static constexpr auto max_number_of_samples{100'000uz};
static constexpr auto sampling_frequency{10u * 1000u};
static constexpr auto expected_time{static_cast<double>(max_number_of_samples) /
static_cast<double>(sampling_frequency)};
adc_continuous_handle_t adc1_handle{};
std::array<uint8_t, 256uz> result{};
// Times how long ADC takes to take "max_number_of_samples" at configured
// sampling frequency
void task_function(void*) {
uint32_t ret_num{};
size_t number_of_samples{};
// Start ADC
int64_t previous_us{esp_timer_get_time()};
ESP_ERROR_CHECK(adc_continuous_start(adc1_handle));
for (;;) {
// Read samples
if (adc_continuous_read(
adc1_handle, data(result), size(result), &ret_num, 0u) != ESP_OK)
continue;
// Sum all, once done print time it took to measure them
number_of_samples += ret_num;
if (number_of_samples < max_number_of_samples) continue;
number_of_samples = 0uz;
// Convert time it took from us to s (10^6)
auto const actual_us{esp_timer_get_time()};
auto const delta_time{(actual_us - previous_us) / 1e6};
printf("Taking %zu samples @ %uHz took %fs instead of %fs\n",
max_number_of_samples,
sampling_frequency,
delta_time,
expected_time);
// Stop ADC and read till empty (might print error message that ADC is
// already stopped since print before takes long)
adc_continuous_stop(adc1_handle);
while (adc_continuous_read(
adc1_handle, data(result), size(result), &ret_num, 0u) == ESP_OK)
;
// Restart ADC
previous_us = esp_timer_get_time();
ESP_ERROR_CHECK(adc_continuous_start(adc1_handle));
}
}
extern "C" void app_main() {
// Init ADC in continuous mode
adc_continuous_handle_cfg_t adc_config{
.max_store_buf_size = size(result) * SOC_ADC_DIGI_RESULT_BYTES * 8u,
.conv_frame_size = size(result),
};
ESP_ERROR_CHECK(adc_continuous_new_handle(&adc_config, &adc1_handle));
std::array patterns{
adc_digi_pattern_config_t{
.atten = ADC_ATTEN_DB_0,
.channel = ADC_CHANNEL_0,
.unit = ADC_UNIT_1,
.bit_width = ADC_BITWIDTH_12,
},
adc_digi_pattern_config_t{
.atten = ADC_ATTEN_DB_0,
.channel = ADC_CHANNEL_0,
.unit = ADC_UNIT_1,
.bit_width = ADC_BITWIDTH_12,
},
};
adc_continuous_config_t dig_cfg{
.pattern_num = size(patterns),
.adc_pattern = data(patterns),
.sample_freq_hz = sampling_frequency,
.conv_mode = ADC_CONV_SINGLE_UNIT_1,
.format = ADC_DIGI_OUTPUT_FORMAT_TYPE2,
};
ESP_ERROR_CHECK(adc_continuous_config(adc1_handle, &dig_cfg));
// Create a max priority task on core 1
xTaskCreatePinnedToCore(
task_function, NULL, 4096uz, NULL, ESP_TASK_PRIO_MAX, NULL, 1);
for (;;)
vTaskDelay(pdMS_TO_TICKS(1000u));
}I've also created a zip which contains this example: esp_idf_issue_10612-master.zip
/edit Ok, I'm sorry I've been an idiot again. Factor 4... is the difference between the size in bytes and the resulting integer.... oh boy.
felixcollins
commented
1 year ago @higaski it looks like you are configuring two patterns with the same channel.
higaski
commented
1 year ago I'm aware of that, but since I didn't care about the conversion results anyhow it didn't matter. I've just written that snippet to estimate the sampling frequency.
felixcollins
commented
1 year ago Could it be causing the sampling rate to be doubled?
higaski
commented
1 year ago Could it be causing the sampling rate to be doubled?
No definitely not. The number of channels defined in the pattern does not have any effect on the sampling rate, at least not in the ESP-IDF version I've been using.
I've fixed my stupid mistake and updated the example: https://github.com/espressif/esp-idf/issues/10612
Using the high resolution timer I can hardly find any discrepancy between the set sampling rate and the actual one. When taking 100.000 samples @ 10kHz the error measured this way is ~ <0.03% which is good enough for my use-cases.
Tom-Lichtinghagen
commented
1 year ago Hi,
I'm also facing the problem, that the real sampling freqeuncy is about 0.8 times the configured sampling frequency. Is there still no solution for this?
felixcollins
commented
1 year ago Has anyone from the IDF team even seen this yet? Hello out there!? This is a pretty serious bug.
felixcollins
commented
1 year ago higaski
commented
1 year ago The issue seems to be platform dependent. I've run your code on an ESP32-S3 and the error rate there is nowhere even near that high.
=~=~=~=~=~=~=~=~=~=~=~= PuTTY log 2023.05.10 12:22:44 =~=~=~=~=~=~=~=~=~=~=~=
Time: 76863611, samples per second: 40250
Time: 77863611, samples per second: 40250
Time: 78863611, samples per second: 40250
Time: 79863611, samples per second: 40250
Time: 80863611, samples per second: 40500
milindmovasha
commented
1 year ago The issue seems to be platform dependent. I've run your code on an ESP32-S3 and the error rate there is nowhere even near that high.
=~=~=~=~=~=~=~=~=~=~=~= PuTTY log 2023.05.10 12:22:44 =~=~=~=~=~=~=~=~=~=~=~= Time: 76863611, samples per second: 40250 Time: 77863611, samples per second: 40250 Time: 78863611, samples per second: 40250 Time: 79863611, samples per second: 40250 Time: 80863611, samples per second: 40500
Yes, I can confirm that I have tried the original code I submitted in bug report on ESP32-S2 and ESP32-S3 modules and it works as expected. It is specific to ESP32 module.
felixcollins
commented
1 year ago Could this PR fix the issue? https://github.com/espressif/esp-idf/pull/11411
klaus-liebler
commented
1 year ago I am facing similar issues with my ESP32-S3 and IDF 5.0.2 My test application is
#include <esp_log.h>
#include <esp_attr.h>
#include <freertos/FreeRTOS.h>
#include <freertos/task.h>
#include <esp_adc/adc_continuous.h>
#define TAG "MAIN"
#if (SOC_ADC_DIGI_RESULT_BYTES == 2)
#define ADC_TEST_OUTPUT_TYPE ADC_DIGI_OUTPUT_FORMAT_TYPE1
#else
#define ADC_TEST_OUTPUT_TYPE ADC_DIGI_OUTPUT_FORMAT_TYPE2
#endif
constexpr uint32_t ADC_TEST_FREQ_HZ{20 * 1000};
constexpr uint32_t CONVERSIONS_IN_ONE_CALLBACK{1};
constexpr uint32_t MEASUREMENT_DURATION_SECS{10};
static volatile DRAM_ATTR uint32_t successfulSamples{0};
static volatile DRAM_ATTR uint32_t errors{0};
adc_continuous_handle_t handle{nullptr};
extern "C" bool IRAM_ATTR NOINLINE_ATTR s_conv_done_cb(adc_continuous_handle_t handle, const adc_continuous_evt_data_t *edata, void *user_data)
{
if(edata->size==CONVERSIONS_IN_ONE_CALLBACK*SOC_ADC_DIGI_RESULT_BYTES){
successfulSamples=successfulSamples+CONVERSIONS_IN_ONE_CALLBACK;
}else{
errors=errors+1;
}
return false;
}
extern "C" void app_main(void)
{
adc_continuous_handle_cfg_t adc_config = {};
adc_config.max_store_buf_size = 1024;
adc_config.conv_frame_size = CONVERSIONS_IN_ONE_CALLBACK*SOC_ADC_DIGI_DATA_BYTES_PER_CONV;
adc_continuous_new_handle(&adc_config, &handle);
adc_digi_pattern_config_t adc_pattern[SOC_ADC_PATT_LEN_MAX];
adc_pattern[0].atten = ADC_ATTEN_DB_0;
adc_pattern[0].channel = ADC_CHANNEL_0;
adc_pattern[0].unit = ADC_UNIT_1;
adc_pattern[0].bit_width = SOC_ADC_DIGI_MAX_BITWIDTH;
adc_continuous_config_t dig_cfg = {};
dig_cfg.sample_freq_hz = ADC_TEST_FREQ_HZ,
dig_cfg.conv_mode = ADC_CONV_SINGLE_UNIT_1;
dig_cfg.format = ADC_TEST_OUTPUT_TYPE;
dig_cfg.adc_pattern = adc_pattern;
dig_cfg.pattern_num = 1;
adc_continuous_config(handle, &dig_cfg);
adc_continuous_evt_cbs_t cbs = {};
cbs.on_conv_done = s_conv_done_cb;
adc_continuous_register_event_callbacks(handle, &cbs, nullptr);
ESP_LOGI(TAG, "Start ADC with %luHz and %lu samples per callback and %lu seconds measurement time", ADC_TEST_FREQ_HZ, CONVERSIONS_IN_ONE_CALLBACK, MEASUREMENT_DURATION_SECS);
adc_continuous_start(handle);
vTaskDelay(pdMS_TO_TICKS(MEASUREMENT_DURATION_SECS*1000));
adc_continuous_stop(handle);
ESP_LOGI(TAG, "Stop ADC: successful:%lu (expecting: %lu!), errors %lu", successfulSamples, MEASUREMENT_DURATION_SECS*ADC_TEST_FREQ_HZ, errors);
while (true)
{
ESP_LOGI(TAG, "Heap %6lu", esp_get_free_heap_size());
vTaskDelay(pdMS_TO_TICKS(10000));
}
}The relevant log is
I (302) MAIN: Start ADC with 20000Hz and 1 samples per callback and 10 seconds measurement time
I (10312) MAIN: Stop ADC: successful:176266 (expecting: 200000!), errors 0So, instead of 200,000 samples in 10 seconds, I get 176,266.
Regards, Klaus
Tom-Lichtinghagen
commented
1 year ago @klaus-liebler that result is strange because higaski said that it works when using an ESP32-S3 derivate. The only difference I can see is that you are using IDF 5.0.2 and higaski is using IDF 5.2.
Can you switch to the IDF 5.2 and doublecheck whether your code works with that version? Maybe @higaski can also try it with IDF 5.0.2 to have a better evidence?
higaski
commented
1 year ago I'm getting 0.18 to 0.19% error with 5.0.2
klaus-liebler
commented
1 year ago I gave v5.2-dev-823-g903af13e84 a try. The results are more or less the same as before:
I (335) MAIN: Start ADC with 20000Hz and 1 samples per callback and 10 seconds measurement time
I (10345) MAIN: Stop ADC: successful:175425 (expecting: 200000.000000!), errors 0, deviation: -12.287500%Well, I've tried to narrow down the problem further and changed the samples per callback drastically to 256. That means, there are not 20000, but only 78 ISR calls per second. With both IDF versions, the result ist the same.
I (335) MAIN: Start ADC with 20000Hz and 256 samples per callback and 10 seconds measurement time
I (10345) MAIN: Stop ADC: successful:199680 (expecting: 200000.000000!), errors 0, deviation: -0.160000%So, the problem is not the frequency itself, but the very frequent calls into my ISR routine. Obviously, the ESP32 hardware or software architecture seems to be overwhelmed with 20k calls per second.
I then checked it exactly and tried different callback frequencies. The table and the diagram shows the dependency of the sample rare error [%] on the callback frequency [Hz] for different ADC frequencies (20kHz, 40kHz, 60kHz, 80kHz)
So, the error is more or less proportional to the callback frequency and somehow dependend on the ADC frequency. At least on my test setup, 60kHz ADC frequency seems to be the "sweet spot".
Cheers,
Klaus
Tom-Lichtinghagen
commented
1 year ago Thanks @klaus-liebler and @higaski, but this effect seems to be a different problem. In case of using the ESP32 changing the amount of callbacks does not have any impact of the factor of 0.8.
klaus-liebler
commented
1 year ago @Tom-Lichtinghagen , do you recommend to file another issue for this or is this just beyond the limits of the architecture?
Tom-Lichtinghagen
commented
1 year ago Hi,
switching to the ESP32-S3 solved the problem regarding the sample rate deviation. But now I'm facing the problem, that the pool overflow interrupt is triggered although I read out the pool fast enough. Does anyone has face this problem also?
Another question from my side is if the conversion frame size and the maximal pool buffer size refer to all configured adc channels? That would imply that the time between two conversion done interrupts would depend on the number of used adc channel (with the conditional that the conversion frame size is the same). But tests did not show the behavior.
phoddie
commented
11 months ago FWIW – on ESP32 with ESP-IDF v5.1.1 the the sampling rate is still incorrect. When used to record audio through an analog input, it plays back at noticeably a higher pitch than the input.
cazou
commented
5 months ago Hi !
I am confronted to this issue it seems, with indeed, my requested frequency (44100 Hz) divided by ~1.22 on ESP32.
From what I can see, the ADC continuous driver uses the i2s DMA, which is configured where @felixcollins mentioned in https://github.com/espressif/esp-idf/issues/10612#issuecomment-1498487433
According to the esp32 TRM:
For high
performance audio applications, the analog PLL output clock source APLL_CLK must be used to acquire
highly accurate I2Sn_CLK and BCK.(Page 310, 12.3 The Clock of I2S Module)
But in the initialization code, of the adc_hal, I2S_CLK_SRC_DEFAULT is used, which is set to the PLL_D2_CLK(160M) clock: The clock the TRM says is not stable enough for audio.
The DAC continuous driver has a clk_src configuration field that can be set to DAC_DIGI_CLK_SRC_APLL, but is not present for ADC. I will be looking at porting this to the ADC continuous driver to see if the clock is more precise.
Did anyone successfully got the correct frequency with PLL_D2_CLK(160M) ?
cazou
commented
5 months ago There is no noticeable improvement when using APLL as clock source. But I was able to confirm that the I2S clocks are correctly configured to closely match the requested sampling frequency. It is also configured the same way when using the continuous DAC, which is outputting the signal at the correct frequency.
In my test program, the interrupt routine is also called correctly:
edata->size in adc_continuous_evt_cbs_t::on_conv_done will get this value to the same as counting the number of bytes returned by adc_continuous_read() since the call to start()adc_continuous_evt_cbs_t::on_pool_ovf is never calledBut still, the effective sampling frequency is 36082 Hz instead of 44100. (/1.22)
In v4.4, with a similar program (the adc_continuous and i2s APIs have changed a lot between v4 and v5), the effective frequency is 43690 Hz, which is acceptable for me but could probably be better. That means that the issue is probably introduced with the new API in v5, but more importantly, that the issue is in software, not in HW (or at least it has a SW solution).
I will be looking into the DMA configuration next. It could be misconfigured and ends up timeout-ing sometimes.
cazou
commented
5 months ago I pushed a simple fix, which is more a workaround, but can help for now. Next, I'd like to analyze why those 2 samples are lost in continuous mode.
Note that continuous mode will not start if the max_num is disabled.
felixcollins
commented
5 months ago Now, will Espressif investigate a proper fix for this!? It seems like @cazou has found an important clue.
cazou
commented
5 months ago This is definitely an issue on the ESP32. The same code on esp32-H2 (or others as mentioned by other people) yields the correct frequency.
Only on ESP32 is there a comment saying:
//On esp32, ADC can only be continuously triggered when ADC_LL_DEFAULT_CONV_LIMIT_EN == 1, ADC_LL_DEFAULT_CONV_LIMIT_NUM != 0
All the other ones have it deactivated by default (and the API only uses the default value):
$ git grep ADC_LL_DEFAULT_CONV_LIMIT_EN
components/hal/esp32/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 1
components/hal/esp32c3/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 0
components/hal/esp32c6/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 0
components/hal/esp32h2/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 0
components/hal/esp32s2/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 0
components/hal/esp32s3/include/hal/adc_ll.h:#define ADC_LL_DEFAULT_CONV_LIMIT_EN 0Also, the ESP32 is the only one using the i2s DMA. Some use SPI or GDMA. There is no GDMA on ESP32 but there is a SPI DMA, maybe using that one would work better, but I don't know if that is possible.
It would be nice to have an investigation on espressif side, at least to confirm that it is an issue on the ESP32 and that it won't be solved.
Maybe @mythbuster5 could shine some light on this ?
jsb1
commented
4 months ago Found something (ESP 32, Arduino 2.04, ESP.getChipRevision() is 1 )
Setting SYSCON.saradc_ctrl2.meas_num_limit=0; after starting DMA, I'm getting exact sample rates - as far as clock dividers allow.
Before:
Buffer#787140, Bytes=99600, Lastbytes=1200, sps=49800.000000, ret=0, is2ret=0 After: Buffer#41877006, Bytes=100000, Lastbytes=1200, sps=50000.000000 ret=0, is2ret=0
ADC values seem to be stable. I'll let it run for a day.
The question is now: Where is the Bug? Maybe in chip documentation? And in all code derived from that?
BTW: With APLL there are only 0 values from adc. Maybe clock is not routed to it.
cazou
commented
4 months ago Indeed ! because of ADC can only be continuously triggered when ADC_LL_DEFAULT_CONV_LIMIT_EN == 1, I assumed it had to stay that way, but it doesn't have to apparently.
I'm also getting way better results this way. So, as a workaround if people have this issue:
adc_continuous_start:
#include <hal/misc.h>
#include <soc/syscon_struct.h>And add this after the call to adc_continuous_start:
ESP_LOGI(TAG, "Deactivating meas_num_limit...");
HAL_FORCE_MODIFY_U32_REG_FIELD(SYSCON.saradc_ctrl2, meas_num_limit, 0);The ESP_LOGI seems necessary for some reason, maybe a delay issue. You may still miss 2 samples at the beginning though.
I'll create a PR to force espressif to hopefully move a bit on this
jsb1
commented
4 months ago Just an Info for Arduino users: DMA code on Arduino 2.0.4 / ESP-IDF 4.x is completely nonsense. Use Arduino 3 beta based on ESP-IDF 5.1+ and continous ADC API !!!
jsb1
commented
4 months ago And Yes, there might be a bug on chip. Sometimes when setting setting SYSCON.saradc_ctrl2.meas_num_limit=0, DMA stands still. Prepare Your software for reboot when DMA does not deliver data!
jsb1
commented
4 months ago My startup code looks like this now:
volatile uint32_t sumBytesAtIRQ;
....
for (int retry = 0; sumBytesAtIRQ == 0 && retry < 1000; retry++)
vTaskDelay(1 / portTICK_PERIOD_MS);
SYSCON.saradc_ctrl2.meas_num_limit = 0;static bool IRAM_ATTR convDoneCB(adc_continuous_handle_t handle, const adc_continuous_evt_data_t *edata, void *user_data)
{
MyType *self = (MyType *)user_data;
self->cpuCyclesAtIRQ = esp_cpu_get_cycle_count();
self->sumBytesAtIRQ += edata->size;
return pdFALSE;
}Seems to be safe.
Answers checklist.
General issue report
Hello, I have modified the ESP-IDF examples/peripherals/adc/continuous_read/main/continuous_read_main.c as follows to check the samples generated per second
Getting the following output when the above example is run on ESP32-DevKit-V1 board:
As seen in the results the samples are generated at rate 65287 samples/second whereas configured sampling frequency is 80000. I have tried with different values of desired sample frequency. There is always a ratio of 1.22 between expected desired frequency to achieved sampling rates.
Please help me understand why there is a difference between achieved sample rate vs configured sampling frequency.