fuse -> ROS 2 : Port Time

[methylDragon]

See: #276

Description

This PR ports time constructs (e.g. ros::Time, ros::Duration, timers, etc.) to ROS2 across the entire stack. I also "downgraded" to std::chrono when it made sense to.

It also introduces a new TimeStamp class (and uses it in most places) to allow for interoperability with std::chrono time points and rclcpp::Time (and to also introduce the explicit concept of uninitialized time (as distinguished from zero-initialized time)). The class is a wrapper around rclcpp::Time. Edit: This is now obsoleted.

PR Layout

I recommend looking at the individual commits when reviewing. I tried to have a commit for each type of construct.

Notes

Note that the use of rclcpp::Time as the base class means that comparisons and arithmetic between different timestamps will need to have the same clock type. They'd otherwise result in runtime errors (which is a good thing.) This means that a user-written timestamp will need to be populated with the correct clock type. I've tried to do this for the stack, but we'll only find out once we can get the stack building and tests running. Note that this will be something that downstream users implementing their own plugins have to be mindful of.

Also, some changes here require changes in how nodes are getting passed around, so expect those in a future PR. I left some comments to remind myself to port them.

Pinging @svwilliams for visibility.

[sloretz]

Do we need fuse_core::TimeStamp? I don't think the RCL_CLOCK_UNINITIALIZED in the constructor is needed anymore since 0 on any clock type will be invalid (for better or worse).\

Any conversions to chrono types can be done in free functions, but I'm not sure converting to std::chrono types is a good idea. There's no guarantee that the clock used to get the ROS time is the same clock used by std::chrono. For example, for steady time rcutils is using CLOCK_MONOTONIC_RAW while gcc's chrono implentation is using CLOCK_MONOTONIC. std::chrono::time_point<> returned by both might give incorrect comparisons because one is subject to NTP adjustments. The same could happen with system time depending on which C++ standard library implementation is used.

[BrettRD]

fuse_core::TimeStamp was mostly part of a plan to support backports to Fuse for ROS1
std::chrono isn't required, as long as there's enough feature parity that Fuse doesn't diverge substantially between ROS1 and ROS2 branches

I do worry about applications crashing when a (valid) time zero arrives

[methylDragon]

I think we're currently at a point where we can indeed use rclcpp::Time 🤔
The only other reason I can think of for wrapping the time class would be to add metadata, which isn't a concern at the moment.

For posterity though, I've saved the current state in a gist: https://gist.github.com/methylDragon/ba1a39f619d7f9ca22dfc9fd49e51b40

As for crashing when time zero arrives, the only time that could occur is if a system clock is reporting time 0 on its epoch (meaning it'd be in 1970, 1 Jan), or if we have time 0 in a simulation. The former is unlikely to happen, and the latter can be guarded against with wait_for_valid(), so I tihnk it's fairly safe. In either case, those timestamps would be considered invalid.

I'll also update all Time instantiations of (0, 0) to (0, 1), to reflect the time validity. Change is pending.

[BrettRD]

Again, the point of fuse_core::TimeStamp was for ROS1 backports, and I still think that's an important usage for a wrapper class, even if it's just a typedef.
Time is currently the only ROS dependency in user-written fuse_variables. If you wrap time, migration of variable definitions between versions of ROS becomes trivial

I also want to emphatically dispute "unlikely" because ROS2 uses signed 32 bit integers on the system epoch, which overflows to negative values and stays there in a little less than 16 years from now.
This will never be a problem in ROS1 because nobody will accumulate 60 years of up-time on any ROS1 roscore process
The embedded systems world has conventions where we use overflow-tolerant arithmetic, but it strictly requires that the actual time values are never used as logical flags.

[sloretz]

Again, the point of fuse_core::TimeStamp was for ROS1 backports, and I still think that's an important usage for a wrapper class, even if it's just a typedef.
Time is currently the only ROS dependency in user-written fuse_variables. If you wrap time, migration of variable definitions between versions of ROS becomes trivial

The typedef will need to be done to both ROS 1 and ROS 2 branches to be useful. Supporting ROS 1 is out of scope for our effort, but I don't see a reason against doing so as an enhancement to both branches later. In the meantime user-written fuse_variables can support both ROS versions with #ifdef RCLCPP_VERSION_MAJOR and typedef. That's not convenient, but I would guess downstream code probably already has some ROS-awareness at the build system level because of catkin.

I also want to emphatically dispute "unlikely" because ROS2 uses signed 32 bit integers on the system epoch, which overflows to negative values and stays there in a little less than 16 years from now.

This is a larger issue than we can solve in the fuse repo. ROS 2 will probably follow whatever OMG DDS does https://issues.omg.org/issues/DDS15-13

[methylDragon]

Note to self: I'm going to need to:

• Eventually: make sure this all runs by syncing up the appropriate clock types (no mismatches between RCL_ROS_TIME and RCL_SYSTEM_TIME should occur)
• Keep Time(0, 0) for when time is truly meant to be uninitialized
• Migrate to Time(0, 1) for when it's meant to signify "earliest valid time" (e.g. for comparisons)
  • What happens if Time(0, 1) gets compared to Time(0, 0)???? D:

[methylDragon]

I've tried to make a best effort decision for when to use (0, 0) (uninitialized time) and (0, 1) (earliest time) from reading the code.
This is something to test when the whole stack eventually builds.

cfd4d21

It's complicated by the fact that uninitialized time is sometimes used (just like in tf), to grab the latest time. And other times used to signify non-initialization, or used for comparison. For most cases it's pretty clear, but there are some cases (e.g. for comparison for deletion) where I'm unsure if it would be favorable to allow (0, 0) to be deleted or not.

In those cases I erred on yes.

[methylDragon]

Also in cfd4d21#diff-33b6e1985c2434b43effda68029aab772a2cf3efd8f7d74c91bdfa9c89dee37dR235, since we moved back to using rclcpp::Time, I needed to decide which clock type to use for the publisher synchronizer's internal time (for its starting time and subsequent time.)

I went with RCL_ROS_TIME since that made sense. But there is nothing stopping a user from dropping in a transaction into the graph with a different clock type.

[sloretz]

Since the clock_interface is used to get a temporary clock for one call, I'd recommend this method takes a const rclcpp::Clock & instead of the interface.

[methylDragon]

I'll take in an rclcpp::Clock::SharedPtr

[methylDragon]

59d10dd

[sloretz]

rclcpp::Clock::SharedPtr as a parameter signals that the passed in clock is going to keep the Clock alive longer than just the function call.

I see that the intent of making clock optional is so it can fallback to a system time clock; however, this function is in a .cpp file and is not exposed in the header file. All calls to it do pass in a clock, so the the fallback behavior of using system time isn't needed. I think const rclcpp::Clock & is a better fit.

[methylDragon]

c18049b

I actually wanted to ask about this one, does this mean that the is_valid and wait_for_valid methods in my time patch should also take in const refs?

I was thinking to use shared_ptrs because the node interface returns a shared ptr to the node's clock, so it'd avoid needing to do a dereference, and also because, in my view, the node's clock would continue to exist.

[sloretz]

I actually wanted to ask about this one, does this mean that the is_valid and wait_for_valid methods in my time patch should also take in const refs?

Probably, yeah 😶

I was thinking to use shared_ptrs because the node interface returns a shared ptr to the node's clock, so it'd avoid needing to do a dereference, and also because, in my view, the node's clock would continue to exist.

Fair point, a Clock::SharedPtr is more convenient given other APIs return that.

and also because, in my view, the node's clock would continue to exist.

From the perspective of beaconsToPointcloud(), the clock only has to exist while the it's being run.

[sloretz]

I'd recommend passing a clock so that a caller may use a ROS_TIME clock if they want.

[sloretz]

Since this is an internal function, same comment as beaconsToPointcloud about unconditionally taking a clock. I see that where it's used it's called without a clock, but it could be given *node->get_clock() there.