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

WIP: Generated using the latest features from MoveIt #74

Closed
wants to merge 11 commits into from

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Oct 1, 2020

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:

  • Identify and document changes made after generating from MSA
  • Discuss new poses
  • Test with moveit_tutorials

@tylerjw tylerjw requested a review from rhaschke October 1, 2020 22:30
@v4hn
Copy link

v4hn commented Oct 2, 2020 via email

@tylerjw tylerjw self-assigned this Oct 2, 2020
@tylerjw tylerjw added the help wanted Extra attention is needed label Oct 2, 2020
@tylerjw tylerjw linked an issue Oct 2, 2020 that may be closed by this pull request
6 tasks
@tylerjw
Copy link
Member Author

tylerjw commented Oct 3, 2020

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
Copy link
Member Author

tylerjw commented Oct 3, 2020

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.

  • Getting Started
  • MoveIt Quickstart in RViz
  • Move Group C++ Interface
    • The ready state is slightly different causing some of the paths to be changed slightly, but overall this tutorial works. After this is submitted the images and video should be updated.
    • Noticed the generated rviz file doesn't have the rviz_visual_tools gui panel. I'm going to put that file back and note it as not a generated file.
  • Move Group Python Interface
    • Python3 migration is needed to run this on Noetic.
    • Tutorial works the same with the new and old configuration.
    • The position of the box using both the new and old configuration is odd.
    • offset_box_old
  • MoveIt Commander Scripting
  • Robot Model and Robot State
  • Planning Scene
  • Planning Scene Monitor
  • Planning Scene ROS API
    • Object spawns inside the gripper. See image.
    • The last step "Remove the object from the collision world" doesn't seem to work as the object is not removed. This seemingly broken behavior exists with the old config too.
    • planning_scene_api_tutorial_collision
  • Motion Planning API
  • Motion Planning Pipeline
  • Creating MoveIt Plugins
  • Visualizing Collisions
    • This one is currently broken on the current setup.
  • Time Parameterization
  • Planning with approximated Constraint Manifolds
  • Pick and Place
    • This works as-is but generates a much crazier path because of the new collision model for the robot.
  • MoveIt Grasps
  • MoveIt Task Constructor
  • MoveIt Deep Grasps
  • Subframes
    • This is broken with the old configuration with master of MoveIt on Noetic. The object spawns floating in space between the grippers. The grippers should probably be closed to "hold" the cylinder but that is probably not the primary cause of the issues with this tutorial. Each of the steps fails to plan, I'm not sure why. @felixvd have you seen this?
    • subframes_tutorial
  • MoveItCpp Tutorial
  • MoveIt Setup Assistant
  • URDF and SRDF
  • Low Level Controllers
  • Preception Pipeline Tutorial
  • IKFast Kinematics Solver
  • TRACK-IK Kinematics Solver
  • Kinematics Configuration
  • Custom Constraint Samplers
  • OMPL Planner
  • CHOMP Planner
  • STOMP Planner
  • TrajOpt Planner
  • Planning Adapter Tutorials
  • Joystick Control Teleoperation
  • Realtime Arm Servoing
  • Benchmarking
  • Integration/Unit Tests

Generated then modified files

  • launch/moveit.rviz - restored to state on melodic-devel

Custom Files added for Tutorials

  • placeholder

Modifications needed for tutorials

  • Update video and images for "Move Group C++ Interface"
  • Fix the position of the box in the "Move Group Python Interface" so it is in between the fingers.
  • Update MSA tutorial to include instructions for the default positions of the end effector.
  • Clarify the "Example" section of "Benchmarking" to include instructions for the collision scene objects.
  • Add instructions to "Benchmarking" for building warehouse_ros_mongo from source until it is released to Noetic.
  • Add note to tutorials README.md that rosdoc_lite is needed to run the build_locally.sh script.
  • Fix "Planning Scene ROS API" tutorial to spawn the object in the gripper and remove it from the world in the last step.
  • Fix "Visualizing Collisions"
  • Fix "Subframes" tutorial

@v4hn
Copy link

v4hn commented Oct 4, 2020

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
Copy link
Member Author

tylerjw commented Oct 4, 2020

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
Copy link
Member Author

tylerjw commented Oct 4, 2020

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
Copy link

v4hn commented Oct 5, 2020

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
Copy link
Member Author

tylerjw commented Oct 5, 2020

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.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Overall, I really think it makes a lot of sense to keep configs up-to-date with the MSA, imo it's just strainge if this is not possible. I'm just not sure what setups we would break by doing this. Of course we should keep modifications like with the srdf files and default poses consistent.

config/panda_arm.xacro Outdated Show resolved Hide resolved
</node>
<node name="joint_state_desired_publisher" pkg="topic_tools" type="relay" args="joint_states joint_states_desired" />
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should really remove this

