2

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: audacity

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

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: Architecture

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.

State Machine

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:

I2S 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 */
}

DMA

Wonky
  • 264
  • 2
  • 11
  • 1
    Can't say that this is the problem, but `HAL_I2S_RxCpltCallback` is called in an ISR. You probably don't want to do your endianness conversion in there. Do that in the non-ISR task that reads the queue. – pmacfarlane Jan 26 '23 at 21:11
  • 1
    Also, you receive some data in `aud_buf`, and queue it in a message queue. But then immediately start a _new_ DMA into the same buffer! You need some kind of double buffering, where you are receiving into one while the other is being saved. You might be able to make use of the DMA's "half complete" interrupt to facilitate that. – pmacfarlane Jan 26 '23 at 21:12

1 Answers1

2

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.

pmacfarlane
  • 3,057
  • 1
  • 7
  • 24
  • Doesn't `osMessageQueuePut` makes a copy of the message? – Flexz Jan 27 '23 at 05:16
  • @Flexz I don't know. If it does, I guess that mitigates the overwriting problem, but adds more overhead for the copy (in an ISR). – pmacfarlane Jan 27 '23 at 11:24
  • Thank you for your feedback! It does make a copy, but still I agree with you that the ISR has too much overhead and it is probably causing trouble. The noise chunk in between real audio has exactly 62 samples so it makes me think that by the time the ISR starts a new DMA receive its already late for the next packet of 62 bits (32 bits left channel, 32 bits right) so it buffers in some trash and then for the next packet is again in sync... ad infinitum. I'm implementing circular DMA and moving endiannes conversion to the task, see what I obtain. Thanks! – Wonky Jan 27 '23 at 11:31
  • Bingo! You were right mate!! Thanks a lot, I was hitting my head against the desk hehe. I implemented a circular buffer and some double buffering with the `halfcplt` and `cplt` ISR functions from HAL and now the noise is gone. I also got rid of the queue and replaced it with a semaphore. I will edit my question to add the code for the solution. Thanks again. – Wonky Jan 27 '23 at 12:18
  • That's great! A semaphore seems more sensible, copying all that data is pointless. I've updated my answer with the information @Flexz provided about the copy. – pmacfarlane Jan 27 '23 at 12:25