search

PR2 / pr2_apps

15 stars 24 forks source link

Pr2 teleop general for hydro not use move group #13

Closed aginika closed 9 years ago

aginika commented 10 years ago

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.

TheDash commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

TheDash commented 10 years ago

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 https://github.com/PR2/pr2_kinematics/issues/2).

TheDash commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

So , will this PR be merged?

TheDash commented 10 years ago

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)