Skip to content

Fix portSET_INTERRUPT_MASK_FROM_ISR definition for atomic operation #940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 3, 2024

Conversation

chinglee-iot
Copy link
Member

@chinglee-iot chinglee-iot commented Dec 28, 2023

Fix portSET_INTERRUPT_MASK_FROM_ISR definition for atomic operation

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:

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

@chinglee-iot chinglee-iot requested a review from a team as a code owner December 28, 2023 12:14
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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 Coverage Δ
unittests 93.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jefftenney
Copy link
Contributor

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

ActoryOu
ActoryOu previously approved these changes Jan 2, 2024
Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>
chinglee-iot and others added 4 commits January 3, 2024 10:59
Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>
Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>
Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>
Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]>
ActoryOu
ActoryOu previously approved these changes Jan 3, 2024
Copy link

sonarqubecloud bot commented Jan 3, 2024

Quality Gate Passed 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

@chinglee-iot chinglee-iot merged commit be880a1 into FreeRTOS:main Jan 3, 2024
@chinglee-iot chinglee-iot deleted the fix-atomic-define branch January 3, 2024 07:47
laroche pushed a commit to laroche/FreeRTOS-Kernel that referenced this pull request Apr 18, 2024
…unning again. Fixed a typo in a file. Added a section to the link file (FreeRTOS#940)

Co-authored-by: Soren Ptak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants