-
Notifications
You must be signed in to change notification settings - Fork 70
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
[AttitudeEstimator] [Span] adding fixes, features, bindings and tests #522
Conversation
I am a bit confused by these changes:
In general, I am not sure what is the problem that caused this change. In some condition you are providing invalid data to the algorithm? |
The problem actually arose with the initial angular velocity state estimate being set to all zeros. And true, it doesnt make sense to normalize the quaternion in the
That's true. We need to add a check, if they are unit vectors or not, and act accordingly. My bad, I was not thinking straight. I shall make the changes. |
Wipping for now. |
cc @GiulioRomualdi @traversaro added checks on measurements and unit quaternion. |
@@ -95,7 +123,7 @@ bool iDynTree::AttitudeMahonyFilter::propagateStates() | |||
|
|||
// system dynamics equations | |||
q = q + (dq*(m_params.time_step_in_seconds*0.5)); | |||
if (q.norm() != 0) | |||
if (q.norm() != 0 || q.norm() != 1) |
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.
Realized that this chould be &&
condition to be a stronger check. At the moment, still could pass as a weaker check! Would you like me to change this to && ?
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 am confused. How can the norm be both 0.0 and 1.0 ?
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.
Normalize only if the norm is not zero (to avoid nan) and the norm is not one(to become unit quaternion if already not)
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.
Does it make sense ? Or should we actually split it into two conditions ?
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.
Why the quaternion should be ever 0.0?
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.
Because we only typedef UnitQuaternion and not type check. We don't establish anywhere that the vector will have unit norm or not have zero norm at all.
Worst case check!
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.
If the quaternion norm is zero, then an error should be emitted at some point, and the propagateStates
method shoul return false
, right?
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.
True. Makes sense. I will change the condition.
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.
@traversaro fixed this condition and improved the test.
{ | ||
// to avoid nan | ||
toEigen(m_Acc_y).normalize(); | ||
iDynTree::reportError("AttitudeMahonyFilter", "updateFilterWithMeasurements", "Cannot retrieve unit vector from linear acceleration measuremnts."); |
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.
AttitudeQuaternionEKF
iDynTree::Vector3 magMeasUnitVector; | ||
if (!getUnitVector(magMeas, magMeasUnitVector)) | ||
{ | ||
iDynTree::reportError("AttitudeMahonyFilter", "updateFilterWithMeasurements", "Cannot retrieve unit vector from magnetometer measuremnts."); |
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.
same.
@@ -434,6 +449,10 @@ bool iDynTree::AttitudeQuaternionEKF::ekf_f(const iDynTree::VectorDynSize& x_k, | |||
auto dq(toEigen(composeQuaternion2(orientation, correction))); | |||
|
|||
x_hat_plus.block<4,1>(0, 0) = q + (dq*(m_params.time_step_in_seconds*0.5)); | |||
if (q.norm() != 0 || q.norm() != 1) |
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.
same as above
Is this WIP or it is ready for review? |
Yes, by itself, it should be ready. However, I am running some tests with actual data to see if the implementation checks. Also, if it is ok to add generated matlab bindings along with this PR, then I think it can still be wip-ped. |
You have some specific settings to compile this? On my Ubuntu 18.04 machine this branch is failing with error:
|
@traversaro I think this PR is good to go, except that the Travis is complaining for some iDynTree Lua bindings. Please take a look and let me know. |
The failures are just on Xenial, as we are phasing out Xenial support, I think there is no problem. Can you disable bindings testing on the Xenial jobs? Thanks! |
Could you please tell me how to do that? |
friendly ping @traversaro |
Sorry, I guess the message was not sent. You need to change the |
Th travis still complains,
Server issues? |
There is a similar feailure in the devel branch, I think it is something independent from the PR. |
Last request: can you update the release notes https://github.com/robotology/idyntree/blob/devel/doc/releases/v0_12.md ? |
For the future: when you re-generate Matlab bindings, please commit them in a separate commit w.r.t. to the changes in the |
Done. Thank you. Excuse me for not adhering to the conventional way of committing bindings. I did not pay attention. I will take care of that in the future. |
Thanks! |
Thanks to you! |
cc @traversaro