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

Imu test - test multiple IMUs and save data #69

Merged
merged 11 commits into from
Mar 1, 2024

Conversation

martinaxgloria
Copy link
Contributor

This PR adds the possibility to run the test for multiple IMUs at a time to check the Euler angles values.

These changes were tested both in simulation and on the real robot (iCubGenova11).

@pattacini pattacini changed the base branch from master to devel February 15, 2024 15:02
@pattacini
Copy link
Member

Changed base branch to devel.

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.

Excellent work!

Just a comment not blocking the PR!

src/imu/imu.h Outdated Show resolved Hide resolved
@martinaxgloria martinaxgloria changed the title Imu test - test multiple IMUs at time Imu test - test multiple IMUs and save data Feb 22, 2024
@martinaxgloria
Copy link
Contributor Author

In 46d8d1c I integrated the robometry framework to save relevant measurements (i.e. expected and measured orientation data, joints positions and velocities, error, and so on) for future inspections.

cc @Nicogene @traversaro

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.

@martinaxgloria consider rebasing your branch over latest devel:

Introduced conda-based dependencies that speed up the CI. Please also enable this test in the CI and add the required dependencies

@martinaxgloria
Copy link
Contributor Author

Yes, thanks @Nicogene, I'll do it

@martinaxgloria
Copy link
Contributor Author

In d35471a, I implemented the possibility of testing all the available IMUs by passing to the sensorsList parameter an empty list or all. Moreover, I added a bash that contains some ctpService calls to set a sequence of movements of the interested joints (left_arm, right_arm and neck ones) during which the sensors were tested.

cc @Nicogene @traversaro

@martinaxgloria
Copy link
Contributor Author

Tested on iCubGenova11:

test-iCubGenova11.mp4

Since robotology/icub-models-generator#233, the test result is:

fail

cc @Nicogene @traversaro

@traversaro
Copy link
Member

Great!

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!

Some small points to be addressed, but we have almost landed 🛬

Have you forgot to commit the move.sh script?

src/imu/imu.cpp Outdated Show resolved Hide resolved
src/imu/imu.cpp Outdated Show resolved Hide resolved
src/imu/imu.cpp Outdated Show resolved Hide resolved
src/imu/imu.h Outdated Show resolved Hide resolved
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.

Ah I forgot, now that the CI is conda based should be easy to enable the compilation of this test in the CI adding also the additional dependencies

@martinaxgloria
Copy link
Contributor Author

Ah I forgot, now that the CI is conda based should be easy to enable the compilation of this test in the CI adding also the additional dependencies

Done here 936c4e3 and 513a57a

cc @Nicogene

@Nicogene Nicogene merged commit f6de7ed into robotology:devel Mar 1, 2024
3 checks passed
@martinaxgloria martinaxgloria deleted the imu-test-stint2 branch March 1, 2024 13:17
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.

4 participants