Add disableImplicitNetworkWrapper to gazebo_yarp_forcetorque, gazebo_yarp_depthcamera, gazebo_yarp_lasersensor, gazebo_yarp_imu and gazebo_yarp_controlboard

[traversaro]

Fix #585 .

I tested this on a iCub-like humanoid robot that already migrated to the new nws/nwc architecture, and it is working fine.

[ste93]

I have just one question, you left the yarp device name in this PR when the if is enabled (configuration via yarprobotinterface) while when is disabled from the compiler the device name is not left. I think it would be better if both maintains the same behavior so there will be no mistake in the future? @traversaro

[traversaro]

I have just one question, you left the yarp device name in this PR when the if is enabled (configuration via yarprobotinterface) while when is disabled from the compiler the device name is not left.

Sorry, I am not sure I get what you mean. I guess that you mean that there is a slight different behaviour if disableImplicitNetworkWrapperServer is passed or when GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS CMake option, but I am not able to understand what this difference is (in which sense "the device name is not left"?).

[ste93]

I just mean in the case disableImplicitNetworkWrapperServer the if ends at line 121 while in case GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS the if ends at line 138, shuld both end at line 138 or 121?

gazebo-yarp-plugins/plugins/forcetorque/src/ForceTorque.cc

Line 123 in 33d8c1e

if(!driver_properties.check("yarpDeviceName"))

this line was my question

[Nicogene]

I got a little bit lost about these flags, so there is a possibility that I didn't get the purpose of this PR.
This flag is added in order to allow to not open the wrapper from the configuration file when you are not disabling the implicit wrapper?

If so I would change the option name, I think that the the implicit wrapper that is set in the DeviceDriver factory, should be treated separately respect the mandatory wrapper configuration from the ini file.

[traversaro]

This flag is added in order to allow to not open the wrapper from the configuration file when you are not disabling the implicit wrapper?

Exactly, basically is permitting to disable the implicit wrapper via configuration file on a plugin instance basis, instead of disabling globally it via configuration option at the compile time, to avoid all the problems listed in #585 .

If so I would change the option name

Sure, feel free to propose a better name, thanks!

I think that the the implicit wrapper that is set in the DeviceDriver factory, should be treated separately respect the mandatory wrapper configuration from the ini file.

Not sure I get what you mean by this.

[Nicogene]

From my understanding the implicit wrapper is

gazebo-yarp-plugins/plugins/forcetorque/src/ForceTorque.cc

Lines 57 to 63 in 880af12

	#ifndef GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS
	netWrapper = "analogServer";
	#endif // GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS
	// Add my gazebo device driver to the factory.
	::yarp::dev::Drivers::factory().add(new ::yarp::dev::DriverCreatorOf< ::yarp::dev::GazeboYarpForceTorqueDriver>
	("gazebo_forcetorque", netWrapper.c_str(), "GazeboYarpForceTorqueDriver"));

If I pass an empty string to the Drivers::factory, when I open a device without wrapper, it opens the default wrapper.
The thing that you are preventing with that flag, is that the user setting disableImplicitNetworkWrapperServer can skip the open of the wrapper from configuration file, from my point of view this is an explicit instantiation of the wrapper/server.

The thing is that if GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS = OFF AND disableImplicitNetworkWrapperServer = true I would expect to have a device with no wrapper, instead the default one is opened. I would call the flag useDefaultWrapperServer, this means that you don't need to add the parameters of the wrapper server in the conf file

[Nicogene]

I hope to have been clear, because this is not super clear in my mind 😆

[traversaro]

From my understanding the implicit wrapper is

This is what not we (me and @ste93) have been calling implicit wrapper, so to avoid confusion I propose to call:

• "yarp implicit wrapper" to identify what @Nicogene is calling implicit wrapper
• "gyp implicit wrapper" to identify what @traversaro is calling implicit wrapper, and @Nicogene is calling explicit wrapper

Let's try to stick to this nomenclature otherwise it is impossible to discuss. : )

The thing is that if GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS = OFF AND disableImplicitNetworkWrapperServer = true I would expect to have a device with no wrapper, instead the default one is opened.

Are you sure? Did you checked this? According to my understanding of YARP API, if GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS = OFF AND disableImplicitNetworkWrapperServer = true no wrapper is opened, but I could be wrong.

[ste93]

I think the same as @traversaro , as I made the changes when the empty string is passed no wrapper is opened, but it is attached later with yarprobotinterface.

[Nicogene]

Are you sure? Did you checked this? According to my understanding of YARP API, if GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS = OFF AND disableImplicitNetworkWrapperServer = true no wrapper is opened, but I could be wrong.

Mmm actually no, I am sure that yarpdev does it, but now I recall that yarp::dev::PolyDriver does not have this mechanism of opening the default wrapper if it is missing in the ini.

I could give it a try or @drdanz @randaz81 can confirm this

For the flag name, if I am the only one that thinks that the implicit wrapper is the one not defined in the ini of the device, I can adapt 👍🏻

[ste93]

I think I've lost some passages, maybe we can update on teams?

[traversaro]

Mmm actually no, I am sure that yarpdev does it, but now I recall that yarp::dev::PolyDriver does not have this mechanism of opening the default wrapper if it is missing in the ini.

I could give it a try or @drdanz @randaz81 can confirm this

Actually we have the code, so we can just inspect this! The second argument of DriverCreatorOf is accessible in the code via the getWrapper method (see https://github.com/robotology/yarp/blob/702271e6f0fb685847938a2d892c0f629aab29f6/src/libYARP_dev/src/yarp/dev/Drivers.h#L111). In PolyDriver, that method is only accessed if the wrapped option is passed (see https://github.com/robotology/yarp/blob/d7b6df3af0c15bdc156e023da5882e3ce3893c69/src/libYARP_dev/src/yarp/dev/PolyDriver.cpp#L275), and that option is passed when you use yarpdev (see https://github.com/robotology/yarp/blob/d7b6df3af0c15bdc156e023da5882e3ce3893c69/src/libYARP_dev/src/yarp/dev/Drivers.cpp#L472), but never in the gazebo-yarp-plugins ?

To clarify further, in gazebo-yarp-plugins, we could already substitute all the second arguments of DriverCreatorOf calls with empty strings and the behaviour of the plugins (regardless of the options) would be exactly the same.

[traversaro]

Recap of meeting with @ste93 and @Nicogene:

• Rename the option to be aligned with the name of the CMake option
• For @Nicogene it was not clear the wrapper by default is a yarpdev-only stuff and not a PolyDriver stuff. See Add disableImplicitNetworkWrapper to gazebo_yarp_forcetorque, gazebo_yarp_depthcamera, gazebo_yarp_lasersensor, gazebo_yarp_imu and gazebo_yarp_controlboard #586 (comment) .
• @ste93 suggested that if disableImplicitNetworkWrapperServer is passed, then yarpDeviceName is required to mirror the behaviour when GAZEBO_YARP_PLUGINS_DISABLE_IMPLICIT_NETWORK_WRAPPERS is enabled and eventually the future behaviour.

[traversaro]

@Nicogene @ste93 I addressed your comments, once you like this PR I will propagate it to all related plugins.

[Nicogene]

LGTM!

[ste93]

sorry I've noticed now, there are some problems building the code

[traversaro]

sorry I've noticed now, there are some problems building the code

They were thre in devel once I started working, if I rebase they should go away.

[Nicogene]

After the last update meeting, it emerged that we need to discuss more about how to handle this, then this PR is blocked for now.

[ste93]

Fyi @randaz81

[traversaro]

After the last update meeting, it emerged that we need to discuss more about how to handle this, then this PR is blocked for now.

As we had then a meeting that agreed on the step forwards (discussed in #594), I think we can go forward with this PR.

[traversaro]

After the last update meeting, it emerged that we need to discuss more about how to handle this, then this PR is blocked for now.

As we had then a meeting that agreed on the step forwards (discussed in #594), I think we can go forward with this PR.

I tested this on a iCub-like humanoid robot that already migrated to the new nws/nwc architecture, and it is working fine, I think we can proceed (I also added the changes for do a 4.2.0 release).

[traversaro]

Please ignore the Windows CI failure that will be fixed by #610 .

[traversaro]

fyi @Nicogene