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

Update CM service QoS so that we don't loose service calls when using many controllers. #643

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

destogl
Copy link
Member

@destogl destogl commented Feb 12, 2022

In a project, we are using many controllers that are (almost all) activated when starting ros2_control_node.
The other controllers should be at least configured on start.
We are using spawners for this, which are calling CM services in the background.

Until now, we used rolling release from 9-30-2021 for core ROS packages. ros2_control is used from source.
Trying to port the code to newer rolling, e.g., 11-11-2021 or the newest 1-28-2022 we get an error that not all controllers are getting configured or activated as before.
After some investigation, we realized that the default QoS profile for services is keeping only 10 callbacks in queue and dropping the rest.
This PR provides a solution to this which is very CM-specific, i.e., keep all history of callbacks and do not drop any of them.

Maybe this also deserves a discussion on ROS2 RMW-packages, but still, we should merge this here for us to provide reliable behavior to users.

@destogl destogl force-pushed the update-qos-service-profile-for-cm branch from 6a38352 to 8a76043 Compare February 12, 2022 12:28
@nbbrooks
Copy link

The Parameters profile looks appropriate here.

Here is a summary of the current QOS profiles available:

QOS Name Reliability History
Default Reliable Last 10
Services Reliable Last 10
Parameters Reliable Last 1,000
Sensor data Best effort Last 5

Personally, I think Default should not be a thing, and there should be a "high_freq_data" profile that is Best effort, Last 1. And it should be used for nearly all pub/sub, except for low frequency publishers where a drop could actually have an impact. Maybe rename sensor data to low_freq_data. I'm not a DDS expert by any means though.

@bijoua29
Copy link
Contributor

Not sure Parameters is the appropriate profile since any number you use, even 1000, is arbitrary if semantically you mean that you don't want to drop any updates. The correct setting for that is 'keep all'.

@nbbrooks
Copy link

Not sure Parameters is the appropriate profile since any number you use, even 1000, is arbitrary if semantically you mean that you don't want to drop any updates. The correct setting for that is 'keep all'.

Good point. Maybe Parameters should keep all?

Else there could be a new profile for reliable, keep all.

Co-authored-by: Nicky Kim <kheo1772@gmail.com>
@destogl destogl merged commit 89f4db8 into master Feb 18, 2022
@destogl destogl deleted the update-qos-service-profile-for-cm branch February 18, 2022 09:25
pac48 pushed a commit to pac48/ros2_control that referenced this pull request Jan 26, 2024
…s-controls#659)

(cherry picked from commit b47b42df93e264c901ee8293b4db3c4c2e1fcc19)

Co-authored-by: gwalck <guillaume.walck@stoglrobotics.de>
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.

None yet

5 participants