ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
154 stars 181 forks source link

Feature Add: Interfaces check private namespaced ~robot_ip_address before public parameter. #203

Closed JoshOlds closed 3 years ago

JoshOlds commented 6 years ago

Currently, the robot_state_interface and joint_trajectory_interface look to the public parameter server to get the robot_ip_address parameter. This can cause problems with multiple robots (in my case, multiple subsystems) running on a single ROS system.

This change allows the interfaces to search for a private namespaced IP parameter before resorting to the public parameter. This preserves backwards compatibility while allowing for launch files to specify private parameters within a tag.

gavanderhoorn commented 6 years ago

Hi, thanks for the PR.

The idea behind most of the nodes in industrial_core is that instead of using private namespaces, they should be pushed down into namespaces collectively if multiple instances are needed (ie: ns="left" and ns="right" for a dual-arm system fi).

Afaik, this should work for robot_ip_address as well, as the code you changed did not use an absolute name, but a relative one (ie: no leading /). This allows you to specify robot_ip_address once, and all nodes in that namespace should be able to find it. If we use private parameters everywhere, we'd have to duplicate them into all node namespaces.

I'm not necessarily against what you propose (although ros::param::search(..) might be a bit cleaner), just wanted to let you know.

gavanderhoorn commented 3 years ago

I'm going to close this, as the current implementation should already support namespacing parameters, it just doesn't look in the private namespace of industrial_robot_client nodes.

The reason for this is that robot_ip_address is not really a parameter just for any single node, it's typically used by all IRC nodes in a system (they're talking to the same remote IP). Having robot_ip_address as a private parameter will greatly complicate such a setup, as the ROS parameter clients typically don't search down into other namespaces. Only up. They would then need to know where to "find" a specific robot_ip_address parameter, which makes the whole setup rather brittle. Alternatively, we'd have to duplicate parameters for all affected nodes, which would also be less than ideal.

gavanderhoorn commented 3 years ago

I'd want to thank you for making the effort of submitting a PR in any case. Even if it's a few years after the fact.