[BUG] xEventGroupBitsFromISR API doesn't work correctly for RL78 MCU(same as No.740 Issue)

[KeitaKashima]

Describe the bug

I posted issue No.740 before but I could not share the reproduced code before.

Now I can create it and provide it in this new issue ticket.

#740

-- Detail of the previous posting.

Issue: xEventGroupBitsFromISR API is not working properly on RL78.

The correct procedure is "BRK execution -> stack save -> context switch -> stack load", but due to an implementation error, it seems that only the context switch is done.

Therefore, it cannot return to the original and cannot move.

FreeRTOS-Kernel/portable/GCC/RL78/portmacro.h

Line 109 in a5bf4d9

#define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do { if( xHigherPriorityTaskWoken ) vTaskSwitchContext(); } while( 0 )
Fault (current description)
#define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) if( xHigherPriorityTaskWoken ) vTaskSwitchContext()

Countermeasures (Fixed)

The following changes will fix the problem.

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

--
Target

• Development board: FPB-RL78/G23 https://www.renesas.com/products/microcontrollers-microprocessors/rl78-low-power-8-16-bit-mcus/rtk7rlg230csn000bj-rl78g23-128p-fast-prototyping-board-rl78g23-128p-fpb
• Instruction Set Architecture: RL78
• IDE and version: e2 studio v2024-04
• Toolchain and version: CC-RL v1.12.01

Line 109 in a5bf4d9

Host

• Host OS: Win10

To Reproduce

Please use the below Project and see the readme.

https://github.com/KeitaKashima/kernel_issue_740

Expected behavior
xEventGroupBitsFromISR API is not working properly on RL78.

It seems that portYIELD_FROM_ISR() does not work correctly.

Screenshots

Additional context

Hi team,

I am sorry that I could not share the project the issue happened in [Issue 740] (#740).

I created the project to reproduce the issue on RL78 MCU.

You can get the project of e2studio(eclipse base IDE) as below repo.

https://github.com/KeitaKashima/kernel_issue_740/tree/main

I use the CC-RL of Compiler and below env.

Test environment

• Kit : FPB-RL78/G23
• Compiler : CC-RL v1.12.01
• IDE : e2 studio v2024-04 (eclipse base IDE)

Could you check the behavior and ReadMe of URLs?

[chinglee-iot]

@KeitaKashima
Thank you for your information. I will look into these information and discuss with you here.

[chinglee-iot]

@KeitaKashima

Can you also reference "Writing interrupt service routines" in this guide and update your ISR handler? The guide suggests to wrapping you ISRHandler with portSAVE_CONTEXT and portRESTORE_CONTEXT if a context switch is requested in the ISR.

Compared to the change suggested in this issue, context switch is handled in the ISR instead of an extra BRK software interrupt, which saves the time for handling BRK software interrupt. If this method works, we would like to suggest you adapt this guide in your design.

[KeitaKashima]

@chinglee-iot

I read the guide and I can understand what you want. I will check the code whether it works fine.
However, it requires putting the wrapper routine for every ISR... This has a big impact on our current platforms.

So I want to fix the issue with an only few modifications.

Below are the ideas for fixing this issue. It adds the portSAVE_CONTEXT and portRESTORE_CONTEXT in the portYIELD_FROM_ISR macro.
Is that acceptable for you?

#define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do{ if( xHigherPriorityTaskWoken ) {vPortYield();} } while( 0 )

Originally I referred to the below implementation about my fixing code when I created the fixing code of this ticket.

FreeRTOS-Kernel/portable/Renesas/RX600v2/portmacro.h

Line 104 in dbf7055

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

[chinglee-iot]

@KeitaKashima
Please share with us the result if this method works on your platform. The wrapper routine is only required if a context switch is requested in your ISR. Can you elaborate on the impact to your platforms for us? Then we can further discuss the impact.

[KeitaKashima]

@chinglee-iot

I checked your proposal workaround and it can solve this issue as well.

Can you elaborate on the impact of your platforms on us? Then we can further discuss the impact.

I think the workaround you proposed needs a Wrapper ISR function for the contextSwitch interrupts.
Our MCU drivers provides the ISR routine without the Wrapper routine as the driver layer.
We want to avoid that change to the base design.
If we use this Wrapper ISR workaround, we should redefine and provide new drivers.
But the way that I proposed in this ticket can keep current ISR function styles, then the users don't have any effect on their code.
So I would like you to allow to use my proposal workaround as we can change only the porting layer of FreeRTOS.

[aggarg]

The earlier ISR implementation would look like the following:

/* Assembly Code. */
vISRWrapper:
    portSAVE_CONTEXT
    call vISRHandler
    portRESTORE_CONTEXT
    reti

/* C Code. */
void vISRHandler( void )
{
    BaseType_t xHigherPriorityTaskWoken = pdFALSE;

    xSemaphoreGiveFromISR( xSemaphore, &( xHigherPriorityTaskWoken ) );

    /* Expended portYIELD_FROM_ISR. */
    if( xHigherPriorityTaskWoken != pdFALSE )
    {
        vTaskSwitchContext();
    }
}

The new code does not require assembly wrapper and therefore, ISR implementation would look like the following:

/* C Code. */
void vISRHandler( void )
{
    BaseType_t xHigherPriorityTaskWoken = pdFALSE;

    xSemaphoreGiveFromISR( xSemaphore, &( xHigherPriorityTaskWoken ) );

    /* Expended portYIELD_FROM_ISR. */
    if( xHigherPriorityTaskWoken != pdFALSE )
    {
        vPortYield(); /* BRK --> Context save --> Context switch --> Context restore. */
    }
}

If an old application starts using the updated port, their ISR implementation would be like -

/* Assembly Code. */
vISRWrapper:
    portSAVE_CONTEXT
    call vISRHandler
    portRESTORE_CONTEXT
    reti

/* C Code. */
void vISRHandler( void )
{
    BaseType_t xHigherPriorityTaskWoken = pdFALSE;

    xSemaphoreGiveFromISR( xSemaphore, &( xHigherPriorityTaskWoken ) );

    /* Expended portYIELD_FROM_ISR. */
    if( xHigherPriorityTaskWoken != pdFALSE )
    {
        vPortYield(); /* BRK --> Context save --> Context switch --> Context restore. */
    }
}

In this case, there will be 2 pairs of context save and restore - one in the assembly wrapper and other as a result of vPortYield.

Would you please confirm that this would just be a performance penalty and would be functionally correct.

If yes then we can go ahead with your suggestion. It may be good to include a conditional check which an existing application can use if they do not want to pay the performance penalty:

#ifndef portREQUIRE_ASM_ISR_WRAPPER
    #define portREQUIRE_ASM_ISR_WRAPPER 0
#endif

#if( portREQUIRE_ASM_ISR_WRAPPER == 1 )
    #define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do { if( xHigherPriorityTaskWoken != pdFALSE ) vTaskSwitchContext(); } while( 0 )
#else
    #define portYIELD_FROM_ISR( x ) do { if( x ! = pdFALSE ) portYIELD(); } while( 0 )
#endif

What do you think?

[KeitaKashima]

@aggarg
My team checked the behavior in the above two cases.

Case 1) It works fine without a Wrapper(portREQUIRE_ASM_ISR_WRAPPER = 0 and without a vISRWrapper routine).
Case 2) with a Wrapper of 2 pairs of context save and restore (portREQUIRE_ASM_ISR_WRAPPER = 1 and with vISRWrapper routine), it doesn't work fine... It seems that the CPU was out of control...

Now we analyze why the CPU was out of control when running Case 2.

[aggarg]

Thank you for investigating and sharing your findings. Let us know whatever you find. If you determine that it won't work, one other option is to provide a new wrapper which can be used by the new applications and you can use that in your driver ISRs -

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

What do you think?

[KeitaKashima]

@aggarg

We eventually found the cause of the issue NOT running the Application correctly when enable both of Context save/restore at vISRWrapper and portYIELD_FROM_ISR().

The cause is to restore the AX register of SP without updating the currentTCB.
The SP is restored to the same value that is stored a second time when it is expected to get the value of SP stored the first time.

I mean the SP value is restored the same value of (3) and (4) because currentTCB doesn't update between (3) and (4) of the below progress.

The progress is below order. 
 portSAVE_CONTEXT :(1)
 portSAVE_CONTEXT :(2)
 portRESTORE_CONTEXT :(3)
 portRESTORE_CONTEXT :(4)   <= This is the issue of reading the wrong SP value from the current TCB.


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

vISRWrapper:
    portSAVE_CONTEXT    ;  **(1)**
    call vISRHandler
    portRESTORE_CONTEXT ;  **(4)**
    reti
 
vISRHandler(void)
{
    if( xHigherPriorityTaskWoken != pdFALSE )
    {
        portYIELD_FROM_ISR(); /* BRK --> Context save**(2)** --> Context switch --> Context restore**(3)**. */
    }
}

L91 is restore the AX in portRESTORE_CONTEXT , but it seems that it cannot do the nested called...

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/GCC/RL78/isr_support.h#L91

Operation:
MOVW AX, !_pxCurrentTCB <= This _pxCurrentTCB is not updated when reading the nested context save/restore.

[aggarg]

@KeitaKashima Thank you for the detailed investigation and a very clear explanation. I understand the issue. What do you propose about solving this? What do you think about having a separate YIELD_FROM_ISR and the application can decide which one to use based on if they have an ASM wrapper or not -

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

[KeitaKashima]

@aggarg , Thank you for your reply. I talked with my team members about this.

I would like to select the solution you proposed before.(here).

It works fine when the User doesn't execute the Nested context save/resume.

My recommend proposal(Same as your proposal)

ISR wrapper and routine

#if portREQUIRE_ASM_ISR_WRAPPER == 1
/* Assembly Code. */
vISRWrapper:
    portSAVE_CONTEXT
    call vISRHandler
    portRESTORE_CONTEXT
    reti
#endif /* #if portREQUIRE_ASM_ISR_WRAPPER == 1 */

/* C Code. */
void vISRHandler( void )
{
    BaseType_t xHigherPriorityTaskWoken = pdFALSE;

    xSemaphoreGiveFromISR( xSemaphore, &( xHigherPriorityTaskWoken ) );

    /* Expended portYIELD_FROM_ISR. */
    if( xHigherPriorityTaskWoken != pdFALSE )
    {
        vPortYield(); /* BRK --> Context save --> Context switch --> Context restore. */
    }
}

portmacro.h file

/* portmacro.h file */

#ifndef portREQUIRE_ASM_ISR_WRAPPER
    #define portREQUIRE_ASM_ISR_WRAPPER 0
#endif

#if( portREQUIRE_ASM_ISR_WRAPPER == 1 )
    #define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do { if( xHigherPriorityTaskWoken != pdFALSE ) vTaskSwitchContext(); } while( 0 )
#else
    #define portYIELD_FROM_ISR( x ) do { if( x ! = pdFALSE ) portYIELD(); } while( 0 )
#endif

About using #define portYIELD_FROM_ISR_NO_ASM_WRAPPER( x ) do { if( x ! = pdFALSE ) portYIELD(); } while( 0 )

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

This will work fine but this way affects current Users who already use RL78's FreeRTOS platform because it using portYIELD_FROM_ISR() in the ISR routine.

So I think it is not better way.

[aggarg]

So you want to have a config configREQUIRE_ASM_ISR_WRAPPER which defaults to 0 and then define portYIELD_FROM_ISR like the following -

#if( configREQUIRE_ASM_ISR_WRAPPER == 1 )
    #define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do { if( xHigherPriorityTaskWoken != pdFALSE ) vTaskSwitchContext(); } while( 0 )
#else
    #define portYIELD_FROM_ISR( xHigherPriorityTaskWoken ) do { if( xHigherPriorityTaskWoken != pdFALSE ) portYIELD(); } while( 0 )
#endif

This seems good to me as it is backward compatible. The existing applications continue to work without requiring any change. When an application wants to get rid of ASM wrappers, it can set configREQUIRE_ASM_ISR_WRAPPER to 1 in their FreeRTOSConfig.h and then remove ASM wrappers.

Would you like to raise a PR for this?