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 termination issues #62

Merged
merged 6 commits into from
Feb 3, 2016
Merged

Fix termination issues #62

merged 6 commits into from
Feb 3, 2016

Conversation

suryaambrose
Copy link
Member

This PR fixes the segfault happening every time the bridge is terminated.

@k-okada I also took the liberty of including your commit to fix the termination error due to tactile and bumpers, hope you don't mind. If you do I'll remove it.

This fixes #57 and the other problems discussed in #61 (not including the deadlock caused by audio yet, but it is on the way)

@vrabaud
Copy link
Contributor

vrabaud commented Jan 31, 2016

LGTM, @k-okada , feel free to merge and release.

Change-Id: I1443ce10576f10ceda5041139c90a3df2e65f043
Calling subscribe or unsubscribe while the callback is being called
is already protected on naoqi side. So no need to protect it on the bridge
side, this is what previously led to a deadlock.
We only need mutex protection on configuration variable (publishing,
recording, logging) and also make sure calling subscribe and unsubscribe
at the same time is not possible (even though this is also protected in
naoqi).

Change-Id: Iae604c047046fec9e24832dd4df5017ff4ae724f
@suryaambrose suryaambrose changed the title Fix some termination issues Fix termination issues Feb 1, 2016
@suryaambrose
Copy link
Member Author

These last two commits fix audio and a compilation issue I had with qibuild. It does not seem to impact compilation with catkin.
This pull request fixes:

  • segfault on termination due to bad session destruction
  • segfault on termination due to incorrect bumper/tactile unsubscribe (thanks @k-okada for the fix)
  • deadlock on termination due to audio mutex concurrency

k-okada added a commit that referenced this pull request Feb 3, 2016
@k-okada k-okada merged commit 0a1428f into ros-naoqi:master Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants