Conversation
sloretz
force-pushed
the
reuse_callback_generator
branch
from
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.
Minor nit: I'd put a blank line before this try block, just to visually distinguish it from the if block above.
| """Runs callbacks in the thread which calls :func:`SingleThreadedExecutor.spin`.""" | ||
|
|
||
| def __init__(self): | ||
| super().__init__() |
There was a problem hiding this comment.
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.
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.
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.
D'oh, sorry. I forgot that bit. Never mind, you are totally correct.
| """Runs callbacks in the thread which calls :func:`SingleThreadedExecutor.spin`.""" | ||
|
|
||
| def __init__(self): | ||
| super().__init__() |
There was a problem hiding this comment.
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 |
mikaelarguedas
left a comment
mikaelarguedas
left a comment
There was a problem hiding this comment.
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 |
sloretz
force-pushed
the
reuse_callback_generator
branch
from
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.
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
3 participants
This adds a wrapper around
wait_for_ready_callbacksthat 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)