Add function for checking QoS profile compatibility

[jacobperron]

Given QoS profiles for a publisher and a subscription, this function
will return true if the profiles are compatible.
Compatible profiles implies that the subscription and publisher will
be able to communicate.

Connected to ros2/rmw#299

[jacobperron]

This conflicts with #43, so I'll likely have to rebase once that lands.

[jacobperron]

Compatibility checks are based on the tables found here: https://index.ros.org/doc/ros2/Concepts/About-Quality-of-Service-Settings/#qos-compatibilities

[jacobperron]

@ivanpauno Thanks for the review. I think I've address all of your comments.

[audrow]

This looks good to me. The QoS compatability check function makes sense, and the tests do a nice job to confirm that results are as expected for different QoS profile combinations. Nice job!

[mjeronimo]

Just for my edification: I've always wondered about the inconsistent terminology, IMO: "publisher" and subscription" versus, "publisher" and "subscriber" or "publication" and "subscription." Any quick background info on that?

[jacobperron]

TBH, I can't recall why that terminology. I'm just following suit with the terminology used throughout the rest of the ROS 2 stack, though I think we're also inconsistent about what terminology we're using 🙃 The tutorials seem to use "subscriber" instead of "subscription".

[jacobperron]

@wjwwood Probably has some insight.

[wjwwood]

A publisher publishes, a subscription is a receipt of a subscription, it isn't a subscriber. So subscriber is the wrong term. A node is kind of a subscriber as it is the thing that subscribes. A publication could be a thing, but because our object publishes, it is a publisher.

[mjeronimo]

@wjwwood Interesting. I guess I just think of it more like this (probably attracted to the symmetry/uniformity):

Anyways, thanks for the info. It helps to fill in some of the blanks.

[eboasson]

The policies in ROS have this annoying SYSTEM_DEFAULT thing which I have always read as the application explicitly "don't care, do what you think is best", which doesn't make much sense to me. Secondly, I've always interpreted that setting as valid only when creating an endpoint, but never as a value returned by the underlying middleware.

If that interpretation is correct (which it may not be, given that the SYSTEM_DEFAULT is handled later in this function), then it seems illogical that a default deadline setting would be treated differently here. Doesn't it make far more sense to simply say the default deadline is ∞ (however represented)? That way, there always is a deadline.

If you then define the comparison functions such that ∞ is handled correctly, this all becomes much simpler, IMHO.

[ivanpauno]

Secondly, I've always interpreted that setting as valid only when creating an endpoint, but never as a value returned by the underlying middleware.

Yes, that should be the case.

If that interpretation is correct (which it may not be, given that the SYSTEM_DEFAULT is handled later in this function), then it seems illogical that a default deadline setting would be treated differently here. Doesn't it make far more sense to simply say the default deadline is ∞ (however represented)? That way, there always is a deadline.

But the "system default" could be different from infinite, e.g. if you specify in a qos profile file a default profile with a different deadline value (which would be weird, but possible).
I would only use this function with "actual" qos profiles , and not with one where an item can be "SYSTEM_DEFAULT", so we can also consider failing if an item is "SYSTEM_DEFAULT".

[ivanpauno]

Actually, this new function is failing when a policy has value "UNKNOWN", which is used when an entity has a qos value valid for the underlying middleware that hasn't an equivalent in ROS 2.

I'm not sure why this function should return a compatibility warning when "SYSTEM_DEFAULT" and an error when "UNKNOWN".
I actually think that the opposite makes more sense, or either returning a compatibility warning or error in both.

@jacobperron what do you think?

[jacobperron]

But the "system default" could be different from infinite, e.g. if you specify in a qos profile file a default profile with a different deadline value (which would be weird, but possible).

I wasn't sure if RMW_QOS_DEADLINE_DEFAULT is equivalent to "system default". If it is, then I agree we should treat it the same as other system default values (e.g. warn).

I would only use this function with "actual" qos profiles , and not with one where an item can be "SYSTEM_DEFAULT", so we can also consider failing if an item is "SYSTEM_DEFAULT".

Unfortunately, all of the widely used profiles defined in rmw use "SYSTEM_DEFAULT" values. I think it would be unfortunate if this function failed doing something like this:

rmw_qos_profile_check_compatible( rmw_qos_profile_sensor_data, rmw_qos_profile_sensor_data, ...);

Although, producing a warning isn't much better...

Maybe we should change the default values for those profiles 😅

Actually, this new function is failing when a policy has value "UNKNOWN", which is used when an entity has a qos value valid for the underlying middleware that hasn't an equivalent in ROS 2.

I don't know if this applies here. This is a common implementation for DDS that presumes the RMW supports all the ROS 2 QoS settings (we could document it here to be explicit). If an RMW doesn't support all of the QoS policies, then they should probably provide their own implementation of this function, especially if the RMW is not a DDS implementation.

[ivanpauno]

What is this function for?

I imagine it being used in two ways:

• Check if the qos profile of two existing endpoints is compatible (for example, using the qos profiles got from the rmw_get_publishers_info_by_topic/rmw_get_subscription_info_by_topic).
  This use case doesn't seem super useful though, better to use the "Qos incompatible event".
• Check if an user provided profile is compatible with the profile of an existing(s) endpoint (one profile got from the functions above, the other is user provided).
  Useful if you want to "make up" a profile compatible with existing entities.

In case 1, "SYSTEM_DEFAULT" is impossible, and "UNKNOWN" is a possible value.
In case 2, the user provided profile can have a "SYSTEM_DEFAULT" value, but I'm not sure why someone would pass that.

I don't know if this applies here. This is a common implementation for DDS that presumes the RMW supports all the ROS 2 QoS settings (we could document it here to be explicit). If an RMW doesn't support all of the QoS policies

Yes, and in that case you can still get "UNKNOWN".
e.g.:

• create a publisher with durability=SYSTEM_DEFAULT
• pass a qos profile file that specifies "DDS_TRANSIENT_DURABILITY_QOS" (using topic filters if desired) (transient is not the same as transient local)
• call rmw_get_publishers_info_by_topic(), the qos profile returned will have durability=UNKNOWN.

Same applies to any application mixing native DDS with ROS 2.

[jacobperron]

@ivanpauno Yeah, those are the two uses of this function that I had in mind. And the first one is the primary motivation for this API. AFAIK, tools that want to introspect the ROS graph (e.g. ros2doctor and rqt_graph) cannot get "QoS incompatible" events for all nodes in the graph. This API let's them warn about (possible) compatibility issues.

I guess it is technically possible to get a "SYSTEM_DEFAULT" back from rmw_get_publishers_info_by_topic. Though it's not very useful, there's nothing that says it can't return such a value. My thought is that this function will report possible compatibility issues to the best of it's ability. If it gets a policy value of "SYSTEM_DEFAULT" or "UNKNOWN" then the best we can do is warn about possible incompatibilities. Originally, I didn't think that it was possible to get "UNKNOWN" values from rmw_get_publishers_info_by_topic / rmw_get_subscription_info_by_topic, but if that is the case then I agree we should change the behavior of this function to produce a warning instead.

[ivanpauno]

AFAIK, tools that want to introspect the ROS graph (e.g. ros2doctor and rqt_graph) cannot get "QoS incompatible" events for all nodes in the graph

Ah yes, that's true.

I guess it is technically possible to get a "SYSTEM_DEFAULT" back from rmw_get_publishers_info_by_topic

rmw_get_publishers_info_by_topic should not return "SYSTEM_DEFAULT", but a more specific value.

but if that is the case then I agree we should change the behavior of this function to produce a warning instead.

Using a warning for both system default and unknown sounds good.

[jacobperron]

Warning on "unknown" values: 6c789b4

[wjwwood]

I guess it is technically possible to get a "SYSTEM_DEFAULT" back from rmw_get_publishers_info_by_topic

rmw_get_publishers_info_by_topic should not return "SYSTEM_DEFAULT", but a more specific value.

I think that rmw_get_publishers_info_by_topic should try and avoid returning system default, or shouldn't ever. The idea is that system default is a stand-in for "some externally specified setting", so when querying it for a real entity, it should just return the ROS equivalent or unknown if there isn't an equivalent.

[jacobperron]

To circle back to the original comment, I think I'll leave the current implementation as-is since it seems to agree with the RMW API docs and https://index.ros.org/doc/ros2/Concepts/About-Quality-of-Service-Settings/#qos-compatibilities.
However, I do think that replacing the default duration value with an explicit infinity value would be better (related to ros2/rmw#301). Pending further changes to the RMW API, I'm happy to come back and simplify the logic in this implementation.

While endpoints may not return "SYSTEM_DEFAULT" values, it's possible that other use-cases not involving endpoint queries will call this function. So I think it's nice to warn about "SYSTEM_DEFAULT" values, instead of returning an error with no info about other possible incompatibilities.

[jacobperron]

CI: ros2/rmw#299 (comment)

[jacobperron]

@ros-pull-request-builder retest this please

[jacobperron]

@ros-pull-request-builder retest this please