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 QoS Profiles in CLI - Playback #356

Merged
merged 4 commits into from
Apr 13, 2020
Merged

Override QoS Profiles in CLI - Playback #356

merged 4 commits into from
Apr 13, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Apr 7, 2020 •
edited
Loading

  • Add a --qos-profile-overrides-path to the play verb.

Should go after #358

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

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

@piraka9011 piraka9011 force-pushed the allabana/override-qos-pb-cli branch from 8cdcc30 to d5b1deb Compare April 8, 2020 19:22
@Karsten1987 Karsten1987 mentioned this pull request Apr 8, 2020
Closed
21 tasks
@piraka9011 piraka9011 force-pushed the allabana/override-qos-pb-cli branch 3 times, most recently from 21d08e4 to 2b6d924 Compare April 10, 2020 13:04
@piraka9011 piraka9011 changed the title WIP: Override QoS Profiles in CLI - Playback Override QoS Profiles in CLI - Playback Apr 10, 2020
@piraka9011 piraka9011 marked this pull request as ready for review April 10, 2020 13:05
@piraka9011
Copy link
Contributor Author

There's a Cyclone DDS failure in the CI

   8: 1586535200.199535 [0] test_recor: using network interface eth0 (udp/10.1.0.4) selected arbitrarily from: eth0, docker0
  8: [INFO] [1586535200.212596225] [rosbag2_transport]: Listening for topics...
  8: [INFO] [1586535200.218610361] [rosbag2_transport]: Subscribed to topic '/mixed_nondelivery_policies'
  8: 
  8: >>> [rcutils|error_handling.c:108] rcutils_set_error_state()
  8: This error state is being overwritten:
  8: 
  8:   'Security was requested but the Cyclone DDS being used does not have security support enabled. Recompile Cyclone DDS with the '-DENABLE_SECURITY=ON' CMake option, at /home/runner/work/rosbag2/rosbag2/ros_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:834'
  8: 
  8: with this new error message:
  8: 
  8:   'the given context is not valid, either rcl_init() was not called or rcl_shutdown() was called., at /home/runner/work/rosbag2/rosbag2/ros_ws/src/ros2/rcl/rcl/src/rcl/guard_condition.c:69'
  8: 
  8: rcutils_reset_error() should be called after error handling to avoid this.
  8: <<<
  8: [ERROR] [1586535200.220040669] [rosbag2_transport]: Failed to record: Failed to create interrupt guard condition in Executor constructor: the given context is not valid, either rcl_init() was not called or rcl_shutdown() was called., at /home/runner/work/rosbag2/rosbag2/ros_ws/src/ros2/rcl/rcl/src/rcl/guard_condition.c:69

Seems to come from downstream. Might need to compile with the flag as specified in the warning.

@piraka9011
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/42ee6b2990c30503b44a3b8e9de2289b/raw/b8b685264d8466997d888639c19467b9f4119adb/ros2.repos
BUILD args: --packages-up-to ros2bag
TEST args: --packages-select ros2bag
Job: ci_launcher

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

@piraka9011
Copy link
Contributor Author

New run with flake8 fix

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

zmichaels11
zmichaels11 reviewed Apr 10, 2020
View reviewed changes
ros2bag/test/resources/empty_bag/metadata.yaml Show resolved Hide resolved
ros2bag/test/resources/empty_bag/metadata.yaml Outdated Show resolved Hide resolved
ros2bag/test/test_play_qos_profiles.py Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
topic_qos_overrides.insert(std::pair<std::string, rclcpp::QoS>(topic_name, qos_profile));
}
} else {
throw std::runtime_error{"QoS profile overrides object is not a Python dictionary."};
Copy link
Contributor

Choose a reason for hiding this comment

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

This if-else block could have been written as:

rcpputils::require_true(PyDict_Check(object), "QoS profile overrides object must be a Python dictionary.");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need to add rcpputils as a dependency to this package. I'd prefer to f/u that up in a refactor PR.

rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp Outdated Show resolved Hide resolved
emersonknapp
emersonknapp approved these changes Apr 11, 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.

This LGTM on addressing Zach's comments and green CI

@piraka9011
Improve tests for utilities
082d3a6 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Add overrides to play verb
431a952 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Flake8
2a85412 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Comments and brace-init
1407e5f 
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 7e35b10 into ros2:master Apr 13, 2020
@piraka9011 piraka9011 deleted the allabana/override-qos-pb-cli branch April 13, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmichaels11 zmichaels11 zmichaels11 left review comments

@emersonknapp emersonknapp emersonknapp 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.
3 participants
@piraka9011 @emersonknapp @zmichaels11