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

QoS Profile Overrides - Player #353

Merged
merged 12 commits into from
Apr 13, 2020
Merged

QoS Profile Overrides - Player #353

merged 12 commits into from
Apr 13, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Apr 6, 2020 •
edited
Loading

  • Add playback option to override QoS profiles for topics.
  • Enable SusbcriptionManager to accept a QoS profile.

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

Signed-off-by: Anas Abou Allaban aabouallaban@pm.me

@Karsten1987 Karsten1987 mentioned this pull request Apr 6, 2020
Closed
21 tasks
@piraka9011 piraka9011 changed the title WIP: QoS Profile Overrides - Player QoS Profile Overrides - Player Apr 7, 2020
@piraka9011 piraka9011 marked this pull request as ready for review April 7, 2020 14:04
@emersonknapp emersonknapp added this to In progress in Foxy API Freeze Apr 7, 2020
Karsten1987
Karsten1987 reviewed Apr 7, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

Generally this looks good to me. I just feel that we could enhance the tests a bit more.

rosbag2_test_common/include/rosbag2_test_common/subscription_manager.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play.cpp Outdated Show resolved Hide resolved
emersonknapp
emersonknapp reviewed Apr 8, 2020
View reviewed changes
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play.cpp Outdated
Comment on lines 150 to 160
// The previous subscriber requested durability VOLATILE which is the default in rosbag2.
// We override the requested durability to TRANSIENT_LOCAL so that we can receive messages.
// If the previous subscription requested TRANSIENT_LOCAL and we overrode with VOLATILE, then we
// would not receive any messages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add "wait for condition" checks with timeouts to check that "something doesn't happen". My only complaint with these tests is that they take a minimum of.. say 3 seconds as a reasonable timeout, so the test suite starts to take a long time when we have a lot of them. I can't think of a better way, though, given the mechanics of DDS discovery...

zmichaels11
zmichaels11 approved these changes Apr 8, 2020
View reviewed changes
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play.cpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp mentioned this pull request Apr 10, 2020
Merged
@piraka9011
Copy link
Contributor Author

@emersonknapp I was able to resolve the fastrtps issue by using reliability policies instead of durability. Might need to open an issue against fastrtps for this.

@piraka9011
Copy link
Contributor Author

piraka9011 commented Apr 10, 2020 •
edited
Loading

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/61645b2f212b620d7b4ad528ee410b87/raw/4222ca24ef476e6593aaa38d45e555c8feadc6e8/ros2.repos
BUILD args: --packages-up-to rosbag2_transport
TEST args: --packages-select rosbag2_transport
Job: ci_launcher

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

ament_export_interfaces() warning was resolved in #360

@Karsten1987
Copy link
Collaborator

There are quite a few tests failing on Windows. Is that known?

@emersonknapp
Copy link
Collaborator

Aware now, not sure where it came from. Investigating - insights welcome

@piraka9011
Copy link
Contributor Author

piraka9011 commented Apr 10, 2020 •
edited
Loading

@Karsten1987 @emersonknapp they all appear to be the same

Failed to find symbol 'rosidl_typesupport_connext_c__get_message_type_support_handle__rcl_interfaces__msg__Log' in library
[ERROR] [1586489902.463660500] [rosbag2_transport]: Failed to play: failed to initialize rcl node: type support handle implementation 'rosidl_typesupport_c' 
...
Failed to find symbol 'rosidl_typesupport_introspection_cpp__get_message_type_support_handle__test_msgs__msg__Strings' in library
[ERROR] [1586489931.244866000] [rosbag2_test_common]: Leaking memory. Error: rmw_serialize: type support trouble, at C:\ci\ws\src\ros2\rmw_cyclonedds\rmw_cyclonedds_cpp\src\rmw_node.cpp:1137
...

and

C++ exception with description "failed to initialize rcl node: type support handle implementation 'rosidl_typesupport_c' (00007FF81EFD4210) does not match valid type supports ('rosidl_typesupport_connext_cpp' (00007FF81EE831C0), 'rosidl_typesupport_connext_c' (00007FF81EE32170)), at C:\ci\ws\src\ros2\rmw_connext\rmw_connext_cpp\src\rmw_publisher.cpp:82, at C:\ci\ws\src\ros2\rcl\rcl\src\rcl\publisher.c:173" thrown in the test fixture's constructor.

Seems to be an issue with the class loader in RMW somewhere?

@piraka9011
WIP: QoS Profile Overrides - Player
b7e9530 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Improve testing & docs
274741b 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Provide sub manager qos override
bbeba80 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Refactor magic number
688cefe 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Use rosbag2 QOS
aadc953 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Add test, extract f-n
90e44db 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Add more tests
36ef95f 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Update iterator
88e1cfd 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
auto and brace init
71f37af 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Improve test and docs
7bb9df1 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Fix tests, move f-n, return early
2535ab1 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Use get() instead of wait_for()
37cc76f 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Copy link
Contributor Author

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

@piraka9011 piraka9011 merged commit 72a62ea into ros2:master Apr 13, 2020
@piraka9011 piraka9011 deleted the allabana/override-qos-pb branch April 13, 2020 16:49
@wjwwood wjwwood moved this from In progress to Done in Foxy API Freeze Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-moulard thomas-moulard thomas-moulard left review comments

@emersonknapp emersonknapp emersonknapp approved these changes

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@zmichaels11 zmichaels11 zmichaels11 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Foxy API Freeze
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@piraka9011 @Karsten1987 @emersonknapp @thomas-moulard @zmichaels11