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

Fix bond handling during nodelet unloading #51

Merged
merged 4 commits into from
Nov 1, 2016

Conversation

dseifert
Copy link
Contributor

This fixes points 2 and 3 of #50 and introduces two changes:

  1. The callback for broken bonds is reset when unloading, to avoid calling unload() again.
  2. nodelet load uses an ASyncSpinner in order to handle bond communication during shutdown.

@mikaelarguedas
Copy link
Member

nice catch! tested locally and it works just fine, all nodelets are unloaded properly.
Would it be possible to provide an automated test with numerous nodelet loads and confirming that they are all unloaded properly?

Thanks for the contribution!

@dseifert
Copy link
Contributor Author

What is the definition of a nodelet being unloaded properly?

Daniel Seifert added 4 commits October 30, 2016 18:39
… to work

During shutdown, the bonds still need to communicate their status in order
for the nodelet to properly/cleanly/quickly unload. This requires the node
to spin.
This avoids the nodelet::LoaderROS::unload() method to be called
twice for the same nodelet, causing an error output.
@dseifert
Copy link
Contributor Author

@mikaelarguedas I added tests for my changes. I don't think it is necessary to add a test for multiple nodelets for these, as the issues are reproducable with single nodelets, too (though "success" for the proper bond handling is a bit hard to check for right now, basically it is not getting a certain DEBUG output).

@dseifert dseifert mentioned this pull request Oct 30, 2016
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

@dseifert Thanks for adding the tests, multiple nodelets unloading was not tested before and this PR definitely improves of the unloading behaviour regardless of the number of nodelets. So I'm in favor of merging it.
Thanks for the contribution

@mikaelarguedas mikaelarguedas merged commit 6dcff94 into ros:indigo-devel Nov 1, 2016
@kejxu
Copy link
Contributor

kejxu commented Jul 11, 2019

thanks for the detailed explanation @dseifert, really helpful! Just curious, why is single-threaded ros::AsyncSpinner spinner(1) needed? In other words, what is the problem with ros::spinOnce();? Thanks!

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.

None yet

3 participants