ros2_rust icon indicating copy to clipboard operation
ros2_rust copied to clipboard

QoS for Clients and Services

Open samouwow opened this issue 1 year ago • 7 comments

Addresses #391

samouwow avatar May 20 '24 06:05 samouwow

@samouwow thanks for your PR. However, I'm hesitant to accept this change, neither rclcpp or rclpy accept arbitrary QoS settings for clients and services, among some of the reasons is the issue of interoperability. Publishers and subscribers accept different values for their QoS, but require the other end to have compatible settings (e.g. a publisher's reliability QoS is set as fire and forget, whereas a subscription is created as reliable wouldn't work). Adding this change to the API may break compatilibity with the rest of client libraries.

esteve avatar May 20 '24 14:05 esteve

@esteve thanks for the quick response.

As best I can tell both rclpy and rclcpp have an API to provide a custom QoS when creating clients and servers, this PR would simply bring rclrs in line with those other libraries.

I will concede both of those client libraries have the QoS as default values, would you feel more comfortable if rclrs had one method for creating clients/servers with QoS and one without?

E.g. create_client() and create_client_qos()?

P.s. in case it helps the argument, I need this feature to interop with a C++ ROS2 node, so from my perspective the lack of this API breaks compatibility

samouwow avatar May 20 '24 23:05 samouwow

@samouwow my bad, you're right, I thought neither APIs had that option. Adding a secondary API that takes QoS argument would work, or perhaps just keep create_client as a legacy API and have a Client builder instead would feel more idiomatic

@jhdcs @maspe36 thoughts?

esteve avatar May 21 '24 04:05 esteve

I like the idea of a client builder. Rust does like its builder patterns, after all.

jhdcs avatar May 21 '24 14:05 jhdcs

I also think we should look into creating a client builder instead of proliferating create_<type>_<extras> functions.

maspe36 avatar Jul 14 '24 18:07 maspe36

@samouwow thanks for all the work. Would you be interested in adding builders for Clients and Services? That way we could add QoS settings and many other options when either structs are created.

esteve avatar Aug 15 '24 12:08 esteve

I've opened #427 which introduces an option builder pattern to allow services and clients to override specific fields in the default client/service QoS. It applies the same option builder pattern to subscriptions and publishers, so all the different primitive creation functions are consistent.

I believe that addresses the last round of feedback on this PR.

mxgrey avatar Nov 17 '24 17:11 mxgrey

We now have the options builder for services and clients merged, which allows users to set their QoS. I believe it would be reasonable to close this PR now.

mxgrey avatar Aug 11 '25 13:08 mxgrey