espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.89k stars 7.33k forks source link

i2s_read returns stale values (from the unconsumed tail of the dma buffer) (IDFGH-3737) #5655

Open aCaB opened 4 years ago

aCaB commented 4 years ago

Environment

Problem Description

The internal dma buffer pointer is not properly reset across i2s_adc_enable/i2s_adc_disable which causes the unconsumed tail of the previous read to be returned in place of the fresh data.

Expected Behavior

I expect that:

  1. i2s_adc_enable sets the start of the sampling session
  2. i2s_read returns data collected since i2s_adc_enable

Actual Behavior

After calling i2s_adc_enable, i2s_read returns cached (old) data.

Steps to reproduce

  1. setup the i2s_adc reader with buffers of size 1024
  2. feed a known level to the adc channel - e.g. 0
  3. call i2s_adc_enable
  4. do a partial i2s_read - e.g. read only 512 bytes
  5. call i2s_adc_disable
  6. verify the data received match the set level
  7. change the adc level - e.g. to 0xfff
  8. call i2s_adc_enable again
  9. call i2s_read again
  10. note that the first portion of the read contains the old value instead of the new one - i.e. 0 instead of 0xfff

Code to reproduce this issue

/* SETUP */
i2s_config_t i2s_config = {
         .mode = I2S_MODE_MASTER | I2S_MODE_RX | I2S_MODE_ADC_BUILT_IN,
         .sample_rate =  16000,
         .bits_per_sample = 16,
         .communication_format = I2S_COMM_FORMAT_STAND_MSB,
         .channel_format = I2S_CHANNEL_FMT_ONLY_LEFT,
         .intr_alloc_flags = 0,
         .dma_buf_count = 16,
         .dma_buf_len = 1024
};
i2s_driver_install(I2S_NUM_0, &i2s_config, 0, NULL);
i2s_set_adc_mode(ADC_UNIT_1, ADC1_CHANNEL_0);

/* READ ONCE  */
/* Note: set the input to a known stable value so it's easy to identify it */
size_t my_len = 512, bytes_read;
i2s_adc_enable(I2S_NUM_0);
/* Note: only the first 512 bytes are read from the current dma buffer */
i2s_read(I2S_NUM_0, my_buff, my_len, &bytes_read, portMAX_DELAY);
i2s_adc_disable(I2S_NUM_0);
/* NOTE THAT THE EXPECTED VALUES ARE RETURNED */

/* Note: change the input level to a different value so it's easy to identify it */
vTaskDelay(10000 / portTICK_RATE_MS);

/* READ SAMPLES #1 */
size_t my_len = 1024, bytes_read;
i2s_adc_enable(I2S_NUM_0);
i2s_read(I2S_NUM_0, my_buff, my_len, &bytes_read, portMAX_DELAY);
i2s_adc_disable(I2S_NUM_0);
/* NOTE THAT THE RETURNED VALUES  INITIALLY MATCH THE OLD VALUE */
aCaB commented 4 years ago

For what it's worth, it's possible to work around the problem in different ways.

One way is simply to discard the initial values read (up to the size of the dma buffer). This however introduces some latency.

Another way is to manually reset the state of the internal dma pointer. This is tricky as there seems to be no way to quickly set dma->curr_ptr to NULL via the api. Nonetheless it's possible to force a full dma buffer reallocation by changing bits_per_sample to a different value and back. E.g. assuming your desired bits_per_sample value is 16, you can add the following in between i2s_adc_disable and i2s_adc_enable:

i2s_set_clk(I2S_NUM_0, EXAMPLE_I2S_SAMPLE_RATE, 24, 1);
i2s_set_clk(I2S_NUM_0, EXAMPLE_I2S_SAMPLE_RATE, 16, 1);

Again this is far from ideal as it triggers full reallocation of the dma buffers.

It seems to me the reset should happen automatically upon i2s_adc_disable

Alvin1Zhang commented 4 years ago

Thanks for reporting, we will look into.

Ethan-CYQ commented 4 years ago

Hi, @aCaB There is no APIs to set DMA->curr_ptr to NULL or reallocate the DMA buffer directly, you can use i2s_set_clk to clear the old value in the DMA buffer as you said.