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 action_msgs::msg::GoalStatus::STATUS_PREEMPTED #105

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

Conversation

naiveHobo
Copy link

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

As per ros2/rclcpp#1104 and the corresponding PR ros2/rclcpp#1117, this PR adds STATUS_PREEMPTED to action_msgs/GoalStatus to refer to a goal that was preempted by the client.

@jacobperron jacobperron self-assigned this May 20, 2020
@jacobperron
Copy link
Member

I don't think it's as simple as adding a new field to the message, there is also code in rcl_action and rclpy that works with GoalStatus. Before making further changes, I suggest a discussion and changes around the design of the state machine (see ros2/rclcpp#1104 (comment))

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.

This fix itself is LGTM.

@SteveMacenski
Copy link

@jacobperron mentions that it may be worth adding the PREEMPTING as well - I agree as long as the intention is to add that to the state machine.

@SteveMacenski
Copy link

@naiveHobo I think the best route here is to add the PREEMPTING message he asked for so that these messages are complete and then work with Jacob to update the state machine. Given we had one like this for ROS1, it shouldn't be too big of an undertaking. Once we have the design and this message in place, we can get the rclcpp PR merged.

@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