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

use node interfaces throughout tf2_ros #108

Merged
merged 10 commits into from
May 10, 2019
Merged

use node interfaces throughout tf2_ros #108

merged 10 commits into from
May 10, 2019

Conversation

Karsten1987
Copy link
Contributor

@Karsten1987 Karsten1987 commented May 8, 2019 •
edited
Loading

this PR is based on #100 and rebased on top of current ros2 master.

It is using the node interfaces consistently throughout the static_transform_broadcaster, transform_broadcaster and transform_listener.

this should fix, provide solutions for lifecycle nodes. See #94 and #70

@Karsten1987 Karsten1987 added the in review Waiting for review (Kanban column) label May 8, 2019
@Karsten1987 Karsten1987 self-assigned this May 8, 2019
@Karsten1987 Karsten1987 changed the title Karsten1987/fix tf listener use node interfaces throughout tf2_ros May 8, 2019
@Karsten1987 Karsten1987 mentioned this pull request May 8, 2019
Closed
Lalit Begani and others added 3 commits May 8, 2019 17:27
@Karsten1987
Use node interface pointers in the TransformListener class
3840eb7 
In the TransformListener class, use node interfaces instead of using the node directly
so that the code works with either rclcpp::Node or rclcpp_lifecycle::LifecycleNode.
Retain the existing node-based interface for backwards compatibility.
@Karsten1987
Format code based on ament_cpplint and ament_uncrustify
ccc4bb2
@Karsten1987
use node interfaces consistently
6e1a315 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987
adher to new rclcpp API
48dedcf 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987 Karsten1987 force-pushed the karsten1987/fix-tf-listener branch from 87c25e2 to 48dedcf Compare May 9, 2019 03:04
@Karsten1987
Copy link
Contributor Author

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

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented May 9, 2019 •
edited
Loading

new round of CI:

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

@tfoote @wjwwood This needs a re-review given that we had to change a fair amount of code while rebasing this PR on top of master.

@dirk-thomas dirk-thomas added the enhancement New feature or request label May 9, 2019
@Karsten1987 Karsten1987 force-pushed the karsten1987/fix-tf-listener branch from ec68a93 to ff4e97b Compare May 9, 2019 19:26
@Karsten1987
remove std::forward
68d5597 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987 Karsten1987 force-pushed the karsten1987/fix-tf-listener branch from ff4e97b to 68d5597 Compare May 9, 2019 19:29
@Karsten1987
Copy link
Contributor Author

new round of CI:

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

it looks like std::shared_from_this() doesn't go well with std::forward. Not exactly sure why the template deduction failed here, but removing the perfect forwarding solved it on my machine.

@Karsten1987
export symbols for windows
1a2f733 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987
Copy link
Contributor Author

Windows again: Build Status

@Karsten1987
callback also has to be public
4c0835d 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987
Copy link
Contributor Author

So the callback also has to be public:
Windows again: Build Status

wjwwood
wjwwood reviewed May 9, 2019
View reviewed changes
tf2_ros/include/tf2_ros/static_transform_broadcaster.h Outdated
{
auto qos = rclcpp::QoS(rclcpp::KeepLast(100));
// TODO(tfoote) latched equivalent
Copy link
Member

Choose a reason for hiding this comment

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

Make it latched or leave the TODO. @tfoote do you have any input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just for completeness: rclcpp::QoS(rclcpp::KeepLast(100)) is not the same as rclcpp::QoS(100)? Should be default to keep last then?

Copy link
Member

Choose a reason for hiding this comment

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

It is the same.

Copy link
Member

Choose a reason for hiding this comment

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

Also, at least restore the TODO until @tfoote can respond.

tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
tf2_ros/test/listener_unittest.cpp Outdated Show resolved Hide resolved
tf2_ros/test/test_static_transform_broadcaster.cpp

void init_tf_broadcaster()
{
tf_broadcaster_ = std::make_shared<tf2_ros::StaticTransformBroadcaster>(shared_from_this());
Copy link
Member

Choose a reason for hiding this comment

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

Can you not pass this here? and therefore do it in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason why I do this specific test is that the original version with std::forward<NodeT>(node) failed when passing in a shared pointer to node.
see here:

15:41:38                  from /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rviz/rviz_default_plugins/test/rviz_default_plugins/displays/odometry/odometry_display_visual_test.cpp:37:
15:41:38 /home/jenkins-agent/workspace/ci_linux/ws/install/tf2_ros/include/tf2_ros/transform_broadcaster.h:58:68: error: cannot bind non-const lvalue reference of type ‘std::shared_ptr<rclcpp::Node>&’ to an rvalue of type ‘std::shared_ptr<rclcpp::Node>’
15:41:38      publisher_ = rclcpp::create_publisher<tf2_msgs::msg::TFMessage>(
15:41:38                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
15:41:38        std::forward<NodeT>(node), "/tf", qos, options);

So this test is mimicking the behavior applied in the rviz default plugin.

Copy link
Member

Choose a reason for hiding this comment

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

I mean why not pass this (or *this), not shared_from_this(). rclcpp::create_publisher<>(*this, ...) should work I think.

We can follow up on that later, but otherwise this code would not work:

https://github.com/ros2/rclcpp/blob/ef41059a751702274667e2164182c062b47c453d/rclcpp/include/rclcpp/node_impl.hpp#L75

tf2_ros/test/test_transform_broadcaster.cpp Outdated Show resolved Hide resolved
tf2_ros/test/test_static_transform_broadcaster.cpp Outdated Show resolved Hide resolved
tf2_ros/test/test_transform_broadcaster.cpp Outdated Show resolved Hide resolved
tf2_ros/test/test_transform_listener.cpp Outdated Show resolved Hide resolved
@Karsten1987
address review
5fa528d 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987
refactor node wrapper class
49dfa0f 
Signed-off-by: Karsten Knese <[email protected]>
wjwwood
wjwwood approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, for merge as-is. There was a follow up that would be interesting to test, but it can come later.

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented May 9, 2019 •
edited
Loading

given the changes, I run a CI job once more:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status unrelated test failures
  • Windows Build Status

@Karsten1987
restore latched todo
f9dbe3d 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987 Karsten1987 merged commit db7493c into ros2 May 10, 2019
@delete-merged-branch delete-merged-branch bot deleted the karsten1987/fix-tf-listener branch May 10, 2019 00:58
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label May 10, 2019
@tfoote tfoote mentioned this pull request May 13, 2019
Merged
@cottsay cottsay mentioned this pull request May 21, 2019
Closed
@Karsten1987 Karsten1987 mentioned this pull request May 21, 2019
Closed
@Karsten1987 Karsten1987 mentioned this pull request Jul 17, 2019
Closed
@kurshakuz kurshakuz mentioned this pull request Sep 19, 2021
Closed
@JohnTGZ JohnTGZ mentioned this pull request Aug 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allenh1 allenh1 allenh1 left review comments

@wjwwood wjwwood wjwwood approved these changes

@tfoote tfoote tfoote approved these changes

Assignees

@Karsten1987 Karsten1987

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Karsten1987 @wjwwood @tfoote @allenh1 @dirk-thomas