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

fix: HDG deviation handling #237

Merged
merged 1 commit into from
Dec 26, 2022
Merged

fix: HDG deviation handling #237

merged 1 commit into from
Dec 26, 2022

Conversation

tkurki
Copy link
Member

@tkurki tkurki commented Dec 17, 2022

Fixes #235.

Previously deviation was not handled at all. Now if there is deviation it is applied to create nav.headingMagnetic and in addition nav.headingCompass is output. This PR also adds deviation to the output.

If there is no deviation value the output is as previously: the sensor value in the first field is output as is as nav.headingMagnetic and no nav.headingCompass. This is a bit backwards, as what is being output is nav.headingCompass, but I think changing that would warrant a major version change for being so incompatible with the current behavior.

If the sentence includes variation also nav.headingTrue is output.

@tvr256
Copy link

tvr256 commented Dec 17, 2022

I almost agree with the proposed logic, except for when there is no deviation value. In that case would it make sense to set both headingCompass and headingMagnetic to the same value?

@tvr256
Copy link

tvr256 commented Dec 17, 2022

Also, we should be able to populate nav.headingTrue by adding Variation to headingMagnetic

Previously deviation was not handled at all. Now if there is
deviation it is applied to create nav.headingMagnetic and in
addition nav.headingCompass is output. This PR also adds
deviation to the output.

If there is no deviation value the output is as previously:
the sensor value in the first field is output as is as
nav.headingMagnetic and no nav.headingCompass. This is
a bit backwards, as what is being output is nav.headingCompass,
but I think changing that would warrant a major version
change for being so incompatible with the current behavior.
@tkurki
Copy link
Member Author

tkurki commented Dec 17, 2022

Per my slack comment: as I understand it nowadays very few devices output deviation and variation data in HDG sentence and the data is already corrected per the sensors internal calibration. Adding the exact same value as a duplicate with another path for this, the most common case, would provide very little value, only extra data traffic.

I was going to say that derived data plugin provides the magneticTrue calculation and people can do that, but in the case where variation is in the HDG data it makes perfect sense to do it here (added). Again I expect that in the real world this rarely happens, but mostly variation comes from a GPS RMC sentence and derived data plugin is the solution that will work there.

@tkurki tkurki added the fix label Dec 17, 2022
@tvr256
Copy link

tvr256 commented Dec 18, 2022

Understood, and your approach sounds like a sensible compromise.

If a compass doesn't output deviation it will either leave the deviation field empty or set it to 0. Should we treat 0 deviation the same as empty deviation, and only populate nav.headingCompass if it's different to nav.headingMagnetic?

@tkurki
Copy link
Member Author

tkurki commented Dec 26, 2022

In theory a compass could be outputting deviation 0 for some headings. If deviation is there we should probably use it at face value and not try to be too smart about it.

@tkurki tkurki merged commit b927af8 into master Dec 26, 2022
@tkurki tkurki deleted the fix-hdg branch December 26, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HDG sentence incorrectly parsed
2 participants