moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
69 stars 118 forks source link

fixup #716 #724

Closed rhaschke closed 7 years ago

rhaschke commented 7 years ago

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.

v4hn commented 7 years ago

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.

rhaschke commented 7 years ago

@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 #442. Now I fixed it. CSM::waitForCurrentState() didn't do what I expected from the name...

Some more thoughts:

rhaschke commented 7 years ago

Continue discussion in #728.