Allow prefix with no talker ID

[jdgordon]

Hi,

I want to use this NMEA parser for a custom device which doesn't use talker ID's. This change adds a second check to the custom parsers to check for the full talkerID + prefix in the registered parsers instead of just the prefix.

This is the bare minimum to make this work, however happy to implement more edge-case handling if desired. I wasnt sure if this should be the first check on the custom parsers or second. Opted for the second to limit the chance of breaking anything.

[aldas]

Talked ID and Sentence type parsing is done here https://github.com/adrianmo/go-nmea/blob/2740e681bd2912da800676000d9c3a6c0342b07a/sentence.go#L126 I think adding additional check from customParsers is not that eloquent.

I have somewhat similar PR https://github.com/adrianmo/go-nmea/pull/97 that tried to work around CRC check (there are devices that do not send CRC) by introducing way to customize parser. NB: it would help with #98 also as it has callback for dealing with tag blocks and thus allowing buffering tags over multiple sentences.

I could revive this PR by throwing all breaking changes out ( MTK sentence part). And introduce option to customize address field (Talker ID + Sentence type) parsing function.

type SentenceParser struct {
//...
    ParseAddress func(rawAddress) (talkerID string, sentence string, err error)
//...
}

@icholy / @adrianmo what do you think?

[icholy]

I think the implementation is a little hacky, but the behaviour change makes sense to me. I think splitting the prefix into the talker/type in the BaseSentence was a mistake and we should consider undoing that in v2.

@aldas I'm not clear on what your proposed api change would look like.

[aldas]

In relation for this situation: maybe parsePrefix could be changed to deal with addresses with length of 3

https://github.com/adrianmo/go-nmea/blob/2740e681bd2912da800676000d9c3a6c0342b07a/sentence.go#L133-L136

to

    l := len(s)
    if l < 2 { // this will result in error as no sentence has "" (empty) type
        return s, ""
    } else if l == 3 {
        // ugly workaround for sentence which address contains only sentence type
        // these are devices with invalid implementation of NMEA0183 protocol, probably not marine related as this kind
        // of implementation would not pass certification process.
        return "", s
    }
    // NMEA standard says that (non-proprietary) address consist of 5 bytes where first 2 bytes are Talker ID and last 3 sentence type
    return s[:2], s[2:]

This is not particularly better or worse from current logic in my opinion as current implementation assumes that length of 1 or 2 is fine and in that case everything belongs to Talker ID in that case.

---

Separating address (first field after $) into TalkerID and Sentece Type is correct thing. NMEA says that address field is 5 digits/upper case characters where 2 first characters is the talker ID. Talker ID is used to indicate message source purpose (FR is for example Fire detection system, FS fire sprinkler system). By Talker ID you can distinguish what sent the message because you could have multiple devices or same device sends same sentence with different talkerID, in nmea0183 connection.

Most of the time NMEA0183 is used with serial connection (peer to peer) ala over RS232 but for larger installations - like vessels, where there could be multiple devices that require same input (ala multiple devices need same GPS sentences), in that case you would install NMEA0183 multiplexer/buffer that has inputs from multiple RS232 devices and can multiplex them into single or multiple outputs.

TalkerID is little bit naive way to distinguish source of the message and in NMEA2000 protocol this is superseeded by source/destination field as N2K is bus (multiple senders/multiple receivers).

anyway, 99% of cases you only care for last 3 characters in address field - i.e. sentence type (I do not consider proprietary messages here where sentence type could be longer that 3) but there are valid cases when library should expose Talker ID to the user.

---

That PR #97 API would be like that:

    p := nmea.NewSentenceParserWithConfig(nmea.SentenceParserConfig{
        ParseAddress: func(address string) (talkerID string, sentence string, err error) {
            if len(address) < 3 {
                return "", "", errors.New("could not determine talker ID and sentence type from address")
            }
            if len(address) == 3 { // my workaround
                return "", address, nil
            }
            return address[:2], address[2:], nil
        },
        CheckCRC: func(rawCRC string, calculatedCRC string, sentence nmea.BaseSentence) error {
            return nil // do not check CRC at all
        },
    })

    s, err := p.Parse("$HEROT,-11.23,A*FF")

[icholy]

I'd prefer if the SentenceParser was a useful zero value:

p := nmea.SentenceParser{
    ParseAddress: func(address string) (talkerID string, sentence string, err error) {
        if len(address) < 3 {
            return "", "", errors.New("could not determine talker ID and sentence type from address")
        }
        if len(address) == 3 { // my workaround
            return "", address, nil
        }
        return address[:2], address[2:], nil
    },
    CheckCRC: func(rawCRC string, calculatedCRC string, sentence nmea.BaseSentence) error {
        return nil // do not check CRC at all
    },
}

However, for this PR, I think we can get away with just adding

if parser, ok := customParsers[s.Prefix()]; ok {
    return parser(s)
}

Then it will be up to the customParser to fix the Type & Talker for the non-standard message. For example:

nmea.MustRegisterParser("FOOBAR", func(s nmea.BaseSentence) (Sentence, error) {
    s.Type = s.Talker + s.Type
    s.Talker = ""
    return s, nil
})

edit: I also think the prefix customParser lookup should happen before the type lookup. This will let custom parsers handle a specific talker + type before the type parser picks it up.

[aldas]

@icholy, customParsers check is already done before library type switch and by adding else if parser, ok := customParsers[s.Prefix()]; ok { Parse starts to double check that map every time.

also s.Prefix() is not flexible at all. This is because when you now register custom parser you need to provide fixed prefix = talker ID + type and thus making impossible to have application that can handle type with any talker ID.

As I tried to explain - Talker ID depends on type of the hardware. Talker ID is is actually configurable on some devices. NMEA has actually reserved "U[Digit]" ala U1 etc for configurable talker IDs with "instance ID".

For example - lets assume that this library does not have "HBT - heartbeat sentence" and user want to add custom parser for it. They use NMEA multiplexer (let say Shipmodule Miniplex-3 and have connected 3 NMEA0183 capable devices to it as inputs, all these devices emit HBT sentence. But these devices are of different type and therefore their TalkerIDs are different, therefore allowing us to differentiate which device HBT was actually sent.

If we would implement it customParsers[s.Prefix()] - this would mean that we need to actually add 3 variants as parsers to catch all these xxHBT addresses. Moreover - this is really annyoing because if there are changes to the network and Talker ID change - we would need to create new version of that application. It would create maintenance headache when whatever reason you need to for example replace your device to similar (different manufacturer) and now for some reason that device uses different TalkerID - now you need to change your code, recompile and deploy that application. This is really annoying in marine environment. For example - I work for a shipyard in Estonia but we have built vessels for Canada/USA. Now this "mechanical" device replacement becomes to additional "software upgrade"

I would still recommend adding another else if into existing parsePrefix to work with length of 3 case. All NMEA own sentence are 3 character long - so we can make a assumption for case len=3 means that all these characters should belong to TYPE and Talker ID is empty, and therefore have a workaround for device that has incorrectly implemented NMEA0183 spec and does not send TalkerID.

[icholy]

@aldas I may have been unclear, but I think we're agreeing. This is what I was suggesting:

// try to match the whole prefix first
if parser, ok := customParsers[s.Prefix()]; ok {
    return parser(s)
}
// match the type
if parser, ok := customParsers[s.Type]; ok {
    return parser(s)
} 

I would still recommend adding another else if into existing parsePrefix to work with length of 3 case. All NMEA own sentence are 3 character long - so we can make a assumption for case len=3 means that all these characters should belong to TYPE and Talker ID is empty, and therefore have a workaround for device that has incorrectly implemented NMEA0183 spec and does not send TalkerID.

If I'm reading this correctly, this will only help if @jdgordon 's custom device sentences have a prefix length of 3.

[aldas]

aye, you are correct. This would only help with len=3.

[icholy]

The GSENSORD messages are an example of a sentence which would require the full prefix match: https://github.com/adrianmo/go-nmea/pull/53

[aldas]

GSENSORD is especially nasty case. Invalid address and missing CRC.

at some point you maybe cant expect library to have workaround

    line := "$GSENSORD,0.060,0.120,-0.180"
    if strings.HasPrefix(line, "$GSENSORD,") {
        line = "$XXGSENSORD," + line[10:] // fix talkerid+type
        line = line + "*" + nmea.Checksum(line[1:]) // fix CRC
    }
    s, err := nmea.Parse(line)

I am using something similar for my cases with devices missing CRC.

but in this case it is little bit harder - the message contains CRC and that is calculated without talkerID. So maybe doing double check is ok, but it still seems such waste :)

[icholy]

@jdgordon I'm closing this PR in favor of #97 . See my comment at https://github.com/adrianmo/go-nmea/pull/53#issuecomment-1423332197 for more details.

[jdgordon]

@icholy no problem. I'm going to continue using my fork in the mean time. Yeah, our devices dont follow the NMEA spec and it doesnt make sense to assume your lib would support it :) cheers

[icholy]

@jdgordon I think everyone here has had to deal with such devices/sentences at some point. I'm pretty confident that #97 will cover your use-case, but feedback is always appreciated :)