Added rclcpp component to Republish #275

ahcorde · 2023-06-30T15:07:19Z

These changes allow to use the republisher as a rclcpp_component and as a normal node.

An example of how to use this:

ros2 run image_transport republish --in_transport compressed in/compressed:=/camera/image/compressed out/compressed:=/camera/republish/compressed out:=/camera/republish/image_raw

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

adityapande-1995

Looks good ! Could you add a test or an example ? Maybe here : https://github.com/ros-perception/image_common/tree/noetic-devel/image_transport/tutorial/src ?

adityapande-1995 · 2023-07-06T22:36:25Z

image_transport/include/image_transport/republish.hpp

+class Republisher : public rclcpp::Node
+{
+public:
+  //! Constructor


Is the "!" a typo ?

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

adityapande-1995

Looks good ! Is there a way to add a test here ?

mikeferguson

I don't understand why you go through manually processing the parameters?

mikeferguson · 2024-02-15T15:12:19Z

image_transport/src/republish.cpp

-    auto pub = image_transport::create_publisher(
-      node.get(), out_topic,
+    this->pub = image_transport::create_publisher(
+      this->shared_from_this().get(), out_topic,


Suggested change

this->shared_from_this().get(), out_topic,

this, out_topic,

mikeferguson · 2024-02-15T15:12:32Z

image_transport/src/republish.cpp

-    auto sub = image_transport::create_subscription(
-      node.get(), in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),
+    this->sub = image_transport::create_subscription(
+      this->shared_from_this().get(), in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),


Suggested change

this->shared_from_this().get(), in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),

this, in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),

mikeferguson · 2024-02-15T15:12:46Z

image_transport/src/republish.cpp

    // Load transport plugin
    typedef image_transport::PublisherPlugin Plugin;
    pluginlib::ClassLoader<Plugin> loader("image_transport", "image_transport::PublisherPlugin");
    std::string lookup_name = Plugin::getLookupName(out_transport);

    auto instance = loader.createUniqueInstance(lookup_name);
    std::shared_ptr<Plugin> pub = std::move(instance);
-    pub->advertise(node.get(), out_topic);
+    pub->advertise(this->shared_from_this().get(), out_topic);


Suggested change

pub->advertise(this->shared_from_this().get(), out_topic);

pub->advertise(this, out_topic);

mikeferguson · 2024-02-15T15:12:58Z

image_transport/src/republish.cpp

-    auto sub = image_transport::create_subscription(
-      node.get(), in_topic,
+    this->sub = image_transport::create_subscription(
+      this->shared_from_this().get(), in_topic,


Suggested change

this->shared_from_this().get(), in_topic,

this, in_topic,

mikeferguson · 2024-02-15T15:15:15Z

image_transport/src/republish_program.cpp

+  // remove program name
+  args.erase(args.begin());
+
+  std::string in_transport{"raw"};
+  std::string out_transport{""};
+
+  if (image_transport::has_option(args, "--in_transport")) {
+    in_transport = image_transport::get_option(args, "--in_transport");
+  }
+  if (image_transport::has_option(args, "--out_transport")) {
+    out_transport = image_transport::get_option(args, "--out_transport");
+  }
+
+  rclcpp::NodeOptions options;
+  // override default parameters with the desired transform
+  options.parameter_overrides(
+  {
+    {"in_transport", in_transport},
+    {"out_transport", out_transport},
+  });


What is the point of all this? It seems to be the only specialized thing you are doing over the default behavior of generating an executable using rclcpp_components_register_node

mikeferguson · 2024-02-15T15:17:21Z

image_transport/CMakeLists.txt

+  rclcpp_components
+  rclcpp
+)
+rclcpp_components_register_nodes(republish_node "image_transport::Republisher")


You could actually remove the entire add_executable below, delete republish_program.cpp and the utilities files and generate a node that loads the plugin and runs it with:

rclcpp_components_register_node(republish_node PLUGIN "image_transport::Republisher" EXECUTABLE republish )

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2024-02-15T17:05:03Z

When using --out_transport the app is crashing.

mikeferguson · 2024-02-15T17:21:39Z

image_transport/src/republish.cpp

    // Load transport plugin
    typedef image_transport::PublisherPlugin Plugin;
    pluginlib::ClassLoader<Plugin> loader("image_transport", "image_transport::PublisherPlugin");
    std::string lookup_name = Plugin::getLookupName(out_transport);

    auto instance = loader.createUniqueInstance(lookup_name);
    std::shared_ptr<Plugin> pub = std::move(instance);
-    pub->advertise(node.get(), out_topic);
+    pub->advertise(this, out_topic);


should this be this->pub? rather than declaring a new one in the line above? Might that be your crash?

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2024-02-15T17:31:40Z

Yep, but I have now this awesome message

Warning: class_loader.ClassLoader: SEVERE WARNING!!! Attempting to unload library while objects created by this loader exist in the heap! You should delete your objects before attempting to unload the library or destroying the ClassLoader. The library will NOT be unloaded.
         at line 127 in /home/ahcorde/ros2_rolling/src/ros/class_loader/src/class_loader.cpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2024-02-15T17:41:32Z

Error is fixed too. The loader need to keep alive too

mikeferguson · 2024-02-15T19:49:40Z

Honestly - probably 70% of the ROS codebase that uses pluginlib has that error on shutdown

ahcorde · 2024-02-16T08:45:17Z

Linux
Linux-aarch64
Windows

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2024-02-19T21:07:46Z

Linux
Linux-aarch64
Windows

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde added 2 commits June 30, 2023 17:04

Added rclcpp component to Republish

2c4f76d

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

make linters happy

2bef0a2

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

adityapande-1995 reviewed Jul 6, 2023

View reviewed changes

Added feedback

68cb1f5

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from adityapande-1995 July 10, 2023 14:22

adityapande-1995 mentioned this pull request Jul 10, 2023

image_transport publisher nodes should support subscriber specific compression config #273

Open

adityapande-1995 approved these changes Jul 13, 2023

View reviewed changes

ahcorde added the ros1 label Jan 18, 2024

ahcorde added ros2 Issues or Pull Requests targeting ROS2 and removed ros1 labels Feb 15, 2024

Merge branch 'rolling' into ahcorde/rolling/republisher_component

d7de089

ahcorde requested a review from mikeferguson February 15, 2024 12:41

mikeferguson reviewed Feb 15, 2024

View reviewed changes

ahcorde added 2 commits February 15, 2024 18:02

Added feedback

a891224

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

make linters happy

dc66aa5

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from mikeferguson February 15, 2024 17:05

mikeferguson reviewed Feb 15, 2024

View reviewed changes

avoid crash

3807343

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Fixed error messag

4916dca

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from mikeferguson February 15, 2024 17:41

mikeferguson approved these changes Feb 15, 2024

View reviewed changes

Fixed windows warning

ae2e974

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde merged commit 1a559ed into ros-perception:rolling Feb 20, 2024
2 checks passed

ahcorde deleted the ahcorde/rolling/republisher_component branch February 20, 2024 08:59

tonynajjar pushed a commit to angsa-robotics/image_common that referenced this pull request May 15, 2024

Added rclcpp component to Republish (ros-perception#275)

29f5cca

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added rclcpp component to Republish #275

Added rclcpp component to Republish #275

ahcorde commented Jun 30, 2023

adityapande-1995 left a comment

adityapande-1995 Jul 6, 2023

ahcorde Jul 10, 2023

adityapande-1995 left a comment

mikeferguson left a comment

mikeferguson Feb 15, 2024

mikeferguson Feb 15, 2024

mikeferguson Feb 15, 2024

mikeferguson Feb 15, 2024

mikeferguson Feb 15, 2024

mikeferguson Feb 15, 2024

ahcorde commented Feb 15, 2024

mikeferguson Feb 15, 2024

ahcorde commented Feb 15, 2024

ahcorde commented Feb 15, 2024

mikeferguson commented Feb 15, 2024

ahcorde commented Feb 16, 2024

ahcorde commented Feb 19, 2024

	this->shared_from_this().get(), in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),
	this, in_topic, std::bind(pub_mem_fn, &pub, std::placeholders::_1),

	pub->advertise(this->shared_from_this().get(), out_topic);
	pub->advertise(this, out_topic);

Added rclcpp component to Republish #275

Added rclcpp component to Republish #275

Conversation

ahcorde commented Jun 30, 2023

adityapande-1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityapande-1995 left a comment

Choose a reason for hiding this comment

mikeferguson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Feb 15, 2024

Choose a reason for hiding this comment

ahcorde commented Feb 15, 2024

ahcorde commented Feb 15, 2024

mikeferguson commented Feb 15, 2024

ahcorde commented Feb 16, 2024

ahcorde commented Feb 19, 2024