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 - define and configure parameters of the test in the .ini file #67

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

martinaxgloria
Copy link
Contributor

This PR aims to add the proposed improvements that came out from this PR review.

  • the doxygen documentation was added taking as reference the other tests in the repo;
  • make the controlboards a parameter to be defined in the test_imu.ini;
  • delete the hardcoded lists of joints and controlboards, making the code more generalized;
  • make the tolerance on the mean error as a parameter

cc @Nicogene

@martinaxgloria martinaxgloria self-assigned this Jan 24, 2024
@martinaxgloria martinaxgloria changed the title Imu test stint1 Imu test - define and configure parameters of the test in the .ini file Jan 24, 2024
src/imu/imu.cpp Outdated Show resolved Hide resolved
src/imu/imu.h Outdated Show resolved Hide resolved
@martinaxgloria martinaxgloria marked this pull request as ready for review January 25, 2024 07:17

robotName = property.find("robot").asString(); // robot name
portName = property.find("port").asString(); // name of the port from which the data are streamed
partName = property.find("part").asString(); // name of the part of the robot on which the sensor is mounted
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

@traversaro
Copy link
Member

As mentioned in an internal issue, are you sure that the code like this is able to test an IMU on the arm of the iCub? My guess is that loadReducedModelFromFile will fail, as we are asking it to load a model passing as "physical joint" the name of "actuated axis" for the hand. Now that I think of, I am afraid there is the same problem also on the head of any physical iCub, as the head also contains "actuated axis" like eyes_version or eyes_vergence that do not correspond to any physical joint in the URDF. I guess think worked in simulation as in simulation in most models we do not have the problematic joints, but it may emerge if you test with the visuomanip model.

@martinaxgloria martinaxgloria marked this pull request as draft January 25, 2024 08:40
@martinaxgloria
Copy link
Contributor Author

You were right @traversaro, this is the error with iCubGazeboV2_5_visuomanip model:

image

For the time being, I converted this into a draft PR. I'm going to find another solution

@martinaxgloria
Copy link
Contributor Author

I thought about a different solution, but I didn't find any. The only solution is to pass the controlled joints as a list in the configuration file (as per the controlboards), so that we can avoid the presence of the eyes_vergence or any other physical joints.

For the part that concerns the joints movement of the part on which the sensor is mounted, that is:

icub-tests/src/imu/imu.cpp

Lines 174 to 202 in a9b26df

double minLim;
double maxLim;
yarp::os::Property tmpOptions;
int tmpAxisNum;
std::string tmpString;
tmpOptions.put("device", "remote_controlboard");
tmpOptions.put("local", "/tmp");
tmpOptions.put("remote", "/"+robotName+"/"+partName);
tmpDriver.open(tmpOptions);
tmpDriver.view(tmpEncoders);
tmpDriver.view(tmpAxis);
tmpEncoders->getAxes(&tmpAxisNum);
for (int i = 0; i < tmpAxisNum; i++)
{
tmpAxis->getAxisName(i, tmpString);
ilim->getLimits(model.model().getJointIndex(tmpString), &minLim, &maxLim);
moveJoint(model.model().getJointIndex(tmpString), minLim + 5);
yarp::os::Time::delay(1.);
moveJoint(model.model().getJointIndex(tmpString), maxLim - 5);
yarp::os::Time::delay(1.);
}
tmpDriver.close();

There would be the same problem as before because opening a remote_controlboard for the remote port /icub/head and making the view on the IAxis interface, will return also the "actuated joints". Since I wasn't able to find a clean solution, as a workaround I thought that we could add a check: we could verify that the i-th joint retrieved with getAxisName was contained in the axesVec, which is the list of the joints passed in the .ini file.

cc @traversaro @Nicogene

@martinaxgloria
Copy link
Contributor Author

Talking f2f with @traversaro about the last comment, he suggested a better solution in terms of implementation. Instead of that workaround, it would be better to pass a list of the joints to be moved during the test inside the .ini configuration file. In this way, it would be easier (and also in terms of the execution time of the test) to test all the IMUs in future improvements. This would avoid the definition of another remote_controlboard which is unnecessary if we define at the beginning the remotecontrolboardremapper, which has all the information we need.

Thanks again @traversaro for the hints!

cc @Nicogene

@martinaxgloria
Copy link
Contributor Author

With b21417d, the last proposed changes have been implemented: now the list of movable joints is a parameter, as per the controlboards and the mean error value.
This is what the test looks like now:

Screencast.from.01-31-2024.11.43.38.AM.webm

Except for how the error is computed so far (tracked in another issue, to be fixed) and for the issues in the frame of the imu (roll and pitch values are swapped and the roll values jump between -pi and pi), it seems that the refactoring didn't modify the functionality of the test. I'm going to put this PR as ready for review

cc @Nicogene @traversaro

@martinaxgloria martinaxgloria marked this pull request as ready for review January 31, 2024 11:13
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.

LGTM!

@Nicogene Nicogene merged commit e579b72 into robotology:master Feb 1, 2024
2 checks passed
@martinaxgloria martinaxgloria deleted the imu-test-stint1 branch February 1, 2024 09:01
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