search

ros-industrial / motoman_experimental

Experimental packages for Motoman manipulators within ROS-Industrial
Apache License 2.0
15 stars 32 forks source link

Initial commit for ES165RD-II support #12

Closed marip8 closed 4 years ago

gavanderhoorn commented 7 years ago

Nice work.

If I may make some suggestions:

  1. rename the package to motoman_es_support: not entirely sure, but looking at the product lines there appears to be an ES robot series, in which the ES165xx is a particular variant (together with the 200, the 280 and others). Support pkgs are intended to support multiple variants, to cut down on the nr of packages that we'd have to create otherwise.
  2. use 2 spaces for indentation everywhere (xacros, launch and xml files)
  3. make use_gui a private param of joint_state_publisher (in the test_..launch file)
  4. be specific about which variant is supported in the package manifest: right now it says "the base model"
  5. don't use shorted links in the manifest: retention isn't great, making the actual document harder to find if it is ever moved (on yaskawa's servers)
  6. I don't believe motoman_driver is a downloading driver (I see a robot_interface_download_es165rdii.launch file. Did you copy this from an ABB package?)
  7. material names do not get a ${prefix}
  8. ideally, joint transforms translate in a single dimension only (ie: x or z)
  9. the ${radians(..)} is nice, why the inconsistencies? There is a velocity limit there which does the conversion 'manually'.
  10. adding the base frame is nice, thanks for that. I'm not sure however about where you put the origin of that frame in your xacro. Have you verified this?
shaun-edwards commented 7 years ago

I updated the branch to kinetic...please let me know if it's needed in an early branch.

gavanderhoorn commented 7 years ago

Personally I would like to see all of these pkgs (including the mpl80 from #13) to be included in indigo-devel as well.

gavanderhoorn commented 7 years ago

Travis is failing because this repository has it enabled, but not configured.

I've opened #15 to rectify this.

After that is merged, could you rebase this PR @marip8?

marip8 commented 7 years ago

@gavanderhoorn I believe the latest commit addresses your comments in 1, 2, 3, 4, 7, 8, and 9.

Regarding 5, the URL to the datasheet has ampersands, so the package failed to build when I included it. Can you advise on how to get around that issue?

Regarding 6, I did copy that launch file from an ABB package; should I rename it to something more appropriate or do something else with it?

Regarding 10, I'm not sure what Motoman considers its world coordinate frame to be relative to the base_link; I'll look into fixing that

marip8 commented 7 years ago

@gavanderhoorn I can do the rebase when that change is made

gavanderhoorn commented 7 years ago

@marip8 wrote:

Regarding 5, the URL to the datasheet has ampersands, so the package failed to build when I included it. Can you advise on how to get around that issue?

You could perhaps replace them with &.

Regarding 6, I did copy that launch file from an ABB package; should I rename it to something more appropriate or do something else with it?

Take a look at the other support pkgs in motoman (motoman_sia20d_support fi): you'll see they have robot_interface_streaming_*.launch files. The MotoROS driver is a streaming driver, so the launch file should reflect that.

Regarding 10, I'm not sure what Motoman considers its world coordinate frame to be relative to the base_link; I'll look into fixing that

I think https://github.com/ros-industrial/motoman/pull/167#issuecomment-292966787 by @EricMarcil could help here:

The Motoman standard is that the robot coordinate system is at the S-axis rotation axis but at the height of the L-axis (2nd axis) rotation center.

Making the base frame correspond to that should make us conform to what Motoman does.

gavanderhoorn commented 7 years ago

See motoman_mpl_support/urdf/mpl80_macro.xacro for an example which I think you could duplicate.

gavanderhoorn commented 7 years ago

@marip8: could you rebase on kinetic-devel? The Travis config was just merged in #15 and I'd like to see this PR tested as well.

marip8 commented 7 years ago

@shaun-edwards

shaun-edwards commented 7 years ago

@marip8, is there a reason you changed me to maintainer? I am the maintainer for motoman but you could also be a maintainer (especially for packages with which you are more familiar). Just thought I'd ask.

marip8 commented 7 years ago

@shaun-edwards I'm fine with being the maintainer for this package. I was initially under the impression that the maintainer tag was for the repo manager, so that's why I had you there

shaun-edwards commented 7 years ago

@marip8, being a maintainer is a commitment...are you up for it? We can discuss tomorrow. I'll be in the office in the morning.

shaun-edwards commented 7 years ago

@marip8, travis is failing here: https://travis-ci.org/ros-industrial/motoman_experimental/jobs/233467864#L708

Let me know if you need help doing this tomorrow.

gavanderhoorn commented 4 years ago

Continued in #49.