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

Improve yarpmotorgui open speed #2911

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

S-Dafarra
Copy link
Contributor

During the XPrize finals, we noticed that the yarpmotorgui can take some time to start.

I check the code and I found that there are some checks on whether there are other yarpmotogui opened. See for example

Contact adr=Network::queryName(nameToCheck.toLatin1().data());

My guess is that this call can be costly in case of a delayed network.

Moreover, there was a useless call to get each joint position PID:

Pid myPid(0,0,0,0,0,0);
yarp::os::SystemClock::delaySystem(0.005);
m_iPid->getPid(VOCAB_PIDTYPE_POSITION, k, &myPid);

Finally, there are some delays to make sure that the reading of the joint values is valid:

bool ret = false;
SystemClock::delaySystem(0.050);
do {
ret = m_iencs->getEncoders(m_positions);
if (!ret) {
yError("%s iencs->getEncoders() failed, retrying...\n", partName.toLatin1().data());
SystemClock::delaySystem(0.050);
}
} while (!ret);
yInfo("%s iencs->getEncoders() ok!\n", partName.toLatin1().data());

Hence, with this PR:

  • I have added a dummy port such that the check to avoid conflicts is done only once, instead of multiple times per part.
  • I have removed the useless PID request.
  • I have parallelized the opening of the different parts using std::async

I have measured the initialization time before and after when opening yarpmotorgui with the robot on Gazebo in my PC (so all local communication), and the improvement was more than half a second.

This is to avoid expensive checks with the nameserver for every part we open
The parts are still parsed in order, but while waiting for one part, the others can configure
@S-Dafarra S-Dafarra requested a review from randaz81 as a code owner December 9, 2022 16:17
@update-docs
Copy link

update-docs bot commented Dec 9, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:19 — with GitHub Actions Inactive
@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:19 — with GitHub Actions Inactive
@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:19 — with GitHub Actions Inactive
@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:42 — with GitHub Actions Inactive
@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:42 — with GitHub Actions Inactive
@S-Dafarra S-Dafarra temporarily deployed to code-analysis December 9, 2022 16:42 — with GitHub Actions Inactive
@S-Dafarra
Copy link
Contributor Author

In agreement with @randaz81, I will try to run one test also on Windows and eventually Mac to make sure that the new multithreaded behavior is not causing issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants