[HumanLogger] Logger silently failing to save file when attach device not found

[lrapetti]

When a quantity is set to be saved, but the corresponding interface is not attached, the device runs but the data are not saved.

e.g. enabling logHumanDynamics but not attaching a IHumanDyanamics interface

[lrapetti]

cc @RiccardoGrieco

[lrapetti]

The behaviour is similar to what observed in #338, I think the start() is never called. The reason should be due to the logic in

human-dynamics-estimation/devices/HumanLogger/HumanLogger.cpp

Lines 355 to 367 in 11ad67f

	if ((!pImpl->settings.logHumanState || pImpl->iHumanState) &&
	(!pImpl->settings.logHumanDynamics || pImpl->iHumanDynamics))
	{
	if (!pImpl->configureBufferManager()) {
	yError() << logPrefix << "Failed to configure buffer manager for the logger.";
	return false;
	}
	if(!start()) {
	yError() << logPrefix << "Failed to start the loop.";
	return false;
	}
	}

[RiccardoGrieco]

Indeed the issue is due to #342 (comment).

The problem resides in how yarprobotinterface handles the attach phase. We need to have the call to start in the attach method since attachAll is not called when there is only one device to be attached.
On the other hand, we cannot rely on the order of the attach calls to understand when to perform the checks on the attached devices.

With that said, I can think of the following 2 solutions:

1. Call start a priori. The periodic thread will have then an initialization phase with a timeout. If after the timeout the required devices are not attach, give an error and stop.
   Drawback: yarprobotinterface will be running happily until the timeout expires in case of failure, which can be misleading.
2. Change the way yarprobotinterface handles the priority in calling attach over attachAll and always call attachAll in case of IMultiWrapper. I sketched this solution with this simple commit.
   Drawback: this change affects all the yarprobotinterface users

[RiccardoGrieco]

A 3rd solution:

3. Remove IWrapper from the implemented interfaces. This will force yarprobotinterface to call attachAll and we can move the checks there.

[lrapetti]

Can't we have a variable nr_attach_driver that by default is 1, and it is set to driverList.size() in case attachAll is called? In this way we would be able to handle the start either in the attach or attachAll, and we can also check if all the expected interfaces are attached?

btw I don't know if there exist something already implemented, that we are not aware of, in the yarprobotinterface implementation. Maybe @traversaro or @randaz81 can suggest some solution

[randaz81]

I am not sure about your problem, maybe its better to clarify f2f. In any case, please be aware that there exists two different interfaces, WrapperSingle with the attach() method, and IMultipleWrapper with the attachAll() method. You have to choose the correct one for your use case. Is this your issue?

[traversaro]

@RiccardoGrieco I think the problem is that attach is called by attachAll. One thing that we can rely on is that only one of attach or attachAll is called, not both. So I think the correct way to do is to move the code that now is is attach to a method called attachHelper or similar and remove any code related to start from it, and then call both start and attachHelper from attach and attachAll. Even better, what you can do is to keep the start code only in attachAll, and just call attachAll from attach.

[lrapetti]

I am not sure about your problem, maybe its better to clarify f2f. In any case, please be aware that there exists two different interfaces, WrapperSingle with the attach() method, and IMultipleWrapper with the attachAll() method. You have to choose the correct one for your use case. Is this your issue?

Hi @randaz81, after discussing with @RiccardoGrieco the issue in general is probably due to the fact that we are using both the interfaces

human-dynamics-estimation/devices/HumanLogger/HumanLogger.h

Lines 26 to 27 in 58ad913

	, public yarp::dev::IWrapper
	, public yarp::dev::IMultipleWrapper

What was suggested by @RiccardoGrieco in #342 (comment) might probably be the solution.

[traversaro]

Anyhow, the solution of just removing yarp::dev::IWrapper is indeed easier, so if we do not need to use IWrapper probably the way to go is to do that.

[lrapetti]

A 3rd solution:

3. Remove IWrapper from the implemented interfaces. This will force yarprobotinterface to call attachAll and we can move the checks there.

This is being implemented by @RiccardoGrieco in https://github.com/robotology/human-dynamics-estimation/tree/remove-human_logger-single_wrapper, and I am currently testing it. Seems to work fine, I am just investigating a termination segfault

[lrapetti]

I have tested the code in https://github.com/robotology/human-dynamics-estimation/tree/remove-human_logger-single_wrapper with all the cases combining logHumanState logHumanDynamics and attaching both and one device at the time, and the behaviour is the expected one (I have also checked that the expected data is logged).

The segmentation fault was no longer happening after I clean my local modification, but we should keep an eye open if we observe it again.

@RiccardoGrieco feel free to open the PR with the fix

[RiccardoGrieco]

@RiccardoGrieco feel free to open the PR with the fix

PR #344 opened

[lrapetti]

PR merged, @RiccardoGrieco @traversaro do you think we should do a release with this bugfix?

[traversaro]

PR merged, @RiccardoGrieco @traversaro do you think we should do a release with this bugfix?

If you find it useful, ok for me!

[lrapetti]

I have drafted the new release (#345), meanwhile we can close this issue, thanks all!