Add preempted ResultCode for WrappedResult and preempt API for ServerGoalHandle #1117
Conversation
…GoalHandle Signed-off-by: Sarthak Mittal <[email protected]>
|
The current version assumes the availability of |
…hard code to 7 Signed-off-by: Sarthak Mittal <[email protected]>
| @@ -581,7 +581,8 @@ class Client : public ClientBase | |||
| if ( | |||
| goal_status == GoalStatus::STATUS_SUCCEEDED || | |||
| goal_status == GoalStatus::STATUS_CANCELED || | |||
| goal_status == GoalStatus::STATUS_ABORTED) | |||
| goal_status == GoalStatus::STATUS_ABORTED || | |||
| goal_status == 7) // GoalStatus::STATUS_PREEMPTED) | |||
There was a problem hiding this comment.
https://github.com/ros2/rcl_interfaces/blob/master/action_msgs/msg/GoalStatus.msg needs to be updated.
| ASSERT_LT(0u, received_msgs.size()); | ||
| auto & msg = received_msgs.back(); | ||
| ASSERT_EQ(1u, msg->status_list.size()); | ||
| EXPECT_EQ(action_msgs::msg::GoalStatus::STATUS_ABORTED, msg->status_list.at(0).status); |
There was a problem hiding this comment.
This is expected to STATUS_ABORTED? not STATUS_PREEMPTED?
There was a problem hiding this comment.
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(). The message received here is published by rcl_actions.
There was a problem hiding this comment.
thanks, i understand now.
|
I believe that rclpy action also needs to be updated based on this fix. Otherwise we lose the consistency. |
|
If rclpy and action_msgs is also expected to make this update, it would make sense to add preemption to rcl_action altogether. |
|
@naiveHobo do you mean it assumes the I don't necessarily think that we have to implement the python version as well. That could be left to another party that cares about that (which we don't) unless the maintainers here require it. |
| @@ -37,7 +37,8 @@ enum class ResultCode : int8_t | |||
| UNKNOWN = action_msgs::msg::GoalStatus::STATUS_UNKNOWN, | |||
| SUCCEEDED = action_msgs::msg::GoalStatus::STATUS_SUCCEEDED, | |||
| CANCELED = action_msgs::msg::GoalStatus::STATUS_CANCELED, | |||
| ABORTED = action_msgs::msg::GoalStatus::STATUS_ABORTED | |||
| ABORTED = action_msgs::msg::GoalStatus::STATUS_ABORTED, | |||
| PREEMPTED = 7 // action_msgs::msg::GoalStatus::STATUS_PREEMPTED | |||
There was a problem hiding this comment.
Yeah, please add this to the GoalStatus
|
|
|
The cppcheck failures are happening on the master branch, so I guess it's not related: http://build.ros2.org/view/Fdev/job/Fdev__rclcpp__ubuntu_focal_amd64/25/testReport/ |
|
Waiting for ros2/rcl_interfaces#105 to pass so |
|
Thanks for linking that here. @jacobperron who's the owner of that one? |
If you're referring to the rcl_interfaces PR, nobody yet; the core ROS 2 repositories have shared maintainership across the team. Typically, people will be assigned during triage meetings. I can take a quick look at the proposed changes, but these kinds of API changes will have to wait until post-Foxy release. |
|
Makes sense, I wouldn't expect otherwise. It would be nice to get in at a soon-after sync. This would be a critical fault for Navigation2 if this wasn't available in Foxy at all. It currently blocks the only reasonable solution some severe architectural issues. |
|
@naiveHobo it looks like there's a merge conflict. Mind resolving it? |
Signed-off-by: Sarthak Mittal [email protected]
Closes #1104