Add parameter to use system default QoS for image subscriptions in stereo_image_proc#521
Conversation
The parameter lets the user configure the reliability QoS for the camera image and camera info topics. In some applications, we want to ensure the arrival of messages. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Same as done previously for the point cloud node. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
…file Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
I don't know what's up with CI. It looks like the build didn't make it to stereo_image_proc. |
That's typically an error I see when I'm trying to compile complex code on an embedded platform because its out of RAM with no swap set. I re-ran it. I suspect its just a one time CI blip from Circle's side. edit: failed again. Try removing the 2 worker threads here. Though this PR is still building fine off dashing, but I don't see any changes you've made that would really push this over the edge. I'm curious on your opinion on how to handle this generally. Throughout navigation, image pipeline, pcl, etc we have parameters to set what would be "typically" sensor QoS to standard QoS. I wonder if its possible to standardize this kind of if/else configuration or if its possible to do a runtime remap of the QoS settings for a publisher like we can with the topic names. e.g. |
SteveMacenski
left a comment
There was a problem hiding this comment.
I'm fine with it as is, though I think the parameter name and the logic could be cleaned up. We have the QoS set to sensor by default and if the param use_system_default_qos is true, then we just override it as the system default QoS.
This is in line with the ouster drivers and seems to be a more general solution for typically sensor data that you may want to override to reliable. I'm open to thoughts though. I'm not sure I care the specific mechanics as long as there's something reasonable we can generalize to ROS2 packages processing sensor data.
There are reasons to not allow configuring the QoS generally (it could break a node authors assumptions), however, IMO there should be a standard mechanism in ROS 2 that authors can opt-in to, allowing certain QoS settings to be configured. I think deciding what QoS should be configurable and how we configure it deserves more thought and so I was planning to create a design doc to propose and discuss ideas. I likely won't get to it until post-Foxy, but if you're so inclined you are welcome to start brainstorming and a design doc. |
That's another interesting thought: giving ranges of valid QoS that can be configured. I'd hope though that there's some standard way of representing those parameters concisely and not fill up yaml files with QoS. That's a more general solution and I think that's the way to probably go. From my experience, the only time I've exposed these is for swapping between sensor and default QoS for pretty much the same reason you submitted this PR, hence a focus on that use-case. Anyhow, conversation for another time and place. Do you have an objection to the |
Give the user the option to use the RMW QoS settings for the publishers and subscriptions in the stereo pipeline. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
SteveMacenski
left a comment
There was a problem hiding this comment.
One last question and otherwise good to go
|
|
||
| pub_disparity_ = create_publisher<stereo_msgs::msg::DisparityImage>("disparity", 1); | ||
| const auto disparity_pub_qos = use_system_default_qos ? | ||
| rclcpp::SystemDefaultsQoS() : rclcpp::QoS(1); |
There was a problem hiding this comment.
Why rclcpp::QoS(1) and not rclcpp::SensorDataQoS()? I see the existing code was just the 1, but if we're exposing this option, shouldn't it be SensorDataQoS?
There was a problem hiding this comment.
Ci is working now too, oddly
There was a problem hiding this comment.
rclcpp::QoS(1) is equivalent to just passing 1, which is using the default QoS with depth = 1. This actually has different settings than the SensorDataQoS().
There was a problem hiding this comment.
I understand that, I'm asking for that change to be made to SensorDataQoS to complement the subscribers for consistency (that we should have had before)
There was a problem hiding this comment.
Ah, sorry, misunderstood.
In the original port, I consciously left the disparity pub/sub pair (and the point cloud publisher) reliable by default as it was closest to the ROS 1 behaviour. But, seeing we've already changed the image subscribers to best effort, I don't mind changing all pubs and subs to use best effort by default if that's your preference.
There was a problem hiding this comment.
I'll say that I think any decision is better than what's there before this PR. I don't think we need to go too overboard in deciding this. Lets come up with the best for now and there should be an image-pipeline-full analysis of QoS and making it all consistent later.
There was a problem hiding this comment.
I suppose the question would be: in a pipeline of processing data, at what point should we switch from unreliable to reliable (or should we at all) between stages?
I think this is the question. And I'd posit the answer is different per application; we're not going to find an ideal configuration for everyone. By adding an option to fall back to the RMW configuration (system default), we give the user full control over how they'd like to configure the pipeline.
For default behaviour, I think making the nodes in the pipeline the most flexible (most compatible with other QoS settings) is a reasonable choice. For reliability, this means defaulting all subscriptions to best effort and all publishers to reliable. I think this is the current state of the PR.
There was a problem hiding this comment.
I'm not sure I agree since the default config or Qos(1) are incredibly similar. I think that's a misuse of the variable for use_system_default_qos since the opposite for a subscriber is a sensordataQoS and the publisher is essentially the same thing.
I'd merge this if we just removed the use of use_system_default_qos for the publisher entirely and just defaulted to system default for that (for now). We can leave that bear alone for another time.
There was a problem hiding this comment.
Maybe we have a different view of use_system_default_qos. To me it means the user is responsible for configuring the QoS directly with the RMW. I don't think it's safe to assume the ROS default settings (QoS(1)) will be similar to the RMW default.
I've removed the use of the flag for the publisher for now (9827cbe).
If we change the publishers to default to SensorDataQoS in the future, I suggest releasing the change into a new ROS distro because it will break out-of-the-box experience for users. E.g. point clouds in RViz will not render without changes to the RViz point cloud display QoS configuration.
There was a problem hiding this comment.
Yeah, fair enough. We're still in porting mode at the moment. We're nearing the point where we can do a release but its currently unreleased in any ROS2 distribution as of date.
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
Waiting on CI |
In some applications, it is desired to have a "reliable" stereo pipeline where we ensure that stereo image messages are always received (ie. no dropped messages). This change gives the user that option.
We are still defaulting to best effort reliability for subscriptions because it is the least restrictive when it comes to publisher/subscription QoS compatibility.