Skip to content

fix(freertos): Correct taskRESERVED_TASK_NAME_LENGTH macro definition #1241

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

Merged

Conversation

sudeep-mohanty
Copy link
Contributor

@sudeep-mohanty sudeep-mohanty commented Feb 7, 2025

Description

The taskRESERVED_TASK_NAME_LENGTH definition introduced in #1227 is incorrect for the condition configNUMBER_OF_CORES > 9 as it will never be reached due to the overlapping preprocessor condition of configNUMBER_OF_CORES > 1 before it.

This PR updates the removes the preprocessor condition for configNUMBER_OF_CORES > 9 and adds a note during idle task naming about it.

Test Steps

  • Set configNUMBER_OF_CORES > 9 and build any port for the FreeRTOS kernel.

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.

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

@sudeep-mohanty sudeep-mohanty requested a review from a team as a code owner February 7, 2025 08:06
@sudeep-mohanty
Copy link
Contributor Author

sudeep-mohanty commented Feb 7, 2025

Hi, @kstribrnAmzn @aggarg! A minor correction is needed to the change introduced in #1227. PTAL Thanks!

@sudeep-mohanty
Copy link
Contributor Author

Test updates for failed CI run here - FreeRTOS/FreeRTOS#1324.

@aggarg
Copy link
Member

aggarg commented Feb 7, 2025

If we do this, we need to update the task name writing code as well because we only write one character for the core number as of now - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/tasks.c#L3601.

With the current code, the idle task name would contain wrong core number if the number of cores happen to be more than 9. Since task name is just debugging aid, I think it okay as opposed to adding more task name formatting code. What do you think?

@sudeep-mohanty
Copy link
Contributor Author

If we do this, we need to update the task name writing code as well because we only write one character for the core number as of now - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/tasks.c#L3601.

With the current code, the idle task name would contain wrong core number if the number of cores happen to be more than 9. Since task name is just debugging aid, I think it okay as opposed to adding more task name formatting code. What do you think?

Sure, I agree that avoiding more complex task name formatting for multi-digit core numbers is acceptable. The downside is that the idle task name will contain a "random" ASCII character if the core id exceeds 9. It might be worth considering an update in the future to handle this case if needed. In the meantime, do you think we should add a comment to clarify this limitation and prevent potential confusion?

@aggarg
Copy link
Member

aggarg commented Feb 7, 2025

In the meantime, do you think we should add a comment to clarify this limitation and prevent potential confusion?

Yes, sure. Would you please do that in this PR ;)

@sudeep-mohanty sudeep-mohanty force-pushed the fix/reserved_task_name_len_define branch from ec4b02b to 71390c6 Compare February 7, 2025 16:35
@sudeep-mohanty
Copy link
Contributor Author

Added a note. PTAL again Thanks!

This commit updates the definition of taskRESERVED_TASK_NAME_LENGTH in
tasks.c to fix an unreachable preprocessor condition.

Signed-off-by: Sudeep Mohanty <[email protected]>
@sudeep-mohanty sudeep-mohanty force-pushed the fix/reserved_task_name_len_define branch from 71390c6 to 5762092 Compare February 10, 2025 07:46
@kar-rahul-aws kar-rahul-aws merged commit fbaeba3 into FreeRTOS:main Feb 10, 2025
17 checks passed
@kstribrnAmzn
Copy link
Member

Thanks for fixing this :) I noticed this soon after when doing some safety certification work and we chose to fix it in the same way - but I hadn't had the chance to upstream this work since I was out.

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.

4 participants