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

Implement PGRMZ support #59

Merged
merged 1 commit into from
Oct 16, 2022
Merged

Implement PGRMZ support #59

merged 1 commit into from
Oct 16, 2022

Conversation

Turbo87
Copy link

@Turbo87 Turbo87 commented Oct 6, 2022

$PGRMZ is used by a lot of gliding computers like the https://gliding.lxnav.com/products/lx9000/ to transmit the current barometric altitude.

This PR implements basic support for it. It is currently implemented to output the raw altitude in feet without any conversions.

@elpiel
Copy link
Member

elpiel commented Oct 10, 2022

Great progress for the support of RMZ messages.

There are a few points that need addressing:

  • Add new sentence type to the fn parse_str()
  • Check not only the message_id (RMZ) but also the talker_id, it should be PG. I think we can create an UnknownTalkerId error and pass a custom expected value (&'a [&'a str]) for PG
  • It would be great to also add tests regarding this case (talker_id != PG should give out an error)

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Base: 76.80% // Head: 76.76% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (f65c8cc) compared to base (03695a6).
Patch coverage: 75.00% of modified lines in pull request are covered.

❗ Current head f65c8cc differs from pull request most recent head 7b0ef1f. Consider uploading reports for the commit 7b0ef1f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
- Coverage   76.80%   76.76%   -0.04%     
==========================================
  Files          20       21       +1     
  Lines         776      792      +16     
==========================================
+ Hits          596      608      +12     
- Misses        180      184       +4     
Impacted Files Coverage Δ
src/parser.rs 79.91% <ø> (ø)
src/sentences/mod.rs 0.00% <ø> (ø)
src/sentences/rmz.rs 75.00% <75.00%> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Turbo87
Copy link
Author

Turbo87 commented Oct 13, 2022

  • Add new sentence type to the fn parse()

tbh I'm not convinced that we should be merging this data with the rest in the parse() function. the PGRMZ sentence usually contains a barometric or pressure altitude, while the regular NMEA sentences contain GPS altitudes, which can often have significant offsets due to the current weather situation. if we would merge them and pretend like they contain the same information then the altitude will regularly jump all over the place.

@elpiel
Copy link
Member

elpiel commented Oct 14, 2022

@Turbo87 I was referring to the parse_str(I made a typo) which is able to parse all types of sentences, not like the NMEA::parse():

pub fn parse_str(sentence_input: &str) -> Result<ParseResult, Error> {

@Turbo87
Copy link
Author

Turbo87 commented Oct 14, 2022

Ah, I see. That makes sense :)

I'll update the PR in the next couple of days once EuroRust is over

@elpiel
Copy link
Member

elpiel commented Oct 14, 2022

Awesome, thank you 🥳

@elpiel elpiel merged commit 500175e into AeroRust:main Oct 16, 2022
@elpiel
Copy link
Member

elpiel commented Oct 16, 2022

Thank you 🥳

@elpiel elpiel added Hacktoberfest Issues suitable for Hacktoberfest hacktoberfest-accpeted Hacktoberfest accepted PR and removed Hacktoberfest Issues suitable for Hacktoberfest labels Oct 16, 2022
@elpiel elpiel linked an issue Oct 20, 2022 that may be closed by this pull request
@Turbo87 Turbo87 deleted the pgrmz branch January 24, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accpeted Hacktoberfest accepted PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting additional sentences
2 participants