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

Make rosbag2 nodes locally unique by suffixing with PID #1432

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Jul 25, 2023

Fixes #425

Copies same approach from https://github.com/AAlon/ros2cli/blob/master/ros2cli/ros2cli/node/direct.py#L37

Adds a --node-name option to all verbs that create a ros2 node. Defaults to prefix_PID by using Python os.getpid(). This way all running players/recorders will have locally unique names by default. "Locally" because within the local machine the PID will be unique, but may not be within a multi-machine system. In this case it probably pays to be explicit anyways, and the --node-name option allows for that.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp requested a review from a team as a code owner July 25, 2023 18:11
@emersonknapp emersonknapp requested review from gbiggs, jhdcs and MichaelOrlov and removed request for a team July 25, 2023 18:11
@emersonknapp emersonknapp changed the title Make rosbag2 nodes unique by suffixing with PID like ros2cli does Make rosbag2 nodes locally unique by suffixing with PID Jul 25, 2023
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@emersonknapp In general LGTM.
I am thinking that now the node name became autogenerated by default and it will be difficult to figure out what it is to send service requests from the command line.
It would be very handful to output the final node_name to the LOG_INFO() stream at least.

@@ -148,6 +148,14 @@ def add_standard_reader_args(parser: ArgumentParser) -> None:
'By default attempts to detect automatically - use this argument to override.')


def add_standard_node_args(parser: ArgumentParser, prefix: str) -> None:
"""Add arguments for for verbs that create a Node."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Add arguments for for verbs that create a Node."""
"""Add arguments for verbs that create a Node."""

Nitpick, need to remove extra "for".

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.

Name rosbag2 node uniquely
2 participants