Revert "Regression Bug Fix: Fix Incorrect Return of MSVC-MingW portYIELD_FROM_ISR (#1207)"

[bknicholls]

Description

PR #1207 reverted portYieldFromISR to an earlier implementation that did a return. That has broken the demo and real world code on main as calling portYieldFromISR does a return where it isn't expected.

The description of the PR says the reason is that prvProcessSimulatedInterrupts calls portYIELD_FROM_ISR but that's not the case in current main.

Returning from portYieldFromISR doesn't match the behaviour of the other ports as they only act on the value and don't return a result.

It looks like the revert and tests were done at an earlier commit before current main was merged in the PR.

Test Steps

Build the demo project in Visual Studio and the build will succeed with a warning.

Build the demo project in Eclipse and it is treated as a build failure.

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[aggarg]

Which demo are you talking about? The signature of an interrupt handler on Windows port is supposed to be uint32_t Handler( void ) which needs to be installed using vPortSetInterruptHandler - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/MSVC-MingW/portmacro.h#L219.

The description of the PR says the reason is that prvProcessSimulatedInterrupts calls portYIELD_FROM_ISR but that's not the case in current main.

prvProcessSimulatedInterrupts relies on the return value of the interrupt handler and therefore, the interrupt handler is required to return the correct value - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/MSVC-MingW/port.c#L424.

[bknicholls]

These are the three demos in the base FreeRTOS repo.
https://github.com/FreeRTOS/FreeRTOS/tree/2dcc47ecbab427a7800682713e3f06ae5bdd9ca4/FreeRTOS/Demo/WIN32-MSVC-Static-Allocation-Only
https://github.com/FreeRTOS/FreeRTOS/tree/2dcc47ecbab427a7800682713e3f06ae5bdd9ca4/FreeRTOS/Demo/WIN32-MSVC
https://github.com/FreeRTOS/FreeRTOS/tree/2dcc47ecbab427a7800682713e3f06ae5bdd9ca4/FreeRTOS/Demo/WIN32-MingW

Sorry I see what you're talking about now. It's only just clicked for me the "port" part in the name. It's a bit of a pain as the reason I'm using the Windows simulator is to test application code off my device but it seems this implementation of portYIELD_FROM_ISR is different to all the others.

How do you deal with this in your code?

[bknicholls]

I've come up with a concept using thread local storage to pass back the result instead of using a return. Is this something you'd consider?

main...vypex-software:FreeRTOS-Kernel:windows-yield-isr

[aggarg]

I'm using the Windows simulator is to test application code off my device but it seems this implementation of portYIELD_FROM_ISR is different to all the others.

The issue is not the implementation of the portYIELD_FROM_ISR but the different signature of the ISR. Since the interrupt installation is dependent on the platform by its very nature, how about doing something like the following:

/* Common code. */
BaseType_t CommonInterruptHandler( void )
{
    BaseType_t xHighPriorityTaskWoken = pdFALSE;

    /* Handle interrupt and potentially call FreeRTOS FromISR APIs. */

    return xHighPriorityTaskWoken;
}

/* Device code. */
void DeviceInterruptHandler( void )
{
    BaseType_t xHighPriorityTaskWoken = pdFALSE;

    xHighPriorityTaskWoken = CommonInterruptHandler();

    portYIELD_FROM_ISR( xHighPriorityTaskWoken );
}

/* WinSim code. */
uint32_t WinSimInterruptHandler( void )
{
    BaseType_t xHighPriorityTaskWoken = pdFALSE;

    xHighPriorityTaskWoken = CommonInterruptHandler();

    portYIELD_FROM_ISR( xHighPriorityTaskWoken );
}

Alternatively, if you want to have compile time flags, you can do something like the following:

#ifdef WINSIM
    uint32_t InterruptHandler( void )
#else
    void InterruptHandler( void )
#endif
    {
        BaseType_t xHighPriorityTaskWoken = pdFALSE;

        /* Handle interrupt and potentially call FreeRTOS FromISR APIs. */

        portYIELD_FROM_ISR( xHighPriorityTaskWoken );
    }

[bknicholls]

The issue is not the implementation of the portYIELD_FROM_ISR but the different signature of the ISR. Since the interrupt installation is dependent on the platform by its very nature, how about doing something like the following:

Got it. Makes sense that installation varies by platform. In my simulator I was using that function signature and not the yield when registering my fake interrupts for hardware I have simulated so they are fine, but other parts of the software that I haven't included in the simulation yet get broken because of the return in portYIELD_FROM_ISR means the enclosing function signature needs to change too. This looks like what's happening in the demo as well.

Since the Windows simulator as far as I can see is the only version that handles portYIELD_FROM_ISR by doing a return it would be nice if there is a way it can align with the other ports.

[bknicholls]

I'll close this since it doesn't fit the current implementation of the Windows port interrupts. Is there another place we can continue the discussion?

[aggarg]

Is there another place we can continue the discussion?

Yes, please use FreeRTOS forums - https://forums.freertos.org/