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

Override Subscriber QoS - Record #346

Merged
merged 13 commits into from
Apr 7, 2020
Merged

Override Subscriber QoS - Record #346

merged 13 commits into from
Apr 7, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Apr 2, 2020 •
edited
Loading

  • Add a qos_profiles string to the recording options
  • Expose Publisher's QoS profile option in PublisherManager
  • Override a topic's QoS profile if an override is specified.

Part of #125
Part of ros-tooling/aws-roadmap#218

Signed-off-by: Anas Abou Allaban [email protected]

@piraka9011 piraka9011 changed the title Override Subscriber QoS Override Subscriber QoS - Record Apr 2, 2020
@Karsten1987 Karsten1987 mentioned this pull request Apr 2, 2020
Closed
21 tasks
@piraka9011
Copy link
Contributor Author

piraka9011 commented Apr 3, 2020 •
edited
Loading

Remember that the ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION flag must be enabled.

add_definitions(-DROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION)
# Or
colcon build ... --cmake-args -DROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION

@emersonknapp I could add it temporarily to the CMakeLists to verify it compiles with the action and remove it later.

zmichaels11
zmichaels11 approved these changes Apr 3, 2020
View reviewed changes
rosbag2_transport/CMakeLists.txt Outdated Show resolved Hide resolved
emersonknapp
emersonknapp requested changes Apr 3, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Looking a lot better, some structural comments

rosbag2_transport/src/rosbag2_transport/recorder.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.cpp Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.hpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Show resolved Hide resolved
@piraka9011
Override Subscriber QoS
a84a33b 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Fix merge conflict
a0aa4b6 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Remove extra topic
db47c2b 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Auto and brace init
ecab114 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Switch override to map, move override logic
a93de07 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Rename subscription f-n and update docs
73f406d 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Add qos sub flag
64fac93 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Move ifdefs, update comments, fix include paths
5c0c12c 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Fix merge conflicts
e463ed9 
Signed-off-by: Anas Abou Allaban <[email protected]>
thomas-moulard
thomas-moulard reviewed Apr 6, 2020
View reviewed changes
rosbag2_transport/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_transport/CMakeLists.txt Show resolved Hide resolved
rosbag2_transport/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/recorder.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_record.cpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp mentioned this pull request Apr 7, 2020
Merged
@piraka9011
Clean up
ee80ffd 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Fix include dirs
398cc7f 
Signed-off-by: Anas Abou Allaban <[email protected]>
@piraka9011
Copy link
Contributor Author

@ros2/aws-oncall @prajakta-gokhale - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/42402c78b9ffea330dd6b88a3841b6d2/raw/aed855a901d65a26df9ff5eadc8e452ef6c20a2a/ros2.repos
BUILD args: --packages-up-to rosbag2_transport
TEST args: --packages-select rosbag2_transport
Job: ci_launcher

@piraka9011
Copy link
Contributor Author

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

@emersonknapp
Copy link
Collaborator

The Windows cmake warning seems ephemeral, there is no actual output except mayybe something around spdlog. All tests passed on Windows, which I am not used to seeing. Considering this a clean CI run and merging

@emersonknapp emersonknapp merged commit aa36517 into ros2:master Apr 7, 2020
@piraka9011 piraka9011 deleted the override-subscription-qos branch April 7, 2020 19:44
@piraka9011 piraka9011 mentioned this pull request Apr 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dabonnie dabonnie dabonnie left review comments

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@mm318 mm318 mm318 approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@piraka9011 @emersonknapp @thomas-moulard @mm318 @zmichaels11 @dabonnie