search

moveit / moveit_ros

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

fix trajectory service blocking callback queue #717

Closed rhaschke closed 7 years ago

rhaschke commented 7 years ago

As an ExecuteService request might block (waiting for trajectory execution to finish), processing of other events/callbacks from the main spinner thread would be blocked.

Hence, ExecuteService is now served from a separate spinner thread, using a separate callback queue. This enables trajectory stopping in #713.

An alternative, IMHO even better approach is to turn the service into an action, which provides feedback and can be explicitly interrupted. I did that, but it requires several changes to existing moveit_config packages: as the service capability would be renamed (from MoveGroupExecuteService to MoveGroupExecuteAction all move_group.launch files would need an update.

However, I could think of a slow transition, adding the new capability in Kinetic and loading both the old and new capability in newly created move_config packages. In some later release we could remove the old service capability, hoping that meanwhile everybody has adapted their move configs. What do you think of that idea?

rhaschke commented 7 years ago

If accepted, this PR should be applied to Jade and Kinetic as well. It's an important bug fix.

davetcoleman commented 7 years ago

+1 after code-reviewing, would appreciate someone (doesn't have to be a maintainer) verifying this does as stated

davetcoleman commented 7 years ago

However, I could think of a slow transition, adding the new capability in Kinetic and loading both the old and new capability in newly created move_config packages. In some later release we could remove the old service capability, hoping that meanwhile everybody has adapted their move configs.

+1

rhaschke commented 7 years ago

Considered review comments.

rhaschke commented 7 years ago

With #716 this PR isn't strictly necessary anymore. However,, as it is bad programming practice to block the main spinner thread, I suggest to merge anyway. Implementation provides a nice example how to solve similar situations.

davetcoleman commented 7 years ago

thanks!

also - i notice you pushed the branch directly to the ros-planning org - i think its cleaner practice to keep your own branch on your fork, so that we don't have to occasionally cleanout forgotten branches (which I've done several times for MoveIt) and so users can quickly find relevant *-devel branches