Skip to content

[FIX] add NULL pointer check to xTaskIncrementTick() #1249

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

Closed
wants to merge 2 commits into from

Conversation

InnerSteff
Copy link
Contributor

add NULL pointer check of pxDelayedTaskList to xTaskIncrementTick()

Description

if the system tick interrupt is started independent from the
scheduler the first tick increment might occur before the
the first task is created and therefore pxDelayedTaskList is
still not set

Test Steps

Set up the system tick hander before the scheduler is started

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.

Related Issue

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

@InnerSteff InnerSteff requested a review from a team as a code owner February 19, 2025 17:28
@InnerSteff
Copy link
Contributor Author

/bot run formatting

@AhmedIsmail02
Copy link
Member

AhmedIsmail02 commented Feb 20, 2025

@InnerSteff I'm trying to figure out the use cases where the SysTick Timer would be configured and started by the application before the scheduler is started (which would re-configure and take over the SysTick Timer), are you facing any issues with the current implementation?

@InnerSteff
Copy link
Contributor Author

Yes I had an issue with it. In my special case I can not use the SysTick Timer. And it happened that xTaskIncrementTick() was called before the first task was created. This lead to the NULL point access. And a crash of my application. It took me quite long to find the reason for the crash. The assert would have helped me a lot, and I thought it might also help others.

@archigup
Copy link
Member

Instead of an assert, should this not ignore the systick if the scheduler is not yet started?

@AhmedIsmail02
Copy link
Member

AhmedIsmail02 commented Feb 21, 2025

In my special case I can not use the SysTick Timer.

If the SysTick Timer is not configured and enabled by the application, then the SysTick Timer Interrupt handler SysTick_Handler which calls xTaskIncrementTick() shouldn't be triggered.
If you're using a different timer than SysTick Timer for the application, then that timer should have its own interrupt handler (rather than using the one that is supposed to be used by the kernel) and if that's the only timer available to be used by both the application and the kernel, then before vTaskStartScheduler() is called the IRQ assigned to that timer interrupt should be changed to the port defined timer interrupt handler SysTick_Handler. If you can use more than one timer, I would say the application should have a different timer than the kernel one and both of them shall have different interrupt handlers.

@InnerSteff
Copy link
Contributor Author

I am not using the SysTick_Handler at all, I even removed its initialization from xPortStartScheduler(). I have set up another interrupt handler and worked around the issue by checking the scheduler state via xTaskGetSchedulerState() before calling xTaskIncrementTick().

My application is working fine now. But it was difficult to find the NULL pointer access which caused my application to crash. Thats why I am suggesting this improvement.

@aggarg
Copy link
Member

aggarg commented Feb 24, 2025

I have set up another interrupt handler and worked around the issue by checking the scheduler state via xTaskGetSchedulerState() before calling xTaskIncrementTick().

The correct way to use a different timer other than SysTick is to override the weak function vPortSetupTimerInterrupt. Implement vPortSetupTimerInterrupt and initialize your timer in this function. Would that fix your issue?

@InnerSteff
Copy link
Contributor Author

No this would not fix my issue, because I must setup my Interrupt handler before I start the scheduler. Therefore setting up my interrupt within xPortStartScheduler() is not an option for me.

@aggarg
Copy link
Member

aggarg commented Feb 24, 2025

because I must setup my Interrupt handler before I start the scheduler.

Do you mean to say that you must start your hardware timer that drives the FreeRTOS tick, before the scheduler is started? If yes, would you please share the reason? Also, which hardware are you using?

@InnerSteff
Copy link
Contributor Author

Yes, you are right. My system tick must be synchronized with the periodic communication of an external second processor. Therefore, my system tick interrupt is actually not a timer, but the receive interrupt of the communication interface. The communication interface must be set up before the scheduler is started, because some communication must be done before creating the tasks and starting the scheduler.

Using the system tick time or any other processor internal timer led to drifting apart of the system tick and the communication. Therefore, I decided to use the communication interrupt also for the system tick

@aggarg
Copy link
Member

aggarg commented Feb 24, 2025

Thank you for explaining. This does seem a special case where some Rx interrupt drives the tick instead of a periodic timer. In that case, your implementation of checking the scheduler state is the correct one.

I recommend against adding this additional assert in xTaskIncrementTick. Since this function is called at a high frequency, introducing an assert for this special case would be an unnecessary performance penalization in normal operations. The potential cost outweighs the benefit.

@InnerSteff InnerSteff closed this Feb 25, 2025
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.

5 participants