Closed traversaro closed 4 years ago
cc @robotology/iit-dynamic-interaction-control
I saw the parameters and found it odd, but assumed those parameters were chosen after some experiments and reasoning. I actually missed the documentation saying they can be set. Do we have an idea of how fast we would have to go to actually be affected?
I saw the parameters and found it odd, but assumed those parameters were chosen after some experiments and reasoning. I actually missed the documentation saying they can be set. Do we have an idea of how fast we would have to go to actually be affected?
I think we choose the 3.0 value approximately when the iCub balancing performance were the following: https://www.youtube.com/watch?v=jaTEbCsFp_M . I do not have a precise intuition about which kind of experiments could be affected: but I would advise against using intuition here. It is much more easy to just get the measurement from an experiment, filter it with a first order low pass filter with cutoff frequency of 3.0, and see if the signal are distorted in a significant way.
I think we choose the 3.0 value approximately when the iCub balancing performance were the following: https://www.youtube.com/watch?v=jaTEbCsFp_M
I remember that back in time we did some other tests concerning other sources of noise (FT, etc). See https://github.com/robotology/codyco-modules/issues/128#issuecomment-126234394
The bug was fixed in #63. I will close this issue. Feel free to reopen it, whenever necessary.
TL;DR: Since 2016, despite what it was documented, the
wholebodydynamics
YARP device ignored the parametersimuFilterCutoffInHz
,forceTorqueFilterCutoffInHz
,jointVelFilterCutoffInHz
andjointAccFilterCutoffInHz
, and so instead used the default value of 3.0 Hertz (see https://github.com/robotology/whole-body-estimators/blob/838a66ea9778ee10c291891e6b4c7077a438a052/devices/wholeBodyDynamics/WholeBodyDynamicsDevice.cpp#L606) for all the low pass filters that filtered the inputs (imu measurements, ft measurements, joint velocity and acceleration measurements) of the torque and external force estimation algorithm.Detailed Explanation
These parameters can still be set via the RPC YARP port of wholebodydynamics, but I do not think anyone has been done this in the last few years, so in practice I think this may be affecting any "fast" experiments that used the low-level torque control, for example fast YOGA or torque-based walking.
Proposed Solution
Actually implement what is written in the docs (see https://github.com/robotology/whole-body-estimators/blob/838a66ea9778ee10c291891e6b4c7077a438a052/devices/wholeBodyDynamics/WholeBodyDynamicsDevice.h#L125) and make these parameters compulsory, so users can find these parameters in the
wholebodydynamics.xml
file and be immediately aware that filtering is done. Just setting a default value in the code if no parameter is provided is hiding this logic too much.Post mortem
Why this happened? I checked a bit the original commits, and the commit in which the documentation about the (actually not implemented) parameter was added is https://github.com/robotology/codyco-modules/commit/788b4918125553f8b0b27b2719cd6a53603139da .
That commit seems to be some kind of compilation hot-fix, in which accidentally I also included the change in the documentation (probably it was a work in progress, and I accidentally added it with
git commit -a
or a carelessgit add *
. I think that teaches us a good lesson: no matter how small a fix seems to be, always do it through a Pull Request, so at least you have an occasion to self-review it yourself when you merge the PR. In this case, if I had opened a Pull Request, even without an external reviewer I would have probably noticed the wrong documentation I was adding to the repo.