rclcpp_actions: Add preempted ResultCode for WrappedResult and preempt API for ServerGoalHandle

SteveMacenski commented 4 years ago

Bug report

Required Info:

Operating System: Ubuntu 18
Installation type: Source
Version or commit hash: N/A
DDS implementation: Fast RTPS
Client library (if applicable): rclcpp_action

Feature request

Feature description

Right now, there's no good way to handle preemption in rclcpp_actions. In the navigation2 simple action server wrapper we set preemption to abort. The issue we face is that there's no way to differentiate between an abort caused by a true failure in the server (e.g. couldn't plan, couldn't move my forklift to N inches, etc) and an abort caused by a request for preemption.

This is important for the client to be able to distinguish why it received a result code. So I propose adding to the API a preempt() analog to abort() and returns to the client a rclcpp_action::ResultCode::PREEMPTED result code so we know why it returned for the case of a preemption.

Let me know your thoughts on this strategy. I can file some tickets in navigation2 and I can work with some folks to implement this if its something that would be merged into rclcpp_actions. We're in the middle of a discussion in a few tickets about how to work around for the short term.

naiveHobo commented 4 years ago

I'd love to take a shot at this.

naiveHobo commented 4 years ago

Another thing to consider is adding the preemption API in rcl_actions. The action_msgs::msg::GoalStatus msgs published on /_action/status will still have their status set to ABORTED when preemption takes place in through ServerGoalHandle::preempt().

This would also require the addition of action_msgs::msg::GoalStatus::STATUS_ABORTED in: https://github.com/ros2/rcl_interfaces/blob/master/action_msgs/msg/GoalStatus.msg

SteveMacenski commented 4 years ago

@naiveHobo the GoalStatus is definitely a requirement. Anywhere that cancel exists, preempt must as well. Another example is ResultCode.

jacobperron commented 4 years ago

We originally did not include goal states for server-side preemption in an effort to reduce the complexity of the state machine. In retrospect, having additional states would have been useful for the "simple" action use-case where we let the server cancel (instead of abort) goals.

There has already been some discussion about how this feature would be desired for implementing a "simple" action server: https://github.com/ros2/rclcpp/issues/759. I followed up with a PR adding a way for the server to cancel a goal (https://github.com/ros2/rclcpp/pull/884), but decided not to merge to avoid confusing the semantics of "aborted" and "canceled" terminal states.

I think adding an additional state to indicated server-side preemption makes sense, but we should be careful in the design. Currently, we have a "canceling" state that allows for the server to do some cleanup tasks if it wants (see design doc). I suspect we probably want to do something similar if we added server-side preemption. E.g. "Executing" -> "Canceling" (or maybe a new state "Server-initiated canceling") -> "Server canceled".

I would prefer that we focus on amending design of the state machine first, and once that is sorted we can make the necessary changes throughout the stack (rcl, rclcpp, rclpy, and examples). A PR against ros2/design and/or a call for comments on Discourse seem like good ideas to me.

SteveMacenski commented 4 years ago

I'm not sure what this has to do with the simple wrapper - this is largely independent. Its also possible for a worker-action that can process N at a time to want to create a distinction between goals stopped due to failure or due to lack of resources to process (e.g. you have a server with 8 processing threads and you want to always process the 8 newest ones, when #9 comes in, you want to distinguish that you're preempting one of them due to lack of resources rather than a failure of the processing itself). Though I agree that the strongest thematic argument for this would be a "simple" single-goal processing action server.

I don't disagree with that. I wonder about the practical benefits given the preemption should be immediate, but I understand why being complete is really helpful.

jacobperron commented 4 years ago

I'm not sure what this has to do with the simple wrapper

I thought this was the primary motivation for this ticket (it's referenced in the description). The N-goal server you described is a good example too (sounds like a generalized version of the simple action server from ROS 1).

I wonder about the practical benefits given the preemption should be immediate, but I understand why being complete is really helpful.

I'm not convinced the preemption should be immediate. There's a user execution thread running that might like to do some cleanup (e.g. bring the robot into a safe state) before it gives up control to the next goal.

SteveMacenski commented 4 years ago

That was just more for context in case someone came back and asked where its being used. I air on the side of too much information rather than too little information.

might like to do some cleanup

Super fair - agreed.

ros2 / rclcpp