Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QoS for Clients and Services #404

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

samouwow
Copy link

Addresses #391

@esteve
Copy link
Collaborator

esteve commented May 20, 2024

@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 esteve self-requested a review May 20, 2024 14:52
@samouwow
Copy link
Author

samouwow commented May 20, 2024

@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

@esteve
Copy link
Collaborator

esteve commented May 21, 2024

@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?

@jhdcs
Copy link
Collaborator

jhdcs commented May 21, 2024

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

@maspe36
Copy link
Collaborator

maspe36 commented Jul 14, 2024

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

@esteve
Copy link
Collaborator

esteve commented Aug 15, 2024

@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.

@mxgrey
Copy link
Collaborator

mxgrey commented Nov 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants