Do not allow traj execution from PlanningComponent

[AndyZe]

Merge with moveit/moveit2_tutorials#563

MoveItCpp and PlanningComponent have very similar execute() functions. One does not call the other. Here, I advocate for deleting the PlanningComponent version because I don't think something focused on planning should handle execution. If you like this, there are some small updates to make in moveit_tutorials but that is the only effect I foresee.

bool PlanningComponent::execute(bool blocking)

MoveItCpp::execute(const std::string& group_name, const robot_trajectory::RobotTrajectoryPtr& robot_trajectory,
                   bool blocking)

If you do NOT like this PR, then can we flip it to delete the MoveItCpp version of execute()? AFAIK it is not used anywhere

[codecov]

Codecov Report

Base: 50.45% // Head: 50.42% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (17e2757) compared to base (c6733bd).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
- Coverage   50.45%   50.42%   -0.03%     
==========================================
  Files         374      374              
  Lines       31335    31335              
==========================================
- Hits        15807    15797      -10     
- Misses      15528    15538      +10     

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp	65.97% <0.00%> (-1.45%)	⬇️
moveit_ros/moveit_servo/src/pose_tracking.cpp	77.11% <0.00%> (-0.46%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[henningkayser]

But, one does call the other? The intention was to make it easier to forward the last plan solution for execution, similar to how MGI is doing it. I agree though, that we should keep our API clean and that removing it would improve separation of concerns.

[sjahr]

I like this change too 👍
The API diff looks something like this, I guess:

-planning_component.plan();
+auto motion_plan_response = planning_component.plan();
-planning_component.execute();
+planning_component(group_name, motion_plan_reponse.trajectory)

I think the new version is more readable.

Wouldn't it be possible to drop the group name argument for MoveItCpp::execute(...) as well, since the robot_trajectory::RobotTrajectoryPtr in MotionPlanResponse contains a group name?

[henningkayser]

Can you first deprecate this function before removing it without notice?

[AndyZe]

Wouldn't it be possible to drop the group name argument for MoveItCpp::execute(...) as well, since the robot_trajectory::RobotTrajectoryPtr in MotionPlanResponse contains a group name?

Ooh, I like that idea 👍

[mergify]

This pull request is in conflict. Could you fix it @AndyZe?

[AndyZe]

I'll make a moveit2_tutorials PR that should be merged simultaneously with this one. Here it is: moveit/moveit2_tutorials#563

[sjahr]

@henningkayser @AndyZe Are we ready to merge this?

[AndyZe]

Yeah, it's ready as far as I know.