use configSTACK_DEPTH_TYPE consequently (updated for 11.0.x)

feilipu commented 8 months ago

Title

Use configSTACK_DEPTH_TYPE consequently throughout.

Description

The configuration configSTACK_DEPTH_TYPE is used partially, but many places the stack depth variable type is assumed to be uint16_t, and in others the assumption is uint32_t, and the variable names are sometimes us... or ul... respectively.

This PR implements consequent use of configSTACK_DEPTH_TYPE throughout and adjusts variable names to be ux... or pux... as appropriate.

The default value remains set as __uint16_t__, although some ports assume uint32_t without necessarily redefining it as advised.

Files have been updated for updated for 11.0.x.

Note that some extended coverage tests fail because they don't use the correct variable size, as some ports don't configure the configSTACK_DEPTH_TYPE correctly. Rather they incorrectly assume the default is uint32_t.

Just as a side note; the contents of this PR have been tested by 1,000s of users over the past 6 years in Arduino_FreeRTOS. Anyone who has come to learn FreeRTOS via the Arduino IDE will expect the stack type behaviour found in this PR.

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 7 months ago

/bot run formatting

Skptak commented 7 months ago

In the commit that introduced configSTACK_DEPTH_TYPE (link), it's mentioned that reason this was introduced was to keep the existing APIs backwards compatible.

Changing the variable name from ulStackDepth to uxStackDepth, while correct according to the FreeRTOS Naming Convention, would break backwards compatibility

In my opinion, if we're going to introduce a backwards compatibility breaking change like this, we should instead remove configSTACK_DEPTH_TYPE and use StackType_t instead. This way the value would then be the correct bit width for any architecture it is running on, instead of relying upon a separate configuration option?

feilipu commented 7 months ago

We should instead remove configSTACK_DEPTH_TYPE and use StackType_t instead.

That path fails for 8-bit devices, which often (usually) need more than 256 bytes of stack for each Task. Hence the configuration default has always been set to uint16_t.

Unfortunately also, historically many ports didn't change that default setting to something sensible or efficient for their architecture. So there is mixed variable casting between 16-bit and 32-bit code, which has been addressed in this PR.

Also the current code is inefficient on 64-bit architectures, which has also been addressed by this PR too.

The new FreeRTOSConfig.h template file provides StackType_t as the default configuration for configSTACK_DEPTH_TYPE rather than the actual default value. It is also worth noting that the comment describing current usage is incorrect and can be fixed, depending on the outcome of this PR.

Perhaps setting StackType_t as the default configuration is the best option going forward, as you have suggested?

But the stack depth type always needs to remain configurable, to cover all device architectures.

I can adjust the default configSTACK_DEPTH_TYPE to StackType_t in this PR, but add the configuration of uint16_t for all 8-bit ports if that is the agreed path forward...? But it is probably best done as a separate PR, as and when needed.

n9wxu commented 7 months ago

I like these changes and have approved them. We need a matching PR to fix the tests and demo's. Briefly looking at these CI failures shows there are only a few places where changes are required.

aggarg commented 7 months ago

I have made the following changes in the recent commit:

Default configSTACK_DEPTH_TYPE to StackType_t.
Do not rename TaskParameters_t.usStackDepth and TaskStatus_t.usStackHighWaterMark as this would avoid breaking existing applications. More than the existing applications, I am concerned about any existing FreeRTOS aware debug tools which might be using TaskStatus_t.

Other than those there were some build failures which I have addressed. Let me know if you are good with these and we will merge this PR.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5040a67) 93.42% compared to head (418ce5b) 93.42%.

Files	Patch %	Lines
tasks.c	94.44%	1 Missing :warning:

Additional details and impacted files

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

feilipu commented 7 months ago

Default configSTACK_DEPTH_TYPE to StackType_t.

Agreed that is the best solution as discussed above.

Perhaps (in a separate PR) some 8 bit CPU ports will need to have their FreeRTOSConfig.h configuration set to uint16_t, if it is not already the case.

Also the documentation will need to be updated in certain places to reflect the new default.

Do not rename TaskParameters_t.usStackDepth and TaskStatus_t.usStackHighWaterMark as this would avoid breaking existing applications. More than the existing applications, I am concerned about any existing FreeRTOS aware debug tools which might be using TaskStatus_t.

Agreed. Noting that this path is not compliant with the FreeRTOS variable naming standards, which may lead to later confusion.