Awful timeout implementation in SDMMC

[Mikk Leini]

SDMMC HAL driver implementation is full of timeouts like this:

uint32_t count = SDMMC_CMDTIMEOUT * (SystemCoreClock / 8U / 1000U);
do
{
  if (count-- == 0U)
  {
    return SDMMC_ERROR_TIMEOUT;
  }
  sta_reg = SDMMCx->STA;
} while (SOME_FLAG_NOT_SET);

It may work in a single-threaded application on some basic CPU which runs fixed clocks per cycle, but it's not accurate on CPUs with dual-issue pipeline. All preempting interrupts and in case RTOS is used, all higher priority tasks, prolong this timeout. And vice versa - the longer this timeout gets the more it blocks lower priority tasks and interrupts. And the timeout isn't even in microseconds to justify it. SDMMC_CMDTIMEOUT value is 5000 milliseconds (by the comment). It's bad implementation by any standard.

It's been so for a long time in many STM32 series (F4, H7, H7RS, N6, etc.). It's reported at least 6 years ago: I may have encountered a bug in SDMMC_GetCmdResp1 ... - STMicroelectronics Community

I didn't check all the posts in forum, but I bet this implementation has caused problem for a lot of engineers.

How about fixing it? With HAL_GetTick() for start. Those while loops all over HAL checking for HAL_GetTick() aren't good for RTOS use either, but at least the timing would be correct.

Created second post about the general issue: HAL blocking lower priority tasks and interrupts - STMicroelectronics Community

[LCE]

Almost the worst possible solution for checking timeouts! :D

So many reasons for not using HAL - in most peripherals, there are only a few which are really "fast and clean".

[Ozone]

> I didn't check all the posts in forum, but I bet this implementation has caused problem for a lot of engineers.

You can bet.

Seeing this "feature" implementation was one of the reasons for me to forgo Cube/HAL at all.
As expected, this was not really a one-off.
I either use the old SPL, or bare metal code.

[Seb]

While being occasional coder as focusing more on hardware boards, one question: If the delays and timeouts were using the core debug cycle counter, would this be an improvement? (no pipeline, interrupt, compile optimisation/maker time "stretch"). This is what I do on I2C by IO bit bang with auto calibration, a code developped for board bringup with an intern here : Solved: NUCLEO-C5A3ZG STM32C5 QSPI Display Bring up Projec... - STMicroelectronics Community  Thanks!

[LCE]

I'd say that almost every other solution is better than while(count--).
The only reason to use this is ... lazyness! :D 

Only using hardware timers or the cycle counter can give accurate timings.

Especially for delays / timeouts much bigger than the SysTick it's kinda embarrassing to use while(count--).

[Mikk Leini]

In the past projects I have "worked it around" by placing all FreeRTOS tasks on the same priority so RTOS does time-slicing and nothing starves for too long. But in current project we need priorities and need to react reliably and fast to some events. I gave it a try and modified SDMMC driver to use HAL_GetTick() and implemented yielding function from my another post (HAL cooperative mode feature request - STMicroelectronics Community). It works. When I inject errors into SD bus, other same and lower priority tasks won't starve anymore and there is no snowball effect of failures like it was before.

TLDR this is the code:

do
{
  if ((HAL_GetTick() - tickstart) > SDMMC_CMDTIMEOUT)
  {
    return SDMMC_ERROR_TIMEOUT;
  }
  HAL_Yield();    
} while (!CONDITION);

/**
 * @brief Provides other tasks time to execute shortly (single RTOS tick)
 *        while waiting and polling HAL events.
 */
void HAL_Yield(void)
{
  /* Kernel running and call not from ISR ? */
  if ((osKernelGetState() == osKernelRunning) && (__get_IPSR() == 0U))
  {
    osDelay(1);
  }
}