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

Add preemption to rcl_action #679

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

naiveHobo
Copy link

@naiveHobo naiveHobo commented Jun 11, 2020

Signed-off-by: Sarthak Mittal [email protected]

Adds terminal state PREEMPTED and PREEMPT event to goal state machine in order to differentiate between a goal aborted due to genuine abort logic and goal aborted by the server due to preemption.

Based on the discussions in the following ticket: ros2/design#284
Depends on: ros2/rcl_interfaces#105

RCLActionDesign

Signed-off-by: Sarthak Mittal <[email protected]>
@naiveHobo naiveHobo marked this pull request as ready for review June 11, 2020 15:29
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly lgtm, but i am not so sure canceling should be preempted. I do not have any use case on this transition, could you share your thoughts?

},
[GOAL_STATE_CANCELING] = {
[GOAL_EVENT_SUCCEED] = _succeed_event_handler,
[GOAL_EVENT_ABORT] = _abort_event_handler,
[GOAL_EVENT_PREEMPT] = _preempt_event_handler,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure canceling state can be preempted.

@naiveHobo
Copy link
Author

naiveHobo commented Jun 12, 2020

I assumed preemption should work the same way as abort since that's how preemption is handled right now. But I get your point, preemption makes sense only when a previous goal is executing.

@sloretz sloretz self-assigned this Jun 25, 2020
@ivanpauno
Copy link
Member

@sloretz friendly ping

@fujitatomoya
Copy link
Collaborator

It's been 2 months. @sloretz friendly ping.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants