Feature/crx10ial

[Zedderrs]

This PR aims to implement the moveit_config and support for the crx10ial robot

[gavanderhoorn]

Thanks for the new PR.

Just for my understanding: this is the raw export created by the SW URDF plugin, correct?

Would you be available for making/accepting corrections -- mainly to get this in-line with the other support packages here in this repository?

[Zedderrs]

Just for my understanding: this is the raw export created by the SW URDF plugin, correct?

I started with the sw2urdf output but ultimately needed to modify it somewhat to make it work.

Would you be available for making/accepting corrections -- mainly to get this in-line with the other support packages here in this repository?

Yes definitely!

[gavanderhoorn]

Great. I'll add some high-level comments in a first-round review.

[gavanderhoorn]

Due to the fact this was an almost direct export by the SW URDF plugin, the structure of the package and contents of the files is rather different from what we have in all our other support packages.

As we'd like to maintain a level of consistency, I've reviewed the support package (so just the 7406edc commit) and noted everything in which the package in this PR diverges from the standard layout of support packages.

I wanted to add this 'intro', as it's a bit of a long list, and I did not want to dump this here without a short explanation.

Many of the points below don't take long to address, others might need a little more time. In most cases I've included links to examples, tutorials or documentation which explains why we do these things like this. Most of the time, looking at other support packages should give you a template for how to do it.

So with that out of the way, here are some of the issues I found with the current state of the support package in 7406edc:

1. please use the base series name for the package name. In this case that would be fanuc_crx10ia_support
2. please remove all references to and dependencies on Gazebo. Support for specific simulators should be provided by simulator-specific packages
3. please compare the structure of your package to (for instance) fanuc_m10ia_support. Could you restructure your package to that layout? See Working with ROS-Industrial Robot Support Packages on the ROS wiki for some more information on why that layout and how the files and directories should be used, and what their names should be
4. please also compare the contents of the .launch files in that package (or any of the other support packages). See again the Working with ROS-Industrial Robot Support Packages tutorial linked above.
5. we don't have permission from Fanuc to use "detailed meshes" in our robot support packages. See RViz only shows 'collision quality' models in the troubleshooting guide on the ROS wiki for some more context. Could you please generate collision quality meshes and use them everywhere in the xacro:macro?
6. please remove the export.log
7. please use 2-space indentation everywhere
8. please make sure to never use any uppercase characters in file or directory names (STL files in this case)
9. please use the standard package.xml common to all support packages (see the fanuc_m10ia_support package for a template)
10. please make sure to use appropriate names for the xacro:macro (so the macro it defines, not the file): prefix by fanuc_
11. please try to reorient meshes such that they are 'naturally' aligned with both the zero-pose of the robot and the standard ROS REP-103 axis chirality (ie: forward: X+, to the left: Y+, up: Z+). Almost all meshes appear to need a rotation over the X axis right now.
12. please try to avoid translations and rotations in joints over and in more than one dimension. joint_2 and joint_3 translate in both Y and Z. Please compare the kinematic structure of (for instance) the fanuc_m10ia xacro:macro
13. please specify correct types and limits for each joint: all joints appear to be set to continuous without any limits, which does not seem to be correct for the modelled variant
14. please try to use colors and/or materials from fanuc_resources. If the required material is not available, please define new ones and include them in this PR
15. please include the default set of frames in the xacro:macro: base, flange and tool0. Refer to fanuc_m10ia_support for a template, and see Coordinate Frames for Serial Industrial Manipulators for more information on those frames
16. please do not include any additional frames in the base .xacro files. Specifically: remove the addition of the world link and joint in crx10ial_robot.urdf.xacro

[gavanderhoorn]

The CI run also highlighted some catkin_lint problems:

  fanuc_crx10ial_support: CMakeLists.txt: error: find_package(roslaunch) has no REQUIRED option
       * The package cannot build without this dependency, so it should
       * be marked as REQUIRED accordingly. Use if(roslaunch_FOUND)
       * clauses to use optional packages.
       * You can ignore this problem with --ignore missing_required
  fanuc_crx10ial_support: CMakeLists.txt: error: unconfigured build_depend on 'robot_state_publisher'
       * You declare a build dependency on another package but neither
       * call find_package() nor have it listed as catkin component in
       * the find_package(catkin) call.
       * You can ignore this problem with --ignore unconfigured_build_depend
  fanuc_crx10ial_support: CMakeLists.txt: error: unconfigured build_depend on 'rviz'
  fanuc_crx10ial_support: CMakeLists.txt: error: unconfigured build_depend on 'joint_state_publisher_gui'
  catkin_lint: checked 48 packages and found 4 problems

these should be automatically addressed by using the standard content for both CMakeLists.txt and package.xml.

[gavanderhoorn]

Marking request changes so we don't accidentally merge this.

[gavanderhoorn]

@Zedderrs: would you have time to address the points I identified with the support package?

[gavanderhoorn]

@Zedderrs: did you close this on purpose?

[Zedderrs]

@Zedderrs: did you close this on purpose?

Yes sorry, there were some factors preventing me from keeping this PR open unfortunately. I hope to submit one again in the future...but for now it must be closed