-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Hamilton Quaternion Multiplication Order #7908
Conversation
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 was worried about attitude_estimator_q, but it seems to be using the old math library in PX4 still. I verified LPE still flies in SITL on this branch.
@priseborough Have you been able to test this on ekf2 yet? |
2fe7acb
to
2b405d7
Compare
I just updated this to master to do some flight testing with it. |
I have flight tested it with https://github.com/PX4/ecl/tree/pr-ekfQuatMultOrder and it works |
See PX4/PX4-ECL#323 |
2b405d7
to
44a4655
Compare
I rebased with successful auto-merge again foremost to run CI again on an up-to-date branch. |
Apparently VTOL does a flyaway with these changes as luckily CI succeeds to show. I can reproduce this in SITL. Let me fix that. |
I found a clue: When I update the ecl submodule as well like @priseborough wrote in #7908 (comment) then everything works again. I expected the two matrix submodules to be included without overlap such that either the ecl or the Firmware could use a different version of matrix and does not need to be synced at all time.
This means as soon as the ecl library is compiled within the PX4 Firmware it doesn't matter anymore what matrix library is checked out in the ecl library itself because for every call to a matrix also in the ecl library the matrix submodule in PX4 Firmware gets used. Anyways I'll update the ecl submodule in this pr and I would kindly ask @priseborough to merge PX4/PX4-ECL#323 soon such that the PX4/PX4-ECL@dd5b852 commit is in the ecl/master history. |
44a4655
to
702b688
Compare
…d according ecl changes Note: ecl needs to be updated at the same time because as soon as the ecl is compiled within PX4 Firmware the matrix submodule checked out by PX4 Firmware is used for every call to a matrix or quaternion.
… all remaining modules
702b688
to
00e14d8
Compare
Thanks @priseborough for promptly merging PX4/PX4-ECL#323 such that I could checkout the merge commit PX4/PX4-ECL@160e4d6 (current ecl/master) here by interactive rebase. I did SITL tests with MC and VTOL and now everything should be fine and ready to merge. |
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.
Looks all good. Let's get this in!
After merging PX4/PX4-Matrix#34 I went through the entire Frimware source and was surprised to find out that there is no non-commutative quaternion multiplication of two
matrix::Quaternion
s except for the ecl library. I hope I'm not missing anything here but it seems to me that the library is mainly user for conversions Dcmf, Eulerf, Quatf. And the only test for this multiplication outside of the matrix library is one a quaternion with itself here: https://github.com/PX4/Firmware/blob/2b714e079b97976da1b6f633699f7b79f1c01389/src/systemcmds/tests/test_matrix.cpp#L216-L220What I ended up switching are some calls that were simplified in the past but not applied to any instance in the modules.
@dagar Please quickly check the
.copyTo
call.@priseborough Please add whenever you're finished and ready for testing your pending changes to the ecl from here https://github.com/PX4/ecl/tree/pr-ekfQuatMultOrder to this pr. I'm not sure what happens when you have a different commit checked out in your own matrix submodule than in the submodule I update here.