search

robotology / human-dynamics-estimation

Software repository for estimating human dynamics
BSD 3-Clause "New" or "Revised" License
80 stars 28 forks source link

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

Closed lrapetti closed 1 year ago

lrapetti commented 1 year ago

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 commented 1 year ago

cc @RiccardoGrieco

lrapetti commented 1 year ago

The behaviour is similar to what observed in https://github.com/robotology/human-dynamics-estimation/pull/338, I think the start() is never called. The reason should be due to the logic in https://github.com/robotology/human-dynamics-estimation/blob/11ad67f1d31f54d42aa1954a6a1a6e83a5892570/devices/HumanLogger/HumanLogger.cpp#L355-L367

RiccardoGrieco commented 1 year ago

Indeed the issue is due to https://github.com/robotology/human-dynamics-estimation/issues/342#issuecomment-1461742517.

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 commented 1 year ago

A 3rd solution:

  1. Remove IWrapper from the implemented interfaces. This will force yarprobotinterface to call attachAll and we can move the checks there.
lrapetti commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 https://github.com/robotology/human-dynamics-estimation/blob/58ad9134fd7a879090b45e218ca3bac132898c5b/devices/HumanLogger/HumanLogger.h#L26-L27

What was suggested by @RiccardoGrieco in https://github.com/robotology/human-dynamics-estimation/issues/342#issuecomment-1477588983 might probably be the solution.

traversaro commented 1 year ago

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 commented 1 year ago

A 3rd solution:

  1. 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 commented 1 year ago

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 commented 1 year ago

@RiccardoGrieco feel free to open the PR with the fix

PR #344 opened

lrapetti commented 1 year ago

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

traversaro commented 1 year ago

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

If you find it useful, ok for me!

lrapetti commented 1 year ago

I have drafted the new release (https://github.com/robotology/human-dynamics-estimation/pull/345), meanwhile we can close this issue, thanks all!