Skip to content

Make create_client accept rclcpp::QoS#1964

Merged
sloretz merged 2 commits intorollingfrom
sloretz__create_client_qos
Jul 11, 2022
Merged

Make create_client accept rclcpp::QoS#1964
sloretz merged 2 commits intorollingfrom
sloretz__create_client_qos

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Jul 7, 2022

This adds a version of create_client that accepts rclcpp::QoS instead of rmw_qos_profile_t. It's available via rclcpp::create_client, rclcpp::Node::create_client, and rclcpp_lifecycle::LifecycleNode::create_client.

While here I also made the implementation of rclcpp_lifecycle::LifecycleNode::create_client use the free function rclcppp::create_client.

@sloretz
Make create_client accept rclcpp::QoS
bc06a30 
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
@sloretz sloretz added the enhancement New feature or request label Jul 7, 2022
@sloretz sloretz self-assigned this Jul 7, 2022
@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Jul 7, 2022

Should we deprecate the old signature?

fujitatomoya
fujitatomoya reviewed Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix looks good to me, but i got the same question #1964 (comment).
rmw_qos_profile_t should not be used in rclcpp?
the follow-up would be ParametersQoS?

@sloretz
Deprecate passing rmw_qos_profile_t to create_client
25ef848 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
This was referenced Jul 8, 2022
Merged
Closed
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jul 8, 2022

Should we deprecate the old signature?

25ef848 deprecates the old signature, and ros2/system_tests#506 uses the new signature downstream.

the follow-up would be ParametersQoS?

Once this goes in I'll follow up with similar changes to create_service. I see ParameterService and Sync/AsyncParameterClient have the same issue, so those would be a good follow up too. I opened an issue for those in case I don't get time #1967

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jul 8, 2022

CI (build: --packages-above-and-dependencies rclcpp rclcpp_lifecycle test: --packages-select rclcpp rclcpp_lifecycle test_rclcpp test_quality_of_service)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@sloretz sloretz merged commit f6056be into rolling Jul 11, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__create_client_qos branch July 11, 2022 17:09
@sloretz sloretz mentioned this pull request Jul 11, 2022
Merged
@Abishalini Abishalini mentioned this pull request Oct 4, 2022
Merged
6 tasks
@mergify mergify bot mentioned this pull request Mar 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@alsora alsora alsora approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@sloretz sloretz

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sloretz @alsora @mjcarroll @fujitatomoya