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.

Sign up for GitHub

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

Jump to bottom

Add demo of how to use qos overrides #474

Merged
merged 8 commits into from
Dec 11, 2020
Merged

Add demo of how to use qos overrides #474

merged 8 commits into from
Dec 11, 2020

Conversation

ivanpauno
Copy link
Member

Simple talker/listener example showing how to use the qos overrides parameters.

@ivanpauno ivanpauno added the enhancement New feature or request label Nov 9, 2020
@ivanpauno ivanpauno requested a review from jacobperron November 9, 2020 16:50
@ivanpauno ivanpauno self-assigned this Nov 9, 2020
jacobperron
jacobperron reviewed Nov 9, 2020
View reviewed changes
quality_of_service_demo/rclcpp/src/qos_overrides_listener.cpp Outdated Show resolved Hide resolved
quality_of_service_demo/rclcpp/src/qos_overrides_talker.cpp Outdated Show resolved Hide resolved
quality_of_service_demo/README.md Outdated Show resolved Hide resolved
quality_of_service_demo/README.md Outdated Show resolved Hide resolved
@jacobperron
Copy link
Member

Depends on ros2/rclcpp#1408

jacobperron
jacobperron approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM with green CI.

quality_of_service_demo/rclcpp/src/qos_overrides_talker.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request Nov 9, 2020
Merged
wjwwood
wjwwood approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm. It would be good to address the TODO's as in my experience they don't often get addressed otherwise, but in general I think this turned out well.

quality_of_service_demo/rclcpp/src/qos_overrides_talker.cpp Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member Author

lgtm. It would be good to address the TODO's as in my experience they don't often get addressed otherwise, but in general I think this turned out well.

👍
I didn't add getters to rclcpp::QoS in the same PR, but I'm planning to open a follow up doing so.

@ivanpauno ivanpauno mentioned this pull request Nov 18, 2020
Merged
@ivanpauno
Copy link
Member Author

I will update how to code a qos validation callback after ros2/rclcpp#1467 gets in, but in the meantime we can get this one in:

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

@ivanpauno
Copy link
Member Author

I released a new rclcpp rolling version ros/rosdistro#27370 should fix the Rpr checker when it gets merged.

@clalancette
Copy link
Contributor

We had a lot of regressions on ci.ros2.org on the day that ros2/rclcpp#1465 went in: https://ci.ros2.org/view/nightly/job/nightly_win_rel/1755/#showFailuresLink . We don't yet know the exact cause of those regressions, but the QoS override patch is one of the few things that went in that day.

What I'm asking for here is to just hold off on merging this until we've identified which of the patches from Nov 17 caused the regression. That way there is less to back out if we need to do that. Thanks.

@ivanpauno
Copy link
Member Author

We had a lot of regressions on ci.ros2.org on the day that ros2/rclcpp#1465 went in: https://ci.ros2.org/view/nightly/job/nightly_win_rel/1755/#showFailuresLink . We don't yet know the exact cause of those regressions, but the QoS override patch is one of the few things that went in that day.

ros2/system_tests#452 is the bug report for those regressions, right?

@jacobperron
Copy link
Member

I don't think there's anything holding this up anymore.

@clalancette
Copy link
Contributor

I don't think there's anything holding this up anymore.

Agreed. The nightly builds are now really close to green, so whatever lingering problems there were I think we've fixed in the meantime. So I think we can go forward here.

@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

ivanpauno and others added 7 commits December 9, 2020 13:51
@ivanpauno @clalancette
Add demo of how to use qos overrides
5f495d7 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno @jacobperron @clalancette
spelling
560dacb 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @jacobperron @clalancette
spelling
7a3a567 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @jacobperron @clalancette
Improve readme
5c15f7d 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @jacobperron @clalancette
Improve readme
19ed1e1 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @clalancette
Update after last API changes
525eef8 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@clalancette
Remove unused message_size_ member variable.
066f427 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the ivanpauno/qos-overrides-demo branch from 224cb05 to 066f427 Compare December 9, 2020 14:01
@clalancette
Copy link
Contributor

CI:

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

@ivanpauno
Use rclcpp::QoS getters
7e57e57 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
jacobperron
jacobperron approved these changes Dec 10, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@ivanpauno
Copy link
Member Author

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

@ivanpauno ivanpauno merged commit 149e501 into master Dec 11, 2020
@ivanpauno ivanpauno deleted the ivanpauno/qos-overrides-demo branch December 11, 2020 14:35
@ivanpauno ivanpauno mentioned this pull request Jan 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@wjwwood wjwwood wjwwood approved these changes

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @jacobperron @clalancette @wjwwood