Issue if tick rollover occurs while waiting for flash

[LKett.1]

I note in the post (from July 31, 2020 at 8:33 AM) entitled "FLASH_WaitForLastOperation CubeMx error" (here: https://community.st.com/s/question/0D53W00000EDrjiSAD/flashwaitforlastoperation-returns-error-in-stm32g070), that there is a mention of an error in the `FLASH_WaitForLastOperation()` function.

I was looking through my current code, trying to fix an error and found that it is still there, at least in the `Drivers\STM32G0xx_HAL_Driver\Src\stm32g0xx_hal_flash.c` file, within in my current project.

In function `FLASH_WaitForLastOperation()`, the timeout is checked in a way that will fail if the value returned by `HAL_GetTick()` rolls over during the operations. The relevant lines of code are these:

HAL_StatusTypeDef FLASH_WaitForLastOperation(uint32_t Timeout)
{
...
  uint32_t timeout = HAL_GetTick() + Timeout;
...
  while ((FLASH->SR & error) != 0x00U)
  {
    if (HAL_GetTick() >= timeout)
    {
      return HAL_TIMEOUT;
    }
  }
...

I believe this should instead be written more like:

HAL_StatusTypeDef FLASH_WaitForLastOperation(uint32_t Timeout)
{
...
  uint32_t wait_start_ms = HAL_GetTick();
... 
  while ((FLASH->SR & error) != 0x00U)
  {
    if ((HAL_GetTick() - wait_start_ms) >= Timeout)
    {
      return HAL_TIMEOUT;
    }
  }
...

for it to not have a problem, if a rollover occurs just after line 4 (of the codes above), and the while loop exiting (when `(FLASH->SR & error)` becomes `0x00U`.

[Andrei Chichak]

For the casual reader, if you don't quite understand what's happening here, I wrote an article on this situation for the embedded.fm podcast.

https://embedded.fm/blog/2016/9/26/scheduling-code-on-bare-metal

[Tesla DeLorean]

Yeap, this is idiot code.

I thought we managed to get most of this out of the HAL libraries earlier on, but they are persistent..

[Pavel A.]

Opened issue on github

https://github.com/STMicroelectronics/stm32g0xx_hal_driver/issues/1

[Piranha]

Checked with CubeMX... For CubeF4 this flaw was fixed in v1.2.0 in June 2014. CubeG0 v1.0.0 was released on October 2018. But of course some genius in the HAL team found it or reinvented. And, using variables with a names "Timeout" and "timeout" in a single function, which differ only by the case of a single letter, is also a sign of a great "professional" behind the code.

[Piranha]

By the way, the article has an error, which was pointed out by the second comment. When the size of a tick type is smaller than the size of a (signed) int, because of integer promotion the subtraction and comparison is done with an int type and the code will fail because of the negative result at overflow. Therefore the result of the subtraction must be cast to the appropriate unsigned type and only then the comparison can be done.

Also looked at the code from the book (file page 156) mentioned in the article:

tTime TimePassed(tTime since) {
  tTime now = gSystemTicks;
  if (now >= since) {return (now - since);}
  // rollover has occurred
  return (now + (TIME_MAX-since));
}

Not only it is suboptimal, but it is also incorrect. For this implementation to return the correct passed time on overflow, the line 5 must be:

return (now + 1 + (TIME_MAX - since));

And mathematically we can see that with a limited unsigned type the 1+TIME_MAX rolls over to 0 and we are effectively left with just a now-since, which is the same code as for non-overflow case.

[LKett.1]

Thank you, everyone, That's reassuring.

Having checked the functions again, I note that `timeout` is set to `HAL_GetTick() + Timeout;` twice in the current code, and two operations waited for, each for up to `Timeout` milliseconds, so the whole function could last for (up to) around twice the length of `Timeout` milliseconds, which is not noted/implied by the function.

Should the value of `timeout` (or whatever that is replaced by, so it's not too similar to the function argument `Timeout`) not be set (to the MCU timer value from `HAL_GetTick()`) a second time, so that the total wait is around one `Timeout`, not two `Timeout`s?

I also note that the timeout check in the line I suggested:

  if ((HAL_GetTick() - wait_start_ms) >= Timeout)

does not protect against the integer promotion that Piranha mentioned, so it should probably be:

if ( (uint32_t)(HAL_GetTick() - wait_start_ms) >= Timeout )

or similar?

[Piranha]

On these 32-bit processors the int type is signed 32-bit. And for this piece of code the types of return value of HAL_GetTick() and wait_start_ms variable are uint32_t, which is "larger" (has a higher rank) than the int type on this platform. Therefore the calculation and the result will anyway be of an unsigned 32-bit type.

Conclusion - the cast of the result is necessary, but it will be a bug only when the rank of a time tick type is smaller than the size of an int type on the particular platform.

[fenugrec]

I've been using this which I believe is correct and should cover u16 and u32 ticks , regardless of sizeof(int) :

/** compare unsigned / u16 / u32 ticks */
#define TS_ELAPSED(cur, last, period) ((typeof(last))((cur) - (last)) >= (period))
 
e.g.
static uint16_t tick_last = ....
uint16_t now = get_tick();
if (TS_ELAPSED(now, tick_last, INTERVAL_MS)) { do_stuff(); ....}

[Piranha]

The typeof() specifier in a GCC specific extension. The newer C++ standards have decltype() specifier. But this is not really a problem.

typedef uint16_t tick_t;

Just define a basic dedicated type in a single place and use it everywhere you use those time ticks. That is exactly how the type definition is meant to be used! And that is also how a nicely designed generic RTOS, schedulers, timers etc. do it.