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

Remap two nodes of one executable file with same node name in different namespaces #806

Open
iuhilnehc-ynos opened this issue Sep 21, 2020 · 8 comments
Labels
backlog enhancement New feature or request

Comments

@iuhilnehc-ynos
Copy link
Collaborator

iuhilnehc-ynos commented Sep 21, 2020

Bug report

Required Info:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • source
  • Version or commit hash:
  • DDS implementation:
    • Fast-RTPS
  • Client library (if applicable):
    • rclcpp

Steps to reproduce issue

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__node:=aaa \
    -r aaa:__ns:=/ns1 \                                     // Must use aaa because of current `Order of Applying Remapping Rules`
    -r subscriber_node:__node:=aaa \
    -r aaa:__ns:=/ns2                                       // bad

Expected behavior

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__ns:=/ns1 \
    -r publisher_node:__node:=aaa \
    -r subscriber_node:__ns:=/ns2 \
    -r subscriber_node:__node:=aaa

$ ros2 node list
/ns1/aaa
/ns2/aaa

Actual behavior

$ ros2 node list
/ns1/aaa
/ns1/aaa

Additional information

Remapping rules are applied in the following order:

    1. Node name remapping
    1. Namespace remapping // Should namespace remapping be the first one?
    1. All other rules
@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Sep 21, 2020

I know the old usage in user applications must be updated after this fix.
such as updating from

    -r publisher_node:__node:=aaa \
    -r aaa:__ns:=/ns1                                     // aaa is not effective.

to

    -r publisher_node:__node:=aaa \
    -r publisher_node:__ns:=/ns1

but I think using this order of remapping rules that remapping namespace before node name seems no limitation.

@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos

Actual behavior

I have the following result, it is slightly different from yours. (with ros2/ros2@ca09b31)

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__ns:=/ns1 \
    -r publisher_node:__node:=aaa \
    -r subscriber_node:__ns:=/ns2 \
    -r subscriber_node:__node:=aaa
[WARN] [1600843894.399257225] [rcl.logging_rosout]: Publisher already registered for provided node name. If this is due to multiple nodes with the same name then all logs for that logger name will go out over the existing publisher. As soon as any node with that name is destructed it will unregister the publisher, preventing any further logs for that name from being published on the rosout topic.
[INFO] [1600843894.899459042] [aaa]: Publisher: 'Hello, world! 0'
[INFO] [1600843894.899907305] [aaa]: Subscriber: 'Hello, world! 0'

$ ros2 node list
WARNING: Be aware that are nodes in the graph that share an exact name, this can have unintended side effects.
/aaa
/aaa

could you confirm the problem?

@fujitatomoya
Copy link
Collaborator

okay, now i see it. sorry i was too early to ask you.

@iuhilnehc-ynos
Copy link
Collaborator Author

Never mind. Do you think if we should reorder the remapping of node name and namespace?

@iuhilnehc-ynos iuhilnehc-ynos changed the title Remap two nodes with same node name in different namespaces Remap two nodes of one executable file with same node name in different namespaces Sep 25, 2020
@ivanpauno
Copy link
Member

what happens if you do:

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__ns:=/ns1 \
    -r publisher_node:__node:=aaa \
    -r subscriber_node:__ns:=/ns2 \
    -r subscriber_node:__node:=aaa \                  

does thus work?

Based on the design doc, I think it should be used like that.

@ivanpauno ivanpauno added the question Further information is requested label Oct 14, 2020
@iuhilnehc-ynos
Copy link
Collaborator Author

does thus work?

No, it doesn't work. This's my expectation, so I think we need to update the design document of Order of Applying Remapping Rules

@ivanpauno
Copy link
Member

No, it doesn't work. This's my expectation, so I think we need to update the design document of Order of Applying Remapping Rules

Ah, ok. Yeah reading the docs again that doesn't work.

Remapping two nodes that originally have a different name to a same new name but with a different namespace doesn't seem possible.
I'm wondering if that use case is important, why would someone want to do that? I mean, picking a different names for both doesn't seem to be bad, and it avoids the issue.

The only downside of the current approach is that some examples can be counter-intuitive, but I'm not sure if that can completely be avoided.

@ivanpauno ivanpauno added enhancement New feature or request backlog and removed question Further information is requested labels Oct 29, 2020
@ivanpauno
Copy link
Member

I will left this open and marked as backlog.
For pushing the modification further, a non-breaking alternative should be implemented.

See ros2/design#299 (comment) for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants