feat: timers API

[mxgrey]

This PR splits the timer.rs module out of #446 so it can be reviewed separately after #446 is merged.

[mxgrey]

@esteve This is embarrassing since I was vocally supportive of committing the cargo lockfiles, but now I'm thinking that might have been a mistake. I'm noticing two issues in practice:

1. Every run of colcon build seems to update the lockfiles. In fact a bunch of different tools that I use on a regular basis are triggering automatic updates of the lockfiles. It's creating a lot of unnecessary churn in the git history.
2. More crucially, I'm seeing this error happen in the minimum supported Rust compiler version CI. Essentially any new lockfiles that get generated and committed will be incompatible with the "Minimal" CI.

I've had to resort to manually deleting the Cargo.lock for the worker_demo, logging_demo, and now timer_demo examples each time I commit in order to get the Minimal CI to pass.

I'd suggest we remove the lockfiles from the git history or else this is going to cause a great deal of ongoing annoyance where everyone is constantly reverting lockfiles before committing 😞

[mxgrey]

@knmcguire Sorry to summon you into this random PR but it seems a new CI failure has popped up for Windows.

I have a feeling this is due to this recent PR in colcon-cargo, and we'll see this failure show up for the main branch on Tuesday during its weekly CI run.

[knmcguire]

Thanks for letting me know ! I don't get messages of the CI failures on windows so I was unaware of this. I'll dig into it and make a PR

The cargo lock did solve some things for the windows CI though so if that is removed we might need to deal with those failures, but let's see when it comes.

[knmcguire]

See this PR: #495

[jhdcs]

It looks very good for the most part! I just noticed some unsafe calls that didn't have Safety comments associated with them.

[knmcguire]

Managed to get this demo to run on my machine (ubuntu on wsl2) 😄

I'm not comfortable to do a full indepth review yet... but I've added some thoughts.

[knmcguire]

Maybe good to have a bin here so that we can run it through ROS2 command?

[mxgrey]

Is this what you had in mind?

Since the example is in src/main.rs, by default it will generate a binary with the same name as the package, examples_timer_demo. But I see in the other example packages there's a certain naming pattern for the binaries, so now I've copied that pattern here.

[knmcguire]

ahhh gotcha.

And yes exactly like that. naming consistency is good :)

[knmcguire]

Works for me and is a nice example :)

Does ROS2 have an example like this actually? Not used in the tutorials right?

I guess the only nitpick would be, that all other python/cpp examples use create_timer()

[mxgrey]

See this reply.

[knmcguire]

I was first confused on why there were some much commented out lines, but then I realized that this was for the documentation. Nice!

[knmcguire]

This goes a bit too deep for my knowledge I'm afraid. But are these timer naming's consistent with the api of rclcpp or rclpy ?

[mxgrey]

The naming is intentionally not the same because in rclrs we're introducing the concept of a one-shot timer, which is different from a repeating timer.

The type system of the Rust language makes a strong distinction between the traits FnMut() and FnOnce(). Repeating timers can use callbacks with the FnMut() trait but cannot use callbacks with the FnOnce() trait. One-shot timers are able to use callbacks with the FnOnce() trait, but we have to guarantee that the callback will only get triggered one time, and then after that the timer must be inert (not contain any callback).

Because of this we end up with three possible kinds of timers:

• Repeating
• One-shot
• Inert

rclcpp and rclpy don't have this distinction because they have no concept of FnMut versus FnOnce, and I don't think they even support a concept of one-shot timer, although the subject has been discussed before.

In my mind this is the kind of divergence that is expected between the client libraries of different languages, in order to reflect differences in what those languages offer.

[knmcguire]

aha.... yes

In hindsight I can indeed see quite differences between the c++ and python tutorials as there are some differences with that as well. Then focusing on api naming consistency shouldn't be the goal as long as it is feature complete.

thanks for the explanation! I missed/forgot this.

[mxgrey]

@jhdcs thanks for noticing the absence of safety comments. I've added them in via a589997

[mxgrey]

CI is currently failing because of an upstream issue which is fixed by ros2-rust/rosidl_runtime_rs#13

[mxgrey]

When I did a squash merge to resolve merge conflicts from #446, I lost the commit history from #440 which this PR was based off of.

I've added 775b170 which should prompt GitHub to credit @agalbachicar and @JesusSilvaUtrera when this PR gets squash-merged, but I would appreciate if whichever maintainer merges this PR can double-check that the final commit message includes

Co-authored-by: Agustin Alba Chicar <ag.albachicar@gmail.com>
Co-authored-by: Jesús Silva <79662866+JesusSilvaUtrera@users.noreply.github.com>

[luca-della-vedova]

I wonder if there is any risk for odd behavior here when a user decides to create a timer with the NodeClock?

Specifically, because we keep an Arc to the underlying rcl_clock that is wrapped by both Timers and Clocks, if a user was to create a Timer with the Node clock it would be kept alive even after the Node is destroyed.
This is possibly desirable for steady clocks, but there is an odd edge case if the Clock is using RosTime when use_sim_time:=true.
Since RosTime is updated through a /clock subscription but the Node has been destroyed, I suspect the Timer silently won't be updated anymore, making it effectively useless.
I don't have a clear solution for this (maybe making the ownership to the Node's clock Weak and offering an API to know whether the underlying clock still exists? But this is also not great) so I'm curious to know what you think

[mxgrey]

One simple solution would be to add a node: Node or node: Arc<NodeHandle> field to the Timer struct so that the node is guaranteed to remain valid for as long as the Timer is valid.

OTOH if a Timer is meant to use the Node's clock and the Node gets dropped, it kind of makes logical sense that the Timer simply stops being triggered since the Node no longer has time. But it's probably not the behavior that a user expects.

IIRC we are only allowing Timers to be created through Nodes, so I see nothing wrong with adding the node field like most other primitives have to express that dependency.

[mxgrey]

I've given TimerState a dependency on Node when using NodeTime: 2e2dce0

[esteve]

@mxgrey sorry it took so long, can you fix the conflicts? Now that rclrs 0.5.0 is out, we can merge this soon. Thanks.

[mxgrey]

@esteve no worries, merge conflicts are fixed!

[esteve]

@mxgrey thanks!

[esteve]

@jhdcs I hope it's ok with you, I'll merge this PR. Given the US government shutdown, that @jhdcs can't work on ros2-rust during the shutdown, and that @mxgrey has addressed @jhdcs 's feedback, I'll go ahead and merge this PR.

Thanks @mxgrey for the great work, and @jhdcs @knmcguire @luca-della-vedova for their reviews.