-
Notifications
You must be signed in to change notification settings - Fork 47
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
Enable topic "ros_discovery_info" for rmw_connextdds #253
Conversation
* Enable ros_discovery_info for rmw_connextdds Signed-off-by: Andrea Sorbini <asorbini@rti.com>
Does it make sense to actually remove support for |
I don't think the PR really precludes the use of The code change will be needed to have tests pass once we start testing on ci.ros2.org (so far I've been testing with a modified ros2.repos). I'm trying to coordinate all changes linked in #9 with the help of @ivanpauno and @clalancette, and I appreciate any input in making this transition as smooth as possible for everyone. In general, I think this PR might be one of the lower priority ones from the list, so we can delay merging it until other changes have been put into place. |
I agree with that, but this PR doesn't seem to be removing support of We could also keep a reference to |
If the change to the test is necessary for the switch to rmw_connextdds to move forward I'm 👍 However we should not break the tutorials by referencing not yet existing rmw implementations in the nightlies. The tutorials changes can be opened as a follow-up once rmw_connextdds is integrated and part of the artifacts produced by the buildfarm.
|
…w_connextdds. Signed-off-by: Andrea Sorbini <asorbini@rti.com>
I restored |
Signed-off-by: Andrea Sorbini <asorbini@rti.com>
Signed-off-by: Andrea Sorbini <asorbini@rti.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me. @mikaelarguedas @SidFaber any questions/comments/concerns about this?
No concerns here @clalancette, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for iterating
This PR replaces all references to
rmw_connext_cpp
withrmw_connextdds
.It also adds
rmw_connextdds
to the list of RMW implementations which support "built-in" topicros_discovery_info
.See rticommunity/rmw_connextdds #9 for a list of related PRs, and an overview of all the changes required to replace ros2/rmw_connext (
rmw_connext_cpp
) with rticommunity/rmw_connextdds in the ROS2 source tree.