Add bindings for Multiple Analog Sensors interfaces #1756

nunoguedelha · 2018-06-18T09:23:20Z

Motivation

After the integration of the multiple analog sensors interfaces and respective devices (refer to #1526, #1586, #1676) we need to access those interfaces from scripting languages like Matlab, and so, create the respective bindings.
On top of that, some redundant code is cluttering the SWIG interface file yarp.i, which can be avoided by using the appropriate macros.

Description

Added bindings for Multiple Analog Sensors interfaces :

minor refactoring of main header MultipleAnalogSensorsInterfaces.h defining the interfaces
added main header to yarp/dev/all.h for a proper inclusion of the interfaces definitions in the mex source files yarpXXX_wrap.cxx
include main interfaces header in yarp.i and wrapped the cast functions (PolyDriver*)->view(<sensor_interface*>) for each one of the interfaces
extended the new interfaces for wrapping the methods which Swig struggles to handle

Did some refactoring in the SWIG interface file `yarp.i` for reducing redundant code :

all wrappers for casting from PolyDriver to specific device interfaces are now generated through a Swig macro.
all new class extensions are done using a Swig macro defined in macrosForMultipleAnalogSensors.i
refactored the from/toMatlab() wrappers and extended them to the classes : std::vector<double>, std::vector<bool>, std::vector<int>.

(std::vector<std::string> is a bit more tricky and was left out for now.)

Update the changelog

nunoguedelha · 2018-06-18T09:23:49Z

CC @traversaro @fjandrad

nunoguedelha · 2018-06-18T09:28:27Z

@traversaro , I'm still completing the last point (impacting IVector_fromTo_matlab.i), but you can already review the other changes in the current commits.

Nicogene · 2018-06-18T10:39:25Z

Please add few lines in the changelog

Nicogene · 2018-06-19T08:23:22Z

Please rebase over the latest devel and resolve the conflict, thanks!

traversaro · 2018-06-19T09:03:12Z

By the way, the conflicts are probably due to #1757 , i.e. CAST_POLYDRIVER_TO_INTERFACE needs to be used also with the IInterfaceControl.

jgvictores · 2018-06-19T10:10:08Z

@nunoguedelha CAST_POLYDRIVER_TO_INTERFACE looks really cool, I believe it's something for which I couldn't come up with a solution within #1607. Thanks!

Regarding the files added within the PR, I'd like to share a small concern.

#698 was open due to a new file bindings/things.i (included by distributed yarp.i) being added to YARP but not distributed within the YARP Windows binary installer. This was fixed by the removal of bindings/things.i.

This PR adds bindings/macrosForMultipleAnalogSensors.i and bindings/matlab/vectors_fromTo_matlab.i (both for inclusion by yarp.i). Does anybody (@mbrunettini @traversaro @drdanz ?) know how/where the YARP Windows binary installer packaging is done to avoid reaching the same situation?

traversaro · 2018-06-19T10:41:47Z

Regarding the files added within the PR, I'd like to share a small concern.

I am not 100% sure, but I guess it should be sufficient to add all the files that we want to install to https://github.com/robotology/yarp/blob/devel/bindings/CMakeLists.txt#L91 . All the installation logic should be there, I would expect the Windows installer to just collect what is installed by the CMake install target and put in the installer.

PeterBowman · 2018-06-19T11:12:11Z

Does anybody (...) know how/where the YARP Windows binary installer packaging is done to avoid reaching the same situation?

@jgvictores see robotology/yarp-packaging. Per /windows/README.txt:

(...) Everything is built with CMake, so you can always search for the relevant *.sln file and open it to build manually.

- small refactoring of main header MultipleAnalogSensorsInterfaces.h defining the insterfaces - added main header to yarp/dev/all.h for a proper inclusion of the interfaces definitions in the mex source files yarpXXX_wrap.cxx - include main interfaces header in `yarp.i` and wrapped the cast functions `(PolyDriver*)->view(<sensor_interface*>)` for each one of the interfaces - extended the interface `IThreeAxisGyroscopes` for wrapping the methods hard to handle by Swig

- all extensions are done as for `IThreeAxisGyroscopes`, and using a Swig macro defined in `macrosForMultipleAnalogSensors.i` - all wrappers for casting from PolyDriver to specific device interfaces are now generated through a Swig macro.

Completed the extension of the classes std::vector<double>, std::vector<bool>, std::vector<int>, yarp::sig::Vector with these wrappers.

nunoguedelha · 2018-06-19T14:39:25Z

@traversaro @PeterBowman Thanks for the clarifications.

Nicogene

LGTM! I will merge it as soon as the test passes, thanks!

jgvictores · 2018-06-19T15:56:19Z

Should https://github.com/nunoguedelha/yarp/blob/feature/addMultipleAnalogSensorsBindings/bindings/CMakeLists.txt#L91 be changed from:

foreach(f CMakeLists.txt yarp.i README.md)

to:

foreach(f CMakeLists.txt yarp.i README.md macrosForMultipleAnalogSensors.i matlab/vectors_fromTo_matlab.i)

or similar?

Nicogene · 2018-06-20T07:15:43Z

So is it needed an additional change in bindings/CMakeLists.txt before merging right?

jgvictores · 2018-06-20T07:23:23Z

That's what I've understood from the comments above. : )

