Implement validity checks for rclcpp::Clock

[methylDragon]

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.

[methylDragon]

Latest build

• Linux
• Linux-aarch64
• Windows

[methylDragon]

Changes incorporated!

[methylDragon]

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

@ros-pull-request-builder retest this please

[fujitatomoya]

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

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?

[fujitatomoya]

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]

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

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: https://github.com/ros2/rcl/pull/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.

[methylDragon]

Hey @fujitatomoya , with the merge of https://github.com/ros2/rcl/pull/1018 , the validity check is now codified in rcl! I think we can merge this PR once CI goes green, what do you think?

[methylDragon]

@ros-pull-request-builder retest this please

[methylDragon]

This PR has been updated to use this PR instead:

• https://github.com/ros2/rcl/pull/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]

@ros-pull-request-builder retest this please

[fujitatomoya]

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

[methylDragon]

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

[methylDragon]

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

@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 https://github.com/ros2/rcl/pull/1021 only introduces new free functions, which I believe is also ABI stable

[fujitatomoya]

"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]

@mergifyio backport humble

[mergify[bot]]

backport humble

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

• [ ] sender-permission>=write

[methylDragon]

@Mergifyio backport humble

[mergify[bot]]

backport humble

✅ Backports have been created

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