-
Notifications
You must be signed in to change notification settings - Fork 43
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
Parse remaining fields for RMC sentence #66
Parse remaining fields for RMC sentence #66
Conversation
Thank you for working on this issue with RMC sentences. I will make sure to review the PR in the coming days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The only change is the double
(f64
) usage in one of the parsers.
I've opened a separate issue to investigate whether f64
accuracy is needed and if so, to find option that will allow the user to choose between f64
& f32
: #67
Codecov ReportBase: 79.24% // Head: 79.41% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 79.24% 79.41% +0.16%
==========================================
Files 22 22
Lines 824 850 +26
==========================================
+ Hits 653 675 +22
- Misses 171 175 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Thank you for working on this issue! 🎉 |
This PR solves issue #60 and adds parsing of the remaining fields of RMC sentences:
I added a couple more basic tests to check for sentences with 11, 12, or 13 fields, as well as some other tests for missing fields.
Magnetic variation
Since magnetic variation is a field combination (degrees + E/W) in a couple of other sentences (HDG and RMA), I added a combinator in
utils
for it that can be reused. However I'm not sure whether my implementation is correct, I followed the explanations from Magnetic Declination 3.2 - Non-adjustable compasses, but please double-check that both implementation and test are correct.Navigational status
Following the discussion on Discord, I found a specification of RMC by Orolia, where the values used for the nav status field are different, and conflicting with the ones mentioned by GPSD's RMC description. It was decided that we should stick to GPSD compliance for the current implementation and parse the field into an
RmcNavigationStatus
enum value instead of achar
.