Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory #790

mdnrz · 2023-09-10T17:51:09Z

Description

This PR introduces configKERNEL_PROVIDED_STATIC_MEMORY option which the application can set to 1 to use the default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory functions.

If the application enables static allocation (i.e. sets configUSE_STATIC_ALLOCATION to 1) and does not provide the above 2
functions, it will result in linker error. The application has two options:

Set configKERNEL_PROVIDED_STATIC_MEMORY to 1 to use the default implementations of these functions.
Provide implementations of these 2 functions.

Note that default definitions are only available for non-MPU ports. The reason is that the stack alignment requirements vary for different architectures.

Test Steps

Build a FreeRTOS project and set configUSE_STATIC_ALLOCATION to 1. Observe linker error for missing symbols vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory.

Set configKERNEL_PROVIDED_STATIC_MEMORY to 1. The linker error goes away and the default implementations in the tasks.c are used.
Set configKERNEL_PROVIDED_STATIC_MEMORY to 0 and provide implementation of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory. The linker error goes away and the default implementations are used.

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

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

vApplicationGetTimerTaskMemory Define __weak funtions to allocate idle-task and timer-task memories statically to avoid undefined reference errors when compiling with configSUPPORT_STATIC_ALLOCATION flag.

rawalexe · 2023-09-10T22:26:37Z

Thank you so much for your submission, I'll pass this along to the team so one can review the PR.

codecov · 2023-09-10T22:33:30Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (c3ece08) 94.35% compared to head (ce181f2) 94.35%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #790   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files           6        6           
  Lines        2443     2443           
  Branches      598      598           
=======================================
  Hits         2305     2305           
  Misses         85       85           
  Partials       53       53

Flag	Coverage Δ
unittests	`94.35% <ø> (ø)`

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

Files Changed	Coverage Δ
tasks.c	`94.81% <ø> (ø)`

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

htibosch · 2023-09-11T02:41:02Z

tasks.c

+
+#if (configSUPPORT_STATIC_ALLOCATION == 1)
+
+__attribute__((__weak__)) void vApplicationGetIdleTaskMemory (StaticTask_t **ppxIdleTaskTCBBuffer,


It looks good to me, except that Visual C doesn't recognise __attribute__((__weak__)).
Visual C is used to compile and run demos for both FreeRTOS and for +TCP.
You might get it working when using #pragma WEAK in case WIN32 is defined.

Thanks for your suggestion, but as FreeRTOS is used with many different compilers, compiler specific syntax can only appear in the port layer (i.e. in the C files that are only built with which ever compiler's syntax is used).

Thanks for the reviews. So should it be added to GCC port directory?
There exists a /portable/ThirdParty/GCC/RP2040/idle_task_static_memory.c, but it doesn't have weak attribute and also shouldn't it be made visible using extern ?

I do not think that would be a good place either because we will need to have the same code for all the supported compilers. I think it is best handled in the documentation, which we have done here - https://www.freertos.org/a00110.html#configSUPPORT_STATIC_ALLOCATION.

There exists a /portable/ThirdParty/GCC/RP2040/idle_task_static_memory.c

That is a third party port and not from us.

shouldn't it be made visible using extern ?

Unless explicitly marked static, a function is by default visible.

RichardBarry · 2023-09-11T04:29:04Z

tasks.c

+
+#if (configSUPPORT_STATIC_ALLOCATION == 1)
+
+__attribute__((__weak__)) void vApplicationGetIdleTaskMemory (StaticTask_t **ppxIdleTaskTCBBuffer,


Thanks for your suggestion, but as FreeRTOS is used with many different compilers, compiler specific syntax can only appear in the port layer (i.e. in the C files that are only built with which ever compiler's syntax is used).

paulbartell · 2023-09-11T20:32:08Z

@RichardBarry and @aggarg Would the following be an acceptable approach?

define a new port-specific macro for compilers which support weak functions.. something like

#define portWEAK_SYMBOL __attribute__((__weak__))

define the weak functions vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory if and only if portWEAK_SYMBOL is defined using portWEAK_SYMBOL instead of the compiler-specific __attribute__((__weak__)) spec?

Introduce configKERNEL_PROVIDED_STATIC_MEMORY option which the application can set to 1 to use the default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory functions. If an application enables static allocation (i.e. sets configUSE_STATIC_ALLOCATION to 1) and does not provide the above 2 functions, it will result in linker error. The application has two options: 1. Set configKERNEL_PROVIDED_STATIC_MEMORY to 1 to use the default implementations of these functions. 2. Provide implementations of these 2 functions. Signed-off-by: Gaurav Aggarwal <[email protected]>

Signed-off-by: Gaurav Aggarwal <[email protected]>

tasks.c

include/FreeRTOS.h

Skptak · 2023-09-18T16:57:28Z

tasks.c

+                                        uint32_t * pulIdleTaskStackSize )
+    {
+        static StaticTask_t xIdleTaskTCB;
+        static StackType_t uxIdleTaskStack[ configMINIMAL_STACK_SIZE ];


Would it make sense to align these to the region size for MPU ports?
It becomes wasted memory on the ArmV8-M MPU ports, as they only require 32 byte alignment, but it would make this usable for the ArmV7-M MPU ports, as they require MPU region alignment to the size of the region.
I think doing this would allow these functions to be used by all of our ports

These have been made part of privileged data and that section is aligned using linker scripts. There is no need of any explicit alignment.

Well the task stack will be turned into an MPU region, so if that's not aligned correctly it can lead to issues depending on the architecture.
I know on the ArmV7-R that having an unaligned MPU region can lead to "Unpredictable Behaviour"
And it looks like on the ArmV7-M that "Any unaligned access to Device or Strongly Ordered memory generates an alignment UsageFault".

Where right now the portPRIVILEGED_RAM_REGION MPU region over-writes the portSTACK_REGION, where I don't think this is an issue, as you stated.

However, if we ever set portSTACK_REGION to have a higher priority than portPRIVILEGED_RAM_REGION, which would allow dynamic creation of unprivileged tasks, this could potentially be a pretty tough issue to debug.

All that said, I don't think this is something worth holding up this PR over.

Yes, you are right. Stack memory needs to satisfy alignment requirements as we use an MPU region to guard that. I think it is best to supply these default implementations for non-MPU ports as alignment requirements very with the architecture.

Signed-off-by: Gaurav Aggarwal <[email protected]>

Add __weak definitions for vApplicationGetIdleTaskMemory and

0d9fdd2

vApplicationGetTimerTaskMemory Define __weak funtions to allocate idle-task and timer-task memories statically to avoid undefined reference errors when compiling with configSUPPORT_STATIC_ALLOCATION flag.

mdnrz requested a review from a team as a code owner September 10, 2023 17:51

htibosch reviewed Sep 11, 2023

View reviewed changes

RichardBarry requested changes Sep 11, 2023

View reviewed changes

Fix formatting in tasks.c

8fbc647

mdnrz requested review from htibosch and RichardBarry September 11, 2023 05:55

aggarg and others added 4 commits September 18, 2023 12:01

Fix formatting

7d273c9

Signed-off-by: Gaurav Aggarwal <[email protected]>

Fix formatting

272e917

Signed-off-by: Gaurav Aggarwal <[email protected]>

Merge branch 'main' into mehdi

8ff0c61

aggarg changed the title ~~Add __weak definitions for vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory~~ Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory Sep 18, 2023

aggarg previously approved these changes Sep 18, 2023

View reviewed changes

Skptak reviewed Sep 18, 2023

View reviewed changes

tasks.c Show resolved Hide resolved

Skptak reviewed Sep 18, 2023

View reviewed changes

tasks.c Show resolved Hide resolved

Skptak reviewed Sep 18, 2023

View reviewed changes

include/FreeRTOS.h Show resolved Hide resolved

Skptak reviewed Sep 18, 2023

View reviewed changes

Tag idle TCBs and Stacks as privileged data

fb0e81d

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg dismissed their stale review via fb0e81d September 18, 2023 17:00

aggarg previously approved these changes Sep 18, 2023

View reviewed changes

aggarg and others added 2 commits September 19, 2023 09:07

Merge branch 'main' into mehdi

0e47601

Make default implementations for non-MPU ports only

dbee798

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg dismissed their stale review via dbee798 September 19, 2023 12:20

aggarg approved these changes Sep 19, 2023

View reviewed changes

RichardBarry approved these changes Sep 19, 2023

View reviewed changes

shubnil approved these changes Sep 20, 2023

View reviewed changes

Merge branch 'main' into mehdi

ce181f2

aggarg merged commit 7cd201c into FreeRTOS:main Sep 20, 2023
27 of 28 checks passed

IsaacDynamo mentioned this pull request Jan 14, 2024

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

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory #790

Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory #790

mdnrz commented Sep 10, 2023 •

edited by aggarg

Loading

rawalexe commented Sep 10, 2023

codecov bot commented Sep 10, 2023 •

edited

Loading

htibosch Sep 11, 2023 •

edited

Loading

RichardBarry Sep 11, 2023

mdnrz Sep 11, 2023

aggarg Sep 11, 2023

RichardBarry Sep 11, 2023

paulbartell commented Sep 11, 2023 •

edited

Loading

Skptak Sep 18, 2023

aggarg Sep 18, 2023 •

edited

Loading

Skptak Sep 18, 2023

aggarg Sep 19, 2023


		#if (configSUPPORT_STATIC_ALLOCATION == 1)

		__attribute__((__weak__)) void vApplicationGetIdleTaskMemory (StaticTask_t **ppxIdleTaskTCBBuffer,

Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory #790

Add default implementations of vApplicationGetIdleTaskMemory and vApplicationGetTimerTaskMemory #790

Conversation

mdnrz commented Sep 10, 2023 • edited by aggarg Loading

Description

Test Steps

Checklist:

Related Issue

rawalexe commented Sep 10, 2023

codecov bot commented Sep 10, 2023 • edited Loading

Codecov Report

htibosch Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

RichardBarry Sep 11, 2023

Choose a reason for hiding this comment

mdnrz Sep 11, 2023

Choose a reason for hiding this comment

aggarg Sep 11, 2023

Choose a reason for hiding this comment

RichardBarry Sep 11, 2023

Choose a reason for hiding this comment

paulbartell commented Sep 11, 2023 • edited Loading

Skptak Sep 18, 2023

Choose a reason for hiding this comment

aggarg Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Skptak Sep 18, 2023

Choose a reason for hiding this comment

aggarg Sep 19, 2023

Choose a reason for hiding this comment

mdnrz commented Sep 10, 2023 •

edited by aggarg

Loading

codecov bot commented Sep 10, 2023 •

edited

Loading

htibosch Sep 11, 2023 •

edited

Loading

paulbartell commented Sep 11, 2023 •

edited

Loading

aggarg Sep 18, 2023 •

edited

Loading