Add clock type to node_options #1982

jefferyyjhsu · 2022-08-08T21:59:18Z

Signed-off-by: Jeffery Hsu jefferyyjhsu@gmail.com
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.

alsora · 2022-08-15T09:36:32Z

CI

Linux
Linux-aarch64
Windows

alsora · 2022-08-15T09:38:06Z

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

fujitatomoya

lgtm with test for NodeOption

rclcpp/src/rclcpp/node_interfaces/node_clock.cpp

rclcpp/include/rclcpp/node_options.hpp

asymingt · 2022-08-20T00:56:28Z

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?

alsora · 2022-08-20T07:49:25Z

@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)

asymingt · 2022-08-20T16:00:24Z

@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.

alsora · 2022-08-30T11:20:04Z

New CI:

Linux
Linux-aarch64
Windows

alsora · 2022-08-30T11:20:55Z

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

rclcpp/test/rclcpp/test_node.cpp

fujitatomoya · 2022-08-30T21:36:15Z

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

iuhilnehc-ynos

minor nitpick

rclcpp/src/rclcpp/node_options.cpp

alsora · 2022-08-31T11:07:36Z

@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.

alsora · 2022-09-01T16:29:18Z

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

CI

Linux
Linux-aarch64
Windows

alsora · 2022-09-02T10:46:56Z

Linux
Linux-aarch64
Windows

alsora · 2022-09-23T08:09:46Z

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?

fujitatomoya · 2022-09-23T15:45:30Z

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

fujitatomoya · 2022-09-24T16:13:11Z

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

CC: @clalancette

rclcpp/src/rclcpp/time_source.cpp

fujitatomoya · 2022-11-08T22:36:38Z

@jefferyyjhsu can you address DCO issue?

jefferyyjhsu · 2022-11-08T22:39:12Z

@fujitatomoya , yeah. Sorry, I did remember to fix it by rebasing it locally but forgot to finish it before pushing.

alsora · 2022-11-22T15:51:42Z

New CI

Linux
Linux-aarch64
Windows

alsora · 2022-11-24T19:46:03Z

The windows failure happens also in a completely unrelated PR #1979

ivanpauno · 2022-11-28T17:09:01Z

See #1979 (comment)

…ctor accordingly Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

…. Add test cases. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

ros_time_active_ in ClocksState::attachClock() Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Following the reasoning in c54a6f1, on_set_parameters doesn't try to enforce use_sim_time to be of boolean type explicitly. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

fujitatomoya · 2022-11-30T18:41:06Z

Windows

alsora · 2022-12-05T20:14:35Z

Running full CI.
The github check reports failures on rclcpp tests but they seem unrelated.

Linux
Linux-aarch64
Windows

alsora · 2022-12-13T13:26:03Z

Ping; can we merge this PR?

methylDragon · 2022-12-15T04:40:18Z

Erm.. I'm getting the same issue locally as the Rpr job
https://build.ros2.org/job/Rpr__rclcpp__ubuntu_jammy_amd64/469/testReport/junit/rclcpp/TestTimeSource/callbacks/

Something is causing the callback test to fail, though it happens sporadically
When the test is run standalone there are no issues, but when it's run together something breaks. (It could just be a flaky test though)

methylDragon · 2022-12-15T04:41:43Z

Pinging @jefferyyjhsu

jefferyyjhsu · 2022-12-19T05:27:46Z

@methylDragon, sorry for the delay. I tried reproducing the error with colcon test --packages-select rclcpp but failed. How are you running the tests?
In rclcpp/test/rclcpp/test_time_source.cpp, I only added 2 isolated test cases.

TEST(TimeSource, valid_clock_type_for_sim_time)
{
rclcpp::init(0, nullptr);

rclcpp::NodeOptions options;
auto node = std::make_sharedrclcpp::Node("my_node", options);
EXPECT_TRUE(
node->set_parameter(
rclcpp::Parameter(
"use_sim_time", rclcpp::ParameterValue(
true))).successful);
rclcpp::shutdown();
}

TEST(TimeSource, invalid_clock_type_for_sim_time)
{
rclcpp::init(0, nullptr);

rclcpp::NodeOptions options;
options.clock_type(RCL_STEADY_TIME);
auto node = std::make_sharedrclcpp::Node("my_node", options);
EXPECT_FALSE(
node->set_parameter(
rclcpp::Parameter(
"use_sim_time", rclcpp::ParameterValue(
true))).successful);
rclcpp::shutdown();
}

methylDragon · 2022-12-19T18:52:44Z

@methylDragon, sorry for the delay. I tried reproducing the error with colcon test --packages-select rclcpp but failed. How are you running the tests? In rclcpp/test/rclcpp/test_time_source.cpp, I only added 2 isolated test cases.

TEST(TimeSource, valid_clock_type_for_sim_time)
{
rclcpp::init(0, nullptr);
rclcpp::NodeOptions options;
auto node = std::make_sharedrclcpp::Node("my_node", options);
EXPECT_TRUE(
node->set_parameter(
rclcpp::Parameter(
"use_sim_time", rclcpp::ParameterValue(
true))).successful);
rclcpp::shutdown();
}
TEST(TimeSource, invalid_clock_type_for_sim_time)
{
rclcpp::init(0, nullptr);
rclcpp::NodeOptions options;
options.clock_type(RCL_STEADY_TIME);
auto node = std::make_sharedrclcpp::Node("my_node", options);
EXPECT_FALSE(
node->set_parameter(
rclcpp::Parameter(
"use_sim_time", rclcpp::ParameterValue(
true))).successful);
rclcpp::shutdown();
}

Hey there! I am also running it with colcon test --packages-select rclcpp. It seems like this issue only happens randomly though, so I suspect it's a flaky test or a previously existing flaky test that's excacerbated by a change here? Or I could be wrong and its completely unrelated to this PR :o

It definitely shows up on the buildfarm though; though sporadically

ivanpauno · 2022-12-19T18:58:04Z

Erm.. I'm getting the same issue locally as the Rpr job
https://build.ros2.org/job/Rpr__rclcpp__ubuntu_jammy_amd64/469/testReport/junit/rclcpp/TestTimeSource/callbacks/

Something is causing the callback test to fail, though it happens sporadically

I think that test has been flaky for a long time (before this PR was merged), I don't think the failure is related to these changes.

methylDragon · 2022-12-19T19:35:43Z

Ah, okay, disregard my comment then! Sorry for the noise 🙇

* add clock type to node_options and change node/lifecycle_node constructor accordingly Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * Modify TimeSource::NodeState class to work with different clock types. Add test cases. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * Change on_parameter_event to output RCLCPP_ERROR and check ros_time_active_ in ClocksState::attachClock() Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * Remove a redundant include Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * Disallow setting use_sim_time to true if a clock_type can't support it. Following the reasoning in c54a6f1, on_set_parameters doesn't try to enforce use_sim_time to be of boolean type explicitly. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * minior style change Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> * remove reason string for successful result Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com> Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch 2 times, most recently from 7eb5651 to d22dfac Compare August 8, 2022 22:14

alsora approved these changes Aug 15, 2022

View reviewed changes

fujitatomoya reviewed Aug 16, 2022

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_clock.cpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/node_options.hpp Show resolved Hide resolved

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch 3 times, most recently from 53af9d5 to bca2d75 Compare August 18, 2022 17:29

asymingt mentioned this pull request Aug 20, 2022

Add a create_timer method to Node and LifecycleNode classes #1975

Merged

6 tasks

fujitatomoya reviewed Aug 30, 2022

View reviewed changes

rclcpp/test/rclcpp/test_node.cpp Show resolved Hide resolved

iuhilnehc-ynos reviewed Aug 31, 2022

View reviewed changes

rclcpp/src/rclcpp/node_options.cpp Outdated Show resolved Hide resolved

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch 2 times, most recently from 87e1e0b to bd57012 Compare August 31, 2022 23:55

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch 3 times, most recently from 906bbf2 to bee7cf3 Compare September 24, 2022 00:42

alsora reviewed Sep 24, 2022

View reviewed changes

rclcpp/src/rclcpp/time_source.cpp Outdated Show resolved Hide resolved

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch from 755638a to f369888 Compare November 8, 2022 22:37

alsora mentioned this pull request Nov 24, 2022

Add support for timers on reset callback #1979

Merged

jefferyyjhsu added 7 commits November 28, 2022 14:23

add clock type to node_options and change node/lifecycle_node constru…

0477dc7

…ctor accordingly Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Modify TimeSource::NodeState class to work with different clock types…

8e85b94

…. Add test cases. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Change on_parameter_event to output RCLCPP_ERROR and check

a6bd25c

ros_time_active_ in ClocksState::attachClock() Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Remove a redundant include

b2db556

Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

Disallow setting use_sim_time to true if a clock_type can't support it.

63464e2

Following the reasoning in c54a6f1, on_set_parameters doesn't try to enforce use_sim_time to be of boolean type explicitly. Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

minior style change

eb8128c

Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

remove reason string for successful result

49eeb45

Signed-off-by: Jeffery Hsu <jefferyyjhsu@gmail.com>

jefferyyjhsu force-pushed the Add-options-for-different-clock-type-in-ROS-2-node branch from f369888 to 49eeb45 Compare November 28, 2022 21:25

fujitatomoya merged commit 432bf21 into ros2:rolling Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clock type to node_options #1982

Add clock type to node_options #1982

jefferyyjhsu commented Aug 8, 2022

alsora commented Aug 15, 2022

alsora commented Aug 15, 2022

fujitatomoya left a comment

asymingt commented Aug 20, 2022 •

edited

Loading

alsora commented Aug 20, 2022 •

edited

Loading

asymingt commented Aug 20, 2022

alsora commented Aug 30, 2022

alsora commented Aug 30, 2022

fujitatomoya commented Aug 30, 2022

iuhilnehc-ynos left a comment

alsora commented Aug 31, 2022 •

edited

Loading

alsora commented Sep 1, 2022 •

edited

Loading

alsora commented Sep 2, 2022

alsora commented Sep 23, 2022 •

edited

Loading

fujitatomoya commented Sep 23, 2022

fujitatomoya commented Sep 24, 2022

fujitatomoya commented Nov 8, 2022

jefferyyjhsu commented Nov 8, 2022

alsora commented Nov 22, 2022

alsora commented Nov 24, 2022

ivanpauno commented Nov 28, 2022

fujitatomoya commented Nov 30, 2022

alsora commented Dec 5, 2022

alsora commented Dec 13, 2022

methylDragon commented Dec 15, 2022 •

edited

Loading

methylDragon commented Dec 15, 2022

jefferyyjhsu commented Dec 19, 2022

methylDragon commented Dec 19, 2022 •

edited

Loading

ivanpauno commented Dec 19, 2022

methylDragon commented Dec 19, 2022

Add clock type to node_options #1982

Add clock type to node_options #1982

Conversation

jefferyyjhsu commented Aug 8, 2022

alsora commented Aug 15, 2022

alsora commented Aug 15, 2022

fujitatomoya left a comment

Choose a reason for hiding this comment

asymingt commented Aug 20, 2022 • edited Loading

alsora commented Aug 20, 2022 • edited Loading

asymingt commented Aug 20, 2022

alsora commented Aug 30, 2022

alsora commented Aug 30, 2022

fujitatomoya commented Aug 30, 2022

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

alsora commented Aug 31, 2022 • edited Loading

alsora commented Sep 1, 2022 • edited Loading

alsora commented Sep 2, 2022

alsora commented Sep 23, 2022 • edited Loading

fujitatomoya commented Sep 23, 2022

fujitatomoya commented Sep 24, 2022

fujitatomoya commented Nov 8, 2022

jefferyyjhsu commented Nov 8, 2022

alsora commented Nov 22, 2022

alsora commented Nov 24, 2022

ivanpauno commented Nov 28, 2022

fujitatomoya commented Nov 30, 2022

alsora commented Dec 5, 2022

alsora commented Dec 13, 2022

methylDragon commented Dec 15, 2022 • edited Loading

methylDragon commented Dec 15, 2022

jefferyyjhsu commented Dec 19, 2022

methylDragon commented Dec 19, 2022 • edited Loading

ivanpauno commented Dec 19, 2022

methylDragon commented Dec 19, 2022

asymingt commented Aug 20, 2022 •

edited

Loading

alsora commented Aug 20, 2022 •

edited

Loading

alsora commented Aug 31, 2022 •

edited

Loading

alsora commented Sep 1, 2022 •

edited

Loading

alsora commented Sep 23, 2022 •

edited

Loading

methylDragon commented Dec 15, 2022 •

edited

Loading

methylDragon commented Dec 19, 2022 •

edited

Loading