@JafarAbdi
Copy link
Member

JafarAbdi commented Dec 18, 2020

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

@rickstaa

This comment has been minimized.

This commit makes suer that the 'moveit_simple_control_manager' is added
as a dependency. This dependency is needed for running the move_group
script with a real robot.
* Adds rviz_tutorial argument

* Fixes virtual frame world not found warning
@rickstaa
Copy link
Contributor

I created frankaemika/franka_ros#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

This comment has been minimized.

@rhaschke
Copy link
Contributor

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 rickstaa#26 and cleaned it a little bit, following these rules (see here for the result):

  • commit messages shouldn't include PR IDs from your fork
  • fixups due to changes in MSA behavior were squashed directly into the base commit

Regarding your questions:

  • 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.
  • 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?
  • If the existing files differ to the MSA-generated ones: Keep the generated ones, if generic files (e.g. planning pipeline settings) are concerned. But keep robot-specific settings like joint limits etc. For those, MSA can only use the defaults from the URDF.
  • @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)

@rickstaa
Copy link
Contributor

@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 rickstaa#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. frankaemika/franka_ros#162 is related to this pull request might people want to chip in.

@rhaschke
Copy link
Contributor

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

This comment has been minimized.

@rhaschke
Copy link
Contributor

rhaschke commented Oct 4, 2021

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

@rickstaa

This comment has been minimized.

@rickstaa
Copy link
Contributor

rickstaa commented Oct 20, 2021

@rhaschke I think I just created the last pull request needed to release the Noetic branch (i.e. rhaschke#8). Just to be sure, tomorrow I will perform some tests on the real robot to see if rickstaa#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 8566b70). In this branch, tree/82378877879f7d40727819b1c3345eeb4308d98b was used. Similar to WIP: Generated using the latest features from MoveIt #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 f0c2bca). 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 2aaa01d).

  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 1d55c98).

  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 aa24fae). This should be fixed upstream by, for example, by improving the collision geometries or by giving users the ability to use meshes instead (Adds 'use_collision_mesh' arg to the robot description frankaemika/franka_ros#154).

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

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

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

  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 89d9b44).

  10. I added the files of the LERP planner that is created in the creating-moveit-plugins tutorial (see 85756cd). 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 4c390ac and c2ccca2).

  12. I added the demo_gazebo.launch launch file which spawns a simulated Panda robot in Gazebo (see c46c81c). 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

  • panda.srdf.xacro
  • hand.xacro
  • panda_arm.xacro
  • lerp_planning.yaml
  • stomp_planning.yaml
  • trajopt_planning.yaml
  • panda_controllers.yaml
  • panda_gripper_controllers.yaml

launch

  • demo_stomp.launch (Was not present in the melodic-devel branch).
  • demo_chomp.launch (Not used in the moveit_tutorials).
  • demo_lerp.launch (Used in the moveit_tutorials).
  • lerp_planning_pipeline.launch.xml
  • ompl-chomp_planning_pipeline.launch.xml
  • panda_gripper_moveit_controller_manager.launch.xml
  • run_benchmark_trajopt.launch
  • stomp_planning_pipeline.launch.xml
  • trajopt_planning_pipeline.launch.xml

Moveit_tutorial changes

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

  • setup_assistant_tutorial.html:
    • Should have updated pictures (i.e., menu order was changed).
    • Panda description file was changed to be /franka_description/robots/panda.urdf.xacro the hand is now enabled by using the hand:=true xacro parameter.
    • I think the documentation should mention the panda_finger_joint2 as the passive joint.
    • The virtual joint doesn't appear to be needed in the panda_arm group. Maybe remove it as it will cause errors when using a gazebo simulation?
  • Maybe the [panda_control_moveit_rviz.launch] file should be documented in the documentation.
  • The demo_chomp.launch file was added to the panda_moveit_config repository, but this file is not used in the moveit_tutorials.
  • The lerp planner documentation talks about a lerp_example.launch file but this file is not included I think this should be changed to refer the demo_lerp.launch file.

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

  • We need to re-test all the moveit_tutorials with the new Noetic branch.

@rhaschke
Copy link
Contributor

Big thanks for your work, @rickstaa 🎉
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 #74 (comment)?
Closing here.

@rhaschke rhaschke closed this Oct 20, 2021
@simonschmeisser
Copy link

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

@rickstaa rickstaa mentioned this pull request Oct 21, 2021
26 tasks
@rickstaa
Copy link
Contributor

@rhaschke I created #89. Feel free to request changes to the TODOS that are in there.

@rickstaa
Copy link
Contributor

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

@rickstaa
Copy link
Contributor

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

This was referenced Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Noetic Release (0.8.0)
9 participants