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.