Add support for clocks

[nnmm]

See https://github.com/ros2/rcl/blob/master/rcl/include/rcl/time.h

Since this is a complex feature, it would be good to have some prior experience with the code base and/or seek early-stage feedback from other contributors.

[DS3a]

Quick clarification... I need to create a time.rs which has the same functions that are in time.c, using the bindings from rcl_bindings_generated.rs right?

[DS3a]

Can someone let me know if I'm going in the right direction? so that I can make changes in case I'm wrong?

[nnmm]

@DS3a appreciate the question. So, you should use the functions from time.c, via our generated bindings, but rclrs should not just wrap the functions 1:1. That would not be an optimal API. For instance, you should make use of the Rust standard library facilities (like the std::time::Duration type). I strongly recommend to familiarize yourself with how the time functionality from rcl is used in rclcpp and/or rclpy first. That should hopefully give you a more concrete idea.

I'd also recommend to tackle this issue in smaller steps, not port the full functionality at once.

Hope this helps. Let us know if you have further questions!

[DS3a]

Okay, that makes sense... I'll look into rclcpp::Time before I start working with time.rs. Thank you for the answer, I shall let you know if any more pop up

[esteve]

@DS3a to understand better how time is handled in ROS2, you can also have a look at https://design.ros2.org/articles/clock_and_time.html

Regarding types, using std::time::Duration for example, might be possible to use, but I encourage you to use the native ROS2 types (e.g. rclrs::Duration, wrapping rcl_duration_t) as it'll provide better integration with the rest of ROS2. There may be conversion utilities between ROS2 and Rust types to make their usage easier, though. Have a look at the APIs of both rclcpp and rclpy to get inspiration.

[DS3a]

@DS3a to understand better how time is handled in ROS2, you can also have a look at https://design.ros2.org/articles/clock_and_time.html

Regarding types, using std::time::Duration for example, might be possible to use, but I encourage you to use the native ROS2 types (e.g. rclrs::Duration, wrapping rcl_duration_t) as it'll provide better integration with the rest of ROS2. There may be conversion utilities between ROS2 and Rust types to make their usage easier, though. Have a look at the APIs of both rclcpp and rclpy to get inspiration.

Got it... I did start looking into it to get ideas as to how to implement it in rust... I've started building duration.rs which is a dependancy for time.rs. and both of them are a dependancy for clock.

Here is the file. I have yet to add implementations for the operators which have to be performed on it... I thought of starting with the constructors and stuff. Now rust doesn't allow function overloading which could have been used for the new() (line 26) function in impl Duration, so I created an enum; DurationFrom (line 8) to supply arguments based on the users requirements. The contributors documentation was pretty clear about the alphabetic order of the impl blocks, but wasn't really clear about the constructors... Is this the right protocol? or is there anything else? I have skimmed through the other code and none of the struct functions required overloading. Should I stick to this enum or create separate explicit functions for each type of argument(s) I want the user to be able to supply?

[nnmm]

Regarding types, using std::time::Duration for example, might be possible to use, but I encourage you to use the native ROS2 types (e.g. rclrs::Duration, wrapping rcl_duration_t) as it'll provide better integration with the rest of ROS2.

Could you explain in more detail? In what instance would it be beneficial to not use std::time::Duration?

[DS3a]

Regarding types, using std::time::Duration for example, might be possible to use, but I encourage you to use the native ROS2 types (e.g. rclrs::Duration, wrapping rcl_duration_t) as it'll provide better integration with the rest of ROS2.

Could you explain in more detail? In what instance would it be beneficial to not use std::time::Duration?

I don't think there is an instance where it wouldn't be beneficial... although if I were to nitpick; using it would require type casting from u64 to i32 which I believe adds SNR which isn't good. However it isn't that huge an issue, as the noise would be negligible.

Also, apologies... I might have been unclear with my question. I didn't want to avoid std::time::Duration. I wanted to provide users with more options to construct the Duration object. This is what has been done in rclcpp, using function overloading. And hence I have created an enum to act as arguments. One of the options do permit the use of std::time::Duration. Should i keep that as the only option and create separate functions for users who might want to avoid it? or is just the one constructor enough (as done in rclpy)?

Also... In the future (for some other functionality) where it might be mandatory to prove different ways to initialize, should I create multiple functions with different names? or create an enum to act as an argument?

[esteve]

@DS3a I suggest you start implementing support for clocks as you think is appropriate and then we can provide you with feedback, I'm really not a fan micromanaging contributions, it just creates a lot of interference. But of course, if you need advice, we're more than happy to help 🙂

[DS3a]

Okay... I'll start implementing it with minimal interference. I'll start adding examples where I think further explanation is required. I'm guessing that works. Thanks. I shall post questions if I have any queries/require advice.

[DS3a]

I've finished implementing time.rs, as well as duration.rs, I have used std::time::Duration as well as provided options to use other ways. while writing clock.rs I came across jump_threshold_t which requires rcl_duration_t to be negative. Negative durations aren't supported on std::time::Duration, which requires the arguments to be unsigned ints, however the rcl_types (time related) are all signed integers. Should I remove the std::time::Duration implementations? or make a wrapper around them to support negative integers?
I am using the std library to convert seconds to nanoseconds and the other way round, and I will try to use it to make the ros2 bindings as much as I can.

Although, I don't know how far that would be going? Is it common practice to write wrappers around standard libraries? or rewrite them to suit the users' needs?

[DS3a]

Here's the repository I forked from here to add support for time: https://github.com/DS3a/ros2_rust.git

can someone let me know if I'm going in the right direction?
the files to checkout are time.rs, duration.rs and clock.rs

[nnmm]

Hey @DS3a, I'll take a look today.

[esteve]

@DS3a could you submit a PR against ros2-rust? Even if it's a draft. It'll be easier for us to check the differences and to provide feedback. Thanks!

[esteve]

@DS3a also, before you submit the PR, I'd advice you rename the branch where you're making the changes. Right now your changes are in the main branch of your fork, which is not a very descriptive name.

[DS3a]

@DS3a could you submit a PR against ros2-rust? Even if it's a draft. It'll be easier for us to check the differences and to provide feedback. Thanks!

okay... gimme a sec

[esteve]

okay... gimme a sec

Thanks 🙂

[DS3a]

@DS3a also, before you submit the PR, I'd advice you rename the branch where you're making the changes. Right now your changes are in the main branch of your fork, which is not a very descriptive name.

ahhh... makes sense... I'll make another pr

[esteve]

ahhh... makes sense... I'll make another pr

Perfect! Thank you so much