Skip to content

Conversation

@allenh1
Copy link
Contributor

@allenh1 allenh1 commented Jun 25, 2021

This is a revival of #37.

The original changeset made by @jcmonteiro has been rebased on the latest code + adapted where necessary, and the feedback left by @Karsten1987 has been addressed.

Let me know if there's anything else that should go in!

jcmonteiro and others added 4 commits June 14, 2021 15:15
Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>
Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>
Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>
@gezp
Copy link

gezp commented Jul 15, 2021

I have a idea to improve it, when we want to use Subscriber for LifecycleNode, we should define

message_filters::Subscriber<sensor_msgs::msg::LaserScan,rclcpp_lifecycle::LifecycleNode>  sub_;
// it looks like too long

we can adapt Subscriber class to be used with LifecycleNode and Node by using rclcpp::node_interfaces::NodeBaseInterface::SharedPtr , a example in tf2_ros/transform_listener.h , like this:

template<class NodeT>
Subscriber(NodeT &node, ...){
   node_base_interface_ = node->get_node_base_interface();
}

then, we can create subscription by using node_base_interface_ .

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Jul 15, 2021

@mjcarroll can you rerun CI?

@tfoote @Karsten1987 can you re-review and/or approve this PR? I believe it meets all of the requirements Karsten wanted. It's something we're trying to use in Nav2 in order to remove excess rclcpp::Node's from our servers as part of @gezp's summer project with me. A sync to rolling after merging would be massive beneficial as well if possible 😄 (but I won't push it if I can get only one 😉 )

This is also something we rely on quite a bit in Nav2. I'd be happy to take on a co-maintainership role on this project as well if that would help for future inquiries.

@mjcarroll
Copy link
Member

@ros-pull-request-builder retest this please

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Jul 15, 2021

Thank you both for the help! Does this need a second maintainer review to be merged? Else, I've reviewed independently and am satisfied with this solution.

@mjcarroll mjcarroll merged commit a3e415f into ros2:master Jul 16, 2021
gezp added a commit to gezp/navigation2 that referenced this pull request Jul 17, 2021
Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>
@SteveMacenski
Copy link
Contributor

@mjcarroll can we get a sync with rolling?

@mjcarroll
Copy link
Member

@SteveMacenski
Copy link
Contributor

Thank you!

@allenh1 allenh1 deleted the add_lifecycle_node_support branch August 2, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants