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

Unintended change of frequency of iCub's controlBoard_nws_yarp instances from 100 Hz to 50 Hz #378

Closed
traversaro opened this issue Aug 30, 2022 · 4 comments · Fixed by #379
Assignees
Labels

Comments

@traversaro
Copy link
Member

Historically, iCub's Network Wrapper Servers (i.e. the YARP devices that publish informations over YARP ports) have run at 100 Hz. I may be wrong, but it seems to me that in the latest NWS/NWC layers migration (see #321 and subsequent PRs), we switched to use 50 Hz frequency. It seems that this happend because we removed all the <param name="period">10</param> parameters, meaning that now we rely on the default period of controlBoard_nws_yarp, that is 1/0.02 = 50 Hz, see https://github.com/robotology/yarp/blob/v3.7.2/src/devices/ControlBoardWrapper/ControlBoard_nws_ros.h#L41 .

@pattacini
Copy link
Member

pattacini commented Aug 31, 2022

Quite an important change that went on unseen, actually.
Thanks @traversaro for the nice catch.

I would ask @randaz81 to comment on this.

For the time being, the quick fix on e.g. iCub 3 could be to make the NWS explicitly run at 100 Hz resorting to the proper parameters in the config files.
Remember that periods are now in expressed seconds (i.e., 0.01) and no longer milliseconds.

cc @davidetome

@pattacini
Copy link
Member

pattacini commented Aug 31, 2022

We started from iCubGenova01 and iCubGenova09 and spread the changes to the other robots.
Apparently, both iCubGenova01 and iCubGenova09 no longer expose the parameter period explicitly.

@pattacini pattacini added the bug label Aug 31, 2022
@pattacini
Copy link
Member

@Nicogene is on it.
The idea is to update the robots' config files to set the period explicitly.

@traversaro
Copy link
Member Author

Note that the problem was originally spotted by @GiulioRomualdi .
fyi @S-Dafarra

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

Successfully merging a pull request may close this issue.

3 participants