Correct handling of navigational status in RMC and GNS #108
Conversation
There was a misunderstanding on what the "navigation status" field should include. The enum values were for the "positioning system mode indicator" which is the preceding field. This changes the enum values to correspond to actual ones. I realize that in principle this is a breaking change, however there's no such thing (as far as I can tell) as the previous navigational status values (as those are in fact mode values, which we don't keep an enum for), so I doubt anyone should depend on them... If they do, that will be a bug by itself and breaking it to highlight the error may be desirable. Fixes adrianmo#107
|
I think this change is OK (being author for this problem). In some sense current API is/was (completely) broken - rendering these sentences useless in some situations. @icholy what you think, you have stronger opinions. |
|
@aldas if you're ok with it then I'm ok with it. |
|
maybe we could add some ability to switch |
There was a problem hiding this comment.
so far this seemed to work because "S" (old NavStatusSimulated and new NavStatusSafe) and "V" (old NavStatusDataValid and new NavStatusNotValid) from old and new enum collide and these are most used values. Probably this was the reason this was not found before. RMC is very common sentence.
If anyone depend on these enums and this change breaks their build - this is probably better than silently fail in production parsing uncommon values.
|
Thanks, I added back the relevant constants with some commentary. |
|
This was not released for some reason? v1.8.0 is latest with Feb 4, 2023 ? |
There was a misunderstanding on what the "navigation status" field should include. The enum values were for the "positioning system mode indicator" (variously "FAA mode" / "FFA mode" in this code) which is the preceding field. This changes the enum values to correspond to actual ones.
I realize that in principle this is a breaking change, however there's no such thing (as far as I can tell) as the previous navigational status values (as those are in fact mode values, which we don't keep an enum for), so I doubt anyone should depend on them... If they do, that will be a bug by itself and breaking it to highlight the error may be desirable.
Fixes #107