Construction of transform listener

[domire8]

Since I was looking a bit closer into tf2_ros I saw that there are actually several ways to construct the TransformListener. Note that we are currently doing that in modulo:

this->tf_listener_ = std::make_shared<tf2_ros::TransformListener>(*this->tf_buffer_);

However, this constructor creates yet another node under the hood, one that we can see as transform_listener_impl_XXX. I don't know why I never questioned this but there is this constructor that takes the node as argument. I believe we should at least give the transform listener access to the node to avoid an additional one being created.

Additionally, there is also the spin_thread argument that defaults to true, see what it does here. It does create another executor under the hood but here I'm not sure if that is a good thing or not.

What do you think @eeberhard @bpapaspyros ?

[eeberhard]

Ah, very interesting, good catch. If every component with a transform listener is creating a new node and new executor, that adds a bit of complexity to the ros graph.

Using the parent node certainly makes a lot of sense.

Regarding the spin_thread arg, I see it serving two purposes:

• it creates an independent callback group, which prevents high-frequency tf messages from blocking other callbacks in the same group like services and so on (priority inversion)
• it creates a new thread for an executor, which means the independent callback group for tf messages can be executed in parallel even if the node is otherwise part of a single-threaded executor

In principle, it might be nice to use the existing executor for tf listeners - it means we don't rely on the kernel to resolve multithreaded scheduling but instead place the duty on the Executor (which, with the EventsExecutor, is very capable of handling such tasks and would give more control or access for debugging / profiling if needed). But, based on the implementation of the TF listener class, we aren't able to split the logic; we either use the parent executor and have the tf listeners in the default callback group (not ideal, because then they won't be executed in parallel even though they could be), or we have them in a new callback group but also executed by a new thread.

Also, the Python executor is still pretty awful so adding tf callbacks even in a separate callback group could bog it down regardless.

What we can take away from this is that the transform listener class is threadsafe in any case, so apart from the scheduling logic, spinning it in the background threaed is functionally equivalent to spinning it in parallel in a separate callback group of the main thread.

TL;DR, I would pass the parent node interface to the constructor and keep spin_thread true

[domire8]

That sounds good!

[domire8]

Interestingly, in Python the spin_thread is false by default

[eeberhard]

Interestingly, in Python the spin_thread is false by default

I think we should consider benchmarking how this behaves as True, since we noticed executor performance issues with Python components that were specifically noticeable with TF (manifesting as jittery frames which could be caused by blocked execution)

[domire8]

Note to self, propagate this to modulo controllers on the breaking branch

[domire8]

I thought about ways to benchmark this properly, but apart from checking how fast frames update in RViz, I don't really know. Do you have an idea.

Also, I looked more closely at the Python constructor and I assume that the spin_thread is disabled by default because in the Python implementation, the node is added entirely to the created executor. In the C++ case, only the callback group is added to the executor. Since a node can only be added to one executor, they maybe wanted to avoid weird default behavior in Python.