search

ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
224 stars 192 forks source link

Update remapping order of __ns and __node to not affect each other #299

Closed iuhilnehc-ynos closed 4 years ago

iuhilnehc-ynos commented 4 years ago

I encountered an issue about remapping two nodes that originally have a different name to the same new name but with a different namespace.

ex. there are two nodes in examples_rclcpp_minimal_composition, and we want to remap node names publisher_node -> /publisher_different_ns/test_same_node subscriber_node -> /subscriber_different_ns/test_same_node

we want to do remapping names like the following way,

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__ns:=/publisher_different_ns \
    -r publisher_node:__node:=test_same_node \
    -r subscriber_node:__ns:=/subscriber_different_ns \
    -r subscriber_node:__node:=test_same_node

rather than using

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r test_same_node:__ns:=/publisher_different_ns \ 
    -r publisher_node:__node:=test_same_node \
    -r test_same_node:__ns:=/subscriber_different_ns \
    -r subscriber_node:__node:=test_same_node

, since the existing remapping order that parsing __node before __ns causes the latter command to use the same node_name prefix of __ns parameter for two nodes.

Related to https://github.com/ros2/rcl/issues/806

Signed-off-by: Chen Lihui Lihui.Chen@sony.com

iuhilnehc-ynos commented 4 years ago

I have no idea if we should apply for using the FQN prefix and remapping order. To push this commit is mainly for discussion.

clalancette commented 4 years ago

@sloretz Since you wrote the original design document here, could you maybe weigh in on this design change?

ivanpauno commented 4 years ago

With this proposal, how can the following example be solved:

remap /ns1/node /ns2/node to /new_node1 /new_node2 respectively.

?

This isn't currently possible, but it wasn't uncommon in ROS 1 I think.

iuhilnehc-ynos commented 4 years ago

remap /ns1/node /ns2/node to /new_node1 /new_node2 respectively.

That's interesting. I think this case should be also supported.

$ ros2 run package exec_name \
    --ros-args \
    -r /ns1/node:__ns:=/ \              # the FQN must use original value
    -r /ns1/node:__node:=new_node1 \    # the FQN must use original value
    -r /ns2/node:__ns:=/ \
    -r /ns2/node:__node:=new_node2

and remap publisher_node subscriber_node to /publisher/test_node /subscriber/test_node

$ ros2 run package exec_name \
    --ros-args \
    -r /publisher_node:__ns:=/publisher \
    -r /publisher_node:__node:=test_node \
    -r /subscriber_node:__ns:=/subscriber \
    -r /subscriber_node:__node:=test_node

after using the original value for prefix, it also breaks the backward compatibility. ex. /ns1/node -> /new_node1

$ ros2 run package exec_name \
    --ros-args \
    -r new_node1:__ns:=/ \
    -r node:__node:=new_node1 \
$ ros2 run package exec_name \
    --ros-args \
    -r node:__ns:=/ \
    -r node:__node:=new_node1 \

Edit: If you agree with the above information, I'll update the design, otherwise I think it's time to close this issue.

ivanpauno commented 4 years ago

Edit: If you agree with the above information, I'll update the design, otherwise I think it's time to close this issue.

I think that a proposal that handle both cases above correctly would be acceptable.

iuhilnehc-ynos commented 4 years ago

In this PR, I would not add the FQN prefix in it because it's different from the remapping order. If it's necessary, I'll create a new PR for it.

@ivanpauno I have updated the design, please review it.

ivanpauno commented 4 years ago

@iuhilnehc-ynos I think the proposal is reasonable.

It would be great if you can edit the post description and include a motivation of the changes and examples of what the change enables (so future reviewers can understand the rationale of the change).

In this PR, I would not add the FQN prefix in it because it's different from the remapping order. If it's necessary, I'll create a new PR for it.

Combined with allowing FQN in prefixes, this allow even more use cases. I don't think that both changes need to be done at the same time, but it would be good to explain how it works in the description.

iuhilnehc-ynos commented 4 years ago

@ivanpauno

I don't think that both changes need to be done at the same time, but it would be good to explain how it works in the description.

Thanks for your understanding. I have updated the description.

sloretz commented 4 years ago

Implementation considerations - it seems possible to implement this by passing the original name arg to rcl_remap_node_namespace() - which could be done by calling rcl_remap_node_namespace() before rcl_remap_node_name() so the name variable is unchanged.

https://github.com/ros2/rcl/blob/e9c87e279bf7509c940b87fd4d6c5dff301bc735/rcl/src/rcl/node.c#L220-L240

iuhilnehc-ynos commented 4 years ago

But then it doesn't look so weird if I order them this way

Thanks for your example.

I think it should be made clearer in the Match Part of a Rule section that whether nodename: refers to the pre-remaping name or the post-remapping name depends on the rule.

But there already exist information about this pre-remapping and post-remapping in Order of Applying Remapping Rules Within each category, the rules are applied in the order in which the user gave them. If adding this kind of information in the Match Part of a Rule section, it seems a little duplicated.

sloretz commented 4 years ago

But there already exist information about this pre-remapping and post-remapping in Order of Applying Remapping Rules Within each category, the rules are applied in the order in which the user gave them. If adding this kind of information in the Match Part of a Rule section, it seems a little duplicated.

If not the match rule section, how about another example showing node name remapping and topic name remapping together? I think it'd be helpful because this case looks a little confusing:

 -r alice:__node:=bob
 -r alice:foo:=baz
 -r bob:foo:=bar
ivanpauno commented 4 years ago

Hi @iuhilnehc-ynos, here some feedback from the ros2 team meeting:

Bikeshedding: --inorder-name-ns could have a better name.

iuhilnehc-ynos commented 4 years ago

Was this proposal the result of a real use case? i.e. were you actually trying to remap /node1, /node2 to /ns1/node and /ns2/node in a real application?

No. These cases can be avoided in users' applications.

To avoid a breaking change (that in many cases will be a silent breakage), could this option been enabled with a prefix?

Yes, adding this option can avoid the breaking change, but it seems not kindly for users because there are two kinds of rules.

If you don't mind, I want to close this issue.

fujitatomoya commented 4 years ago

@iuhilnehc-ynos CC: @ivanpauno

I do not have any strong justification to push this design either.

ivanpauno commented 4 years ago

Yes, adding this option can avoid the breaking change, but it seems not kindly for users because there are two kinds of rules.

Agreed, but "silent breakage" (the one that doesn't make your build fail and makes your program behave different than before) is a big issue for existing users.

If you don't mind, I want to close this issue.

Sounds good to me. If the current order of remapping rules is a problem for some use cases, adding the --inorder-name-ns option would be the best path forward. It's not super nice to have two different orders for remapping rules, but this is a corner case that most users won't run into.

If you have any other alternatives to avoid breaking pre-existing code, that also would be acceptable.

Thanks for the proposal @iuhilnehc-ynos !

ivanpauno commented 4 years ago

Based on the previous comments, I'm going to close this. If there is an intention to push a non-breaking alternative of the proposal forward feel free to reopen.