robotology / robometry

Telemetry suite for logging data from your robot 🤖
https://robotology.github.io/robometry
Other
14 stars 9 forks source link

Fix close method of DeviceDumper #136

Closed isorrentino closed 3 years ago

isorrentino commented 3 years ago

I noticed this log message when using the dumper with the option autosave:

acceleration does not contain data, skipping
velocity does not contain data, skipping
encoders does not contain data, skipping

By checking the code I noticed that when the close() is called, it tries to save data to a file. But in case the option autosave is true, data have already been written. I added a statement to check whether the option is true. If yes, the saving is not called.

cc @prashanthr05

Nicogene commented 3 years ago

That kind of check is already done by yarp::telemetry::Buffermanager:

https://github.com/robotology/yarp-telemetry/blob/2aa42452807809e3ff57e2d947e5f5b292479090/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferManager.h#L111-L113

Probably it makes sense to just remove those lines in the telemetryDeviceDumper and let the BufferManagers do the autosave if enabled

isorrentino commented 3 years ago

@Nicogene I modified the code following your suggestion.

Nicogene commented 3 years ago

Sorry probably I didn't make myself clear, I meant to delete just the save to file in the closure of telelemetryDeviceDumper, then https://github.com/robotology/yarp-telemetry/pull/136/commits/08ed7a8f99769dfc68c8cd17445240aa0c91002a is sufficient, I would not change the behaviour of the BufferManager

isorrentino commented 3 years ago

With just https://github.com/robotology/yarp-telemetry/commit/08ed7a8f99769dfc68c8cd17445240aa0c91002a, when the option autosave is false, data are not written to a file. That is why I modified also BufferManager.

Nicogene commented 3 years ago

Probably the auto_save parameter is ambiguous, it is meant to just save automatically the data when you close your application that uses the BufferManager, but also if it is false the periodic save is not disabled.

See: https://github.com/robotology/yarp-telemetry/blob/2aa42452807809e3ff57e2d947e5f5b292479090/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferConfig.h#L35-L36

The warning you saw was due to the fact, even if auto_save is set to true, on closure telemetryDeviceDumper try to save the data, but it is already done by the BufferManager under the hood, and then the result is that BufferManager.saveToFile is called twice consecutively.

isorrentino commented 3 years ago

Thanks, @Nicogene. I modified the code again and removed the previous commits.