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 compatible QoS profiles #59

Closed
wants to merge 7 commits into from

Conversation

jacobperron
Copy link
Member

Connects to ros2/rmw#304

Currently, just contains an implementation for the subscription. I'll wait for feedback before adding the equivalent function for publishers.

Given one or more publisher QoS profiles, modify the given subscription QoS profile to be compatible with most of them.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
// QoS profile is compatible with all publishers QoS profiles in the input
if (compatible_publisher_profiles) {
for (size_t i = 0u; i < publisher_profiles_length; ++i) {
compatible_publisher_profiles[i] = true;
Copy link
Member

Choose a reason for hiding this comment

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

While this seems silly for DDS rmw implementations, I think it's a potentially useful feature for future non-DDS implementations, and keeps users of the interface aware of the possibility that the QoS matching rules are not fixed at this time.

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.

I think this is reasonable.

@@ -374,4 +374,62 @@ qos_profile_check_compatible(
return RMW_RET_OK;
}

rmw_ret_t
qos_profile_get_most_compatible_for_subscription(
Copy link
Member

Choose a reason for hiding this comment

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

should this function also take in accound deadline?

You need deadline_publisher < deadline_subscription for them to match

Pass in endpoint info array directly and output an endpoint info array for reporting incompatibilities.
Update implementation and tests accordingly.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Given one or more publisher endpoints, modify the given subscription QoS profile to be compatible with most of them.

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

a few minor nitpicks, it looks good to me.

Comment on lines +103 to +104
* This implements the rmw API \ref rmw_qos_profile_get_most_compatible_for_subscription().
* See \ref rmw_qos_profile_get_most_compatible_for_subscription() for more information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* This implements the rmw API \ref rmw_qos_profile_get_most_compatible_for_subscription().
* See \ref rmw_qos_profile_get_most_compatible_for_subscription() for more information.
* This implements the rmw API \ref rmw_qos_profile_get_most_compatible_for_publisher().
* See \ref rmw_qos_profile_get_most_compatible_for_publisher() for more information.

publisher_profile->reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE;
publisher_profile->durability = RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL;

// Only use "manual by topic" liveliness if at least one subscription is using manual by topic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Only use "manual by topic" liveliness if at least one subscription is using manual by topic
// Only use "manual by topic" liveliness if at least one subscription is using manual by topic

@jacobperron
Copy link
Member Author

Closing in favor of ros2/rmw#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.

4 participants