-
Notifications
You must be signed in to change notification settings - Fork 224
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
Reuse ready callbacks generator #159
Conversation
211a8dc
to
9b072ae
Compare
rclpy/rclpy/executors.py
Outdated
self._last_args = args | ||
self._last_kwargs = kwargs | ||
self._cb_iter = self._wait_for_ready_callbacks(*args, **kwargs) | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: I'd put a blank line before this try block, just to visually distinguish it from the if block above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 786b4cf
|
||
|
||
class SingleThreadedExecutor(Executor): | ||
"""Runs callbacks in the thread which calls :func:`SingleThreadedExecutor.spin`.""" | ||
|
||
def __init__(self): | ||
super().__init__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that a method that is not overloaded in the child class means that the parent version of the method is used. I believe this applies to all methods included the __init_
one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about this, so I just tried it out with the following program:
class Base:
def __init__(self):
print("Base init")
class Sub:
def do_something(self):
print("Sub do_something")
x = Sub()
x.do_something()
If you run that under python3, the output you get is just "Sub do_something"; you never see the "Base init" printed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try the same thing with Sub inheriting from base ?
class Base:
def __init__(self):
print("Base init")
class Sub(Base):
def do_something(self):
print("Sub do_something")
x = Sub()
x.do_something()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, sorry. I forgot that bit. Never mind, you are totally correct.
|
||
|
||
class SingleThreadedExecutor(Executor): | ||
"""Runs callbacks in the thread which calls :func:`SingleThreadedExecutor.spin`.""" | ||
|
||
def __init__(self): | ||
super().__init__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that a method that is not overloaded in the child class means that the parent version of the method is used. I believe this applies to all methods included the __init_
one
Does this new code have this property because we always process everything from the previous wait set before constructing a new one? (I just want to make sure I understand) |
@clalancette Yup Edit: Assuming of course that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, @clalancette is reviewing as well so let's wait for his feedback as well
Adds wrapper around wait_for_ready_callbacks Wrapper reuses generator if arguments match while it's still producing Adds TimeoutException and raises it when waiting times out
The only concern I have is with the recursive nature of |
786b4cf
to
b7fa6d6
Compare
@clalancette eliminated the recursion in b7fa6d6 CI |
""" | ||
# if an old generator is done, this variable makes the loop get a new one before returning | ||
got_generator = False | ||
while not got_generator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the loop a lot better; thanks!
ros2/rclpy#159 changed wait_for_ready_callbacks to manage the generator internally and return just a tuple
This adds a wrapper around
wait_for_ready_callbacks
that reuses the callback generator when multiple things are ready. It avoids unnecessary wait list construction and waiting. It also prevents one callback (like a very fast timer) from starving other callbacks (similar to ros2/rclcpp#392).Taken from #140 and improved upon.
CI (New run after fixing a merge conflict)