ros2_rust icon indicating copy to clipboard operation
ros2_rust copied to clipboard

Basic Timer implementation

Open agalbachicar opened this issue 1 year ago • 4 comments

Description

This is a basic Timer implementation. It is heavily based on what other client libraries do, i.e. use RCL bindings and provide a safe API against the bindings.

Details

It is by no means a final version but it is a good first approach to something close to the final version. We will happily change everything you tell us to change :)

It is important to look at the TimerCallback type trait definition.

We tried to keep mutable and non-mutable functions within Timer but then when integrating it against WaitSet, Node and Executor we had to make it all "non-mutable".

This has been done in collaboration (peer programming) with @tulku , @JesusSilvaUtrera, @jballoffet :confetti_ball:

How to test it?

  • [x] Add end to end example -> fbb862903dcf88210626c062324619cbe53f2bab

We wrote an end to end example, we will soon add it so it can be show how to do it. We introduced some unit tests, we can certainly extend them as soon as we define the final API.

Notes

We observed some non-trivial delays in the callback execution. Certainly the overhead between the bindings and what we are doing is non-negligible. We would appreciate some input here.

Identified discussion points

  • i64 for durations, shall we use proper Duration types?
  • Using the Clock::with_source() makes the test in Timer crash with SIGSEGV. See the comment and test in timer.rs.

agalbachicar avatar Nov 29 '24 19:11 agalbachicar

In reference to your discussion points, I think that having timers use Duration would be valuable.

jhdcs avatar Nov 29 '24 20:11 jhdcs

For visibility, I've opened https://github.com/agalbachicar/ros2_rust/pull/5 which targets this PR and addresses most of the feedback that I expect the working group will have for this PR.

I recommend that others hold off on reviewing this until https://github.com/agalbachicar/ros2_rust/pull/5 gets settled because any reviews in the meantime will likely become outdated soon.

@agalbachicar separately from the PR that I opened, you'll need to merge the latest main branch into your PR branch in order for the CI to pass.

mxgrey avatar Dec 02 '24 14:12 mxgrey

And also here, thanks a lot for such a feedback, @mxgrey ! I'll review it with my peers and get back as soon as possible.

agalbachicar avatar Dec 02 '24 15:12 agalbachicar

Just a friendly reminder that it would be nice to have timers in ros2_rust.

I have seen in https://github.com/agalbachicar/ros2_rust/pull/5 that @mxgrey has done a rewrite. Could we try to upstream this new version or is this in conflict with the async refactor and needs further API changes?

romainreignier avatar Feb 27 '25 09:02 romainreignier

I think it would be reasonable to close this PR in favor of #480

The original authors for this PR are credited in the new one.

mxgrey avatar Aug 11 '25 13:08 mxgrey

Thanks! Closing then.

agalbachicar avatar Aug 21 '25 06:08 agalbachicar