-
Notifications
You must be signed in to change notification settings - Fork 8k
Support absolute timings for ESP Timer (High Resolution Timer) (IDFGH-16718) #17807
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
base: master
Are you sure you want to change the base?
Support absolute timings for ESP Timer (High Resolution Timer) (IDFGH-16718) #17807
Conversation
👋 Hello robot-controller, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| if (alarm_us != 0) { | ||
| timer->alarm = alarm_us; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Deadlock in esp_timer_restart_at_impl with timer_lock
The esp_timer_restart_at_impl function causes a deadlock by calling timer_remove while already holding the non-recursive timer_list_lock. It also inconsistently ignores an alarm_us value of 0, preventing absolute alarm times at boot, unlike other _at timer functions.
KonstantinKondrashov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
It would be good to add a few notes in the doc (esp_timer.rst) and add some tests in pytest_esp_timer.py.
| return ESP_ERR_INVALID_STATE; | ||
| } | ||
|
|
||
| return esp_timer_restart_at_impl(timer, period_us, alarm_us); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we have to check alarm_us, != 0, and greater than the current time, and return an error.
For all 3 functions.
| if (!is_initialized()) { | ||
| return ESP_ERR_INVALID_STATE; | ||
| } | ||
| return esp_timer_start_once_at_impl(timer, alarm_us); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the check of alarm_us
|
|
||
| esp_err_t ESP_TIMER_IRAM_ATTR esp_timer_start_once_at(esp_timer_handle_t timer, uint64_t alarm_us) | ||
| { | ||
| if (timer == NULL || alarm_us <= esp_timer_impl_get_time() || alarm_us == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Timer Race: Time Slips, Schedules Break.
Race condition in esp_timer_start_once_at: the validation alarm_us <= esp_timer_impl_get_time() occurs before acquiring the timer lock, but time can advance between this check and the actual timer insertion inside the critical section. This allows timers with alarm times in the past to be scheduled if sufficient time elapses between validation and insertion, potentially causing immediate or missed callbacks.
| esp_err_t ESP_TIMER_IRAM_ATTR esp_timer_start_periodic_at( | ||
| esp_timer_handle_t timer, uint64_t period_us, uint64_t first_alarm_us) | ||
| { | ||
| if (timer == NULL || first_alarm_us <= esp_timer_impl_get_time() || first_alarm_us == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Time Warp: Past Timers Get Scheduled
Race condition in esp_timer_start_periodic_at: the validation first_alarm_us <= esp_timer_impl_get_time() occurs before acquiring the timer lock, but time can advance between this check and the actual timer insertion inside the critical section. This allows timers with alarm times in the past to be scheduled if sufficient time elapses between validation and insertion, potentially causing immediate or missed callbacks.
|
|
||
| esp_err_t ESP_TIMER_IRAM_ATTR esp_timer_restart_at(esp_timer_handle_t timer, uint64_t period_us, uint64_t alarm_us) | ||
| { | ||
| if (timer == NULL || alarm_us <= esp_timer_impl_get_time() || alarm_us == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Time-Sensitive Race: Timers Schedule Unexpectedly
Race condition in esp_timer_restart_at: the validation alarm_us <= esp_timer_impl_get_time() occurs before acquiring the timer lock, but time can advance between this check and the actual timer insertion inside the critical section. This allows timers with alarm times in the past to be scheduled if sufficient time elapses between validation and insertion, potentially causing immediate or missed callbacks.
|
Thank you for the review! The new commits add the missing checks for alarm times as well as updates to the tests and the docs to reflect the new functionality. |
|
Thanks for the update. LGTM. |
|
sha=80516c24ac5edd659977e8836951a4b4e9ee4c05 |
Description
The ESP Timer API is very useful for scheduling delayed and / or periodic timer. In some cases however, multiple timers should execute in a synchronized manner with respect to each other, with high precision in terms of relative timing between timers.
The current implementation only allows scheduling timers relative to the current moment of scheduling. This makes it more difficult to achieve precise relative timing between two or more timers, because the timer alarm time is defined partially by when the timer is scheduled.
In this PR, three new functions have been introduced to facilitate scheduling timers based on absolute time (as returned by esp_timer_get_time()): esp_timer_start_once_at(), esp_timer_start_periodic_at(), and esp_timer_restart_at(). They correspond to esp_timer_start_once(), esp_timer_start_periodic(), and esp_timer_restart(), respectively.
esp_timer_start_once_at() accepts a parameter
alarm_usdefining the timestamp at which the timer should fire instead of the relativetimeout_usof esp_timer_start_once().esp_timer_start_periodic_at() accepts a parameter
first_alarm_usdefining the timestamp at which the timer should fire for the first time in addition to the arguments of esp_timer_start_periodic().esp_timer_restart_at() accepts a parameter
alarm_usdefining the timestamp at which the timer should fire next. Differently to esp_timer_restart(), that implies that the second parameter is unused if the timer to be restarted is a one-shot timer. For periodic timers, that parameter defines the period at which the timer will fire afteralarm_us.Testing
The added functions have been added to the esp_timer example to showcase its functionality. This example also served to test the new functions.
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Adds absolute-timing support to ESP Timer via new APIs to start/restart at an absolute timestamp, with corresponding docs, example updates, and tests.
esp_timer_start_once_at,esp_timer_start_periodic_at,esp_timer_restart_atfor absolute-time scheduling.*_at_implhelpers; validate non-zero, future alarms.esp_timer_start_once_at_impl,esp_timer_start_periodic_at_impl,esp_timer_restart_at_impl.alarmand handle new-period semantics.docs/en/api-reference/system/esp_timer.rst.examples/system/esp_timer):*_atAPIs andesp_timer_restart_at.pytest_esp_timer.pyto assert absolute-timed callbacks and restart behavior; update expected logs.Written by Cursor Bugbot for commit 80516c2. This will update automatically on new commits. Configure here.