Add capability of execute known trajectory with a ROS action by wkentaro · Pull Request #60 · moveit/moveit

wkentaro · 2016-08-17T04:09:34Z

Moved from moveit/moveit_ros#719
Depends on moveit/moveit_msgs#24

gavanderhoorn · 2016-08-17T07:25:53Z

moveit_ros/move_group/src/default_capabilities/execute_path_action_capability.cpp

+  {
+    std::string response = "Cannot execute trajectory since ~allow_trajectory_execution was set to false";
+    action_res.error_code.val = moveit_msgs::MoveItErrorCodes::CONTROL_FAILED;
+    execute_action_server_->setAborted(action_res, response);


Is the lack of a return here on purpose?

Thanks! I fixed it.

davetcoleman · 2016-08-17T15:28:32Z

moveit_ros/move_group/src/default_capabilities/execute_path_action_capability.cpp

+  setExecutePathState(IDLE);
+}
+
+void MoveGroupExecutePathAction::executePathCallback_Execute(const moveit_msgs::ExecutePathGoalConstPtr& goal, moveit_msgs::ExecutePathResult &action_res)


120 char line limit

davetcoleman · 2016-08-17T15:36:22Z

Thanks for porting this to the new repo, I'm excited about this new feature

wkentaro · 2016-08-18T05:34:38Z

Thanks for the reviews. I fixed them.

davetcoleman · 2016-08-18T06:00:12Z

moveit_ros/move_group/src/default_capabilities/execute_path_action_capability.cpp

+void MoveGroupExecutePathAction::executePathCallback_Execute(const moveit_msgs::ExecutePathGoalConstPtr& goal,
+                                                             moveit_msgs::ExecutePathResult &action_res)
+{
+  ROS_INFO("Execution request received for ExecutePath action.");


missed this earlier: needs _NAMED("move_group"

Oops. Fixed.

davetcoleman · 2016-08-18T06:04:36Z

moveit_ros/planning_interface/move_group_interface/src/move_group.cpp

-    moveit_msgs::ExecuteKnownTrajectory::Response res;
-    req.trajectory = plan.trajectory_;
-    req.wait_for_execution = wait;
-    if (execute_service_.call(req, res))


Since we are no longer using execute_service_ within move_group, I think we can remove it from this class (but keep the capability in general for backwards compatibility)

e.g. we don't need execute_service_ as a MoveGroup member variable and we don't need to initialize it

-1
See main comment.

davetcoleman · 2016-08-18T06:05:30Z

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

wkentaro · 2016-08-18T06:09:03Z

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

Yeah, no problem.

davetcoleman · 2016-08-18T06:39:46Z

just merged the moveit_msgs change and retriggered this travis build

davetcoleman · 2016-08-18T06:40:43Z

moveit_ros/planning_interface/move_group_interface/src/move_group.cpp

                               (node_handle_, move_group::PLACE_ACTION, false));
    waitForAction(place_action_client_, wait_for_server, move_group::PLACE_ACTION);

-    execute_service_ = node_handle_.serviceClient<moveit_msgs::ExecuteKnownTrajectory>(move_group::EXECUTE_SERVICE_NAME);


You can also remove #include <moveit_msgs/ExecuteKnownTrajectory.h>

Thanks. Fixed.

rhaschke · 2016-08-18T11:08:35Z

@davetcoleman Please wait before merging. I want to have a look too.
We definitely need a transition process, deprecating the old execute_service as mentioned before (moveit/moveit_ros#719 (comment)).

davetcoleman · 2016-08-18T18:06:28Z

@rhaschke sounds great. this current PR doesn't break any old functionality AFAIK, but I think you're saying that what is missing is a warning message and inline comments telling users not to use the old excute_service. Then we can remove it completely in ROS-L

rhaschke · 2016-08-19T19:15:53Z

@wkentaro @davetcoleman: Sorry, I accidentally pressed merged, while I was looking for the commandline instructions. I reverted for now, because I have some more comments.

This current PR doesn't break any old functionality AFAIK

NO! There are lots of moveit_config packages out there, which do not yet configure the new capability.
Hence, they will not be able to execute trajectories anymore.

The MoveGroupInterface should try to connect to the ExecuteAction first, and if that fails, resort to the ExecuteService (printing a deprecation warning).

Furthermore, you should adapt the template of moveit_setup_assistant.

@wkentaro I'm afraid that you need to reopen as new PR. I apologize.

wkentaro · 2016-08-19T20:03:55Z

Reopened here #85

davetcoleman · 2016-08-19T23:14:07Z

@rhaschke good points

wkentaro · 2016-08-20T05:29:26Z

Fixed in #85

wkentaro mentioned this pull request Aug 17, 2016

Add capability of execute known trajectory with a ROS action moveit/moveit_ros#719

Closed

wkentaro added 2 commits August 17, 2016 13:11

Add capability of execute known trajectory with a ROS action

f129b5f

Use action for trajectory execution in move_group_interface

26c5d0d

wkentaro force-pushed the execute-path-action branch from 5c4fca4 to 26c5d0d Compare August 17, 2016 04:11

gavanderhoorn reviewed Aug 17, 2016
View reviewed changes

Add missing return when execution is disabled

e094ddc

davetcoleman reviewed Aug 17, 2016
View reviewed changes

wkentaro added 4 commits August 18, 2016 14:30

Fix for line length limit 120

466b3e5

Use _NAMED for logging

985f2b0

Remain execute service client for backward compatibility

cd12eca

Remove no need comments

d66bf03

davetcoleman reviewed Aug 18, 2016
View reviewed changes

Use _NAMED for logging

0eb3d26

davetcoleman reviewed Aug 18, 2016
View reviewed changes

Remove no need execute service client member variable

edd3516

davetcoleman reviewed Aug 18, 2016
View reviewed changes

Remove no need including of ExecuteKnownTrajectory.h

9ea30d5

rhaschke self-assigned this Aug 18, 2016

rhaschke merged commit faaf0fc into moveit:indigo-devel Aug 19, 2016

wkentaro deleted the execute-path-action branch August 19, 2016 19:00

wkentaro restored the execute-path-action branch August 19, 2016 19:16

wkentaro mentioned this pull request Aug 20, 2016

Reopen #60: Add capability of execute known trajectory with a ROS action #85

Closed

rhaschke mentioned this pull request Aug 20, 2016

Reopen #85: Add capability to execute trajectory with a ROS action #94

Merged

130s mentioned this pull request Aug 30, 2016

Add note in ExecuteKnownTrajectory service to recommend ExecuteTrajectory action. moveit/moveit_msgs#29

Merged

davetcoleman mentioned this pull request Apr 11, 2018

Remove deprecated ExecuteTrajectoryServiceCapability in Melodic #833

Merged

5 tasks

JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022

Constraint samplers (moveit#60)

a802f49

Conversation

wkentaro commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Aug 17, 2016

Uh oh!

wkentaro commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Aug 18, 2016

Uh oh!

wkentaro commented Aug 18, 2016

Uh oh!

davetcoleman commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Aug 18, 2016

Uh oh!

davetcoleman commented Aug 18, 2016

Uh oh!

rhaschke commented Aug 19, 2016

Uh oh!

wkentaro commented Aug 19, 2016

Uh oh!

davetcoleman commented Aug 19, 2016

Uh oh!

wkentaro commented Aug 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wkentaro commented Aug 17, 2016 •

edited

Loading