search

STMicroelectronics / stm32h7xx_hal_driver

Provides the STM32Cube MCU Component "hal_driver" of the STM32H7 series.
BSD 3-Clause "New" or "Revised" License
86 stars 36 forks source link

fix the bug that the spi suspend flag is not cleared #48

Closed huoxingdawang closed 3 months ago

huoxingdawang commented 3 months ago

This PR fix the issue #47 .

The thing is that the SPI master in the rx only mode won't wait the CPU to get data from the fifo. So if a interrupt occured during the HAL_SPI_Receive, data will lose and the while (hspi->RxXferCount > 0UL) will nerver exit, which will then cause a timeout error.

There is actually a bit MASRX designed for the SPI mater to wait CPU, but after the CPU get data, the SUSP flag need to be cleared in order to start the receive according to the reference manual.

Without clearing the susp bit, the SPI master won't receive data anymore, which will also cause a timeout. Very much like the case without setting MASRX.

This PR fixed this bug by adding codes to clear the susp flag in HAL_SPI_Receive. After my testing, I think it works very well.

image

KRASTM commented 3 months ago

Hello @huoxingdawang,

It is no bug but there is a restriction when enabling the suspend mode. https://github.com/STMicroelectronics/stm32h7xx_hal_driver/blob/7a9b4eff63551204638c15f6b9faa0e1cfea219e/Src/stm32h7xx_hal_spi.c#L118-L121

For the moment the polling mode does not support this approach, maybe in the future.

I think you can use HAL_SPI_Receive_IT instead of HAL_SPI_Receive.

Please, allow me to close this PR.

With regards.

huoxingdawang commented 3 months ago

Hello @KRASTM,

Thanks for the test, but in my opinion, clearing the SUSP flag is a normal receive process. For the cases that all interrupts of the SPI is not enabled (the are all disabled in the SPI_CloseTransfer and not enabled again in the HAL_SPI_Receive), the condition "EOT interrupt is activated" will not be reached at all. So the SuspendCallback is not called, I think it matches what is said in the restriction.

Besides, in my opinion, it is a strange thing to call a callback function in a polling function.

Additionally, the current situation is that if other interrupts are enabled, HAL_SPI_Receive is not usabled at all !!! The data loss caused by other interrupts may occur at any time. This not only makes the MASRX flag completely invalid as it makes the SPI receiver pause forever, but also cause strange compatibility issues during the upgrade process just like what I mentioned in #47.

What makes this even worse is that the SUSP flag seems not to be cleared in SPI_CloseTransfer, this will cause the following usage of the SPI to be really strange. Sometimes I can detect SUSP flag even after transmit only.

In summary, I hope to review the need of this PR.

Thanks for the reviewing on this PR again.

With regards.

huoxingdawang commented 3 months ago

Hello @KRASTM ,

I saw a image So could I ask what I shoud do now, open a new PR or not? It seems that I can not reopen this PR :-(.

With regards.

KRASTM commented 3 months ago

Hello @huoxingdawang,

Sorry for the misunderstood, I meant by the (thumbs up) that I will check more your PR. As I Said, our Developpement team is aware of this enhancement. But unfortunately, we do not have a date yet.

For the moment, there is no need to keep the PR open. I will keep you informed if there are any update.

With regards.