moveit / panda_moveit_config

The Panda robot is the flagship MoveIt integration robot
http://docs.ros.org/kinetic/api/moveit_tutorials/html/
103 stars 171 forks source link

WIP: Generated using the latest features from MoveIt #74

Closed tylerjw closed 2 years ago

tylerjw commented 3 years ago

Description

This was created by generating using the master branch of MoveIt. There are a handful of files that were not generated (i.e. config/panda_arm_hand.srdf.xacro), I'd like to document how they are created and what their use is in the README.

Lastly, I chose two new poses that are as close to the original ones as I could make and stay out of collision. I didn't include the transport pose because I was not able to make that pose and stay out of collision. I've included images of each pose here: panda_extended panda_ready panda_transport

Lastly, this should be tested with moveit_tutorials to make sure it doesn't break any of those.

Notes to reviewers

Here are the things that need to be done before this is ready:

v4hn commented 3 years ago

The transport pose is the configuration used to ship the robot in its original box. That's the main reason why it was part of the configuration.

Yes, it's in collision with the current model and this was discussed somewhere before. The MoveIt-compatible trajectory controller can't move the robot to this pose because the internal self-collision check will fail as well. Apparently they have a separate internal routine to move to this pose.

tylerjw commented 3 years ago

I checked through the commits since the last release and this only backs out one change. There is a PR to add the transport pose to one of the files. This specific file I think we can safely remove, this I explain below.

This brings us to a point of discussion. There are two sorts of files in this repo that are not generated by moveit_setup_assistant. The first is xacro files that were copied versions of the old robot description. I believe these can be safely removed.

The next are files that support specific tutorials and were manually added here. An example of that is the lerp_planning_pipeline.launch.xml and related changes. I'm not sure what the right approach is for these sorts of files. I think it makes sense to document what these files are so that in the future if someone needs to generate this package to update it they know what to do about those files.

That or we should move these launch and config files out of this repo and into the tutorials repo to go alongside their respective tutorials. A third option would be to extend the setup assistant to generate these files if they are actually general-purpose launch files that would be useful for other robot setups.

@rhaschke @v4hn do you have thoughts on how we should best maintain these manually added files? I am working through identifying them and trying to validate the functionality of this PR with tutorials.

tylerjw commented 3 years ago

Here is a list of the tutorials on master of moveit_tutorials with my notes. I'll add notes as I work through each one comparing it to the current version of panda_moveit_config.

Generated then modified files

Custom Files added for Tutorials

Modifications needed for tutorials

v4hn commented 3 years ago

The first is xacro files that were copied versions of the old robot description. I believe these can be safely removed.

This is not the "old robot description", but the srdf file that is split in multiple sub-files. If you drop the ability to load the arm without gripper from the updated package, they should be removed. Some users might be interested in using the arm with other endeffectors, but they should usually generate their own moveit config anyway. So unless the distinction is required for some tutorial, I assent to revert to a single srdf file.

A third option would be to extend the setup assistant to generate these files if they are actually general-purpose launch files that would be useful for other robot setups.

If some files are general-purpose, they should be added to the templates. The LERP planner is created in the tutorial though, so I would just keep it in the panda config with an appropriate comment at the top of the file.

That or we should move these launch and config files out of this repo and into the tutorials repo to go alongside their respective tutorials.

The planning_pipeline "plugin-like" structure does not allow for this and we discussed some time ago in some issue whether we want to extend it to do so. At least my personal opinion was that it does not make sense to add plugin mechanisms to launch files and people can directly include them in their own launch files. However, I'm not sure how much overhead that would create for the tutorial's launch file, which should definitely be as simple as possible to support new users.

tylerjw commented 3 years ago

This is not the "old robot description", but the srdf file that is split in multiple sub-files. If you drop the ability to load the arm without gripper from the updated package, they should be removed. Some users might be interested in using the arm with other endeffectors, but they should usually generate their own moveit config anyway. So unless the distinction is required for some tutorial, I assent to revert to a single srdf file.

