rviz plugin: stop execution + update of start state to current by rhaschke · Pull Request #713 · moveit/moveit_ros

rhaschke · 2016-07-15T00:08:47Z

This PR combines ideas of #709 and #710 (and would replaces those PRs) to allow

stopping of an executed trajectory (not working for execute-only button, see comment in Stop execution on motion planning rviz plugin #709)
updating the start state after execution, if current state is selected in gui (fixes Set the start planning state as "current" after finish the execution of a trajectory #646)

I consider this work-in-progress. @davetcoleman Should we address this comment like suggested?

rhaschke · 2016-07-15T00:13:27Z

...lanning_scene_rviz_plugin/include/moveit/planning_scene_rviz_plugin/planning_scene_display.h


  // pass the execution of this function call to a separate thread that runs in the background
-  void addBackgroundJob(const boost::function<void()> &job, const std::string &name);
+  void addBackgroundJob(const boost::function<void()> &job, const std::string &name, bool ownThread=false);


Due to this change, the PR is not ABI compatible. However, this is only related to the rviz plugin, which shouldn't be linked elsewhere. However, if in doubt, we can also target jade-devel.

The only thing that bothers me about this is that its not using ROS's underscore variable convention ;-)

Where do you want to have the underscores? As far as I understand it, only member variables should be suffixed by an underscore. Is there anything else?

all other argument names in this file use underscore notation, so please change ownThread to own_thread.

I agree that breaking ABI in this code is well-justified by safety concerns.
You are not using addBackgroundJob anywhere though. :-)
Also, it might be a good idea to have a separate function name for that, instead of extending the existing one with orthogonal code. spawnBackgroundJob?
Alternatively, we could simply stick with the boost::thread one-liner you used in this request.
But then it might still be a good idea to have explicit comments above each of these lines to explain why this should not be addBackgroundJob.

@v4hn @davetcoleman Whatever you prefer. Please vote ;-)

+1 on a one-line spawnBackgroundJob function and a comment above that explains why this sometimes makes sense to use instead of addBackgroundJob.
I would be interested in a real explanation myself :-)

+1 to @v4hn's comment

davetcoleman · 2016-07-15T02:58:05Z

I think its a good idea to address #709 (comment) but I understand if you don't have the cycles for it right now

davetcoleman · 2016-07-15T02:59:54Z

visualization/motion_planning_rviz_plugin/src/motion_planning_frame_planning.cpp

 {
  ui_->execute_button->setEnabled(false);
-  planning_display_->addBackgroundJob(boost::bind(&MotionPlanningFrame::computeExecuteButtonClicked, this), "execute");
+  boost::thread(boost::bind(&MotionPlanningFrame::computeExecuteButtonClicked, this));


Why use boost threads when this class already has its own background job framework setup? Seems like a departure of the functional style of the rest of the class.

Executing the motion in the (single) background thread blocks visual updates of the scene robot.

Unfortunately, this is only mentioned in the commit log. I will add a source-code comment.

v4hn · 2016-07-15T13:30:36Z

visualization/motion_planning_rviz_plugin/src/motion_planning_frame_planning.cpp

  ui_->execute_button->setEnabled(false);
-  planning_display_->addBackgroundJob(boost::bind(&MotionPlanningFrame::computeExecuteButtonClicked, this), "execute");
+  // executing the motion in the (single) background thread, blocks visual updates of the scene robot
+  boost::thread(boost::bind(&MotionPlanningFrame::computeExecuteButtonClicked, this));


Why are visual updates blocked? I suppose planning scene updates don't get through?
Maybe we could have two callback queues, one for callback related functions and one for handling of user interaction?
I suppose it is a good idea to have a uniform way to run background computation on button pressed throughout rviz-moveit.

Without separate threads for this, the background-job-thread was mainly blocked with waiting for the trajectory execution to finish. Everything else going to the background job simply needs to wait as well - which doesn't make sense.
To have a more uniform "look&feel", I augmented addBackgroundJob() with an extra option ownThread defaulting to false. For some reason the use of this code got lost when cleaning up my messy local repo. However, you suggested to go with boost::thread only. I have a slight preference for that too. Please vote for an option!

rhaschke · 2016-07-16T05:06:21Z

I addressed all comments. Remaining issue is to turn ExecuteService in ExecuteAction and of course deal with #716.

rhaschke · 2016-07-16T18:40:04Z

Further cleaned up and rebased.
Disabling QueryStartState by default as suggested in #710.
Relies on #717.
For correct working of start-state updates, we need to fix #716 as well.
Could be merged now.

davetcoleman · 2016-07-17T15:34:31Z

visualization/motion_planning_rviz_plugin/src/motion_planning_frame_planning.cpp

+void MotionPlanningFrame::onFinishedExecution(bool success)
+{
+  // visualize result of execution
+  if (success) ui_->result_label->setText("Executed");


you should line break after if() statement per ROS style

davetcoleman · 2016-07-17T15:35:22Z

+1

executing the motion in the (single) background thread blocks visual updates of the scene robot

…ot's current state." when planning+executing

This property is "only" used by people playing around with planning internals, e.g. the people maintaining the code.. The "average" user does not expect to be able to manipulate the start state of the motion-request te makes from rviz - it doesn't make sense if you want to actually move the robot. Therefore, disable the start-state by default to reduce the initial complexity the average user is confronted with.

- cleanup structure of updateQueryStateHelper - updateStartStateToCurrent after execution - visualization: stop execution button - suppress warning "Execution of motions should always start at the robot's current state." when planning+executing - rviz display: disable Query Start State by default

davetcoleman · 2016-07-18T09:37:10Z

since this is a relatively major change (new GUI button) to a stable (indigo) release I think you should announce this on the mailing list

also, please be sure to cherry-pick to jade and kinetic

thanks!

cherry-picked #713 from indigo-devel

wkentaro · 2016-07-18T10:52:25Z

@davetcoleman @rhaschke Thanks for working for this!

davetcoleman · 2016-07-18T11:23:35Z

the thanks goes to @rhaschke !

rhaschke · 2016-07-18T12:23:13Z

please be sure to cherry-pick to jade and kinetic

Dave, you already merge-committed #718 into jade-devel. I would have loved to use a squashing commit only, i.e. forwarding jade-devel to the cherry-pick branch.
Anybody minds to rebase jade-devel there?

The same I would do for kinetic. For me this eases reading the git graph:
Merge commits are unique contributions, while cherry-picks could be committed directly.
I just had to use a PR to let Travis do the compile checking.

rhaschke · 2016-07-18T12:36:54Z

since this is a relatively major change (new GUI button) to a stable
(indigo) release I think you should announce this on the mailing list

Dear Dave,
before announcing on the mailing list, I would like to finish #716,
which is a major pre-requisite for #713 to work correctly.
Hopefully, I find some time this evening to continue on that.
In the same vein, I'm planning to merge those changes to Kinetic only as
a bundle.

davetcoleman · 2016-07-18T13:54:08Z

I just had to use a PR to let Travis do the compile checking.

Please make note of that in your PR if you don't want it actually merged yet.

Dave, you already merge-committed #718 into jade-devel.

Yes, I hadn't seen the PR until after I made that comment

You're announcement timeline sounds good, thanks!

- cleanup structure of updateQueryStateHelper - updateStartStateToCurrent after execution - visualization: stop execution button - suppress warning "Execution of motions should always start at the robot's current state." when planning+executing - rviz display: disable Query Start State by default

rhaschke mentioned this pull request Jul 15, 2016

safer rviz display #710

Closed

rhaschke reviewed Jul 15, 2016
View reviewed changes

davetcoleman reviewed Jul 15, 2016
View reviewed changes

rhaschke mentioned this pull request Jul 15, 2016

Stop execution on motion planning rviz plugin #709

Closed

v4hn reviewed Jul 15, 2016
View reviewed changes

rhaschke mentioned this pull request Jul 16, 2016

fix trajectory service blocking callback queue #717

Merged

rhaschke added 2 commits July 16, 2016 18:09

cleanup structure of updateQueryStateHelper

fea1f5a

updateStartStateToCurrent after execution

d0fab1b

rhaschke force-pushed the wip-stop-execution branch 2 times, most recently from f07c9f5 to efab226 Compare July 16, 2016 18:33

davetcoleman reviewed Jul 17, 2016
View reviewed changes

rhaschke and others added 4 commits July 17, 2016 19:49

visualization: stop execution button

f9808b8

rviz plugin: execute motion in separate thread

96664f7

executing the motion in the (single) background thread blocks visual updates of the scene robot

suppress warning "Execution of motions should always start at the rob…

298a67d

…ot's current state." when planning+executing

rhaschke force-pushed the wip-stop-execution branch from efab226 to 181e180 Compare July 17, 2016 17:50

rhaschke mentioned this pull request Jul 18, 2016

fix #442: race conditions when updating PlanningScene state #716

Merged

rhaschke merged commit 6a18450 into indigo-devel Jul 18, 2016

rhaschke deleted the wip-stop-execution branch July 18, 2016 08:26

davetcoleman added a commit that referenced this pull request Jul 18, 2016

Merge pull request #718 from ros-planning/wip-stop-execution

034800d

cherry-picked #713 from indigo-devel

wkentaro mentioned this pull request Jul 18, 2016

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

Closed

rhaschke added a commit that referenced this pull request Jul 20, 2016

merge cherry-picks #713, #717, #716 from jade-devel

197fb83

rhaschke added a commit that referenced this pull request Jul 21, 2016

Merged cherry-picks #713, #717, #716, #724 from jade-devel

9acace1

rhaschke mentioned this pull request Aug 20, 2016

[changelog] Bad encoding. moveit/moveit#91

Merged

jspricke mentioned this pull request Aug 23, 2016

indigo-devel ABI incompabilities moveit/moveit#121

Closed

Conversation

rhaschke commented Jul 15, 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!

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 Jul 15, 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!

rhaschke commented Jul 16, 2016

Uh oh!

rhaschke commented Jul 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Jul 17, 2016

Uh oh!

davetcoleman commented Jul 18, 2016

Uh oh!

wkentaro commented Jul 18, 2016

Uh oh!

davetcoleman commented Jul 18, 2016

Uh oh!

rhaschke commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhaschke commented Jul 18, 2016

Uh oh!

davetcoleman commented Jul 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rhaschke commented Jul 15, 2016 •

edited

Loading

rhaschke commented Jul 18, 2016 •

edited

Loading