Conversation
|
CI to check for breakages in other repos
|
tfoote
left a comment
There was a problem hiding this comment.
A few small tweaks but otherwise lgtm
tf2_ros/src/tf2_monitor.cpp
Outdated
| { | ||
| 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.
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.
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.
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.
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
| 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.
Differentiate in fixed point here too.
| 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.
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.
Added rclcpp::Time::seconds() in ros2/rclcpp#526
wjwwood
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Should these comments be left here?
|
CI
|
|
CI again
|
|
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::Bufferclass requires a clock to be passed into it's constructor.tf2_ros::Bufferalso registers time jump handlers and clears itself instead of relying ontf2_ros::TransformListenerto do it.Todo
rclcpp::Timeinstead oftf2::TimePointtf2::TimePointandrclcpp::Time