Reduce transform listener nodes

[gezp]

Signed-off-by: zhenpeng ge zhenpeng.ge@qq.com

As described in #440, I use new callback group and executor with dedicated thread so that we could avoid creating internal node which is overhead.

currently, i don't break the API, but i think the constructor TransformListener(tf2::BufferCore & buffer, bool spin_thread = true) need be deprecated, due to create internal Node.

the another constructor is enough to use like this.

tf_buffer_ = std::make_shared<tf2_ros::Buffer>(get_clock());
//case1: spin in dedicated thread (use new callback group and executor)
tf_listener_ = std::make_shared<tf2_ros::TransformListener>(*tf_buffer_, shared_from_this(), true);
//case2: its on the application to spin
tf_listener_ = std::make_shared<tf2_ros::TransformListener>(*tf_buffer_, shared_from_this(), false);

[gezp]

i don't know how to fix the CI failures, i need some helps, thank you!

[SteveMacenski]

16:34:21 KeyError: "The cache has no package named 'ros-rolling-rclcpp'"

Not your fault, I don't believe

[SteveMacenski]

I feel like you can do better than options2 here. How about tf_options and tf_static_options?

[SteveMacenski]

A space here might be nice for a logical break between creating the subscribers and then setting up the executor

[SteveMacenski]

@tfoote I'd try to assign you as a reviewer if I had the permissions to do so 😄

Edit: Whoops, looks like @clalancette is the listed maintainer.

[tfoote]

From a quick read through this looks good. I haven't fully validated or tested it. But the internal node creation was a workaround for the lack of callback groups. Switching back to the dedicated thread with a callback group is a good step forward.

[SteveMacenski]

@tfoote what are the next steps here? I'd looked over it again and I don't see any reason this wouldn't be functionally the same.

[gezp]

please review this PR, i need your suggestion, thank you! @clalancette @tfoote.

[ahcorde]

• Linux
• Linux-aarch64
• macOS
• Windows

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1

[SteveMacenski]

@ahcorde any other action required for merging? Not that I'm a maintainer here, but I took a look and I see no issues that would pop up either.

[jacobperron]

I think this broke an existing use-case where we expect the node to have other callbacks processed, not just the the two subscriptions added here. E.g. previously I could write something like this:

#include <rclcpp/rclcpp.hpp>
#include <tf2_ros/buffer.h>
#include <tf2_ros/transform_listener.h>
#include <std_msgs/msg/string.hpp>

#include <iostream>
#include <memory>
#include <chrono>
#include <thread>

int main(int argc, char ** argv)
{
  rclcpp::init(argc, argv);

  auto node = std::make_shared<rclcpp::Node>("foo_node");

  auto callback = [](const std_msgs::msg::String & msg) {
    std::cerr << "Message received: " << msg.data << std::endl;
  };  
  auto sub = node->create_subscription<std_msgs::msg::String>("foo", 1, callback);

  tf2_ros::Buffer buffer(node->get_clock());
  tf2_ros::TransformListener listener(buffer, node, true);

  while (rclcpp::ok()) {
    // rclcpp::spin_some(node);
    std::this_thread::sleep_for(std::chrono::seconds(1));
  }
  rclcpp::shutdown();

  return 0;
}

Maybe assuming callbacks will be handled by the transform listener is a bad assumption, but the signature where we provide our own Node (and we set spin_thread=true) seems ill-defined.

[SteveMacenski]

my_node is never spun in this example -- how could the subscription receive messages? The spin_thread is w.r.t. the TF callbacks to isolate them from the rest of the system, its not there to process your own callbacks in your own application. That was the intent of this PR to make TF2's system decoupled from the application code. You should spin_some() in your while loop to resolve this issue (which you should have been doing before this PR anyway?). This PR is creating an executor / callback group for the TF internal needs to be processed, so that's not connected to your application subscriber using the default executor that you haven't called in the application example. 😄

[jacobperron]

I might be wrong about what I think is happening; I will report back with a complete example.

[jacobperron]

I've updated the code in my comment to be a compilable example. It appears that I can get the example to work if I uncomment the rclcpp::spin_some() call in the while-loop. IIRC, this would result in a runtime error previously. Should it be the new recommended approach?

[SteveMacenski]

Yes -- before I think it was a design error to have it spin for the user application too, that seems like a breakage in separation of concerns to have TF2 processing your execution model for your application. I would have assumed that it did what it is now doing and just spin a thread for TF2 to process its information internally. This was made because we want TF2 to process its own info and not be blocked or blocking other application callbacks & reducing the number of Node objects in complex stacks like Nav2.

[jacobperron]

I agree that it makes sense to not have users rely on the TransformListener executor, however the current change silently breaks anyone who was relying on this behavior (it took me a while to figure out why my callbacks were not being triggered). It would be nice if we could detect this scenario and warn users. At the very least, I think we should add a release note for ROS Humble.

[SteveMacenski]

100% agree

[jacobperron]

It does not seem straight forward to add a warning, but I've added a release note in ros2/ros2_documentation#2127

[jacobperron]

It might also be worth pointing out here that the Python implementation still follows the old logic of spinning on the entire node (instead of a callback group containing only the TF topics):

geometry2/tf2_ros_py/tf2_ros/transform_listener.py

Lines 93 to 96 in 16562ce

	def run_func():
	self.executor.add_node(self.node)
	self.executor.spin()
	self.executor.remove_node(self.node)

I can create an issue to track updating it as well.