-
Notifications
You must be signed in to change notification settings - Fork 119
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
fuse -> ROS 2 fuse_core: Fix Async (Redux) #294
fuse -> ROS 2 fuse_core: Fix Async (Redux) #294
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
3f2ec60
to
cf58d75
Compare
I've updated the PR to include the following changes:
We uncovered a potential bug in rclcpp's executors' handling of mutually exclusive callback groups, but in this case it doesn't block the progress for fuse_core |
See: #276 |
Signed-off-by: methylDragon <methylDragon@gmail.com>
cf58d75
to
c0bb3f7
Compare
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.
Approving, but double checking with @sloretz that my changes are alright
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
I made a few changes after self-reviewing and reading this bit of fuse_tutorials.
|
@ros-pull-request-builder retest this please |
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 realized we should have initialized_ = false
inside internal_stop
so the user can choose to reinitialize the model
Aside from that, minor comments to consider
@ros-pull-request-builder retest this please |
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.
Let's add an initialized_ = false
in the internal stops, other than that, LGTM!
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Added in 9c95cf6 |
@ros-pull-request-builder retest this please |
@methylDragon Here's an alternative to #293. I'm running the
test_async_sensor_model
test locally with--retest-until-fail 1000
. I think this addresses the two issues found so far.@svwilliams for visibility