-
Notifications
You must be signed in to change notification settings - Fork 70
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
Document which QoS policies are correctly read by rmw_get_publishers/subscriptions_info_by_topic #308
Conversation
…subscriptions_info_by_topic Signed-off-by: Ivan Santiago Paunovic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I guess some RMW implementations should probably be updated to match this documentation. In that case, this may be considered an API change and perhaps we should hold until after we branch for Galactic.
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
The rmw implementations already match this behavior, so this is only documentation. I don't remember if they set the policies that are not updated to |
Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: William Woodall <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
bc07bf9
to
10b6c6b
Compare
Fastrtps was already doing that https://github.com/ros2/rmw_fastrtps/blob/7bb3563bebd20c3cae03231c2e321bf132651449/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp#L149. I send a patch to connextdds to fix this ros2/rmw_connextdds#28. |
PR checker is happy, this a docs only PR so no need of more CI. |
Based on ros2/rclcpp#1626 (comment).