new Frame Transform interfaces + ovrdevice refactoring

[aerydna]

This pr do a lot of different things. unfortunately all of them are strictly related so it cannot be splitted in different pr (or it's very painful doing it). also, splitting it in different pr makes very unclear the sense of some modifications.

the necessity of this pr emerged with the urge to treat devices that produces frames (poses in the 3D space), like ovrdevice and vicon device, in the standard yarp way (with the interface + wrapper + client combo, P2P paradigm).
Until now, a device that produces frames had to open a frameTransformClient inside and comunicate via the frameTransformServer to other clients via a Client-Server-Client architecture, different from the usual P2P comunication pattern.
while this has some advantages it also forces a network comunication via yarp ports even when it is not needed (so it is not possible to open the device locally and get his data via c++ calls). In order to solve this i developed two new interfaces that can be adapted to both paradigm.

IFrameSource: this particular interface can seems unusual cause it does not have any pubblic pure virtual method. This is because all the beaviour and the math involved in chaining the Frames toghether can be reused among all the devices and it is already implemented. instead it have a sort of protected intra-class interface, to allow a correct comunication between the parent class and the derived calsses, enforcing a lot of stuff and robustifying the architecture. in particular this is useful to build up a callback mechanism that allow the user to trigger a function call on every update of the device (even remotely).

pr modification list:

- introduces a dependency in yarp to a new library of robotology called RobotologyTemplateLibrary, which contains some utilities that should not stay in yarp but since the library is still an unstable stub, the dependency is coordinated by the new flag YARP_ENABLE_UNSTABLE. we cannot wait for the library to be stable, cause the development of this new library is strictly related to his possible usage. and it is only used in this pr at the moment, and it will likely used fisrt in yarp. with @traversaro we agreed to flag the new library as stable within max 2 releases of yarp and from then continue to develop the library with the usual deprecation policies. moving the dependency outside the YARP_ENABLE_UNSTABLE flag.
the work on this became way too time expensive and some minor importance elements have been reduced

- definition of a new global flag YARP_EXPERIMENTAL. useful to confine part of the code that uses the new library or that are in a experimental state.

• with a tool offered by the new library, two new interfaces has been developed. Actually they come from the IFrameTransform interface, splitted in two parts. one for devices which only produces frames, IFrameSource, and one that rappresent the version 2 of the IFrameTransform, and one for set some tf via function call from the user application (IFrameSet.. i know the name i ugly and misleading. go on, propose a new one!).

• yarp::math::FrameTransform name issue. two new name to identify the frame structure has been added: frameId and parentFrame. this should substitute the old dst_frame_id and src_frame_id which have been deprecated. the new implementation solve the [FrameTransformClient] getChainedTransform method gives undesired results #1775

• ovrdevice has been adapted to the new paradigm, deriving from iFrameSource, eliminating both the inclusion of the FrameTransformClient and the yarp port to publish the headset pose. the new implementation is confined by YARP_EXPERIMENTAL, otherwise the old implementation is active. this allows to use it without the transform server, taking the data directly from him via c++ code or remotely in a p2p via the network wrappers

• FrameReceiver the client side network wrapper.

• FrameBroadcaster the produces side network wrapper.

these last three devices support the new callback system. so it is possible to have a user application triggered and syncronized with the ovrdevice thread.

26/03/2019 the network wrappers comunication is made via publisher and subscriber so the distribution over multiple subscriber of the tf data is guaranteed by the topic mechanism. also the ros comunication can be archieved via topics because the message used by the network wrapper is yarp::rosmsg::tf2_msgs::TFMessage.

notes:

• everithing is still to be tested
  - i think in the future, the FrameTransformClient should implement the new IFrameManager interface, which is Client-Server-client compliant. @randaz81 told me it is used a lot outside yarp so i leaved it untouched. at this point the FrameTransform client and server can be dismissed..
  - no ROS compatibility have been added in the FrameBroadcaster for now. it can be done in a future pr as it to be quick and easy. ROS compatibility added
  ~~- a lot of interoperations with the other paradigm (FrameTransformClient/Server) are possible, though none of them have been developed at the moment. ~~
  no interoperation with the old paradigm is needed anymore.
  - since there are probably 762 different ways to do the cmake parts, i probably picked the wrong one and i presume it have to be corrected. so if you have suggestions regarding the "cmake-shining-compliant" way to accomplish what i've done don't be afraid of fullfill my lack of experience with cmake. the cmake part is very small right now
  - if it isn't clear the relation between all the element of the pr is the following: RobotologyTemplateLibrary Result type is used in the ----> new interfaces to manage Frames which have been developed to change the comunication paradigm of the -----> ovrdevice and Others. viceversa the ovrdevice stands as an example of the new implementation. the pr is much more simpler
• fixes the dic-iit/element_retargeting_from_human#72 and it is a preparation for the 73 and 74.
• there will be, for sure, someone who don't like the names of the new flags/interfaces/something and has better alternative. not a problem for me. Just decide the names you like and i will change it.
• the changelog and the method documentation will be added/updated after the review process (i expect it to be long)
• sorry for possible english mistake

@GiulioRomualdi @DanielePucci @drdanz @Nicogene @randaz81 @traversaro @barbalberto

[traversaro]

Some comments.

[traversaro]

For this and other methods, the old interface was:

bool getAllFrameIds (std::vector< std::string > &ids) override

the old method permitted to get the the frameIds and the similar quantities without any dynamic memory allocation, while the new signature does not permit this. I suggest to also reintroduce the old signature to not constrain any user of the interface to have a dynamic memory allocation when using the interface.

[aerydna]

the old method had also a dynamic memory allocation as the frame count where subject to change. also if your concern is the std containers reinitialized at every frame we can simply make these container class member and return const references to them. or, if you insist, we can also expose overloads of these methods, similar to the old ones.. but first think about it.

[traversaro]

the old method had also a dynamic memory allocation as the frame count where subject to change

As long as the capacity of the passed container is always greater than the maximum size, there is no dynamic memory allocation.

we can simply make these container class member and return const references to them

How do you enforce proper mutual exclusion in this case?

[aerydna]

there's no way for the user to know the size currently and for the mutual exclusion, perhaps we can making it thread-local. maybe in that case the problem is when you have multiple instances. i have to check this cause i don't remember if it implies the staticness

[aerydna]

after a quick search i found this article https://web.archive.org/web/20130930101140/http://cpp-next.com/archive/2009/08/want-speed-pass-by-value
didn't read it completely but seems to definitly promote the pass by value practice for best performances and for some caveats it propose different solution from the classical output function argument way. i'll read it with attention asap.

A good idea for get rid of this question once for all would be to do some test. i think i will do it. A thing that we can try is also to move the implementation of these method in the header in order to move the compilation of these function on the user side, allowing for further optimization in case of inner-loops calls.

[traversaro]

there's no way for the user to know the size currently

In practice on a given closed system you can measure the maximum number of frames, and call .reserve with a value greater than that. More practically, what typically happens is that the vector is resized a few time, but as the capacity is not change when the vector if shrinked, so there actually are dynamic memory allocation only when the number of frames increased (not always to be honest, as typically the strategy used in std::vector on resize to increase its buffer is always to double its capacity if a reallocation is needed, regardless of the actual size requested.

perhaps we can making it thread-local

Personal opinion: I do not think that forcing any implementation of the getAllFrameIds method to have a thread local std::vector<std::string> buffer (or even just the additional complexity of doing it once) just to avoid the signature with the out variable passed as a reference is worth it.

after a quick search i found this article https://web.archive.org/web/20130930101140/http://cpp-next.com/archive/2009/08/want-speed-pass-by-value
didn't read it completely but seems to definitely promote the pass by value practice for best performances and for some caveats it propose different solution from the classical output function argument way. i'll read it with attention asap.

That article assumes that the in any case the std::vector<std::string> needs to be created ex-novo every time by the caller, and it is just arguing that for non-virtual methods defined in the same compilation unit of caller the compiler is able to ensure that the std::vector<std::string> is created only once. Our case is quite different: we are discussing virtual methods (so the compiler at compile time has no way of knowing which method will be actually called) and we are discussing how to avoid any kind of memory allocation, i.e. that the std::vector<std::string> is created even only once.

A good idea for get rid of this question once for all would be to do some test. i think i will do it.

Are you discussing speed tests or dynamic memory allocation tests? I am actually arguing to have an interface that does not force dynamic memory allocation, the increased speed would be just a byproduct from my point of view.

A thing that we can try is also to move the implementation of these method in the header in order to move the compilation of these function on the user side, allowing for further optimization in case of inner-loops calls.

As this is a virtual call that the users will call by just having a pointer to IFrameSource, no matter what, the method cannot be inlined by the compiler, because the compiler has no idea of which method is actually called.

[traversaro]

To be honest I just realized that on an increase in the number of the frames there always be a dynamic memory allocation if the additional frame id does not fit in the sso buffer. However not having dynamic memory allocation if the frame numbers do not change seems already quite convenient to me.

[aerydna]

Personal opinion: I do not think that forcing any implementation of the getAllFrameIds method to have a thread local std::vectorstd::string buffer (or even just the additional complexity of doing it once) just to avoid the signature with the out variable passed as a reference is worth it.

i would personally get rid of this kind of signature cause it bothers me a lot. none of the framework i use has this kind of signature. this should mean something. anyway if we find a way to have the same behaviour with a signature semantically appropriate why don't use every c++11/14 construct to do it?

Our case is quite different: we are discussing virtual methods (so the compiler at compile time has no way of knowing which method will be actually called)

this is true.. as it is true that is not mandatory at all for those method to be virtual. i had this doubt also for other reasons. removing the "virtualness" of the method would make the IFrameSource not an interface anymore, but a simple partially virtual public superclass. i don't know, i have to think about it

Are you discussing speed tests or dynamic memory allocation tests? I am actually arguing to have an interface that does not force dynamic memory allocation, the increased speed would be just a byproduct from my point of view.

ok this was not clear for me, thank you for pointing it out. if we add the external reference to be valorized, the interface should do a check on the capacity of the vector and return fail if inappropriate right? in this case i should also add a method to know the frame count. but what to do with device such as the vicon, where new marker can pop in or out in any moment?

[traversaro]

anyway if we find a way to have the same behaviour with a signature semantically appropriate why don't use every c++11/14 construct to do it?

Not sure if I got what you mean here.

ok this was not clear for me, thank you for pointing it out. if we add the external reference to be valorized, the interface should do a check on the capacity of the vector and return fail if inappropriate right? in this case i should also add a method to know the frame count. but what to do with device such as the vicon, where new marker can pop in or out in any moment?

Consistently with the rest of YARP, I do not think we need to ensure there are no dynamic memory allocations at all, but I just think we should avoid to have an interface that forces the users to have a quite huge number of dynamic memory allocations.

[traversaro]

Some additional comments:

• As the YARP_ENABLE_UNSTABLE flag changes the public API, downstream users of YARP need a way to know if the YARP library that they find with find_package(YARP ..) was compiled with the YARP_ENABLE_UNSTABLE or not (to understand if they can use the IFrameSource.h file or not). This can be done by either adding a new CMake component to the package, or set a CMake variable in YARPConfig.cmake (but probably @drdanz was trying to deprecated those)

• As the <robotology/Result.h> header is include in public headers, and as the RobotologyTemplateLibrary CMake imported target is linked as PUBLIC to YARP_dev:

target_link_libraries(YARP_dev PUBLIC RobotologyTemplateLibrary)

then in YARPConfig.cmake if YARP_ENABLE_UNSTABLE is set to ON, find_dependency(RobotologyTemplateLibrary REQUIRED) should be added, otherwise any downstream project consuming the YARP::YARP_dev library will actually have a linker error due to the not found RobotologyTemplateLibrary library not found. (Off topic: if the RobotologyTemplateLibrary target was exported, with a NAMESPACE [see https://github.com/open62541/open62541/issues/2415], at least the error would be during the CMake configuration).

• Is the file src/devices/ovrheadset/calib.png for what is used? The YARP repo is already quite massive, and if we can avoid inserting another ~120 Kb it can be useful.

It may be just my ordinary fear of new stuff, but the new YARP_ENABLE_UNSTABLE stuff still scares me and I am not fully convinced by it, to be honest. The complexities include stuff like the first thing mentioned in this comment, or the fact that any additional binary option effectively doubles the number of configuration for which we should test YARP. I am not sure that the additional complexity of it is worth. Perhaps the simpler solution is just to try to get the testing and "iterations" of a new interface before a YARP release, and if it is not possible to do so, we explicitly mark them as unstable in the documentation.

[traversaro]

Some more comments.

[aerydna]

Some additional comments:
As the YARP_ENABLE_UNSTABLE flag changes the public API, downstream users of YARP need a way to know if the YARP library that they find with find_package(YARP ..) was compiled with the YARP_ENABLE_UNSTABLE or not (to understand if they can use the IFrameSource.h file or not). This can be done by either adding a new CMake component to the package, or set a CMake variable in YARPConfig.cmake (but probably @drdanz was trying to deprecated those)
As the <robotology/Result.h> header is include in public headers, and as the RobotologyTemplateLibrary CMake imported target is linked as PUBLIC to YARP_dev:
target_link_libraries(YARP_dev PUBLIC RobotologyTemplateLibrary)
then in YARPConfig.cmake if YARP_ENABLE_UNSTABLE is set to ON, find_dependency(RobotologyTemplateLibrary REQUIRED) should be added, otherwise any downstream project consuming the YARP::YARP_dev library will actually have a linker error due to the not found RobotologyTemplateLibrary library not found. (Off topic: if the RobotologyTemplateLibrary target was exported, with a NAMESPACE [see https://github.com/open62541/open62541/issues/2415], at least the error would be during the CMake configuration).

i totally agree.

Is the file src/devices/ovrheadset/calib.png for what is used? The YARP repo is already quite massive, and if we can avoid inserting another ~120 Kb it can be useful.

it is for the next feature of the ovrdevice so it will end up there anyway. it will be visualized inside the oculus when in regulation mode. if you it bother you i can remove it and reinsert it in the next pr i'm working on. Or we can simply leave it there

[aerydna]

It may be just my ordinary fear of new stuff, but the new YARP_ENABLE_UNSTABLE stuff still scares me and I am not fully convinced by it, to be honest. The complexities include stuff like the first thing mentioned in this comment, or the fact that any additional binary option effectively doubles the number of configuration for which we should test YARP. I am not sure that the additional complexity of it is worth. Perhaps the simpler solution is just to try to get the testing and "iterations" of a new interface before a YARP release, and if it is not possible to do so, we explicitly mark them as unstable in the documentation.

• the instability is not only an instability of yarp itself but is dependent to an instability of an external library.

• i think the unstable part should not be tested because it should be possible to break it in anyway

• we can put the new class under a experimental namespace. the problem is only with existent class we want to improve with experimental feature.

• if you (or someone else) are too scared we can simply mark the library stable as it is, remove all the unstable stuff and wait the next release of yarp to release the breaking changes to the ovrdevice. my fear is that it is time consuming and i personally would fork the features i need (devices or interfaces) putting them in external repository in order to start using them immediatly in other projects then subsituting with the yarp version once it come out. this would increase the work overload

• another option is for me to develop things outside yarp (forking existent devices if needed) and when ready include them in yarp. this would increase the work overload though

• this option was imho the best way to contribute to yarp quickly remaining robust (with the modification you suggested).

[aerydna]

@traversaro, maybe the experimental namespace is the best way to handle this (it was also suggested by @drdanz ). the thing we can do for existent classes (like oculus) is to fork them under the experimental namespace. with it, for a user is clear that he is using experimental features, subject to changes. what do you think?

[traversaro]

it is for the next feature of the ovrdevice so it will end up there anyway. it will be visualized inside the oculus when in regulation mode. if you it bother you i can remove it and reinsert it in the next pr i'm working on. Or we can simply leave it there

It is a bit difficult to discuss if it is ok or not to merge it or not if it is not used in the current PR. What happens if for example during the discussion for the new feature it turns out that adding the .png is not desirable or necessary? I would personally just add it in the new PR that actually just uses it, but I guess it is up to @drdanz to decide on that.

if you (or someone else) are too scared we can simply mark the library stable as it is, remove all the unstable stuff and wait the next release of yarp to release the breaking changes to the ovrdevice. my fear is that it is time consuming and i personally would fork the features i need (devices or interfaces) putting them in external repository in order to start using them immediatly in other projects then subsituting with the yarp version once it come out. this would increase the work overload

Ack. Consider that here there is a trade-off between the additional work necessary for you, and the additional complexity and associated problems (and resulting work) that may affect any YARP's user.
However, I guess this is again something for which @drdanz's opinion would be determinant.

[aerydna]

Ack. Consider that here there is a trade-off between the additional work necessary for you, and the additional complexity and associated problems (and resulting work) that may affect any YARP's user.
However, I guess this is again something for which @drdanz's opinion would be determinant.

it's not mandatory that is a additional work for me. i can work and correct things outside of yarp and then maybe someone else have to reintegrate or integrate them into it. maybe they'll end up to not be integrated at all while being useful things just because no one have time/want to spend time doing it. i understand your point though and i suggest to discuss this f2f

[traversaro]

it's not mandatory that is a additional work for me. i can work and correct things outside of yarp and then maybe someone else have to reintegrate or integrate them into it. maybe they'll end up to not be integrated at all while being useful things just because no one have time/want to spend time doing it. you may think this as the documentation of a software. i understand your point though and i suggest to discuss this f2f

Sorry, I meant a trade-off between "additional work that of people that actually need and benefits from the YARP_ENABLE_UNSTABLE" and all the other users that do not use YARP_ENABLE_UNSTABLE. I did not meant personally you. : )

[Nicogene]

I have not a great insight in the iFrameTransform interface and the respective client/server, so I can't review it in terms of functionality. I have only notes about the code cleaning/formatting but I think it will be changed and refactored a lot, I wiil review it later then.

[randaz81]

The PR is very large and touches multiple aspects, making it difficult to review.
Let's start from the functionalities. Could you provide one or more diagrams which describe the relationship between the classes you propose, justifying their usage in some application scenarios (with/without a device driver, with/without ROS etc) ?
See for example the diagram below, which depicts the current architecture:

[pattacini]

The PR is very large and touches multiple aspects, making it difficult to review.

I tend to agree with @randaz81.
Further, I think it'd be worth discussing offline the need for an external library.

[aerydna]

actually, from a user perspective it follows the usual yarp devices architecture. with an interface, a device driver and two network wrapper (server side and client side).

[aerydna]

with @lornat75 we agreed to discuss the new library introduction in a yarp council, if possible this very week. i have to ask @drdanz when we can schedule it

[aerydna]

classical yarp device architecture

ps.
actually the arrow between the two pc in the remote example should be attached to the "FrameReceiver".. i forgot to bring it in front in draw.io

the architeture has improved a lot (it includes ROS interoperation, distributed comunication via topics and an interface to set some tf from the user application and stream them through the network) and a new schema will be uploaded asap

as you may notice the IFrameManager interface is not used. this is because is the interface that should replace the IFrameTransform in the future, making the FrameTransformClient implementing it

@randaz81 as i said earlier no ros interoperation is implemented at the moment. however for implementing, is enough to make the FrameBroadcaster a ros publisher

[aerydna]

the pr has been closed due to some technical disagreement