rclcpp Add clock type to node

Signed-off-by: Jeffery Hsu [email protected] This PR adds the option for selecting clock source in Node/LifecycleNode thru NodeOptions. A similar option is already present in rclcpp::Timer but it is currently missing in Nodes/LifecycleNode. There's currently no way to set Node::now() and all other time-related functions to use clock sources other than ROS_TIME.

Aug 08 '22 21:08 jefferyyjhsu

CI

Linux
Linux-aarch64
Windows

Aug 15 '22 09:08 alsora

NOTE: this PR is from iRobot. It looks good to me, but I would like also feedbacks from other reviewers

Aug 15 '22 09:08 alsora

Forgive me if I misunderstand this PR, but I think being able to have Node::now() driven by sim time is currently possible with setting use_sim_time=true parameter on the node. After this PR lands you should be able to create both sim and wall timers with Node::create_timer() and Node::create_wall_timer(). Is there a use case beyond these that I'm missing?

Aug 20 '22 00:08 asymingt

@asymingt the purpose of this PR is to allow to set the node clock type. There are 3 types of clocks in ros: real time, monotonic and "ros".

Currently, you must use a clock of type "ros" for a node. A clock of this type uses a real time clock and also the clock topic if available (and if use_sim_time is set to true).

There are a lot of applications where a real time clock is not suitable and it is required to use a monotonic clock (and as said before, the "ros" clock is not monotoni)

Aug 20 '22 07:08 alsora

@asymingt the purpose of this PR is to allow to set the node clock type. There are 3 types of clocks in ros: real time, monotonic and "ros".

Currently, you must use a clock of type "ros" for a node. A clock of this type uses a real time clock and also the clock topic if available (and if use_sim_time is set to true).

There are a lot of applications where a real time clock is not suitable and it is required to use a monotonic clock (and as said before, the "ros" clock is not monotoni)

Understood. Thank you, and sorry for the distraction.

Aug 20 '22 16:08 asymingt

New CI:

Linux
Linux-aarch64
Windows

Aug 30 '22 11:08 alsora

@fujitatomoya I think that comments have been addressed. Anything else to do?

Aug 30 '22 11:08 alsora

either @iuhilnehc-ynos or @Barry-Xu-2018 , could you help us review on this?

I was thinking that, i may be missing something.

TimeSource and ClockState are dedicated to manage the ROS simulation time. User must be able to switch on/off simulation time afterward.
Having Node::Option::clock_type can be used to default clock type creating timer and so on.

CC: @alsora @jefferyyjhsu

Aug 30 '22 21:08 fujitatomoya

@fujitatomoya I agree on having the clock type to be used as the default clock type for timers... I was assuming that it was already the case.

About simulated time: users should be able to turn it on/off UNLESS they manually set the node clock to monotonic... In that case simulated time is not currently expected to work and it should throw an exception/error somewhere.

Aug 31 '22 11:08 alsora

EDIT: ignore this CI! I made a mistake when configuring it!

CI

Linux
Linux-aarch64
Windows

Sep 01 '22 16:09 alsora

Linux
Linux-aarch64
Windows

Sep 02 '22 10:09 alsora

Hi, are there any more changes that we should address before proceeding with this PR? I see that there are some failures in windows tests, are these known problems?

Sep 23 '22 08:09 alsora

@alsora @jefferyyjhsu sorry to be late, just left couple of comments.

Sep 23 '22 15:09 fujitatomoya

@ivanpauno can you also take a look at this, this changes behavior a bit.

CC: @clalancette

Sep 24 '22 16:09 fujitatomoya

Changes look good to me, I just left some minor comments. New CI

Linux
Linux-aarch64
Windows

Sep 24 '22 17:09 alsora

CI seems to run with the wrong settings.

Sep 26 '22 09:09 iuhilnehc-ynos

My bad! I copied the repos file in the wrong field..! New CI

Linux
Linux-aarch64
Windows

Sep 26 '22 09:09 alsora

Hi, anything else to address before merging this PR?

Oct 03 '22 11:10 alsora

@fujitatomoya @iuhilnehc-ynos is there anything else we should address before merging this PR?

Nov 07 '22 11:11 alsora

@alsora some comments are not resolved or addressed yet?

I am okay to go with this, but requesting another maintainer to take a look at this, since it changes Node API and meaning of NodeClock::ros_clock_.

CC: @ivanpauno @clalancette @wjwwood

Nov 07 '22 18:11 fujitatomoya

@fujitatomoya, I went back and resolved some of the minor comments that @alsora and I discussed internally and decided to keep it the same way. Please let me know if I am still missing anything. Thanks!

Nov 07 '22 22:11 jefferyyjhsu

@jefferyyjhsu thanks for iterating with us, final CI below.