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

Follow up upstream deprecations concerning control board interfaces #180

Closed
PeterBowman opened this issue Apr 24, 2018 · 16 comments
Closed
Assignees

Comments

@PeterBowman
Copy link
Member

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
Copy link
Member Author

PeterBowman commented Apr 25, 2018

[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):

  • bool setTorquePid(int j, const Pid &pid)
  • bool setTorquePids(const Pid *pids)
  • bool setTorqueErrorLimit(int j, double limit)
  • bool setTorqueErrorLimits(const double *limits)
  • bool getTorqueError(int j, double *err)
  • bool getTorqueErrors(double *errs)
  • bool getTorquePidOutput(int j, double *out)
  • bool getTorquePidOutputs(double *outs)
  • bool getTorquePid(int j, Pid *pid)
  • bool getTorquePids(Pid *pids)
  • bool getTorqueErrorLimit(int j, double *limit)
  • bool getTorqueErrorLimits(double *limits)
  • bool resetTorquePid(int j)
  • bool disableTorquePid(int j)
  • bool enableTorquePid(int j)
  • bool setTorqueOffset(int j, double v)

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

  • bool getBemfParam(int j, double *bemf)
  • bool setBemfParam(int j, double bemf)

[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:

  • bool getMotorTorqueParams(int j, yarp::dev::MotorTorqueParameters *params)
  • bool setMotorTorqueParams(int j, const yarp::dev::MotorTorqueParameters params)

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

  • bool setRefTorques(const int n_joint, const int *joints, const double *t)

What can be done here:

  1. Require YARP 2.3.70 either at root CMakeLists.txt (ref) or focusing offending devices (see roboticslab-uc3m/openrave-yarp-plugins@18291fa).
  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
Copy link
Member Author

PeterBowman commented Apr 25, 2018

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 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.

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
Copy link
Member Author

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

@PeterBowman
Copy link
Member Author

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

@PeterBowman PeterBowman changed the title Follow up upstream changes in yarp::dev::ITorqueControl(Raw) Follow up upstream deprecations concerning IPidControl(Raw) Apr 27, 2018
@PeterBowman PeterBowman self-assigned this Apr 27, 2018
@PeterBowman PeterBowman changed the title Follow up upstream deprecations concerning IPidControl(Raw) Follow up upstream deprecations concerning control board interfaces May 17, 2018
@PeterBowman
Copy link
Member Author

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
Copy link
Member Author

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 added a commit that referenced this issue May 17, 2018
@PeterBowman
Copy link
Member Author

Done at e28a984.

@PeterBowman
Copy link
Member Author

To sum up:

  • Now, we require YARP 2.3.70 in order to compile this repository.
  • Pre-2.3.70 bits of code (e.g. header inclusions, some CMake modules now loaded by find_package(YARP)) were deleted.
  • All deprecations (as of YARP 2.3.70) have been removed, as well as other silently deleted components (setOpenLoopMode).
  • setBemfParam(Raw) and getBemfParam(Raw) will no longer exist at YARP 3.0. These have been hidden behind macro guards that check YARP_VERSION_MAJOR!=3.
  • Since current YARP devel branch (future 3.0) still uses previous major version number (i.e. 2.3.73.x), I introduced a temporary hack to fool CMake into thinking it's already 3.0. The YARP_VERSION_MAJOR macro is redefined and shadows the one generated by <yarp/conf/version.h>:
    # Hack, fool CMake to think that YARP already switched to 3.0 while still at devel.
    # https://github.com/roboticslab-uc3m/yarp-devices/issues/180
    if(NOT YARP_VERSION_SHORT VERSION_LESS 2.3.73)
    target_compile_definitions(CanBusControlboard PUBLIC YARP_VERSION_MAJOR=3)
    endif()
  • getMotorTorqueParams, setMotorTorqueParams and setRefTorques have been implemented just in the controlboard-like devices (e.g. CanBusControlboard). CAN node devices have been left out, see Derive from concrete YARP classes instead of providing custom implementation #162.
  • There is no IControlMode2 available for YARP bindings at 2.3.70, thus some examples will require 2.3.72 in order to change control mode.
  • Related to the previous point, one has to manually encode the corresponding vocab:
    mode.setControlModes(yarp.IVector(axes, yarp.Vocab_encode('vel')))

    It's never a good option to fiddle with internals like these, YARP may change the way vocabs work at any time. It has to be done this way in non-C++ code since SWIG will not wrap macro definitions. It will work though if control mode vocabs were stored in an enumeration (check VOCAB_PIXEL_...). I might start an issue/pr for this matter.

@jgvictores
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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

@jgvictores
Copy link
Member

PS: Related: roboticslab-uc3m/kinematics-dynamics#150

@jgvictores
Copy link
Member

Fix has been implemented and merged upstream, robotology/yarp#1696

@jgvictores
Copy link
Member

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

@PeterBowman
Copy link
Member Author

Related: c2023a1 (const correctness of IPositionDirect::setPositions in YARP 3.0 per robotology/yarp#1351).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants