Skip to content

Allow specifying an initial timer trigger time in create_timer/create_wall_timer - #3270

Open
thomasmoore-torc wants to merge 4 commits into
ros2:rollingfrom
thomasmoore-torc:timer-initial-call-time
Open

thomasmoore-torc wants to merge 4 commits into
ros2:rollingfrom
thomasmoore-torc:timer-initial-call-time

Conversation

@thomasmoore-torc

Copy link
Copy Markdown

rclcpp: Allow specifying an initial timer trigger time in create_timer/create_wall_timer

Branch: timer-initial-call-time (this repo: ros2/rclcpp)
Depends on: ros2/rcl#1335 — "Allow specifying an initial timer trigger time" (adds rcl_timer_init3). This PR requires that change to be available and should land after it.

Summary

Building on the new rcl_timer_init3 (see the companion rcl PR), this adds rclcpp-level overloads that let callers specify an explicit initial trigger time for a timer instead of always waiting one full period before the first callback.

Typical motivating cases: firing a timer immediately then repeating every period, staggering multiple timers' first calls to avoid a thundering herd, or aligning a timer's first call to a specific point in time (e.g. synchronizing with an external schedule).

Changes

  • rclcpp::TimerBase / rclcpp::GenericTimer gain constructors taking an explicit rclcpp::Time initial_call_time in addition to period, calling rcl_timer_init3 under the hood.
  • rclcpp::create_timer(...) / rclcpp::create_wall_timer(...) (free functions in create_timer.hpp) gain overloads taking initial_call_time before period.
  • rclcpp::Node::create_timer(...) / rclcpp::Node::create_wall_timer(...) gain matching overloads.
  • All additions are new overloads — no existing signatures change.
  • Tests added to test_timer.cpp and test_create_timer.cpp covering: invalid-argument handling for the new overloads, that a future initial_call_time is honored (next trigger reflects it, not now + period), and a fix-regression test (call_timer_forwards_autostart_regardless_of_initial_call_time_overload) confirming autostart is respected consistently across both the old and new overloads.

Compatibility

Purely additive. Existing create_timer/create_wall_timer/TimerBase/GenericTimer/WallTimer call sites are unaffected.

Testing

Built and tested against rolling in a ros:rolling-ros-base container via colcon build/colcon test (3091 tests, 0 failures across rclcpp, rclcpp_action, rclcpp_lifecycle, rclcpp_components, built together with the companion rcl branch), plus uncrustify/cpplint lint targets.

Landing order

Depends on ros2/rcl#1335 (rcl_timer_init3). Please land that one first, or merge together.

🤖 Generated with Claude Code

Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
@jmachowinski

Copy link
Copy Markdown
Collaborator

This breaks the Events and EventsCBG executor for sure.

@jmachowinski

Copy link
Copy Markdown
Collaborator

Hm, this will actually work, as I use rcl_timer_get_next_call_time in the timer_manager....

@thomasmoore-torc

Copy link
Copy Markdown
Author

@jmachowinski - the essence of this PR is very simple. The actual meat of the change is the modification of the following line in rcl to allow for the option of specifying the initial value of next_call_time:

- atomic_init(&impl.next_call_time, now + period);
+ atomic_init(&impl.next_call_time, initial_call_time);

The rest of the change to rcl and rclpp is adding the additional overloads to enable specifying the initial_call_time. The internal function of the timer is otherwise unchanged.

WallTimer (and create_wall_timer) always construct their underlying
Clock as RCL_STEADY_TIME, but the new initial_call_time overloads
accepted a Time of any clock type without checking it. Passing e.g. a
ROS-time Time into create_wall_timer silently produced a garbage
next_call_time, since rcl treats the value as a bare nanosecond count
with no cross-clock conversion.

TimerBase's initial_call_time constructor now throws std::runtime_error
if initial_call_time's clock type doesn't match the timer's clock,
consistent with the existing clock-type-mismatch checks in
Time::operator- and Clock::sleep_until. The check is skipped when the
clock itself is uninitialized, leaving that case to the existing
rcl-level validation.

Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@thomasmoore-torc

Copy link
Copy Markdown
Author

Pushed a follow-up commit (5a1e0e4) adding clock-type validation for the new initial_call_time overloads — while looking into this thread I noticed WallTimer/create_wall_timer accepted an initial_call_time of any clock type without checking it against the timer's own (always-steady) clock, which would silently produce a garbage next_call_time. Not related to the executor question above, but seemed worth fixing here.

@jmachowinski

Copy link
Copy Markdown
Collaborator

I have multiple points were I think the design needs some rework.

The Interface :
I don't like the interface of giving an exact point in time. If the goal is to smear the timers over time, a interface with an interval and an offset in that interval might be more useful.
The start time would then be (time since 1970) + interval * ((time since 1970) % interval) + offset

Reset uses internally now and moves the timer call point around.
I think we should introduce a version here that accepts an absolute time point and the interval offset time point as well. With this we could sync timers by external means.

For the rcl change, would it not be easier to extend the reset API instead of a new init version ? Calling the existing init wit start false, and afterwards reset with the parameters would also do the trick.

The autostart parameter does not make any sense with the current init interface.
I would also not call it timer_initX, but instead something like timer_init_with_start_time etc

@thomasmoore-torc

Copy link
Copy Markdown
Author

The goal is to be able to create a set of timers across a set of ROS2 nodes which fire relative to each other in a deterministic manner. With the current use of now() when determining the start time of each timer, the start time of the set of timers in a distributed system is completely non-deterministic, leading to inconsistent timing issues from run to run of the system.

The concept of an interval and offset is precisely how we're determining the start time to use for the timer. However, I wanted the interface to be more generic in order to allow for other use cases. We can certainly add both interfaces.

I hadn't considered the reset() function's use of now(), which would certainly defeat the point of this change if an application is starting and stopping the timer. I agree that we should extend reset() for this use case, but I'm not sure that I agree that we should require creating the timer with autostart disabled and then having to call reset(). It seems like an unnecessary 2 step process.

Adds rclcpp::TimerBase::resume(), a thin wrapper over the new
rcl_timer_resume() (see the companion rcl change): unlike reset(), it
preserves the timer's existing schedule phase, only catching up if it
is overdue rather than unconditionally recomputing from now(). This is
what makes autostart=false combined with an explicit initial_call_time
actually useful: a timer can be created paused with a specific
phase-anchored schedule and later resumed without losing it, which
reset() cannot do.

Also fixes a gap found while writing this: Node::create_timer(
initial_call_time, ...) had no way to pass autostart at all, unlike
its create_wall_timer sibling overload, even though the underlying
free function already supported it.

Adds rclcpp::compute_phase_aligned_time(clock, interval, phase): a
small utility to compute the smallest instant >= now() of the form
k * interval + phase. This lets independent nodes/processes sharing a
synchronized clock agree on the same aligned timer start instants
without exchanging an explicit time out-of-band.

Renamed the internal call from rcl_timer_init3 to
rcl_timer_init_with_start_time to match the companion rcl rename.

Signed-off-by: Thomas Moore <thomas.moore@torc.ai>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@thomasmoore-torc

Copy link
Copy Markdown
Author

After thinking about this some more, I've decided to add a resume() method. It works similarly to reset(), except it maintains the phase alignment of the timer. This will allow for delay starting or pausing phase-aligned timers, which makes autostart=false practical for timers with a specified start time. In order to allow for creation of phase-aligned timers, a helper method has been added for computing the start time to be passed to the timer creation method.

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.

2 participants