ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
468 stars 298 forks source link

ROS_NAMESPACE set results in message loopback #254

Open agoeckner opened 4 years ago

agoeckner commented 4 years ago

Bug report

Required Info:

Steps to reproduce issue

In terminal 1:

source /opt/ros/melodic/setup.bash
export ROS_NAMESPACE=test
rostopic echo /topic

In terminal 2:

source /opt/ros/eloquent/setup.bash
export ROS_NAMESPACE=test
ros2 topic echo /topic

In terminal 3:

source /opt/ros/eloquent/setup.bash
export ROS_NAMESPACE=test
ros2 topic pub /topic topic_type -1

Expected behavior

After publishing to /topic in terminal 3, a single message should be printed in each of terminal 1 and terminal 2.

Actual behavior

After publishing to /topic in terminal 3, a single message is printed to terminal 1, but two messages are printed to terminal 2. This is caused by loopback as follows:

  1. ROS2 publishes to /topic
  2. Bridge sends message to ROS1
  3. Bridge receives its own message in ROS1
  4. Bridge re-publishes message in ROS2

Additional information

The offending code is located here: https://github.com/ros2/ros1_bridge/blob/master/include/ros1_bridge/factory.hpp#L175 That line must be changed to account for ROS namespaces.

Workaround

If you need an immediate fix, run ros1_bridge as follows:

(unset ROS_NAMESPACE; ros2 run ros1_bridge dynamic_bridge)&
dirk-thomas commented 4 years ago

Thanks for the report.

That line must be changed to account for ROS namespaces.

Please consider to contribute a pull request for this issue.

iuhilnehc-ynos commented 4 years ago

@agoeckner

Thank you for reporting this issue. After using the steps you provided, I can't reproduce it.

For clearly, I supposed you did as following,

terminal 1:
    $ source /opt/ros/melodic/setup.bash
    $ roscore

terminal 2:
    $ source /opt/ros/melodic/setup.bash
    $ export ROS_NAMESPACE=test
    $ rostopic echo /chatter
      (expected to get message from ros1_bridge, the topic name is '/chatter'. About ros1, names that start with a "/" are global -- they are considered fully resolved.)

terminal 3:
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( useless here )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 topic echo /chatter std_msgs/msg/String
    $ (expected to get message from 'ros2 topic pub', the topic name is '/chatter')

terminal 4:
    $ source /opt/ros/melodic/setup.bash
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( not necessary, but for trying reproduce the issue. The workaround said 'unset ROS_NAMESPACE', assume you set it for ros1_bridge )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 run ros1_bridge dynamic_bridge

terminal 5:
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( useless here )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 topic pub /chatter std_msgs/msg/String "{data: 'Hello World'}"
        or (RMW_IMPLEMENTATION=rmw_connext_cpp ros2 run demo_nodes_cpp talker)
      (send data to ros1_bridge and 'ros2 topic echo', the topic name is '/chatter'

From what I know, ROS_NAMESPACE is an environment variable for ROS1, not ROS2.

Actual behavior

  1. terminal 2 received message data about '/chatter' once
  2. terminal 3 received message data about '/chatter' once

I tried using the namespace for ROS1 and ROS2 to reproduce this issue, but the behavior is also correct.

terminal 1:
    $ source /opt/ros/melodic/setup.bash
    $ roscore

terminal 2:
    $ source /opt/ros/melodic/setup.bash
    $ export ROS_NAMESPACE=test
    $ rostopic echo chatter
      (not to topic name started with '/'. expected to get message from ros1_bridge, the topic name is '/test/chatter')

terminal 3:
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( useless here )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 topic echo /test/chatter std_msgs/msg/String
    $ (expected to get message from 'ros2 topic pub', the topic name is '/test/chatter', use 'test/chatter' directly because 'ros2 topic' seems not support '--ros-args -r __ns:=test')

terminal 4:
    $ source /opt/ros/melodic/setup.bash
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( not necessary, but for trying reproduce the issue. The workaround said 'unset ROS_NAMESPACE', assume you set it for ros1_bridge )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 run ros1_bridge dynamic_bridge

terminal 5:
    $ source /opt/ros/eloquent/setup.bash
    $ export ROS_NAMESPACE=test ( useless here )
    $ RMW_IMPLEMENTATION=rmw_connext_cpp ros2 topic pub /test/chatter std_msgs/msg/String "{data: 'Hello World'}"
        or (RMW_IMPLEMENTATION=rmw_connext_cpp ros2 run demo_nodes_cpp talker --ros-args -r __ns:=test)
      (send data to ros1_bridge and 'ros2 topic echo', the topic name is '/chatter'

log as below:

terminal 2:
...
data: "Hello World: 35"
---
data: "Hello World: 36"
---
data: "Hello World: 37"
---

terminal 3:
...
data: 'Hello World: 35'
---
data: 'Hello World: 36'
---
data: 'Hello World: 37'
---

terminal 5:
...
[INFO] [test.talker]: Publishing: 'Hello World: 35'
[INFO] [test.talker]: Publishing: 'Hello World: 36'
[INFO] [test.talker]: Publishing: 'Hello World: 37'

Could you provide more info about how to reproduce this issue?

iuhilnehc-ynos commented 4 years ago

@agoeckner friendly ping

agoeckner commented 4 years ago

@agoeckner friendly ping

Sorry, I forgot about this. I will try to get more info when I go in to work tomorrow.

iuhilnehc-ynos commented 4 years ago

@agoeckner It's okay. If you ready to post more info, I appreciate that.

joelbudu commented 3 years ago

@agoeckner any update on this issue please? I'm experiencing this bug as well.

agoeckner commented 3 years ago

@agoeckner any update on this issue please? I'm experiencing this bug as well.

No, sorry. We don't have budget/approval for fixing ROS issues unfortunately. However, the workaround listed in OP does work well.

fujitatomoya commented 3 years ago

https://github.com/ros2/ros1_bridge/issues/254#issuecomment-631257706 seems to be a no problem we have.

@hidmic can we close this issue?

joelbudu commented 3 years ago

I have this issue when I create run the ros1_bridge with any __ns argument. The /tf and /tf_static topics are producing an extremely high frequency as shown below:

Screenshot from 2021-08-24 16-49-31

Without a __ns argument the frequencies are normal as below:

Screenshot from 2021-08-24 16-51-44

fujitatomoya commented 3 years ago

@joelbudu thanks for sharing information, so then let's keep this open. CC @iuhilnehc-ynos

joelbudu commented 3 years ago

The offending code is located here: https://github.com/ros2/ros1_bridge/blob/master/include/ros1_bridge/factory.hpp#L175 That line must be changed to account for ROS namespaces.

Based on the information provided by @agoeckner I tested a fix by changing lines https://github.com/ros2/ros1_bridge/blob/master/include/ros1_bridge/factory.hpp#L193-L198

to

const std::string key = "callerid";
    const std::string node_name = "/ros_bridge";

    if (connection_header->find(key) != connection_header->end()) {
      const std::string callerid = connection_header->at(key);
      if ( callerid.compare(callerid.size() - node_name.size() ,node_name.size(),node_name) == 0) {
        return;
      }
    }

Essentially this searches to find if the node node ends with the string "/ros_bridge"