Add GSensord - not sure how to deal with checksums being optional #53

freman · 2019-04-12T00:58:26Z

NB: I don't actually expect you to merge this request, I've broken the handling of missing checksums.

I also think this might be a specific usecase and not a standards type issue.

Basically the Navman MIVUE dashcams add a $GSENSORD sentence that doesn't have checksums.

I did consider refactoring to make types checksum aware so they can complain if they're expecting a checksum or not, but I wasn't sure how you'd like to proceed.

Probably best to just leave it out entirely, or have an unexpected type handler, but then I could just do that with a substring match before parsing.

Sample data:

$GSENSORD,0.060,0.120,-0.180
$GPRMC,073815.000,A,2703.7507,S,15257.3940,E,30.35,36.48,060419,,,A*74
$GPGGA,073815.000,2703.7507,S,15257.3940,E,1,12,0.8,8.4,M,39.6,M,,0000*4B
$GPGSA,A,3,22,31,03,01,193,26,194,18,23,16,14,11,1.6,0.8,1.4*31
$GPGSV,4,1,16,01,44,297,37,03,55,211,28,09,01,263,,11,19,310,22*7C
$GPGSV,4,2,16,14,22,118,27,16,27,032,40,18,33,331,30,22,80,195,37*7D
$GPGSV,4,3,16,23,28,245,32,26,40,065,42,31,42,142,40,32,25,248,*7A
$GPGSV,4,4,16,42,57,343,29,193,78,210,24,194,39,337,36,195,15,346,27*4D
$GSENSORD,-0.120,0.120,-0.250
$GPRMC,073816.000,A,2703.7438,S,15257.3995,E,30.42,35.62,060419,,,A*79
$GPGGA,073816.000,2703.7438,S,15257.3995,E,1,12,0.8,8.4,M,39.6,M,,0000*4D
$GPGSA,A,3,22,31,03,01,193,26,194,18,23,16,14,11,1.6,0.8,1.4*31
$GPGSV,4,1,16,01,44,297,37,03,55,211,27,09,01,263,,11,19,310,21*70
$GPGSV,4,2,16,14,22,118,25,16,27,032,39,18,33,331,31,22,80,195,37*70
$GPGSV,4,3,16,23,28,245,33,26,40,065,43,31,42,142,29,32,25,248,*75
$GPGSV,4,4,16,42,57,343,30,193,78,210,24,194,39,337,36,195,15,346,29*4B

coveralls · 2019-04-12T01:03:59Z

Coverage remained the same at 100.0% when pulling 0aaf168 on freman:master into 7572fbe on adrianmo:master.

icholy · 2019-04-12T01:57:40Z

It's really annoying that this vendor didn't follow the convention of prefixing proprietary message types with "P" talker id.

freman · 2019-04-12T02:00:03Z

Yeh that'd be nice.

I thought about adding an api for adding other messages so it didn't require modifying the library for every vendor. But that still runs into the same question of handling the checksum.

sentence.go

sentence_test.go

icholy · 2019-04-12T02:41:56Z

sentence.go

-			"nmea: sentence checksum mismatch [%s != %s]", checksum, checksumRaw)
+
+	var checksumRaw string
+	splice := strings.Split(raw[startIndex+1:], ChecksumSep)


Use strings.SplitN and limit to 2 parts.

icholy · 2019-04-12T02:44:31Z

@adrianmo I'm ok with this, what do you think?

adrianmo

We are not checking that sentences contain the checksum anymore. E.g. the following sentence was invalid before, but it's accepted now.

$GPGSV,4,2,15,15,41,038,,18,45,328,19,20,60,095,34,21,54,261,14

I'm OK with adding sentences that do not contain a checksum, if their spec states so, but we should keep enforcing the checksum for those sentences that require it.

Maybe there should be a boolean field in the sentence struct to indicate whether or not the sentence type requires a checksum. What do you think @icholy?

adrianmo · 2019-04-15T13:52:51Z

gsensord.go

+)
+
+// GSensord represents measured g-loadings in the x, y and z axis.
+// http://aprs.gids.nl/nmea/#gsa


This link is not pointing to the GSensord sentence spec, but to GSA. Can you update it and reference the GSensord spec?

adrianmo · 2019-04-15T14:00:18Z

gsensord_test.go

+	},
+}
+
+func TestGSensord(t *testing.T) {


can you add tests cases for other, non-valid, sentences? E.g. sending non-float values to x,y,z; more arguments than expected, including checksum...

adrianmo · 2019-04-15T14:06:51Z

sentence.go

-			"nmea: sentence checksum mismatch [%s != %s]", checksum, checksumRaw)
+
+	var checksumRaw string
+	splice := strings.Split(raw[startIndex+1:], ChecksumSep)


adrianmo · 2019-04-15T14:12:27Z

sentence.go

-		return BaseSentence{}, fmt.Errorf(
-			"nmea: sentence checksum mismatch [%s != %s]", checksum, checksumRaw)
+
+	var checksumRaw string


Can we maintain and update the variable declaration block we had before? I find it more elegant?

adrianmo · 2019-04-15T14:20:07Z

sentence.go

@@ -87,6 +93,9 @@ func parseSentence(raw string) (BaseSentence, error) {

 // parsePrefix takes the first field and splits it into a talker id and data type.
 func parsePrefix(s string) (string, string) {
+	if s == TypeGSensord {


Since we are adding a new, empty, prefix for Gsensord, we should add a test case for this.

icholy · 2019-04-15T14:52:23Z

@adrianmo it's just really annoying that this message type breaks all the conventions/rules. The more I think about it, the more I want to completely special-case it. I need to think about it more.

edit: while we're at it, we could do the same for all "P" prefix messages and the vdo ones.

freman · 2019-04-15T23:20:37Z

The more I think about it, the more I want to completely special-case it. I need to think about it more.

Yeh I honestly didn't expect this to get merged in any way in it's current format.
I figured a refactor around message types and having messages know they should require checksums was in order.

Edit: and if I'm honest I could have just checked if the message was prefixed with $GSENSORD before calling the library and handled it in code outside of the library.

icholy · 2019-04-16T15:16:42Z

@freman this probably won't land as-is or soon. I'd recommend checking message prefixes in your code for the time being. I'm experimenting with different approaches which give the messages more control over how they are parsed. Once we find a good solution, this will need to be rebased against that. I encourage you to leave this PR open until that happens.

adrianmo · 2020-02-13T10:19:49Z

@icholy @freman any thoughts on this PR? I'm OK with taking this forward as-is if we address the current and future comments. We can deal with message refactoring later on, in a separate PR.

icholy · 2020-02-13T18:20:30Z

@adrianmo that sounds fine to me.

icholy · 2023-02-08T22:39:38Z

With the new SentenceParser being added in #97 , it will be possible to implement a custom parser for GSENSORD. I think that's good enough for now and we don't need to have build-in support for it. Example:

const TypeGSensord = "GSENSORD"

type GSensord struct {
	nmea.BaseSentence
	X float64 // X-axis G value
	Y float64 // Y-axis G value
	Z float64 // Z-axis G valye
}

p := nmea.SentenceParser{
    ParsePrefix: func(prefix string) (string, string, error) {
        if prefix == TypeGSensord {
            return "", prefix, nil
        }
        return nmea.ParsePrefix(prefix)
    },
    CheckCRC: func(s nmea.BaseSentence, fields string) error {
        if s.Type == TypeGSensord {
            return nil
        }
        return nmea.CheckCRC(s, fields)
    },
    CustomParsers: map[string]nmea.ParseFunc{
        TypeGSensord: func(s nmea.BaseSentence) (nmea.Sentence, error) {
            p := nmea.NewParser(s)
            p.AssertType(TypeGSensord)
            m := GSensord{
                BaseSentence: s,
                X:            p.Float64(0, "x-axis g value"),
                Y:            p.Float64(1, "y-axis g value"),
                Z:            p.Float64(2, "z-axis g value"),
            }
            return m, p.Err()
        },
    },
}

m, _ := p.Parse("$GSENSORD,0.060,0.120,-0.180")

Add GSensord - not sure how to deal with checksums being optional

5fba25c

icholy requested changes Apr 12, 2019

View reviewed changes

sentence.go Outdated Show resolved Hide resolved

sentence.go Outdated Show resolved Hide resolved

sentence_test.go Outdated Show resolved Hide resolved

freman added 2 commits April 12, 2019 12:30

Action some feedback

5e58a28

Leave the talker blank

0aaf168

icholy reviewed Apr 12, 2019

View reviewed changes

icholy approved these changes Apr 13, 2019

View reviewed changes

adrianmo self-requested a review April 15, 2019 10:34

adrianmo requested changes Apr 15, 2019

View reviewed changes

icholy mentioned this pull request Feb 7, 2023

Allow prefix with no talker ID #101

Closed

icholy closed this Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GSensord - not sure how to deal with checksums being optional #53

Add GSensord - not sure how to deal with checksums being optional #53

freman commented Apr 12, 2019

coveralls commented Apr 12, 2019 •

edited

Loading

icholy commented Apr 12, 2019 •

edited

Loading

freman commented Apr 12, 2019

icholy Apr 12, 2019

adrianmo Apr 15, 2019

icholy commented Apr 12, 2019

adrianmo left a comment

adrianmo Apr 15, 2019

adrianmo Apr 15, 2019

adrianmo Apr 15, 2019

adrianmo Apr 15, 2019

adrianmo Apr 15, 2019

icholy commented Apr 15, 2019 •

edited

Loading

freman commented Apr 15, 2019 •

edited

Loading

icholy commented Apr 16, 2019

adrianmo commented Feb 13, 2020

icholy commented Feb 13, 2020

icholy commented Feb 8, 2023 •

edited

Loading

Add GSensord - not sure how to deal with checksums being optional #53

Add GSensord - not sure how to deal with checksums being optional #53

Conversation

freman commented Apr 12, 2019

coveralls commented Apr 12, 2019 • edited Loading

icholy commented Apr 12, 2019 • edited Loading

freman commented Apr 12, 2019

icholy Apr 12, 2019

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

icholy commented Apr 12, 2019

adrianmo left a comment

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

adrianmo Apr 15, 2019

Choose a reason for hiding this comment

icholy commented Apr 15, 2019 • edited Loading

freman commented Apr 15, 2019 • edited Loading

icholy commented Apr 16, 2019

adrianmo commented Feb 13, 2020

icholy commented Feb 13, 2020

icholy commented Feb 8, 2023 • edited Loading

coveralls commented Apr 12, 2019 •

edited

Loading

icholy commented Apr 12, 2019 •

edited

Loading

icholy commented Apr 15, 2019 •

edited

Loading

freman commented Apr 15, 2019 •

edited

Loading

icholy commented Feb 8, 2023 •

edited

Loading