-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Duration::seconds #536
Conversation
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) |
double | ||
Duration::seconds() const | ||
{ | ||
return std::chrono::duration<double>(std::chrono::nanoseconds(rcl_duration_.nanoseconds)).count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Having a value of |
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 |
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 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 |
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. |
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... |
I removed the Independent of this please comment with your opinion if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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())
These are two getters which I found helpful (and missing) while porting more robot web tools packages.