Repository navigation
Re-key EventsCBGExecutor timers after a time jump (backport #3344) - #3403
Open
mergify[bot] wants to merge 2 commits into
Open
mergify[bot] wants to merge 2 commits into
mergify[bot] wants to merge 2 commits into
Conversation
rcl moves next_call_time of a ROS time timer when the clock jumps back by more than one period and when ROS time is activated or deactivated, without calling the on reset callback. TimerQueue keyed its running timers by a copy of next_call_time taken when the timer was added, fired or reset, so after such a jump the timer thread slept until the stale deadline: a /clock jump from 1000 s back to 900 s silenced a 100 ms timer for 100 s of simulated time, and a timer that was created before use_sim_time was enabled never fired again. Register rcl's schedule change callback on every ROS time timer. It runs inside the clock jump callback with the clock mutex held, which the timer thread takes while holding its wait mutex, so it only flags the timer; the timer thread re-keys flagged timers before it computes its next wakeup, and the flag is part of its wait predicate. Signed-off-by: Yadunund Vijay <yadunund@gmail.com> Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit ae09d1b) # Conflicts: # rclcpp/src/rclcpp/executors/events_cbg_executor/timer_manager.hpp # rclcpp/test/rclcpp/CMakeLists.txt
Contributor
Author
|
Cherry-pick of ae09d1b has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Contributor
|
Pulls: #3403
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3342
EventsCBGExecutortimers can stall after backward ROS time jumps or whenuse_sim_timeis enabled at runtime becauseTimerQueueretains stale deadlines.This PR uses the new schedule change callback from ros2/rcl#1352 to update timer deadlines. The callback only flags timers for rescheduling to avoid deadlocks.
test_executors_time_jumpverifies both scenarios. It fails on Rolling and passes with both fixes.Depends-On: ros2/rcl#1352
Is this user-facing behavior change?
Yes. Timers continue working after backward
/clockjumps (e.g.,ros2 bag play --loop --clock) and whenuse_sim_timeis enabled at runtime.Did you use Generative AI?
Yes. Claude Code (Claude Fable 5.1) assisted with investigation, implementation, and tests. Changes were reviewed and tested manually.
Additional Information
Reusing the on-reset callback causes a deadlock with
TimeSource::set_clock().This is an automatic backport of pull request #3344 done by Mergify.