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

Pr2 teleop general for hydro not use move group #13

Conversation

aginika
Copy link

@aginika aginika commented Aug 28, 2014

Don't merge this PR yet.( I need to test on real robot)

This doesn't require to launch move_group.launch.
Instead of running move_group, this node will calculate the ik and fk by itself using moveit kinematics API.
I would like to choose whether #12 or this one.

@UltronDestroyer
Copy link
Contributor

Appreciated @aginika. What's the difference between #12 and #13? Let me know when this is tested on the robot and I'll confirm the PR.

@aginika
Copy link
Author

aginika commented Aug 28, 2014

The difference between #12 and #13 is
whether it call service to move_group to solve ik or calculate ik by the node itself(without calling service).

In #12 , because pr2_teleop_general node will send service to move_group with moveit_msgs/GetPositionIK, we need to run move_group node. This is similar to what pr2_teleop_general do with using pr2_arm_kinematics_node when groovy.
But if we already run move_group node before we start pr2_teleop_general_joystick.launch(I think we should include move_group.launch in this launch), the original move_group will die when we launch it.
If we escape the move_group.launch with roslaunch namespace, we need to remap some topics like /(namespace)/joint_states
(We think these odd changes will make other people in trouble)

In #13, we don't use move_group, so we don't need to include move_group.launch(only we need to include plannning_context.launch), We calcurate with using moveit::core::RobotStates, JointModelGroups and so on.

We think this would be a little better than #12.
There may be more good solution or better way of #12, so please tell me!

I think we could test on real robot on this week. When I finish, I will write here.

@aginika
Copy link
Author

aginika commented Sep 2, 2014

We tested on the real Robot with #12 and #13 and it seems to work.

@UltronDestroyer
Copy link
Contributor

Thank you. If this looks good on the robot, I'll just check and make sure the code changes look right. If I'm understanding correctly, dependency on kinematics_msgs was removed. (Which is in line for our current design as decided in PR2/pr2_kinematics#2).

@UltronDestroyer
Copy link
Contributor

I'm a bit confused. Do you want me to merge #12 or #13? Which one can I close? Or do both need to be merged sequentially?

@aginika
Copy link
Author

aginika commented Sep 2, 2014

If I'm understanding correctly, dependency on kinematics_msgs was removed.

Yes, that's true. In both request #12 #13 , the kinematics_msgs is removed.

@aginika
Copy link
Author

aginika commented Sep 2, 2014

Do you want me to merge #12 or #13? Which one can I close? Or do both need to be merged sequentially?

We want you to merge #13. In case anyone think the using move_group(#12) will be more better than #13 , we send both Pull Request.

Sorry for confusing you. Please merge #13 and close #12 if there are no problems.

@aginika
Copy link
Author

aginika commented Sep 9, 2014

So , will this PR be merged?

@UltronDestroyer
Copy link
Contributor

Sorry Aginika, I had made the changes while this PR existed (and forgot that it existed). I changed the code to use Moveit, like yours did. Looks like we have two solutions moving forward. Mine has been untested on the robot, and yours has, meaning its further ahead than what we have.

I'll walk through the merge tonight and see where the changes differ (I probably missed some things since it hasn't been tested yet)

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.

2 participants