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

[iCubGenova09] Increase the speed of the low level (WBD 500Hz) #20

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Aug 23, 2022

This PR decreases the period of the WBD from 0.01 s to 0.002s. The PR also increases the rate of the hardware devices and the network wrapper.

This comment will first details the profiling analysis and then I will analyze each parameter that I modified

Time profiling

WBD profiling

Before entering into the details of the PR I would like to show you the time profiling result of WBD. The analysis has been performed by exploiting the timers I implemented in robotology/whole-body-estimators#142

The following table contains the average time (the average time window is equal to 1 minute). The labels publish computed forces etc. refer to the different sections in the main loop in robotology/whole-body-estimators@d49bdd1 The label all indicates the time spent to perform the entire loop.

name time average (s) number of deadline miss time at deadline miss (s)
publish 0.0004850322333 0 0
compute forces 0.0003449385246 0 0
calibration 5.510801666667e-07 0 0
kinematics 3.934341346667e-05 0 0
remove offset 8.4732465e-06 0 0
contact points 2.486420513333e-05 0 0
read sensors 4.683991183333e-05 0 0
all 0.0009521654851667 1 0.002652847

Accordingly to the profiling, WBD is able to handle this frequency 🚀

Board messages

The boards stream sometimes the following warning. However, the robot seems working as expected.

[WARNING]  from BOARD 10.0.1.11 (right_leg-eb11-j0_2), src LOCAL, adr 0, time 1664s 41m 268u: (code 0x0000000d, par16 0x012e par64 0x00010093003e01e0) -> SYS: the TX phase of the control loop has last more than wanted. In par16: TX execution time [usec]. In par64: num of tx can2 and can1 frames and latest previous execution times of TX, RX, DO [usec] + .

yarprobotinterface.txt contains the log of the yarprobotinterface. Perhaps @traversaro or @S-Dafarra have some hints.

PR analysis

  • iCubGenova09/estimators/wholebodydynamics.xml
    • <param name="devicePeriodInSeconds">0.002</param> increase the frequency of wbd from the default value 100Hz to 500Hz
  • iCubGenova09/hardware/FT/left_arm-eb1-j0_1-strain.xml
    • I decreased the tfPeriod from 10 ms to 2ms (500Hz)
  • iCubGenova09/hardware/electronics/face-eb23-j2_5-eln.xml
    • I decreased the TXrateOfRegularROPs from 3 (or 5) to 2 (500Hz here you can find some documentation)
  • iCubGenova09/hardware/inertials/head-inertial.xml
    • I decreased the acquisitionRate from 10 to 5 (200Hz)
  • iCubGenova09/hardware/inertials/left_arm-eb1-IMU.xml
    • I decreased the acquisitionRate from 50 to 5 (200Hz). I noticed that all the inertials attached to the FT were running at 20Hz before this PR
  • iCubGenova09/wrappers/FT/left_arm-FT_wrapper.xml
    • I decreased the period from 10 ms to 2ms (500Hz)
  • iCubGenova09/wrappers/inertials/left_foot-IMU_wrapper.xml
    • I decreased the period from 10 ms to 2ms (500Hz)
  • iCubGenova09/wrappers/motorControl/left_arm-mc_wrapper.xml
    • I decreased the period from 0.02 s to 0.002s (500Hz). It is important to notice that 0.02 s is the default value of the controlBoard_nws_yarp device so before this PR the rate was 50Hz.

This substitutes #10

@GiulioRomualdi GiulioRomualdi self-assigned this Aug 23, 2022
@GiulioRomualdi
Copy link
Member Author

cc @S-Dafarra @isorrentino @DanielePucci

Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR!

The comments are related to a set of changes that in my opinion is not straightforward. If you happen to have some cross-reference that indicates why those changes are needed, it could be useful for future reference.

@S-Dafarra
Copy link
Member

Also, did you notice any error or warning in the logger regarding timeouts?

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Aug 23, 2022

Also, did you notice any error or warning in the logger regarding timeouts?

I'm going to check it on the robot right now

@GiulioRomualdi GiulioRomualdi force-pushed the devel_iCubGenova09_high_freq branch from 763cff1 to 00b7c42 Compare August 23, 2022 14:04
@GiulioRomualdi
Copy link
Member Author

@S-Dafarra @traversaro I updated the comment #20 (comment)

@S-Dafarra
Copy link
Member

@pattacini could we ask for your support to make sure that these modifications are:

  • increasing the frequency of IMUs to 100Hz
  • all MAS interfaces for the FTs go at 500Hz
  • all control boards go at 500Hz
  • the modifications to the electronics folder are coherent with the above modifications. This is where we are more in doubt

@DanielePucci
Copy link
Member

CC @traversaro

@pattacini
Copy link
Member

Hi @S-Dafarra

I'll be checking that tomorrow with the fixers.

@@ -43,7 +43,7 @@
</group>

<group name="SETTINGS">
<param name="acquisitionRate"> 10 </param>
<param name="acquisitionRate"> 5 </param>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I got it right, it seems that 10 is the fastest allowed rate as per https://github.com/robotology/icub-main/blob/master/src/libraries/icubmod/embObjInertials/embObjInertials.cpp#L527-L538.

Have you seen the following warning message popping up?

embObjInertials::sendConfig2MTBboards() has detected a wrong acquisition rate =...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this device is of type embObjIMU, so I am not sure if any limit like that exists.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I got it right, it seems that 10 is the fastest allowed rate as per https://github.com/robotology/icub-main/blob/master/src/libraries/icubmod/embObjInertials/embObjInertials.cpp#L527-L538.

Have you seen the following warning message popping up?

embObjInertials::sendConfig2MTBboards() has detected a wrong acquisition rate =...

The 100 Hz frequency (acquisitionRate = 10 ms) is the maximum frequency that we can squeeze from the IMU Bosch BNO055. And that is due to HW limitation, in particular to the way the chip replies over the used I2C bus. We thoroughly explored this limitation some time ago.

So, we should correct the value to be 10.

On the other hand, in here the intention is to

  • increasing the frequency of IMUs to 100Hz

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this device is of type embObjIMU, so I am not sure if any limit like that exists.

Regarding this, the function in question is then at https://github.com/robotology/icub-main/blob/26c74355c9d6463762352f41f8f1a1df593844ca/src/libraries/icubmod/embObjIMU/eo_imu_privData.cpp#L314-L340.

Copy link

@marcoaccame marcoaccame Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this device is of type embObjIMU, so I am not sure if any limit like that exists.

@traversaro: thanks for it. I checked and device embObjIMU does not check vs incorrect values of acquisitionRate as the original device embObjInertials did. Maybe in the migration towards the new device something was missed.

But the FW indeed does the check and clips values in range [5, 200] ms. The lower possible value is 5 because in some cases (e.g., when the board asks via I2C only one or two values such accel and gyro) 5 ms can be OK. But again, 10 ms is the recommended lower value that we should use. Also because the fusion algorithm inside the BNO055 chip produces new values only every 10 ms.

@@ -3,7 +3,7 @@


<device xmlns:xi="http://www.w3.org/2001/XInclude" name="head-imuFilter" type="imuFilter">
<param name="period"> 20 </param>
<param name="period"> 2 </param>
Copy link

@marcoaccame marcoaccame Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardware section instructs boards to retrieve new IMU values every 10 ms.

So, the best period of the wrapper is 10 ms. To have 2 ms is not wrong, but it may stress the system more than we need.

This comment applies to all the wrappers inside folder inertials.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Sep 29, 2022

Thank you @marcoaccame! I'm going to apply your suggestions See e2201aa and 1ce2566

@marcoaccame
Copy link

Thank you @marcoaccame! I'm going to apply your suggestions See e2201aa and 1ce2566

OK, now the PR now seems fine by me.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Sep 30, 2022

I will squash all the commits and I will test it on the robot. If everything goes smooth we can merge it

@GiulioRomualdi GiulioRomualdi force-pushed the devel_iCubGenova09_high_freq branch from 1ce2566 to 8b98ecd Compare September 30, 2022 08:43
- increasing the frequency of IMUs to 100Hz
- all MAS interfaces for the FTs go at 500Hz
- all control boards go at 500Hz
@GiulioRomualdi GiulioRomualdi force-pushed the devel_iCubGenova09_high_freq branch from 8b98ecd to 274612c Compare September 30, 2022 12:44
@S-Dafarra S-Dafarra merged commit 6d58d6f into devel_iCubGenova09 Oct 4, 2022
@GiulioRomualdi GiulioRomualdi deleted the devel_iCubGenova09_high_freq branch February 14, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants