Implement validity checks for rclcpp::Clock #2040

methylDragon · 2022-11-01T21:28:05Z

This PR implements the analogues of the isValid and waitForValid methods in rclcpp.

~~I also added the necessary tests.~~

~~Note: In this case, I added a special case for treating RCL_CLOCK_UNINITIALIZED as invalid as well, since there isn't an analogous clock type in the ROS 1 API.~~

Pinging @sloretz for visibility.

Edit: Waiting on:

Implement rcl_clock_time_started rcl#1021

methylDragon · 2022-11-01T21:33:10Z

Latest build

Linux
Linux-aarch64
Windows

rclcpp/src/rclcpp/clock.cpp

rclcpp/include/rclcpp/clock.hpp

rclcpp/test/rclcpp/test_time.cpp

methylDragon · 2022-11-01T22:53:25Z

Changes incorporated!

rclcpp/include/rclcpp/clock.hpp

rclcpp/test/rclcpp/test_time.cpp

sloretz

LGTM with green CI!

fujitatomoya

@methylDragon thanks for the contribution.

Could you explain the actual problem that you meet to address with this PR? I am not really following the what needs to be done here, thanks in advance!

rclcpp/src/rclcpp/clock.cpp

rclcpp/include/rclcpp/clock.hpp

rclcpp/src/rclcpp/clock.cpp

methylDragon · 2022-11-02T23:51:30Z

@methylDragon thanks for the contribution.

Could you explain the actual problem that you meet to address with this PR? I am not really following the what needs to be done here, thanks in advance!

Hey Tomoya (@fujitatomoya), thanks for the reviews (and the exception docs catches)! This PR is supposed to add rclcpp analogues of the isValid and waitForValid methods (in ROS 1 time), used for checking if a clock's time is "valid"/"initialized" and so codifying that notion (from the design docs.)

I'm upstreaming this change because it came up in one of my porting efforts.

Notably, this notion of validity is separate from the rcl notion, since it also includes the fact that time 0 is also invalid (that is, it applies to the time reflected by the clock.)

methylDragon · 2022-11-03T07:08:39Z

@ros-pull-request-builder retest this please

fujitatomoya · 2022-11-03T17:21:59Z

@methylDragon thank you for iterating with me.

If time has not been set it will return zero if nothing has been received. A time value of zero should be considered an error meaning that time is uninitialized.

https://design.ros2.org/articles/clock_and_time.html, says above. thanks for pointing out the design.

Notably, this notion of validity is separate from the rcl notion, since it also includes the fact that time 0 is also invalid (that is, it applies to the time reflected by the clock.)

this becomes my question that why these notions are different? by design, rcl_clock_valid should return false if the time is zero? and then we can call rcl_clock_valid from rclcpp and rclpy?

@clalancette @ivanpauno @wjwwood @sloretz what do you think?

clalancette · 2022-11-03T17:28:25Z

this becomes my question that why these notions are different?

Yes, this is a good question.

It turns out that ROS has been considering time==0 as invalid since practically forever. It is not ideal, since time==0 is actually valid; it is just the start of time, not invalid time. If we were totally redesigning this today, I think we might designate something like -1 as invalid, with 0 just being the start of time. But if we did that now, we'd risk subtly breaking a lot of simulation applications that made some assumptions about past behavior. So while it is not ideal, I think we should continue to treat time==0 as invalid.

by design, rcl_clock_valid should return false if the time is zero? and then we can call rcl_clock_valid from rclcpp and rclpy?

But this is a good question; it would be nice if this policy were encoded in the rcl layer, and then just inherited by the client libraries. @methylDragon Do you think that is feasible?

rclcpp/src/rclcpp/clock.cpp

fujitatomoya · 2022-11-03T17:44:27Z

It turns out that ROS has been considering time==0 as invalid since practically forever. It is not ideal, since time==0 is actually valid; it is just the start of time, not invalid time. If we were totally redesigning this today, I think we might designate something like -1 as invalid, with 0 just being the start of time.

agree on this. I was wondering the same thing, thanks for pointing this out.

methylDragon · 2022-11-03T18:57:06Z

@clalancette

But this is a good question; it would be nice if this policy were encoded in the rcl layer, and then just inherited by the client libraries. @methylDragon Do you think that is feasible?

It'll be nice to put this in the rcl layer, though I'm not too sure how to do it (well, more precisely, how to name the methods without causing API/ABI breaks...)

We need a distinct way to check if the members of a clock are initialized (currently what rcl_clock_valid() is doing), since that gets used by the client libraries (e.g. in the ros_time_active() method, which then feeds into the clock setting methods (well, actually that seems to be the only place...)). Checking for validity in the rcl_clock_valid() sense is useful because it lets us check if a clock can even become valid (in the non-zero time sense) or not.

So if I were to add it into the rcl layer, I need to add a new method. The problem is:

I either need to change the current rcl_clock_valid()'s name and then have this new method be the rcl_clock_valid(), necessitating changes across every client library that uses the method. But I'm pretty sure this will break ABI/API, since the names will change.
Or I need to add a new method, but then it'd be confusing because rcl_clock_valid() would not be encoding the notion of validity in the design docs... Is there a better method name for the new method I can use?

If we can't resolve the naming issue, I think it'd be more intuitive to place it in the client libraries? Or maybe I'm missing something :V

Alternatively, do I even need to care about breaking ABI/API if I'm implementing this for rolling, or especially given that the scope of downstream use is so small :o

methylDragon · 2022-11-08T03:01:38Z

I just had an idea, I will create an rcl_time_valid() method in rcl to encode the notion of non-zero time validity, and use it alongside rcl_clock_valid() here.

Implemented here: ros2/rcl#1018

I will update this PR to use that new implementation in the rcl layer.

Edit: Updated. Builds and tests pass locally with that rcl PR's changes.

ivanpauno · 2022-11-08T14:34:02Z

rclcpp/include/rclcpp/clock.hpp

+   */
+  RCLCPP_PUBLIC
+  bool
+  wait_for_valid(Context::SharedPtr context = contexts::get_global_default_context());


what's the use case of wait_for_valid()?
It seems like a strange feature to me

It'll block till the first valid time. In most cases this will be waiting for sim to start (and time to march forward)

@ivanpauno friendly ping on this comment.

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon · 2022-12-05T10:41:50Z

This PR has been updated to use this PR instead:

Implement rcl_clock_time_started rcl#1021 instead
This PR will fail until that PR is merged

It doesn't make sense for a non-zero time point to be invalid, so the names and implementations have been changed to make this PR about checking for whether a clock has started or not

methylDragon · 2022-12-05T22:23:30Z

@ros-pull-request-builder retest this please

fujitatomoya · 2022-12-05T23:37:37Z

@methylDragon thanks for iterating with patience, so this has been update and ready to review, right?

methylDragon · 2022-12-06T00:44:00Z

@methylDragon thanks for iterating with patience, so this has been update and ready to review, right?

Hey @fujitatomoya , this is ready, yep! Though it'll take awhile for rcl to get released so CI works...

Signed-off-by: methylDragon <methylDragon@gmail.com>

rclcpp/src/rclcpp/clock.cpp

rclcpp/include/rclcpp/clock.hpp

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon · 2022-12-14T02:10:16Z

Linux
Linux-aarch64
Windows

emersonknapp · 2023-06-12T21:49:40Z

@methylDragon Can we backport this to Humble? I believe "new non-virtual methods on a class" is an ABI-stable change, so it seems to me like the answer is yes. The dependency ros2/rcl#1021 only introduces new free functions, which I believe is also ABI stable

fujitatomoya · 2023-06-12T22:45:35Z

"new non-virtual methods on a class" is an ABI-stable change

yeah, correct. i believe backport should be fine. besides, this is something we add, there is no user application behavior change.

emersonknapp · 2023-06-12T23:09:11Z

@Mergifyio backport humble

mergify · 2023-06-12T23:09:13Z

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

sender-permission>=write

methylDragon · 2023-06-12T23:21:12Z

@Mergifyio backport humble

mergify · 2023-06-12T23:21:16Z

backport humble

✅ Backports have been created

#2210 Implement validity checks for rclcpp::Clock (backport #2040) has been created for branch humble

(cherry picked from commit c091fe1)

(cherry picked from commit c091fe1) Co-authored-by: methylDragon <methylDragon@gmail.com>

…rt 0.15.7 (#3) * Implement validity checks for rclcpp::Clock (ros2#2040) (ros2#2210) (cherry picked from commit c091fe1) Co-authored-by: methylDragon <methylDragon@gmail.com> * rclcpp: check rcl version [humble] Signed-off-by: tomoya.kimura <tomoya.kimura@tier4.jp> --------- Signed-off-by: tomoya.kimura <tomoya.kimura@tier4.jp> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: methylDragon <methylDragon@gmail.com>

methylDragon requested a review from sloretz November 1, 2022 21:28

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 92a4666 to 53779d0 Compare November 1, 2022 21:40

sloretz reviewed Nov 1, 2022

View reviewed changes

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 58028ea to 869e4ed Compare November 1, 2022 22:53

methylDragon requested a review from sloretz November 1, 2022 22:53

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 869e4ed to 74282af Compare November 1, 2022 23:32

sloretz reviewed Nov 2, 2022

View reviewed changes

rclcpp/include/rclcpp/clock.hpp Outdated Show resolved Hide resolved

rclcpp/test/rclcpp/test_time.cpp Outdated Show resolved Hide resolved

methylDragon force-pushed the ch3/clock-wait-for-valid branch 2 times, most recently from 937c213 to 23271a6 Compare November 2, 2022 00:28

sloretz approved these changes Nov 2, 2022

View reviewed changes

methylDragon force-pushed the ch3/clock-wait-for-valid branch 2 times, most recently from eb680b4 to 6a54627 Compare November 2, 2022 03:49

fujitatomoya requested changes Nov 2, 2022

View reviewed changes

methylDragon requested a review from fujitatomoya November 3, 2022 00:33

fujitatomoya requested changes Nov 3, 2022

View reviewed changes

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

methylDragon force-pushed the ch3/clock-wait-for-valid branch from b431888 to 6face35 Compare November 3, 2022 18:56

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 2d311e9 to 5d6d65b Compare November 3, 2022 21:13

methylDragon mentioned this pull request Nov 3, 2022

fuse -> ROS 2 : Port Time locusrobotics/fuse#283

Merged

This was referenced Nov 8, 2022

Implement validity checks for time points methylDragon/rcl#1

Closed

Implement validity checks for time points ros2/rcl#1018

Merged

ivanpauno reviewed Nov 8, 2022

View reviewed changes

Use rcl_time_point_valid

ab95f66

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon force-pushed the ch3/clock-wait-for-valid branch from a7eec28 to ab95f66 Compare December 5, 2022 10:09

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 407a4cc to 36aad12 Compare December 5, 2022 19:41

Use rcl_clock_time_started instead

25b8a81

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon force-pushed the ch3/clock-wait-for-valid branch from 36aad12 to 25b8a81 Compare December 6, 2022 00:45

fujitatomoya requested changes Dec 6, 2022

View reviewed changes

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

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

Implement calculation for wait duration

1df3675

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon requested a review from fujitatomoya December 14, 2022 19:33

fujitatomoya approved these changes Dec 20, 2022

View reviewed changes

methylDragon merged commit c091fe1 into ros2:rolling Dec 20, 2022

methylDragon deleted the ch3/clock-wait-for-valid branch December 20, 2022 02:19

sloretz mentioned this pull request Dec 28, 2022

Use rclcpp::Clock::wait_until_started locusrobotics/fuse#303

Merged

methylDragon mentioned this pull request Mar 9, 2023

Time: Enable operators with different clock sources if one of the times is zero. #2113

Open

emersonknapp mentioned this pull request Jun 12, 2023

[humble] When using sim time, wait for /clock before beginning recording (backport #1378) ros2/rosbag2#1392

Merged

mergify bot pushed a commit that referenced this pull request Jun 12, 2023

Implement validity checks for rclcpp::Clock (#2040)

f87d548

(cherry picked from commit c091fe1)

mergify bot mentioned this pull request Jun 12, 2023

Implement validity checks for rclcpp::Clock (backport #2040) #2210

Merged

fujitatomoya pushed a commit that referenced this pull request Jun 14, 2023

Implement validity checks for rclcpp::Clock (#2040) (#2210)

613d192

(cherry picked from commit c091fe1) Co-authored-by: methylDragon <methylDragon@gmail.com>

tkimura4 pushed a commit to tier4/rclcpp that referenced this pull request Aug 17, 2023

Implement validity checks for rclcpp::Clock (ros2#2040) (ros2#2210)

9db1876

(cherry picked from commit c091fe1) Co-authored-by: methylDragon <methylDragon@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement validity checks for rclcpp::Clock #2040

Implement validity checks for rclcpp::Clock #2040

methylDragon commented Nov 1, 2022 •

edited

Loading

methylDragon commented Nov 1, 2022 •

edited

Loading

methylDragon commented Nov 1, 2022

sloretz left a comment

fujitatomoya left a comment

methylDragon commented Nov 2, 2022 •

edited

Loading

methylDragon commented Nov 3, 2022

fujitatomoya commented Nov 3, 2022

clalancette commented Nov 3, 2022

fujitatomoya commented Nov 3, 2022

methylDragon commented Nov 3, 2022 •

edited

Loading

methylDragon commented Nov 8, 2022 •

edited

Loading

ivanpauno Nov 8, 2022

methylDragon Nov 8, 2022

fujitatomoya Dec 20, 2022

methylDragon commented Dec 5, 2022 •

edited

Loading

methylDragon commented Dec 5, 2022

fujitatomoya commented Dec 5, 2022

methylDragon commented Dec 6, 2022

methylDragon commented Dec 14, 2022

emersonknapp commented Jun 12, 2023

fujitatomoya commented Jun 12, 2023

emersonknapp commented Jun 12, 2023

mergify bot commented Jun 12, 2023

methylDragon commented Jun 12, 2023

mergify bot commented Jun 12, 2023 •

edited

Loading

Implement validity checks for rclcpp::Clock #2040

Implement validity checks for rclcpp::Clock #2040

Conversation

methylDragon commented Nov 1, 2022 • edited Loading

methylDragon commented Nov 1, 2022 • edited Loading

methylDragon commented Nov 1, 2022

sloretz left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

methylDragon commented Nov 2, 2022 • edited Loading

methylDragon commented Nov 3, 2022

fujitatomoya commented Nov 3, 2022

clalancette commented Nov 3, 2022

fujitatomoya commented Nov 3, 2022

methylDragon commented Nov 3, 2022 • edited Loading

methylDragon commented Nov 8, 2022 • edited Loading

ivanpauno Nov 8, 2022

Choose a reason for hiding this comment

methylDragon Nov 8, 2022

Choose a reason for hiding this comment

fujitatomoya Dec 20, 2022

Choose a reason for hiding this comment

methylDragon commented Dec 5, 2022 • edited Loading

methylDragon commented Dec 5, 2022

fujitatomoya commented Dec 5, 2022

methylDragon commented Dec 6, 2022

methylDragon commented Dec 14, 2022

emersonknapp commented Jun 12, 2023

fujitatomoya commented Jun 12, 2023

emersonknapp commented Jun 12, 2023

mergify bot commented Jun 12, 2023

❌ Command disallowed due to command restrictions in the Mergify configuration.

methylDragon commented Jun 12, 2023

mergify bot commented Jun 12, 2023 • edited Loading

✅ Backports have been created

methylDragon commented Nov 1, 2022 •

edited

Loading

methylDragon commented Nov 1, 2022 •

edited

Loading

methylDragon commented Nov 2, 2022 •

edited

Loading

methylDragon commented Nov 3, 2022 •

edited

Loading

methylDragon commented Nov 8, 2022 •

edited

Loading

methylDragon commented Dec 5, 2022 •

edited

Loading

mergify bot commented Jun 12, 2023 •

edited

Loading