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

Add OPW kinematics parameter files #284

Merged
merged 8 commits into from
May 19, 2020

Conversation

JeroenDM
Copy link
Contributor

To get the ball rolling on issue #245, I added the kinematic parameters from this repository to the support packages.


For the file name, I went for opw_kinematics_... instead of opw_params_... suggested here because it documents that these parameters are related to the robot kinematics.

As far as the parameter names go, I used the MoveIt specific kinematic_solver_, again because this documents what the parameters are for, and is backward compatible. But on this one, I'm also open to merge this diff and remove the MoveIt specific name.


Based on the discussion in #245, I think it would be nice if the MoveIt setup assistant configures the plugin automatically. For example by adding this line to the launch file as suggested here:

<rosparam command="load" ns="manipulator" file="$(find fanuc_m10ia_support)/config/opw_params_m10ia.yaml"/>

Or as an alternative, the setup assistant could copy the parameters to the kinematics.yaml file.


I also like this idea of @simonschmeisser

I have another idea I would like to throw in here:
While we seem to agree that deriving the parameters directly from URDF might be too difficult/impossible/error-prone ... we could still try to embed the OPW parameters into the URDF. There is support for loading YAML files from xacro, so we could put the parameters in a generic file in config and load this from _macro.xacro. It could then be added as a parameter/child nodes of tool_0 link. The MoveIt kinematics.yaml could then be completely generic again and other consumers like Descartes could either read the yaml/rosparam or be extended to parse the robot_description/urdf as well

Do you think this idea is worth a prototype?

I think this should be the responsibility of the MoveIt setup assistant, and not of the plugin.

If we fixed the way the plugin loads the parameters from the ROS parameter server, we can change as much as we want with the way these parameters end up there...


What do you think? @gavanderhoorn @simonschmeisser
The conversation in #245 was a long one, so I'm not sure I processed al the suggestions. :)

@JeroenDM
Copy link
Contributor Author

Note
I used the recently introduced forward kinematic test to check the parameters and then manually copy-pasted them in this repository.

@gavanderhoorn
Copy link
Member

@JeroenDM: I've added a few commits here.

Would you be able/willing to add the parameters for the remaining models/variants as well?

@gavanderhoorn gavanderhoorn changed the title WIP: add opw kinematics config files Add OPW kinematics parameter files May 19, 2020
@gavanderhoorn
Copy link
Member

I'm going to merge this as soon as Travis tells me CI passes.

Copy link
Contributor

@simonschmeisser simonschmeisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite verbose but I guess that doesn't hurt

@gavanderhoorn
Copy link
Member

Contrary to something like the joint names .yamls, OPW is not a universally known concept in ROS.

So I wanted to make sure users encountering these files would be able to understand what they are for, and how they could be used.

All too often I find configuration settings (in .yamls or directly in .launch files) which have no accompanying documentation or comment whatsoever, which makes it difficult to understand what's going on.

Hence the comment.

@gavanderhoorn
Copy link
Member

Travis is having difficulty reaching Canonical's servers. Bla.

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @JeroenDM 👍

And thanks for making the plugin available.

@JeroenDM
Copy link
Contributor Author

The files still use the old parameter prefix kinematic_solver_, which changed to opw_parameters_.

Would you be able/willing to add the parameters for the remaining models/variants as well?

I would, but not in the coming weeks. I have a little bit too much going on at the moment, sorry...

@JeroenDM
Copy link
Contributor Author

And thanks for making the plugin available.

It was a great way to learn online collaboration and git :)

@gavanderhoorn
Copy link
Member

The files still use the old parameter prefix kinematic_solver_, which changed to opw_parameters_.

Ah, that was close. I literally hovered over the Squash and merge button when your comment appeared.

I should've paid more attention.

I'll fix the keys.

@JeroenDM
Copy link
Contributor Author

Ah, that was close. I literally hovered over the Squash and merge button when your comment appeared.

haha, I was in the middle of writing the comment when I saw your new comment appear and hit the comment button as fast as I could :)

@gavanderhoorn
Copy link
Member

The files still use the old parameter prefix kinematic_solver_, which changed to opw_parameters_.

It was actually kinematics_solver_* to opw_kinematics_*, so your comment confused me.

But 453d974 should fix this.

@JeroenDM
Copy link
Contributor Author

I can quickly test it manually in a ROS workspace if you want, but it looks good to me now. I like the extra comment explaining what it is.

@gavanderhoorn gavanderhoorn merged commit 8e66897 into ros-industrial:indigo-devel May 19, 2020
@gavanderhoorn
Copy link
Member

I can quickly test it manually in a ROS workspace if you want, but it looks good to me now. I like the extra comment explaining what it is.

I've actually done that here locally :)

Which was a good thing, as I discovered I'd used the wrong prefix for the keys .. again ..

@gavanderhoorn
Copy link
Member

Thanks again for the contribution.

Now on to Melodic/Noetic and dropping the IKFast plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants