Should we provide a node to TF listeners?

[orduno]

We're not passing a node to the tf listener constructor in a few places on our code, for example:

https://github.com/ros-planning/navigation2/blob/a05c9440e76dfd0d76f243740c143a2a1b5d7c1d/nav2_costmap_2d/src/costmap_2d_ros.cpp#L135

Which by default will create a node and spin it:
https://github.com/ros2/geometry2/blob/64a19215f60aca42471a7da202085169dd39e6cc/tf2_ros/src/transform_listener.cpp#L59

This creates five transform listener nodes on our stack. From ros2 node list we get:

/transform_listener_impl_55c875370a30
/transform_listener_impl_55d272afebe0
/transform_listener_impl_55d45e65e560
/transform_listener_impl_56124de85420
/transform_listener_impl_5628bcd073e0

For example, in the case of Costmap2DROS (which is a node) it could just pass itself.

I'd prefer to leave it as is for now unless it is causing problems - at least till the reentrancy problem is addressed.

If we put the transform listener on the same node, we now need to worry about providing a thread for the transform listener, or we need to ensure we can never wait for a transform.

[SteveMacenski]

we need to ensure we can never wait for a transform

I think inherently we do this because we have to wrap all TF calls for exception handling.

My understanding of the TF2 listener as opposed to TF1 is that the Listener's entire goal is to create the node to spin and give the transforms from the topics to the buffer for use. If you give it a node, you're really just taking away the listener and its just a wrapper around a callback.

I don't necessarily think that's bad, but I'd ask Tully why there's no constructor to pass a node. There may be good reason. If there isn't, I'm 100% on board with removing the extra node as you've proposed

we need to ensure we can never wait for a transform

I think inherently we do this because we have to wrap all TF calls for exception handling.

The main problem with waiting for a transform was that sim_time couldn't advance and time out the wait if the node with the clock wasn't spinning.

The new setCreateTimerInterface, I believe, is meant to avoid that problem. You can now provide a clock on a separately running node.

However, I just realized that we already provide our node as the source of that clock in the setCreateTimerInterface call. We probably have the worst of both worlds; we can't block the rclcpp_node or time will stop advancing, but we also have the extra node.

If we go ahead with this change, though, we need to be careful. I saw a lot more dropped transforms when we shared the main node's thread. I'd feel better about making this change if we switched to multi-threaded executors with the incoming transforms being processed as a separate callback group.

[SteveMacenski]

That sounds reasonable to me, multithreaded executors aren't something to be avoided

[SteveMacenski]

Is there an action from this ticket? Should we file something in geometry2 or were folks impressions to leave ‘as is’?

I'd vote for "leave 'as is'". Trying to provide our own node caused performance problems when ros2/geometry2#121 got merged in. I think we really need to consider multi-threaded executors if we want to put the tf_listener on the same node. I don't think that's worth the effort at the moment. Maybe it's something to revisit if we switch to multi-threaded executors for something else.

[SteveMacenski]

I think maybe filing a ticket on geometry2 with the proposal and getting some advice on best way to make it all work out to minimize the number of nodes, but I agree I don't think there's an action for our stack without a larger change / understanding

[SteveMacenski]

Closing- I think we’re just leaving to status queue

[roncapat]

@SteveMacenski I would like to ask you whether it may be possible or not to revive this at distance of 5 years or so for re-evaluation (either by reopening or by opening a new one).
At least in one place a TF Listener is created using the node:

navigation2/nav2_bt_navigator/src/bt_navigator.cpp

Line 57 in 40a0451

tf_listener_ = std::make_shared<tf2_ros::TransformListener>(*tf_, this, false);

and I believe that, at distance of years, just ensuring spin_thread = true would work, with the benefit of clearly understanding via ros2 topic info -v /tf for example who is listening (instead of having random node names).

[SteveMacenski]

Sure thing - identify the places that we do this with a suggested change and lets talk about it in a new ticket. Note that we only added spin_thread as part of TF as a Nav2 contribution 4 years ago, so when this was discussed that wasn't an option. It looks like you might find some legacy locations we didn't adjust that for