traversaro · 2018-06-20T08:18:44Z

Yep, @nunoguedelha can you do the modification?

…from the build material (typically for such files to be found by the windows installer)

nunoguedelha · 2018-06-20T13:08:49Z

Sure, done.

Nicogene · 2018-06-23T08:12:47Z

Merged, thanks!

jgvictores · 2018-06-29T16:38:08Z

After some thought on #1776, pretty sure this will suffer from matlab/vectors_fromTo_matlab.i losing the matlab/ path, and will therefore not be found by yarp.i. Commenting here as this (matlab/vectors_fromTo_matlab.i) has been merged to devel, whereas #1776 affects master.

PeterBowman · 2018-08-02T19:05:12Z

After some thought on #1776, pretty sure this will suffer from matlab/vectors_fromTo_matlab.i losing the matlab/ path...

Confirming that vectors_fromTo_matlab.i is installed into <prefix>/share/yarp/bindings/ while <prefix>/share/yarp/bindings/matlab/ is expected.

...and will therefore not be found by yarp.i.

Can't test it by myself, but I'm curious about the following lines (introduced by this PR):

yarp/bindings/yarp.i

Lines 452 to 455 in 4bca59c

    
           #ifdef SWIGMATLAB 
        
             // Extend IVector for handling conversion of vectors from and to Matlab 
        
             %include "matlab/vectors_fromTo_matlab.i" 
        
           #endif

There is no SWIGMATLAB macro listed in the SWIG docs. How does it actually get defined?

nunoguedelha · 2018-08-02T22:37:29Z

Confirming that vectors_fromTo_matlab.i is installed into /share/yarp/bindings/ while /share/yarp/bindings/matlab/ is expected

