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 functions to get a compatible QoS profiles #319

Closed
wants to merge 4 commits into from

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Apr 8, 2022

Connects to #304

Given one or more publisher (subscription) QoS profiles, modify the given subscription (publisher) QoS profile such that it is compatible with most of the publisher (subscription) profiles.
Do this while not sacrificing the level of service when possible.

This API is ultimately intended to be leveraged by ROS tools that typically want to be flexible with respect to the QoS when subscribing or publishing to a topic.


Opening for early feedback about the API. There is a reference implementation for one of the functions in ros2/rmw_dds_common#59

My next step is to add rcl functions that query the ROS graph to collect QoS info and then use this API to return the most compatible QoS profile, e.g.

rcl_ret_t rcl_get_compatible_subscription_qos_from_topic(
  const rcl_node_t * node,
  const char * topic_name,
  rmw_qos_profile_t * subscription_qos_profile
)

I'm leaning towards putting the endpoint query logic in rcl, since it can be implemented in an RMW-agnostic way (using the API proposed in this PR). However, we could also consider doing all of that logic in RMW in case the RMW implementation can do something more efficient 🤷

Given one or more publisher (subscription) QoS profiles, modify the given
subscription (publisher) QoS profile such that it is compatible with most
of the publisher (subscription) profiles.
Do this while not sacrificing the level of service when possible.

This API is ultimately intended to be leveraged by ROS tools that
typically wants to be flexible with respect to the QoS when subscribing or
publishing to a topic.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Here's a WIP PR for rcl: ros2/rcl#976

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 265 to 268
* \return `RMW_RET_OK` if the operation was successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profiles` is `nullptr`, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profiles_length` is 0, or
* \return `RMW_RET_INVALID_ARGUMENT` if `subscription_profile` is `nullptr`, or
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* \return `RMW_RET_OK` if the operation was successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profiles` is `nullptr`, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profiles_length` is 0, or
* \return `RMW_RET_INVALID_ARGUMENT` if `subscription_profile` is `nullptr`, or
* \return `RMW_RET_OK` if the operation was successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `subscription_profiles` is `nullptr`, or
* \return `RMW_RET_INVALID_ARGUMENT` if `subscription_profiles_length` is 0, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profile` is `nullptr`, or

*
* \param[in] publisher_profiles: An array of QoS profiles for publishers.
* \param[in] publisher_profiles_length: The number of elements in `publisher_profiles`.
* \param[out] subscription_profile: QoS policies that are compatible with the majorty of

Choose a reason for hiding this comment

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

Suggested change
* \param[out] subscription_profile: QoS policies that are compatible with the majorty of
* \param[out] subscription_profile: QoS policies that are compatible with the majority of

A side from the typo, could it be that "majority" is somehow ambiguous here? I mean, we probably want to know if the policy is "compatible enough" to be used and that implies that is compatible with all input publisher profiles. Is my understanding correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

For DDS implementations, it will be compatible with all input publisher profiles, but I'm purposefully using the word "majority" here to leave room for non-DDS implementations that may not be able to match all input publishers (their QoS compatibility rules may be different).

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I have one suggestion.
I'm not sure if compatible_subscription_profiles/compatible_publisher_profiles will be useful in practice, but having them is okay.

* This parameter is optional and may be `nullptr`.
* If provided, it must be the same length as the publisher profiles array.
* \return `RMW_RET_OK` if the operation was successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if `publisher_profiles` is `nullptr`, or
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a RMW_RET_MOST_COMAPATIBLE_NOT_ALL?
Or sth like that, to indicate that the returned profile is not compatible with all the provided profiles, without the need of providing a compatible_publisher_profiles array.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to that idea. My original thinking was that if there are one or more incompatible profiles, then the user could invoke this function again with just those profiles in an effort to match them. But, as you point out, I'm not sure if this would be used in practice 🤷

Copy link
Member

Choose a reason for hiding this comment

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

We could do that, but is that better than the array? It seems like the array just provides strictly more information. If the function tells me it matches some but not all, my immediate next question is "which ones"? At least that's my thinking.

Copy link
Member

Choose a reason for hiding this comment

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

I think RMW_RET_MOST_COMAPATIBLE_NOT_ALL and the array could be useful (the array is optional btw), just because it is easier for the caller to check, rather than always requesting the array and then iterating over it to know if it's a 100% fit or only a partial fit.

Copy link
Member Author

@jacobperron jacobperron Apr 13, 2022

Choose a reason for hiding this comment

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

I like the new return code in addition to the output array of booleans; I can add it.

I considered the array of bools a compromise; because we could alternatively return an array of structs describing precisely what QoS policies are compatible, incompatible, or unknown for each input QoS profile. This seems like it would add a lot of detail and implementation overhead for something I'm not sure anyone would use.

Copy link
Member

Choose a reason for hiding this comment

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

I considered the array of bools a compromise; because we could alternatively return an array of structs describing precisely what QoS policies are compatible, incompatible, or unknown for each input QoS profile. This seems like it would add a lot of detail and implementation overhead for something I'm not sure anyone would use.

That's fair, I guess knowing which it is not compatible with doesn't tell you much more, but I guess then there are direct comparison functions, where you could compare the one it didn't match with the one it proposed and get more details on why they don't match that way.

Pass in endpoint info array's directly to the function. This is more ergonomic for the intended use-case.
Also change output variable to be an endpoint info array instead of a bool array.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
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.

looks good to me.

@jacobperron
Copy link
Member Author

See #320 (and connected PRs) for an alternative to this PR. IMO, it makes the interface a lot nicer for the user for a couple reasons:

  • They only have to change the QoS policy value in order to take advantage of the feature; instead of calling extra functions and doing error handling.
  • They can choose which policies they was to be adaptive; it's not all-or-nothing like this PR.

@jacobperron
Copy link
Member Author

Closing in favor of #320

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.

None yet

5 participants