SignalK / nmea0183-signalk

NMEA0183 to Signal K parser
Other
30 stars 48 forks source link

APB and BOD sentences not decoded according to SignalK spec #239

Open tvr256 opened 1 year ago

tvr256 commented 1 year ago

I've noticed a number of areas where the APB and BOD parsers don't follow the SignalK spec:

The APB parser incorrectly stores "bearing to next waypoint" to navigation.courseRhumbline.bearingToDestinationTrue which is not defined in the SignalK spec. Instead, it should be stored to navigation.courseRhumbline.nextPoint.bearingTrue. (Note, the RMB sentence also contains the same value and already stores it to the correct SignalK field).

Both the APB and BOD parsers incorrectly store "bearing from previous waypoint to next waypoint" to navigation.courseRhumbline.bearingOriginToDestinationTrue which is not defined in the SignalK spec. Instead, it should be stored to navigation.courseRhumbline.bearingTrackTrue.

The APB sentence stores the next waypoint ID to navigation.courseRhumbline.nextPoint.ID. This field isn't mentioned in the SignalK spec, should it be added?

The confusion is probably due to the NMEA spec mentioning "origin waypoint" and "destination waypoint". They actually mean previous and next waypoint, but it could easily be misinterpreted as the route starting point and final destination.

Techstyleuk commented 8 months ago

With regard to "navigation.courseRhumbline.nextPoint.ID" what should it be? if we get a consensus, I will fix APB

tvr256 commented 8 months ago

Thanks for fixing! With regards to "navigation.courseRhumbline.nextPoint.ID", I think the code is doing the right thing but it needs to be added to the SignalK courseRhumbline spec

Techstyleuk commented 8 months ago

The Code for APB has both "bearingTrackTrue" and "bearingOriginToDestinationTrue" this could be for legacy reasons, so I am reluctant to remove it unless @tkurki says it is OK to remove? See the Current code below:

` {

        path: `navigation.courseRhumbline.bearingTrack${bearingOriginToDestType}`,

        value: bearingOriginToDest,

      },

      {
        path: `navigation.courseRhumbline.bearingOriginToDestination${bearingOriginToDestType}`,

        value: bearingOriginToDest,

      },`

BOD already has: ` {

        path: 'navigation.courseRhumbline.bearingTrackTrue',

        value: bearingOriginToDestination.True || null,

      },

      {

        path: 'navigation.courseRhumbline.bearingTrackMagnetic',

        value: bearingOriginToDestination.Magnetic || null,

      },`

Is this part fixed already?

tvr256 commented 8 months ago

I've just tested BOD and it looks correct to me (except the waypoint ID isn't in the SignalK spec). So either its been fixed or I raised the bug in error

$INBOD,123.45,T,124.56,M,NEXT_WP_ID,PREVIOUS_WP_ID*44

image

tvr256 commented 8 months ago

I've just retested APB, here's what I've found:

$INAPB,A,A,123.45,R,N,A,A,234.56,T,DEST_WP_ID,345.67,M,346.78,T,A*6E

Incorrectly mapped to SignalK:

Not mapped to SignalK

Correctly mapped to SignalK:

image

Techstyleuk commented 8 months ago

OK, with regard to BOD, I will leave as is.

On APB, I can correct the .nextpoint.bearingTrue/Magnetic entries and the .autopilot.targetHeadingTrue/Magnetic but I will wait until @tkurki comments about removal of .courseRhumbline.bearingOriginToDestinationTrue/Magnetic as this may need to be left for legacy reasons

tvr256 commented 8 months ago

Looks like the legacy field names were directly copied using the APB field name instead of the SignalK field name. So it could be a simple typo.

I agree we need to be careful removing fields in case someone is using the legacy field names. PyPilot springs to mind, I'll alert @seandepagnier

panaaj commented 8 months ago

It would also be prudent to consider the Course API which maintains both the V1 paths and the V2 paths.