rclcpp_action: take and execute service entities in priority. #2471
Conversation
Signed-off-by: Tomoya.Fujita <[email protected]>
|
| std::make_shared<std::tuple<rcl_ret_t, std::shared_ptr<void>>>( | ||
| ret, status_message)); | ||
| } else if (pimpl_->is_goal_response_ready) { | ||
| // Take the service entities 1st to apply to the client |
There was a problem hiding this comment.
The change looks good, but I'm not sure I understand the meaning of this comment.
What's a "service entity"?
There was a problem hiding this comment.
terms might be wrong. but it was supposed to mean it should take the action service executables 1st before action subscriptions.
There was a problem hiding this comment.
I would suggest a more verbose comment, for example:
/*
The order of these statements is important when using waitset-based executors because this method
will be called only once per executor iteration, regardless of how many events are ready.
We first check whether the state of the action goal has changed (i.e. whether we received a goal response,
a result response or a cancellation response).
This ensures that secondary events (e.g. feedback messages) won't prevent the goal state from advancing.
*/
|
@alsora thanks for the review. this will change the action client behavior internally so i need to do more verification locally, and i would like to have more feedback for this before further. please keep it open for a while. |
|
Uhm, this implementation is ontop of a racy state... We should merge #2250 first. |
|
Regarding the issue itself. From my point of view, the design of the action is broken, as it uses multiple communication channels to communicate, thus creating this kind of problems. Btw you can create the same bug by spamming feedback, and make an action never terminate. |
|
@jmachowinski thanks for the comment. this one is not in the merge queue yet, i need to take some more time to verify this, since this changes the action behavior, i am expecting that userspace (tests) would break. |
|
#2612 replaces this one. |
address #2451 and #1679
Note: more tests need to be done before CI.