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

[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.