Skip to content
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

Merged
merged 3 commits into from Aug 8, 2018

Conversation

Projects
None yet
5 participants
@sloretz
Copy link

commented Jul 17, 2018

This is updating tf2_ros to use rclcpp::Time. The tf2_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 on tf2_ros::TransformListener to do it.

Todo

  • Change buffer_interface to use rclcpp::Time instead of tf2::TimePoint
  • Make it easy to covert between tf2::TimePoint and rclcpp::Time
  • Update all downstream code

@sloretz sloretz added the in review label Jul 17, 2018

@sloretz sloretz self-assigned this Jul 17, 2018

@dirk-thomas dirk-thomas requested a review from tfoote Jul 19, 2018

@sloretz sloretz force-pushed the use_ros2_time branch from 2880b8b to 0207c54 Jul 26, 2018

@sloretz sloretz added in progress and removed in review labels Jul 26, 2018

@sloretz

This comment has been minimized.

Copy link
Author

commented Jul 26, 2018

CI to check for breakages in other repos

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@tfoote
Copy link

left a comment

A few small tweaks but otherwise lgtm

@@ -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);

This comment has been minimized.

Copy link
@tfoote

tfoote Jul 26, 2018

It's better to do the diff of the Time datatypes then convert to seconds. ala ros#310 It will keep the precision in the fixed point math.

This comment has been minimized.

Copy link
@dhood

dhood Jul 26, 2018

Member

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)

This comment has been minimized.

Copy link
@tfoote

tfoote Jul 26, 2018

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.

This comment has been minimized.

Copy link
@dhood

dhood Jul 26, 2018

Member

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?

@@ -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);

This comment has been minimized.

Copy link
@tfoote

tfoote Jul 26, 2018

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)

This comment has been minimized.

Copy link
@tfoote

tfoote Jul 26, 2018

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.

This comment has been minimized.

Copy link
@sloretz

sloretz Jul 27, 2018

Author

Added rclcpp::Time::seconds() in ros2/rclcpp#526

This comment has been minimized.

Copy link
@sloretz
@wjwwood
Copy link
Member

left a comment

I started reviewing, but others finished before me. I'll just post my one comment I had queued up...

};

//using Duration = std::chrono::nanoseconds;
//using TimePoint = std::chrono::time_point<std::chrono::system_clock, Duration>;

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jul 26, 2018

Member

Should these comments be left here?

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jul 26, 2018

Member

Here and below.

@sloretz

This comment has been minimized.

Copy link
Author

commented Aug 8, 2018

CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@sloretz

This comment has been minimized.

Copy link
Author

commented Aug 8, 2018

CI again

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@tfoote

tfoote approved these changes Aug 8, 2018

@sloretz sloretz added in review and removed in progress labels Aug 8, 2018

@sloretz

This comment has been minimized.

Copy link
Author

commented Aug 8, 2018

Thanks, for the reviews! Merging.

@sloretz sloretz merged commit 7dafb4d into ros2 Aug 8, 2018

@sloretz sloretz deleted the use_ros2_time branch Aug 8, 2018

@sloretz sloretz removed the in review label Aug 8, 2018

@dhood

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

@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

@wjwwood

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@sloretz if you need help with the changes to amcl (maybe others) w.r.t. this change let me know. We still have failing nightly jobs, as @dhood pointed out.

@sloretz

This comment has been minimized.

Copy link
Author

commented Aug 14, 2018

@sloretz sloretz referenced this pull request Sep 21, 2018

Closed

ROS2 time vs TF Time #5

@dirk-thomas dirk-thomas referenced this pull request Sep 21, 2018

Open

Functionality around time #530

0 of 7 tasks complete
@IanTheEngineer

This comment has been minimized.

Copy link

commented Dec 14, 2018

The API change for tf2::Buffer to use ROS2 Time warrants a migration note (and example usage) for Bouncy->Crystal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.