-
Notifications
You must be signed in to change notification settings - Fork 173
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
Conversation
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.
|
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 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. |
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
|
5bd8808
to
5753af7
Compare
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.
If some files are general-purpose, they should be added to the templates.
The |
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. |
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. |
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. |
5753af7
to
07e539e
Compare
There was a problem hiding this 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.
</node> | ||
<node name="joint_state_desired_publisher" pkg="topic_tools" type="relay" args="joint_states joint_states_desired" /> |
There was a problem hiding this comment.
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
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 |
This comment has been minimized.
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
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. |
This comment has been minimized.
This comment has been minimized.
Thanks a lot, @rickstaa, for pushing this forward. A new release of panda_moveit_config is long overdue!
Regarding your questions:
|
@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
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).
Good point I did not yet look into this. I check and track all the changes that need to be performed to the
I'm trying to fix this with @frankaemika. frankaemika/franka_ros#162 is related to this pull request might people want to chip in. |
I just formulated these rules in a common-sense fashion to answer you questions in a generic way. I hope other maintainers agree.
I wouldn't include them by default into |
This comment has been minimized.
This comment has been minimized.
Sure! Even though the planner is failing, the config is probably still fine. |
This comment has been minimized.
This comment has been minimized.
@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 guideFor future reference, the following steps were performed to release the Noetic branch:
Files that were manually createdConfig folder
launch
Moveit_tutorial changesI found the following changes that need to be performed to the moveit_tutorials:
Moveit_setup_assistant changesI found several improvements that can be made to the MSA.
Other upstream problems that still exist
Other things we need to do
|
Big thanks for your work, @rickstaa 🎉 Could you please open a new issue (e.g. titled |
Note that STOMP moved to a separate repo some while ago and the issue was just fixed: ros-industrial/stomp_ros#26 |
@simonschmeisser Thanks a lot for letting me know I will re-test the STOMP planner. |
@simonschmeisser Your right the upstream issue has been solved thanks for letting me know. |
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: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: