Document that Time and Duration are explictly ROS Time

[tfoote]

Fixes #94

[dirk-thomas]

If these are supposed to be ROS time how do they convey the information which clock they belong to? Without I don't see how the updated comment is helpful.

[tfoote]

Can you clarify what is ambiguous about stating that these are ROS Time, one of the three types of times called out in the referenced design document.

[dirk-thomas]

Can you clarify what is ambiguous about stating that these are ROS Time, one of the three types of times called out in the referenced design document.

Of which of the three types is the value?

[tfoote]

There are three time abstractions in the reference design document:

1. System TIme
2. ROS Time
3. Steady Time

Of which it is "ROS Time"

[dirk-thomas]

Why can't a user not use the messages for system / steady time? How can a user determine what clock the ROS time is being coming from?

[tfoote]

This is explicitly documenting that you cannot use this for steady time or system time.

Steady time and system time are not semantically equivalent and cannot be substituted arbitrarily. Any usage of steady time requires additional logic and tracking to validate the epoch upon which it's operating. And there are extra error conditions that need to be tracked and recovered from.

If you get sensor data from two different epochs you're whole processing algorithm now needs significatnly more logic to deal with things like differencing timies and adding durations which might fail. This is specifically something that we don't want to require to use our default time representation. Already allowing time jumps and resets is complicated enough.

As we discussed in the in the review following on development discussions we would like to create a fully independent rcl Steady Time datatype.

There are separate issues as followups to track creating Steady Time message and implementation

[dirk-thomas]

I am not sure this reflects reality. Since there are no other time messages (for system or steady) I expect this representation to be used for all three kinds. Narrowing the scope in this comment won't change that. And without have alternatives I don't think it is helpful.

So I will just vote with a +0 on this.

[tfoote]

Indeed without additional specific support for the other types in messages it's quite possible that people will abuse this field. However since most use cases for steady time and system time are typically only local most of their usages do not go through the network which means sending messages about them is less common. We can and should add support for that. But explicitly documenting that this is for communicating just ROS Time will help avoid unexpected behavior in the future by keeping people from putting those other datatypes through this field.

We can and should add support for the other datatypes. (As ticketed) But until that time making sure that this is explicit and consistent is the most important. Users can always create their own data structure for those times too. I've also seen cases which integrate non-abstract time primitives that are hardware connected into their messages such as clock ticks etc. We're not blocking that, we're just not making it as convenient as possible with a standard abstraction for them to use.

[jacobperron]

I think this is a step in the right direction.

It is still not clear if the time comes from the system clock or an alternate time source (ie. /clock topic),
although I'm not sure if it makes sense to indicate that in the message (e.g. if rosbag plays back stamped data that indicates system time, it clearly is not going to match system time).

[dirk-thomas]

@tfoote It looks like this PR has introduce a regression: https://ci.ros2.org/view/nightly/job/nightly_linux_debug/1590/testReport/junit/ros2interface.ros2interface.test/test_cli/test_cli/

Since I don't see any CI builds for this PR I assume none have been triggered before merging this? Please address the failing test asap.

[dirk-thomas]

This patch should be released into rolling in order to make the ros2cli dev job (http://build.ros2.org/job/Rdev__ros2cli__ubuntu_focal_amd64/) pass.

[jacobperron]

This patch should be released into rolling in order to make the ros2cli dev job (http://build.ros2.org/job/Rdev__ros2cli__ubuntu_focal_amd64/) pass.

Alternatively, resolving ros2/ros2cli#541 should also make the job green. But, making a patch release should be more straight-forward.

[tfoote]

1.0.1 of rcl_interfaces released into rolling: ros/rosdistro#25627 However this also is on the Foxy branch so we really need to resolve ros2/ros2cli#541 otherwise we're going to break tests over there when there's similar release there too.

[jacobperron]

However this also is on the Foxy branch so we really need to resolve ros2/ros2cli#541 otherwise we're going to break tests over there when there's similar release there too.

This change (and the ros2cli change) hasn't been backported to the foxy branch. If we backport one, I suggest we backport the the other. But I don't think backporting to Foxy is necessary.