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

Support lifecycle node #304

Open
wants to merge 16 commits into
base: rolling
Choose a base branch
from

Conversation

Benjamin-Tan
Copy link

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

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

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

@Benjamin-Tan
Copy link
Author

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

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

These changes should fix the issue

image_transport/src/camera_publisher.cpp Outdated Show resolved Hide resolved
image_transport/src/camera_subscriber.cpp Outdated Show resolved Hide resolved
image_transport/src/image_transport.cpp Outdated Show resolved Hide resolved
image_transport/src/publisher.cpp Outdated Show resolved Hide resolved
image_transport/src/subscriber.cpp Outdated Show resolved Hide resolved
@Benjamin-Tan
Copy link
Author

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.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

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
Copy link

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.

@Benjamin-Tan Benjamin-Tan requested a review from ahcorde May 3, 2024 10:48
image_transport/test/test_message_passing_lifecycle.cpp Outdated Show resolved Hide resolved
}

/*
TEST_F(MessagePassingTestingLifecycle, stress_message_passing)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove ?

Copy link
Author

Choose a reason for hiding this comment

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

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

image_transport/test/test_publisher_lifecycle.cpp Outdated Show resolved Hide resolved
image_transport/test/test_qos_override_lifecycle.cpp Outdated Show resolved Hide resolved
image_transport/test/test_subscriber_lifecycle.cpp Outdated Show resolved Hide resolved
@ahcorde
Copy link
Collaborator

ahcorde commented May 15, 2024

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

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

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

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

std::shared_ptr<NodeType> node_;
Copy link
Member

Choose a reason for hiding this comment

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

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?\

Copy link
Author

@Benjamin-Tan Benjamin-Tan May 20, 2024

Choose a reason for hiding this comment

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

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.

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

std::shared_ptr<NodeType> node_;
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Author

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.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

conflicts

@Arun-Prasad-V
Copy link

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

@Benjamin-Tan
Copy link
Author

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

@ahcorde ahcorde added the ros2 Issues or Pull Requests targeting ROS2 label Sep 26, 2024
@ahcorde
Copy link
Collaborator

ahcorde commented Sep 26, 2024

@mikeferguson do you mind to take a look here?

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you mind to merge with rolling? This PR is some PR behind.

I tried to compile the image_transport plugins and some other packages in image_pipeline and build is broken. Do you mind also to take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros2 Issues or Pull Requests targeting ROS2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ROS2] image_transport does not support LifecycleNode
5 participants