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.

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

Constrain the list of topics bridge_logger subscribes to to only those actually published by some vehicle_topics.launch file #757

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

peci1
Copy link
Collaborator

@peci1 peci1 commented Jan 19, 2021

If bridge_logger is meant to only record statistics for topics published from the bridge container (a.k.a. vehicle_topics.launch of some robot), it actually records more than that. This could be a possible performance problem. And it's very easy to run into it and make it even worse.

The basic problem is that BridgeLogger detects the topics of interest by plain substring matching:

if (info.name.find("front_scan") != std::string::npos ||
info.name.find("points") != std::string::npos ||
info.name.find("image_raw") != std::string::npos ||
info.name.find("depth") != std::string::npos ||
info.name.find("imu") != std::string::npos ||
info.name.find("magnetic_field") != std::string::npos ||
info.name.find("air_pressure") != std::string::npos ||
info.name.find("battery_state") != std::string::npos ||
info.name.find("imu") != std::string::npos)

This matches too much. This is an example list of topics recorded on our EXPLORER_X1_SENSOR_CONFIG_2 robot (called X1):

$ ls bridge_logger--*
bridge_logger--X1-battery_state.log			  bridge_logger--X1-points_slow.log
bridge_logger--X1-front_rgbd-depth-image_raw.log	  bridge_logger--X1-points_slow_filtered.log
bridge_logger--X1-front_rgbd-depth-optical-image_raw.log  bridge_logger--X1-points_slow_filtered_planner.log
bridge_logger--X1-front_rgbd-image_raw.log		  bridge_logger--X1-rear_rgbd-depth-image_raw.log
bridge_logger--X1-front_rgbd-optical-image_raw.log	  bridge_logger--X1-rear_rgbd-depth-optical-image_raw.log
bridge_logger--X1-front_rgbd-points.log			  bridge_logger--X1-rear_rgbd-image_raw.log
bridge_logger--X1-front_rgbd-points_slow.log		  bridge_logger--X1-rear_rgbd-optical-image_raw.log
bridge_logger--X1-front_rgbd-points_slow_filtered.log	  bridge_logger--X1-rear_rgbd-points.log
bridge_logger--X1-imu-data.log				  bridge_logger--X1-rear_rgbd-points_slow.log
bridge_logger--X1-imu_pose.log				  bridge_logger--X1-rear_rgbd-points_slow_filtered.log
bridge_logger--X1-left_rgbd-depth-image_raw.log		  bridge_logger--X1-right_rgbd-depth-image_raw.log
bridge_logger--X1-left_rgbd-depth-optical-image_raw.log   bridge_logger--X1-right_rgbd-depth-optical-image_raw.log
bridge_logger--X1-left_rgbd-image_raw.log		  bridge_logger--X1-right_rgbd-image_raw.log
bridge_logger--X1-left_rgbd-optical-image_raw.log	  bridge_logger--X1-right_rgbd-optical-image_raw.log
bridge_logger--X1-left_rgbd-points.log			  bridge_logger--X1-right_rgbd-points.log
bridge_logger--X1-left_rgbd-points_slow.log		  bridge_logger--X1-right_rgbd-points_slow.log
bridge_logger--X1-left_rgbd-points_slow_filtered.log	  bridge_logger--X1-right_rgbd-points_slow_filtered.log
bridge_logger--X1-other_viewpoints.log			  bridge_logger--X1-viewpoints.log
bridge_logger--X1-points.log				  bridge_logger--robot_data-X1-points_slow_filtered_planner-draco.log

I see there our own internal topics *_rgbd/points_slow[_filtered], imu_pose, other_viewpoints, viewpoints and robot_data/X1/points_slow_filtered_planner/draco (yes, even the robot_data namespace matches!). And if I run the simulation with our h264 image transport package, it even subscribes to the *_rgbd/image_raw/h264 and *_rgbd/depth/image_raw/h264 topics which is bad because they are pretty computationally intensive and we don't normally use them all the time.

So, instead of the generic substring matching, I rewrote the logger do the matching much more tightly - mostly using EndsWith() or HasPart() (which matches whole strings between slashes). This results in matching more closely just the sets of bridge-related topics.

To make sure no "proper" bridge topic gets lost, I went through all submitted models examining exactly what would the previous rules match, and making sure they would match them too with this PR. I did that by calling the following command in submitted_models folder for each of the substituted lines:

find . -name '*.launch' -exec grep -Po 'to="\K[^"]*front_scan[^"]*(?=")' {} \; | sort -u
front_scan
front_scan
points
downward_realsense/points
front/depth/points
front_down_rgbd_camera/depth/points
front_facing_rgbd_camera/depth/points
front_laser/points
pan_tilt_rgbd_camera/depth/points
points
rgbd_camera/depth/points
rgbd_camera_down/depth/points
rgbd_camera_up/depth/points
rgbd_front/points
rgbd_rear/points
tof_bottom/depth/points
tof_top/depth/points
image_raw
back_left/image_raw
back_right/image_raw
camera_front/image_raw
camera_front/optical/image_raw
camera_left/image_raw
camera_rear/image_raw
camera_rear/optical/image_raw
camera_right/image_raw
depth/image_raw
depth/optical/image_raw
down/image_raw
down/optical/image_raw
downward_realsense/image_raw
downward_realsense/optical/image_raw
front_down/image_raw
front_down/optical/image_raw
front_facing/image_raw
front_facing/optical/image_raw
front/image_raw
front/left/image_raw
front/left/optical/image_raw
front/optical/image_raw
front/right/image_raw
front/right/optical/image_raw
image_raw
left/image_raw
left/optical/image_raw
omni/camera_0/image_raw
omni/camera_0/optical/image_raw
omni/camera_1/image_raw
omni/camera_1/optical/image_raw
omni/camera_2/image_raw
omni/camera_2/optical/image_raw
omni/camera_3/image_raw
omni/camera_3/optical/image_raw
omni/camera_4/image_raw
omni/camera_4/optical/image_raw
omni/camera_5/image_raw
omni/camera_5/optical/image_raw
optical/depth/image_raw
optical/image_raw
pan_tilt/image_raw
pan_tilt/optical/image_raw
rear/image_raw
rear/optical/image_raw
rgbd_front/image_raw
rgbd_front/optical/image_raw
rgbd_rear/image_raw
rgbd_rear/optical/image_raw
right/image_raw
right/optical/image_raw
up/image_raw
up/optical/image_raw
depth Most of it gets covered by suffixes `points`, `depth` and `image_raw`. Just Explorer R2 has `depth/image` which is made a special case in this PR.
depth/camera_info
depth/image
depth/image_raw
depth/optical/image_raw
down/depth
down/optical/depth
downward_realsense/depth
downward_realsense/optical/depth
front/depth
front/depth/points
front_down/depth
front_down/optical/depth
front_down_rgbd_camera/depth/points
front_facing/depth
front_facing/optical/depth
front_facing_rgbd_camera/depth/points
front/optical/depth
optical/depth/camera_info
optical/depth/image
optical/depth/image_raw
pan_tilt/depth
pan_tilt/optical/depth
pan_tilt_rgbd_camera/depth/points
rgbd_camera/depth/points
rgbd_camera_down/depth/points
rgbd_camera_up/depth/points
rgbd_front/depth
rgbd_front/optical/depth
rgbd_rear/depth
rgbd_rear/optical/depth
tof_bottom/depth
tof_bottom/depth/points
tof_bottom/optical/depth
tof_top/depth
tof_top/depth/points
tof_top/optical/depth
up/depth
up/optical/depth
imu
imu/data
imu/front/data
imu/rear/data
magnetic_field
magnetic_field
magnetic_field/front
magnetic_field/rear
air_pressure
air_pressure
battery_state
battery_state

The previous implementation also matched a few camera_info topics, but as OnSensorMsg doesn't have a CameraInfo handler, they got ignored anyways (but subscribed). I chose to ignore them, but it's easy to add these to the bridge logger, too.

I also removed the whole section of code that ignored topics like parameter_updates etc. - it's no longer needed.

This PR solves #595 (as a side-effect).

@peci1
Copy link
Collaborator Author

peci1 commented Jan 19, 2021

This is the list of files I get with our EXPLORER_X1 robot with this PR:

$ ls bridge_logger--X1-*
bridge_logger--X1-battery_state.log                       bridge_logger--X1-points.log
bridge_logger--X1-front_rgbd-depth-image_raw.log          bridge_logger--X1-rear_rgbd-depth-image_raw.log
bridge_logger--X1-front_rgbd-depth-optical-image_raw.log  bridge_logger--X1-rear_rgbd-depth-optical-image_raw.log
bridge_logger--X1-front_rgbd-image_raw.log                bridge_logger--X1-rear_rgbd-image_raw.log
bridge_logger--X1-front_rgbd-optical-image_raw.log        bridge_logger--X1-rear_rgbd-optical-image_raw.log
bridge_logger--X1-front_rgbd-points.log                   bridge_logger--X1-rear_rgbd-points.log
bridge_logger--X1-imu-data.log                            bridge_logger--X1-right_rgbd-depth-image_raw.log
bridge_logger--X1-left_rgbd-depth-image_raw.log           bridge_logger--X1-right_rgbd-depth-optical-image_raw.log
bridge_logger--X1-left_rgbd-depth-optical-image_raw.log   bridge_logger--X1-right_rgbd-image_raw.log
bridge_logger--X1-left_rgbd-image_raw.log                 bridge_logger--X1-right_rgbd-optical-image_raw.log
bridge_logger--X1-left_rgbd-optical-image_raw.log         bridge_logger--X1-right_rgbd-points.log
bridge_logger--X1-left_rgbd-points.log

@osrf-jenkins
Copy link

Build finished. 15 tests run, 0 skipped, 0 failed.

4 similar comments
@osrf-jenkins
Copy link

Build finished. 15 tests run, 0 skipped, 0 failed.

@osrf-jenkins
Copy link

Build finished. 15 tests run, 0 skipped, 0 failed.

@osrf-jenkins
Copy link

Build finished. 15 tests run, 0 skipped, 0 failed.

@osrf-jenkins
Copy link

Build finished. 15 tests run, 0 skipped, 0 failed.

@nkoenig nkoenig merged commit 8f5187b into osrf:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants