HAL_GPIO_TogglePin() not thread-safe

[David Littell]

Posted on May 30, 2018 at 21:31

STM32Cube_FW_F7_V1.11.0's HAL_GPIO_TogglePin() appears broken in a multi-threaded environment:

void HAL_GPIO_TogglePin(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin)

{

  /* Check the parameters */

  assert_param(IS_GPIO_PIN(GPIO_Pin));

   GPIOx->ODR ^= GPIO_Pin;

}

One fix might be:

void HAL_GPIO_TogglePin(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin)

{

  /* Check the parameters */

  assert_param(IS_GPIO_PIN(GPIO_Pin));

  if( (GPIOx->ODR & GPIO_Pin) != 0x0 )

    {

      GPIOx->BSRR = ((uint32_t) GPIO_Pin) << 16;   /* Set a BSRR.BRx bit so the GPIO goes low. */

    }

  else   /* the GPIO should be set high... */

    {

      GPIOx->BSRR = (uint32_t) GPIO_Pin;   /* Set a BSRR.BSx bit so the GPIO goes high. */

    }

}

I suspect all the HAL's may suffer from this...

[Tilen MAJERLE]

Posted on May 30, 2018 at 21:35

Hello,

method you provided is not thread safe aswell. The only thread safe option is to use mutex/sem before calling TogglePin function. Hardware does not support toggling with single instruction like set/reset operation with BSRR register.

Best regards,

Tilen

[waclawek.jan]

Posted on May 30, 2018 at 21:44

Maybe David's solution is not thread safe in an absolute meaning of the word, as two threads manipulating the same bit may get into conflict. But, isn't that a bit pathologic scenario?

OTOH, the original solution is plain wrong, as it will influence other bits in the same GPIO port.

JW

[Tilen MAJERLE]

Posted on May 30, 2018 at 21:55

Hello

Waclawek.Jan

‌,

I do agree with you that itis better, but it is technically and theoretically still wrong. It may happen you have wrong output levels.

Best regards,

Tilen

[waclawek.jan]

Posted on May 30, 2018 at 22:09

Again, who in his right mind would toggle an output bit from two threads, and then expect 'correct' output?

JW

[EDIT]

I put it in an unnecessarily confrontative manner, and I apologize. Allow me to rephrase.

While David's solution is still theoretically not perfect, would you agree that the existing one is practically dangerous, and suggest the Cube team to make the change?

If needed, an accompanying comment can be made in the documentation, explaining the theoretical dangers.

Thanks,

Jan

[Garnett.Robert]

Hi,

If you have code that must complete uninterrupted shouldn't you put it in a critical section, i.e. interrupts disabled?

Toggling a pin from two different threads seems an odd thing to do. I can imagine having one thread turning the pin on and another turning it off and vice versa, but toggling?

Regards

Rob