Timers #480
Timers #480
Conversation
Signed-off-by: Michael X. Grey <[email protected]>
mxgrey
force-pushed
the
worker_with_timers
branch
from
a2af202 to
2d20c4e
Compare
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
|
@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:
I've had to resort to manually deleting the 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 😞 |
Signed-off-by: Michael X. Grey <[email protected]>
|
@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 |
|
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. |
|
See this PR: #495 |
| /// Gets the period of the timer | ||
| pub fn get_timer_period(&self) -> Result<Duration, RclrsError> { | ||
| let mut timer_period_ns = 0; | ||
| unsafe { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
| /// Checks whether the timer is canceled or not | ||
| pub fn is_canceled(&self) -> Result<bool, RclrsError> { | ||
| let mut is_canceled = false; | ||
| unsafe { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
| /// Retrieves the time since the last call to the callback | ||
| pub fn time_since_last_call(&self) -> Result<Duration, RclrsError> { | ||
| let mut time_value_ns: i64 = 0; | ||
| unsafe { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
| /// Retrieves the time until the next call of the callback | ||
| pub fn time_until_next_call(&self) -> Result<Duration, RclrsError> { | ||
| let mut time_value_ns: i64 = 0; | ||
| unsafe { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
rclrs/src/timer.rs
Outdated
| /// Resets the timer. | ||
| pub fn reset(&self) -> Result<(), RclrsError> { | ||
| let mut rcl_timer = self.handle.rcl_timer.lock().unwrap(); | ||
| unsafe { rcl_timer_reset(&mut *rcl_timer) }.ok() |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
|
|
||
| /// Checks if the timer is ready (not canceled) | ||
| pub fn is_ready(&self) -> Result<bool, RclrsError> { | ||
| let is_ready = unsafe { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
rclrs/src/timer.rs
Outdated
| /// except to keep track of when the timer has been called. | ||
| fn rcl_call(&self) -> Result<(), RclrsError> { | ||
| let mut rcl_timer = self.handle.rcl_timer.lock().unwrap(); | ||
| unsafe { rcl_timer_call(&mut *rcl_timer) }.ok() |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
| } | ||
|
|
||
| impl<Scope: WorkScope> RclPrimitive for TimerExecutable<Scope> { | ||
| unsafe fn execute(&mut self, payload: &mut dyn Any) -> Result<(), RclrsError> { |
There was a problem hiding this comment.
Shouldn't this have a safety comment?
| name = "examples_timer_demo" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
There was a problem hiding this comment.
Maybe good to have a bin here so that we can run it through ROS2 command?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ahhh gotcha.
And yes exactly like that. naming consistency is good :)
| let _timer = worker.create_timer_repeating(timer_period, move |count: &mut usize| { | ||
| *count += 1; | ||
| println!( | ||
| "Drinking 🧉 for the {}th time every {:?}.", |
There was a problem hiding this comment.
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()
| @@ -893,6 +895,214 @@ impl NodeState { | |||
| ) | |||
| } | |||
|
|
|||
| /// Create a [`Timer`] with a repeating callback. | |||
There was a problem hiding this comment.
I was first confused on why there were some much commented out lines, but then I realized that this was for the documentation. Nice!
| /// The executor needs to be [spinning][1] for a timer's callback to be triggered. | ||
| /// | ||
| /// Timers can be created by a [`Node`] using one of these methods: | ||
| /// - [`NodeState::create_timer_repeating`][2] |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
|
CI is currently failing because of an upstream issue which is fixed by ros2-rust/rosidl_runtime_rs#13 |
This PR splits the
timer.rsmodule out of #446 so it can be reviewed separately after #446 is merged.