Nunocky / Nucleo_L476RG_WavPlay

Play WAV files read from SD Card
13 stars 3 forks source link

Clicking sound - too many samples requested to DMA? #1

Closed mjuhanne closed 3 years ago

mjuhanne commented 3 years ago

Hello and thank you for your nice example.

Can you please re-check the number of samples requested to DMA: HAL_DAC_Start_DMA(&hdac1, DAC_CHANNEL_1, (uint32_t*)dmaBuffer[dmaBank], numSamples * 2 , DAC_ALIGN_12B_R);

but it should be HAL_DAC_Start_DMA(&hdac1, DAC_CHANNEL_1, (uint32_t*)dmaBuffer[dmaBank], numSamples , DAC_ALIGN_12B_R);

Otherwise there's horrible sound 50% of the time when DAC is playing data outside the buffer. I tested example .wav files from https://www2.cs.uic.edu/~i101/SoundFiles/ I checked that in my project the DMA data length is correct (Half-word ).

Note that the data size seems to be correct in your other example project (L476_DAC) and I have no problem playing the sound.

Nunocky commented 3 years ago

Thank you for the report. I'm sorry it took me so long to get a response. I tried to play the sample WAV files you mentioned here, and all the files played fine in my environment without any modification of the source code.

https://www.youtube.com/watch?v=oQGmY4_SJQU

The value of numSamples is a variable that depends on the number of bytes per sample (bytesPerSample) and the number of channels, so you may want to recheck that.

Translated with www.DeepL.com/Translator (free version)

Nunocky commented 3 years ago

Also, what about the DMA settings?

スクリーンショット 2021-01-29 20 37 54
mjuhanne commented 3 years ago

Yes, I checked the DMA settings and they are identical with yours. To be honest, I did not try to compile your code 100% because I don't have a SD card reader with SDIO but instead I copy-pasted parts of your code to my own project that uses SD card with SPI interface.

However I still don't understand how the audio can sound flawless with your code (with numSamples 2) , because the length parameter to _HAL_DAC_StartDMA is "number of DMA read/write operations" and NOT bytes. The DMA Data Width is Half word (16 bits) and we use 16-bit data with dmaBuffer (even though it's defined as uint8_t but is addressed as uint16_t in prepareDACBuffer_8Bit/16bit) so the length should be numSamples (and not numSamples2).

Is there something I have misunderstood? Can you also please check if your code works with numSamples (*1) ? Onegai shimasu! :)

Best regards, Marko

Nunocky commented 3 years ago

🤔

https://github.com/Nunocky/Nucleo_L476RG_WavPlay/blob/main/CubeIDE/L476RG_WavPlay/Drivers/STM32L4xx_HAL_Driver/Src/stm32l4xx_hal_dac.c#L747-L766

  • @param Length The length of data to be transferred from memory to DAC peripheral

And the definition of the HAL_DMA_Start_IT function used in HAL_DAC_Start_DMA,

https://github.com/Nunocky/Nucleo_L476RG_WavPlay/blob/main/CubeIDE/L476RG_WavPlay/Drivers/STM32L4xx_HAL_Driver/Src/stm32l4xx_hal_dma.c#L464-L474

  • @param DataLength The length of data to be transferred from source to destination

Both are written only as "length", not "number of DMA read / write operations". Where was this description?

mjuhanne commented 3 years ago

Yes, it's surprisingly ambiguous! HAL documentation only talks about "length of data", but doesn't state what unit (byte, word, half-word) it means. Even though not explicitly stated, it is implied that the unit is in fact the "Data Width" set in CubeMX.

Further digging/debugging the source code it can be seen that HAL_DAC_Start_DMA calls DMA_SetConfig which modifies CNDTR register

/* Configure DMA Channel data length */
  hdma->Instance->CNDTR = DataLength;

Looking at the STM32 reference manual RM0351 On page 343 it is explained that DMA_CNDTRx "contains the remaining number of data items to transfer (number of AHB ‘read followed by write’ transfers)." And these read-write transfers are byte, half-word or word long. On page 344:

Programmable data sizes
The transfer sizes of a single data (byte, half-word, or word) to the peripheral and memory
are programmable through, respectively, the PSIZE[1:0] and MSIZE[1:0] fields of the
DMA_CCRx register.

In Drivers/STM32L4xx_HAL_Driver/Inc/stm32l4xx_hal_dma.h

struct {
....
  uint32_t PeriphDataAlignment;       /*!< Specifies the Peripheral data width.
                                           This parameter can be a value of @ref DMA_Peripheral_data_size */

  uint32_t MemDataAlignment;          /*!< Specifies the Memory data width.
                                           This parameter can be a value of @ref DMA_Memory_data_size */
....
} DMA_InitTypeDef;

In HAL_DAC_MspInit the data length is set (configured in CubeMX DMA Data Width)

hdma_dac_ch1.Init.PeriphDataAlignment = DMA_PDATAALIGN_HALFWORD;
hdma_dac_ch1.Init.MemDataAlignment = DMA_MDATAALIGN_HALFWORD;

and it is used in HAL_DMA_Init to set the CCR register.

  /* Get the CR register value */
  tmp = hdma->Instance->CCR;

  /* Clear PL, MSIZE, PSIZE, MINC, PINC, CIRC, DIR and MEM2MEM bits */
  tmp &= ((uint32_t)~(DMA_CCR_PL    | DMA_CCR_MSIZE  | DMA_CCR_PSIZE  |
                      DMA_CCR_MINC  | DMA_CCR_PINC   | DMA_CCR_CIRC   |
                      DMA_CCR_DIR   | DMA_CCR_MEM2MEM));

  /* Prepare the DMA Channel configuration */
  tmp |=  hdma->Init.Direction        |
          hdma->Init.PeriphInc           | hdma->Init.MemInc           |
          hdma->Init.PeriphDataAlignment | hdma->Init.MemDataAlignment |
          hdma->Init.Mode                | hdma->Init.Priority;

  /* Write to DMA Channel CR register */
  hdma->Instance->CCR = tmp;
Nunocky commented 3 years ago

I found the cause! :-D

The location to set the flg_dma_done variable, which means the completion of the DMA transfer, was wrong.

Previously, this variable was set in DMA1_Channel3_IRQHandler, but this interrupt was also generated when half of the DMA transfer had elapsed, so the DMA transfer had to be twice as long as it should have been.

The correct place is the HAL_DAC_ConvCpltCallbackCh1 callback.

The changes can be found below. https://github.com/Nunocky/Nucleo_L476RG_WavPlay/pull/2/files

Without your report, I would not have been able to notice this bug. Thank you so much!

mjuhanne commented 3 years ago

Glad you find the culprit! It's sometimes very hard to find bugs when two errors cancel each other out :)

Thank you again for the nice example code. This helps me develop a STM32+ESP32 based alarm clock that plays music from SD card.