Wrapper related classes and interfaces

[drdanz]

I'll just dump some reasoning I've been doing on wrapper related interfaces

The classes related to Wrappers in YARP at the moment are:

• yarp::dev::IWrapper which has 2 functions
  • virtual bool attach(PolyDriver*) = 0
  • virtual bool detach() = 0
• yarp::dev::IMultipleWrapper which has 2 functions
  • virtual bool attachAll(const PolyDriverList&) = 0
  • virtual bool detachAll() = 0

Most devices implement IMultipleWrapper. Among these, the ones which can attach only one device are all doing the same thing, i.e. perform a check on the size of the PolyDriverList, getting the PolyDriver for the first element, and performing the actual attach on this. I think this is a source of duplicated code that should be avoided.

I believe that at some point it might be decided that IWrappers was supposed to be removed, to replace it everywhere with IMultipleWrapper. Nonetheless for some reason it was never deprecated.

My opinion is that most wrappers should implement both unless there is a good reason for not having one:

• Wrappers that can attach multiple elements can be attached to a list of one element (for example a ControlBoardWrapper that attaches to a fakeControlBoard. (This does not apply to deviches that cannot attach only one device, but require two or more to work)
• Wrappers that can attach only one device should be able to accept a list of one element, since that is something that they can handle.

Anyway, implementing all 4 functions in all the driver does not make too much sense:

• In wrappers that can attach only one device, the logic to attach only one driver from a list would be copied in all attachAll() which should basically ensure that the list size is one, take the first element, and call attach()
• In wrappers that can attach multiple devices, the logic to attach only one driver would be copied in all attach(), which should create a PolyDriverList, add the device (defining some name), and calling attachAll()
• The detach and detachAll methods, are basically the same.

The IMultipleWrapper interface is definitely more versatile, on the other hand, for devices that can attach only one device, calling attachAll() is more complicated than just calling attach().
Moreover, the "All" in the name of the methods, is not very significant.

From my point of view, it would make sense to make IMultipleWrapper inherit from IWrapper, but it would also make sense to do the opposite.

Also, another thing that I would like to consider is the I in the name, which is something that, in my opinion, should be reserved to interfaces to part of the robot that can be forwarded using nws/nwc, but this is not the case here. For example PolyDriver, DeviceDriver, etc. do not have the I. But on the other hand, the I seems to be used to represent "pure interfaces", but they are not used almost anywhere.

Concluding, I think that it would make sense to have just one interface that is something like this:

• yarp::dev::Wrapper
  • virtual bool attach(yarp::dev::PolyDriver*) = 0
  • virtual bool attach(const PolyDriverList&) = 0
  • virtual bool detach() = 0

and then 2 separate interfaces

• yarp::dev::MultipleWrapper : public Wrapper
  • bool attach(PolyDriver*) override

and

• yarp::dev::SingleWrapper : public Wrapper
  • bool attach(const PolyDriverList&) override final

and perhaps also

• yarp::dev::[something like MultipleOnly]Wrapper : public MultipleWrapper
  • bool attach(PolyDriver*) override final { /* print some error */ return false; }

In this way, the intention of who is creating a wrapper can be immediately very clear:

• If the device can attach only one device, it should derive from SingleWrapper and override attach(yarp::dev::PolyDriver*)
• If the device can attach more than one device it should derive from MultipleWrapper and override attach(const PolyDriverList&)
• If the device cannot attach only one device, it should derive from MultipleOnlyWrapper and override attach(const PolyDriverList&). This one could probably be avoided, but being able to do a dynamic_cast is perhaps more efficient and less error prone than checking the result of the attach(PolyDriver*) call which will not tell you if the attach failed because you are trying to attach a single device to a wrapper that can attach only two or more, or if

Other options could be to split the Wrapper interface in 2 (which is basically what we have now) and perhaps add some SingleWrapper and MultipleWrapper classes that derive from IWrapper and IMultipleWrapper, or to use some template/sfinae magic to remove the function that should not be used, or

Concluding, I'm not really sure how to handle this and I'm open to suggestions.

[drdanz]

WrapperSingle and WrapperMultiple implemented in #2589