Skip to content

Use double/int_least64_t for coordinates to support GPS RTK#83

Open
rovo89 wants to merge 2 commits intokosma:masterfrom
rovo89:double
Open

Use double/int_least64_t for coordinates to support GPS RTK#83
rovo89 wants to merge 2 commits intokosma:masterfrom
rovo89:double

Conversation

@rovo89
Copy link
Copy Markdown

@rovo89 rovo89 commented Feb 9, 2025

The README mentions that five decimal digits can be parsed for coordinates. However, GPS RTK modules are built for even higher precision and therefore send eight decimal digits (tested with a UM980 module by Witmotion). This precision gets lost when using float/int_float32_t. Therefore, this PR introduces minmea_double with int_least64_t value and scale and uses them for coordinates. The easier way would be adjusting minmea_float, but I didn't want to add overhead to fields that don't need such precision and it would be strange to have a type with that name handled like a double.

Tests are adjusted, README not yet. I can do that if you show interest to actually merge this.

GPS RTK modules like Unicore UM9xx have a higher precision for latitude
and longitude.
@yahyayozo
Copy link
Copy Markdown

@rovo89 I think this would be very helpful, I got the same issue with precision when working with ZED-F9P module
Are you willing to continue with this PR?

@rovo89
Copy link
Copy Markdown
Author

rovo89 commented Nov 20, 2025

From technical point of view I think it's fine, so you can just use my fork. Of course I'd prefer to get this merged, and I'd adjust the README for that, but as long as there is no interest by the maintainers, I don't want to spend the time.

@kosma
Copy link
Copy Markdown
Owner

kosma commented Jan 13, 2026

I can't decide if this is a good idea or not. Pushing everyone to use 64-bit values can get expensive on smaller platforms, which this library aims to support and embrace. But then, high-precision is also a valid use case. Perhaps we should have a defined type for the lat/lon/alt values, and allow the user to redefine it if they don't like the default 32-bit precision? Or have a #define flag that changes everything to higher precision? What do you think?

@hraftery
Copy link
Copy Markdown
Contributor

I think the same considerations from #72 apply. Based on that discussion, I strongly oppose introducing operations on doubles on unsuspecting uses. However, I'm not sure about merely storing 64 bit integers.

Treating the two considerations separately, the operations on doubles one is easier to manage. I believe (hope?) we can simply provide functions like minmea_tocoord_double() as an alternative to minmea_tocoord(), and let the user choose. The mere presence of operations on doubles in the code are not an issue, only forcing their use is.

So the more fundamental consideration is what to store the coord in. RTK requires 64 bits be available to capture the 13 digits that may appear on the wire. If you're not using a RTK receiver on a 32-bit platform, 32 bits go to waste and scanf does an unnecessary long long operation. The overhead is not something that can be avoided at run time.

In that case, I can't think of anything less problematic than a build time define that enables 64 bit storage. By defaulting to off, it eliminates unintended consequences, and existing users are unaffected. And as a build time define, when on there is no runtime complexity that might undermine performance or static analysis - it is always 64 bit.

Perhaps then over time, as take up of the 64 bit option becomes prevalent and confidence in the impact grows, the 32 bit option could be superseded.

But I stress that forcing 64 bit on everyone is a minefield. There's a fine demonstration of that in the PR - the int_least64_t values appear to be printf'ed using %ld. That assumes a 64-bit platform and I believe will fail on 32-bit. This is just one common example of 64-bit assumptions. The correct formatter in this case is PRId64.

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