ros-industrial / robotiq

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

Feature/robotiq ros control #63

Closed athackst closed 7 years ago

athackst commented 8 years ago

I've been working on getting my robotiq-s model gripper working with the CAN bus plus implementing a ros-control interface. I'd like to contribute my work back, so I integrated it into the current robotiq_s_model_control package. Currently, I've abstracted out the comm layer through a base client class so that my changes should also work with ethercat, though I don't have an ethercat device to test.

Summary of changes

Is there any interest in the community for this?

I'm open to suggestions/improvements (and somebody please test on ethercat).


This change is Reviewable

shaun-edwards commented 8 years ago

@athackst, thanks for the contribution. There are a significant amount of changes in this PR. My initial thought is that we should pull this into a future release (probably jade). Another option would be to modify this PR such that it doesn't delete or modify any existing object in an incompatible way. There's a little extra work to do this, but it allows us to deprecate old classes while introducing the new ones. What do you think?

athackst commented 8 years ago

Sure, I think I can pretty easily return the ethercat client class to mostly the original with the exception of the derived base class. I could also return the ethercat node to the original too, but then people using the ethercat version wouldn't get ros-control, but maybe the ros-control version would be better as a different node? Doing a rosrun on the executable should bring up the same interface as before without loading any controllers and should run as expected, so I was leaning toward not a different node. I tried to maintain the ros interface, but not having the ethercat device I wasn't quite sure on the namespacing.

Also, thinking about future release: It might be better to have the comm-specific nodes located in a comm-specific package so that you only need to install libraries for the one you care about. Or perhaps wrapping the comm client into a pluginlib instead?

athackst commented 8 years ago

Not sure why CI is failing? The error message says can't find rake file?

gavanderhoorn commented 8 years ago

Not sure why CI is failing? The error message says can't find rake file?

the repository hasn't been setup yet for travis, but it is enabled. Hence the failures.

shaun-edwards commented 8 years ago

@Jmeyer1292, can you take a quick look at this PR and chime in. I believe it effects/is related to the EtherCAT work.

Jmeyer1292 commented 8 years ago

@shaun-edwards I'll take a look. It'll take me a day or two to go through this.

shaun-edwards commented 8 years ago

@Jmeyer1292, it will take a day or two to review or you won't get to it for a day or two?

Jmeyer1292 commented 8 years ago

It'll take an evening to go through the code, and I'll want to do my due diligence and test our ethercat grippers. So both.

shaun-edwards commented 8 years ago

@Jmeyer1292, let's just go through the code at this point. I think there will be some structural changes we will need to make, but I want to be certain you agree with the approach that @athackst is taking.

shaun-edwards commented 8 years ago

@Jmeyer1292, ping.

shaun-edwards commented 8 years ago

New header files and source files should contain a license. The ROS-I preferred license is Apach 2.0.

athackst commented 7 years ago

Added the licenses!

shaun-edwards commented 7 years ago

Thanks @athackst, I'll take a look at this in the upcoming week.

shaun-edwards commented 7 years ago

I've switched the PR over to the jade branch (it was pretty easy, see here). The jade travis builds are failing due to a missing soem dependency. I've pinged the maintainer to see if they can do a release. I'm going to approve the PR and will address this in #52.

gavanderhoorn commented 7 years ago

If you want to get Travis working again and soem can be build from sources, see ros-industrial/staubli_experimental for a way to do that with industrial_ci. The .travis.rosinstall file lists additional repositories that need to be checked out by Travis before starting the build.

shaun-edwards commented 7 years ago

@gavanderhoorn, thanks for the tip. Is there a reason that it's a .travis.rosinstall instead of a rosinstall file? I think the latter may be more recognizable to those who might try to build from source (maybe not).

gavanderhoorn commented 7 years ago

It's a feature of industrial_ci, not a .rosinstall file for general use.

See Build depended packages from source - Use .rosinstall file to specify the depended packages source repository.