When can a Duration/Time be negative?

[dhood]

Spinning off from ros2/geometry2#67 (comment) so I don't hijack that thread (and so the convo doesn't get lost).

I thought that Durations could not store negative values but from @tfoote's comment it sounds like that understanding is incorrect. The source of my misunderstanding is this line:

rclcpp/rclcpp/src/rclcpp/duration.cpp

Line 92 in af6e86c

throw std::runtime_error("cannot store a negative duration point in rclcpp::Duration");

Should that check be removed? Or is it appropriate in that situation?

[dhood]

this question can be extended to Time as well, because it also seems to have signed storage underneath, but has similar exceptions e.g.:

rclcpp/rclcpp/src/rclcpp/time.cpp

Line 52 in 3a50368

throw std::runtime_error("cannot store a negative time point in rclcpp::Time");

[tfoote]

Time cannot have a negative value. It's nanoseconds since the epoch. That looks like a copy and paste error from the time class. There's no such thing as a duration point.

The time point is a point on the timeline. A duration is the difference between two time points on the timeline. Time can be differentiated to a duration, Duration can be added to time, But time cannot be added together. 1990 + 1991 doesn't make any sense.

There are tests for negative values here: https://github.com/ros2/rclcpp/blob/master/rclcpp/test/test_duration.cpp but clearly not coverage on negative values being assigned.

[dhood]

I recall now the storage type changed from unsigned to signed: #429

Here's what I suspect:

• At some point, Time couldn't be negative, so the exceptions were added.
• Time can be negative now, the exceptions should have been removed during that change but were overlooked.
• Duration can be negative, but was copied from Time so has exception(s) by accident that should be removed.

[dhood]

Durations can be negative and the exceptions were removed in #527 for rclcpp and ros2/rclpy#220 for rclpy.

Currently both rclcpp and rclpy will still raise on negative Times, even though we use signed storage underneath: rclcpp, rclpy

I am of the opinion that these exceptions also need to be removed, since Time messages can be negative and time storage in rcl can be negative. Some may argue that instead, while technically we want to have signed storage for reasons related to MISRA compliance, we don't actually want users to create negative times in the client libraries, and so these exceptions should remain. I think this will lead to confusion and inconsistencies since the messages can be filled with negative times and perhaps some client libraries will end up supporting negative times while others won't. Because of that I think that now that we have made the decision to allow negative time points in the message and the rcl storage, we should allow users to create negative time points in client libraries, should they wish.

@sloretz Would you agree? I ask because #527 and ros2/rclpy#220 only covered negative durations and not negative times, but it may have just been to keep the work in two separate passes.

[sloretz]

I'm not sure if negative time should be allowed or not. Negative time makes sense as a time prior to the epoch, but the time design doc says

A time value of zero should be considered an error meaning that time is uninitialized

Allowing negative values means there is a discontinuity where addition or subtraction that results in zero is invalid.

I skipped negative time in ros2/rclpy#220 because I'm not sure about it, and I only need negative durations for an rclpy time jump callback PR I'm working on.

[dhood]

Just for posterity, I don't think the design doc is saying that all times of zero are invalid, but that if a ROS clock's now() returns zero then ROS time is uninitialized. So that would only happen if someone has explicitly set the ros time override to be zero for that clock. I agree that there's still discontinuity, but want to clarify that it is not for time points in general, but time points returned in a specific context (we might be on the same page for this but the sentence you captured excluded relevant context).

That a ROS clock returns zero when sim time is uninitialised has been questioned before, and I don't think it's actually the case at the moment. If you have a ROS clock with sim time but the ros time override hasn't been set, you'll actually just get an uninitialised time point at the moment.

I'm glad you brought the point up because I think it was less of a concern before the notion of negative Times was introduced, and it is something that we need to address (and currently has a bug). (There are a number of ways we could fix address it, including initialising it to zero as the design doc says, or signalling by other means that sim time is active but uninitialised). I think we should have that conversation, and swap to supporting negative times in client libraries. I'll let @tfoote weigh in.

[wjwwood]

In my opinion, if you allow negative time point values, then you need to consider zero to be a valid time point as well. Otherwise you have a single second in time that you cannot represent in simulated time.

This implies you need a new way to indicate that sim time is active but not initialized. We could use an exception instead, though if people regularly need to catch that exception it might not be very ergonomic.

[tfoote]

In my opinion, if you allow negative time point values, then you need to consider zero to be a valid time point as well.

This is definitely true. When we were using unsigned values 0 was used as a value that indicated time had not been initialized. It was conveniently the default value for most datatypes so would initialize to uninitialized. And all simulators actively integrated also started at zero so that worked well as well. If we add negative time support we will need to remove the representation of zero as uninitialized since it's in the middle of the possible time values.

I agree that adding this support would significantly change the API and make interacting with the code a little more complicated. @dhood you're right that at some level it's a special return code from the clock now method. But in many other places we've also used the shorthand that time 0 is uninitialized and it has the great value that you can know that basically it has never been purposely set since it's still the default value even for random data fields so it has value beyond the now() return values.

An example of using the zero vale is that in tf queries, 0 means time not set, find the latest time that's valid. We'd either have to create a specific enum or value that would represent time not set or add another data element to indicate it and other signaliing mechanisms too.

Due to the extra complexity that would be required I'd suggest that we stick with the existing representation of 0 is uninitized and time is only valid in positive ranges. This will keep all the time logic in ROS1 code compatible as well when porting. We still have several decades of time support in the past and future from current time. And we know that this datatype is not useful for the representation of long term time durations. Supporting representations of the decades before 1970 doesn't seem to add much value. So I'd suggest that we consider times <=0 uninitialized in ROS 2 code.

[dhood]

It does seem like allowing the full signed range of times to be valid will lead to a lot of code changes when porting from ROS 1. As you mentioned, time of 0 is treated as a special case in many places.

Here's my understanding of the history:

• In 2014 we added Time.msg with signed seconds field (int32). I tracked down this original commit but couldn't find the rationale (ROS 1 doesn't support negative time).
• In 2016 we added tests for negative time seconds. https://github.com/ros2/system_tests/pull/122/files#diff-831fe03f2d54d3547eae0bc2f01db942R399
• In 2017 we had these discussions about how we use signed field in the message, but unsigned storage for the time point (uint64 at the time, now it's changed to int64): rclcpp::Time operators #352, https://discourse.ros.org/t/of-clocks-and-simulation-betimes-and-otherwise/1587
  • In both discussions we spoke about swapping the message to int64, but it will cause more of a hassle porting ROS 1 code.
• In 2017 we added code for working with Time types in rclcpp and prevented them from being created as negative: https://github.com/ros2/rclcpp/pull/351/files#diff-ca6a5ec20b6b888cf9e195fa3f534973R91
• Later in 2017 we changed the storage of time points from uint64 to int64: Convert ROS2 time value to signed int64_t rcutils#79

I have not been able to find a reason for why the Time message allows for negative fields. We want signed storage for MISRA compatibility, but I don't think we actually expect anyone to use negative time.

---

So, updating my stance from what I wrote in #525 (comment) before I realised there doesn't seem to be much of a reason for the Time message to be negative:

• In rcutils we have signed storage but it's just a technical detail, we don't want people to use negative time.
• Client libraries should not allow users to create negative time.
• We probably want to change the message definition to int64 but if nothing else we should change it to not allow signed values. Then ROS time would not be able to return negative values.
• Time of 0 can continue to be treated as a special case.

example usage of iszero

[tfoote]

Thanks for all the digging into the history @dhood I think your proposal sounds good. We might as well make it 64bits. It will make it possible to send much larger values, but avoids a rollover debate/new epoch for a long time. It will cost 32 bits for approximately every message though, which on smaller platforms or low bandwidth links will be noticed.