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 pid to launch_ros node name as suffix #98

Merged
merged 1 commit into from Nov 28, 2019

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Nov 25, 2019

Fixes #2 . As the title says, it adds the pid to the node's name as a suffix. Is there any need to create a test for this?

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM!

A test would be nice, but I don't think it's necessary for this small change given the overhead involved.

@BMarchi BMarchi requested a review from hidmic November 26, 2019 15:17
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 26, 2019

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

@@ -61,7 +62,9 @@ def _function(self, context: launch.LaunchContext):
if 'rcl_init called while already initialized' in str(exc):
pass
raise
self.__launch_ros_node = rclpy.create_node('launch_ros', context=self.__rclpy_context)
self.__launch_ros_node = rclpy.create_node(
'launch_ros_{}'.format(os.getpid()),
Copy link
Contributor

Choose a reason for hiding this comment

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

@BMarchi appending launch PID helps disambiguate but doesn't make it a hidden a node -- you need a leading underscore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see the requirement to make the launch_ros node hidden. Are you sure about appending that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I just tossed in a feature request. It's not in the original ticket, you're right. Disregard (I'll propose it afterwards).

@BMarchi BMarchi merged commit 848b201 into master Nov 28, 2019
@BMarchi BMarchi deleted the BMarchi/add_pid_suffix_to_launch_ros branch November 28, 2019 13:48
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.

[launch_ros] launch_ros node should have an anonymous name
3 participants