search

adrianmo / go-nmea

A NMEA parser library in pure Go
MIT License
226 stars 77 forks source link

MTK implementation is currently meant/suitable only for command PMTK001 but there are other messages/commands in MTK #90

Closed aldas closed 1 year ago

aldas commented 2 years ago

I think MTK implementation is not correct. MTK is command protocol embedded into NMEA sentence. Fields that current MTK struct has are meant for MTK packet "PMTK001" but actually MTK packet can have variable amound of data fields.

See:

icholy commented 2 years ago

Not sure how to fix this without a breaking change.

aldas commented 2 years ago

Unless new struct and some ugly if are not added to separate PMTK001 from other MTK command - breaking change is way to go.

In current form I think MTK is quite broken implementation. If breaking change is done this switch https://github.com/adrianmo/go-nmea/blob/e6fa0a78bdaa21f6e039a25b0603527ee6a5b2c1/sentence.go#L184-L187 and this if https://github.com/adrianmo/go-nmea/blob/e6fa0a78bdaa21f6e039a25b0603527ee6a5b2c1/sentence.go#L127-L129

should be also revisited. At the moment 001 is set to mtk.BaseSentence.Type value. I think Type should still be MTK as s.DataType() is used in switch statements (especially in readme.md examples).

Maybe implementation like that

type MTK struct {
    BaseSentence
    Command MTKBase
}

type MTKBase interface {
    Type() string // last 3 digits of sentence prefix ($PMTK001 = 001)
}

type MTK001 struct {
    Cmd  int64
    Flag int64
}

func (m *MTK001) Type() string {
    return "001"
}

// and other command packets from https://www.sparkfun.com/datasheets/GPS/Modules/PMTK_Protocol.pdf or https://www.rhydolabz.com/documents/25/PMTK_A11.pdf

// newMTK constructor
func newMTK(s BaseSentence) (MTK, error) {
    cmdType := s.Type[3:]
    s.Type = "MTK"
    p := NewParser(s)

    mtk := MTK{
        BaseSentence: s,
        Command:      nil,
    }
    switch cmdType {
    case "001":
        mtk.Command = &MTK001{
            Cmd:  p.Int64(0, "command"),
            Flag: p.Int64(1, "flag"),
        }
    default:
        p.err = fmt.Errorf("unknown MTK command type: %v", cmdType)
    }
    return mtk, p.Err()
}

so you could do switch mtk.Type() { ... } in your own code after parsing the sentence and knowning it is of type MTK

aldas commented 2 years ago

@icholy and @adrianmo: I propose breaking change:

basically treat this sentence as any other proprietary sentence and if there is someone that needs other MTK protocol sentences in future, these can be implemented as their own structs.

and notify @krasi-georgiev of this breaking change as he is author and probably sole user of MTK001 sentence

This is where struct and constant is used https://github.com/arribada/LoraTracker/blob/605a0cde5e8cd5cc4f083ef87d779419e2bb8ddd/sender/GPSSender/main.go#L312

from that code is clear that in that case only sentence MTK001 is expected and other MTK sentences are "hand written" in this function https://github.com/arribada/LoraTracker/blob/605a0cde5e8cd5cc4f083ef87d779419e2bb8ddd/sender/GPSSender/main.go#L240

aldas commented 1 year ago

somewhat resolved by #97 MTK is marked deprecated and newer SentenceParser does not add MTK as parser, it has PMTK001 parser. Old global parser still supports MTK