use configSTACK_DEPTH_TYPE consequently #338

feilipu · 2021-05-29T07:27:55Z

Use configSTACK_DEPTH_TYPE consequently.

Previously the constant configSTACK_DEPTH_TYPE was introduced to support stacks larger than UBaseType_t in 8-bit machines.
This PR uses the configSTACK_DEPTH_TYPE type constant to consequently set the stack size report variable to the appropriate (smallest correct) type.

Extend the use of configSTACK_DEPTH_TYPE which enables developers to define the type used to hold stack usage counters. Defaults to uint16_t for backward compatibility. #define configSTACK_DEPTH_TYPE to a type (for example, uint64_t) in FreeRTOSConfig.h to override the default.

EDIT Revised the various names for stack variables under the standardised naming convention.

This now aligns with #350, so that the stack depth type is also configurable.

Related Issue

Previously discussed and configSTACK_DEPTH_TYPE constant introduced here.

These changes have been in Arduino FreeRTOS since Jan 2018, as noted here and in later commits.

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

codecov · 2021-06-02T00:23:50Z

Codecov Report

Merging #338 (bd7810e) into main (741185f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #338   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           4        4           
  Lines        1272     1272           
  Branches      342      342           
=======================================
  Hits         1172     1172           
  Misses         53       53           
  Partials       47       47

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

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 741185f...bd7810e. Read the comment docs.

feilipu · 2021-06-19T10:39:14Z

Hi @RichardBarry could you get your pr-bar-raiser to take a look please?
Thanks, 🙏

RichardBarry

I didn't make the same comment on each change, but basically the configSTACK_DEPTH_TYPE default should match the prior code, which means uint16_t, which then invalidates the use of the constant where the data type has always been uint32_t. There are some updates to the code comments that should be made though.

include/FreeRTOS.h

RichardBarry · 2021-06-23T02:41:37Z

include/task.h

- *                               const char *pcName,
- *                               uint32_t ulStackDepth,
+ *                               const char * const pcName,
+ *                               const configSTACK_DEPTH_TYPE ulStackDepth,


This function always used uint32_t so this change is not consistent with configSTACK_DEPTH_TYPE defaulting to uin16_t, as per my previous comment.

RichardBarry · 2021-06-23T02:42:04Z

include/task.h

@@ -465,7 +465,7 @@ typedef enum
 #if ( configSUPPORT_STATIC_ALLOCATION == 1 )
    TaskHandle_t xTaskCreateStatic( TaskFunction_t pxTaskCode,
                                    const char * const pcName,     /*lint !e971 Unqualified char types are allowed for strings and single characters only. */
-                                    const uint32_t ulStackDepth,
+                                    const configSTACK_DEPTH_TYPE ulStackDepth,


As per my comment for the prototype of this function in the code comments.

RichardBarry · 2021-06-23T02:43:33Z

include/task.h

@@ -1652,7 +1652,7 @@ configSTACK_DEPTH_TYPE uxTaskGetStackHighWaterMark2( TaskHandle_t xTask ) PRIVIL
     */
    void vApplicationGetIdleTaskMemory( StaticTask_t ** ppxIdleTaskTCBBuffer,
                                               StackType_t ** ppxIdleTaskStackBuffer,
-                                               uint32_t * pulIdleTaskStackSize ); /*lint !e526 Symbol not defined as it is an application callback. */
+                                               configSTACK_DEPTH_TYPE * pulIdleTaskStackSize ); /*lint !e526 Symbol not defined as it is an application callback. */


As per my comment for the prototype in the code comments.

RichardBarry · 2021-06-23T02:43:39Z

include/task.h

@@ -1641,7 +1641,7 @@ configSTACK_DEPTH_TYPE uxTaskGetStackHighWaterMark2( TaskHandle_t xTask ) PRIVIL
 #if ( configSUPPORT_STATIC_ALLOCATION == 1 )
    /**
     * task.h
-     * <pre>void vApplicationGetIdleTaskMemory( StaticTask_t ** ppxIdleTaskTCBBuffer, StackType_t ** ppxIdleTaskStackBuffer, uint32_t *pulIdleTaskStackSize ) </pre>
+     * <pre>void vApplicationGetIdleTaskMemory( StaticTask_t ** ppxIdleTaskTCBBuffer, StackType_t ** ppxIdleTaskStackBuffer, configSTACK_DEPTH_TYPE * pulIdleTaskStackSize ) </pre>


Here too - I think this was always uint32_t so should not use th econfigSTACK_DEPTH_TYPE constant.

RichardBarry · 2021-06-23T02:44:25Z

include/timers.h

@@ -1330,7 +1330,7 @@ BaseType_t xTimerGenericCommand( TimerHandle_t xTimer,

    /**
     * task.h
-     * <pre>void vApplicationGetTimerTaskMemory( StaticTask_t ** ppxTimerTaskTCBBuffer, StackType_t ** ppxTimerTaskStackBuffer, uint32_t *pulTimerTaskStackSize ) </pre>
+     * <pre>void vApplicationGetTimerTaskMemory( StaticTask_t ** ppxTimerTaskTCBBuffer, StackType_t ** ppxTimerTaskStackBuffer, configSTACK_DEPTH_TYPE * pulTimerTaskStackSize ) </pre>


This too was always uint32_t.

RichardBarry · 2021-06-23T02:44:40Z

include/timers.h

@@ -1341,7 +1341,7 @@ BaseType_t xTimerGenericCommand( TimerHandle_t xTimer,
     */
    void vApplicationGetTimerTaskMemory( StaticTask_t ** ppxTimerTaskTCBBuffer,
                                          StackType_t ** ppxTimerTaskStackBuffer,
-                                              uint32_t * pulTimerTaskStackSize );
+                                          configSTACK_DEPTH_TYPE * pulTimerTaskStackSize );


As per comment on the prototype for this function in the code comments.

feilipu · 2021-06-23T03:12:48Z

@RichardBarry thank you for the detailed comments.

The only purpose of this PR is indeed to change the data type to a known configurable value for the size of stack return.

Whether this configSTACK_DEPTH_TYPE defaults to uint16_t or uint32_t is of little importance. I set the default to uint32_t to pass the merge tests. It can be set to uint16_t if the tests are also updated to use configSTACK_DEPTH_TYPE.

the use of the constant where the data type has always been uint32_t.

In my opinion, this is bug. Simply setting the configSTACK_DEPTH_TYPE to uint64_t would demonstrate it as such.
The return stack type needs to be consistently configurable across all of its uses, as this PR seeks to achieve.

RichardBarry · 2021-06-23T04:02:40Z

Perhaps I should give some background.

Original ports of FreeRTOS were to 8 and 16-bit chips, hence using uint16_t to hold stack sizes made sense. uint16_t soon proved too small as FreeRTOS became popular on 32 and now even 64-bit chips though. Hence newer functions, like xTaskCreateStatic(), used uint32_t instead of uint16_t and there were feature requests to likewise change to uint32_t in the older functions too. However FreeRTOS has a policy of not breaking backward compatibility unless absolutely necessary (which I think has only been a couple of times in its history). That, in part, is achieved using the configENABLE_BACKWARD_COMPATABILITY constant. That defaults to 1, so the default is backward compatibility. All constants that are enabled by configENABLE_BACKWARD_COMPATABILITY must default to a value that makes the code behave as it did before a change was made, so configSTACK_DEPTH_TYPE must be uint16_t, otherwise there is no point to the constant. When it is uint16_t all function prototypes have the exact same data types as before configSTACK_DEPTH_TYPE was defined.

All constants that start "config" are intended to be user definable in FreeRTOSConfig.h, so if you want all the stack size variables to be of size configSTACK_DEPTH_TYPE then it is intended you define it as such in FreeRTOSConfig.h.

feilipu · 2021-06-23T04:17:34Z

Thanks, and I understand the reasons for, and history of, compatibility.

Would it be more acceptable to wrap the uint32_t data type definitions in configENABLE_BACKWARD_COMPATABILITY, and have the correct configSTACK_DEPTH_TYPE data type definitions as the non-backwards compatible alternative?

If so, I can update the PR to that end.

EDIT

CBMC test now fails because the default configSTACK_DEPTH_TYPE was reverted to uint16_t, but uint32_t was expected by test suite.
Posix build now fails as it uses inappropriate "unsigned short" name for xStatus.usStackHighWaterMark of type configSTACK_DEPTH_TYPE. The standardised variable name should be xStatus.uxStackHighWaterMark.

Revert for backwards compatibility

n9wxu · 2021-12-31T18:47:51Z

There are many good ideas here but many of the changes will break backwards compatibility more than we can address with configENABLE_BACKWARD_COMPATIBILITY. Your PR did point out a big issue with mpu_xTaskCreate() that will need to be addressed. I am going to close this PR but we will continue to keep the stack types in mind when we can break backward compatibility.

) Move the PKCS FreeRTOS#11 Mutual Auth demo from the LTS development branch. Remove left over and unused mbed TLS contexts.

use configSTACK_DEPTH_TYPE consequently

0b626b8

feilipu requested a review from a team as a code owner May 29, 2021 07:27

update default to uint32_t

848a70c

feilipu added 3 commits June 2, 2021 12:01

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

51934b6

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

cde8062

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

5157761

RichardBarry requested changes Jun 23, 2021

View reviewed changes

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

4dceecd

feilipu added 14 commits June 23, 2021 15:34

Update FreeRTOS.h

be27225

Revert for backwards compatibility

Update portable.h

779d915

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

4a74dfe

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

158730b

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

9867312

configSTACK_DEPTH_TYPE - unify stack variable naming

f3ca628

update lexicon.txt

6b8489d

update typo lexicon.txt

f7fff74

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

f1e095d

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

c188eac

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

27c0794

Update task.h

92052c3

Update timers.h

37d1177

Merge branch 'main' into consequent_configSTACK_DEPTH_TYPE

bd7810e

n9wxu closed this Dec 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

use configSTACK_DEPTH_TYPE consequently #338

use configSTACK_DEPTH_TYPE consequently #338

Uh oh!

feilipu commented May 29, 2021 •

edited

Loading

Uh oh!

codecov bot commented Jun 2, 2021 •

edited

Loading

Uh oh!

feilipu commented Jun 19, 2021

Uh oh!

RichardBarry left a comment

Uh oh!

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

RichardBarry Jun 23, 2021

Uh oh!

feilipu commented Jun 23, 2021 •

edited

Loading

Uh oh!

RichardBarry commented Jun 23, 2021

Uh oh!

feilipu commented Jun 23, 2021 •

edited

Loading

Uh oh!

n9wxu commented Dec 31, 2021

Uh oh!

Uh oh!

use configSTACK_DEPTH_TYPE consequently #338

use configSTACK_DEPTH_TYPE consequently #338

Uh oh!

Conversation

feilipu commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!