-
Notifications
You must be signed in to change notification settings - Fork 38
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
Introduce Advanceable Runner and SharedResource #272
Conversation
Ubuntu 20.04 debug + Valgrind is failing. I think it is related to https://stackoverflow.com/questions/5610677/valgrind-memory-leak-errors-when-using-pthread-create |
fe93321
to
1967ad7
Compare
auto function = [&]() -> bool { | ||
while (this->m_isRunning) | ||
{ | ||
const auto wakeUpTime = BipedalLocomotion::clock().now() + m_dT; |
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.
Just FYI, this kind of implementation may lead to a drift in the execution (because the old wakeUpTime
is not equal to BipedalLocomotion::clock().now()
) that could problematic when doing data collection, see robotology/yarp#2515 . I think that @isorrentino already experienced something similar in one of her issues.
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.
Do you have any suggestions to avoid this problem?
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 understood the problem! This thread is not periodic if there are deadline misses, right?
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.
auto start = BipedalLocomotion::clock().now();
foo();
auto end = BipedalLocomotion::clock().now();
BipedalLocomotion::clock().sleepFor(m_dT - (end - start));
may solve the problem, right?
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.
or simply
wakeUpTime += m_dt:
But this may lead to a problem if the clock is reset
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.
If I'm not mistaken @Giulero proposed something in gazebo yarp plugins
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.
On detecting clock reset and handling them, you can also check robotology/yarp#2494 and robotology/yarp#800 .
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.
If I'm not mistaken @Giulero proposed something in gazebo yarp plugins
Sorry, I sent my reply but I did not saw yours, indeed my links are related to that.
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.
Hi @traversaro here I tried to implement what you suggested while taking into account possible time reset e91d6ad
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.
Great, it seems ok!
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.
Minor comment.
1967ad7
to
e91d6ad
Compare
This is the latest block missing for #260.
Related PR: #256