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

Update MoveIt Setup Assistant documentation for ROS Humble #647

Merged
merged 54 commits into from May 12, 2023

Conversation

Robotawi
Copy link
Contributor

@Robotawi Robotawi commented Mar 30, 2023

Description

This pull request updates the MoveIt Setup Assistant tutorial to ROS 2 Humble and enhances the documentation. The following changes have been made:

  1. Updated all screenshots to reflect the current state of the Setup Assistant.
  2. Enhanced the explanations for various concepts, including virtual and passive joints, making it easier for users to understand their relevance to the Setup Assistant.
  3. Added descriptions for new items in the updated Setup Assistant, such as ros2_control URDF modification, ROS 2 Controllers, and MoveIt Controllers.
  4. To improve organization, if a single step requires multiple screenshots, they are added in a separate directory
  5. Update the recommended next steps to contain more beginner friendly tutorials/examples.

Please review the changes and let me know if any further modifications are required.

Checklist

  • Required by CI: Code is auto formatted using clang-format(there is no C++ code in this tutorial)
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

Closes #22

@tylerjw
Copy link
Member

tylerjw commented Mar 30, 2023

wow, thank you for doing this. tagging @MarqRazz as he's been recently working through MSA.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

As stated by others, thanks for taking this on! I mostly am commenting with formatting, as I notice a few things:

  1. It will make things easier for code review and further changes if you stick to one sentence per line of code in the .rst file.
  2. The formatting of names of code, planning groups, etc. is inconsistent. Sometimes it's unformatted, sometimes bold, sometimes italic, and sometimes code_font. Please consider passing through and unifying things a little better.

To the above, my suggestion would be:

  • If it's a menu item, button name, etc. use bold font
  • If it's the name of a class (MoveGroupInterface) or planning group (panda_arm) use code font

Also, I see lots of formatting CI failures. You can to install and use pre-commit to check these things locally prior to submitting.

doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

Great job porting this @Robotawi! Suggestions and clarification below...

doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved

Step 3: Add Virtual Joints
--------------------------
Virtual joints are used primarily to attach the robot to the world.
For the Panda, we will define only one virtual joint attaching the *panda_link0*
of the Panda to the *world* world frame. This virtual joint represents the motion of the base of the robot in a plane.
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something here? I know that the moveit_config included in the resources package sets this to a floating joint but what does that do for the user? You can't move the robot in the scene in Rviz so why make it floating? This statement represents the motion of the base of the robot in a plane makes it sound like it can move but the tutorial tells the user to set it to fixed so I suggest we change it to:
This virtual joint represents a fixed link to the world.

In pretty much all of my robot's URDF's I include a world link and leave this virtual joint empty (even though the setup assist and moveit complain when running) but everything works as expected. If someone know's more about how MoveIt uses the virtual joint and its advantages/disadvantages I would love a better explanation here!

Copy link
Contributor Author

@Robotawi Robotawi May 7, 2023

Choose a reason for hiding this comment

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

I am truly thankful for your comment, @MarqRazz!

My statement is wrong. It should be a fixed virtual joint, and it is optional because the Panda arm has a fixed base. I have also used single and multiple arms URDFs without virtual joints and the models worked as expected.

I did some searches and checked older documentations. My conclusion is as follows.

  1. A fixed virtual joint provides fixes a robot base to the world. Here is a relevant documentation.

  2. A planar virtual joint provides 3 DOF and attaches a mobile base to the world (this would be the one to use for mobile manipulators). Here is relevant documentation and a screenshot from Mobile Manipulation workshop by PickNik team members, page 13. It states virtual planar joint is used for modeling the Stretch mobile manipulator, and connecting its base_frame to the odometry frame.

Planar joint for Stretch mobile manipulator

  1. A floating virtual joint provides 6 DOF and would be the one to use if we attach an arm to a quadcopter for example. Here is a relevant issue.

I think the moveit_config in the resources package should use a fixed joint. I updated the tutorial based on this information.

@sea-bass, can you please share with us how MoveIt uses the virtual joint and its advantages/disadvantages?

Thank you!

doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
@sea-bass
Copy link
Contributor

@Robotawi Do you have time to continue working on this PR? There is a lot of interest from users to get this tutorial updated.

If you don't think you'll be able to get to this feedback soon, could you let us know so we can take over the PR and get it merged on our end? Thanks.

Robotawi and others added 2 commits May 5, 2023 10:17
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
@Robotawi
Copy link
Contributor Author

Robotawi commented May 8, 2023

I apologize for my late response. Thank you, @sea-bass and @MarqRazz , for your valuable feedback! I kindly request you to take a look at my responses to the unresolved comments. If everything is okay, please proceed with the review. Your time and efforts are greatly appreciated!

@Robotawi Robotawi requested review from sea-bass and MarqRazz May 8, 2023 13:11
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

I think we're close! Only had minor formatting stuff, except for some comments on the IK solvers, since TRAC-IK is actually not yet available in ROS 2, nor are these optimization-based solvers necessarily faster than KDL. So I think the wording should be more neutral and point to simply "changing away from the default solver".

doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
doc/examples/setup_assistant/setup_assistant_tutorial.rst Outdated Show resolved Hide resolved
Comment on lines 521 to 524
Setup Custom Inverse Kinematics Solvers

* Faster IK solvers than the default KDL solver are available.
For more information, refer to the :doc:`IKFast Kinematics SolverIKFast </doc/examples/ikfast/ikfast_tutorial>` and :doc:`TRAC-IK Kinematics Solver </doc/examples/trac_ik/trac_ik_tutorial>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

As per my previous comment on IK plugins, try use more neutral wording. Specifically,

  • KDL is actually pretty fast already, so switching away from KDL is usually to get better performance especially in the face of constraints. IKFast is the actual faster exception because it can generate analytical solutions, but other solvers that use optimization such as TRAC-IK, Bio-IK, and pick_ik are actually slower.
  • Also, TRAC-IK is not yet available in ROS 2 so I would advise against remove it and point to pick_ik instead if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

trac_ik and bio_ik are not slower than KDL! See https://bitbucket.org/traclabs/trac_ik/src/master/

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow, that's surprising considering TRAC-IK is literally KDL + an optimization based solver in parallel. I guess the authors made some improvements to the Orocos KDL package...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TRAC-IK is an enhancement of KDL + an optimization based solver. This enhancement is referred to as KDL-RR (Random Restarts) by the authors. Unlike the default KDL solver, KDL-RR uses a time-based run loop instead of a fixed number of iterations. Additionally, it utilizes random restarts (seeds) to overcome local minima when detected.

@sea-bass, I updated the tutorial to use neutral wording when alternative IK solvers are mentioned, and took a note this point may need reconsideration after more IK plugins are available.

Robotawi and others added 2 commits May 10, 2023 23:32
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

This is super close, I build the web page locally and it looks great! After you address @sea-bass's changes and consider some of my suggestions I think this is good to go.

@Robotawi
Copy link
Contributor Author

I sincerely appreciate your feedback, @sea-bass and @MarqRazz! I would like to request another review of the changes made based on your suggestions.

Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

@Robotawi thanks again for getting this tutorial updated!

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

I think there are just some small formatting issues that failed CI (see here), but otherwise looks great to me.

Thanks @Robotawi for going through this tutorial and addressing our review. I look forward to your continued work on IK solvers for GSoC :)

@Robotawi
Copy link
Contributor Author

The formatting issue is fixed. Thanks again for your feedback! It is what improved the tutorial.

I am glad about this contribution and look forward to continuing working closely with you to make the IK solvers project a success!

@Robotawi Robotawi requested a review from sea-bass May 12, 2023 16:05
@sea-bass sea-bass merged commit 406ba62 into moveit:main May 12, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port "MoveIt Setup Assistant" tutorial
5 participants