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.

Sign up for GitHub

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

Jump to bottom

Port tf2 ros message filter with ros2 tf2 and message filters #81

Merged
merged 10 commits into from
Nov 22, 2018
Merged

Port tf2 ros message filter with ros2 tf2 and message filters #81

merged 10 commits into from
Nov 22, 2018

Conversation

gaoethan
Copy link
Contributor

@gaoethan gaoethan commented Nov 9, 2018

It requires another PR ros2/message_filters#13 of ros2 message filter to work.

@gaoethan
Port tf2 message filter to ros2
76ba05c 
 - add ros2 package dependencies and rules
 - remove boost with std functions instead
 - use rclcpp Time and Duration
 - remove APIs to node callback queue due to no callback queue
   in ros2 now
 - Change failure callback register with failure prompting due to
   no corresponding boost signal2 in C++11 and later
 - Fix expected transform count in case of time tolerance
 - Adapt to ros2 tf2 APIs
@gaoethan
Add tf2_ros_test_message_filter to enable feature checking
89948b1
@tfoote tfoote added the in review Waiting for review (Kanban column) label Nov 9, 2018
@gaoethan gaoethan mentioned this pull request Nov 9, 2018
Merged
@mkhansenbot
Copy link

@tfoote - we would like to get this one and the linked dependency merged in time for the Crystal release, as these are dependencies for the ROS2 Navigation which we're targeting for Crystal (as you already know).

Please let us know if that can be done in time.

clalancette
clalancette requested changes Nov 13, 2018
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be reformatted to reduce the diffs and make it easier to review. There is also a potential for races with the removal of locks which needs to either be fixed, or at the very least have a comment saying why they are safe to remove.

tf2_ros/include/tf2_ros/message_filter.h Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/message_filter.h Outdated Show resolved Hide resolved
@bpwilcox bpwilcox mentioned this pull request Nov 13, 2018
Merged
@mkhansenbot mkhansenbot mentioned this pull request Nov 15, 2018
Closed
34 tasks
@mkhansenbot
Copy link

@clalancette - please review again. Our Navigation team is depending on this for the Crystal release. Thanks!

@clalancette
Some small style fixes.
6282609 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
clalancette requested changes Nov 20, 2018
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of more things that I noticed this time through the review. There are also a couple of nits that I'll push a patch for in a few minutes. We are close enough here that I'm going to run CI on it, but before we can merge we need to fix, or at least have answers for, the couple of things in the review.

tf2_ros/include/tf2_ros/message_filter.h Show resolved Hide resolved
tf2_ros/include/tf2_ros/message_filter.h Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor

CI:

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

@tfoote
Copy link
Contributor

tfoote commented Nov 20, 2018 •
edited
Loading

This added 2 new warning on windows: https://ci.ros2.org/job/ci_windows/5476/warnings43Result/new/
OSX timed out I've retriggered it

  • macOS Build Status

@clalancette did you have those quick patches you mentioned?

@clalancette
Copy link
Contributor

@clalancette did you have those quick patches you mentioned?

I already pushed it in 6282609

@tfoote
Copy link
Contributor

tfoote commented Nov 21, 2018

Thanks @clalancette sorry I was looking for commits after your comment.

gaoethan and others added 4 commits November 21, 2018 11:09
@gaoethan
optimize lock using while handling coming messages
aac3934
@gaoethan
Merge branch 'enable_tf2_message_filter'
25c9cb6
@tfoote
Upgrade all message counts to 64 bitThis should resolve C4267 warning…
69d54cb 
…s about downgrading a size_t.
@clalancette
Add back in the code for the callback queues.
4ec6241 
It is commented out since we don't have underlying support for
it, with a TODO for the future.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
mjcarroll
mjcarroll reviewed Nov 21, 2018
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nit, looks like you are over-including/over-linking here. I believe this was there before you worked on it, but it would be a good time to remove it.

tf2_ros/CMakeLists.txt Outdated Show resolved Hide resolved
@clalancette
Small style fixes to reduce diff.
170a6d6 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Remove unnecessary rmw dependency.
dd41d40 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
clalancette reviewed Nov 21, 2018
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally good with this change now. @tfoote thinks he has found the issue with the failed link on macOS, so once we have that fixed we can run CI again and then hopefully approve and merge.

mjcarroll
mjcarroll approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with CI.

@tfoote
Copy link
Contributor

tfoote commented Nov 21, 2018

I think that this will resolve the linking issue. Instead of adding an export for the library it turns out I don't think it's necessary: ros2/message_filters#17

@clalancette
Copy link
Contributor

clalancette commented Nov 21, 2018 •
edited by tfoote
Loading

Here's a CI run incorporating ros2/message_filters#17:

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

Updated build numbers for crashed agents.

@tfoote tfoote merged commit 697b2f4 into ros2:ros2 Nov 22, 2018
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Nov 22, 2018
This was referenced Nov 23, 2018
Closed
Closed
@yechun1 yechun1 mentioned this pull request Dec 26, 2018
Closed
@Squadrick Squadrick mentioned this pull request Jan 7, 2019
Closed
@DLu DLu mentioned this pull request Dec 2, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfoote tfoote tfoote left review comments

@clalancette clalancette clalancette approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gaoethan @mkhansenbot @clalancette @tfoote @mjcarroll