Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for spin_until_complete #1268

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Apr 2, 2024

Part of ros2/rclcpp#2475

Replaces #919

Similar to the rclcpp changes, this adds spin_until_complete(condition, timeout).

This also adds spin_for.

@christophebedard christophebedard changed the title Replace spin_until_future_complete with spin_until_complete, add spin… Replace spin_until_future_complete with spin_until_complete Apr 2, 2024
@christophebedard christophebedard force-pushed the christophebedard/hliberacki/spin_until branch from fa1df4d to 1127177 Compare April 2, 2024 20:48
Co-authored-by: Hubert Liberacki <hliberacki@gmail.com>
Signed-off-by: Hubert Liberacki <hliberacki@gmail.com>
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard christophebedard force-pushed the christophebedard/hliberacki/spin_until branch from 9b94372 to d0e8130 Compare April 10, 2024 17:22
@christophebedard
Copy link
Member Author

I've updated this PR to reflect the decision in ros2/rclcpp#2475 (comment) to not deprecate spin_until_future_complete and not have spin_until_complete accept both a future and a condition.

@christophebedard
Copy link
Member Author

@fujitatomoya could you review this? This reflects the new changes to ros2/rclcpp#2475, which should be final-ish now.

@christophebedard
Copy link
Member Author

Well this may be going in yet another direction (ros2/rclcpp#2475 (comment)), so let's wait a bit.

@christophebedard
Copy link
Member Author

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.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +272 to +275
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: no mention of the timeout here

Comment on lines +265 to +283
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.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will do it as a follow-up issue/PR.

Comment on lines +441 to +445
self.assertFalse(condition())
t = threading.Thread(target=lambda: set_condition())
t.start()
executor.spin_until_complete(condition, timeout_sec=1.0)
self.assertTrue(condition())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +265 to +270
def spin_until_complete(
node: 'Node',
condition: Callable[[], bool],
executor: Optional['Executor'] = None,
timeout_sec: Optional[float] = None,
) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not return something to indicate if the function returned due to the condition being complete or due to a timeout instead?

Copy link
Member Author

@christophebedard christophebedard Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then that would mean that spin_until_future_complete should also return something similar?

Copy link
Member Author

@christophebedard christophebedard Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants