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

no ros::spinOnce() #82

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

simonschmeisser
Copy link
Contributor

calling ros::spinOnce() in random places ( ;) ) messes up threading. This causes updates to PlanningSceneMonitor and joint state to be handled in our gui thread as opposed to the async spinner thread where we intended them to be handled.

sorry for recommiting, somehow rebasing failed
@davetcoleman
Copy link
Member

This may have big consequences to current users - can you please create a MIGRATION.md file for Melodic and include a note about this change?

See https://github.com/ros-planning/moveit/blob/kinetic-devel/MIGRATION.md for reference

@simonschmeisser
Copy link
Contributor Author

I'll be on vacation for two more weeks but thanks for looking at all the stuff Dave!

I can see only one case where this would negatively affect existing code: when you never ever call spinOnce and don't start any async spinner thread. I would consider this a bug in user code. Do I miss anything? For us this was just a huge bugfix as spinOnce processes the entire content of the message queue in the calling thread. This heavily delayed JointState messages in some cases.

I'll write the migration note when I'm back

@simonschmeisser
Copy link
Contributor Author

@davetcoleman like this?

@davetcoleman
Copy link
Member

Awesome thanks!

@davetcoleman davetcoleman merged commit 87fdfe3 into PickNikRobotics:melodic-devel Sep 6, 2018
@simonschmeisser
Copy link
Contributor Author

I'd love to see this cherry picked to kinetic-devel as well ...

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.

2 participants