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

Initialize APA #119

Closed
wants to merge 31 commits into from
Closed

Initialize APA #119

wants to merge 31 commits into from

Conversation

ANG13T
Copy link
Contributor

@ANG13T ANG13T commented Feb 9, 2024

No description provided.

Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

In general, pretty good PR! Thank you for your contribution.
There are a few minor things I found and the use of enums over bool/char would be ideal.

We should make a good examples with one of the other sentences to follow when developing a new one. Not all impls are in sync let alone the documentation so we should have a better guidance and an example to follow

src/sentences/apa.rs Outdated Show resolved Hide resolved
Comment on lines 26 to 39
// Field Number:

// 1. Status, BOOLEAN, V = Loran-C Blink or SNR warning A = general warning flag or other navigation systems when a reliable fix is not available
// 2. Status, BOOLEAN, V = Loran-C Cycle Lock warning flag A = OK or not used
// 3. Cross Track Error Magnitude
// 4. Status, BOOLEAN, L = Left or R = Right
// 5. Cross Track Units, N = Nautical miles or K = Kilometers
// 6. Status, BOOLEAN, A = Arrival Circle Entered, V = Not Entered
// 7. Status, BOOLEAN, A = Perpendicular passed at waypoint, V = Not Passed
// 8. Bearing origin to destination
// 9. M = Magnetic, T = True
// 10. Destination Waypoint ID
// 11. Checksum
//
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Field Number:
// 1. Status, BOOLEAN, V = Loran-C Blink or SNR warning A = general warning flag or other navigation systems when a reliable fix is not available
// 2. Status, BOOLEAN, V = Loran-C Cycle Lock warning flag A = OK or not used
// 3. Cross Track Error Magnitude
// 4. Status, BOOLEAN, L = Left or R = Right
// 5. Cross Track Units, N = Nautical miles or K = Kilometers
// 6. Status, BOOLEAN, A = Arrival Circle Entered, V = Not Entered
// 7. Status, BOOLEAN, A = Perpendicular passed at waypoint, V = Not Passed
// 8. Bearing origin to destination
// 9. M = Magnetic, T = True
// 10. Destination Waypoint ID
// 11. Checksum
//
/// Field Number:
///
/// 1. Status, BOOLEAN, V = Loran-C Blink or SNR warning A = general warning flag or other navigation systems when a reliable fix is not available
/// 2. Status, BOOLEAN, V = Loran-C Cycle Lock warning flag A = OK or not used
/// 3. Cross Track Error Magnitude
/// 4. Status, BOOLEAN, L = Left or R = Right
/// 5. Cross Track Units, N = Nautical miles or K = Kilometers
/// 6. Status, BOOLEAN, A = Arrival Circle Entered, V = Not Entered
/// 7. Status, BOOLEAN, A = Perpendicular passed at waypoint, V = Not Passed
/// 8. Bearing origin to destination
/// 9. M = Magnetic, T = True
/// 10. Destination Waypoint ID
/// 11. Checksum
///

src/sentences/apa.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@@ -26,7 +26,7 @@ use crate::{parse::NmeaSentence, sentences::utils::array_string, Error, Sentence
/// 1. Status, BOOLEAN, A = Arrival circle entered, V = not passed
/// 2. Status, BOOLEAN, A = perpendicular passed at waypoint, V = not passed
/// 3. Arrival circle radius
/// 4. Units of radiuos, nautical miles
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

src/sentences/apa.rs Outdated Show resolved Hide resolved
src/sentences/apa.rs Outdated Show resolved Hide resolved
// 6. Status, BOOLEAN, A = Arrival Circle Entered, V = Not Entered
// 7. Status, BOOLEAN, A = Perpendicular passed at waypoint, V = Not Passed
// 8. Bearing origin to destination
// 9. M = Magnetic, T = True
Copy link
Member

Choose a reason for hiding this comment

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

Same goes for Magnetic/True. Seems a better fit for an enum instead.

@elpiel
Copy link
Member

elpiel commented Feb 13, 2024

  • Now that Support defmt #92 is merged, could you please add the cfg_attr for defmt-03 on the APA struct? Thank you in advance!

ANG13T and others added 3 commits April 1, 2024 21:21
@ANG13T
Copy link
Contributor Author

ANG13T commented Apr 2, 2024

@elpiel Updated this PR with the fixes and suggestions! Let me know what you think. Thanks so much!

This was referenced May 5, 2024
@elpiel elpiel closed this May 5, 2024
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.

2 participants