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

Tutorial for running multiple groups/arms. #621

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ur10
Copy link

@ur10 ur10 commented Apr 6, 2021

Description

Added a short tutorial for running multiple groups/arms.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@welcome
Copy link

welcome bot commented Apr 6, 2021

Thanks for helping in improving MoveIt and open source robotics!

@ur10 ur10 changed the title Tutorial for running multiple groups/arms #465 Tutorial for running multiple groups/arms (#465). Apr 6, 2021
@ur10 ur10 changed the title Tutorial for running multiple groups/arms (#465). Tutorial for running multiple groups/arms. Apr 6, 2021
@tylerjw tylerjw requested a review from MarqRazz April 8, 2021 21:47
Copy link

@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.

Thanks for adding the tutorial @ur10!

For the new tutorial to appear on the web page you need to add it to the index.rst file by adding doc/multi_group/multi_group_tutorial after line 53. This is also why the first CI test is failing.

The other test is failing because of #613

@ur10
Copy link
Author

ur10 commented Apr 9, 2021

Thanks for the review! I will make the necessary changes.

Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

Thanks for this. It's a great start towards more complex tutorials.

I think this is very helpful, but it should have more detail and example code. Especially setting goals for groups with multiple end effectors is not very intuitive. There are some examples here. It might be worth linking to the repo in general (although I'm biased) and the Applications page.

To make this tutorial really good and useful, while writing please imagine a user who knows basically nothing and who wants to execute it. Do they need to add or set something on their own? Do they have to think before being able to see something move? Are you suppling everything that they might ask themselves or might get stuck on? Will they know where to look? Finally, is the code reasonably boilerplate-like so it can be a reference? All of that will be a plus.

doc/multi_group/multi_group_tutorial.rst Outdated Show resolved Hide resolved
doc/multi_group/multi_group_tutorial.rst Outdated Show resolved Hide resolved
Moving multiple arms asynchronously
--------------------------------------

As of now, there is no accepted method provided by moveit to move multiple arms asynchronously. For more details kindly refer to this `issue <https://github.com/ros-planning/moveit/issues/2287>`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not quite right. I described a workaround and the conditions under which it can be used here, and jrgnicho mentioned another one in the thread. "no accepted method" is also too vague.

Copy link
Member

Choose a reason for hiding this comment

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

I believe he refactored this well

--------------------------------------

Currently we can not plan for two separate trajectories for two different arms and execute them separately using the ``MoveGroupInterface``.
The workaround to this is to take the planned trajectories for left and right arm and send them as a goal to the action server of the robot arm .The problem however,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The workaround to this is to take the planned trajectories for left and right arm and send them as a goal to the action server of the robot arm .The problem however,
The workaround to this is to take the planned trajectories for left and right arm and send them as a goal to the action server of the robot arm. The problem however,

Copy link
Contributor

Choose a reason for hiding this comment

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

@ur10 No offense, but if you could go through this with a spell checker, that would help a lot.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, ok. I will do a quick check.

Moving multiple arms asynchronously
--------------------------------------

As of now, there is no accepted method provided by moveit to move multiple arms asynchronously. For more details kindly refer to this `issue <https://github.com/ros-planning/moveit/issues/2287>`_.
Copy link
Member

Choose a reason for hiding this comment

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

I believe he refactored this well

Copy link
Member

@davetcoleman davetcoleman 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 a really great contribution to the tutorials!!

spinner.start();

std::string movegroup_name, ee1_link, ee2_link;
int counter = 1;
Copy link
Member

Choose a reason for hiding this comment

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

add comment about what this is for

Copy link
Author

Choose a reason for hiding this comment

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

Got it. I will make the changes

nh.param<std::string>("ee_link1", ee1_link, "panda_1_link8" );
nh.param<std::string>("ee_link2", ee2_link, "panda_2_link8" );

// We choose the rate at which this node should run. For our case it's 0.2 Hz(5 seconds)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We choose the rate at which this node should run. For our case it's 0.2 Hz(5 seconds)
// We choose the rate at which this node should run. For our case it's 0.2 Hz (5 seconds)

ROS_WARN("Something went wrong moving the robots to home position.");
}

ros::Duration(2.0).sleep(); // 2 seconds
Copy link
Member

Choose a reason for hiding this comment

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

Are sleep timers this long really necessary?

Copy link
Author

Choose a reason for hiding this comment

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

No. A 1-second sleep should suffice.

loop_rate_->sleep();
}

counter = (counter +1 ) % 2;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
counter = (counter +1 ) % 2;
counter = (counter++) % 2;

Slightly fancier?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this seems better. :)

Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

Just some short comments.

target1.pose.position.x = 0.8;
target1.pose.position.y = 0.3;
target1.pose.position.z = 0.7;
target1.pose.orientation.w = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use tf::createQuaternionMsgFromRollPitchYaw to make this meaningful and shorter

Comment on lines +12 to +13
But before starting with multiple arms, the concept of planning groups, there usage and significance of multiple planning groups for multiple end effectors
needs to be understood clearly. kindly go through `this <https://answers.ros.org/question/321843/how-is-moveit-planning-group-supposed-to-be-used/>`_ answer if you are a beginner/new to these concepts.
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the comment, please use a spell checker to fix grammar etc. throughout the text


Robot model used
----------------
This tutorial will use the two panda arms for demonstration. The urdf of the two panda arms can be seen `here <https://github.com/frankaemika/franka_ros/blob/kinetic-devel/franka_description/robots/dual_panda_example.urdf.xacro>`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

URDF

Moving the two arms in sync
-----------------------------

First of all define the individual arms and their end-effectors in respective groups as shown -
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
First of all define the individual arms and their end-effectors in respective groups as shown -
First of all define the individual arms and their end-effectors in respective groups as shown below:

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the other occurrences of this hyphen

.. image:: end_effectors.png
:width: 800px

Then combine all the planning groups into a common planning group, in this case - "both_bots" in the srdf file : ::
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Then combine all the planning groups into a common planning group, in this case - "both_bots" in the srdf file : ::
Then combine all the planning groups into a common planning group, in this case - "both_bots" in the srdf file: ::

No spaces before colons

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.

4 participants