Skip to content
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

use configSTACK_DEPTH_TYPE consequently (updated for 11.0.x) #942

Merged
merged 59 commits into from
Jan 27, 2024

Conversation

feilipu
Copy link
Contributor

@feilipu feilipu commented Jan 1, 2024

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.

@n9wxu
Copy link
Member

n9wxu commented Jan 18, 2024

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.

sirnish
sirnish previously approved these changes Jan 18, 2024
@feilipu feilipu dismissed stale reviews from sirnish and n9wxu via b834528 January 19, 2024 02:59
feilipu and others added 7 commits January 19, 2024 13:59
Update ulStackDepth to uxStackDepth

Co-authored-by: Soren Ptak <[email protected]>
Also add uint32_t cast prvGetMPURegionSizeSetting.
Revert casting of ( uint32_t ) pxBottomOfStack
Update unpaired critical section in vTaskDelete for readability (FreeRTOS#958)
@aggarg
Copy link
Member

aggarg commented Jan 25, 2024

I have made the following changes in the recent commit:

  1. Default configSTACK_DEPTH_TYPE to StackType_t.
  2. 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.

Copy link

codecov bot commented Jan 25, 2024

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

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.

@feilipu
Copy link
Contributor Author

feilipu commented Jan 25, 2024

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.

Other than those there were some build failures which I have addressed.

Thanks.

Let me know if you are good with these and we will merge this PR.

Please proceed.

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@kar-rahul-aws
Copy link
Member

Thanks @feilipu . We will update the documentation for the changes.

@kar-rahul-aws kar-rahul-aws merged commit 14dd5b5 into FreeRTOS:main Jan 27, 2024
17 checks passed
feilipu added a commit to feilipu/FreeRTOS-Kernel that referenced this pull request Jan 27, 2024
use configSTACK_DEPTH_TYPE consequently (updated for 11.0.x) (FreeRTOS#942)
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.

6 participants