search

ros2 / rviz

ROS 3D Robot Visualizer
BSD 3-Clause Clear License
299 stars 212 forks source link

ROS abstraction needs a (multithreaded) executor to add nodes to #548

Open ddengster opened 4 years ago

ddengster commented 4 years ago

So I'm having issues whereby if create a subscriber, then I create a publisher and call publish() onto the same topics using the same node I will hit a deadlock. The fix I know of is to make a separate executor and add nodes to it ala this commit:

https://github.com/ros-planning/moveit2/pull/195/commits/a0c5c9ca43cb480f8ea80e600a3018432606328f

This should be considered a workaround.

And also from this old PR: https://github.com/ros2/rviz/pull/197 it seems there was a ros_abstraction function that added nodes to the executor but it was removed. Perhaps it's time to put it back in?

I've also noted that we use a SinglethreadedExecutor that spins in a update function at 100ms. This essentially impacts response timings. Perhaps we should use a MultithreadedExecutor and spin it on a thread?

sloretz commented 4 years ago

@wjwwood friendly ping :)

wjwwood commented 4 years ago

I don't know if we can use a multi-threaded spinner because I think there are some interactions with Qt that assume it's all happening in the main thread.

Instead I think that displays should have their own executor (as a group or individually). We already worked around something like this in https://github.com/ros2/rviz/pull/551 which involved creating a new node.

With https://github.com/ros2/rclcpp/pull/1218 we will have the ability to make new executors (therefore have more threads) as needed.

If someone wants to try any of these strategies to improve things then that would be appreciated but they will need to test it extensively to shake out any race conditions, especially if the idea is to try the multi-threaded executor approach (not my first choice).