moveit2 icon indicating copy to clipboard operation
moveit2 copied to clipboard

Use sensor data QOS profile for sensor_msgs::Image and sensor_msgs::PointCloud

Open nbbrooks opened this issue 4 years ago • 3 comments

Description

I have been working with a Realsense lately and suspect there are some network traffic issues. I'm starting to read up on how we should be using DDS effectively. This first PR will handle updating point clouds/depth images publishers to use BEST_EFFORT for instead of RELIABLE. Some explanation of the differences are available here.

Currently the Realsense drivers use the system_default QOS, which has reliable settings, but the launch files take in parameters to change it for various topics. Testing this with a Realsense, you would need to launch it with the following parameters: pointcloud_qos:=SENSOR_DATA color_qos:=SENSOR_DATA depth_qos:=SENSOR_DATA infra_qos:=SENSOR_DATA

Similarly, Rviz plugins for visualizing image/pointcloud data should use Reliability Policy: Best Effort - I have not yet tested if it will autodetect the QOS settings from the publisher and match them.

When testing this PR with applications using streaming images, users should run ros2 doctor -r and look for warnings about mismatched QOS settings between pub and sub.

More info about some of the defined QOS policies:

"sensor_data": what we should use for streaming large data constructs

static const rmw_qos_profile_t rmw_qos_profile_sensor_data =
{
  RMW_QOS_POLICY_HISTORY_KEEP_LAST,
  5,
  RMW_QOS_POLICY_RELIABILITY_BEST_EFFORT,
  RMW_QOS_POLICY_DURABILITY_VOLATILE,
  RMW_QOS_DEADLINE_DEFAULT,
  RMW_QOS_LIFESPAN_DEFAULT,
  RMW_QOS_POLICY_LIVELINESS_SYSTEM_DEFAULT,
  RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT,
  false
};

"default" - what is currently used by MoveIt and by default by realsense

static const rmw_qos_profile_t rmw_qos_profile_default =
{
  RMW_QOS_POLICY_HISTORY_KEEP_LAST,
  10,
  RMW_QOS_POLICY_RELIABILITY_RELIABLE,
  RMW_QOS_POLICY_DURABILITY_VOLATILE,
  RMW_QOS_DEADLINE_DEFAULT,
  RMW_QOS_LIFESPAN_DEFAULT,
  RMW_QOS_POLICY_LIVELINESS_SYSTEM_DEFAULT,
  RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT,
  false
};

"system_default" - this is different than "default" and is defined by the ros middleware. I'm not sure what these values are for cyclone. This is just a note for people to know that in the context of ROS2 DDS "default" != "system_default"

static const rmw_qos_profile_t rmw_qos_profile_system_default =
{
  RMW_QOS_POLICY_HISTORY_SYSTEM_DEFAULT,
  RMW_QOS_POLICY_DEPTH_SYSTEM_DEFAULT,
  RMW_QOS_POLICY_RELIABILITY_SYSTEM_DEFAULT,
  RMW_QOS_POLICY_DURABILITY_SYSTEM_DEFAULT,
  RMW_QOS_DEADLINE_DEFAULT,
  RMW_QOS_LIFESPAN_DEFAULT,
  RMW_QOS_POLICY_LIVELINESS_SYSTEM_DEFAULT,
  RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT,
  false
};

Best-effort settings reduce the amount of network traffic since the DDS implementation does not have to incur the overhead of reliable communications, where publishers require acknowledgements for messages sent to subscribers and must resend samples that have not been properly received.

Work to be done:

  • [] Discuss adding the QOS to be used as a parameter versus just changing the hardcoded policy
  • [] Perform some characterization of if/when this has a significant impact on network performance

Checklist

  • [ ] Required by CI: Code is auto formatted using clang-format
  • [ ] Extend the tutorials / documentation reference
  • [ ] Document API changes relevant to the user in the MIGRATION.md notes
  • [ ] Create tests, which fail without this PR reference
  • [ ] Include a screenshot if changing a GUI
  • [ ] While waiting for someone to review your request, please help review another open pull request to support the maintainers

nbbrooks avatar Sep 02 '21 23:09 nbbrooks

Codecov Report

Base: 50.92% // Head: 50.95% // Increases project coverage by +0.04% :tada:

Coverage data is based on head (1af8c7c) compared to base (8dcbe40). Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #664      +/-   ##
==========================================
+ Coverage   50.92%   50.95%   +0.04%     
==========================================
  Files         378      378              
  Lines       31671    31671              
==========================================
+ Hits        16125    16135      +10     
+ Misses      15546    15536      -10     
Impacted Files Coverage Δ
moveit_ros/moveit_servo/src/pose_tracking.cpp 77.26% <0.00%> (+0.48%) :arrow_up:
moveit_ros/moveit_servo/src/servo_calcs.cpp 67.43% <0.00%> (+1.46%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Sep 03 '21 00:09 codecov[bot]

This pull request is in conflict. Could you fix it @nbbrooks?

mergify[bot] avatar Mar 11 '22 16:03 mergify[bot]

@henningkayser @jliukkonen I made some changes based on what you said, give it a look and let me know if anything else needs changing. I wasn't quite sure about the changes in depth_self_filter_nodelet so let me know if those aren't what you meant.

henrygerardmoore avatar Jun 28 '22 17:06 henrygerardmoore

I rebased this, if it passes CI I'm going to merge this. Speak now if I should not do that.

tylerjw avatar Nov 15 '22 16:11 tylerjw

This pull request is in conflict. Could you fix it @nbbrooks?

mergify[bot] avatar Nov 21 '22 17:11 mergify[bot]