Introduce QoS property #409

Martin-Idel · 2019-06-06T07:27:13Z

Fix #360

Idea:

Introduce a new set of properties to any display with a topic to change all interesting QoS settings
In order not to clutter the panel too much (the settings are probably only relevant to a subset of users), we hide the settings below a topic property.
This allows to cleanup the unreliable property as well as the queue_size property.

Not in this PR:

There are to-do comments to switch to using rclcpp::QoS. I did not do this change to simplify reviewing and because at least for the property, I think it is much easier to work with the rmw profile as it simplifies working with all options.

Feedback needed for:

What kind of feedback can and should we give the user if messages with an incompatible profile are shown? For instance, if the publisher uses volatile but I set transient local, then no message gets received. Should we and how can we provide user feedback?
Do we need the lifetime options for RViz (no real documentation so I'm not sure whether this is necessary for rviz)?
I'm unsure whether all options in the enums contained in rmw_qos_profile_t are necessary. In particular, I'm unsure whether we need the Unknown options.
I would suggest adding a separate QoS property for every topic, i.e.: if a display can subscribe to multiple topics (e.g. map or camera), every topic property should get an associated qos property. This has not been done, yet.
Publishing displays/tools should probably also use this property?

jacobperron · 2019-09-10T01:50:27Z

What kind of feedback can and should we give the user if messages with an incompatible profile are shown? For instance, if the publisher uses volatile but I set transient local, then no message gets received. Should we and how can we provide user feedback?

I think this would be nice to have, but I'm not sure the best way to detect incompatible publishers. I think one way would involve comparing the number of matched publishers to the actual number of publishers for a given topic. We can get the number of matched publishers with SubscriptionBase::get_publisher_count(), and number of publishers on a topic with NodeGraph::count_publishers().

I think it we be okay to leave this as an enhancement "TODO" for the first pass.

Do we need the lifetime options for RViz (no real documentation so I'm not sure whether this is necessary for rviz)?

Consider that lifespan and liveliness are not addressed the relevant design document I would say we can ignore them for now.
We can always add them later as needed.

I'm unsure whether all options in the enums contained in rmw_qos_profile_t are necessary. In particular, I'm unsure whether we need the Unknown options.

I think the unknown options are more for error handling and we probably shouldn't include those as options in RViz.

I would suggest adding a separate QoS property for every topic, i.e.: if a display can subscribe to multiple topics (e.g. map or camera), every topic property should get an associated qos property. This has not been done, yet.

👍 Makes sense to me.

Publishing displays/tools should probably also use this property?

Seems reasonable. For example, I could imagine someone wanting the initial pose tool to have durability set to transient local.

Martin-Idel · 2019-09-11T21:01:57Z

I think it we be okay to leave this as an enhancement "TODO" for the first pass.

Sounds good to me.

I would suggest adding a separate QoS property for every topic, i.e.: if a display can subscribe to multiple topics (e.g. map or camera), every topic property should get an associated qos property. This has not been done, yet.

+1 Makes sense to me.

Map display and Camera (those are the two displays with an additional subscription) also use the qos profile in their second subscription now. However, I did not yet add another profile property. The reason is that both displays only have one topic property and the second subscription is derived "magically" from the first (e.g. the map update topic is the map topic + "_update"). Therefore, there is no good place to have a separate qos property. Does it make sense to have different QoS settings for the camera and the camera info topic? What about the map and map update topic?
If yes (and for the map, I'd say it makes sense), I'd propose adding a second topic property. If the name of the map_update and the camera_info topics are more than a convention (e.g. some rule), I could try to make them non-editable.

Seems reasonable. For example, I could imagine someone wanting the initial pose tool to have durability set to transient local.

Publishing tools now have their own qos_profile.

I tried to use the rclcpp::QoS-API everywhere, except for the property where I believe it's simpler to use the rmw types.

Apart from this, I'm nearly done - one test is failing that I have to fix.

Martin-Idel · 2019-09-11T23:13:10Z

Test is fixed - the rclcpp::QosInitialization::from_rmw tripped me up, it doesn't do what I thought from the name because it only fills certain fields...

I'm done for now, we only need to decide what to do with map and camera, otherwise this is good to review.

jacobperron · 2019-09-12T00:39:06Z

Does it make sense to have different QoS settings for the camera and the camera info topic? What about the map and map update topic?
If yes (and for the map, I'd say it makes sense), I'd propose adding a second topic property. If the name of the map_update and the camera_info topics are more than a convention (e.g. some rule), I could try to make them non-editable.

I agree it makes sense to have separate QoS settings for the map and map_update topics, and therefore an additional topic property. I guess it makes sense for the camera and camera_info topics as well. Looking at the image_transport code, it looks like they use the same QoS settings, but I think this is more of a convention. I'm not aware of any REP or standard stating otherwise.

I'm fine leaving this as a follow-up enhancement, but I'll leave it up to you.

Martin-Idel · 2019-09-14T07:33:07Z

@jacobperron I'm done. I tested it locally and it all seemed to work.

Regarding the camera and map display, here is what I did:

Since the camera display should use image tools anyway and there is an open ticket Camera display makes unclear assumptions about camera info topic #207 I'll leave it as is, i.e. the camera info topic will always have the same profile as the camera topic.
For the map display, I introduced a new read-only property for the update topic which has a QoS property. This means:
- You can change the QoS settings for the map and the update topic separately
- You can see where the update topic will be subscribed to
- But you cannot change where the update topic will subscribe - this is (as of now) fixed to the old behaviour

It's not difficult to change the behaviour to allow the user to change the update topic, too, but there are a few questions and fine points to this (Should we keep the old behaviour, i.e. automatically subscribe to the update topic? In code, the functions are a bit intertwined, one has to be careful to not produce an infinite update loop), so I think we should wait until somebody really needs this.

When this gets merged, we should also open a ticket to track future improvements for user feedback regarding subscriptions to existing topics which have different QoS properties.

jacobperron

Looks good to me, mostly just minor comments.

This will also need a rebase before running CI.

jacobperron · 2019-09-17T15:41:16Z

rviz_common/include/rviz_common/properties/qos_profile_property.hpp


 private Q_SLOTS:
-  void updateQueueSize();
+  void updateQosProfile();

 private:
  rviz_common::properties::IntProperty * queue_size_property_;


The term depth is used in the context of DDS. What do you think about renaming instances of "queue" to "depth"?

I used "depth" instead of "queue_size" as "depth_size" seemed redundant. Hopefully caught everything.

rviz_common/src/rviz_common/properties/qos_profile_property.cpp

rviz_common/include/rviz_common/properties/qos_profile_property.hpp

rviz_common/src/rviz_common/properties/qos_profile_property.cpp

rviz_common/test/properties/qos_profile_property_test.cpp

rviz_default_plugins/src/rviz_default_plugins/displays/map/map_display.cpp

- not a "true" property, but a container of properties - will call a callback when changed, similar to queue_size_property before - this will allow the property to be put below each ros topic property

- Will be superseded by QOS property

will be superseded by qos profile property

- below ros topic property to not needlessly clutter the panel

The fluent API is nice, but for the property itself, it seems easier to just use the rmw type. The alternative is a lot of if-statements which is not easier to read

Martin-Idel · 2019-09-19T18:23:37Z

I added the suggestions and rebased it. Ready for the next round.

jacobperron · 2019-09-19T18:44:01Z

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2019-09-19T20:27:50Z

There a Windows warning and looks like the change may have introduced some test failures on Linux and macOS

Deleting the queue size property changes the position of all properties. Tests which rely on the absolute position need to be changed, too.

Martin-Idel · 2019-09-19T21:25:49Z

The tests had a simple problem: Removing the queue size property (which was always below the topic property and thus the second property) changes the position of all other properties by one. There are a few tests which actually rely on the position of the property - those are the ones that failed.

So hopefully, that should fix all warnings and test failures (I ran the failing tests locally on Linux and they are fine now).

jacobperron

LGTM pending CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel mentioned this pull request Sep 7, 2019

Map is not rendered if not published before rviz launch #400

Closed

Martin-Idel force-pushed the introduce_qos_property branch from 1bf3aa2 to 9235106 Compare September 11, 2019 20:43

Martin-Idel force-pushed the introduce_qos_property branch from 9235106 to f3db9bf Compare September 11, 2019 23:11

jacobperron self-requested a review September 12, 2019 00:39

Martin-Idel force-pushed the introduce_qos_property branch from ee02d28 to 528e719 Compare September 14, 2019 07:21

jacobperron reviewed Sep 17, 2019

View reviewed changes

jacobperron mentioned this pull request Sep 18, 2019

Set MapDisplay topic durability to transient local #461

Closed

Martin-Idel added 8 commits September 19, 2019 02:54

Add qos_profile property as new property

2eb61fb

- not a "true" property, but a container of properties - will call a callback when changed, similar to queue_size_property before - this will allow the property to be put below each ros topic property

Delete queue size property

f2a7c74

- Will be superseded by QOS property

Delete unreliable property

2690e99

will be superseded by qos profile property

Use qos profile property in ros topic display

ac1dce1

- below ros topic property to not needlessly clutter the panel

Use rclcpp::QoS profile API where it makes sense

9bdceb5

The fluent API is nice, but for the property itself, it seems easier to just use the rmw type. The alternative is a lot of if-statements which is not easier to read

Add qos property to tools

e2df65f

Make sure that a resubscription happens on profile changes

27afd77

Add update property to map to allow different QoS settings

eff450b

Martin-Idel force-pushed the introduce_qos_property branch 2 times, most recently from 925eff4 to f3c91ec Compare September 19, 2019 18:19

Martin-Idel added 2 commits September 19, 2019 20:20

Fix review comments

9584141

Add methods for subscribing/unsubscribing to map update separately

a060815

Martin-Idel force-pushed the introduce_qos_property branch from f3c91ec to a060815 Compare September 19, 2019 18:21

Martin-Idel added 2 commits September 19, 2019 23:06

Fix Windows warning

4e541d2

Fix tests after introducing qos property

51ff430

Deleting the queue size property changes the position of all properties. Tests which rely on the absolute position need to be changed, too.

jacobperron approved these changes Sep 19, 2019

View reviewed changes

jacobperron merged commit 0ca33e3 into ros2:ros2 Sep 19, 2019

soldierofhell mentioned this pull request Nov 19, 2020

DataStreamROS2 Plugin incompatible subscription due to QoS profile facontidavide/PlotJuggler#309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce QoS property #409

Introduce QoS property #409

Martin-Idel commented Jun 6, 2019

jacobperron commented Sep 10, 2019 •

edited

Loading

Martin-Idel commented Sep 11, 2019

Martin-Idel commented Sep 11, 2019 •

edited

Loading

jacobperron commented Sep 12, 2019

Martin-Idel commented Sep 14, 2019

jacobperron left a comment

jacobperron Sep 17, 2019

Martin-Idel Sep 19, 2019

Martin-Idel commented Sep 19, 2019

jacobperron commented Sep 19, 2019

jacobperron commented Sep 19, 2019 •

edited

Loading

Martin-Idel commented Sep 19, 2019

jacobperron left a comment

Introduce QoS property #409

Introduce QoS property #409

Conversation

Martin-Idel commented Jun 6, 2019

jacobperron commented Sep 10, 2019 • edited Loading

Martin-Idel commented Sep 11, 2019

Martin-Idel commented Sep 11, 2019 • edited Loading

jacobperron commented Sep 12, 2019

Martin-Idel commented Sep 14, 2019

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron Sep 17, 2019

Choose a reason for hiding this comment

Martin-Idel Sep 19, 2019

Choose a reason for hiding this comment

Martin-Idel commented Sep 19, 2019

jacobperron commented Sep 19, 2019

jacobperron commented Sep 19, 2019 • edited Loading

Martin-Idel commented Sep 19, 2019

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron commented Sep 10, 2019 •

edited

Loading

Martin-Idel commented Sep 11, 2019 •

edited

Loading

jacobperron commented Sep 19, 2019 •

edited

Loading