STM32F303 CubeMX - LL_GPIO_TogglePin() - race condition?

[SKled.1]

__STATIC_INLINE void LL_GPIO_TogglePin(GPIO_TypeDef *GPIOx, uint32_t PinMask)
{
  WRITE_REG(GPIOx->ODR, READ_REG(GPIOx->ODR) ^ PinMask);
}

I was trying out a Nucleo F303RE board and installed CubeIDE and the HAL/LL library, then stumbled upon this.

That function, part of the LL API, first takes a temporary copy of the whole GPIO output register, *then* could be interrupted by an ISR or RTOS task which may change a certain pin bit on the same GPIO port, and then control returns back to above function, which then writes, directly into the ODR of the GPIO an outdated value of ODR xor'red with PinMask - oops, reverting the pin state change made by the ISR.

Should it not be more like this?

__STATIC_INLINE void LL_GPIO_TogglePin(GPIO_TypeDef *GPIOx, uint32_t PinMask)
{
	if (GPIOx->ODR & PinMask)
		GPIOx->BRR = PinMask;
	else GPIOx->BSRR = PinMask;
}

[TDK]

This comes up occasionally. Do a google search for "HAL_GPIO_TogglePin site:community.st.com". The second form you suggest also has issues in that if you try to toggle PA0 and PA1 at the same time, and they have different states, only one will be toggled. A better solution uses the fast that SET bits override RESET bits (or maybe the opposite, I'm not checking).

https://community.st.com/s/question/0D53W000000aTnT/bizarre-gpio-behaviour-halgpiotogglepin-doesnt-allow-toggle-multiple-pins

[SKled.1]

Okay, but the routine is called TogglePin, not TogglePins. So one would not rightfully expect it to handle more than one.

I'm aware the parameter is called PinMask, which could be tempting one to think it's for more than one, but it's certainly not a pin number or index, it's already shifted, which is expressed by that.

Well, I have done constructs before where I just always put the opposite bits in BRR than those put in BSRR with appropriate masking.

The solution in the thread you linked to is funny - I mean for me, because all those years, I was not aware the upper 16bit of BSRR actually do something :D

The reference manual says there: "Note: If both BSx and BRx are set, BSx has priority"

Such sneaky little notes. I guess that thread's variant is nicer as there is only 1 register write.

[TDK]

> Okay, but the routine is called TogglePin, not TogglePins. So one would not rightfully expect it to handle more than one.

I'm aware the parameter is called PinMask, which could be tempting one to think it's for more than one, but it's certainly not a pin number or index, it's already shifted, which is expressed by that.

Example code uses this to toggle several pins at once, and the description says it can be used on multiple pins at once. You can think it's wrong if you want. I'm not the one that needs convinced.

> The solution in the thread you linked to is funny

I agree, it's 10/10 for functionality but 5/10 for readability. Can't have it all.

[SKled.1]

True 🙂