Discrepency between rclcpp::Time constructor, member functions, and builtin Time message interface

[jmount1992]

There is, in my opinion, a pretty annoying discrepancy between the rclcpp::Time object member functions and the Builtin Interface Time message. It's annoying in that it can easily catch out developers. It caught me out.

RCLCPP Time Object

The rclcpp::Time object has the following constructors and member functions (only constructors and member functions relevant to the issue are provided):

Constructors:

• Time::Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type) : rcl_time_(init_time_point(clock_type)) source
• Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type) : rcl_time_(init_time_point(clock_type)) source
• Time::Time(const builtin_interfaces::msg::Time & time_msg, rcl_clock_type_t clock_type) : rcl_time_(init_time_point(clock_type)) source

Member Functions:

• int64_t Time::nanoseconds() constsource returns total time since epoch (seconds + nanoseconds)
• double Time::seconds() constsource returns total time since epoch (seconds + nanoseconds)

Builtin Interface Time Message

The Builtin Interfaces Time message documentation is as follows:

# Time indicates a specific point in time, relative to a clock's 0 point.

# The seconds component, valid over all int32 values.
int32 sec

# The nanoseconds component, valid in the range [0, 10e9).
uint32 nanosec

Discrepancy

After utilising the Builtin Interface Time Message, and given the first constructor listed in the rclcpp::Time object takes in seconds and nanoseconds separately, one may accidentally assume the seconds and nanoseconds member function returns their component values only and not the total time since epoch.

Potential Fixes

I understand this is a minor issue. However, discrepancies like this cause confusion. Many pieces of code are likely built upon the rclcpp::Time::seconds and rclcpp::Time::nanoseconds functions. However, would any of the following fixes have potential?

1. Swapping the order of the rclcpp::Time constructors so the nanoseconds only constructor comes first?
2. Improving the member function documentation for the rclcpp::Time::seconds and rclcpp::Time::nanoseconds functions. For example, "returns the total time since epoch in seconds (seconds + nanoseconds)".
3. Add member functions to return only the seconds or nanoseconds components, not the total, since epoch?

[mjcarroll]

Seems like a reasonable improvement.

I'm adding this to the upcoming ROS 2 PMC meeting agenda.

[alsora]

My preference is for option 2.

Improving the member function documentation for the rclcpp::Time::seconds and rclcpp::Time::nanoseconds functions. For example, "returns the total time since epoch in seconds (seconds + nanoseconds)".

Currently we hint at that by the fact that when the nanoseconds are used to denote the time since epoch they are represented as int64_t and not as uint32_t.

I don't like option 3, as I think that it would be exposing an internal detail that would cause more harm than good.

[clalancette]

My preference is for option 2.

Yes, agreed.

I don't like option 3, as I think that it would be exposing an internal detail that would cause more harm than good.

Agreed with this as well.

[jmount1992]

Currently we hint at that by the fact that when the nanoseconds are used to denote the time since epoch they are represented as int64_t and not as uint32_t.

And, similarly for seconds as the seconds() member function returns double rather than int32_t

I don't like option 3, as I think that it would be exposing an internal detail that would cause more harm than good.

Out of curiosity, what internal details would be exposed and what harm would that potentially cause?

[alsora]

The use of independent seconds and nanoseconds fields in the builtin_interfaces::msg::Time message has the purpose of providing "almost" 64 bit accuracy, while being more human readable (when you do things like ros2 topic echo) than if having a single 64 bit field.

Users almost never need to use the two fields separately.

[jmount1992]

The use of independent seconds and nanoseconds fields in the builtin_interfaces::msg::Time message has the purpose of providing "almost" 64 bit accuracy, while being more human readable (when you do things like ros2 topic echo) than if having a single 64 bit field.

Users almost never need to use the two fields separately.

Makes sense, having that within the documentation, or some form of that, would be beneficial in my opinion. Even to simply help highlight the discrepancy between the builtin_interfaces::msg::Time and rclcpp::Time objects

[tfoote]

+1 for option 2

[ilarioazzollini]

I can help with this!