ami-iit / bipedal-locomotion-framework

Suite of libraries for achieving bipedal locomotion on humanoid robots
https://ami-iit.github.io/bipedal-locomotion-framework/
BSD 3-Clause "New" or "Revised" License
132 stars 36 forks source link

Logged data corrupted when adding vectors as exogenous signal #846

Open GiulioRomualdi opened 2 months ago

GiulioRomualdi commented 2 months ago

We realized that some data are corrupted and the joints values are not there after closing the logger

When closing the device we got the following errors

[ERROR][matioCpp::Variable::createVar] The name should not be empty.
[ERROR][matioCpp::Struct::Struct] The element at index 6 (0-based) is not valid.

cc @S-Dafarra

GiulioRomualdi commented 2 months ago

I tried to replicate the issue on my machine but unfortunately the data are correctly logged

GiulioRomualdi commented 2 months ago

cc @carloscp3009

S-Dafarra commented 2 months ago

The first message is thrown from here: https://github.com/ami-iit/matio-cpp/blob/1d4008094d7362ae5d0b1c84443f3769d5011e91/src/Variable.cpp#L18-L23

The second message is thrown from https://github.com/ami-iit/matio-cpp/blob/1d4008094d7362ae5d0b1c84443f3769d5011e91/src/Struct.cpp#L53-L57 but I would say at this point it is already late because here the variables have already been created.

Another thing I can exclude is that this error is not thrown from the automatic conversion from a C++ struct. This is because the struct is filled by adding fields one by one (see here) that is using a different piece of code.

At this point I guess that the problem might occur when creating the output structure in robometry.

Are you able to open the corrupted file in Matlab?

S-Dafarra commented 2 months ago

I just realized that there is a flaw in this piece of code https://github.com/ami-iit/matio-cpp/blob/1d4008094d7362ae5d0b1c84443f3769d5011e91/src/Struct.cpp#L53-L58

When creating the matio struct, I need to pass a nullptr terminated vector. The problem is that if, for some reason, one of the variables I wanted to add to the struct is not valid, I am pushing a nullptr to the vector, preventing matio to add anything that comes next.

In fact, in the "corrupted" file, there is data, but not all the data

image

At the moment, it is not clear yet what is the faulty signal, but we can patch the pipeline in two places:

S-Dafarra commented 2 months ago
  • in matioCpp printing a more description message and avoiding to push nullptr pointers when creating the struct.

Relevant PR https://github.com/ami-iit/matio-cpp/pull/80

GiulioRomualdi commented 2 months ago

At the moment, it is not clear yet what is the faulty signal, but we can patch the pipeline in two places:

  • in matioCpp printing a more description message and avoiding to push nullptr pointers when creating the struct.
  • in robometry making sure that no signal can have empty name.

Hi @S-Dafarra! Really thank you for deepering in the problem! I was wondering if the problem is in the camera signal in the mat file

S-Dafarra commented 2 months ago
  • in robometry making sure that no signal can have empty name.

Done in https://github.com/robotology/robometry/pull/189

@carloscp3009 @GiulioRomualdi if you could test them, I will proceed with the merge process

S-Dafarra commented 2 months ago

At the moment, it is not clear yet what is the faulty signal, but we can patch the pipeline in two places:

  • in matioCpp printing a more description message and avoiding to push nullptr pointers when creating the struct.
  • in robometry making sure that no signal can have empty name.

Hi @S-Dafarra! Really thank you for deepering in the problem! I was wondering if the problem is in the camera signal in the mat file

I noticed that camera.jabra.rgb is empty, while camera.realsense.rgb.data seems to contain only 1.7153e+09 repeated 20371 times

S-Dafarra commented 2 months ago

I wonder whether in a "good" dataset the entries on the main level are the same of https://github.com/ami-iit/bipedal-locomotion-framework/issues/846#issuecomment-2103022355

carloscp3009 commented 2 months ago

At the moment, it is not clear yet what is the faulty signal, but we can patch the pipeline in two places:

  • in matioCpp printing a more description message and avoiding to push nullptr pointers when creating the struct.

  • in robometry making sure that no signal can have empty name.

Hi @S-Dafarra! Really thank you for deepering in the problem! I was wondering if the problem is in the camera signal in the mat file

I noticed that camera.jabra.rgb is empty, while camera.realsense.rgb.data seems to contain only 1.7153e+09 repeated 20371 times

The Jabra Camera was disable for the last recording if I remember well. On the other hand, the realsense data present in the mat file is just the timestamp of each consecutive frame, so I would say it's accurate.

S-Dafarra commented 2 months ago

I wonder whether in a "good" dataset the entries on the main level are the same of #846 (comment)

This is how it should look like image

The main suspect is that the "orientations" field appeared with a null name.

GiulioRomualdi commented 2 months ago

Looking at the YarpRobotLoggerDevice I noticed that robometry::BufferManager m_bufferManager; is placed at line 178 while the names of the variables start from line 206.

The buffer manager saves the matfile while its destructor is called however as explained in https://stackoverflow.com/questions/2254263/order-of-member-constructor-and-destructor-calls the members are guaranteed to be initialized by order of declaration and destroyed in reverse order. This may create issues while saving the file

A possibility is to store the buffermanager in a unique_ptr and manually handle the destruction

GiulioRomualdi commented 2 months ago

After several attempts, we understood the problem was related to the connection of an exogenous signal vector. We had two main problem:

  1. initMetadata uses the signalFullName for setting the channel name. However signalFullName is not set for classical vector signal. https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/devices/YarpRobotLoggerDevice/src/YarpRobotLoggerDevice.cpp#L839
  2. YarpRobotLoggerDevice::initMetadata uses the metadataNames to compute the size of the channel of the vector. https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/devices/YarpRobotLoggerDevice/src/YarpRobotLoggerDevice.cpp#L641 However, in some places, the provided metadata is empty so the the channel size is set to zero. https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/devices/YarpRobotLoggerDevice/src/YarpRobotLoggerDevice.cpp#L839
GiulioRomualdi commented 2 months ago

I will open the PR with the modification discussed in https://github.com/ami-iit/bipedal-locomotion-framework/issues/846#issuecomment-2104786532

Still, I think we need to add a test perhaps using the new gazebo (cc @xela-95) to intercept this earlier.

GiulioRomualdi commented 1 month ago

Cc @nicktrem

GiulioRomualdi commented 1 month ago

I renamed the issue to make it clearer. Currently, the logger is broken when a vector signal is logged as exogenous. No problems for vectors collection

GiulioRomualdi commented 1 month ago

https://github.com/ami-iit/bipedal-locomotion-framework/pull/849 should fix the issue