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

Fix flash_safe_execute timeout under FreeRTOS #2021

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jackwilsdon
Copy link
Contributor

In some cases, the flash lockout task can start on the wrong core and not have time to move to the correct core before exit is called. This causes a timeout as the exit function is looking at the wrong core when checking for the lockout task.

Fixes #2007.

In some cases, the flash lockout task can start on the wrong core and
not have time to move to the correct core before exit is called. This
causes a timeout as the exit function is looking at the wrong core when
checking for the lockout task.
TaskHandle_t task_handle;
if (pdPASS != xTaskCreate(flash_lockout_task, "flash lockout", configMINIMAL_STACK_SIZE, (void *)core_num, 0, &task_handle)) {
if (pdPASS != xTaskCreateAffinitySet(flash_lockout_task, "flash lockout", configMINIMAL_STACK_SIZE, (void *)core_num, 0, 1u << (core_num ^ 1), &task_handle)) {
return PICO_ERROR_INSUFFICIENT_RESOURCES;
}
lockout_state[core_num] = FREERTOS_LOCKOUT_LOCKER_WAITING;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this line should be moved before the call to xTaskCreateAffinitySet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes it probably should, as otherwise it'll use the previous lockout state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then there's no point creating it with a low priority? May as well make it super high immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sounds reasonable! I'll change it to do that and give it a test tomorrow 👍

@kilograham kilograham added this to the 2.1.0 milestone Nov 6, 2024
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.

3 participants