ITorqueControl::setTorqueOffset should be deprecated or properly defined?

[traversaro]

I noticed that there is a function ITorqueControl::setTorqueOffset present in the ITorqueControl that is not implemented in any icub device.

Furthermore its role is not discussed in the ControlMode documentation, where there is a t_off value used for gravity compensation in impedance, that is however not used in torque control and is the one setted using yarp::dev::IImpedanceControl::setImpedanceOffset.

@randaz81 @barbalberto I guess this method is just a left over, and is not used anywhere, right?

[barbalberto]

Good point finding out that no iCub device implements the setTorqueOffset, but please keep in mind that YARP is not iCub, for example walkman device is implementing it.

Yarp is a middleware meant to be used by many robots, each one with their own specific behaviour.
Right now yarp is used by iCub, cer, coman, walkman and probably other robots out there so I think we cannot use a specific case to deprecate methods already public and stable.

yarp::dev::IImpedanceControl::setImpedanceOffset may be used to compensate gravity as the same way torqueOffset does, but they will be equivalent only if the robot is in impedance mode. If the robot is controlled in torque mode then setImpedanceOffset cannot be used and I think the setTorqueOffset will still play its role, but maybe @randaz81 has a better knowledge here.

The document says:

In all control modes, an additional feed-forward input, Phi_off is available.

So in my understanding each controlmode shall have a feedforward methods in its interface: for the torque control mode i think the setTorqueOffset does this job.

IMHO the methods fits perfectly in the interface and still make sense. Same algorithms may leverage on it, for example an user can have a rfModule devoted only to compute gravity compensation and another one for torque/impedance control. In this case having 2 methods is needed.

I think the real point here is: if iCub does not implement this feature shall we implement it?
Shall the iCub sw use this feedforward method?

[traversaro]

Good point finding out that no iCub device implements the setTorqueOffset, but please keep in mind that YARP is not iCub, for example walkman device is implementing it.

Yarp is a middleware meant to be used by many robots, each one with their own specific behaviour.
Right now yarp is used by iCub, cer, coman, walkman and probably other robots out there so I think we cannot use a specific case to deprecate methods already public and stable.

I totally agree, I was just checking all the devices that I know of and that are available as open source (that even include non-IIT robots such as Teo and Vizzy, for example). I did not include Teo to the discussion because I did not want to be too verbose, but I checked and setTorqueOffset is not implemented even there. If the coman/walkman were available as open source, I would definitely check them, but apparently I can't not even find them anymore in the Robotology Gitlab. Opening an issue is meant exact to discuss those details, and not to assume that iCub is the only robot using YARP.

In all control modes, an additional feed-forward input, Phi_off is available.

So in my understanding each controlmode shall have a feedforward methods in its interface: for the torque control mode i think the setTorqueOffset does this job.

The setTorqueOffset as the name seems to imply and as you describe it (i.e. for use in gravity compensation) is an offset in the desired torque (i.e. a quantity expressed in Nm), while Phi_off is an offset on the PID output, i.e. a current/PWM depending on low level PID implementation. A torque offset as you describe it could make sense, but is not the Phi_off as described in the ControlMode documentation.

[barbalberto]

Walkman inplementation is here:
https://gitlab.robotology.eu/walkman-drc/walkmanmod/blob/master/deviceDriver/YARPInterfacesImplementation/walkmanTorqueControlRaw.cpp

Phi_off is an offset on the PID output, i.e. a current/PWM depending on low level PID implementation. A torque offset as you describe it could make sense, but is not the Phi_off as described in the ControlMode

You are right, maybe the Phi_off is not the right example.

My point is that the interface is an abstraction layer which expresses a concept/functionality of what a torque control is and it has to be agnostic about the hardware implementations.

I find that, when speaking about interfaces, it is more clear thinking with a top-down approach (from the concept to header file/the list of functions) rather than bottom-up (from hw to user).

I think an interface have to be omni-comprehensive and offer all the services the user may need, so as long as the methods conceptually belong to the purpose of the interface and make sense in it, then that method has the right to stay there.

However if by design the yarp::dev::IImpedanceControl::setImpedanceOffset. and setTorqueOffset are meant to do the same thing, and this is perceived as confusing, we can modify the design to have it more clear. For example by using the same name in both interfaces. If, instead, they are meant to do different things, then personally I believe that it is not used is not a reason to remove it.
Maybe in the future some will use it.

But this is just my personal opinion.

[traversaro]

Walkman inplementation is here:
https://gitlab.robotology.eu/walkman-drc/walkmanmod/blob/master/deviceDriver/YARPInterfacesImplementation/walkmanTorqueControlRaw.cpp

Great!

However if by design the yarp::dev::IImpedanceControl::setImpedanceOffset. and setTorqueOffset are meant to do the same thing, and this is perceived as confusing, we can modify the design to have it more clear. For example by using the same name in both interfaces. If, instead, they are meant to do different things, then personally I believe that it is not used is not a reason to remove it.
Maybe in the future some will use it.

The main problem is that (at the moment) the semantics of the setTorqueOffset is not defined, so we can't say that is "meant to do". Checking real implementations of a undocumented interface (in this case in iCub) was just a way to try to understand if there was a de-facto consensus among users about the semantics of that method. Given that no such consensus exists (iCub ignores the call, while Walkman uses it) we should try to define the semantics of the method, or just deprecated it.

[traversaro]

I modified the title to make clear that we are open to any solution. :D

[barbalberto]

👍
@randaz81 any comment on this?

[barbalberto]

I spoke briefly with @randaz81.
What follows is just the history, not a proposal.

The 2 methods ITorqueControl::setTorqueOffset and IImpedanceControl::setImpedanceOffset are actually meant to do the same thing: sending a torque offset.
Now there are 2 different methods because the implementation maybe different depending of the firmware implementation.

In the case of IImpedanceControl::setImpedanceOffset this torque value is the only torque reference to be sent to the hw, so it is straightforward. The other reference is a position one so threre is no conflict between the two and the torque offset can be sent 'as is' without any change.

In the case of ITorqueControl::setTorqueOffset instead, if the firmware does not provide a specific message for the torque offset (this is the iCub case), then this offset needs to be summed up to the last reference and sent as the new reference.

To make the implementation easier a for each specific use case were provided.