STM32 - Reading I2S to record a .WAV file. Audio choppy, what is causing it?

I'm using an STM32 (STM32F446RE) to receive audio from two INMP441 mems microphone in an stereo setup via I2S protocol and record it into a .WAV on a micro SD card, using the HAL library.

I wrote the firmware that records audio into a .WAV with FreeRTOS. But the audio files that I record sound like Darth Vader. Here is a screenshot of the audio in audacity:

if you zoom in you can see a constant noise being inserted in between the real audio data:

I don't know what is causing this.

I have tried increasing the MessageQueue, but that doesnt seem to be the problem, the queue is kept at 0 most of the time. I've tried different frame sizes and sampling rates, changing the number of channels, using only one inmp441. All this without any success.

I proceed explaining the firmware.

Here is a block diagram of the architecture for the RTOS that I have implemented:

It consists of three tasks. The first one receives a command via UART (with interrupts) that signals to start or stop recording. the second one is simply an state machine that walks through the steps to write a .WAV.

Here the code for the WriteWavFileTask:

        switch(audio_state)
        {
        case STATE_START_RECORDING:

            sprintf(filename, "%saud_%03d.wav", SDPath, count++);

            do
            {
                res = f_open(&file_ptr, filename, FA_CREATE_ALWAYS|FA_WRITE);
            }
            while(res != FR_OK);

            res = fwrite_wav_header(&file_ptr, I2S_SAMPLE_FREQUENCY, I2S_FRAME, 2);

            HAL_I2S_Receive_DMA(&hi2s2, aud_buf, READ_SIZE);
            audio_state = STATE_RECORDING;
            break;

        case STATE_RECORDING:
            osDelay(50);
            break;

        case STATE_STOP:
            HAL_I2S_DMAStop(&hi2s2);
            while(osMessageQueueGetCount(AudioQueueHandle)) osDelay(1000);

            filesize = f_size(&file_ptr);
            data_len = filesize - 44;
            total_len = filesize - 8;
            f_lseek(&file_ptr, 4);
            f_write(&file_ptr, (uint8_t*)&total_len, 4, bw);
            f_lseek(&file_ptr, 40);
            f_write(&file_ptr, (uint8_t*)&data_len, 4, bw);
            f_close(&file_ptr);

            audio_state = STATE_IDLE;
            break;

        case STATE_IDLE:
            osThreadSuspend(WAVHandle);
            audio_state = STATE_START_RECORDING;
            break;

        default:
            osDelay(50);
            break;

Here are the macros used in the code for readability:

#define I2S_DATA_WORD_LENGTH    (24)                // industry-standard 24-bit I2S
#define I2S_FRAME               (32)                // bits per sample
#define READ_SIZE               (128)               // samples to read from I2S
#define WRITE_SIZE              (READ_SIZE*I2S_FRAME/16) // half words to write
#define WRITE_SIZE_BYTES        (WRITE_SIZE*2)      // bytes to write

#define I2S_SAMPLE_FREQUENCY    (16000)             // sample frequency

The last task is the responsible for processing the buffer received via I2S. Here is the code:

void convert_endianness(uint32_t *array, uint16_t Size) {
    for (int i = 0; i < Size; i++) {
        array[i] = __REV(array[i]);
    }
}
void HAL_I2S_RxCpltCallback(I2S_HandleTypeDef *hi2s)
{
    convert_endianness((uint32_t *)aud_buf, READ_SIZE);
    osMessageQueuePut(AudioQueueHandle, aud_buf, 0L, 0);

    HAL_I2S_Receive_DMA(hi2s, aud_buf, READ_SIZE);
}
void pvrWriteAudioTask(void *argument)
{
  /* USER CODE BEGIN pvrWriteAudioTask */
    static UINT *bw;
    static uint16_t aud_ptr[WRITE_SIZE];

    /* Infinite loop */
    for(;;)
    {
        osMessageQueueGet(AudioQueueHandle, aud_ptr, 0L, osWaitForever);

        res = f_write(&file_ptr, aud_ptr, WRITE_SIZE_BYTES, bw);
    }
  /* USER CODE END pvrWriteAudioTask */
}

This tasks reads from a queue an array of 256 uint16_t elements containing the raw audio data in PCM. f_write takes the Size parameter in number of bytes to write to the SD card, so 512 bytes. The I2S Receives 128 frames (for a 32 bit frame, 128 words).

The following is the configuration for the I2S and clocks:

Any help would be much appreciated!

Solution

As pmacfarlane pointed out, the problem was with the method used for buffering the audio data. The solution consisted of easing the overhead on the ISR and implementing a circular DMA for double buffering. Here is the code:

#define I2S_DATA_WORD_LENGTH    (24)                // industry-standard 24-bit I2S
#define I2S_FRAME               (32)                // bits per sample
#define READ_SIZE               (128)               // samples to read from I2S
#define BUFFER_SIZE             (READ_SIZE*I2S_FRAME/16) // number of uint16_t elements expected
#define WRITE_SIZE_BYTES        (BUFFER_SIZE*2)     // bytes to write

#define I2S_SAMPLE_FREQUENCY    (16000)             // sample frequency

uint16_t aud_buf[2*BUFFER_SIZE];            // Double buffering
static volatile int16_t *BufPtr;

void convert_endianness(uint32_t *array, uint16_t Size) {
    for (int i = 0; i < Size; i++) {
        array[i] = __REV(array[i]);
    }
}
void HAL_I2S_RxHalfCpltCallback(I2S_HandleTypeDef *hi2s)
{
    BufPtr = aud_buf;
    osSemaphoreRelease(RxAudioSemHandle);
}

void HAL_I2S_RxCpltCallback(I2S_HandleTypeDef *hi2s)
{
    BufPtr = &aud_buf[BUFFER_SIZE];
    osSemaphoreRelease(RxAudioSemHandle);
}

void pvrWriteAudioTask(void *argument)
{
  /* USER CODE BEGIN pvrWriteAudioTask */
    static UINT *bw;

    /* Infinite loop */
    for(;;)
    {
        osSemaphoreAcquire(RxAudioSemHandle, osWaitForever);

        convert_endianness((uint32_t *)BufPtr, READ_SIZE);
        res = f_write(&file_ptr, BufPtr, WRITE_SIZE_BYTES, bw);
    }
  /* USER CODE END pvrWriteAudioTask */
}

Problems

I think the problem is your method of buffering the audio data - mainly in this function:

void HAL_I2S_RxCpltCallback(I2S_HandleTypeDef *hi2s)
{
    convert_endianness((uint32_t *)aud_buf, READ_SIZE);
    osMessageQueuePut(AudioQueueHandle, aud_buf, 0L, 0);

    HAL_I2S_Receive_DMA(hi2s, aud_buf, READ_SIZE);
}

The main problem is that you are re-using the same buffer each time. You have queued a message to save aud_buf to the SD-card, but you've also instructed the I2S to start DMAing data into that same buffer, before it has been saved. You'll end up saving some kind of mish-mash of "old" data and "new" data.

@Flexz pointed out that the message queue takes a copy of the data, so there is no issue about the I2S writing over the data that is being written to the SD-card. However, taking the copy (in an ISR) adds overhead, and delays the start of the new I2S DMA.

Another problem is that you are doing the endian conversion in this function (that is called from an ISR). This will block any other (lower priority) interrupts from being serviced while this happens, which is a bad thing in an embedded system. You should do the endian conversion in the task that reads from the queue. ISRs should be very short and do the minimum possible work (often just setting a flag, giving a semaphore, or adding something to a queue).

Lastly, while you are doing the endian conversion, what is happening to audio samples? The previous DMA has completed, and you haven't started a new one, so they will just be dropped on the floor.

Possible solution

You probably want to allocate a suitably big buffer, and configure your DMA to work in circular buffer mode. This means that once started, the DMA will continue forever (until you stop it), so you'll never drop any samples. There won't be any gap between one DMA finishing and a new one starting, since you never need to start a new one.

The DMA provides a "half-complete" interrupt, to say when it has filled half the buffer. So start the DMA, and when you get the half-complete interrupt, queue up the first half of the buffer to be saved. When you get the fully-complete interrupt, queue up the second half of the buffer to be saved. Rinse and repeat.

You might want to add some logic to detect if the interrupt happens before the previous save has completed, since the data will be overrun and possibly corrupted. Depending on the speed of the SD-card (and the sample rate), this may or may not be a problem.