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

Hamilton Quaternion Multiplication Order #7908

Merged
merged 2 commits into from
Sep 21, 2017
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Sep 4, 2017

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::Quaternions 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-L220

What 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.

dagar
dagar previously approved these changes Sep 4, 2017
jgoppert
jgoppert previously approved these changes Sep 5, 2017
Copy link
Member

@jgoppert jgoppert left a 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.

@jgoppert
Copy link
Member

@priseborough Have you been able to test this on ekf2 yet?

@jgoppert jgoppert dismissed stale reviews from dagar and themself via 2b405d7 September 11, 2017 15:49
@jgoppert
Copy link
Member

I just updated this to master to do some flight testing with it.

@priseborough
Copy link
Contributor

I have flight tested it with https://github.com/PX4/ecl/tree/pr-ekfQuatMultOrder and it works

@priseborough
Copy link
Contributor

See PX4/PX4-ECL#323

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Sep 18, 2017

I rebased with successful auto-merge again foremost to run CI again on an up-to-date branch.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Sep 20, 2017

Apparently VTOL does a flyaway with these changes as luckily CI succeeds to show. I can reproduce this in SITL. Let me fix that.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Sep 20, 2017

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.
Then all the following combinations should work:

PX4                               ecl                                   MC                VTOL
old matrix                        old ecl/matrix                        works             works
new matrix + no needed changes    old ecl/matrix                        fails             fails
old matrix                        new ecl/matrix + needed ekf changes   fails             fails
new matrix + no needed changes    new ecl/matrix + needed ekf changes   works             works

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.
@priseborough @LorenzMeier Is this by design/desired?

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.

…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.
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Sep 21, 2017

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.
Anyone up for a review?

Copy link
Member

@bkueng bkueng left a 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!

@bkueng bkueng merged commit 5bea264 into master Sep 21, 2017
@bkueng bkueng deleted the matrix-quaternion-hamilton branch September 21, 2017 15:45
@MaEtUgR MaEtUgR mentioned this pull request Sep 23, 2017
6 tasks
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.

5 participants