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

Consistent node naming across ros2cli tools #60

Merged
merged 4 commits into from
Feb 12, 2019
Merged

Conversation

AAlon
Copy link
Contributor

@AAlon AAlon commented Nov 16, 2018

connects to ros2/ros2cli#158

Summary

This is part of the improvements suggested here mainly intended for CLI tools' usability with security, and depends on this change to ros2cli. It will make it easy to use ros2bag on security-enabled systems.

  • Launch nodes using the common naming prefix provided by ros2cli.

Related changes

Refer to

https://discourse.ros.org/t/ros2-security-cli-tools/6647/

@ruffsl
Copy link
Member

ruffsl commented Nov 16, 2018

Wouldn't this hardcoding interfere if a user wanted to have rosbag2 to either play or record traffic while advertising itself under a FQN of the user's choosing? Some example use cases:

  • Multiple instances of rosbag2 running simultaneously
    • To avoid node namespace collisions a user would like to designate a unique node name to each
  • Simulating or spoofing publishers or subscribers
    • This might be an anti pattern for against anonymous pubsub, but suppose I want to use rosbag2 playback or record to replicate a target ROS2 network as closely as possible, down to the FQN names of the nodes themselves for simulation or testing purposes.

I think it might be more useful to enable ros2 run like runtime arguments like __node:=<node_name> and __ns:<node_namespace>. This might be more flexible and would give a more consistent user experience with the rest of the ros2 CLI. I think it might be nice if most all ros2 CLI tools accepted ros2 run like overrides to control the underlying nodes they spaw to interface with the ROS2 graph.

@AAlon
Copy link
Contributor Author

AAlon commented Nov 19, 2018

The current rosbag implementation already starts up the node with the hardcoded name "rosbag2", so none of those use cases are currently possible 😢
This PR simply adds a prefix and propagates it from the ros2cli layer; which means that when we'd want to add support for user-specified node/ns names, it will be really simple to set the prefix to be the provided namespace, since it's part of the CLI layer.

@Karsten1987
Copy link
Collaborator

I am not sure about the implications regarding ros2cli, but the hardcoded rosbag2 node name should definitely be changed. I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

@AAlon
Copy link
Contributor Author

AAlon commented Nov 29, 2018

I am not sure about the implications regarding ros2cli, but the hardcoded rosbag2 node name should definitely be changed. I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

Makes sense, but I'll reemphasize the point that this change doesn't contradict any future name changes. We add a prefix so that naming would be consistent across ros2cli nodes by default, making it easier to integrate with security. A future PR could allow users to specify the node name and namespace.

@AAlon
Copy link
Contributor Author

AAlon commented Dec 10, 2018

Any thoughts on this one?

@AAlon
Copy link
Contributor Author

AAlon commented Dec 13, 2018

Rebased properly now.

@ruffsl
Copy link
Member

ruffsl commented Dec 13, 2018

I feel it should adhere with the general way ros2 nodes get their node names such that remapping etc works.

@Karsten1987 how would you suggest this be implemented? Having all unknown args parsed to the ros2bag cli's argparser be passed onto the transport.init(), and consecutively to rclcpp::init; Allowing folks to use the same remapping argument such as __ns:= and __name:=?

rosbag2_transport::Rosbag2Transport transport;
transport.init();
transport.record(storage_options, record_options);
transport.shutdown();

@AAlon
Copy link
Contributor Author

AAlon commented Jan 10, 2019

This needs rebasing, but in any case would be blocked on ros2/ros2cli#158 since it refers to CLI_NODE_NAME_PREFIX from that PR.

@AAlon
Copy link
Contributor Author

AAlon commented Jan 11, 2019

Rebased. Build would fail because it depends on ros2/ros2cli#158 (which depends on ros2/rclpy#259).

@Karsten1987
Copy link
Collaborator

Sorry for being so silent on this, didn't know rosbag was a blocking factor on this.

Allowing folks to use the same remapping argument such as __ns:= and __name:=?

Eventually this rosbag transport interface has to be rewritten a bit more generic to also allow an easy python entry point, but yes. The command line argument should be used to allow remapping et al. That being said, for now I believe it's the correct way to forward all command line arguments to the init function. However, I am unsure if args received in python can just be forwarded to the init function in rclcpp as is.
In any case the implementation here has to be changed:

void Rosbag2Transport::init()
{
rclcpp::init(0, nullptr);
}

And then further down the actual call to setup_node as well:

std::shared_ptr<Rosbag2Node> Rosbag2Transport::setup_node()
{
if (!transport_node_) {
transport_node_ = std::make_shared<Rosbag2Node>("rosbag2");
}
return transport_node_;
}

Looking at the two snippets, I don't think we can easily forward the arguments but rather pre-parse them and pass them to the individual functions.
All that being said, I am happy to adhere with whatever convention is applied in ros2/ros2cli#158

@AAlon
Copy link
Contributor Author

AAlon commented Jan 23, 2019

Sorry for being so silent on this, didn't know rosbag was a blocking factor on this.

Allowing folks to use the same remapping argument such as __ns:= and __name:=?

Eventually this rosbag transport interface has to be rewritten a bit more generic to also allow an easy python entry point, but yes. The command line argument should be used to allow remapping et al. That being said, for now I believe it's the correct way to forward all command line arguments to the init function. However, I am unsure if args received in python can just be forwarded to the init function in rclcpp as is.
In any case the implementation here has to be changed:
rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 53 to 56 in a963c68

void Rosbag2Transport::init()
{
rclcpp::init(0, nullptr);
}
And then further down the actual call to setup_node as well:
rosbag2/rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp

Lines 79 to 85 in a963c68

std::shared_ptr Rosbag2Transport::setup_node()
{
if (!transport_node_) {
transport_node_ = std::make_shared("rosbag2");
}
return transport_node_;
}
Looking at the two snippets, I don't think we can easily forward the arguments but rather pre-parse them and pass them to the individual functions.
All that being said, I am happy to adhere with whatever convention is applied in ros2/ros2cli#158

I'm a bit confused, does that mean you'd be fine with the current PR? do you have any comments we'd need to address?

Thanks.

@Karsten1987
Copy link
Collaborator

Karsten1987 commented Feb 5, 2019

I think it might be more useful to enable ros2 run like runtime arguments like __node:=<node_name> and __ns:<node_namespace>

just to make sure, I am fully understanding what's happening here. The current PR as-is does not address this feature quoted above but rather introduce a node-prefix, is that correct?

if so, I believe this PR is consistent with the changes just recently merged to ros2cli and can be merged.
running CI for it:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator

it looks like we run into the same issue than here:
ros2/rcl#375

i have to see whether ros2/rcl#350 is related to it.

@rutvih
Copy link

rutvih commented Feb 11, 2019

@Karsten1987 fix for logging was pushed here: ros2/rcl#384

@Karsten1987
Copy link
Collaborator

new round of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

test failures are due to updated cppcheck. Unrelated to this change.

@Karsten1987 Karsten1987 merged commit 3397b46 into ros2:master Feb 12, 2019
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.

5 participants