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

Wrap IPWMControl and ICurrentControl in SWIG bindings #1966

Closed
traversaro opened this issue Feb 15, 2019 · 3 comments · Fixed by #2316
Closed

Wrap IPWMControl and ICurrentControl in SWIG bindings #1966

traversaro opened this issue Feb 15, 2019 · 3 comments · Fixed by #2316
Assignees
Labels
Component: Bindings swig, python, java, ruby, perl, octave, matlab, lua, csharp, tcl Fixed in: YARP v3.4.0 Issue Type: Feat/Enh Req This issue requests some new feature or enhancement Resolution: Fixed

Comments

@traversaro
Copy link
Member

traversaro commented Feb 15, 2019

Is your feature request related to a problem? Please describe.
The YARP interfaces for motion control need to be adapted to be used in scripting languages via SWIG, see for example how the IPositionControl or IMotorEncoders interfaces are extended.

The IPWMControl and ICurrentControl are currently wrapped in SWIG (

CAST_POLYDRIVER_TO_INTERFACE(IPWMControl)
) but they are not properly extended to expose all those methods.

Describe the solution you'd like
An implementation that expose the getNumberOfMotors and other methods that are not automatically wrapped by SWIG in a usable form, similar to what done for other interfaces.

@Nicogene Nicogene added Component: Bindings swig, python, java, ruby, perl, octave, matlab, lua, csharp, tcl Issue Type: Feat/Enh Req This issue requests some new feature or enhancement labels Feb 15, 2019
@PeterBowman
Copy link
Member

@traversaro I'm aiming to submit a patch (hopefully before the 3.4 release) targeting this issue. However, I noticed an inconsistency in the pointer-like signature of several methods.

For instance, this is how bool yarp::dev::IPositionControl::getRefSpeed(int j, double * v) is extended in order to properly translate output pointer data in languages that don't support such idiom:

yarp/bindings/yarp.i

Lines 814 to 816 in f91ceb5

bool getRefSpeed(int j, std::vector<double>& data) {
return self->getRefSpeed(j, &data[0]);
}

Here, a one-dimensional vector is expected as data. When working with Python bindings, we'd pass a yarp.DVector(1).

In contrast, this is bool yarp::dev::IEncoders::getEncoder(int j, double * v):

yarp/bindings/yarp.i

Lines 915 to 920 in f91ceb5

double getEncoder(int j) {
double data;
bool ok = self->getEncoder(j, &data);
if (!ok) return 0;
return data;
}

So, two approaches have been followed to tackle this problem, respectively:

  • Preserve boolean return status, accept a one-dimensional list or array and populate it with the expected output value.
  • Ignore the boolean status flag, reduce the number of arguments so that the desired value is returned instead, signalize failure with a dummy return value (in this case, 0).

Judging by the adoption rate in yarp.i, I guess the former is preferred. Would you agree and expect that methods from missing interfaces (in the scope of this issue and beyond) follow this approach? Are there any exceptions to this "rule", or is there any unwritten convention (I'd say the getAxes-like methods don't really need the boolean status and might as well be kept simple, i.e. return the desired value)? Regarding existing methods that embrace the latter solution, should we add the two-argument counterpart for consistency (and perhaps deprecate the old signature)?

@drdanz
Copy link
Member

drdanz commented Jul 6, 2020

@PeterBowman we are aiming at releasing YARP 3.4 at the end of the month, which means we are now in a "soft freeze" state, I think we can make an exception for bindings, but if you want this in 3.4, please submit the patch as soon as possible...

@drdanz
Copy link
Member

drdanz commented Jul 15, 2020

Fixed in master. Thanks @PeterBowman!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Bindings swig, python, java, ruby, perl, octave, matlab, lua, csharp, tcl Fixed in: YARP v3.4.0 Issue Type: Feat/Enh Req This issue requests some new feature or enhancement Resolution: Fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants