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

Add declaration for function to check QoS profile compatibility #299

Merged
merged 10 commits into from
Feb 25, 2021

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Feb 11, 2021

Currently, users who are creating a publisher or subscription can receive 'QoS incompatibility'
events from the RMW if an incompatible endpoint is discovered. While this is useful, we
currently don't have a nice way for application to generally check if two QoS profiles are
compatible. For example, it would be nice if tooling could query the communication graph and
report any detected QoS incompatibilities.

In order to reduce code duplication, I think an API for checking QoS compatibilty should live
in a common place. I've opted for rmw (over a place like rcl) since it's possible QoS
compatiblity rules may vary per RMW vendor. Since rules for all DDS implementations should be
the same, we could put that common logic in rmw_dds_common.


The motivation for this API is to add features to tooling like rqt_graph and ros2doctor to help diagnose QoS compatibility issues. E.g. it would be useful to change how incompatible links in the ROS graph are rendered in rqt_graph.


Connected PRs:

CI for connected PRs listed above:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Currently, users who are creating a publisher or subscription can receive 'QoS incompatibility'
events from the RMW if an incompatible endpoint is discovered. While this is useful, we
currently don't have a nice way for application to generally check if two QoS profiles are
compatible. For example, it would be nice if tooling could query the communication graph and
report any detected QoS incompatibilities.

In order to reduce code duplication, I think an API for checking QoS compatibilty should live
in a common place. I've opted for `rmw` (over a place like `rcl`) since it's possible QoS
compatiblity rules may vary per RMW vendor. Since rules for all DDS implementations should be
the same, we could put that common logic in `rmw_dds_common`.

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit to ros2/rmw_implementation that referenced this pull request Feb 17, 2021
Use enum for output; if one or more policies is set to 'system default', then it's better to warn the caller that we aren't sure if QoS profiles are compatible.

Add optional 'reason', and 'reason_size', parameters for outputing a description of what is (or might be) the incompatiblity.

Update API docs.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

I've refactored the proposed API. Two major changes:

  1. Now instead of a bool, it outputs the compatibility result as an enum type parameter. The return code is reserved for other errors not related to compatibility.
    The compatibility can have one of three states:

    • "OK": the profiles are compatible
    • "WARNING": because one or more policies was set to "system default", we don't actually know if the profiles are compatible.
    • "ERROR": the profiles are incompatible.
  2. Added a string buffer (and size) parameter for setting the reason for an incompatibility or warning.
    The user should provide a pre-allocated array so that memory management is completely in their hands.

@mjeronimo @gbiggs PTAL

@jacobperron
Copy link
Member Author

Oh, and I also renamed the function on a whim.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@mjeronimo
Copy link

I agree with the name change given the change in the return type.

Looks good to me, although I would've preferred a definitive yes/no answer, which would, of course, require a way to query the underlying implementation for its QoS system defaults.

Copy link
Member

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

Some documentation improvements are needed.

@wjwwood
Copy link
Member

wjwwood commented Feb 17, 2021

Added a string buffer (and size) parameter for setting the reason for an incompatibility or warning.
The user should provide a pre-allocated array so that memory management is completely in their hands.

👍 like this even more than what we talked about.

jacobperron added a commit to ros2/rmw_dds_common that referenced this pull request Feb 18, 2021
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

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

I've opened a PR with the implementation for DDS RMWs here: ros2/rmw_dds_common#45

Signed-off-by: Jacob Perron <[email protected]>
This behaves more like other RMW functions.

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit to ros2/rmw_fastrtps that referenced this pull request Feb 18, 2021
jacobperron added a commit to ros2/rmw_fastrtps that referenced this pull request Feb 18, 2021
jacobperron added a commit to ros2/rmw_cyclonedds that referenced this pull request Feb 18, 2021
@jacobperron jacobperron marked this pull request as ready for review February 18, 2021 23:06
jacobperron added a commit to ros2/rmw_implementation that referenced this pull request Feb 18, 2021
@jacobperron
Copy link
Member Author

Thanks for the feedback! I've updated this PR and opened a bunch of connected PRs that implement this for our supported RMWs (see the updated description for a list). The common implementation lives in rmw_dds_common, so this one could use the most scrutiny: ros2/rmw_dds_common#45

jacobperron added a commit to ros2/rclcpp that referenced this pull request Feb 19, 2021
jacobperron added a commit to ros2/rclcpp that referenced this pull request Feb 19, 2021
Signed-off-by: Jacob Perron <[email protected]>
* Must be pre-allocated by the caller. This parameter is optional and may be set to `NULL`.
* \param[out] reason: A detailed reason for a QoS incompatibility or potential incompatibility.
* Must be pre-allocated by the caller.
* This parameter is optional and may be set to `NULL` if the reason information is not

Choose a reason for hiding this comment

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

Use 'nullptr's here instead of NULLs?

Copy link
Member

Choose a reason for hiding this comment

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

nullptr is cpp only AFAIK

Comment on lines 134 to 141
* If any of the profile policies has the value "system default", then it may not be
* possible to determine the compatibilty.
* In this case, the output parameter `compatibility` is set to `RMW_QOS_COMPATIBILITY_WARNING`
* and `reason` is populated.
*
* Profile policies must not have the value "unknown".
* An "unknown" value is considered an error and `RMW_RET_INVALID_ARGUMENT` is returned.
* `compatibility` and `reason` will not be set.
Copy link
Member

Choose a reason for hiding this comment

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

Pending discussion in another PR ros2/rmw_dds_common#45 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 93e9aa2

Unknown values now result in a compatibility warning instead of an error.

Since it's possible for rmw's to return an unknown value if the policy is set to an unsupported value by ROS 2.

Signed-off-by: Jacob Perron <[email protected]>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@jacobperron
Copy link
Member Author

Thanks for all the feedback so far!

I'm just waiting for ci.ros2.org to come back online and then I'll trigger CI and get this merged/released.

@jacobperron jacobperron merged commit c5c05f1 into master Feb 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the jacob/compatible_qos_api branch February 25, 2021 19:15
jacobperron added a commit to ros2/rmw_dds_common that referenced this pull request Feb 28, 2021
* Add function for checking QoS profile compatibility

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

Signed-off-by: Jacob Perron <[email protected]>

* Refactor signature of _write_to_buffer

Signed-off-by: Jacob Perron <[email protected]>

* Set error message instead of setting reason

Signed-off-by: Jacob Perron <[email protected]>

* Error is null reason and non-zero reason_size

Signed-off-by: Jacob Perron <[email protected]>

* Do not set output parameter if an error occurs

Signed-off-by: Jacob Perron <[email protected]>

* Add test for reason buffer too small

Signed-off-by: Jacob Perron <[email protected]>

* Warn on unknown values instead of error

Signed-off-by: Jacob Perron <[email protected]>

* Fixes

* Use vsnprintf over snprintf (or undefined things can happen)
* Handle NULL return value from `*to_str()` functions
* Add more tests

Signed-off-by: Jacob Perron <[email protected]>

* Guard against zero reason_size

Add test case.

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit to ros2/rmw_connext that referenced this pull request Mar 1, 2021
jacobperron added a commit to ros2/rmw_fastrtps that referenced this pull request Mar 1, 2021
* Add RMW function to check QoS compatibility

Implements ros2/rmw#299

Depends on ros2/rmw_dds_common#45

Signed-off-by: Jacob Perron <[email protected]>

* Add tests to exercise new API

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit to ros2/rmw_cyclonedds that referenced this pull request Mar 1, 2021
jacobperron added a commit to ros2/rmw_implementation that referenced this pull request Mar 2, 2021
* Add function for checking QoS profile compatibility

Connected to ros2/rmw#299

Signed-off-by: Jacob Perron <[email protected]>

* Update tests

Unknown values no longer cause errors, but possibly warnings instead.
We can't test this since it is dependent on the RMW.

We can test for compatibility with no default or unknown values as well as invalid input.

Signed-off-by: Jacob Perron <[email protected]>
@asorbini
Copy link

asorbini commented Mar 5, 2021

I noticed this new API from a new test failure in test_rmw_implementation, and added support for it to rmw_connextdds in 91753dc.

I also noticed that these changes have not yet made it to the Rolling Debian binaries, and I had to build the tree from source in order to validate them. What is the period with which those packages are usually regenerated?

@ivanpauno
Copy link
Member

I think that releasing rmw_implementation is pending, but IIUC @jacobperron already released up to rmw.

I also noticed that these changes have not yet made it to the Rolling Debian binaries, and I had to build the tree from source in order to validate them. What is the period with which those packages are usually regenerated?

IIUC the packages used by build.ros2.org for testing will be updated almost right away (delay of some hours), but the public ones (the ones you download when apt install ...) will only get updated after a sync.
I guess there will be a sync in about May as ROS 2 Galactic will be released for that date, though IDK.

@jacobperron
Copy link
Member Author

@ivanpauno is correct. If you switch to the testing repo (see instructions), then you should see these changes already released there. Syncs to main happen periodically (roughly every three weeks), though I don't think there has been one for a couple months now.

@nuclearsandwich what do you think about doing a Rolling sync in the near future?

jacobperron added a commit to ros2/rclcpp that referenced this pull request Mar 25, 2021
* Add API for checking QoS profile compatibility

Depends on ros2/rmw#299

Signed-off-by: Jacob Perron <[email protected]>

* Refactor as free function

Returns a struct containing the compatibility enum value and string for the reason.

Updated tests to reflect behavior changes upstream.

Signed-off-by: Jacob Perron <[email protected]>
clalancette pushed a commit to eboasson/rmw_cyclonedds that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants