Fix Gazebo freezing if YARP_CLOCK is set #537
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix #526 Add also regression test for the issue
xEnVrE
approved these changes
Feb 12, 2021
plugins/clock/src/Clock.cc
Outdated
| m_network = new yarp::os::Network(); | ||
| // To avoid deadlock during initialation if YARP_CLOCK is set, | ||
| // if the YARP network is not initialized we always initialize | ||
| // it with system clock, and then we |
There was a problem hiding this comment.
It seems that here the sentence in the comment is truncated.
plugins/clock/src/Clock.cc
Outdated
| @@ -68,7 +68,20 @@ namespace gazebo | |||
|
|
|||
| void GazeboYarpClock::Load(int _argc, char **_argv) | |||
| { | |||
| m_network = new yarp::os::Network(); | |||
| // To avoid deadlock during initialation if YARP_CLOCK is set, | |||
There was a problem hiding this comment.
Suggested change:
initialation -> initialization
There was a problem hiding this comment.
Thanks, feel free to do this suggestions with GitHub's integrated system!
traversaro
commented
Feb 12, 2021
traversaro
commented
Feb 12, 2021
Fine with me! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #526 .
TL;DR: From gazebo-yarp-plugins 3.6.0, you can again set YARP_CLOCK for the Gazebo simulation, meaning that all the threads of YARP Network Wrapper Servers will be executed with the frequency correctly synchronized with the Gazebo simulation. This also means that if you like you can also set
YARP_CLOCKdirectly in your.bashrcorsetup.shscript.With respect to the solution discussed in #526, i.e. :
in this PR there are two modifications.
First of all, the delayed initialization of the Clock is only done if the yarp::os::Network is not initialized, and this is checked by the
yarp::os::NetworkBase::isNetworkInitialized()method. Secondly, callingyarp::os::Network::useNetworkClockdoes not only need the/clockport to exists, but also wait until something is published on that port and the NetworkClock becomes valid (see useNetworkClock). For this reason, callinguseNetworkClockin the Gazebo Physics thread is going to deadlock, as the Gazebo physics thread is also the one that is publish the new time on the clock. To avoid this, theyarp::os::Network::useNetworkClockis called as part of newstd::threadthat is just created for this.The PR also contains a regression test for the issue, that was failing before the fix. As the issue was quite related to the inner working of YARP, it was not use
NetworkBase::setLocalModeas in the robotinterface and controlboard tests, but rather yarpserver is actually launched before the tests by usingtiny-process-library.