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

Support for specifying QoS per subscriber/publisher/etc? #1

Open
gavanderhoorn opened this issue Jul 8, 2022 · 12 comments
Open

Support for specifying QoS per subscriber/publisher/etc? #1

gavanderhoorn opened this issue Jul 8, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@gavanderhoorn
Copy link

(My apologies for not following the template, but this isn't really a bug report, more of a question. I'd posted it on the Discussions forum, but it's not enabled)

Thanks for making this binding available.

Trying out creating a subscriber for a topic published by a publisher not-under-my-control, I discovered the subscriptions created by Ros2 use RELIABLE, VOLATILE. My publisher used BEST_EFFORT, VOLATILE, which makes them incompatible.

The only reference to QoS is on the Publisher documentation page, and it seems it cannot be changed for subscribers.

I haven't checked, but what would be needed to support specifying QoS parameters per subscribers/publisher/other-QoS-affected-entity?

It's the only thing blocking me from being able to no longer stare at terminals and grepping output of ros2 topic echo ... :)

@gavanderhoorn gavanderhoorn added the bug Something isn't working label Jul 8, 2022
@StefanFabian
Copy link
Owner

It's not that difficult to add.
It would require adding and declaring an enum for the values and adding some overloads for the publisher creation.
I can see if I find some time for this in the next week but I'm currently at RoboCup and I may not have the time before August.

But you're right it's a lot more critical in ROS 2 than in ROS 1.

@StefanFabian
Copy link
Owner

Starting to work on it now.

@StefanFabian StefanFabian added enhancement New feature or request and removed bug Something isn't working labels Jul 28, 2022
@StefanFabian
Copy link
Owner

StefanFabian commented Jul 28, 2022

Alright, unfortunately, I don't have a lot of time to work on this in the next two weeks but I have finished the implementation and some tests for the Subscription. (It's a first draft though so some details might still change)
You can check it out on the feature/qos branch.
The example is also updated to show how to use it.

Roadmap:

  • Subscription
  • Publisher
  • ImageTransportSubscription
  • Documentation

Not sure if it also makes sense for ActionClients, ServiceClients, and Tf.

@gavanderhoorn
Copy link
Author

Hi, thanks already 👍

I'll try to take a look next week and test it.

I'll submit some PRs if I can figure out how to add the necessary changes to publishers and the rest of the entities you mention.

@StefanFabian
Copy link
Owner

The changes aren't that big, if you require them, I can add the functionality and follow up with tests later.
Otherwise, a potential paper deadline currently has a higher priority and I'll finish the rest afterward.

@gavanderhoorn
Copy link
Author

I'm getting back to this, and I'm wondering how far we want to take this.

Almost all ROS 2 entities wrapping DDS readers/writers have associated QoS settings. Would we want to expose those in the QML wrapper as well?

Subscriptions and publications are obvious. But service servers and clients also support it -- although I've seen most users just assuming the defaults will work for them (ie: the default profile).

Action servers and clients also support configuring QoS, but since those are composites of publishers, subscribers and service servers and clients, configuration gets a bit more complicated.

I've not yet looked at how BabelFish supports with QoS for service servers and clients, but first wanted to discuss a bit whether those should be included at all. Personally I'd say yes, but you're the maintainer here ;)

@StefanFabian
Copy link
Owner

Generally, I am always a fan of feature completeness. The question is mostly about priority.
I'm not sure what the use cases are where it would make sense to change the QoS for services or actions.
It's more obvious for topics since they may transmit data with high frequency and short liveliness or low frequency with order dependent data.

I've not yet looked at how BabelFish supports with QoS for service servers and clients, but first wanted to discuss a bit whether those should be included at all.

If it doesn't, I can add it but since most of the implementations for that are just adapted from the default rclcpp implementation, I would assume it does support it without having looked at it.

@gavanderhoorn
Copy link
Author

Service QoS is easily added I believe. BabelFish supports it already.

I won't do Actions then.


Somewhat related: feature/qos seems to contain a couple more commits compared to when I forked it. Two of which appear to be duplicated commits from master. Would you want to fix that, or was that on purpose?

@StefanFabian
Copy link
Owner

Somewhat related: feature/qos seems to contain a couple more commits compared to when I forked it. Two of which appear to be duplicated commits from master. Would you want to fix that, or was that on purpose?

Yeah, I've committed them to the wrong branch and cherry-picked back to master.
I can rewrite the history and remove them but I don't really see an issue with them being there, do you?

I won't do Actions then.

Are you working on a PR right now?
Just so we don't work on the same thing.

@gavanderhoorn
Copy link
Author

Are you working on a PR right now?

yes.

I can rewrite the history and remove them but I don't really see an issue with them being there, do you?

If you're happy, I'm happy :)

@StefanFabian
Copy link
Owner

Okay, then I'll work on humble support for LOEWE-emergenCITY/ros_babel_fish#2 first.

The action client in ros2_babel_fish also supports QoS btw.

@gavanderhoorn
Copy link
Author

Submitted a draft in #3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants