Description

In FreeRTOS.h, portSET_INTERRUPT_MASK_FROM_ISR is always defined to 0 if left undefined.

#define portSET_INTERRUPT_MASK_FROM_ISR()       0

This cause the following code in atomic.h always been evaluated to true by preprocessor.

#if defined( portSET_INTERRUPT_MASK_FROM_ISR )

/* Nested interrupt scheme is supported in this port. */
    #define ATOMIC_ENTER_CRITICAL() \
    UBaseType_t uxCriticalSectionType = portSET_INTERRUPT_MASK_FROM_ISR()

    #define ATOMIC_EXIT_CRITICAL() \
    portCLEAR_INTERRUPT_MASK_FROM_ISR( uxCriticalSectionType )

#else

/* Nested interrupt scheme is NOT supported in this port. */
    #define ATOMIC_ENTER_CRITICAL()    portENTER_CRITICAL()
    #define ATOMIC_EXIT_CRITICAL()     portEXIT_CRITICAL()

#endif /* portSET_INTERRUPT_MASK_FROM_ISR() */

In this PR

Set portHAS_NESTED_INTERRUPTS to 1 if portSET_INTERRUPT_MASK_FROM_ISR and portCLEAR_INTERRUPT_MASK_FROM_ISR are defined by the port.
Update the atomic.h to use #if ( portHAS_NESTED_INTERRUPTS == 1 ) instead.
Update the ports which don't support nested interrupts to remove the define of portSET/CLEAR_INTERRUPT_MASK_FROM_ISR macros. Port should not define these macros if nested interrupt is not supported.

Test Steps

Not define portSET/CLEAR_INTERRUPT_MASK_FROM_ISR in portmacro.h, the following defines are declared in atomic.h:

/* Nested interrupt scheme is NOT supported in this port. */
    #define ATOMIC_ENTER_CRITICAL()    portENTER_CRITICAL()
    #define ATOMIC_EXIT_CRITICAL()     portEXIT_CRITICAL()

Define portSET/CLEAR_INTERRUPT_MASK_FROM_ISR in portmacro.h, the following defines are declared in atomic.h:

/* Nested interrupt scheme is supported in this port. */
    #define ATOMIC_ENTER_CRITICAL() \
    UBaseType_t uxCriticalSectionType = portSET_INTERRUPT_MASK_FROM_ISR()

    #define ATOMIC_EXIT_CRITICAL() \
    portCLEAR_INTERRUPT_MASK_FROM_ISR( uxCriticalSectionType )

Checklist:

[x] 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.

Related Issue

936

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

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (75c4044) 93.78% compared to head (9ccf866) 93.78%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #940 +/- ## ======================================= Coverage 93.78% 93.78% ======================================= Files 6 6 Lines 3186 3186 Branches 885 885 ======================================= Hits 2988 2988 Misses 91 91 Partials 107 107 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/940/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/940/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.78% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jefftenney commented 8 months ago

@chinglee-iot This fix looks right to me. However, I noticed it introduces a "new" requirement for developers not to use the atomic interface from ISRs on ports that don't support nested interrupts. Since there is no need for ISRs to use the atomic interface on those ports, the "new" requirement seems harmless enough. Do you think some additional comments added to atomic.h would help provide clarity?

The atomic interface is intended for use outside of ISRs, on all ports, and also for use inside ISRs on ports that support nested interrupts. However, the atomic interface must not be used from ISRs on ports without support for nested interrupts. ISR code on these ports is already atomic.

Alternatively, the atomic interface could add _fromISR() equivalents to all existing functions and macros. But that would seem to undo what the original author apparently intended for atomic.h.

sonarcloud[bot] commented 7 months ago

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

FreeRTOS / FreeRTOS-Kernel

Fix portSET_INTERRUPT_MASK_FROM_ISR definition for atomic operation #940