-
Notifications
You must be signed in to change notification settings - Fork 23
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
icub3: add complete joint test #193
Conversation
e1a88c9
to
1fd0916
Compare
25b7700
to
f6ad7e0
Compare
expectedDirectionInRootLink.push_back(iDynTree::Direction(-0.267012,-0.960459,-0.0788901)); | ||
expectedDirectionInRootLink.push_back(iDynTree::Direction(0.0713662,0.0619303,-0.995526)); | ||
expectedDirectionInRootLink.push_back(iDynTree::Direction(-0.961047,0.271447,-0.0520081)); | ||
expectedDirectionInRootLink.push_back(iDynTree::Direction(0.267012,0.960459,0.0788901)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being picky, but the code as written in this form would be quite complicate to maintain, as it is difficult to understand which axis correspond to which joint.
Could it make sense to write it as:
axisNames.push_back("torso_roll");
expectedDirectionInRootLink.push_back(iDynTree::Direction(-1,0,0));
axisNames.push_back("l_hip_pitch");
expectedDirectionInRootLink.push_back(iDynTree::Direction(0,-1,0));
axisNames.push_back("r_hip_pitch");
expectedDirectionInRootLink.push_back(iDynTree::Direction(0,-1,0));
And so on so forth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now should be clearer, it would be better to creat a struct with name and expected rotation but it may be refactored in the future
tests/icub-model-test.cpp
Outdated
ok = ok & checkFTSensorIsCorrectlyOriented(comp, rootLink_R_sensorFrameExpected, "l_leg_ft_sensor"); | ||
ok = ok & checkFTSensorIsCorrectlyOriented(comp, rootLink_R_sensorFrameExpected, "r_leg_ft_sensor"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extremely strange. I would expect the FT frames of the FT in the legs to have the Z direction corresponding to the root link, and then having just an additional rotation in the Z direction due to how the sensor is mounted, as it happens in the feet. As we did not validated the FT frames in the model, this may suggest that the frame orientation is not correct, so for the time being I would avoid putting them in the tests if we do not know if this frames are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, moreover now the fts in the legs are disabled, then I remove them from the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments.
f6ad7e0
to
0331188
Compare
0331188
to
b10d0b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
Feel free to merge in the form you prefer (squash/no squash). |
It fixes #179.
It also fixes partially #181
cc @GiulioRomualdi