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

Use Geodetic Converter for NED<->GPS conversions #3839

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

rajat2004
Copy link
Contributor

Fixes: #

About

This PR updates the environment code to use GeodeticConverter instead of EarthUtils which is potentially more inaccurate, described a bit in #1855 (comment), #3364 (comment)

The PR also includes the fixes described in ethz-asl/geodetic_utils#36, ethz-asl/geodetic_utils#28

How Has This Been Tested?

A fair bit of testing needs to be done, to verify that GPS inaccuracy doesn't increase. Unit tests for GeodeticConverter itself which be a good option, something to keep in mind. This was earlier tested with ArduPilot without any problems, but also without any noticeable improvement in the GPS performance. Testing with PX4 will also be good

Screenshots (if appropriate):

@jonyMarino
Copy link
Collaborator

Hi @rajat2004! What is the state of this PR?

@rajat2004
Copy link
Contributor Author

I haven't been able to test this out, and verify if the GPS accuracy is better or worse. It can be closed if preferred

@zimmy87 zimmy87 marked this pull request as ready for review April 4, 2022 18:26
@zimmy87
Copy link
Contributor

zimmy87 commented Apr 4, 2022

We did some sanity-checking on the new output of the GPS sensor, and it looks reasonable to us, plus we didn't observe any performance regression with this PR applied, so we decided we'd like to merge this. thank you for the submission, @rajat2004!

@zimmy87 zimmy87 merged commit 0a3263b into microsoft:master Apr 4, 2022
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.

3 participants