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

fix AHRS offset accumulation #16

Merged
merged 1 commit into from
Nov 19, 2017
Merged

Conversation

ktahar
Copy link
Contributor

@ktahar ktahar commented May 23, 2017

Due to sign inversion here, offsets were doubled rather than canceled in AHRS::updateIMU() (line 136-138 in AHRS.hpp).

@staroselskii
Copy link
Contributor

Sorry it's taken us so long to get back to you. Is it still something you'd like to have merged?

@ktahar
Copy link
Contributor Author

ktahar commented Nov 17, 2017

Yes. I think this one should be reviewed and hopefully merged.
Doubled offsets make AHRS's output rotate without any real rotation of the sensor.
PR will fix this behavior.

@staroselskii
Copy link
Contributor

@ksktahara

Awesome.

Could you please, add a little more context to the commit itself and also conform to the git style we're using. This commit might be used as a reference.

Under context I understand:

  • the motivation
  • how this commit fixes the problem
  • why

Linked list in commit message is of course not a requirement. A commit just should have as much information as possible for later period of time. It'd be also nice if you could rebase on master as there's a conflict right now.

Thank you for your commitment!

remove negative sign at accumulated offset because the offset is subtracted from sensor reading
@ktahar
Copy link
Contributor Author

ktahar commented Nov 19, 2017

Thank you for the review.
I've rebased on master and fixed the message to conform to your style.

@staroselskii staroselskii merged commit 84573fe into emlid:master Nov 19, 2017
@staroselskii
Copy link
Contributor

Awesome. Thanks!

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.

None yet

2 participants