Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

fixup #716 #724

Merged
merged 4 commits into from
Jul 25, 2016
Merged

fixup #716 #724

merged 4 commits into from
Jul 25, 2016

Conversation

rhaschke
Copy link
Contributor

There was a typo introduced when fixing review comments.

However, more important, another race condition showed up when using continuous controllers:
Due to throttling of planning scene updates in move_group's PSM, it takes a while until these updates are published and thus can be received e.g. in rviz. Hence, we need to wait for planning scene updates to arrive for a while (1s). As soon as an update arrives, waiting is finished.

Need to wait for planning scene updates to be received (by rviz, from move_group).
Due to throttling of planning scene updates in move_group's PSM, this might take a while.
rhaschke added a commit that referenced this pull request Jul 21, 2016
fixes raise conditions in updating of PlanningScene state
- unlock mutexes in reverse order of locking
- PlanningSceneMonitor::syncSceneUpdates(ros::Time): sync all scene updates upto given time
- syncSceneUpdates() after execution of trajectories & before fetching current robot state
- sync scene update before starting planning
- validate trajectory before execution
- final fixup #724
rhaschke added a commit that referenced this pull request Jul 21, 2016
@v4hn
Copy link
Contributor

v4hn commented Jul 25, 2016

This generally improves the branch again, so I'll merge it now.
Most importantly it removes the enforce_next_state_update_ variable again, which break the ABI once more (this should be done as fast as possible after the latest break).

However, @rhaschke phoned me last Friday and we agreed on some simplifications in the code structure.
So, I'm still waiting for the new patch.

@v4hn v4hn merged commit a8afa06 into indigo-devel Jul 25, 2016
@rhaschke
Copy link
Contributor Author

@v4hn Realizing the simplification we agreed on in the phone call turned out to be rather difficult:

  1. simplification of stateUpdateTimerCallback() (removing the additional timestamp check) caused dead locks. Hence, I reverted these changes.
  2. I successfully moved validatePlan() from MoveGroupInterface (client side) to MoveGroup's ExecutionServiceCapability (server side) - as agreed.
  3. Simplification of syncSceneUpdates(): We definitely need both while loops and the timeout. When the robot doesn't move at all, we will never receive a scene update. The first loop is required to check for a recent robot state update directly in CSM, while the second loop (in case there is no CSM) waits - with a timeout - for a direct scene update. There is no way to omit this timeout. The only chance would be to trigger all input channels of PSM to send updates - which is not possible. However, in move_group, this doesn't pose a problem, because there is a CSM instantiated.
  4. Removing syncSceneUpdates() after trajectory execution makes sense. Doing so, led to failure of the test suggested in Trajectory start doesn't match Current Position, when using plan/execute #442. Now I fixed it. CSM::waitForCurrentState() didn't do what I expected from the name...

Some more thoughts:

  • I suggest some more API changes: syncSceneUpdates() should be renamed to waitForCurrentRobotState().
  • There are some methods waitForCurrentState() in CurrentStateMonitor, which actually should be renamed to waitForCompleteState() as they don't look at recent timestamps.
  • I'm still arguing for a true method syncSceneUpdates() which incorporates all pending scene updates in the callback queue. I agree to @v4hn that this doesn't guarantee that all scene updates up to a given timestamp are incorporated, because - due to network delays - ROS messages might be received delayed. The only guarantee for a synchronous scene update is the new method applyPlanningScene().

@rhaschke
Copy link
Contributor Author

Continue discussion in #728.

v4hn added a commit to v4hn/moveit_ros that referenced this pull request Aug 4, 2016
This reverts commit a8afa06, reversing
changes made to 27e953e.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants