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.71k stars 1.11k forks source link

Make configSUPPORT_STATIC_ALLOCATION==1 an error for MPU ports #953

Closed IsaacDynamo closed 8 months ago

IsaacDynamo commented 8 months ago

Make configSUPPORT_STATIC_ALLOCATION==1 an error for MPU ports

Description

The vApplicationGet*TaskMemory() functions are not generated for MPU ports even when configSUPPORT_STATIC_ALLOCATION is set. This PR makes setting configSUPPORT_STATIC_ALLOCATION for MPU ports an error. Copied explanation from https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/790#discussion_r1329997898

Checklist:

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

Skptak commented 8 months ago

Hey there and thanks for raising this PR! Setting configSUPPORT_STATIC_ALLOCATION to 1 when using an MPU port is allowed, the end user just needs to supply the vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory functions. There is a statically allocated MPU demo that shows how to do this at FreeRTOS/Demo/CORTEX_MPU_Static_Simulator_Keil_GCC/main.c.

However! As you've pointed out, using an MPU port with configKERNEL_PROVIDED_STATIC_MEMORY set to 1 could lead to data aborts, or undefined behaviour, as the stacks and TCBs provided are not MPU Region aligned. As such I would remove the check for ( configSUPPORT_STATIC_ALLOCATION == 1 ) from your config check, as it is not the actual problem with the kernel provided functions.

Skptak commented 8 months ago

/bot run formatting

IsaacDynamo commented 8 months ago

Hey there and thanks for raising this PR! Setting configSUPPORT_STATIC_ALLOCATION to 1 when using an MPU port is allowed, the end user just needs to supply the vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory functions. There is a statically allocated MPU demo that shows how to do this at FreeRTOS/Demo/CORTEX_MPU_Static_Simulator_Keil_GCC/main.c.

I agree.

However! As you've pointed out, using an MPU port with configKERNEL_PROVIDED_STATIC_MEMORY set to 1 could lead to data aborts, or undefined behaviour, as the stacks and TCBs provided are not MPU Region aligned.

This should not happen because the generated default implementations are already conditional on #if ( ( configSUPPORT_STATIC_ALLOCATION == 1 ) && ( configKERNEL_PROVIDED_STATIC_MEMORY == 1 ) && ( portUSING_MPU_WRAPPERS == 0 ) ), source and source.

This PR only add a compile error when the request for default implementations cannot be fulfilled. This way the user gets a descriptive compiler error, instead of an unhelpful "undefined symbol" link error without context.

As such I would remove the check for ( configSUPPORT_STATIC_ALLOCATION == 1 ) from your config check, as it is not the actual problem with the kernel provided functions.

I think this would make the check unnecessarily strict. When configSUPPORT_STATIC_ALLOCATION == 0 the vApplicationGet*TaskMemory() functions are not needed, so the "misconfiguration" is IMHO harmless.

On the other hand any configuration with configSUPPORT_STATIC_ALLOCATION == 0 and configKERNEL_PROVIDED_STATIC_MEMORY == 1 is illogical and should perhaps be reported. But this would be applicable to both MPU and non-MPU ports. So it should be a seperate check with its own error message.

sonarcloud[bot] commented 8 months ago

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

codecov[bot] commented 8 months ago

Codecov Report

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

Comparison is base (c083af9) 93.42% compared to head (97e3698) 93.42%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #953 +/- ## ======================================= Coverage 93.42% 93.42% ======================================= Files 6 6 Lines 3195 3195 Branches 887 887 ======================================= Hits 2985 2985 Misses 103 103 Partials 107 107 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/953/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/953/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.42% <ø> (ø)` | | 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.

Skptak commented 8 months ago

Hey @IsaacDynamo, thanks for your well written explanation! I had completely missed that those functions were wrapped with portUSING_MPU_WRAPPERS == 0, thanks for catching that!

This PR only add a compile error when the request for default implementations cannot be fulfilled. This way the user gets a descriptive compiler error, instead of an unhelpful "undefined symbol" link error without context.

Where I now totally understand what the goal of this PR is now, thanks!