search

FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.65k stars 1.1k forks source link

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

Open KeitaKashima opened 1 month ago

KeitaKashima commented 1 month ago

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.

https://github.com/FreeRTOS/FreeRTOS-Kernel/issues/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

Line 109 in a5bf4d9

Host

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

image

Additional context

Hi team,

I am sorry that I could not share the project the issue happened in [Issue 740] (https://github.com/FreeRTOS/FreeRTOS-Kernel/issues/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

Could you check the behavior and ReadMe of URLs?

chinglee-iot commented 1 month ago

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

chinglee-iot commented 1 month ago

@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 commented 1 month ago

@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.

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/dbf70559b27d39c1fdb68dfb9a32140b6a6777a0/portable/Renesas/RX600v2/portmacro.h#L104

chinglee-iot commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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?