Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ur_description and ur_gazebo are improved and simplified #624

Open
wants to merge 15 commits into
base: melodic-devel-staging
Choose a base branch
from

Conversation

BobbyCephy
Copy link
Contributor

@BobbyCephy BobbyCephy commented Nov 16, 2022

  • Model specific xacro and launch files are replaced by generic ones.

  • Default arguments and parameters are only defined once in lowest level files by replacing empty strings.

  • Properties that are same as default are omitted.

  • All properties are overridable in upper level files.

  • Arguments and parameters are redefined less by pass_all_args in launch files and using upper level parameters in xacro files.

  • robot_model is changed to model for easier calling for example of: roslaunch ur_description view.launch model:=ur3

  • Macros usifilesng configuration data for link, Gazebo reference link, fixed and revolute joint are added.

  • Following link and joint names are changed for consistent naming, which is desirable for link and joint macros:
    base_link_inertia to base_inertia_link
    elbow_joint to elbow in joint_limits.yaml
    upperarm to upper_arm in physical_parameters.yaml and of mesh files

  • Offset parameters of meshes are added in visual_parameters.yaml, addressing in macro.xacro:

    <link name="${prefix}wrist_1_link">
      <visual>
        <!-- TODO: Move this to a parameter -->
        <origin xyz="0 0 ${wrist_1_visual_offset}" rpy="${pi/2} 0 0"/>
  • transmission_hw_interface only needs to contain e, v or p in lower or upper case, as an abbreviation, to be replaced by
    hardware_interface/EffortJointInterface,
    hardware_interface/PositionJointInterface or
    hardware_interface/VelocityJointInterface.

  • Fractions and squares are added in the cylinder_inertial macro.

  • Gazebo arguments initial_joint_positions, world_pose and unpause at spawning argument are added to control.launch, as by the moveit_setup_assistant.

  • Naming of properties, files and directory structure is simplified, comments and format adjusted.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Nov 16, 2022

Thanks for the PR.

Let me start by a comment on form, not function.

The PR consists of a single commit, changing the following this (from your commit comment):

  • Model specific xacro and launch files replaced by generic ones.
  • Default arguments and parameters only defined once in lowest level files by replacing empty strings.
  • Omit properties same as default. Still all properties can be overridden in upper level files.
  • Less redefining of arguments and parameters by pass_all_args in launch and using upper level parameters in xacro files.
  • transmission_hw_interface only needs to contain parts of position, velocity or effort in lower or upper case.
  • Names of properties, files and directory structure simplified.
  • Macro for individual transmission and fixed joint added.
  • Fractions and squares cylinder_inertial macro.
  • initial_joint_positions, full world_pose and unpause argument at spawning added to control.launch, as by moveit_setup_assistant.
  • robot_model changed to model for easier calling for example: roslaunch ur_gazebo bringup.launch model:=ur5

It touches 98 files.

In its current form, this is difficult to review.

I would suggest to split it up into multiple commits, each introducing (a) coherent set(s) of changes.

As an additional comment, let me also say that (some of) the duplication here was on purpose: in my years of maintaining (ROS) software, I've noticed (and have had the feedback) that the more parameters or arguments something takes, the complexer things become.

Users with programming experience typically do not mind too much, as they are used to variability and configuring that variability using parameters (arguments to functions fi).

Users with other backgrounds tend to appreciate a single file which caters to their use-case, with other files for other use-cases.

It's very clear you have to work with files ending in _ur3 fi if you have a UR3.

At first glance, this PR seems to go in the more-parameters direction. That's not necessarily a bad thing, but I do believe we should be mindful of the complexity of things and how changes like these potentially impact the experience of users.

@gavanderhoorn
Copy link
Member

I would perhaps also suggest to describe your motivation for this PR.

Why did you change things, and why did you change them in this particular way?

@BobbyCephy
Copy link
Contributor Author

Thank you for your quick and extensive reply! I am going to split the commit in the suggested changes.
The large number of touched files is mainly caused by replacing model specific with single generic ones.
The only new parameter and argument is actually only model?
I guess it is easier having only a few files in a single folder to check, which is needed and not many ones that are nearly the same. The only differing model argument should be quickly realized. The main advantage of few generic files is only having to apply changes at a single place, which makes maintaining a lot easier.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Nov 17, 2022

The main advantage of few generic files is only having to apply changes at a single place, which makes maintaining a lot easier.

while I don't disagree, this is probably exactly where the trade-off lies.

Do we optimise for maintenance effort, or for user experience?

Given the fact these are ROS 1 xacro:macros, I'm not sure how much we gain by re-arranging things so late 'in the game'.

@BobbyCephy
Copy link
Contributor Author

I think it is worth reducing the number of specific and generic files to a seventh by introducing one additional parameter. The user experience should be improved as well, since it is easier to get an overview of a small set of files.
In my opinion it is never to late for improvements. Even if few benefit from them directly, one can learn from them in future projects.
A compromise might be a script, that generates specific from generic files on demand, that are ignored by git?

BobbyCephy and others added 11 commits November 20, 2022 15:47
Will be replaced by generic ones
Model specific xacro and launch files replaced by generic ones.
Default arguments and parameters only defined once in lowest level files by replacing empty strings.
Omit properties same as default.
Still all properties can be overridden in upper level files.
Less redefining of arguments and parameters by pass_all_args in launch and using upper level parameters in xacro files.
Paths, file names, names, comments and format adjusted.
Consistent link naming desirable for link and joint macros.
Consistent naming in joint_limits.yaml
Consistent naming in physical_parameters.yaml and of mesh files
Addressing in macro.xacro:
    <link name="${prefix}wrist_1_link">
      <visual>
        <!-- TODO: Move this to a parameter -->
        <origin xyz="0 0 ${wrist_1_visual_offset}" rpy="${pi/2} 0 0"/>
transmission_hw_interface only needs to contain
e, v or p in lower or upper case to be replaced by
hardware_interface/EffortJointInterface,
hardware_interface/PositionJointInterface or
hardware_interface/VelocityJointInterface.
initial_joint_positions, world_pose and unpause at spawning argument added to control.launch, as by moveit_setup_assistant.
Comments adjusted
@BobbyCephy BobbyCephy changed the title Simplified ur_description and ur_gazebo by using model argument Simplified ur_description and ur_gazebo Nov 20, 2022
@BobbyCephy BobbyCephy changed the title Simplified ur_description and ur_gazebo ur_description and ur_gazebo are improved and simplified Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants