STM32H7 I2S HAL bugs

[Klang.Martin]

Posted on June 22, 2018 at 15:58

There seem to be some bugs in the H7 HAL library.

Firstly HAL_I2S_Receive_IT() causes a HardFault because, as far as I can see:

1) OVR flag is not cleared

2) in `HAL_I2S_IRQHandler()` there is an attempt to call ` hi2s->TxISR(hi2s);` when 

TxISR is set to NULL.

void HAL_I2S_IRQHandler(I2S_HandleTypeDef *hi2s)

{

uint32_t itsource = hi2s->Instance->IER;

uint32_t i2ssr = hi2s->Instance->SR;

/* I2S in mode Receiver ------------------------------------------------*/

if(((i2ssr & I2S_FLAG_OVR) != I2S_FLAG_OVR) &&

((i2ssr & I2S_FLAG_RXNE) == I2S_FLAG_RXNE) && ((itsource & I2S_IT_RXNE) !=

RESET))

{

hi2s->RxISR(hi2s); // not called because OVR not cleared

return;

}

/* I2S in mode Transmitter ---------------------------------------------*/

if(((i2ssr & I2S_FLAG_TXE) == I2S_FLAG_TXE) && ((itsource & I2S_IT_RXNE) != RE

SET))

{

hi2s->TxISR(hi2s); // HardFaults when TxISR == NULL

return;

}

Secondly in the polling function, HAL_I2S_Receive(), the destination data pointer is incremented 4x too fast.

In 32-bit and 24-bit data transfer mode, `hi2s->RxXferCount` is set to twice the Size, which is correct since Size is the number of half words to transfer.

Then, in the data transfer loop, the destination pointer is incremented like this:

hi2s->pRxBuffPtr += sizeof(uint32_t);

Since `hi2s->pRxBuffPtr` is of type `uint16_t*` this increments the pointer by 8 bytes with each half word transfer.

Note that the loop is still counting half-words, with `hi2s->RxXferCount--;`

So unless you've allocated an extra 4x buffer space then not only will you get the wrong results, you will also overflow badly and quite likely HardFault again.

I've not had any luck with HAL_I2S_Receive_DMA() so far (using RAM_D1), will report back when I've investigated some more.

This is using STM32CubeMX Version 4.26.0 and STM32Cube_FW_H7_V1.2.0 - latest versions.

#i2s #hal #stm32h7

[Klang.Martin]

Posted on June 22, 2018 at 16:06

Just spotted another bug there:

/* I2S in mode Transmitter ---------------------------------------------*/

if(((i2ssr & I2S_FLAG_TXE) == I2S_FLAG_TXE) && ((itsource & I2S_IT_RXNE) != RE

SET))

Shouldn't it be

/* I2S in mode Transmitter ---------------------------------------------*/

if(((i2ssr & I2S_FLAG_TXE) == I2S_FLAG_TXE) && ((itsource & I2S_IT_TXE) != RE

SET))

[Amel NASRI]

Posted on June 29, 2018 at 18:11

Hi

‌,

1-

in `HAL_I2S_IRQHandler()` there is an attempt to call ` hi2s->TxISR(hi2s);` when

TxISR is set to NULL.

'hi2s->TxISR(hi2s);' is called while being in receiver mode because of the issue you reported in your second post

In fact, this line is wrong:

if(((i2ssr & I2S_FLAG_TXE) == I2S_FLAG_TXE) && ((itsource & I2S_IT_RXNE) != RESET))�?�?

it has to be replaced by:

if(((i2ssr & I2S_FLAG_TXE) == I2S_FLAG_TXE) && ((itsource & I2S_IT_TXE) != RESET))�?�?

This is tracked internally in order to make required update, but you can check it at your end as well.

2- RegardingHAL_I2S_Receive implementation, in the case of data receive in 16-bit mode, we have the following:

 /* Check the RXNE flag */ if (__HAL_I2S_GET_FLAG(hi2s, I2S_FLAG_RXNE)) { if (hi2s->Instance->SR & I2S_FLAG_RXWNE) { *((uint32_t *)hi2s->pRxBuffPtr) = *((__IO uint32_t *)&hi2s->Instance->RXDR); hi2s->pRxBuffPtr += sizeof(uint32_t); hi2s->RxXferCount-=2; } else { *((uint16_t *)hi2s->pRxBuffPtr) = *((__IO uint16_t *)&hi2s->Instance->RXDR); hi2s->pRxBuffPtr += sizeof(uint16_t); hi2s->RxXferCount--; } }�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?

The incrementation 'hi2s->pRxBuffPtr += sizeof(uint32_t);' is done when RXWNE is set in SR register.

RXWNE = 1 if at least one 32-bit data is available in the RxFIFO. So in this case, it will be transferring a 32-bit data.

3- If you would like to use DMA, please refer to

https://community.st.com/docs/DOC-1988-faq-dma-is-not-working-stm32h7-devices

-Amel

[Klang.Martin]

Posted on June 29, 2018 at 18:38

Thank you for your reply Amel. That FAQ link is really useful.

> The incrementation 'hi2s->pRxBuffPtr += sizeof(uint32_t);' is done when RXWNE is set in SR register.

> RXWNE = 1 if at least one 32-bit data is available in the RxFIFO. So in this case, it will be transferring a 32-bit data.

Correct, but 

hi2s->pRxBuffPtr is of type `uint16_t*` so the increment will be twice as big as expected. The buffer will overflow and you will get a HardFault, clobbered data, or both.

What you could do is `

hi2s->pRxBuffPtr += sizeof(uint32_t)/sizeof(uint16_t);` or simply 

`

hi2s->pRxBuffPtr +=

 2;`.

This applies to all hi2s pointer increments in `stm32h7xx_hal_i2s.c`. There are nine of them in that file. It looks like the code has been copied straight from the SPI implementation, where the pointers are of type `uint8_t*` and therefore the increments are correct.

I don't know how you do your QA but with so many blatant bugs it is painfully obvious that this code has never been tested. Looking forward to seeing an update.

[Amel NASRI]

Posted on June 29, 2018 at 19:32

You are right, we need to re-check the implementation in both stm32h7xx_hal_i2s.c and stm32h7xx_hal_i2s_ex.c drivers.

-Amel

[Klang.Martin]

Posted on June 29, 2018 at 20:19

Great, thanks!

[TChat]

Hi,

Those bugs seem to be still present in STM32Cube_FW_H7_V1.3.0

Did you by any chance get to have your i2s transfer working ?

I am particularly interested in the DMA version...

Thanks!

[Klang.Martin]

We got somewhere down the line to a working version with the fixes referenced above, but in the end decided to ditch I2S for SAI, which works surprisingly well.

If you use SAI, just make sure `FrameInit.FSOffset` is correctly set for your peripheral.

[TChat]

Thanks a lot for your answer. I got it to work in polling mode too (thanks to you!), but I still can't get it to work with DMA.

Using SAI sounds good, but our hardware is already done since we are actually migrating from an stm32F7...

[Klang.Martin]

It's a bit crap that they're still pushing package releases with a peripheral component that patently doesn't work.

For DMA, are you allocating the buffer to the right memory segment?

See the reference above from Amel: https://community.st.com/docs/DOC-1988-faq-dma-is-not-working-stm32h7-devices

If you are using GCC you'll have to edit the .ld file.

[AnkhInnovationsLLC]

Yes, we measure our products in time to working with the suppliers we have. 

STM32 has been an absolute nightmare compared to ESP32. Just line after line doesn't work as expected and we need to go low level into a HAL libraries to figure out what's actually going. Really disappointed in our first STM32 project.

I've been working on a trivial I2S loopback, only to find out DMA doesn't even work property on the H7 and the first solution from ST is to "Disable the entire DCACHE".

[TChat]

Yes I am already allocating the buffer to D2 (0x30000000), which I think should be right. (and I haven't enabled the cache yet)

Out of frustration I hacked my board to give SAI a try and indeed there's life... Too bad I can't really use it.

Thanks for your help anyway!