Fixed preempting of execution #6
Conversation
|
@wyattrees @henningkayser Could you please review this .? |
JafarAbdi
force-pushed
the
pr-fix_preempt_execution
branch
from
3b75b5b to
6fd3334
Compare
| while (result_future.wait_for(std::chrono::milliseconds(10)) != std::future_status::ready) { | ||
| if (pimpl()->preempt_requested_) { | ||
| auto cancel_future = ac->async_cancel_goal(goal_handle); | ||
| if (rclcpp::spin_until_future_complete(node, cancel_future) != rclcpp::FutureReturnCode::SUCCESS) { |
There was a problem hiding this comment.
why do we need to spin here? I thought that there would be an executor spinning the node already, no?
There was a problem hiding this comment.
As you can see here it's created locally only for execution
There was a problem hiding this comment.
I guess that makes sense for now... But aren't there some stages that depend on ROS interfaces? Those will need an executor as well, no?
| @@ -87,6 +87,8 @@ namespace move_group { | |||
| ExecuteTaskSolutionCapability::ExecuteTaskSolutionCapability() : MoveGroupCapability("ExecuteTaskSolution") {} | |||
|
|
|||
| void ExecuteTaskSolutionCapability::initialize() { | |||
| action_callback_group_ = | |||
| context_->moveit_cpp_->getNode()->create_callback_group(rclcpp::CallbackGroupType::Reentrant); | |||
There was a problem hiding this comment.
What's the reason to use Reentrant here? This seems inherently unsafe if not handled appropriately.
There was a problem hiding this comment.
To make it possible to call the cancel callback while executing the goal callback
There was a problem hiding this comment.
I see, I thought at first that "Reentrant" would only apply to the same callback and not to other's in the same group. To me it seems like there is a type missing. What I don't really like about this is that you have to take care about redundant calls to the same callback manually, even though that's definitely an edge case.
JafarAbdi
force-pushed
the
pr-fix_preempt_execution
branch
from
6fd3334 to
2d797ac
Compare
JafarAbdi
force-pushed
the
ros2
branch
7 times, most recently
from
09593dc to
71a604c
Compare
JafarAbdi
force-pushed
the
pr-fix_preempt_execution
branch
from
2d797ac to
a760804
Compare
…ting the goal so it does not block the server from accepting a cancel request. While client is waiting for the result future, repeatedly check if preempting was requested and send a cancel request if it was
JafarAbdi
force-pushed
the
pr-fix_preempt_execution
branch
from
a760804 to
fcdce1e
Compare
Rebased #3 on
ros2and small clean-up to minimize diff for later sync between master & ros2 branches