Apparent minor bug in HAL_TIM_IRQHandler()

[JimYuill]

There appears to be a minor bug in HAL_TIM_IRQHandler(), in stm32f7xx_hal_tim.c

https://github.com/STMicroelectronics/STM32CubeF7/blob/master/Drivers/STM32F7xx_HAL_Driver/Src/stm32f7xx_hal_tim.c

APPARENT-BUG SUMMARY:

There's a line-of-code that works, but I think it's just by coincidence:

For a "TIM Update event", the TIMx_SR register's UIF bit is cleared using the mask for the TIMx_DIER register's UIE bit.

The mask for the TIMx_SR register's UIF bit should be used, instead.

Both masks are used to set bit 0, so the code works.

Using the wrong mask is confusing to the reader.

__HAL_TIM_CLEAR_IT() is used to clear the bit.

The file's other calls to __HAL_TIM_CLEAR_IT() may also have this same type of bug.

DESCRIPTION:

In the code section "TIM Update event" (shown below), this line:

__HAL_TIM_CLEAR_IT(htim, TIM_IT_UPDATE);

should be:

__HAL_TIM_CLEAR_IT(htim, TIM_FLAG_UPDATE);

These macros are defined in stm32f7xx_hal_tim.h:

#define TIM_IT_UPDATE           TIM_DIER_UIE             /*!< Update interrupt      */

#define TIM_FLAG_UPDATE          TIM_SR_UIF              /*!< Update interrupt flag     */

#define __HAL_TIM_CLEAR_IT(__HANDLE__, __INTERRUPT__)   ((__HANDLE__)->Instance->SR = ~(__INTERRUPT__))

The code is intended to clear the TIMx_SR register's field: Bit 0 UIF: Update interrupt flag.

So, TIM_FLAG_UPDATE should be used, not TIM_IT_UPDATE.

The file's other calls to __HAL_TIM_CLEAR_IT() may also have the same type of bug, e.g.,

__HAL_TIM_CLEAR_IT(htim, TIM_IT_CC1);

  /* TIM Update event */
  if (__HAL_TIM_GET_FLAG(htim, TIM_FLAG_UPDATE) != RESET)
  {
    if (__HAL_TIM_GET_IT_SOURCE(htim, TIM_IT_UPDATE) != RESET)
    {
      __HAL_TIM_CLEAR_IT(htim, TIM_IT_UPDATE);
#if (USE_HAL_TIM_REGISTER_CALLBACKS == 1)
      htim->PeriodElapsedCallback(htim);
#else
      HAL_TIM_PeriodElapsedCallback(htim);
#endif /* USE_HAL_TIM_REGISTER_CALLBACKS */
    }
  }

[Tesla DeLorean]

Flag is for the _FLAG macro, IT is for the _IT ones, the naming very intentional. The #define likely for convenience

The bit allocations between the registers has consistency at a design level, not by coincidence or happenstance.

[JimYuill]

Thanks for the info.

However, that doesn't seem to explain this:

TIM_IT_UPDATE is defined as TIM_DIER_UIE

This call, below, sets the SR register, and its UIF bit.

It doesn't set the the DIER register and its UIE bit.

__HAL_TIM_CLEAR_IT(htim, TIM_IT_UPDATE)

[Tesla DeLorean]

I understand your point, but there's quite a history to these things, and absent using enum's and type checking I'm not sure it's a big issue. IC design tends to be orthogonal, or coincidental by design..

The SR isn't supposed to be used in a RMW mode either, and there are known hazards with that.

Your concern could be addressed by changing the #define constant/inheritance, but that'd break

#define __HAL_TIM_ENABLE_IT(__HANDLE__, __INTERRUPT__)   ((__HANDLE__)->Instance->DIER |= (__INTERRUPT__))

I'd opt for keeping the APPLES to APPLES naming visible at the User level

In SPL implementations the defines often didn't have a 1:1 relationship with the register bit level, but convey register and bit

[JimYuill]

Thanks for the background info.

I'm new to HAL, but this still seems like a bug.

It's elementary programming practice for variable names to accurately reflect the variable's contents and use. That's not being done in this case.

The HAL functions have hardly any documentation, so it's necessary to read the code to figure-out what they do. So, good programming practices are especially needed.

You mentioned, "there's quite a history to these things".

If I correctly understood what you were saying there, I would think that having mistaken programming traditions doesn't justify not fixing those practices.

Also, expecting users to know HAL's undocumented tradition of mistaken practices is also not good practice.

Thanks,

[Piranha]

Looks like __HAL_TIM_CLEAR_FLAG() and __HAL_TIM_CLEAR_IT() does exactly the same thing. Also, looking at other macros, it is clear that the HAL_***_FLAG() ones operate on SR register, but the HAL_***_IT() ones on DIER register. As for disabling interrupts there is __HAL_TIM_DISABLE_IT(), the __HAL_TIM_CLEAR_IT(), which operates on SR register, should not exist at all. Therefore the correct implementation should be:

__HAL_TIM_CLEAR_FLAG(htim, TIM_FLAG_UPDATE);

And the same should be done to all other places, where __HAL_TIM_CLEAR_IT() is used. Exactly like it is already done for BREAK2 interrupt, which doesn't even have a separate enable bit in DIER register.