Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Don't use RTPS Participant name as node name #393

Merged

Conversation

ivanpauno
Copy link
Member

This is what I mentioned here: ros2/ros2cli#464 (comment).

Currently, the node name and namespace are communicated making use of userData qos.
In case that that qos setting can be parsed in the way ros2 is using it, we're using the RTPS Participant name as a node name, and listing it.

The question is: is that exactly what we want?
I see some value of showing the user other Participants that weren't craeted with ROS, but probably, their name should be showed with a different prefix. An opt-in/opt-out flag to show them or not would also be nice.

In ros2/rmw_fastrtps#312, userData is now being use to communicate the Context name. Connext was failing to parse it, and then showing the RTPS Participant name.

We can take the following paths:

  • Just show Participants that their userData can be parsed correctly (proposed in this PR).
  • Don't change anything here, and accept the participant name to be showed as a node name. Make sure that the tests don't run nodes from a different implementation when they shouldn't (see Avoid using ros_testing, and use launch_testing instead ros2cli#464).

I don't know what's the best option, @hidmic @wjwwood opinions?

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Mar 5, 2020
@ivanpauno ivanpauno requested review from wjwwood and hidmic March 5, 2020 17:25
@ivanpauno ivanpauno self-assigned this Mar 5, 2020
Copy link
Contributor

@hidmic hidmic 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 makes sense. If participants aren't nodes, what are they in this context? There's no concept of a participant in ROS, so nothing should be shown. You can always use DDS tools to introspect your system if need be.

@wjwwood
Copy link
Member

wjwwood commented Mar 6, 2020

I guess it makes sense too, based on @hidmic's reasoning.

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

Failures are unrelated, merging!

@ivanpauno ivanpauno merged commit 86a4bb8 into master Mar 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/dont-use-rtps-participant-name-as-node-name branch March 9, 2020 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants