added option to remove implicit wrappers in lasersensor, controlboard, depthcamera and multicamera

[ste93]

based on #564
This PR adds a flag USE_NEW_WRAPPERS that if enable removes all the wrappers from doublelaser, lasersensor, controlboard, depthcamera and multicamera and allows the use of the new nws present in yarp 3.5.

YARP 3.5 is required now as basis

[ste93]

@traversaro do you think that it should be added a CI with the flag USE_NEW_WRAPPERS on?

[traversaro]

Thanks @ste93 ! I already commented something I noticed, even if the complete review will be easier after #564 gets through. Could you consider reviewing the PR title to something more descriptive? "Removed wrappers" does not sound something that really describe what this PR is doing.

[ste93]

@traversaro rebased on devel

[traversaro]

@traversaro rebased on devel

Let me know when this is ready for review.

[ste93]

Let me know when this is ready for review.

It is ready

[traversaro]

I had a leftover comment, sorry for that .

[traversaro]

I guess also the docs in https://github.com/robotology/gazebo-yarp-plugins/tree/master/plugins/robotinterface#how-to-specify-existing-yarp-devices-to-which-to-attach on how to specify the YARP device instance name should mention that if GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS is enabled, then the yarpDeviceName is used also for the gazebo_yarp_controlboard plugin.

[randaz81]

I recommend to remove plugin doublelaser by this PR, since it cannot work with the new architecture.
Additionally I recommend for the next release to remove it entirely from the repo since it is deprecated and no more in use.

[traversaro]

I recommend to remove plugin doublelaser by this PR, since it cannot work with the new architecture.
Additionally I recommend for the next release to remove it entirely from the repo since it is deprecated and no more in use.

The gazebo_yarp_doublelaser plugin was modified by @drdanz in https://github.com/robotology/gazebo-yarp-plugins/pull/559/files to handle the new workflow and the documentation was also updated to mention it, do you think we should remove it also from the docs?

[ste93]

Just one question: how are you testing that GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS set to OFF continue to work as intended? Are you running some kind of model that assume that the implicit wrappers are there, such as the iCub model, or the non-modified R1 model? gazebo-yarp-plugins has a few tests, but unfortunatly they do not cover a lot of functionalities.

For now I'm testing with the old r1 model, I can try with iCub but I don't have any running simulator now, I'll check it maybe. Do you have any suggestion?

[traversaro]

For now I'm testing with the old r1 model, I can try with iCub but I don't have any running simulator now, I'll check it maybe.

The old R1 model seems fine, but are you testing the case in which you are not using gazebo_yarp_robotinterface and you are relying instead on the implicitly create wrappers, for example via yarpmotorgui?

[ste93]

For now I'm testing with the old r1 model, I can try with iCub but I don't have any running simulator now, I'll check it maybe.

The old R1 model seems fine, but are you testing the case in which you are not using gazebo_yarp_robotinterface and you are relying instead on the implicitly create wrappers, for example via yarpmotorgui?

yes I'll test with yarpmotorgui and yarpview

[randaz81]

The gazebo_yarp_doublelaser plugin was modified by @drdanz in https://github.com/robotology/gazebo-yarp-plugins/pull/559/files to handle the new workflow and the documentation was also updated to mention it, do you think we should remove it also from the docs?

Yes, with the new yarp 3.5, this device can be completely removed from the repo.
It was existing only because it was not possible to do the attach operation which is now possible with the robotinterface plugin.
Its functionalities are now replaced by the same yarp device driver used on the real robot, i.e. https://github.com/robotology/cer/tree/master/cermod/cerDoubleLidar. This device can be now attached to a gazebo lidar device via robotinterface.

[traversaro]

Thanks, the PR seems good to go. The only missing pieces are:

• added option to remove implicit wrappers in lasersensor, controlboard, depthcamera and multicamera #565 (comment)
• Update the CHANGELOG

[traversaro]

Yes, with the new yarp 3.5, this device can be completely removed from the repo.

Cool! Can you approve PR #570 that add a deprecation notice for the users on this, so then in the next release we can remove the plugin?

[ste93]

Thanks, the PR seems good to go. The only missing pieces are:

* [added option to remove implicit wrappers in lasersensor, controlboard, depthcamera and multicamera #565 (comment)](https://github.com/robotology/gazebo-yarp-plugins/pull/565#issuecomment-889717629)

* Update the CHANGELOG

updated both

[ste93]

I've tested it on gazebo, yarpmotorgui, yarplaserscannergui and yarpview both with and without the new option and it seems to work fine :)

[traversaro]

It is a bit strange to have a sentence that uses the present perfect in a document that uses the present, don't you find? Did you tried to check if this sentence is coherent with the rest of the document? I think it may be more clear to remove all together the new sentence, and just add a sentence to the previous period, something like:

Suggested change

	For the `gazebo_yarp_controlboard`, if the `yarpDeviceName` parameter is not specified, for legacy reason the **YARP device instance name** for each created device can also be specified with the `networks` parameter list in the plugin configuration.
	A cmake option (`GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS`) has been added, if this option is enabled then implicit wrappers present in`gazebo_yarp_multicamera`, `gazebo_yarp_lasersensor`, `gazebo_yarp_controlboard` and `gazebo_yarp_depthCamera` are removed, the new way to have them is to attach the new nws to gazebo devices via yarprobotinterface as above.
	If the `GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS` option is set to `OFF` (default value), for the `gazebo_yarp_controlboard` if the `yarpDeviceName` parameter is not specified, for legacy reason the **YARP device instance name** for each created device can also be specified with the `networks` parameter list in the plugin configuration of `yarpDeviceName`. If instead the `GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS` option is set to `ON`, the `gazebo_yarp_controlboard` behaves like the rest of the plugins and requires to take the **YARP device instance name** from the `yarpDeviceName` parameter.

[traversaro]

I've tested it on gazebo, yarpmotorgui, yarplaserscannergui and yarpview both with and without the new option and it seems to work fine :)

Great, can you check my last comment on the docs?

[traversaro]

Thanks @ste93 ! I would Squash and merge, but if you prefer to re-structure your commits please let me know.

[ste93]

no you can squash the commits

[traversaro]

Thanks @ste93 !