you need brackets around these two lines

Changing from synchronous to asynchronous command here, immediately (re)enables the "plan+execute" button.
This changes behaviour. Before, the button was enabled only when move() finished.

Maybe reenabling the button after the execution is necessary?

The button should be only re-enabled after execution. Because you switch to asynchronous execution now, the button gets re-enabled immediately.

Fixed.

Replacing move_group_->execute() with move_group_->asyncMove() is not possible, because behaviour is changed: While the former executes a previously planned, manually validated trajectory, the latter will actually plan a new trajectory and directly execute it - with no chance for inspection/validation.

I cannot accept this PR in this form.

Looking deeper into the reasons, I found the following: There are essentially to methods to plan/execute:

• move_group::plan() and move_group::move() are calls to an action, that can be interrupted. The flag plan_only allows to switch off/on execution.
• However, move_group::execute() is a call to a service (MoveGroupExecuteService), which cannot be interrupted.

In order to allow stopping of a trajectory, the service should be turned into an action as well. As far as I can see, that changed could be hidden transparently behind the existing MoveGroupInterface. However, I'm not sure, whether some (3rd-party) code is directly calling the service. Within the MoveIt code base, I couldn't find any other usage.

@davetcoleman Do you think, this change is reasonable?

Alternatively, we do not allow to stop an executed-only trajectory.

In order to allow stopping of a trajectory, the service should be turned into an action as well.

Precisely speaking, for stopping of a trajectory, the service call with wait_for_execution:=false and stop() method call is enough. The reason I needed to change the execute() method to move() is to get the trajectory execution server status to handle GUI button status correctly in 5503fa9.

Replacing move_group_->execute() with move_group_->asyncMove() is not possible, because behaviour is changed: While the former executes a previously planned, manually validated trajectory, the latter will actually plan a new trajectory and directly execute it - with no chance for inspection/validation.

I see.

the service should be turned into an action as well.

I agree with the change.

Precisely speaking, for stopping of a trajectory, the service call
with |wait_for_execution:=false| and |stop()| method call is enough.
The reason I needed to change the |execute()| method to |move()| is to
get the trajectory execution server status to handle GUI button status
correctly in 5503fa9
The service call doesn't have a stop() method, does it? I guess you are
intermixing with the move action?

@wkentaro: Could you propose a PR (w.r.t. #713) for turning the execute service into an action?
Dave agreed on that approach, but I don't have time for it right now.
I think, having that done too, we can merge this branch.

The service call doesn't have a stop() method, does it? I guess you are
intermixing with the move action?

I don't use stop() method of the move action, but use stop() method in move_group with trajectory_event_manager.
https://github.com/ros-planning/moveit_ros/blob/kinetic-devel/planning_interface/move_group_interface/src/move_group.cpp#L763
https://github.com/ros-planning/moveit_ros/blob/kinetic-devel/planning_interface/move_group_interface/src/move_group.cpp#L130

I think the execution can be interrupted if wait_for_execution=false for execution service, with stop() calling in move_group.

Indeed. That looks promising. Could you prepare a PR for this?

Indeed. That looks promising. Could you prepare a PR for this?

This PR without last commit is that approach. The trajectory execution with execute_button on rviz plugin was able to be interrupted with asyncExecute() and move_group_->stop().
However, it is unable to get the status of execution of service call, so I could not handle the GUI button status, enable/disable, correctly.
That's why I needed to change the asyncExecute() to asyncMove() in the last commit, in order to handle the enable/disable status of GUI precisely.

@wkentaro Doesn't work like you described. Of course, using asynchronous execution (wait_for_execution=false) will allow the stop call to get through and stop motion.
However, asynchronous execution through a service doesn't allow to get feedback when execution finished. We need to turn the service into an action.

Yeah. You're right. I meant it does not work.