Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New test for IMU orientation #65

Merged
merged 11 commits into from
Jan 18, 2024
Merged

New test for IMU orientation #65

merged 11 commits into from
Jan 18, 2024

Conversation

martinaxgloria
Copy link
Contributor

As per title, this PR added a new test that can be run both in simulation environment and on the real robot to test the Euler angles retrieved from the IMU mounted on the RFE.

So far, the test could be run on iCubGenova11 with:

robottestingframework-testrunner --verbose --test plugins/imu.so --param "--robot icub --model /usr/local/src/robot/robotology-superbuild/build/install/share/iCub/robots/iCubGenova11/model.urdf --port /icub/head/inertials --sensor head_imu_0 --frame head_imu_0"

For the future, when probably we'll have to run multiple tests at the same time, there is also the skeleton for the test suites.
A side note (imho it's not so important for the time being): if we want to use the fixture parameter, we have to define and install a .sdf file that will be used to load the model on gazebo. This part is commented out right now, and if someone would launch the test in simulation, the gazebo environment should be launched before on another shell.
I will write all the necessary information in the documentation

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @martinaxgloria!

We have only to figure out how to handle the new dependencies

src/imu/CMakeLists.txt Outdated Show resolved Hide resolved
@Nicogene
Copy link
Member

cc @GiulioRomualdi @traversaro

@Nicogene Nicogene closed this Jan 17, 2024
@Nicogene Nicogene reopened this Jan 17, 2024
axesList.addString("r_shoulder_pitch");
axesList.addString("r_shoulder_roll");
axesList.addString("r_shoulder_yaw");
axesList.addString("r_elbow");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list can be parametrized in future improvements in order to be able to test sensors in other kinematic chains

src/imu/imu.cpp Outdated Show resolved Hide resolved
double minLim;
double maxLim;

for (int i = kinDynComp.model().getJointIndex("neck_pitch"); i <= kinDynComp.model().getJointIndex("neck_yaw"); i++)
Copy link
Member

@Nicogene Nicogene Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this can be generalized it making it a parameter, ideally the user has to specify the 3 joints that have to be controlled, let's remind it for future versions of the test

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only this comment has to be addressed now:

The rest we can be addressed it later

@Nicogene
Copy link
Member

The CI is failing due to macOs but I have remove it from the CI, we do not support brew anymore

src/imu/imu.h Outdated Show resolved Hide resolved
src/imu/CMakeLists.txt Outdated Show resolved Hide resolved
src/imu/CMakeLists.txt Outdated Show resolved Hide resolved
src/imu/CMakeLists.txt Outdated Show resolved Hide resolved

auto error = (expectedImuSignal * imuSignal.inverse()).log();
double err_mean = (std::accumulate(error.begin(), error.end(), 0)) / error.size();
ROBOTTESTINGFRAMEWORK_ASSERT_ERROR_IF_FALSE(err_mean < 0.1, "Error > 0.1! Aborting ...");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to @Nicogene comments, in the future, the threshold could be a parameter, as if for some model/sensors are quite accurate, you may want to make sure that they contibute to be accurate.

src/imu/imu.h Outdated Show resolved Hide resolved
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! A possible future improvements (that are not blocking the PR or need to be done at all cost, but I want to list them here to avoid forgetting):

  • Support specifying multiple sensors
  • Support for automatically testing all the imus (we can get the list from the iDynTree's model)
  • Support for saving tests data in a file for future inspection

src/imu/imu.cpp Outdated Show resolved Hide resolved
@martinaxgloria
Copy link
Contributor Author

I think I integrated all the proposed changes in 03c32b2, except for the parameterization. Please, let me know if I forgot something

cc @Nicogene @traversaro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants