Skip to content

Handle 'best_available' QoS policies#84

Closed
j-rivero wants to merge 2 commits intomasterfrom
jrivero/qos_best_available
Closed

Handle 'best_available' QoS policies#84
j-rivero wants to merge 2 commits intomasterfrom
jrivero/qos_best_available

Conversation

@j-rivero
Copy link
Copy Markdown

Connects to: ros2/rmw#320
Twin of: ros2/rmw_fastrtps#598

If users set certain QoS policies to BEST_AVAILABLE, then we query the graph for endpoint info. The policy is updated such that it is compatible with all endpoints while maintaining the highest possible level of service.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
return nullptr;
}

return rmw_api_connextdds_create_subscription(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we can move the new logic into rmw_api_connextdds_create_subscription? Then we don't have to repeat ourselves both here and in rmw_connextddsmicro. Same idea for the create publisher logic.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, maybe we can't make it work because of a linking issue. The function pointer passed to the rmw_dds_common function is technically implemented in this package, not the common package. I had the same issue in the connected rmw_fastrtps PR.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the possible problem, but I agree with @jacobperron that this logic should definitely be moved to rmw_api_connextdds_create_subscription() (and similarly for publishers, clients, and services), so that it may be shared between both RMWs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There might not be a problem moving the implementation if there is a common implementation for rmw_get_subscriptions_info_by_topic that shares the same signature.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I coded the approach to give a try on building and testing. Worked fine in my local focal system until the point of compiling the system_tests. See #85

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see now, thank you for clarifying @jacobperron. There are rmw_api_connextdds_get_publishers_info_by_topic and rmw_api_connextdds_get_subscriptions_info_by_topic which provide common implementations in rmw_connextdds_common.

@j-rivero #85 looks like the right approach, but it's missing the publisher part (which should go within rmw_api_connextdds_create_publisher in rmw_publication.cpp)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks Andrea, I've implemented your recommendation but test are still failing. Let's continue there #85 (comment)

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
return nullptr;
}

return rmw_api_connextdds_create_subscription(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the possible problem, but I agree with @jacobperron that this logic should definitely be moved to rmw_api_connextdds_create_subscription() (and similarly for publishers, clients, and services), so that it may be shared between both RMWs.

@j-rivero
Copy link
Copy Markdown
Author

j-rivero commented May 6, 2022

Closing, deprecated by #85

@j-rivero j-rivero closed this May 6, 2022
@jacobperron jacobperron deleted the jrivero/qos_best_available branch May 6, 2022 22:08
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.

3 participants