Add timer on reset callback

[mauropasse]

@wjwwood this PR is created after discussions originated from this PR: #966
Instead of adding an on_trigger callback to the rcl guard conditions, we add here an on_reset callback to the rcl timers.

The issue which originated this need, is wake up an events based executor when a timer is reset (which could also be caused by a time jump).

FYI @alsora

[timonegk]

@wjwwood @clalancette It would be very nice to merge this and #995 soon. We are trying to use https://github.com/irobot-ros/events-executor/, but it requires these pull requests, and manually rebuilding rcl and rclcpp with the pull requests merged is not sufficient because the rclcpp pull request includes ABI changes that require a rebuild of all packages using TimerBase. It also seems as if the comments on both pull requests have been addressed.

[Flova]

Another ping on this one due to the breaking ABI changes and the need to build nearly everything from source as a consequence if the events executor is used in a single node.

Also I think you mean ros2/rclcpp#1979 @timonegk right?

[Flova]

As the maintainers were updated recently also a frindly ping at @audrow and @ivanpauno

[ivanpauno]

The issue which originated this need, is wake up an events based executor when a timer is reset (which could also be caused by a time jump).

Could you explain that in more detail?

[mauropasse]

Could you explain that in more detail?

There are two situations in which a wait-set based executor can be awaken when something changes related to the timer, by means of triggering its guard condition:

1. Timer is reset:

   rcl/rcl/src/rcl/timer.c

   Line 431 in e4f7e13

   rcl_ret_t ret = rcl_trigger_guard_condition(&timer->impl->guard_condition);

2. Timer detects a time jump:

   rcl/rcl/src/rcl/timer.c

   Line 113 in e4f7e13

   if (RCL_RET_OK != rcl_trigger_guard_condition(&timer->impl->guard_condition)) {

Before these same situations, the on_reset_callback is called.
Maybe a new on_time_jump_callback could be created, along the on_reset_callback.
In the current state, the on_reset_callback is used for both purposes.

[ivanpauno]

Timer is reset:

I think we trigger the guard condition because the timeout used in rcl_wait() wasn't calculated considering that timer.
A callback isn't trigger immediately when a timer is reset.
Based on that, I'm not sure if you would need to trigger a callback in this case for the events executor.
But if it's needed I think it's fine to add it.

Maybe a new on_time_jump_callback could be created, along the on_reset_callback.

IMO, that sounds better.
It's weird to have a on_reset callback that's also triggered on jump.
Could the clock jump callbacks be used for that though?

[mauropasse]

Could the clock jump callbacks be used for that though?

Yes, no need to add an extra callback for it.
I removed the callback call on _rcl_timer_time_jump. That function is actually used as a clock jump callback you mentioned.

[ivanpauno]

LGTM!

It would be nice to have a review of another maintainer before merging, maybe @audrow or @wjwwood ?

[audrow]

Looks good to me with green CI!