ros-industrial / ur_modern_driver

(deprecated) ROS 1 driver for CB1 and CB2 controllers with UR5 or UR10 robots from Universal Robots
Apache License 2.0
302 stars 340 forks source link

Add configurable reverse ip address (#299) #301

Closed gonzalocasas closed 5 years ago

gonzalocasas commented 5 years ago

As described in #299, this pull requests adds an arg to the driver to allow configuring the reverse ip address used to connect from the robot back to the driver.

The change is rather trivial and has no impact on the actual control parts of the driver, but I tested on real hardware (UR5) just in case, and it works fine.

I did not find documentation for the existing arguments anywhere in the repository, and neither a wiki page documenting them, so I assume the launch files are considered self-documenting enough as they are.

gonzalocasas commented 5 years ago

@gavanderhoorn I applied your suggested corrections except for the defensive check of local_ip emptiness. I assume the change of behavior that it would cause is not desirable, but please correct me if I'm wrong. thx!

gonzalocasas commented 5 years ago

@gavanderhoorn ping 😉

I would really love to get this merged, if there are more comments/suggestions, I'd definitely apply them, but if not, I would appreciate a lot if this could make its way into kinetic-devel 😃

gonzalocasas commented 5 years ago

@gavanderhoorn thx!

Did you forget to change some of the reverse_ip_addresss? I've added some suggestions for those.

There's a bit of naming inconsistency currently and so far, I applied tried to "keep same convention on a per-file basis" (to put it somehow). In ros_main.cpp the other IP argument is called robot_ip_address (instead of robot_ip), so in that place, I also called the new one reverse_ip_address to be as close to existing code as possible. This means that launch files pass <param /> as reverse_ip_address but take <arg /> as reverse_ip (where it matches the existing robot_ip arg).

Now, if you think this mismatch does not matter and you rather have it consistently named reverse_ip, I can quickly change all the remaining ones to that.

gavanderhoorn commented 5 years ago

In ros_main.cpp the other IP argument is called robot_ip_address (instead of robot_ip), so in that place, I also called the new one reverse_ip_address to be as close to existing code as possible. This means that launch files pass <param /> as reverse_ip_address but take <arg /> as reverse_ip (where it matches the existing robot_ip arg).

Right, I forgot about that.

Please ignore my suggestions and let's keep robot_ip_address at the level of the node.

gavanderhoorn commented 5 years ago

Restarting CI build (let's see if Travis wants to play nice).

gavanderhoorn commented 5 years ago

Restarting again. Apparently Travis fixed their issue, so this should now succeed.

miguelprada commented 5 years ago

Sorry for taking so long to act on this.

I can see nothing with the changes and some test I ran with a UR5 seem to work as expected, so I'm going ahead and merging.

Thanks!

gonzalocasas commented 5 years ago

thank you, folks! :)

gavanderhoorn commented 5 years ago

Thanks for the PR @gonzalocasas.