Skip to content

Add functions to get a compatible QoS profiles #319

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Add functions to get a compatible QoS profiles #319

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3328ef
Add functions to get compatible QoS profiles
jacobperron Apr 8, 2022
46e47cf
Fix documentation typo
jacobperron Apr 8, 2022
cd971e1
Fix visibility macros
jacobperron Apr 9, 2022
8befa60
Update function signatures
jacobperron Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions rmw/include/rmw/qos_profiles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,103 @@ rmw_qos_profile_check_compatible(
char * reason,
size_t reason_size);

/// Get compatible QoS policies for a subscription.
/**
* Given one or more publisher QoS profiles, return a QoS profile for a subscirption
* that is compatible with the majority of the publisher profiles while maintaining the highest
* level of service possible.
*
* This function modifies the provided subscription QoS profile in order to make it compatible
* with the publisher profiles.
* It should only modify those members of the QoS profile that it needs to improve compatibility.
* For example, for DDS implementations this function should not modify history depth, which
* has no role in determining compatibility.
*
* It's possible not all input QoS profiles are compatible with the output subscription QoS
* profile.
* You can optionally provide a boolean array that will by set by this function to indicate
* which publisher QoS profiles are compatible with the result.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | Yes
* Uses Atomics | No
* Lock-Free | Yes
*
* \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
Copy link

@j-rivero j-rivero Apr 13, 2022

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

@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.

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).

* the input publisher profiles.
* \param[out] compatible_publisher_profiles: An array of boolean values indicating if the
* corresponding QoS profile at the same index in the publisher profiles array is compatible
* with the resultant subscription QoS profile or not.
* 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

@ivanpauno ivanpauno Apr 13, 2022

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

@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'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

@wjwwood wjwwood Apr 13, 2022

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

@wjwwood wjwwood Apr 13, 2022

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 •
edited
Loading

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

@wjwwood wjwwood Apr 13, 2022

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.

* \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_ERROR` if there is an unexpected error.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_ret_t
rmw_qos_profile_get_most_compatible_for_subscription(
const rmw_qos_profile_t * publisher_profiles,
size_t publisher_profiles_length,
rmw_qos_profile_t * subscription_profile,
bool * compatible_publisher_profiles);

/// Get compatible QoS policies for a publisher.
/**
* Given one or more subscription QoS profiles, return a QoS profile for a publisher
* that is compatible with the majority of the subscription profiles while maintaining the
* highest level of service possible.
*
* This function modifies the provided publisher QoS profile in order to make it compatible
* with the subscription profiles.
* It should only modify those members of the QoS profile that it needs to improve compatibility.
* For example, for DDS implementations this function should not modify history depth, which
* has no role in determining compatibility.
*
* It's possible not all input QoS profiles are compatible with the output publisher QoS profile.
* You can optionally provide a boolean array that will by set by this function to indicate
* which subscription QoS profiles are compatible with the result.
*
* <hr>
* Attribute | Adherence
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | Yes
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] subscription_profiles: An array of QoS profiles for subscriptions.
* \param[in] subscription_profiles_length: The number of elements in `subscription_profiles`.
* \param[out] publisher_profile: QoS policies that are compatible with the majorty of
* the input subscription profiles.
* \param[out] compatible_subscription_profiles: An array of boolean values indicating if the
* corresponding QoS profile at the same index in the subscription profiles array is compatible
* with the resultant publisher QoS profile or not.
* This parameter is optional and may be `nullptr`.
* If provided, it must be the same length as the subscription profiles array.
* \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

@wjwwood wjwwood Apr 13, 2022

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

* \return `RMW_RET_ERROR` if there is an unexpected error.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_ret_t
rmw_qos_profile_get_most_compatible_for_publisher(
const rmw_qos_profile_t * subscription_profiles,
size_t subscription_profiles_length,
rmw_qos_profile_t * publisher_profile,
bool * compatible_subscription_profiles);

#ifdef __cplusplus
}
#endif
Expand Down