-
Notifications
You must be signed in to change notification settings - Fork 195
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
Use ros2 time #67
Use ros2 time #67
Conversation
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.
A few small tweaks but otherwise lgtm
tf2_ros/src/tf2_monitor.cpp
Outdated
@@ -202,7 +205,7 @@ class TFMonitor | |||
if (using_specific_chain_) | |||
{ | |||
auto tmp = buffer_.lookupTransform(framea_, frameb_, tf2::TimePointZero); | |||
double diff = tf2::timeToSec(tf2::get_now()) - tf2_ros::timeToSec(tmp.header.stamp); | |||
double diff = tf2_ros::timeToSec(clock_->now()) - tf2_ros::timeToSec(tmp.header.stamp); |
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.
It's better to do the diff of the Time datatypes then convert to seconds. ala ros/geometry2#310 It will keep the precision in the fixed point math.
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.
could the diff be negative? if so we have to keep in mind that the subtraction of the Times will throw an exception in that case (might have been the reason for converting to seconds first)
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.
It's ok if the diff is negative. The difference between Time objects is a Duration for which negative values are valid. It would only flow if the delta is larger than the maximum duration, plus or minus in which case it will except and overflow.
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 have the wrong understanding about what's allowed for a Duration by the sounds of it: could we move the discussion to ros2/rclcpp#525?
tf2_ros/src/tf2_monitor.cpp
Outdated
@@ -77,7 +78,7 @@ class TFMonitor | |||
{ | |||
frame_authority_map[message.transforms[i].child_frame_id] = authority; | |||
|
|||
double offset = tf2::timeToSec(tf2::get_now()) - tf2_ros::timeToSec(message.transforms[i].header.stamp); | |||
double offset = tf2_ros::timeToSec(clock_->now()) - tf2_ros::timeToSec(message.transforms[i].header.stamp); |
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.
Differentiate in fixed point here too.
@@ -69,6 +69,12 @@ namespace tf2_ros | |||
return (s + std::chrono::duration_cast<std::chrono::duration<double>>(ns)).count(); | |||
} | |||
|
|||
inline double timeToSec(const rclcpp::Time & rclcpp_time) |
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.
This would probably be better suited to be a method on the rclcpp::Time
class similar to https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/time.hpp#L103 . Or at least not exposed publicly 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.
Added rclcpp::Time::seconds()
in ros2/rclcpp#526
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.
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 started reviewing, but others finished before me. I'll just post my one comment I had queued up...
tf2/include/tf2/time.h
Outdated
}; | ||
|
||
//using Duration = std::chrono::nanoseconds; | ||
//using TimePoint = std::chrono::time_point<std::chrono::system_clock, Duration>; |
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.
Should these comments be left 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.
Here and below.
Thanks, for the reviews! Merging. |
@sloretz could you update the turtlebot repos to adapt to these changes? https://ci.ros2.org/job/nightly_turtlebot-demo_linux_release/395/console#console-section-11 |
@dhood @wjwwood Fixed in ros2/cartographer_ros#20 and ros2/navigation#33 |
The API change for |
This is updating tf2_ros to use
rclcpp::Time
. Thetf2_ros::Buffer
class requires a clock to be passed into it's constructor.tf2_ros::Buffer
also registers time jump handlers and clears itself instead of relying ontf2_ros::TransformListener
to do it.Todo
rclcpp::Time
instead oftf2::TimePoint
tf2::TimePoint
andrclcpp::Time