roboticslab-uc3m / yarp-devices

A place for YARP devices
https://robots.uc3m.es/yarp-devices/
9 stars 7 forks source link

Follow up upstream deprecations concerning control board interfaces #180

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

Several methods have been removed in favour of the yarp::dev::IPidControl interface (stable since YARP 2.3.70). Starting from YARP 3.0.0, yarp::dev::ITorqueControl will lack several PID-related methods, which is currently breaking our cron builds at YARP's devel branch (example).

Suggested steps:

PeterBowman commented 6 years ago

[1] Deprecated in YARP 2.3.70 (despite what that inline comment says...) in favor of the yarp::dev::IPidControl interface, removed in current devel (3.0.0):

[2] Removed without any prior advice at current YARP devel (3.0.0):

[3] Not implemented in our devices, YARP provides a trivial implementation for backward compatibility (just a return false;) and seemingly designed to replace the last two methods:

[4] Also not implemented by us, trivial implementation in upstream header file:


What can be done here:

  1. Require YARP 2.3.70 either at root CMakeLists.txt (ref) or focusing offending devices (see https://github.com/roboticslab-uc3m/openrave-yarp-plugins/commit/18291fab2eacaf843f03393520af40b766b2d097).
  2. Remove [1].
  3. Either leave [2] as they are, even if not used anymore starting from YARP 3.0.0 (I'd put a comment there to explain the status quo), or use deprecation macros just like YARP itself does and avoid compiling those methods from 3.0.0 onwards.
  4. Implement [3] and [4], just in case.

All of this applies to the set of ...Raw methods as well. Keep in mind that YARP 3.0.0 is still unstable and may undergo changes at any time before the final release.

PeterBowman commented 6 years ago

Require YARP 2.3.70 either at root CMakeLists.txt

I'd rather go this way, but ASIBOT still has 2.3.68 (at least until I apply the solution mentioned at https://github.com/roboticslab-uc3m/asibot-main/issues/42#issuecomment-383283868) and the protocol changed, which means that two distinct YARP versions would need to run on the same PC: for daily work (e.g. yarp-devices) and just for talking to ASIBOT via remote CB. A temporary, per-device solution seems less invasive.

Either leave [2] as they are, even if not used anymore

Reminds me of the old open loop control mode, also deleted without the usual deprecation process. We ended up leaving a dummy setOpenLoopMode(Raw) definition (example) and some ugly commented-out bits (example). Despite looking a bit noisy, I prefer to hide unused methods behind preprocessor clauses.

PeterBowman commented 6 years ago

PS if ever requiring YARP 2.3.70 or later, safely remove all occurrences: grep -ri openloop.

PeterBowman commented 6 years ago

Same deprecation process has been applied to yarp::dev::IVelocityControl2(Raw) (velocity PID setters/getters). Updating title.

PeterBowman commented 6 years ago

Updating title, several other interfaces have been modified as a result of long planned deprecations. This covers all set<Whatever>Mode(int) as setPositionMode(int) in IPositionControl.

PeterBowman commented 6 years ago

I'd rather go this way, but ASIBOT still has 2.3.68 (at least until I apply the solution mentioned at roboticslab-uc3m/asibot-main#42 (comment)) and the protocol changed, which means that two distinct YARP versions would need to run on the same PC: for daily work (e.g. yarp-devices) and just for talking to ASIBOT via remote CB. A temporary, per-device solution seems less invasive.

As spoken with @jgvictores, we'll set 2.3.70 as the minimum required YARP version.

PeterBowman commented 6 years ago

Done at https://github.com/roboticslab-uc3m/yarp-devices/commit/e28a984e8eb05b74efaece9619dbf066e1f915dd.

PeterBowman commented 6 years ago

To sum up:

jgvictores commented 6 years ago

Related to the previous point, one has to manually encode the corresponding vocab

Pretty convinced the elegant way would be to implement VOCAB in yarp.i.

PeterBowman commented 6 years ago

Not sure how to achieve this. I tested simple .i files and a few macros scattered around header files, everything gets exported except macro functions. That is, VOCAB_CM_POSITION and the like should be wrapped (VOCAB and VOCABX not, for instance), but actually they aren't.

jgvictores commented 6 years ago

Same here. In YARP, things like #define BOTTLE_TAG_INT 1 and #define BOTTLE_TAG_VOCAB (1 + 8) are wrapped. Info: http://www.swig.org/Doc3.0/Scripting.html#Scripting_nn3 (4.2.3).

Been playing around with lying to SWIG (haven't got typedef mechanism to work), will inform after having some results. This for reference: https://stackoverflow.com/questions/11403520/preprocessor-macro-in-swig

jgvictores commented 6 years ago

PS: Regarding https://github.com/robotology/yarp/pull/1655 it seems there would have been more elegant ways (avoiding the second Vocab.h inclusion, or via %ignore).

jgvictores commented 6 years ago

PS: Related: https://github.com/roboticslab-uc3m/kinematics-dynamics/issues/150

jgvictores commented 6 years ago

Fix has been implemented and merged upstream, https://github.com/robotology/yarp/pull/1696

jgvictores commented 6 years ago

PS: requires SWIG >= 3.0.11 (Xenial comes with 3.0.8).

PeterBowman commented 5 years ago

Related: https://github.com/roboticslab-uc3m/yarp-devices/commit/c2023a1629cfd77c9d09f674c32cd7c4fbf25edc (const correctness of IPositionDirect::setPositions in YARP 3.0 per https://github.com/robotology/yarp/issues/1351).