Repository navigation
Allow specifying an initial timer trigger time - #1335
Open
thomasmoore16 wants to merge 6 commits into
Open
thomasmoore16 wants to merge 6 commits into
thomasmoore16 wants to merge 6 commits into
Conversation
Collaborator
|
@jmachowinski we talked about this at ROSCon, and you have an another idea to address this. friendly ping. |
Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
rcl_timer_reset() always recomputes next_call_time as now() + period, which discards the timer's original schedule phase. For a timer initialized with an explicit start time (rcl_timer_init_with_start_time, below) specifically to keep a set of timers aligned across processes, this makes reset() unusable for pausing/resuming such a timer without losing that alignment. rcl_timer_resume() is a separate, unconditional operation: if next_call_time is still in the future, it is left unchanged; if it is in the past (e.g. the timer was canceled and is being resumed later), it is advanced by whole periods until it is in the future again, without shifting the original phase. It also un-cancels the timer, like reset(). The catch-up math is shared with rcl_timer_call_with_info via a new static helper, rather than duplicated. rcl_timer_reset() itself is unchanged, so this is purely additive: existing callers (e.g. rcl_action's goal-expiry timer, which relies on reset() recomputing from now() after a period change) are unaffected. Also renamed rcl_timer_init3 to rcl_timer_init_with_start_time for a more descriptive name; no behavior change. Signed-off-by: Thomas Moore <thomas.moore@torc.ai> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses three issues raised by @fujitatomoya on the PR: - Removed rcl_timer_init()'s deprecated declaration from timer.h. It doesn't belong here: upstream/rolling has already fully removed it (no declaration, no implementation), but this branch's history goes back to an older distro where it still existed, and rebasing onto rolling resurrected it as a fresh addition. Confirmed nothing else in the branch references it. - rcl_timer_init2() read the clock once to compute initial_call_time = now + period, then handed off to rcl_timer_init_with_start_time(), which read the clock a *second*, independent time to set last_call_time. Real time passes between the two reads -- negligible for a monotonic clock, but unbounded for an RCL_ROS_TIME clock if a sim-time jump lands between them. Extracted a shared _rcl_timer_init_impl() that takes `now` as a parameter instead of reading the clock itself; rcl_timer_init2() and rcl_timer_init_with_start_time() each now read the clock exactly once and pass that single reading through. - _rcl_timer_next_call_time_after()'s arithmetic (shared by rcl_timer_call_with_info() and rcl_timer_resume()) could overflow int64_t -- undefined behavior -- for a sufficiently extreme next_call_time/period combination. This existed before in rcl_timer_call_with_info(), where next_call_time was always now() + period and so realistically bounded, but initial_call_time is now caller-supplied and can be set to any value, meaningfully widening the exposure. Added checked-overflow helpers (_would_add_overflow_i64, _would_sub_overflow_i64, _would_mul_overflow_i64_nonneg) and used them throughout; on overflow the result now saturates at INT64_MAX (the timer simply stops becoming ready again) instead of invoking undefined behavior. Applied the same guard to the identical unchecked pattern in rcl_timer_call_with_info() immediately before the call into this helper. Signed-off-by: Thomas Moore <thomas.moore@torc.ai> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each test was verified to fail against the pre-fix code and pass against the fix in the previous commit: - test_timer_init2_reads_clock_exactly_once / test_timer_init_with_start_time_reads_clock_exactly_once: patch rcl_clock_get_now (via mocking_utils) to return a distinct value per call and count invocations. Against the pre-fix code, rcl_timer_init2's call count is 2, not 1 -- restoring the old delegation path (init2 computing initial_call_time, then calling the public rcl_timer_init_with_start_time, which reads the clock again) reproduces this exactly. - test_timer_resume_saturates_instead_of_overflowing: initializes a timer with initial_call_time near INT64_MIN and period INT64_MAX, then resumes it, forcing the catch-up arithmetic to run on inputs designed to overflow. Against the pre-fix unchecked arithmetic, this doesn't crash -- it silently wraps to next_call_time = 0, i.e. the timer would think it's immediately ready. That silent-wrong-answer failure mode, not a clean crash, is exactly why this needed fixing rather than just noting it as a theoretical UB concern. Signed-off-by: Thomas Moore <thomas.moore@torc.ai> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sume() Signed-off-by: Thomas Moore <tcm0116@gmail.com>
thomasmoore16
force-pushed
the
timer-initial-call-time
branch
from
October 31, 2026 20:46
84e7ad6 to
0e8d8c3
Compare
|
@fujitatomoya - based on the conversation in ros2/rclcpp#3270, it sounds like there's an agreed path forward. I've pushed a small update to align this PR with the changes to Note that I am no longer with my previous company, so I'm shifting over to using my personal account to wrap up these PRs. Feel free to re-assign the PR to this account. |
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.
rcl: Allow specifying an initial timer trigger time
Branch:
timer-initial-call-time(this repo:ros2/rcl)Companion PR: ros2/rclcpp#3270 — "Add AlignedTimer and AlignedWallTimer for start-time and phase-aligned timers" (built on top of this change)
Summary
rcl_timer_init2always computes a timer's first trigger time asnow() + period. There is no way to ask for a timer that fires immediately, after a custom initial delay, or at a specific phase-aligned point in time without extra caller-side bookkeeping.This adds
rcl_timer_init_with_start_time, which takes an explicitinitial_call_time(absolutercl_time_point_value_t) alongsideperiod, and updatesrcl_timer_resetso timers initialized withrcl_timer_init_with_start_timepreserve their schedule phase acrosscancel/resetcycles.Changes
rcl_timer_init_with_start_time(timer, clock, context, initial_call_time, period, callback, allocator, autostart): New entry point that seedsnext_call_timedirectly withinitial_call_time(an initial time in the past makes the timer ready immediately) and marks the timer as phase-aligned (impl.aligned = true)._rcl_timer_init_impl: Bothrcl_timer_init2(aligned = false) andrcl_timer_init_with_start_time(aligned = true) read the clock once and passnowto_rcl_timer_init_impl, keepinglast_call_timeandnext_call_timeconsistent on a single clock reading.rcl_timer_resetfor aligned timers:rcl_timer_init2,rcl_timer_resetcontinues to recomputenext_call_time = now + period(unchanged).rcl_timer_init_with_start_time,rcl_timer_resetleavesnext_call_timeunchanged if still in the future, or advances it by wholeperiodmultiples pastnow(with checkedint64_tsaturation to avoid signed overflow) if overdue, while un-canceling the timer.rcl/test/rcl/test_timer.cpp:test_timer_init_with_start_time_with_invalid_argumentstest_timer_init_with_start_time_honors_future_initial_call_timetest_timer_init_with_start_time_with_past_initial_call_time_is_readytest_aligned_timer_reset_does_not_advance_if_not_yet_duetest_aligned_timer_reset_catches_up_if_overduetest_aligned_timer_reset_uncancels_and_preserves_phasetest_timer_init2_reads_clock_exactly_oncetest_timer_init_with_start_time_reads_clock_exactly_oncetest_aligned_timer_reset_saturates_instead_of_overflowingCompatibility
Purely additive — no existing public signatures change, and
rcl_timer_init2/rcl_timer_resetbehavior is unchanged for every existing caller.Testing
Built and tested against
rollingin aros:rolling-ros-basecontainer viacolcon build/colcon test(test_timerand allament_lint_autolinters:uncrustify,cpplint,cppcheck,copyright,lint_cmake,xmllint), as well as together with ros2/rclcpp#3270.Landing order
Prerequisite for companion PR ros2/rclcpp#3270 (
AlignedTimer/AlignedWallTimer), which callsrcl_timer_init_with_start_time. Please land this PR first, or coordinate merges.