Default value for empty string

[munnik]

When a value is missing in a nmea string the float or int value in the parsed sentence is set to 0. This way it is hard to determine if the value is really 0 or the data was missing. E.g. $GPVTG,0.3,T,,M,,N,12.6,K*78 would result in GroundSpeedKnots: 0 and GroundSpeedKPH: 12.6. Now it is easy to guess that that GroundSpeedKnots is invalid, but for TrueTrack: 0.3 and MagneticTrack: 0 it is not possible.

Is there a specific reason why this is done in this way?

I would suggest to add getters to the sentences for all the value like:

func (s VTG) GetTrueTrack() (float64, error) {
    if s.hasValidTrueTrack {
        return s.TrueTrack, nil
    }
    return 0, fmt.Errorf("TrueTrack is missing in the nmea string")
}

These getters can check an internal state for each value and return an error if the value was not available in the original nmea string. This will also not break existing code because it is still possible to use TrueTrack, MagneticTrack etc directly.

[icholy]

We could use a pattern similar to the database/sql#NullInt64.

type Float64 struct {
    Float64 float64
    Valid   bool  // Valid is true if Float64 is not NULL
}

// VTG represents track & speed data.
// http://aprs.gids.nl/nmea/#vtg
type VTG struct {
    BaseSentence
    TrueTrack        Float64
    MagneticTrack    Float64
    GroundSpeedKnots Float64
    GroundSpeedKPH   Float64
}

This is a breaking change and would require a new major version. How often does this come up?

[munnik]

I like that idea even better, but I can imagine that a breaking change is too much for now

Some actual data from my vessel (nc 192.168.1.151 10110 | grep ,,):

$WIXDR,C,,C,WCHR,C,,C,WCHT,A,-0.3,D,PTCH,A,0.2,D,ROLL*68
$WIMDA,30.1088,I,1.0196,B,42.7,C,,,,,,,353.3,T,353.0,M,3.7,N,1.9,M*29
$GPGGA,150822.60,5142.01302,N,00452.01096,E,1,09,0.7,0.0,M,0.0,M,,*57
$GPGNS,150822.60,5142.01302,N,00452.01096,E,A,16,0.7,0.0,0.0,,*32
$YXRSA,-1.1,A,,V*55
$AGHTD,A,0.9,L,,,,,,,,,,,,,,*58
$YXXDR,C,,C,WCHR,C,,C,WCHT,C,,C,HINX,P,1.0188,B,STNP*4B
$GPGGA,150823.60,5142.01301,N,00452.01095,E,1,09,0.7,0.0,M,0.0,M,,*56
$WIXDR,C,,C,WCHR,C,,C,WCHT,A,-0.4,D,PTCH,A,0.1,D,ROLL*6C
$GPGNS,150823.60,5142.01301,N,00452.01095,E,A,16,0.7,0.0,0.0,,*33
$YXRSA,-1.1,A,,V*55
$WIMDA,30.1088,I,1.0196,B,42.7,C,,,,,,,353.1,T,352.8,M,3.7,N,1.9,M*22
$GPGRS,150823.60,1,33.1,-1.4,-68.7,74.8,0.5,11.1,0.1,-29.5,-25.2,-2.2,8.4,,*6E
$GPDTM,W84,,0.0000,N,0.0000,E,0.0,W84*6F
$GPGST,150823.60,,0.734,0.547,53.3,0.589,0.438,0.100*4B
$AGHTD,A,0.9,L,,,,,,,,,,,,,,*58
$YXXDR,C,,C,WCHR,C,,C,WCHT,C,,C,HINX,P,1.0188,B,STNP*4B
$WIXDR,C,,C,WCHR,C,,C,WCHT,A,-0.3,D,PTCH,A,0.2,D,ROLL*68
$GPGGA,150824.65,5142.01301,N,00452.01095,E,1,09,0.7,0.0,M,0.0,M,,*54
$YXRSA,0.9,A,,V*71
$GPGNS,150824.65,5142.01301,N,00452.01095,E,A,16,0.7,0.0,0.0,,*31
$WIMDA,30.1088,I,1.0196,B,42.7,C,,,,,,,354.2,T,353.9,M,3.6,N,1.9,M*27

[icholy]

One option is to introduce the Null types along with methods on the nmea.Parser type. You could then override the default message types. Here's a modified version of the example from the README.

package main

import "github.com/adrianmo/go-nmea"

// A type to hold the parsed record
type XYZType struct {
    nmea.BaseSentence
    Time    nmea.NullTime
    Counter nmea.NullInt64
    Label   nmea.NullString
    Value   nmea.NullFloat64
}

func main() {
    nmea.MustRegisterParser("XYZ", func(s nmea.BaseSentence) (nmea.Sentence, error) {
        p := nmea.NewParser(s)
        return XYZType{
            BaseSentence: s,
            Time:         p.NullTime(0, "time"),
            Label:        p.NullString(1, "label"),
            Counter:      p.NullInt64(2, "counter"),
            Value:        p.NullFloat64(3, "value"),
        }, p.Err()
    })
}

[icholy]

I went though the existing types and it seems like we'd only need Float64 and Int64. Time and Date are already nullable (they have a Valid field). string doesn't need a wrapper type because you can just check if it's an empty string. The change is actually pretty small: https://github.com/icholy/go-nmea/commit/e8f0b8b97ccc4ce84220bc7bf4d444a289f4542d

You could then override the VGK parsing like this:

package main

import "github.com/adrianmo/go-nmea"

// VTG represents track & speed data.
// http://aprs.gids.nl/nmea/#vtg
type VTG struct {
    BaseSentence
    TrueTrack        nmea.Float64
    MagneticTrack    nmea.Float64
    GroundSpeedKnots nmea.Float64
    GroundSpeedKPH   nmea.Float64
}

func main() {
    nmea.MustRegisterParser("VTG", func(s nmea.BaseSentence) (nmea.Sentence, error) {
        p := nmea.NewParser(s)
        return VTG{
            BaseSentence:     s,
            TrueTrack:        p.NullFloat64(0, "true track"),
            MagneticTrack:    p.NullFloat64(2, "magnetic track"),
            GroundSpeedKnots: p.NullFloat64(4, "ground speed (knots)"),
            GroundSpeedKPH:   p.NullFloat64(6, "ground speed (km/h)"),
        }, p.Err()
    })
}

[munnik]

Thanks! I will use this idea in my code as I was already aliasing the VTG (and other sentences) because I needed them to implement another interface. If you want to have the changes in here I'm happy to do them and create a PR for it.

[icholy]

@adrianmo take a look when you have a moment.

[adrianmo]

Sorry for taking this long in commenting on this!

@icholy - love your proposal. That'll offer an easy workaround to the issue whilst not making any breaking change.

@munnik or @icholy since you are the masterminds behind this, would you mind submitting a PR with the changes?

Thank you!

[Maescool]

What i noticed with the mda, mwd and mwv Type's is they have a value after each data, if that is either or not set, data before is valid or not. So I would base on that.

[apexskier]

Another option, that's a bit more idiomatic, would be to make the values pointer types and use nil as the empty value.

However, the nullable float and int types already suggested also seem like they'd work well.

(just my two cents, I haven't hit this personally)

[bezineb5]

Any decision on this? I feel that @icholy has a nice, non-breaking solution, no? My use-case: the GPS device only sends course over ground field if the speed is high enough to compute something meaningful. Thanks!

[icholy]

@bezineb5 @munnik I don't have the bandwidth to open a PR right now. But I'll review one if either of you open it.

[bezineb5]

Thanks @icholy - I made a PR: https://github.com/adrianmo/go-nmea/pull/99 I simply cherry-picked your commit and added test and documentation. I don't know if it would make sense to migrate all fields to those in a future major release?

[icholy]

@bezineb5 thanks!

[icholy]

@adrianmo can you create a release?