I believe this functionality is now provided by the description files in the upstream franka package. From what I could tell our versions of these do not differ in functionality from the official ones now. I have yet to find a tutorial that used them, or tested loading without the gripper using the upstream description files.

tylerjw commented 3 years ago

Today I went through and investigates some of the easier to test tutorials. I'll need to work through the rest of them but unfortunatly I found some that don't work with the old config and the master branch of MoveIt on Noetic. Some of these might be easy fixes but I'd don't believe they should block this change and should hopefully be unaffected by this change.

v4hn commented 3 years ago

This is not the "old robot description", but the srdf file that is split in multiple sub-files.

I believe this functionality is now provided by the description files in the upstream franka package. From what I could tell our versions of these do not differ in functionality from the official ones now. I have yet to find a tutorial that used them, or tested loading without the gripper using the upstream description files.

Are we talking about the same files? hand.xacro in this repository provides SRDF specifications, hand.xacro in franka_ros provides URDF specs. These files do not have anything in common aside from their name.

tylerjw commented 3 years ago

Are we talking about the same files? hand.xacro in this repository provides SRDF specifications, hand.xacro in franka_ros provides URDF specs. These files do not have anything in common aside from their name.

I didn't realize this. I figured they provided the same utility. My reasoning was based on searching for files that used these various files in the panda_moveit_config repo and in all cases it seemed like they were referring to the one in franka_description. I'll put those back.

JafarAbdi commented 3 years ago

Note that I disabled the collision between panda_link8 & panda_link6 + panda_link7 otherwise hand movement is very restricted causing some of the tutorials to not work

davetcoleman commented 3 years ago

This seems overdue for a merge, @tylerjw can we push this in?

tylerjw commented 3 years ago

@davetcoleman I abandoned this because I didn't have the time to finish it. There are a handful of tutorials it needs to be tested with and either this or the tutorials need to be fixed. I've left documentation of all of that above.

davetcoleman commented 3 years ago

@tylerjw by "abandoned" you mean set it aside, correct? You would support someone else taking this on, as its still important right?

tylerjw commented 3 years ago

I mean that I stopped working on it. I have no issue with someone else taking it up. I think that person will need to have a good understanding of the uses of this package (probably more than me) and all the tutorials or they risk getting stuck just as I did.

rickstaa commented 3 years ago

@tylerjw First of all thanks a lot for creating this pull request. @davetcoleman I'm happy to help. I made some changes to ensure that the panda_control_moveit_rviz.launch file can be used again to control the real robot using move it. In addition, I created a pull request on your branch https://github.com/tylerjw/panda_moveit_config/pull/1. I will create additional pull requests, if I run into more errors while using the tylerjw:p/update_latest branch.

rickstaa commented 3 years ago

@tylerjw I updated the robot_description xacro url since the upstream was changed (see https://github.com/tylerjw/panda_moveit_config/pull/3).

rickstaa commented 3 years ago

@tylerjw Thanks for merging the last pull request I created a new pull request with two fixes that are necessary (see https://github.com/tylerjw/panda_moveit_config/pull/6). Further, there is a dependency pull request in https://github.com/tylerjw/panda_moveit_config/pull/4. I will now try to debug why the controllers are not working when launching the franka_example_controllers.move_start.launch script.

rickstaa commented 3 years ago

I created https://github.com/frankaemika/franka_ros/issues/162 to make the Franka Emika team aware of the restrictive hand movement problem. @JafarAbdi please free to add any points I missed or misunderstood in your original explanation to the issue.

rickstaa commented 3 years ago

Last week I gave this feature another try while keeping the points that @tylerjw mentioned in https://github.com/ros-planning/panda_moveit_config/pull/74#issuecomment-703129249 in mind.

To do this, I first created an orphan branch and created the panda_moveit_config again using the latest moveit_setup_assistant. I did this both to get all the latest changes from the moveit_setup_assistant into this repository and to get a sense for which features were missing in the MSA. I then manually compared the new files with those in the melodic-devel branch keeping notes and create pull request where needed. The result of this can be found in this pull request.

https://github.com/rickstaa/panda_moveit_config/pull/26 is nearly ready to be merged onto #74, but I need some help applying some last pull requests (see the sections below).

Pull requests that first need to be reviewed

Below are some pull request that I did not yet merge since they first need to be reviewed by the @ros-planning team.

Upstream pull requests that need to be merged

The pull requests below were created on the frankaemika/franka_ros repository and need to be merged before the Noetic version of the ros-planning/panda_moveit_config can be released.

rhaschke commented 2 years ago

Thanks a lot, @rickstaa, for pushing this forward. A new release of panda_moveit_config is long overdue! I had a look at your branch https://github.com/rickstaa/panda_moveit_config/pull/26 and cleaned it a little bit, following these rules (see here for the result):

Regarding your questions:

rickstaa commented 2 years ago

@rhaschke Thanks a lot for helping me implement this new release.

Thanks for rebasing my commits they look a lot cleaner! I will use https://github.com/rhaschke/panda_moveit_config/tree/noetic-devel-rickstaa and adhere to these rules moving forward. Are these rules written down somewhere in a contributing.md. Or are they requested when people make a pull request?

I suggest including examples for other planning pipelines even though these pipelines are currently not operational or released. Otherwise, it will become hard to re-add those files later from scratch.

Are you okay with adding them commented out since otherwise, they throw errors (see https://github.com/rickstaa/panda_moveit_config/blob/f4e1c046d7ca88d9e06f0969ed81cfda62fb2d0b/launch/move_group.launch#L64-L70).

For the Gazebo simulation, I suggest to use franka_gazebo if that's closer to the real robot. Does that require changes in the tutorials as well?

Good point I did not yet look into this. I check and track all the changes that need to be performed to the moveit_tutorials in https://github.com/rickstaa/panda_moveit_config/pull/26.

@Le-Li-17 please have a look at the long list of PRs filed to Franka. We strongly believe that the URDF model released with version 0.8.1 still has severe issues strongly limiting the motion range of the simulated robot. See Update SRDF for collision model #35 (review)

I'm trying to fix this with @frankaemika. https://github.com/frankaemika/franka_ros/issues/162 is related to this pull request might people want to chip in.

rhaschke commented 2 years ago

I will adhere to these rules moving forward. Are these rules written down somewhere in a contributing.md?

I just formulated these rules in a common-sense fashion to answer you questions in a generic way. I hope other maintainers agree.

I suggest including examples for other planning pipelines even though these pipelines are currently not operational.

Are you okay with adding them commented out since otherwise, they throw errors.

I wouldn't include them by default into move_group.launch, but consider them as optional custom pipelines to be included manually via the pipeline argument.

rickstaa commented 2 years ago

@rhaschke I think we can merge the changes in https://github.com/rickstaa/panda_moveit_config/pull/29 (STOMP algorithm) into https://github.com/rhaschke/panda_moveit_config/tree/noetic-devel-rickstaa even though there appears to be an unsolved upstream issue?

rhaschke commented 2 years ago

Sure! Even though the planner is failing, the config is probably still fine.

rickstaa commented 2 years ago

@rhaschke Just to double-check. The CHOMP and LERP planners are not supposed to work with the panda hand Parallel Jaw gripper?

rickstaa commented 2 years ago

@rhaschke I think I just created the last pull request needed to release the Noetic branch (i.e. https://github.com/rhaschke/panda_moveit_config/pull/8). Just to be sure, tomorrow I will perform some tests on the real robot to see if https://github.com/rickstaa/panda_moveit_config/pull/54 is still needed in ROS Noetic. After that, the Noetic branch is ready to be released when the following upstream PR are merged:

Migration guide

For future reference, the following steps were performed to release the Noetic branch:

  1. I created the initial panda robot configuration using the moveit_setup_assistant (see 8566b70f5e464e6fcb1c7d5664ff6e6e5202876e). In this branch, tree/82378877879f7d40727819b1c3345eeb4308d98b was used. Similar to https://github.com/ros-planning/panda_moveit_config/pull/74 I changed some posed to prevent self collisions.

  2. I modified the panda.srdf file that is created by the moveit_setup_assistant such that we can programmatically enable the gripper (see f0c2bca91d14a5b4cd7192c189d253f4d1fb6407). This needs to be done, since the moveit_setup_assistant currently doesn't propagate xacro arguments you supply to the urdf.xacro file (see this issue). We therefore need to manually add a way to enable or disable the gripper. This can be done by creating a panda.srdf.xacro file.

  3. I replaced the sensor_3d.yaml with the example files in the perception_pipeline tutorial. Also edit the sensor_manager.launch.xml file to point to the right configuration files (see 2aaa01d65491d02ef6c095f7bc7b13a8f52d1be6).

  4. I added the rviz_tutorial flag. While doing this, I made sure that I copied the moveit.rviz and empty.rviz files from the previous ROS version. Additionally, I added the moveit_visual_tools package as a dependency (see 1d55c9851bf388e3e2c29d7ed7390c8edce32f22).

  5. Similar to df0cbd8 I had to disable the collision checking between joint{5, 6} and joint8 since the new collision geometries that were added to https://github.com/frankaemika/franka_ros/releases/tag/0.8.0 are to coarse (see aa24fae70f4a62ec63fc72ecbb0aacdb4283738e). This should be fixed upstream by, for example, by improving the collision geometries or by giving users the ability to use meshes instead (https://github.com/frankaemika/franka_ros/pull/154).

  6. I added the panda_control_moveit_rviz.launch which can be used to control the real robot (see 36d7c9f3b20f5a906b256313f3bbc50145ded820).

  7. I added the TrajOpt files that were found in the melodic-devel branch to the repository ( see bc2bbc75620aaf01eef092fcd7301c22bef4f253 and 88fb27864bfea0f3056a1d1a39771495583320b6).

  8. I added the STOMP files that were found in the melodic-devel branch to the repository ( see 133d524ecd893180024c1d193b49b010f52c96ef) .

  9. I updated the join_limits.yaml file that was created by the MSA to enforce jerk limits. This was done by adding acceleration limits, since MoveIt does not yet support jerk limits (see 89d9b44cdddaca4746111795c8483e2370da5d34).

  10. I added the files of the LERP planner that is created in the creating-moveit-plugins tutorial (see 85756cd3d16c6d520274d40018bacc4c1779b33b). These files were copied from the melodic-devel branch.

  11. I added the demo_chomp.launch and ompl-chomp_planning_pipeline.launch.xml files needed by the chomp_planner_tutorial tutorial. I also updated the chomp config file to the one found in the melodic-devel branch (see 4c390ac0b2370aa5015e1f84c74a2818deaf9330 and c2ccca2d3f043520cfd948b7033737329b7281d5).

  12. I added the demo_gazebo.launch launch file which spawns a simulated Panda robot in Gazebo (see c46c81c8a1e0da1288d8ef4462b8daa10df37213). This launch file uses the new franka_gazebo package that was introduced in franka_ros v0.8.1.

Files that were manually created

Config folder

launch

Moveit_tutorial changes

I found the following changes that need to be performed to the moveit_tutorials:

Moveit_setup_assistant changes

I found several improvements that can be made to the MSA.

Other upstream problems that still exist

Other things we need to do

rhaschke commented 2 years ago

Big thanks for your work, @rickstaa :tada: I have pushed the branch to https://github.com/ros-planning/panda_moveit_config/tree/noetic-devel

Could you please open a new issue (e.g. titled Noetic release) with all the info in https://github.com/ros-planning/panda_moveit_config/pull/74#issuecomment-947646655? Closing here.

simonschmeisser commented 2 years ago

Note that STOMP moved to a separate repo some while ago and the issue was just fixed: https://github.com/ros-industrial/stomp_ros/pull/26

rickstaa commented 2 years ago

@rhaschke I created https://github.com/ros-planning/panda_moveit_config/issues/89. Feel free to request changes to the TODOS that are in there.

rickstaa commented 2 years ago

@simonschmeisser Thanks a lot for letting me know I will re-test the STOMP planner.

rickstaa commented 2 years ago

@simonschmeisser Your right the upstream issue has been solved thanks for letting me know.