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

QoS improvements for image_proc and stereo_image_proc #922

Merged
merged 4 commits into from
Feb 4, 2024

Conversation

mikeferguson
Copy link
Member

@mikeferguson mikeferguson commented Feb 1, 2024

First part of #847

  • Add QoS overrides for all publishers (in the new, standard way)
  • stereo_image_proc: Default subscriber QoS to SensorDataQoS
  • Clean up some of the comments around lazy subscribers, make them more consistent across nodes

@mikeferguson
Copy link
Member Author

A few notes:

  • The default QoS for the publishers has actually always been a bit sketchy and nondeterministic - since the publisher we are subscribing to may or may not actually exist yet when the QoS is probed. In those cases, we get a default of SensorDataQoS (which, is maybe not what we want, if we want to follow REP-2003 we would actually want SystemDefaultsQoS - although we may be stretching REP-2003 a bit here). The situation is a bit better for subscribers now that we lazily subscribe to them, which means their creation is delayed.

I almost think we should just remove the automatic publisher QoS and set it to SystemDefaultQoS + Overrides (which is at least deterministic).

@mikeferguson mikeferguson requested a review from ahcorde February 1, 2024 03:00
pub_options.qos_overriding_options = rclcpp::QosOverridingOptions::with_default_policies();

// Create publisher with QoS matched to subscribed topic publisher
auto qos_profile = getTopicQosProfile(this, image_topic_);
Copy link
Member Author

Choose a reason for hiding this comment

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

only change here is actually passing in image_topic_ rather than "image_raw" (technically still worked before, but leaves open the possibility of the topic names getting out of sync)

(
'right/image_rect_color',
[LaunchConfiguration('right_namespace'), '/image_rect_color']
),
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a relevant patch from #720 (which was an alternative approach, before the overriding was really available)

@mikeferguson mikeferguson merged commit af7f154 into ros-perception:rolling Feb 4, 2024
3 checks passed
@mikeferguson mikeferguson deleted the qos_overhaul branch February 4, 2024 00:43
Kotochleb pushed a commit to Kotochleb/image_pipeline that referenced this pull request May 27, 2024
…#922)

First part of ros-perception#847 
* Add QoS overrides for all publishers (in the new, standard way)
* stereo_image_proc: Default subscriber QoS to SensorDataQoS
* Clean up some of the comments around lazy subscribers, make them more
consistent across nodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants