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

Enable topic "ros_discovery_info" for rmw_connextdds #253

Merged
merged 4 commits into from
Mar 11, 2021

Conversation

asorbini
Copy link
Contributor

@asorbini asorbini commented Mar 5, 2021

This PR replaces all references to rmw_connext_cpp with rmw_connextdds.

It also adds rmw_connextdds to the list of RMW implementations which support "built-in" topic ros_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.

* Enable ros_discovery_info for rmw_connextdds

Signed-off-by: Andrea Sorbini <asorbini@rti.com>
@kyrofa
Copy link
Member

kyrofa commented Mar 5, 2021

Does it make sense to actually remove support for rmw_connext_cpp before it's also removed from the master repos? Maybe it makes more sense to add support for the new one now, and remove support for the old one when it's actually gone.

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #253 (87dd954) into master (2b520b9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #253   +/-   ##
=======================================
  Coverage   77.25%   77.25%           
=======================================
  Files          25       25           
  Lines         664      664           
  Branches       55       55           
=======================================
  Hits          513      513           
  Misses        131      131           
  Partials       20       20           
Flag Coverage Δ
unittests 77.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s_ws/src/j68c7tcb0ir/sros2/sros2/sros2/__init__.py
.../j68c7tcb0ir/sros2/sros2/sros2/command/security.py
.../sros2/sros2/sros2/policy/defaults/dds/__init__.py
...0ir/sros2/sros2/sros2/policy/templates/__init__.py
...cb0ir/sros2/sros2/sros2/verb/generate_artifacts.py
...j68c7tcb0ir/sros2/sros2/sros2/keystore/__init__.py
...cb0ir/sros2/sros2/sros2/policy/schemas/__init__.py
...c/j68c7tcb0ir/sros2/sros2/sros2/policy/__init__.py
...c7tcb0ir/sros2/sros2/sros2/verb/create_keystore.py
...68c7tcb0ir/sros2/sros2/sros2/keystore/_keystore.py
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b520b9...87dd954. Read the comment docs.

@asorbini
Copy link
Contributor Author

asorbini commented Mar 5, 2021

I don't think the PR really precludes the use of rmw_connext_cpp, since all those changes are in documentation.

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.

@ivanpauno
Copy link
Member

Does it make sense to actually remove support for rmw_connext_cpp before it's also removed from the master repos? Maybe it makes more sense to add support for the new one now, and remove support for the old one when it's actually gone.

I agree with that, but this PR doesn't seem to be removing support of rmw_connext_cpp.
It's just replacing in the tutorials rmw_connext_cpp with rmw_connextdds, and adding rmw_connextdds to the _RMW_WITH_ROS_GRAPH_INFO_TOPIC list.

We could also keep a reference to rmw_connext_cpp in the tutorials, but I don't think it's worth it.

@mikaelarguedas
Copy link
Member

We could also keep a reference to rmw_connext_cpp in the tutorials, but I don't think it's worth it.

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.

[ERROR] [1615413395.850699851] [rcl]: Error getting RMW implementation identifier / RMW implementation not installed (expected identifier of 'rmw_connextdds'), with error message 'failed to load shared library 'librmw_connextdds.so' due to dlopen error: librmw_connextdds.so: cannot open shared object file: No such file or directory, at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rcutils/src/shared_library.c:99, at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:74', exiting with 1., at /home/jenkins-agent/workspace/packaging_linux/ws/src/ros2/rcl/rcl/src/rcl/rmw_implementation_identifier_check.c:139

…w_connextdds.

Signed-off-by: Andrea Sorbini <asorbini@rti.com>
@asorbini
Copy link
Contributor Author

I restored rmw_connext_cpp in the tutorials. I will open a new PR to update them again once rmw_connextdds makes it into the nightly builds.

@asorbini asorbini changed the title Replace rmw_connext_cpp with rmw_connextdds Enable topic "ros_discovery_info" for rmw_connextdds Mar 10, 2021
Signed-off-by: Andrea Sorbini <asorbini@rti.com>
Signed-off-by: Andrea Sorbini <asorbini@rti.com>
@ivanpauno ivanpauno requested a review from clalancette March 11, 2021 16:44
Copy link
Contributor

@clalancette clalancette left a 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?

@SidFaber
Copy link

No concerns here @clalancette, thanks

Copy link
Member

@mikaelarguedas mikaelarguedas left a 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

@mikaelarguedas mikaelarguedas merged commit a02492f into ros2:master Mar 11, 2021
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.

6 participants