FreeRTOS wrong macro definition for portEND_SWITCHING_ISR()

[adam-szabo-xtalin]

Describe the set-up
STM32F4 FW1.28.2
CMSIS V1
FreeRTOS Kernel V10.3.1

Describe the bug
In Middlewares/Third_Party/FreeRTOS/Source/portable/GCC/ARM_CM4F/portmacro.h, line 92:

#define portEND_SWITCHING_ISR( xSwitchRequired ) if( xSwitchRequired != pdFALSE ) portYIELD()
#define portYIELD_FROM_ISR( x ) portEND_SWITCHING_ISR( x )

This macro is very unsafe to call, it can totally mess up a code.

Example1:
portYIELD_FROM_ISR( condition1 || condition2 );
Translates to:
if( condition1 || condition2 != pdFALSE ) portYIELD()
which is equal to
if( condition1 || ( condition2 != pdFALSE )) portYIELD()
which is not the intended behaviour, it should translate to:
if(( condition1 || condition2 ) != pdFALSE ) portYIELD()
Although the result of the two if() conditions is the same, it is a bad practice to make a "function-like" macro this way.

Example2:

if( condition1 )
  portYIELD_FROM_ISR( condition2 );
else
  foo();

Translates to:

if( condition1 )
  if( condition2 != pdFALSE )
    portYIELD();
else
  foo();

which is equal to

if( condition1 ) {
  if( condition2 != pdFALSE ) 
    portYIELD();
  else
    foo();
}

which is not the intended operation, it should translate to:

if( condition1 ) {
  if( condition2 != pdFALSE ) 
    portYIELD();
}
else {
  foo();
}

The correct macro is (additional parentheses around xSwitchRequired and do{} while(0) ):

#define portEND_SWITCHING_ISR( xSwitchRequired ) do{ if(( xSwitchRequired ) != pdFALSE ) portYIELD(); } while(0)
#define portYIELD_FROM_ISR( x ) portEND_SWITCHING_ISR( x )

How To Reproduce
There is no need to "reproduce", this is C syntax.
Please check all other macros in the generated code base, libraries. This kind of mistakes can cause a lot of trouble for the developers.

[ASEHSTM]

Hello @adam-szabo-xtalin,

Thank you for your report.
This problem has been resolved in FreeRTOS v10.6.2. You can access the updated version here: https://github.com/STMicroelectronics/stm32-mw-freertos/tree/master

Additionally, this fix will also be included in the the STM32CubeF4 package soon.

With Regards,

[adam-szabo-xtalin]

Thank you for your quick response. I checked portmacro.h file in the linked repository, and found the following line:

#define portEND_SWITCHING_ISR( xSwitchRequired )    do { if( xSwitchRequired != pdFALSE ) portYIELD(); } while( 0 )

The bigger problem is already fixed, which is very good news, but additional parentheses ( ) are still missing around xSwitchRequired in the if statement. There are a few operators | & ^ || && = ... that have lower precedence than != so it would be correct to add the parentheses.

#define portEND_SWITCHING_ISR( xSwitchRequired )    do { if( ( xSwitchRequired ) != pdFALSE ) portYIELD(); } while( 0 )

[ASEHSTM]

Hello @adam-szabo-xtalin,

Thank you for your suggestion. In this specific case, adding parentheses around xSwitchRequired is not necessary because it is treated as a single expression. The existing implementation already ensures correct behavior.

With Regards,