search

robotology / human-dynamics-estimation

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

Change BufferedPort forceStrict to false #279

Closed lrapetti closed 2 years ago

lrapetti commented 2 years ago

This PR changes the configuration of the buffered ports write to the default one (forceStrict = false), since there is no specific reason to work in strict mode (using forceStrict the port might be blocked by a single connected device, but there can be multiple connections on that port, plus we are not interested in enforcing strictly the read of all the data in sequence).

cc @S-Dafarra @kouroshD @traversaro

lrapetti commented 2 years ago

@diegoferigo was there a specific reason for setting forceStrict = true when those wrapper devices were initially conceived?

diegoferigo commented 2 years ago

@diegoferigo was there a specific reason for setting forceStrict = true when those wrapper devices were initially conceived?

I'm not sure, too much time has passed since. I never tested extensively this port flag. This PR updates the calls occurring in the *::update methods, whose rate is configured by the user. Maybe forceStrict could be useful in a multithreading setting, but this does not seem the case.

In any case, does the forceStrict usage affect performance of functionality? It seems to me that this is a proper way to prevent dropping messages. If it does not hurt, I don't see any reason to allow dropping messages, but I'm not 100% sure what the documented the current object will not be sent on connections that are currently busy means.

S-Dafarra commented 2 years ago

The main issue is that if a client is in a delayed network, that option will delay also the sender, and in turn all the other clients.

Suppose that one client has sporadic delays of (200ms). That option will block the sender also for 200ms. Then, instead of just losing some packets on a single unlucky client, you make every client not receiving data for 200ms

diegoferigo commented 2 years ago

The main issue is that if a client is in a delayed network, that option will delay also the sender, and in turn all the other clients.

Suppose that one client has sporadic delays of (200ms). That option will block the sender also for 200ms. Then, instead of just losing some packets on a single unlucky client, you make every client not receiving data for 200ms

If this is true, then it makes sense to switch off this flag. It's not yet 100% clear to me whether the publisher actually has any feedback about the client status when writes to a port, but maybe there are subtleties of the implemented observer pattern that I'm not fully familiar with. Not sure if @traversaro has a better knowledge about it. Regardless, this is ok if it works for you and it was tested on the setup.

Edit: I'm assuming that the carrier is not tcp

kouroshD commented 2 years ago

Since we are here, can you @lrapetti kindly add the option for the HDE remapper device for the type of the carrier in the connection in the line https://github.com/robotology/human-dynamics-estimation/blob/69d7bf004cd0ad2d3cc89b60ca24d4727c9ef879/remappers/HumanStateRemapper/HumanStateRemapper.cpp#L103-L104

lrapetti commented 2 years ago

Edit: I'm assuming that the carrier is not tcp

@diegoferigo Actually we have experienced what described by @S-Dafarra while using tcp carrier.

Since we are here, can you @lrapetti kindly add the option for the HDE remapper device for the type of the carrier in the connection in the line

What are you thinking? We can have a configuration option where you can choose the carrier and set a default one otherwise (which one should be the default one?)

kouroshD commented 2 years ago

What are you thinking? We can have a configuration option where you can choose the carrier and set a default one otherwise (which one should be the default one?)

Yes, that's the ideal solution, I believe so.

lrapetti commented 2 years ago

What are you thinking? We can have a configuration option where you can choose the carrier and set a default one otherwise (which one should be the default one?)

Yes, that's the ideal solution, I believe so.

I start merging the current modification and open an issue ti track the additional changes.

diegoferigo commented 2 years ago

If this is true, then it makes sense to switch off this flag. It's not yet 100% clear to me whether the publisher actually has any feedback about the client status when writes to a port, but maybe there are subtleties of the implemented observer pattern that I'm not fully familiar with. Not sure if @traversaro has a better knowledge about it. Regardless, this is ok if it works for you and it was tested on the setup.

Edit: I'm assuming that the carrier is not tcp

@diegoferigo Actually we have experienced what described by @S-Dafarra while using tcp carrier.

Yep I was saying that my comment should hold if the carrier is not tcp. If it is tcp, instead, the busy connection problem makes totally sense because, due to the protocol, the sender might lock the channel while it keeps trying to make the message be received.

Though, considering the wearables architecture, I remember now that we started using forceStrict because in the WearableData message we also include the timestamp. We wanted to be sure that we receive all the messages regardless of the connection status, particularly to ensure that collected datasets would not miss data due to networking problems. In this use case, reordering of message should be done offline, and disabling forceStrict would mean that few samples could be missing. In a real-time setting, however, I don't see problems of disabling forceStrict even with carries different than tcp.