[thomasmoore16]

rclcpp: Add AlignedTimer and AlignedWallTimer for start-time and phase-aligned timers

Branch: timer-initial-call-time (this repo: ros2/rclcpp)
Depends on: ros2/rcl#1335 — "Allow specifying an initial timer trigger time" (adds rcl_timer_init_with_start_time and preserves phase alignment in rcl_timer_reset). This PR requires that change to be available and should land after it (note: the single-repo Rpr__rclcpp buildfarm job will fail until ros2/rcl#1335 is merged and synced into Rolling).

Summary

Building on rcl_timer_init_with_start_time in ros2/rcl#1335 and incorporating feedback from the Client Library Working Group, this adds dedicated AlignedTimer and AlignedWallTimer classes (along with create_aligned_timer / create_aligned_wall_timer helpers) that let callers either specify an explicit initial trigger time or align periodic timers to (period, phase) boundaries instead of always starting at now() + period.

Unlike GenericTimer and WallTimer (whose reset() re-anchors the schedule to now() + period), AlignedTimer and AlignedWallTimer preserve their schedule phase across cancel() / reset() cycles.

Changes

• rclcpp::AlignedTimer / rclcpp::AlignedWallTimer: Dedicated timer subclasses initialized via rcl_timer_init_with_start_time, supporting both:
  • (clock, initial_call_time, period, callback, context, autostart) (and the wall-clock equivalent without clock), validating that initial_call_time's clock type matches the timer's clock, and
  • (clock, period, phase, callback, context, autostart) (and the wall-clock equivalent), computing the initial phase-aligned trigger time automatically.
• rclcpp::compute_phase_aligned_time(clock, interval, phase): Computes the smallest time >= clock.now() of the form k * interval + phase, allowing independent nodes/processes sharing a synchronized clock to align periodic timers without exchanging a start timestamp out-of-band.
• rclcpp::create_aligned_timer(...) / rclcpp::create_aligned_wall_timer(...) free functions in create_timer.hpp and matching rclcpp::Node::create_aligned_timer(...) / rclcpp::Node::create_aligned_wall_timer(...) member functions.
• Tests in test_timer.cpp and test_create_timer.cpp covering AlignedTimer, AlignedWallTimer, create_aligned_timer, create_aligned_wall_timer, compute_phase_aligned_time, clock-type validation, autostart=false, and phase preservation across reset() (both when not yet due and when catching up from overdue).

Compatibility

Purely additive. Existing TimerBase / GenericTimer / WallTimer / create_timer / create_wall_timer public signatures and behavior are unchanged.

Testing

Built and tested against rolling in a ros:rolling-ros-base container alongside ros2/rcl#1335 (colcon build + colcon test across rcl and rclcpp timer suites and all ament_lint_auto linters: uncrustify, cpplint, cppcheck, copyright, lint_cmake, xmllint).

Landing order

Depends on ros2/rcl#1335 (rcl_timer_init_with_start_time). Please test together via ci.ros2.org and land ros2/rcl#1335 first.

[jmachowinski]

This breaks the Events and EventsCBG executor for sure.

[jmachowinski]

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

[thomasmoore16]

@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.

[thomasmoore16]

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]

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

[thomasmoore16]

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.

[thomasmoore16]

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.

[jmachowinski]

I am not too sure about the API.
I fear it it is now too simple to mess up your code by accidentally calling reset instead of resume.

What do you think about adding dedicated new Timer classes and change how reset works on them. This would have the advantage that the API is consistent between timers, and it is more in line with how other timers are modeled in rclcpp. E.g. WallTimer vs GenericTimer

[jmachowinski]

We discussed this feature in the client library working group, and the general consensus was that we are all in favor of it.
But we also all agreed, that we want to have this as a separate class rather than being controlled by flags.
@thomasmoore-torc are you still willing to implement this, or should be give this out to someone else ?

[tcm0116]

@jmachowinski - I have made the requested changes as well as eliminated the rcl_timer_resume() function in ros2/rcl#1335 to be consistent with the rclcpp implementation.

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.