ros-industrial / robotiq

Robotiq packages (http://wiki.ros.org/robotiq)
BSD 2-Clause "Simplified" License
229 stars 378 forks source link

replace control msgs with robotiq_3f_gripper_articulated_msgs #143

Closed christian-rauch closed 5 years ago

christian-rauch commented 5 years ago

There are two sets of input/output message definitions for the 3-finger gripper. This PR unifies these message definitions and removes redundancy (Fixes #108 ).

It adds rGLV (glove mode) to the articulated msgs to make them compatible (by hash) to the control msgs and consequently replaces all occurances of robotiq_3f_gripper_control/Robotiq3FGripper_robot_{input,output} with robotiq_3f_gripper_articulated_msgs/Robotiq3FGripperRobot{Input,Output}.

Tested with python nodes Robotiq3FGripperTcpNode, Robotiq3FGripperSimpleController and Robotiq3FGripperStatusListener on real Robotiq 3-finger hand.

I also took the freedom to clean-up the python files :-)

christian-rauch commented 5 years ago

@jproberge Can you have a look at this? I know it breaks API for people that use the robotiq_3f_gripper_control message definitions but I think that it makes the usage more clear, since there is only a single interface to the real controller and the simulator.

jproberge commented 5 years ago

@christian-rauch sure! I'm currently busy, but I'll review / check this PR with the others as well this weekend.

Thanks!

jproberge commented 5 years ago

Hi @christian-rauch !

Sorry for the long delays due to lack of time on my side! I've finally managed to review your changes, and I completely agree that moving to a single set of message would be much much better and appropriate. Thanks also for the addition of the missing command, as noticed by Clearpath folks. I'm a user of this gripper in simulation and in the real world, so I totally understand how this would be an improvement.

However, you are right, this will break the interface for people that are using the current version of the package stack and in this vein, I was just wondering why you decided to choose the "articulated" messages (gazebo) over the "non-articulated" (control) ones? I think there is definitely a lot more people depending on the stability of the non-articulated messages rather than the articulated ones (i.e., there is a lot more people using the package to control the devices in the real-world rather than in simulation), so why not replacing the articulated msg by the "control" ones?

christian-rauch commented 5 years ago

I mainly went for articulated msgs because they are already in their own dedicated package robotiq_3f_gripper_articulated_msgs. The message definitions in robotiq_3f_gripper_control are part of the control package and ROS usually recommends to have dedicated message packages. This has the advantage that one can use the messages (e.g. for replaying them or for a simulator) without the need to install the control package on their workstations.

christian-rauch commented 5 years ago

@jproberge Regarding the package for message definitions, are you considering merging this if the common messages are defined in the dedicated package robotiq_3f_gripper_articulated_msgs? Or is there only the chance of merging, if the messages in the controller package are used? I personally think that messages should be placed in dedicated packages (robotiq_3f_gripper_articulated_msgs) but I am willing to use the controller messages if it increases the chances of getting this merged upstream.

jproberge commented 5 years ago

@christian-rauch I too think that the dedicated package for the messages should be used instead of using those from the "gripper_control" package. I've reviewed your changes and I think this can and will be merged. However, I'd just like to test this one last time on real hardware, which I should be able to do on Thursday. I'll report right after.

jproberge commented 5 years ago

This works as expected (I've just tested it) on hardware and I believe it conceptually makes sense.