-
Notifications
You must be signed in to change notification settings - Fork 429
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
Comments
I'd love to take a shot at this. |
Another thing to consider is adding the preemption API in This would also require the addition of |
@naiveHobo the |
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 ( |
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 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. |
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'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. |
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.
Super fair - agreed. |
Bug report
Required Info:
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 toabort()
and returns to the client arclcpp_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.
The text was updated successfully, but these errors were encountered: