add Duration::seconds

[dirk-thomas]

These are two getters which I found helpful (and missing) while porting more robot web tools packages.

[tfoote]

Adding the seconds makes sense. For the is_zero() usages are you using that for logic to detect if it has been set/is valid? If so I might suggest that we use accessors that directly related to that so that in the case that zero is no longer the "invalid" value then the accessor name does not need to change.

Related: #525 (comment)

[dhood]

in tf2 we convert to seconds in a convoluted way to improve accuracy: see ros2/geometry2#34. This allows for tests going to- and from- seconds to pass: https://github.com/ros2/geometry2/blob/a275730813b9cf55943f3f05d57aaee51b32944a/tf2/test/simple_tf2_core.cpp#L177. We may want to consider that here

[dirk-thomas]

The current line in the patch matches the content of the same function in the Time class. I am not sure how the referenced issue applies here since the computation is not complex in any way (in contrast to durationToSec in geometry)?

Can you please propose the specific code you would prefer to implement this method.

[dhood]

I recommend you use https://github.com/ros2/geometry2/blob/7dafb4d6c0c7025dfeeed3cef336113c62601dac/tf2/include/tf2/time.h#L74..L84 to make the computation more complex and more accurate.

[dirk-thomas]

Got it now. The goal is to increase the precision.

It would be good to share that implementation somehow between the various places where we do convert integer nanoseconds into double seconds. That function should probably live in rcutils and would then be used here in rclcpp as well as geometry2. Since that exceeds the scope of this patch I would keep that for a separate set of PRs. (Also a test demonstrating that the precision is increased would be good for that function.)

[dhood]

yeah, I imagine the precision is not expected to be perfect anyway, but the improvement is there for when we want it. can be an enhancement.

[dirk-thomas]

For the is_zero() usages are you using that for logic to detect if it has been set/is valid? If so I might suggest that we use accessors that directly related to that so that in the case that zero is no longer the "invalid" value then the accessor name does not need to change.

Having a value of 0ns doesn't necessarily imply invalid. If you subtract two Time values a duration of 0ns is a perfectly valid result. (the method is for the Time class - not Duration). Therefore Anyway I would prefer the current name is_zero (since that is exactly what the method is checking).

[dhood]

Having a value of 0ns doesn't necessarily imply invalid. If you subtract two Time values a duration of 0ns is a perfectly valid result. Therefore I would prefer the current name is_zero (since that is exactly what the method is checking).

Think tfoote's point was about the intent of the check. Zero is a very specific time to compare against, so we are wondering if you are not actually asking the question "is time initialised" or similar. Perhaps you can clarify the use case you're using it for, because you brought up durations of 0ns being valid, but the PR is adding code for Time::is_zero (not for Durations).

[dirk-thomas]

This is for existing ROS 1 code and I am not going to change any of the semantic for the port. It was checking for isZero before so I would like to check for t.nanosecs == 0. What semantic the check of the code has isn't really relevant for the question if we should add this method.

If the class should also offer a different method to check for validity is a separate question imo.

Let's just decide if we are fine adding a method is_zero as proposed in this PR. If not, I will remove the part from the patch and update all the calling code in the ported package(s) to check the value of nanoseconds explicitly.

[tfoote]

I'd rather not replace the one line integer check with a one line method that doesn't add semantic meaning. If we had the semantic meaning then it would make sense to define the additional API. This is especially true if there might be an API with a semantic meaning that could then be confusingly similar for future developers and cause inconsistent behavior if developers use them interchangeably.

[dirk-thomas]

I'd rather not replace the one line integer check with a one line method that doesn't add semantic meaning.

I was trying to maintain a similar API as in ROS 1 in order to not unnecessarily increase the effort when porting code. But if the consensus is that we shouldn't offer this method (anymore) I will remove it from this patch. Maybe others can confirm that this is what we want to do...

[dirk-thomas]

I removed the is_zero() method in 2fbf62b to unblock this PR (as well as several downstream PRs). Please review so that is can be merged soon.

Independent of this please comment with your opinion if Time should have a is_zero() method.

[mikaelarguedas]

current state of the PR with only the seconds() addition looks good to me 👍.

A ticket tracking the work of exposing the nanosecs to secs conversion from tf2 for reuse would be valuable (and a todo here as well as Time::seconds())