search

robotology / whole-body-estimators

YARP devices that implement estimators for humanoid robots.
26 stars 12 forks source link

Add option to skip initial calibration and use zero offsets #72

Closed prashanthr05 closed 4 years ago

prashanthr05 commented 4 years ago

This PR addresses https://github.com/robotology/whole-body-estimators/issues/67.

The idl files were autogenerated due to addition of parameter skipResetFTSensorOffset startWithZeroFTSensorOffsets to idl/wholeBodyDynamicsSettings/wholeBodyDynamicsSettings.thrift. The autogenerated files are also committed along with this PR.

Since by default the parameter skipResetOffset startWithZeroFTSensorOffsets is set to false (as per the regular workflow), only configuration related to simulation was modified by this PR. devices/wholeBodyDynamics/app/wholebodydynamics-icub-external-six-fts-sim.xml

traversaro commented 4 years ago

Thanks! Minor comments: to be honest, the parameter name skipResetFTSensorOffset probably make sense to existing users that called resetOffets for thousands of times in their life, but I think it is misleading for new users, as it does suggest that the "FT Sensor offset reset" is skipped, while this option does exactly the opposite: is uses zero FT sensor offsets (so, "reset" offset) at the device startup, instead of performing calibration. For this reason, I think that calling something like startWithZeroFTSensorOffsets or similar (suggestions are welcome) would be more clear. Furthermore, also the description is not really clear: "This allows to skip manual calling of resetOffsets for removeing offsets on FT sensors". I think it would me more clear to first have a place were we state (also in the general description before this parameter) that at the beginning the device estimates the offset of the FT sensors, unless the option startWithZeroFTSensorOffsets is passed.

Furthermore note that a similar functionality (let user specify user-defined offsets, not just 0) is also provided in https://github.com/robotology/whole-body-estimators/pull/45, so we should consider if the two options can coexist (probably yes, startWithZeroFTSensorOffsets can be an easy option to set all the FT sensor offset to zero (useful in simulation), while the option https://github.com/robotology/whole-body-estimators/pull/45 can be used for more complex settings in which the offset need to be manually inserted.

@GiulioRomualdi @Giulero @gabrielenava @HosameldinMohamed

prashanthr05 commented 4 years ago

I think that calling something like startWithZeroFTSensorOffsets or similar (suggestions are welcome) would be more clear.

Agreed. I think startWithZeroFTSensorOffsets is clear. But I will wait a bit to understand if we get some inputs from other users.

Furthermore note that a similar functionality (let user specify user-defined offsets, not just 0) is also provided in #45, so we should consider if the two options can coexist (probably yes, startWithZeroFTSensorOffsets can be an easy option to set all the FT sensor offset to zero (useful in simulation), while the option #45 can be used for more complex settings in which the offset need to be manually inserted.

I revisited that PR and noticed that. Indeed, it allows the user to add an FT_OFFSET group to specify the custom offsets. I hope we will not be crowding the source code or the user experience too much by adding a redundant feature (startWithZeroFTSensorOffsets).

However, if its agreed to have both the options so that they can be used without any confusion, I will rename the parameter to what is agreed upon. Otherwise, we can close this PR.

traversaro commented 4 years ago

However, if its agreed to have both the options so that they can be used without any confusion, I will rename the parameter to what is agreed upon.

I agree that having both options make sense, especially for simulation users so that they do not need to add a complex FT_OFFSET group when they can just add a simple option.

traversaro commented 4 years ago

cc @lrapetti @diegoferigo

prashanthr05 commented 4 years ago

Agreed. I think startWithZeroFTSensorOffsets is clear. But I will wait a bit to understand if we get some inputs from other users.

I will proceed to rename the option with startWithZeroFTSensorOffsets, since there has not been any response so far.

traversaro commented 4 years ago

Approved, but check the CHANGELOG suggestion.

traversaro commented 4 years ago

Perfect!