Fixes for Timers

[mxgrey]

Thanks for kicking off the effort on Timers for rclrs with ros2-rust#440

Rather than write out a huge amount of feedback and leave it to you to interpret and implement it, I went ahead and cranked out a series of fixes that I recommend for your PR. I'll explain the various changes in a series of sections.

Fixing object lifecycles

The test_new_with_source_clock test was having a segmentation fault in your original PR. I didn't identify the specific cause of this, but the overarching issue was that the lifecycles of rcl_timer_t and possibly rcl_clock_t were not being managed in a way that's compatible with rcl.

This PR changes both of those objects to use in-place initialization. That means we first create the Arc<Mutex<T>> that will hold the data structure (zero-initializing it to start), and then we call rcl_[...]_init on the object while the mutex is locked. This ensures that the address of the rcl object is never changed. Sometimes rcl is sensitive to the address of its objects moving, other times it's not, but it never hurts to take precautions and keep the addresses fixed throughout the lifetimes of the objects.

Secondly this PR introduces TimerHandle which guarantees that the rcl_clock_t being used by the rcl_timer_t cannot be dropped prematurely. In this new structure, rcl_timer_fini is only called when TimerHandle is dropped, ensuring that rcl_clock_fini cannot be called until afterwards.

Streamlining API

In the original PR there were some superfluous arguments being passed into create_timer. The Context is already known by the Node so there's no reason for the user to pass that in. We can usually assume that the user will want a steady clock for their timer, so we shouldn't make it mandatory for them to pass in a clock.

In this PR we change the create_timer API to use TimerOptions to start off the user with a sensible set of default options, and IntoTimerOptions to conveniently build TimerOptions off of a Duration. This follows the same _Options pattern that was introduced in ros2-rust#422 and which will be applied to all create functions with ros2-rust#429.

One-shot callbacks

A widely desired feature which hasn't made it into rclcpp yet is one-shot timers. The ability to define a one-shot timer is especially important for rclrs since Rust draws a very important categorical difference between FnMut and FnOnce. If we do not have built-in support for one-shot timers then users can never use FnOnce with their timers.

I've generalized the definition of timer callbacks to also support one-shot timers (see AnyTimerCallback) and added APIs to help users with creating repeating timers and one-shot timers. Users can also change the callback of a timer whenever they want, as long as they have access to the &Timer. This means after a one-shot callback has been triggered, the user can pass a new one-shot callback into the same timer, and that new callback will be called the next time the Timer is triggered. In fact the current one-shot callback itself can reload the timer with a new one-shot callback. Users can also switch between repeating, one-shot, or removed callback whenever they want.

Callback flexibility

In the original PR users are expected to pass in a Box<dyn Fn(i64) + ...> as a callback, but most users will find it inconvenient to box their callback before providing it. They are also likely to be confused about what the i64 means.

In this PR we introduce trait TimerCallRepeatedly and trait TimerCallOnce which allow users to directly pass in an FnMut(_) and FnOnce(_) for repeated and one-shot timers respectively. This is similar to SubscriptionCallback which is used to make the subscription API very flexible.

We support three different choices of arguments for these callbacks:

• &Timer: The callback will receive a borrow of the timer that it belongs to. The user can call any function on the timer, including Time::set_callback to change the callback of the Timer and Timer::last_elapsed to check how much time elapsed since their callback was last called. Or Timer::clock to get the clock for the timer and check the time.
• Time: The callback will receive the current time of the clock that is being used by the timer
• Nothing: The callback can have no arguments at all

We could also support Duration which would provide the time lapsed since the last call to the callback. I'm happy to add that if anyone would find that interesting.

Idiomatic API

In the original PR there was significant use of i64 with function names that include _nanos to signal to the user that they should interpret the integers as nanoseconds. This is not idiomatic for Rust. We should prefer rigid explicit types like Duration and Time. This PR updates all rclrs API function arguments and fields to explicitly use Time and Duration, and not use i64 anywhere.

[agalbachicar]

Before diving into this feedback and the code itself to assimilate it, just simply let me say thank you, @mxgrey 😄

[tulku]

Minor, this should be [Self::call] I think.

[mxgrey]

Good catch, those functions were refactored after I wrote that documentation. Fixed here: 464d5d9

[mxgrey]

Note that I've pushed c7c1cba which renames Timer::remove_callback to Timer::set_inert to be more consistent with the naming of Node::create_timer_inert.

[agalbachicar]

LGTM

Sorry for the delay on this and happy new year @mxgrey. Year transitions are usually high on the workload and turbulences.
I appreciate a lot the effort put into this PR to educate us and get a way better timer implementation. I am dubious about moving forward with this PR provided that yours is a complete rewrite that follows the manners of the project and rust way more than ours. I think the team here would not hesitate to just let yours move forward and use this PR as a learning experience. Otherwise, we are happy to contribute on anything we can from your or any other maintainer review in the future to push till completion and merge.