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

Add option to convert position into Area ID #197

Merged
merged 8 commits into from
Apr 1, 2024

Conversation

jbangelo
Copy link
Contributor

This adds another command line flag to automatically convert the given position into an Area ID (--pos-to-area-id). If either this new flag or the --area-id argument are given then the CRA message with Area ID is sent. I also changed several of the uses of f64 to use f32 instead to make sure there weren't rounding issues in the Area ID calculations.

Is this an acceptable way to squeeze in this functionality? I was also thinking about adding these functions into https://github.com/swift-nav/swiftnav-rs, but that's not a dependency for ntripping and would potentially complicate the portability since it pulls in C code as well.

src/main.rs Outdated Show resolved Hide resolved
@jbangelo
Copy link
Contributor Author

One last change added to error check the latitude/longitude values, but ended up pulling in anyhow

Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

So the more i think about this with the offset & the call to trunc, I am worried about if this will exactly match the C code. At 54_000_000 you've actually ran out of bits to describe integer intervals in single precision, and the choice to do the math in floating point then cast to u32 may not be the same as casting to u32 then doing the math.

I'd have to see the ground truth of the C code, to know if this matches but I added a suggestion.

For what it's worth, if you did this all in double precision the numbers would be able to describe exact integers at those sizes of numbers but you made the change to f32 explicitly to match the other implementation.

I don't have context to the importance of getting this exactly right for all inputs, so if this is just navel gazing, feel free to just ignore my comment and merge away.

src/main.rs Outdated Show resolved Hide resolved
@jbangelo jbangelo merged commit cbb939c into master Apr 1, 2024
5 checks passed
@jbangelo jbangelo deleted the jbangelo/add-area-id-calculation branch April 1, 2024 23:26
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