Yarp: Twofold interpretation of pointer parameter in checkMotionDone() reflected in docs and implementations

[PeterBowman]

https://github.com/robotology/yarp/blob/86cfea57195a756fd08df5bc73757ad87fbdffe8/src/libYARP_dev/include/yarp/dev/IPositionControl.h#L80-L85

The output parameter is described as:

flag is a pointer to return value ("and" of all joints)

It is implemented as such in ref1, ref2, ref3 and ref4.

However, this leads to an asymmetry with regard to other motor-interface methods that accept a single pointer. In contrast to the previous behavior, sometimes it is regarded by the implementation as a pointer to an array: ref1, ref2, ref3.

Unless I'm wrong, notice that ControlBoardWrapper's RPC parser (ref) expects a single boolean value, whereas its implementation expects an array of booleans (ref). Memory space outside the boundaries of the declared boolean variable is accessed through the flag[juser] = done[jdevice]; instruction.

Related issues: https://github.com/robotology/yarp/issues/390, https://github.com/robotology/yarp/issues/387.

YARP version: current master branch (v3.1.1+).

[PeterBowman]

TODO (targeting devel branch as of https://github.com/robotology/yarp/commit/fc929769ddf3a5097568ec679b4ac154ab5d562d):

• fakeMotionControl.cpp
• ControlBoardWrapper.h (docs)
• ControlBoardRemapper.cpp
• RemoteControlBoard.cpp
• RPCMessagesParser.cpp
• IPositionControl.h: 1 + 2 (docs)

Also, list of correct implementations:

• DynamixelAX12FtdiDriver.cpp
• ControlBoardWrapper.cpp
• ImplementPositionControl.cpp

[PeterBowman]

Also affects joint-group commands, e.g. RemoteControlBoard::checkMotionDone(int, int*, bool*). Note that RPC protocol changes (bump protocol version).

[barbalberto]

The correct interpretation is the one stated in the documentation: the method checkMotionDone is _always returning a single boolean value_ for all the APIs, like shown in the link you mentioned.

However, this leads to an asymmetry with regard to other motor-interface methods

I know this may be strange if compared to other methods and may lead a developer into error, as you say, but this behavior is documented, so IMHO is acceptable.

Unless I'm wrong, notice that ControlBoardWrapper's RPC parser (ref) expects a single boolean value, whereas its implementation expects an array of booleans (ref).

Actually here you correctly spotted a regression bug introduced in 0ec738a9a33bf8394ac7e84209ae5f6d16e26ed6

Before that commit, all the three implementation of checkMotionDone were correctly returning a single value , see https://github.com/robotology/yarp/blob/09fd2d76e7fd0db786277dcf3d9ced35342dfe6f/src/libYARP_dev/src/devices/ControlBoardWrapper/ControlBoardWrapper.cpp#L1731-L1754
Then a refactoring for optimization introduced the regression bug. Thanks for finding it out, I'll fix it.

About the implementation you linked:

fakeMotionControl.cpp	all calls returns a single value: correct
ControlBoardWrapper.h (docs)	docs is correct
ControlBoardRemapper.cpp	all calls returns a single value: correct
RemoteControlBoard.cpp	all calls returns a single value: correct
RPCMessagesParser.cpp	all calls returns a single value: correct
IPositionControl.h: 1 + 2 (docs)	doc is correct
DynamixelAX12FtdiDriver.cpp	multiple values: WRONG
ControlBoardWrapper.cpp	checkMotionDone(bool *flag) is wrong
ImplementPositionControl.cpp	same regression bug, WRONG

So, to make the story short, there is no twofold interpretation, the only correct interpretation is the one explicitly stated in the documentation. There are some wrong implementation not complying with the documentation.

I'll fix those three implementations to comply with API.

[valegagge]

I also checked icub motion control devices, both can and ethernet; the last one has the bug you noticed.
I'll fix it asap.

We need to fix the bug on yarp and on iCub device concurrently. If I'm not wrong the best way should be before fix iCub and than yarp.

[drdanz]

Fixed by @barbalberto and @valegagge in #2035 and robotology/icub-main#580