HAL_GPIO_TogglePin() bugs!

[zhiyongwoo]

Hi All,

I notice a bug in the HAL_GPIO_TogglePin() when there are two pins with different voltage level. E.g. GPIOF, Pin9 = 1 and Pin 10 = 0. It turns out Pin9 was 1 and Pin10 =0, then Pin9 and 10 = 0 then 1 and so on. Support it should be Pin9 != Pin10 but instead the Pin9=Pin10 and blink together.

Looking to the code, it determine 1 pin only instead of two.

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) == GPIO_Pin)
  {
    GPIOx->BSRR = (uint32_t)GPIO_Pin << GPIO_NUMBER;
  }
  else
  {
    GPIOx->BSRR = GPIO_Pin;
  }
}

Compare to the old StandardPeripheral code:

void GPIO_ToggleBits(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin)
{
  /* Check the parameters */
  assert_param(IS_GPIO_ALL_PERIPH(GPIOx));
 
  GPIOx->ODR ^= GPIO_Pin;
}

Does anyone spotted this bug before? It HAL codes looks like "kids" level kind of coding. The logic was not well thought.

[Danish1]

Looking at the name of the new function, I would think that you are only supposed to toggle a single pin at a time with HAL_GPIO_TogglePin().

And if that's what you ask for, would I be right in thinking that it behaves correctly?

Now IS_GPIO_PIN() doesn't prevent you from asking to toggle multiple bits, and I'd regard that as a programming error. Is that what you think?

So why have they changed the way it tries to work?

I think it is because the new version is better in a multi-threaded or interrupt-driven context.

With the old code, the ^= operation is done in three stages. The first reads the original value, the seconds calculates the desired modification and the third writes back the new value.

Suppose an interrupt (or another thread) occurs between steps 1 and 3, and it changes some other bits in ODR. Those changes will be lost when the modified original is written back. Using BSRR guarantees that the interrupt-time changes are retained.

(Of course it could happen that the interrupt/other-thread changes bits that are being toggled by HAL_GPIO_TogglePin; under those circumstances, who is to say what the desired outcome is).

How might things be written to allow you to toggle multiple bits simultaneously?

You might try something along the lines of (I haven't tried this).

uint32_t old = GPIOx->ODR;
GPIOx->BSRR = ((GPIO_Pin & old) << GPIO_NUMBER) | (GPIO_Pin & ~old);

Regards,

Danish

[waclawek.jan]

Danish,

> Looking at the name of the new function, I would think that you are only supposed to toggle a single pin

Well maybe, but the documentation says otherwise:

So, I say, yes, this *is* a bug, and ST should use your implementation, so keep to their own API and not to break existing code (plus fix the assertion accordingly).

JW

[zhiyongwoo]

Hi Danish and waclawek.jan

Thank you for the reply. Waclawek has pointed what I have missed out in my initial post. I went through the documentations/comments of the function and It convinced me that it is allow user to toggle multiple pins. The "assert_param" doesn't work at all in this case.

I feel very uncomfortable to use HAL lib. Please advice which (LL or HAL or StandardPeripheral) library to use for my new STM project?

PS: BTW, will ST see this "bug" and get it fixed?

Thank you.

[waclawek.jan]

I personally don't use any "library", only the symbols from the CMSIS-mandated device header (i.e. those from [Cube]\Drivers\CMSIS\Device\ST\[family]\Include\).

JW

[zhiyongwoo]

I am with you especially for those basic or straight forward configuration register. But what if you need to have HID/CDC library from ST? They are writing in HAL or LL only. Will you rewrite the code your own way?

[waclawek.jan]

Yes, that's what I do.

I don't say I don't use 3rd party software, but that's after very careful examination and deliberation. And the same amount of examination and deliberation goes into it whenever that 3rd party software changes, thus the very idea of "regular updates" is a substantial no-no.

In this regard, portions of Cube/HAL served to me only to augment the documentation. which especially for the purchased IPs (OTG, ETH) is of very poor quality.

JW

[Tesla DeLorean]

>> It HAL codes looks like "kids" level kind of coding. The logic was not well thought.

Yes, as I've previously observed, the coding and testing is often sophomoric. I think the earlier SPL was developed by an older and smaller HW/IC leaning crew.

[waclawek.jan]

This is 7 month old thread, and I don't know what was exactly the CubeF4 version at that time, but I've just looked into an older v1.21 CubeF4 dated december 2017 and lo and behold:

/**
  * @brief  Toggles the specified GPIO pins.
  * @param  GPIOx Where x can be (A..K) to select the GPIO peripheral for STM32F429X device or
  *                      x can be (A..I) to select the GPIO peripheral for STM32F40XX and STM32F427X devices.
  * @param  GPIO_Pin Specifies the pins to be toggled.
  * @retval None
  */
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;
}

Release note say (oh yes, versioning of Cube was never ST's forte):

V1.7.5 / 08-February-2019

[...]

HAL GPIO update

[...]

   Add missing define for SPI3 alternate function "GPIO_AF5_SPI3" for STM32F401VE devices

   Remove "GPIO_AF9_TIM14" from defined alternate function list for STM32F401xx devices

   HAL_GPIO_TogglePin() reentrancy robustness improvement

So this is a genuine Cube/HAL regression.

@Amel NASRI​  could you please bring this to Cube/HAL crew attention, suggesting Danish's solution from above?

Thanks,

JW

[Piranha]

Exactly the same issue and solution for LL:

https://community.st.com/s/question/0D50X0000BKAJoSSQX/issue-with-llgpiotogglepin-and-irqs