Add ROS2 timers #4

imdexsam · 2024-10-28T08:20:36Z

This PR adds ROS2 timers and enables them to be triggered by waitsets.

rclrs/src/timer.rs

imdex-matthew

Nice work on this it's looking real clean. There wasn't much to comment on, but I've left some thoughts and questions.

imdex-matthew · 2024-10-29T01:59:00Z

rclrs/src/timer.rs

+// Therefore, this type can be safely sent to another thread.
+unsafe impl Send for rcl_timer_t {}
+
+/// Manage the lifecycle of an `rcl_timer_t`, including managing its dependencies


Suggested change

/// Manage the lifecycle of an `rcl_timer_t`, including managing its dependencies

/// Manage the lifecycle of an `rcl_timer_t`, and its dependencies

rclrs/src/timer.rs

imdex-matthew · 2024-10-29T06:43:52Z

rclrs/src/timer.rs

+    pub fn new<F>(
+        context: &Context,
+        clock: Clock,
+        period: Duration,
+        callback: F,
+    ) -> Result<Self, RclrsError>
+    where
+        F: FnMut(&mut Timer) + 'static + Send + Sync,


Could we get away with a lifetime guarantee something like the suggested? What is the benefit of a 'static lifetime here?

Suggested change

pub fn new<F>(

context: &Context,

clock: Clock,

period: Duration,

callback: F,

) -> Result<Self, RclrsError>

where

F: FnMut(&mut Timer) + 'static + Send + Sync,

pub fn new<'a, F>(

context: &'a Context,

clock: Clock,

period: Duration,

callback: F,

) -> Result<Self, RclrsError>

where

F: FnMut(&mut Timer) + 'a+ Send + Sync,

This one often gets me. Keep in mind the 'static trait bound is subtly different from the 'static reference lifetime (as per rust by example).

By my understanding the practical implication of a 'static trait bound (as seen here) is that Timer needs to own the callback, since that fulfills the requirement that "the receiver can hold on to the type for as long as they want and it will never become invalid until they drop it". This includes owning anything captured by a closure being used as the callback.

If we wanted to switch to lifetimes it would probably need to be more like:

pub fn new<'a, F>( context: &Context, clock: Clock, period: Duration, callback: F, ) -> Result<Timer<'a>, RclrsError> where F: FnMut(&mut Timer) + 'a + Send + Sync,

Since the callback needs to live at least as long as the Timer class, so they're what need to share the lifetime (the &Context just contains an Arc that can be cloned).

This works and does mean you can pass in a &mut to closures used for the callback, but there's a trade-off in that we can no longer store the Timer class inside an Arc.

The trade-off comes from Rusts subtyping and variance system. I've only got a basic understanding of it, but it seems Arc<T> creates a new lifetime that is independent of the object it's storing, so Arc<T> itself needs a 'static trait bound on T (as Arc is invariant over T), so the compiler won't allow storing a Timer<'a> that isn't 'static.

With that in mind, the benefits of explicitly tracking the lifetime doesn't seem to outweigh the costs. Closures can easily "own" any data they reference just by using Arc<Mutex> types, working around an inability to use &mut, but there doesn't seem to be a neat workaround to not being able to put Timer classes inside Arcs.

All of that is to say I think we're best served by sticking with 'static here.

That makes good sense, thanks for digging into it and providing a nice succinct answer!

rclrs/src/timer.rs

imdex-matthew · 2024-10-30T02:06:19Z

rclrs/src/timer.rs

+        let new_period = Duration::from_millis(100);
+
+        // Calling set_period will trigger the debug_assert check on the rcl return value.
+        timer.set_period(new_period.clone());


As per: https://github.com/geoff-imdex/ros2_rust/actions/runs/11564633206/job/32190192780#step:11:395

Suggested change

timer.set_period(new_period.clone());

timer.set_period(new_period);

Good catch, fixed.

imdex-matthew · 2024-10-30T02:08:08Z

rclrs/src/timer.rs

+        // SAFETY:
+        // * The timer is initialized, which is guaranteed by the constructor.
+        // * The elapsed_time pointer is allocated on the stack and is valid for the duration of this function.
+        let ret = unsafe { rcl_timer_get_time_since_last_call(&mut *timer, &mut elapsed_time) };


As per: https://github.com/geoff-imdex/ros2_rust/actions/runs/11564633206/job/32190192780#step:11:395

Suggested change

let ret = unsafe { rcl_timer_get_time_since_last_call(&mut *timer, &mut elapsed_time) };

let ret = unsafe { rcl_timer_get_time_since_last_call(& *timer, &mut elapsed_time) };

Good catch, fixed.

imdex-matthew · 2024-10-30T02:08:37Z

rclrs/src/timer.rs

+        // * The timer is initialized, which is guaranteed by the constructor.
+        // * The remaining_time pointer is allocated on the stack and is valid for the duration of this function.
+        unsafe {
+            rcl_timer_get_time_until_next_call(&mut *timer, &mut remaining_time).ok()?;


As per: https://github.com/geoff-imdex/ros2_rust/actions/runs/11564633206/job/32190192780#step:11:395

Suggested change

rcl_timer_get_time_until_next_call(&mut *timer, &mut remaining_time).ok()?;

rcl_timer_get_time_until_next_call(& *timer, &mut remaining_time).ok()?;

Good catch, fixed.

imdex-matthew · 2024-10-30T02:09:06Z

rclrs/src/timer.rs

+        // SAFETY:
+        // * The timer is initialized, which is guaranteed by the constructor.
+        // * The is_ready pointer is allocated on the stack and is valid for the duration of this function.
+        let ret = unsafe { rcl_timer_is_ready(&mut *timer, &mut is_ready) };


As Per: https://github.com/geoff-imdex/ros2_rust/actions/runs/11564633206/job/32190192780#step:11:395

Suggested change

let ret = unsafe { rcl_timer_is_ready(&mut *timer, &mut is_ready) };

let ret = unsafe { rcl_timer_is_ready(& *timer, &mut is_ready) };

Good catch, fixed.

imdex-matthew · 2024-10-30T02:11:44Z

rclrs/src/timer.rs

+/// [5]: crate::Timer::execute
+/// [6]: crate::WaitSet
+pub struct Timer {
+    callback: Arc<Mutex<dyn FnMut(&mut Timer) + Send + Sync>>,


As per: https://github.com/geoff-imdex/ros2_rust/actions/runs/11564633206/job/32190192780#step:11:395

Could be simplified with a type alias.

Good catch, fixed.

mxgrey · 2024-11-02T12:24:10Z

Note that the upstream rclrs repo has an open PR that introduces async executors, and it will make some significant changes to how wait sets work: ros2-rust#421

Be ready for some merge conflicts down the road. I do have a plan to introduce timers on top of the async execution PR, but I'm currently waiting until that PR gets merged first. If you have a need for timers and are interested in jumping forward to the async execution implementation (at the risk of some API instability), then I'd be happy to make a branch that includes timers.

imdexsam · 2024-11-04T03:04:45Z

@mxgrey I appreciate the heads up, that's very useful to know. It looks like you've done a lot of great work on the async executor, that's a nice improvement.

We'll go ahead with the sync version in the short term, as that unblocks our immediate need for timers, but the less time we spend diverged from upstream the better.

If you're willing to do an async timer implementation that would be amazing. You're more than welcome to copy anything here, I would assume the Timer struct will be largely transferrable. Once the async execution is merged into main I might be able to help port timers across too.

…uld never error.

…r callback.

geoff-imdex

Looks really good. Only one extremely pedantic comment/question - the rcl_bindings:: in the timer.rs explicitly includes all the used functions (nice), however this a long list and many other places seem to use the rcl_bindings::* approach unless they're pulling in a trivially small amount of functions. Not sure it's worth changing, but I had to mention

imdexsam force-pushed the add-basic-timer branch from ed5382b to fd720b0 Compare October 28, 2024 23:15

imdexsam commented Oct 28, 2024

View reviewed changes

rclrs/src/timer.rs Show resolved Hide resolved

imdex-matthew reviewed Oct 30, 2024

View reviewed changes

imdex-matthew approved these changes Oct 31, 2024

View reviewed changes

imdexsam force-pushed the add-basic-timer branch from 597f539 to 58f95ed Compare November 4, 2024 07:15

imdexsam added 17 commits November 5, 2024 01:46

* Basic init and fini of the timer class

2a610ca

* Fixed typo claiming to protect against cleanup as part of rcl_*_init

98ff99d

* Added remaining rcl_timer functions

71bc220

* basic rcl usage tests for timer

d182dba

* Replaced returning Result with debug_asserts for functions that sho…

bb539c3

…uld never error.

* Implemented TimerBase::execute

a5a3d22

* Added timers to Node and WaitSet

8d6007a

* Replaced NodeHandle within Timer with a ContextHandle

9378aab

* Added unit test to confirm a timer will trigger the waitset.

3f29461

* Added unit test to executor to confirm spinning will trigger a time…

1f37cfa

…r callback.

* Changed the timer callback type to an FnMut, added some helper docs

fc72b29

* Updated documentation around creating timers

9ed47e4

* Removed outdated comment

a295956

* Added subtrait for TimerCallback to address clippy error

34603d3

* Fixed clippy errors

c03c596

* Corrected grammar within timer handle comment

3a3d7ef

* Added serde feature to remove build warnings

27a619e

imdexsam force-pushed the add-basic-timer branch from 58f95ed to 27a619e Compare November 5, 2024 01:48

imdexsam added 2 commits November 5, 2024 04:25

* Cleaned up weird braces when dealing with timer mutex

28b6eca

* More brace weirdness

ed06edf

geoff-imdex approved these changes Nov 5, 2024

View reviewed changes

imdexsam marked this pull request as draft November 5, 2024 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ROS2 timers #4

Add ROS2 timers #4

imdexsam commented Oct 28, 2024

imdex-matthew left a comment

imdex-matthew Oct 29, 2024

imdex-matthew Oct 29, 2024

imdexsam Nov 1, 2024 •

edited

Loading

imdex-matthew Nov 4, 2024

imdex-matthew Oct 30, 2024

imdexsam Oct 31, 2024

imdex-matthew Oct 30, 2024

imdexsam Oct 31, 2024

imdex-matthew Oct 30, 2024

imdexsam Oct 31, 2024

imdex-matthew Oct 30, 2024

imdexsam Oct 31, 2024

imdex-matthew Oct 30, 2024

imdexsam Oct 31, 2024

mxgrey commented Nov 2, 2024

imdexsam commented Nov 4, 2024

geoff-imdex left a comment

	/// Manage the lifecycle of an `rcl_timer_t`, including managing its dependencies
	/// Manage the lifecycle of an `rcl_timer_t`, and its dependencies

	timer.set_period(new_period.clone());
	timer.set_period(new_period);

	let ret = unsafe { rcl_timer_get_time_since_last_call(&mut *timer, &mut elapsed_time) };
	let ret = unsafe { rcl_timer_get_time_since_last_call(& *timer, &mut elapsed_time) };

	rcl_timer_get_time_until_next_call(&mut *timer, &mut remaining_time).ok()?;
	rcl_timer_get_time_until_next_call(& *timer, &mut remaining_time).ok()?;

	let ret = unsafe { rcl_timer_is_ready(&mut *timer, &mut is_ready) };
	let ret = unsafe { rcl_timer_is_ready(& *timer, &mut is_ready) };

Add ROS2 timers #4

Are you sure you want to change the base?

Add ROS2 timers #4

Conversation

imdexsam commented Oct 28, 2024

imdex-matthew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imdexsam Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey commented Nov 2, 2024

imdexsam commented Nov 4, 2024

geoff-imdex left a comment

Choose a reason for hiding this comment

imdexsam Nov 1, 2024 •

edited

Loading