-
Notifications
You must be signed in to change notification settings - Fork 118
Stop execution on motion planning rviz plugin #709
Conversation
wkentaro
commented
Jul 10, 2016
•
edited
Loading
edited
Sounds cool - can you provide a screenshot of the GUI change? |
I added a GIF. |
@@ -129,15 +135,31 @@ void MotionPlanningFrame::computePlanButtonClicked() | |||
void MotionPlanningFrame::computeExecuteButtonClicked() | |||
{ | |||
if (move_group_ && current_plan_) | |||
move_group_->execute(*current_plan_); | |||
ui_->stop_button->setEnabled(true); | |||
move_group_->asyncExecute(*current_plan_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need brackets around these two lines
Oops, I sent the wip code. Fixed. |
+1, looks great! |
} | ||
|
||
void MotionPlanningFrame::computePlanAndExecuteButtonClicked() | ||
{ | ||
if (!move_group_) | ||
return; | ||
configureForPlanning(); | ||
move_group_->move(); | ||
ui_->stop_button->setEnabled(true); | ||
move_group_->asyncMove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe reenabling the button after the execution is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button should be only re-enabled after execution. Because you switch to asynchronous execution now, the button gets re-enabled immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Nice feature. Have a look at the inline comments. |
Hm. While your recent code will fix the enabling of the stop button, I think the code is rather complicated. |
Yeah, I have tried |
@@ -129,15 +137,28 @@ void MotionPlanningFrame::computePlanButtonClicked() | |||
void MotionPlanningFrame::computeExecuteButtonClicked() | |||
{ | |||
if (move_group_ && current_plan_) | |||
move_group_->execute(*current_plan_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
andmove_group::move()
are calls to an action, that can be interrupted. The flagplan_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. That looks promising. Could you prepare a PR for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. You're right. I meant it does not work.
If you agree, please close this PR in favor of #713. |
Closed in favor of #713. |