DRAFT: Fix: Double spin required since 28.2.0 #2595
Conversation
jmachowinski
force-pushed
the
2589_double_spin
branch
from
a8144e7
to
dd0c6f0
Compare
jmachowinski
requested review from
ivanpauno,
hidmic and
wjwwood
as code owners
jmachowinski
force-pushed
the
2589_double_spin
branch
from
dd0c6f0
to
f938f9a
Compare
|
@alsora can you run the CI on this ? |
Rebuild the collection explicitly before the wait, if the notify_waitable_ has been triggered. Signed-off-by: Janosch Machowinski <[email protected]>
jmachowinski
force-pushed
the
2589_double_spin
branch
from
f938f9a
to
0705e71
Compare
|
There was a problem hiding this comment.
@jmachowinski thanks for working on the fix.
a few CI failures, can you take a look?
| if (notify_cpy->is_ready(rcl_wait_set)) { | ||
| notify_cpy->execute(notify_cpy->take_data()); | ||
|
|
||
| // make sure we don't loos a wakeup |
There was a problem hiding this comment.
typo?
| // make sure we don't loos a wakeup | |
| // make sure we don't lose a wakeup |
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/next-client-library-wg-meeting-friday-16th-august-2024/39130/1 |
|
@jmachowinski A friendly ping to fix CI failures to move forward with this PR. |
|
@MichaelOrlov we spoke about this bug in the last client workgroup meeting. We don't think this is the correct fix, but a step into the right direction.Unfortunately I currently don't have much time I can divert to rclcpp. |
|
PR #2591 merged. I looked a bit more into the waitset executor issue, and IMO even if this PR is likely not the best way to address the problem, it definitely improves the situation in a relatively simple way, fixing the bug reported by the nav stack and passing the unit tests that I added in the other PR. My recommendation is to:
If people agree, I can take care of pushing these changes (or @jmachowinski can do it if he has time). |
This is a first attempt to fix #2589.
Rebuild the collection explicitly before the wait, if the notify_waitable_
has been triggered.