Add support for spin_until_complete #1268
Conversation
christophebedard
requested review from
sloretz and
adityapande-1995
as code owners
christophebedard
changed the title
christophebedard
force-pushed
the
christophebedard/hliberacki/spin_until
branch
from
fa1df4d
to
1127177
Compare
christophebedard
force-pushed
the
christophebedard/hliberacki/spin_until
branch
from
1127177
to
35450f4
Compare
christophebedard
changed the title
christophebedard
force-pushed
the
christophebedard/hliberacki/spin_until
branch
from
35450f4
to
9b94372
Compare
Co-authored-by: Hubert Liberacki <[email protected]> Signed-off-by: Hubert Liberacki <[email protected]> Signed-off-by: Christophe Bedard <[email protected]>
christophebedard
force-pushed
the
christophebedard/hliberacki/spin_until
branch
from
9b94372
to
d0e8130
Compare
|
I've updated this PR to reflect the decision in ros2/rclcpp#2475 (comment) to not deprecate |
|
@fujitatomoya could you review this? This reflects the new changes to ros2/rclcpp#2475, which should be final-ish now. |
|
Well this may be going in yet another direction (ros2/rclcpp#2475 (comment)), so let's wait a bit. |
All good now, this is ready for review. |
There was a problem hiding this comment.
Very quick look from me and it looks pretty good, but I do think the function ought to return something like it does in c++. However, another rclpy maintainer can override me if they think this could be added later or that it isn't necessary.
Also, I feel that the tests are kind of shallow and wouldn't catch some of the ways the function could fail.
| Execute work until the condition is complete. | ||
|
|
||
| Callbacks and other work will be executed by the provided executor until ``condition()`` | ||
| returns ``True`` or the context associated with the executor is shutdown. |
There was a problem hiding this comment.
nitpick: no mention of the timeout here
| def spin_until_complete( | ||
| node: 'Node', | ||
| condition: Callable[[], bool], | ||
| executor: Optional['Executor'] = None, | ||
| timeout_sec: Optional[float] = None, | ||
| ) -> None: | ||
| """ | ||
| Execute work until the condition is complete. | ||
|
|
||
| Callbacks and other work will be executed by the provided executor until ``condition()`` | ||
| returns ``True`` or the context associated with the executor is shutdown. | ||
|
|
||
| :param node: A node to add to the executor to check for work. | ||
| :param condition: The callable condition to wait on. If this condition is not related to what | ||
| the executor is waiting on and the timeout is infinite, this could block forever. | ||
| :param executor: The executor to use, or the global executor if ``None``. | ||
| :param timeout_sec: Seconds to wait. Block until the condition is complete | ||
| if ``None`` or negative. Don't wait if 0. | ||
| """ |
There was a problem hiding this comment.
To go along with the discussion in the rclcpp version of this (ros2/rclcpp#2475 (comment)), I think we need to do a much better job of explaining that the executor must be woken up when the condition changes and condition's value must be changed before the signal to wake up is sent, otherwise you might deadlock here (if not using a timeout).
I think we could make an issue to do this as a follow up, as the docs can be improved post API freeze.
There was a problem hiding this comment.
Sounds good, will do it as a follow-up issue/PR.
| self.assertFalse(condition()) | ||
| t = threading.Thread(target=lambda: set_condition()) | ||
| t.start() | ||
| executor.spin_until_complete(condition, timeout_sec=1.0) | ||
| self.assertTrue(condition()) |
There was a problem hiding this comment.
This is a fairly weak test in my opinion, as you could comment out line 444 and have it pass most of the time. Also, if spin_until_complete() was broken (and did not return before the timeout if the condition was true) this would still pass.
| def spin_until_complete( | ||
| node: 'Node', | ||
| condition: Callable[[], bool], | ||
| executor: Optional['Executor'] = None, | ||
| timeout_sec: Optional[float] = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Should this not return something to indicate if the function returned due to the condition being complete or due to a timeout instead?
There was a problem hiding this comment.
Then that would mean that spin_until_future_complete should also return something similar?
There was a problem hiding this comment.
Just so we agree before I do the changes: do we just want a bool (True means it returned due to the condition being complete, or False otherwise), or do we want something like rclcpp::SpinUntilCompleteReturnCode to indicate SUCCESS or INTERRUPTED or TIMEOUT?
Part of ros2/rclcpp#2475
Replaces #919
Similar to the
rclcppchanges, this addsspin_until_complete(condition, timeout).This also adds
spin_for.