Duration: why no .seconds()?

[AndyZe]

Bug report

Required Info:

• Operating System:
  Ubuntu 16.04
• Installation type:
  • binary
• Version or commit hash:
  • rclcpp: 0.4.0-0xenial-20180208-1432
• Client library (if applicable):
  • rclcpp

Feature description

Feedback from a new ROS2 user: I think the new clock is unnecessarily confusing. Why does the constructor take seconds and nanoseconds, but there's only a method to access nanoseconds?

When I retrieve nanoseconds like so:

rclcpp::Duration delta_t_;
delta_t_.nanoseconds();

Am I getting the total time (seconds+nanoseconds)? Or is it the modulo?

My initial thought was that I should do this to convert to seconds, but I have a feeling this is not correct:
delta_t_.nanoseconds()/1000000000;

[wjwwood]

There's not floating point seconds method right now. You could do duration.nanoseconds() / 1e9 to get floating seconds. There's no reason to not have a seconds() -> double method, I think a pr would be accepted.

I will say that the Time interfaces (along with others) are still subject to change in C++. For one thing, the steady clock and system clocks should not be diffable and should produce a compiler error, but I don't think they do right now.

[dirk-thomas]

Might such a method be subject to precision loss? In that case it might be better to not provide it.

[tfoote]

A conversion to double will potentially loose information. If we provide it it should have a explicit/unique signature so it's not accidentally used. Can you just use the datatypes instead of an untyped double?

[AndyZe]

My use case is multiplying a double to integrate:

error_integral_ += error_.at(0) * delta_t_.nanoseconds()/1e9;

[wjwwood]

As long as the loss of precision is documented I think it would be ok, but I agree doing that lossy conversion for the user is not ideal.

@AndyZe to answer your question

Why does the constructor take seconds and nanoseconds, but there's only a method to access nanoseconds?

more directly: it takes seconds and nanoseconds because that's the message storage format, and it has an option to return nanoseconds (that's "as nanoseconds" not the nanoseconds component of the time) because that's the underlying storage format.

Personally, I'd like to see the time and duration messages use a single integer for storage rather than the split pair, but that would be a disruptive change and I haven't had time to push for that to happen.

Am I getting the total time (seconds+nanoseconds)? Or is it the modulo?

It's the total time in nanoseconds (as in "nanoseconds since the unix epoch").

I think it's a worth while goal to avoid methods on the time class which return only part of the represented time (i.e. no sec() and nsec() methods). Instead we have a method to convert the time object directly into our message type which is stored as seconds and nanoseconds, but they're together.

[AndyZe]

I think it makes sense, too. It's ambiguous, currently. What it this going to do?
rclcpp::Time old(2, 2e9);

Is that 4 seconds, or ?

[wjwwood]

That's correct, it is 4 seconds, see:

rclcpp/rclcpp/src/rclcpp/time.cpp

Lines 48 to 57 in d82ce96

	Time::Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type)
	: rcl_time_(init_time_point(clock_type))
	{
	if (seconds < 0) {
	throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
	}
	rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast<int64_t>(seconds));
	rcl_time_.nanoseconds += nanoseconds;
	}

[mikaelarguedas]

Addressed in #536