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

Fix for issue #169: load xacros in MoveIt cfgs instead of urdfs #170

Conversation

gavanderhoorn
Copy link
Member

As per subject.

Makes MoveIt load top-level xacros instead of the urdfs. This should make editing / updating support pkgs a bit more convenient, as the xacro xacro.py .. step is not needed then.

@gavanderhoorn gavanderhoorn force-pushed the issue169_moveit_load_xacros branch from 5fd6779 to d4ebbd5 Compare June 29, 2015 18:47
@shaun-edwards
Copy link
Member

+1

@gavanderhoorn
Copy link
Member Author

@shaun-edwards: I've changed my approach: instead of including support_pkg/load_M.launch in planning_context.launch (a Moveit Setup Assistant generated file), I've now moved that to the demo.launch and moveit_planning_execution.launch files. I think this has less maintenance overhead, as only demo.launch is an auto-generated file, and the change is minor.

planning_context.launch allows us to prevent it from loading the urdf/xacro, so I'm making use of that.

Similar idea, different implementation.

Instead of loading the '.urdf' top-levels, this change makes all MoveIt config
pkgs load the top-level '.xacro'. Even though users are expected to only use
the provided MoveIt config pkgs as examples or for initial exploratory
planning, they should still be easily editable and updateable. By loading the
top-level xacro directly, users avoid having to run the xacro through xacro.py,
allowing them to skip that step.

In addition, this change updates demo.launch and
moveit_planning_execution.launch to use the 'load_*.launch' files provided by
the robot support pkgs. This is more in-line with the recommended use of
launch files in support pkgs, as it uses the 'public api' of the support pkg
(as described in [1]), instead of directly accessing the xacro.

[1] http://wiki.ros.org/Industrial/Tutorials/WorkingWithRosIndustrialRobotSupportPackages
This has less maintenance overhead, as there are no changes to the
auto-generated MoveIt launch files (only to demo.launch and
moveit_planning_execution.launch).
@gavanderhoorn gavanderhoorn force-pushed the issue169_moveit_load_xacros branch from b4642c5 to cda90fb Compare July 3, 2015 15:27
@gavanderhoorn
Copy link
Member Author

Ping?

@shaun-edwards
Copy link
Member

Doesn't moveit allow you to specify a xacro in the setup wizard? And presumably doesn't it auto generate files that use a xacro instead of a urdf?

@gavanderhoorn
Copy link
Member Author

Yes to both questions.

The resulting config pkg then looks exactly like what this PR introduces, minus the inclusion of the load_*.launch files in demo.launch and moveit_planning_execution.launch, and the load_robot_description=false for planning_context.launch.

So first get the pkgs to do what the Setup Assistant would've generated, then suppress loading the xacro directly by having load_*.launch do it instead.

@shaun-edwards
Copy link
Member

Why change demo.launch? What is the benefit if using our _load launch file?

@gavanderhoorn
Copy link
Member Author

@shaun-edwards wrote:

Why change demo.launch?

Because it should not let planning_context.launch load the xacro. That should be done by load_X.launch in the support pkg.

What is the benefit if using our _load launch file?

From Working with ROS-Industrial Robot Support Packages - Using the provided launch files (here):

The load_m10ia.launch file simply uploads the urdf of this specific model to the robot_description parameter on the parameter server. It is recommended that users include this launch file in their own launch files, instead of loading the urdf or calling xacro directly. Not all models may have a simple, or single, urdf or xacro to load, and this launch file hides those details behind a simple interface.

This PR tries to implement that suggestion by making use of the abstraction provided by the load_X.launch files. Admittedly, there are currently no support pkgs that have such complicated xacros that would actually need this, but as the Fanuc pkgs are (sometimes) used as examples for others, I thought it would make sense to do this.

The MoveIt Setup Assistant would still directly load the .xacro though, so that would be something to address (perhaps extending it to also support 'loading' a launch file).

@shaun-edwards
Copy link
Member

I'd like to limit the number of extra steps required after generating a moveit config. It's bad enough that we have to create a moveit_planning_execution.launch file manually. Editing demo.launch is just one more step. Perhaps we could leave demo.launch alone?

@gavanderhoorn
Copy link
Member Author

I'd like to limit the number of extra steps required after generating a moveit config. It's bad enough that we have to create a moveit_planning_execution.launch file manually. Editing demo.launch is just one more step.

Yes, but unfortunately it cannot be avoided if we want the two to be consistent.

Perhaps we could leave demo.launch alone?

See above: I feel it is important that the two are consistent, so it's either/or for me. demo.launch is essentially moveit_planning_execution.launch, but without the 'active' parts (ie: robot driver / industrial simulator nodes) and configured for the simple controllers of MoveIt.

@shaun-edwards
Copy link
Member

+1 to merging this. However, I don't think we can make this a blanket requirement until we have an automatic generation script (either the MoveIt setup assistant or an update to the generation scripts)

@gavanderhoorn
Copy link
Member Author

I'm going to 'mothball' this one for now, at least until I can update the MoveIt setup assistant to also support 'loading' launch files. Otherwise this will still require too much manual editing of files.

@gavanderhoorn
Copy link
Member Author

Closing this one. Replaced by #208, which is somewhat simpler, but doesn't address the duplication.

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.

2 participants