Add Lazy subscription #195
Conversation
|
This seems like a good feature, thanks @bjsowa ! I've got a couple other PRs to review first, but hoping to get to this soon. |
|
Actually looks like #188 created some merge conflicts for this PR when I merged it. Could you fix those before I review the code? |
bjsowa
force-pushed
the
lazy-subscription
branch
from
b7f6131
to
f8bed40
Compare
|
I manually reapplied the changes as there were too many conflicts. The first 2 commits are not related to lazy subscription functionality. I can split them into separate PRs. The third commit adds the lazy subscription and drops support for ROS Humble. |
|
I split this PR to only include the lazy subscription part. @jonbinney I would appreciate if you could look at #197, #198 and #199 first |
|
Thanks for splitting this up, that will make it easier to review. Quick question - is it feasible to change this to avoid branching for humble (for example by using preprocessor macros?). As soon as we branch for humble we've got to start cherry picking bug fixes, which gets painful quickly. |
|
This should now build on Humble. I took some inspiration from depthai-ros, though I'm now 100% sure it will work on the buildfarm |
bjsowa
force-pushed
the
lazy-subscription
branch
from
511945b
to
736bbfc
Compare
bjsowa
force-pushed
the
lazy-subscription
branch
from
5679429
to
280113d
Compare
|
@jonbinney As all the "child" PR are merged, this one is ready for review. I squashed all commits as it was mostly resolving merge conflicts. |
Co-authored-by: Jonathan Binney <[email protected]>
src/scan_to_cloud_filter_chain.cpp
Outdated
| } | ||
| }; | ||
| cloud_pub_ = this->create_publisher<sensor_msgs::msg::PointCloud2>("cloud_filtered", | ||
| rclcpp::SensorDataQoS(), pub_options); |
There was a problem hiding this comment.
It looks like SensorDataQoS sets the "depth" (number of messages to hang on to in case publishing gets backed up) to 5. If there's not a good reason to change it, I'd prefer to keep it to 10 (see the original create_publisher call). There are also some other QoS settings that differ in SensorDataQoS from just what you get if you just pass in the depth as a number. I think that for this PR, it would be better not to change those. You could do so in a separate PR, but we'd want to go over each element of the QoS struct and discuss why it should be changed.
src/scan_to_cloud_filter_chain.cpp
Outdated
| cloud_pub_ = this->create_publisher<sensor_msgs::msg::PointCloud2>("cloud_filtered", | ||
| rclcpp::SensorDataQoS(), pub_options); | ||
| } else { | ||
| cloud_pub_ = this->create_publisher<sensor_msgs::msg::PointCloud2>("cloud_filtered", |
There was a problem hiding this comment.
Same thing - prefer not to change QoS in this PR unless there is a strong reason
src/scan_to_scan_filter_chain.cpp
Outdated
| scan_sub_.subscribe(this, "scan", rmw_qos_profile_sensor_data); | ||
| } | ||
| }; | ||
| output_pub_ = this->create_publisher<sensor_msgs::msg::LaserScan>("scan_filtered", |
There was a problem hiding this comment.
Same thing as in the scan_to_cloud_filter_chain.cpp - I'd prefer we just pass in 1000 as the QoS for this PR.
src/scan_to_scan_filter_chain.cpp
Outdated
| scan_sub_.subscribe(this, "scan", rmw_qos_profile_sensor_data); | ||
| } | ||
| #else | ||
| output_pub_ = this->create_publisher<sensor_msgs::msg::LaserScan>( |
|
@jonbinney I kept the old QoS settings. A depth value of 1000 seems like an overkill (especially for a reliable publisher) but I'll stick to changes only related to lazy subscription. |
|
Looks good! Yes, 1000 is a lot of scans to keep. I suspect the original thought was "lets keep as many as we can without using crazy amounts of memory". Typical 2D scanners may have around 1000 ranges and intensities per scan, which would be about As far as reliability, even if the QoS is set to "reliable", if you are streaming data over wifi (say running SLAM on a desktop while a mobile robot drives around a building) you may lose connection here and there, especially when transitioning between access points. It's nice not to lose those scans. Thanks for this PR! And the others you made - they are big improvements! |
Implements #194 .
this requires Matched publisher events, which are supported from ROS Iron onward, so a separate branch for ROS Humble is required.
This PR also fixes some issues with parameters in scan filter chains:
get_parameterwon't work.I also changed
spin_sometospinas I see absolutely no reason to use the first method.I can split this PR into multiple ones if needed,