@PeterBowman @jgvictores this is indeed a real issue and is being addressed. The bindings used to be generated using directly the yarp.i interface file from the source directory. The generation process is being improved and will use the file from the install directory (robotology/yarp-matlab-bindings#35), and along with this, the issue on vectors_fromTo_matlab.i path will be fixed.

There is no SWIGMATLAB macro listed in the SWIG docs. How does it actually get defined?

this flag is defined directly in the Mex library source file yarp-matlab-bindings/matlab/autogenerated/yarpMATLAB_wrap.cxx generated by SWIG.

traversaro · 2018-08-03T05:57:44Z

The issue mentioned by @PeterBowman is blocking robotology/yarp-matlab-bindings#35 .

nunoguedelha requested review from barbalberto, drdanz, randaz81 and traversaro as code owners June 18, 2018 09:23

Nicogene added the PR Status: Rebase - Not OK This PR needs to be rebased over the latest commit in the target branch label Jun 19, 2018

nunoguedelha added 3 commits June 19, 2018 16:18

Refactored and completed the from/toMatlab() wrappers

a71047a

Completed the extension of the classes std::vector<double>, std::vector<bool>, std::vector<int>, yarp::sig::Vector with these wrappers.

nunoguedelha force-pushed the feature/addMultipleAnalogSensorsBindings branch from f8260a7 to a71047a Compare June 19, 2018 14:38

Updated changelog v3_1_0.md

d041949

nunoguedelha changed the title ~~WIP: Add bindings for Multiple Analog Sensors interfaces~~ Add bindings for Multiple Analog Sensors interfaces Jun 19, 2018

traversaro approved these changes Jun 19, 2018

View reviewed changes

Nicogene added PR Status: Changelog - OK This PR has a proper changelog entry PR Status: Rebase - OK This PR does not need to be rebased and removed PR Status: Changelog - Not OK This PR does not have a changelog entry, or the changelog needs to be fixed labels Jun 19, 2018

Nicogene removed the PR Status: Rebase - Not OK This PR needs to be rebased over the latest commit in the target branch label Jun 19, 2018

Nicogene approved these changes Jun 19, 2018

View reviewed changes

Add swig input files included by yarp.i, for bindings to be compiled …

389ca5b

…from the build material (typically for such files to be found by the windows installer)

Nicogene merged commit 13121aa into robotology:devel Jun 23, 2018

Nicogene added the Resolution: Merged label Jun 23, 2018

nunoguedelha deleted the feature/addMultipleAnalogSensorsBindings branch June 23, 2018 18:00

Nicogene mentioned this pull request Jun 25, 2018

Java and C# bindings extended analog sensor interfaces do not compile. #1770

Open

nunoguedelha mentioned this pull request Jul 8, 2018

Activating and using the new Multiple Analog Sensors Interfaces implemented in YARP robotology-playground/sensors-calib-inertial#18

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bindings for Multiple Analog Sensors interfaces #1756

Add bindings for Multiple Analog Sensors interfaces #1756

nunoguedelha commented Jun 18, 2018 •

edited

Loading

nunoguedelha commented Jun 18, 2018 •

edited

Loading

nunoguedelha commented Jun 18, 2018

Nicogene commented Jun 18, 2018

Nicogene commented Jun 19, 2018

traversaro commented Jun 19, 2018

jgvictores commented Jun 19, 2018

traversaro commented Jun 19, 2018

PeterBowman commented Jun 19, 2018

nunoguedelha commented Jun 19, 2018

Nicogene left a comment

jgvictores commented Jun 19, 2018

Nicogene commented Jun 20, 2018 •

edited

Loading

jgvictores commented Jun 20, 2018

traversaro commented Jun 20, 2018

nunoguedelha commented Jun 20, 2018

Nicogene commented Jun 23, 2018

jgvictores commented Jun 29, 2018 •

edited

Loading

PeterBowman commented Aug 2, 2018

nunoguedelha commented Aug 2, 2018

traversaro commented Aug 3, 2018

Add bindings for Multiple Analog Sensors interfaces #1756

Add bindings for Multiple Analog Sensors interfaces #1756

Conversation

nunoguedelha commented Jun 18, 2018 • edited Loading

Motivation

Description

Added bindings for Multiple Analog Sensors interfaces :

Did some refactoring in the SWIG interface file yarp.i for reducing redundant code :

nunoguedelha commented Jun 18, 2018 • edited Loading

nunoguedelha commented Jun 18, 2018

Nicogene commented Jun 18, 2018

Nicogene commented Jun 19, 2018

traversaro commented Jun 19, 2018

jgvictores commented Jun 19, 2018

traversaro commented Jun 19, 2018

PeterBowman commented Jun 19, 2018

nunoguedelha commented Jun 19, 2018

Nicogene left a comment

Choose a reason for hiding this comment

jgvictores commented Jun 19, 2018

Nicogene commented Jun 20, 2018 • edited Loading

jgvictores commented Jun 20, 2018

traversaro commented Jun 20, 2018

nunoguedelha commented Jun 20, 2018

Nicogene commented Jun 23, 2018

jgvictores commented Jun 29, 2018 • edited Loading

PeterBowman commented Aug 2, 2018

nunoguedelha commented Aug 2, 2018

traversaro commented Aug 3, 2018

nunoguedelha commented Jun 18, 2018 •

edited

Loading

Did some refactoring in the SWIG interface file `yarp.i` for reducing redundant code :

nunoguedelha commented Jun 18, 2018 •

edited

Loading

Nicogene commented Jun 20, 2018 •

edited

Loading

jgvictores commented Jun 29, 2018 •

edited

Loading