swri_roscpp [Foxy]: TopicServiceClient spins node passed in during call

[jonselling]

When passing in a node to the topic service client, it will spin the node. This will conflict if we are trying to spin the node already externally, and this will cause an exception since the node is already added to an executor.

While it would be nice for the code to spin for you, it might be worthwhile to pass in a flag in the constructor if we want to disable the spinning ourselves.

As a side note, it might be good to make the interface a "rclcpp::Node::SharedPtr" to begin with if you are going to spin the node, instead of using a reference and then ".shared_from_this()".

[mboulet]

It may be worth considering a tf2-like model

[danthony06]

@jonselling sorry for the long delay on this. I'm trying to clear the backlog on a lot of our packages, and I'm starting in on this package. I think I can write the fix for this, but do you have a specific test case I could use to make sure my fix addresses your problem?

[jonselling]

I think a simple test case would be to construct a node that creates a TopicServiceClient and a timer on different callback groups, spins the node with a mutli-threaded spinner, and then call a service in that timer callback. A TopicServiceServer does need to be running from an external node.

Spinning the node may be a desirable option for someone (analogous to spin_until_future_complete) so maybe passing a bool in the constructor or "call" function could be a viable option.

[danthony06]

@jonselling I'm working on this, but I'm also trying to update it so it more closely matches the behavior of ROS2 services, which is taking a little more thought than I expected.

[jonselling]

Is that a requirement? Or do we want to preserve functionality of topic services in ROS1?

When you say match the functionality of ROS 2 services are you talking about making these calls asynchronous?

[danthony06]

My main concern is as written, we can hang forever waiting for the pub/sub pair to become available. This seems a lot more likely in ROS 2, with the focus on poor communication channels and fully distributed computing. I'm working on adding a wait_for call in the system to mimic the ROS2 service functionality.

On Wed, Jun 15, 2022 at 6:38 PM Jonathan Selling @.***> wrote:

Is that a requirement? Or do we want to preserve functionality of topic services in ROS1?

When you say match the functionality of ROS 2 services are you talking about making these calls asynchronous?

— Reply to this email directly, view it on GitHub https://github.com/swri-robotics/marti_common/issues/633#issuecomment-1157076390, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBZWHEQTZ5LUQXG2AOH6DTVPJSP7ANCNFSM5GVVIU7Q . You are receiving this because you commented.Message ID: @.***>

[danthony06]

Okay, I'm going to test this tomorrow and work through the bugs: https://github.com/swri-robotics/marti_common/compare/ros2-devel...danthony06:optional_spin?expand=1

[danthony06]

@jonselling I think this PR is going to fix this issue: https://github.com/swri-robotics/marti_common/pull/672

[danthony06]

https://github.com/swri-robotics/marti_common/pull/672

[jonselling]

FWIW I think the structure is there to optionally spin, however it is not accessible externally since it is an addition to the "Raw" service client class and the regular service client class does not access or set the input so it is always true (we will always spin the node).

Also, there might be some issues even if that was fixed - basically if you use the .call(...) method in a ROS callback (timer or subscription) then it will lock the node and wait indefinitely if using a single threaded spinner (which most people use by default). There is some trickery needed that isn't just "then use a multi-threaded spinner" because by default a multi-threaded spinner acts single threaded unless you setup multiple callback groups.