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

bugfix: altitude parsing problem #172

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

dronesalot
Copy link
Contributor

@dronesalot dronesalot commented Nov 17, 2024

Solved Problem

The NMEA format allows for multiple fix quality characters. The default implementation allows only 4 of these items to be parsed from the stream. When using a newer UM980 GPS unit, it supports 5 different types and causes the parser to mis-read the altitude message. This results in the GPS altitude jumping to 0 whenever a GNS message is received. This resolves the issue by ignoring any additional characters in the parsed message.

The PR also adds the ability to intrepret GLL messages from the GPS.

Fixes #23903

Solution

  • fixes a bug in the NMEA parsing that is specific to newer GPS units

Changelog Entry

For release notes:

Feature/Bugfix Add parsing for the GLL message and fix a parsing bug in the GNS if posMode is more than 4 chars

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/um982-isnt-providing-heading-data/35195/29

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/local-position-dist-bottom-valid-toggling-issue/42326/4

@julianoes julianoes self-requested a review November 25, 2024 06:27
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Have you tested this with both the new and older version?

Or should I cross test with my UM982?

@dronesalot
Copy link
Contributor Author

Thanks! Have you tested this with both the new and older version?

Or should I cross test with my UM982?

Not sure what you mean by older version.

I'm confident with the changes on this and any NMEA GPS receiver since the bug was the inability to properly parse a valid message that the UM98x will generated. I don't think testing with the UM982 is necessary since the fix was straightforward.

I have compiled and flown this with the 15.x branch, but not the 14. Unfortunately I keep missing the weekly developer meetings as it would be easier to describe and check in and present my future ideas for an updated parser.

Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For next time, I'd do the rename of the fieldCount variable which is a refactor in a separate commit from the actual change/addition. That makes it easier to review and reason about.

I will merge this and add a few of my own fixes in another pull request. Would be good to get your testing on that too.

@julianoes julianoes merged commit d1630d4 into PX4:main Nov 27, 2024
@dronesalot
Copy link
Contributor Author

For next time, I'd do the rename of the fieldCount variable which is a refactor in a separate commit from the actual change/addition. That makes it easier to review and reason about.

I will merge this and add a few of my own fixes in another pull request. Would be good to get your testing on that too.

No problem with keeping refactoring in separate revisions. I can test your work if you like.

I've already gotten partway through a complete re-wite. So far, so good as it's less than 1/2 the size. Haven't figured out a good way to measure efficiency, but I'm going to guess at least 10x faster. When I have a pull request for that, the entire thing will be totally different.

@julianoes
Copy link
Contributor

@dronesalot when you're rewriting this, you might consider that we want to pull it into PX4 as a separate driver, like it was done for Septentrio:

https://github.com/PX4/PX4-Autopilot/tree/main/src/drivers/gnss/septentrio

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