Fix a bug that goal can be gone before client gets result. #1005
Conversation
Signed-off-by: Barry Xu <Barry.Xu@sony.com>
There was a problem hiding this comment.
I don't think we should change the goal state machine to fix the bug. Sorry if my earlier comment was confusing (#861 (comment)), but I meant that we can use the absence of a goal on the status topic as a way to know if a goal is expired. In other words, if the goal is not in the message, then we can remove it. See my comment below.
rclcpp_action/src/server.cpp
Outdated
| if (RCL_RET_OK != ret) { | ||
| rclcpp::exceptions::throw_from_rcl_error(ret); | ||
| } | ||
| publish_status(); |
There was a problem hiding this comment.
👍 I think not having the publish_status() call was a bug. I don't think we need to publish a message for every goal that was expired, instead just publish once at the end of the while-loop.
There was a problem hiding this comment.
Thanks for your comments.
I think you means as below. Right ?
// Loop in case more than 1 goal expired
while (num_expired > 0u) {
...
}
publish_status();
| @@ -579,7 +579,6 @@ class Client : public ClientBase | |||
| goal_handle->set_status(status.status); | |||
| const int8_t goal_status = goal_handle->get_status(); | |||
| if ( | |||
| goal_status == GoalStatus::STATUS_SUCCEEDED || | |||
There was a problem hiding this comment.
I think we should remove this condition entirely. Instead, we should check if each goal handle being tracked appears in the status message, and erase it if not. Naively, we could use two nested for-loops checking if each goal handle appears in the status list, and removing the goal handle if it doesn't appear in the status list.
If this works, then we avoid having to change the goal state machine.
There was a problem hiding this comment.
Thanks for your comments.
Naively, we could use two nested for-loops checking if each goal handle appears in the status list, and removing the goal handle if it doesn't appear in the status list.
I confused this action. While status message is received, handle_status_message() is called. We can know which goal status is changed. At this time, we cannot think status message for other goal will not be received in next time.
My thought is
Call publish_status() in ServerBase::execute_check_expired_goals() as below codes. That is, while goal expires, the SUCCEEDED status will be sent twice. The second time of send SUCCEEDED status is while goal expires.
While handle_status_message() is called, if status of goal is SUCCEEDED and current status of goal is also SUCCEEDED, we can remove this goal handle.
Unfortunately, I found that publish_status() is called to publish all goals status while updating any goal status. It means that if there comes another action goal request and status updated, the previous status of all goals will be published again. That is, SUCCEEDED status of goal maybe sent twice even if goal doesn't expire.
There was a problem hiding this comment.
At this time, we cannot think status message for other goal will not be received in next time.
That's a good point. I think we don't want to remove goals that do not have a result, so the condition for removing a goal from the client is: goal has terminal state and goal is not in the status message.
Call publish_status() in ServerBase::execute_check_expired_goals() as below codes. That is, while goal expires, the SUCCEEDED status will be sent twice. The second time of send SUCCEEDED status is while goal expires.
I don't think this is necessary if with what I proposed above: For each goal not in the status message, if that goal is in a terminal state (SUCCEEDED, CANCELED, or ABORTED), then we can remove it.
There was a problem hiding this comment.
I understand what you want.
Please review last commit.
Signed-off-by: Barry Xu <Barry.Xu@sony.com>
Barry-Xu-2018
force-pushed
the
topic-get-action-result
branch
from
cbfd011
to
bc28f81
Compare
|
Based on comments #861 (comment), use new way to fix problem. So this commit is abandoned. |
* Implements play until on top of play for. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Adds python bindings Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Adds test for play until. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Fixes variable name. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Adds the same docstring for play_until and play_duration options than in PlayOptions to clarify usage. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Updates the CLI to combine the seconds and nanoseconds part of the playback_until stamp Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Changes to play verb parameters: - Unifies the use of '-' instead of '_' to split words in arguments. - Uses mutually exclusive arguments: --playback-until-sec and --playback-until-nsec to pass the timestamp. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Solves review comments. - Adds test cases to the player. - Renames play_until and similar occurrences to contain timestamp. - Renames play_for and similar occurrences to not contain for and include duration. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Solves review comments to tests. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com> * Modifies play_until_is_equal_to_the_total_duration The test body now evaluates a bag whose duration is exactly the same as given timestamp in PlayOptions. Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com>
Related issue #861