Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
SteveMacenski opened this issue May 5, 2020 · 7 comments · May be fixed by #1117
Labels
enhancement New feature or request

Comments

@SteveMacenski
Copy link
Collaborator

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
Copy link
Contributor

I'd love to take a shot at this.

@naiveHobo
Copy link
Contributor

naiveHobo commented May 17, 2020

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
Copy link
Collaborator Author

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

@jacobperron
Copy link
Member

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: #759. I followed up with a PR adding a way for the server to cancel a goal (#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.

@jacobperron jacobperron self-assigned this May 20, 2020
@SteveMacenski
Copy link
Collaborator Author

SteveMacenski commented May 20, 2020

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
Copy link
Member

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants