Support lifecycle node #304

Benjamin-Tan · 2024-04-06T15:31:10Z

To close #108,

Had a try with the templating approach similar to #167, but that did not go well without some major refactoring. The current implementation overloads the constructor with LifecycleNode instead, with minimal change to the design/architecture.

Others:

Change raw pointers to shared pointers.
Add lifecycle test

…ransport free function, subscriber filter, transport hints

ahcorde

Thank you for your contribution and for pushing this idea (again)

I have some concerns:

We need to deprecated current constructors and add the new ones. To avod breaking other users
Instead of adding two constructors one for Node and another one for LifecycleNode we should use template<typename NodeT>. You can take a look to ros2/message_filters/include/message_filters/subscriber.h

…mpatibility, according to the PR review.

… to the initial implementation

Benjamin-Tan · 2024-04-21T14:19:49Z

I have changed it to the template approach as per your suggestion.

ahcorde

I tried to compile this with clang and I get the following errors:

/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp::Node>::CameraSubscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp_lifecycle::LifecycleNode>::Subscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp_lifecycle::LifecycleNode> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp_lifecycle::LifecycleNode>::CameraSubscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp::Node>::Subscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp::Node> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[2]: *** [CMakeFiles/list_transports.dir/build.make:202: list_transports] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:197: CMakeFiles/list_transports.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
--- stderr: image_transport                              
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp::Node>::CameraSubscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp_lifecycle::LifecycleNode>::Subscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp_lifecycle::LifecycleNode> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp_lifecycle::LifecycleNode>::CameraSubscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp::Node>::Subscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp::Node> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'

Do you mind to take a look ? you just need to add this --mixin clang-libcxx to your colcon command

Another question: These changes require also to modify the image_transport_plugins?

ahcorde

These changes should fix the issue

image_transport/src/camera_publisher.cpp

image_transport/src/camera_subscriber.cpp

image_transport/src/image_transport.cpp

image_transport/src/publisher.cpp

image_transport/src/subscriber.cpp

Benjamin-Tan · 2024-04-22T14:18:28Z

I tried to compile this with clang and I get the following errors:

/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp::Node>::CameraSubscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp_lifecycle::LifecycleNode>::Subscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp_lifecycle::LifecycleNode> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp_lifecycle::LifecycleNode>::CameraSubscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp::Node>::Subscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp::Node> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[2]: *** [CMakeFiles/list_transports.dir/build.make:202: list_transports] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:197: CMakeFiles/list_transports.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
--- stderr: image_transport                              
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp::Node>::CameraSubscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp_lifecycle::LifecycleNode>::Subscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp_lifecycle::LifecycleNode> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::CameraSubscriber<rclcpp_lifecycle::LifecycleNode>::CameraSubscriber(std::__1::shared_ptr<rclcpp_lifecycle::LifecycleNode>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&, std::__1::shared_ptr<sensor_msgs::msg::CameraInfo_<std::__1::allocator<void> > const> const&)> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s)'
/usr/bin/ld: libimage_transport.so: undefined reference to `image_transport::Subscriber<rclcpp::Node>::Subscriber(std::__1::shared_ptr<rclcpp::Node>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::function<void (std::__1::shared_ptr<sensor_msgs::msg::Image_<std::__1::allocator<void> > const> const&)> const&, std::__1::shared_ptr<pluginlib::ClassLoader<image_transport::SubscriberPlugin<rclcpp::Node> > >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, rmw_qos_profile_s, rclcpp::SubscriptionOptionsWithAllocator<std::__1::allocator<void> >)'

Do you mind to take a look ? you just need to add this --mixin clang-libcxx to your colcon command

Another question: These changes require also to modify the image_transport_plugins?

Had the same issue when recompiling with clang, I have fix those and pushed it.

On your question about the need to modify image_transport_plugins, with the templating approach, it seems to be needed to pass the NodeType, unless you have any alternative to this approach. I tried not to change it but could not find a way around it.

ahcorde

You broke API, there are two possible solutions:

Modify image_transport_plugins and only include this changes on rolling
If you need this backported to jazzy, don't break API and redo some work here

Benjamin-Tan · 2024-04-26T13:37:09Z

To be backported, is the following acceptable?

By using the templating approach for the rest, except for the base class for publisher and subscriber plugin.hpp? I think this would keep the API the same, but go against your initial concern of having advertiseImpl(Node*,...), advertiseImpl(LifecycleNode*,...)

authaldo · 2024-04-28T20:40:20Z

Thanks for the implementation effort, finally a solution to the rather old issue #108 is in sight!

As a side note regarding the refactoring mentioned for a continuation of #167:
I attempted to follow this route (based on #258) a while ago and switched to a implementation built around rclcpp::node_interfaces::NodeInterfaces in my forks of image_common and image_common_plugins.
While this provides (at least in my subjective view) a cleaner implementation by using the interface classes for what they have been designed for, it has of course the drawback of breaking with the current API (at the interface between image_transport and the corresponding plugins).

Switching to the NodeInterfaces made it also necessary to adapt the message filters code, leading to the following (still open) PR. This PR was also the main reason for losing the interest in pushing the other two forks further.

image_transport/test/test_message_passing_lifecycle.cpp

ahcorde · 2024-05-15T10:08:58Z

image_transport/test/test_message_passing_lifecycle.cpp

+}
+
+/*
+TEST_F(MessagePassingTestingLifecycle, stress_message_passing)


I was simply referencing from test_message_passing.cpp, shall this be removed entirely?

image_transport/test/test_publisher_lifecycle.cpp

image_transport/test/test_qos_override_lifecycle.cpp

image_transport/test/test_subscriber_lifecycle.cpp

ahcorde · 2024-05-15T11:10:47Z

This PR is quite big, @mikeferguson do you mind to take a look ?

ahcorde

there are some conflicts, by the way I'm not able to compile the plugins

image_transport/include/image_transport/publisher_plugin.hpp

image_transport/include/image_transport/subscriber_plugin.hpp

mikeferguson · 2024-05-15T13:08:34Z

image_transport/src/camera_publisher.cpp

@@ -68,32 +77,61 @@ struct CameraPublisher::Impl
    }
  }

+  std::shared_ptr<NodeType> node_;


by holding a reference to the Node here, are we going to end up with some sort of hanging reference where the node never goes out of scope?\

I'll change them into weak pointers, does that sound good to you? I would think that smart pointers are preferred over the raw pointers.

mikeferguson · 2024-05-15T13:10:47Z

image_transport/src/camera_subscriber.cpp

        image_received_, info_received_, both_received_);
    }
    image_received_ = info_received_ = both_received_ = 0;
  }

+  std::shared_ptr<NodeType> node_;


by holding a reference to the Node here, are we going to end up with some sort of hanging reference where the node never goes out of scope?

mikeferguson · 2024-05-15T13:21:08Z

image_transport/src/image_transport.cpp

+  }
+  if constexpr (std::is_same_v<NodeType, rclcpp_lifecycle::LifecycleNode>) {
+    return Publisher(node, base_topic, kImpl_lifecycle->pub_loader_, custom_qos, options);
+  }


I wonder if this would be clearer by instead just having two fully specialized functions and avoiding the std::is_same_v

The initial implementation of the PR was two specialized functions. I changed it based on #304 (review) , I'm happy to change it back as well.

Benjamin-Tan · 2024-05-20T14:04:25Z

there are some conflicts, by the way I'm not able to compile the plugins

Yeah I think after this PR has been reviewed, I would open a corresponding PR on the image_transport_plugins as well to update the implementation.

ahcorde

conflicts

Arun-Prasad-V · 2024-09-06T12:51:28Z

Any update on this? we want to use lifecycle nodes in our package, but still blocked by image_transport component.

Benjamin-Tan · 2024-09-08T03:32:16Z

It has been awhile since, I am still waiting for the reviewers to comment and feedback on how to move this forward.

Benjamin-Tan added 6 commits April 6, 2024 19:59

Use SharedPtr and update base class in preparation for lifecycle node

c496ccc

Extend lifecycle node to publisher, subscriber

b0dfb86

Extend lifecycle node to camera publisher, camera subscriber, image t…

6707860

…ransport free function, subscriber filter, transport hints

Fix initialising and unit test

ba5e599

Fix formatting and linting error

8eae072

Add test for lifecycle node

a570653

ahcorde requested changes Apr 10, 2024

View reviewed changes

Benjamin-Tan added 3 commits April 21, 2024 22:01

Change the implementation to template class NodeType with backward co…

688e5de

…mpatibility, according to the PR review.

Fix formatting, linting error

d58fdf8

Use raw pointer instead of unique ptr to workaround segfault, similar…

9bf1b8f

… to the initial implementation

Benjamin-Tan requested a review from ahcorde April 21, 2024 14:21

ahcorde requested changes Apr 22, 2024

View reviewed changes

Fix undefined reference when using clang

13fb135

ahcorde requested changes Apr 26, 2024

View reviewed changes

Benjamin-Tan requested a review from ahcorde May 3, 2024 10:48

ahcorde requested changes May 15, 2024

View reviewed changes

ahcorde assigned Benjamin-Tan May 15, 2024

ahcorde requested a review from mikeferguson May 15, 2024 11:10

ahcorde requested changes May 15, 2024

View reviewed changes

mikeferguson reviewed May 15, 2024

View reviewed changes

Benjamin-Tan added 3 commits May 20, 2024 21:50

Include headers from PR comments

4c6d460

Include headers from PR comments <2>

c00ac8b

Benjamin-Tan added 2 commits May 20, 2024 22:23

Improve implementation to use shared ptr directly

9cf7398

Merge remote-tracking branch 'upstream/rolling' into rolling

745b12f

ahcorde requested changes Jun 14, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/rolling' into rolling

84caa21

Benjamin-Tan requested review from ahcorde and mikeferguson September 8, 2024 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support lifecycle node #304

Support lifecycle node #304

Benjamin-Tan commented Apr 6, 2024

ahcorde left a comment

Benjamin-Tan commented Apr 21, 2024

ahcorde left a comment

ahcorde left a comment

Benjamin-Tan commented Apr 22, 2024

ahcorde left a comment

Benjamin-Tan commented Apr 26, 2024

authaldo commented Apr 28, 2024

ahcorde May 15, 2024

Benjamin-Tan May 20, 2024

ahcorde commented May 15, 2024

ahcorde left a comment

mikeferguson May 15, 2024

Benjamin-Tan May 20, 2024 •

edited

Loading

mikeferguson May 15, 2024

mikeferguson May 15, 2024

Benjamin-Tan May 20, 2024

Benjamin-Tan commented May 20, 2024

ahcorde left a comment

Arun-Prasad-V commented Sep 6, 2024

Benjamin-Tan commented Sep 8, 2024

Support lifecycle node #304

Are you sure you want to change the base?

Support lifecycle node #304

Conversation

Benjamin-Tan commented Apr 6, 2024

ahcorde left a comment

Choose a reason for hiding this comment

Benjamin-Tan commented Apr 21, 2024

ahcorde left a comment

Choose a reason for hiding this comment

ahcorde left a comment

Choose a reason for hiding this comment

Benjamin-Tan commented Apr 22, 2024

ahcorde left a comment

Choose a reason for hiding this comment

Benjamin-Tan commented Apr 26, 2024

authaldo commented Apr 28, 2024

ahcorde May 15, 2024

Choose a reason for hiding this comment

Benjamin-Tan May 20, 2024

Choose a reason for hiding this comment

ahcorde commented May 15, 2024

ahcorde left a comment

Choose a reason for hiding this comment

mikeferguson May 15, 2024

Choose a reason for hiding this comment

Benjamin-Tan May 20, 2024 • edited Loading

Choose a reason for hiding this comment

mikeferguson May 15, 2024

Choose a reason for hiding this comment

mikeferguson May 15, 2024

Choose a reason for hiding this comment

Benjamin-Tan May 20, 2024

Choose a reason for hiding this comment

Benjamin-Tan commented May 20, 2024

ahcorde left a comment

Choose a reason for hiding this comment

Arun-Prasad-V commented Sep 6, 2024

Benjamin-Tan commented Sep 8, 2024

Benjamin-Tan May 20, 2024 •

edited

Loading