Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AISMessage::toNMEA in ais.cpp not terminating sentences per NMEA spec #1333

Closed
traegerdev opened this issue Jul 10, 2022 · 5 comments
Closed

Comments

@traegerdev
Copy link

traegerdev commented Jul 10, 2022

NMEA sentences require a sequence at the end of each sentence. toNMEA is concatenating sentences together and then terminating the bunch with an . This may work for some plotters, but not others like TimeZero (a professional plotter).

This fixed it (ais.cpp 86-91):

        // Construct complete sentence with leading ! and trailing checksum
        // and termination sequence \r\n
        nmeaSentence.append("!");
        nmeaSentence.append(nmeaProtected);
        nmeaSentence.append(QString("*%1").arg(checksum, 2, 16, QChar('0')));
        nmeaSentence.append("\r\n");  

I'm totally a noob to Open Source workflow, otherwise I'd have tried the official way to submit the change.

@srcejon
Copy link
Collaborator

srcejon commented Jul 13, 2022

The sentences are already joined with a newline in the last line of the function:

return nmeaSentences.join("\n");

So I think your patch would give \r\n\n

Is the problem that your s/w is requiring a carriage return as well (Presumably it's something Windows based?) So you would just need:

return nmeaSentences.join("\r\n");

But perhaps this should only be for Windows.

@traegerdev
Copy link
Author

traegerdev commented Jul 16, 2022

That return statement does stick a \n on the end, but that breaks protocol without the \r and also breaks protocol when toNMEA has parsed more than one sentence at a time. It puts nothing at the end of the first sentence, or any except the very last sentence.

There needs to be a "\r\n" at the end of each sentence to follow NMEA protocol specification (over RS485/422); it's not a Windows thing. I'd imagine some parsers are okay with the way it is because they stop pulling in characters at the * and don't start again until it finds a $, but not others. The software I'm using is very commonly used for navigation so it's probably purposefully strict.

At any rate, if you'd like it pushed to a merge request, or however that's done, I'm happy to share the fix. I can also give you some packet dumps to show what I'm talking about with multiple sentences being parsed.

@traegerdev
Copy link
Author

Okay I ran a wireshark. I was not understanding toNMEA. It seems to be only handling one sentence at a time. But oddly there are no trailing \n in the packets (this is from a clean build of master - c6c0d2d.

Here's a wireshark dump of two packets, one from master and one from my patch.

master - c6c0d - clean
0000   48 f1 7f f7 21 91 f8 59 71 fc f6 11 08 00 45 00   H...!..Yq.....E.
0010   00 4a c1 13 40 00 40 11 b6 fb c0 a8 20 6e c0 a8   .J..@.@..... n..
0020   20 d5 c0 ab 0b 83 00 36 f7 7b 21 41 49 56 44 4d    ......6.{!AIVDM
0030   2c 31 2c 31 2c 2c 2c 31 35 4e 66 3d 57 50 50 30   ,1,1,,,15Nf=WPP0
0040   30 6f 3f 3f 51 68 45 66 54 3e 3e 34 3f 76 30 32   0o??QhEfT>>4?v02
0050   35 71 38 2c 30 2a 30 33                           5q8,0*03

patched
0000   48 f1 7f f7 21 91 f8 59 71 fc f6 11 08 00 45 00   H...!..Yq.....E.
0010   00 4c 0a c7 40 00 40 11 6d 46 c0 a8 20 6e c0 a8   .L..@.@.mF.. n..
0020   20 d5 d7 8b 0b 83 00 38 1a 47 21 41 49 56 44 4d    ......8.G!AIVDM
0030   2c 31 2c 31 2c 2c 2c 42 35 32 4c 31 77 40 30 30   ,1,1,,,B52L1w@00
0040   3d 6b 6b 46 66 55 4b 56 42 4e 3d 67 77 6b 51 6a   =kkFfUKVBN=gwkQj
0050   44 60 62 2c 30 2a 30 61 0d 0a                     D`b,0*0a..

@srcejon
Copy link
Collaborator

srcejon commented Jul 17, 2022

join adds \r\n between sentences when there are multiple sentences, but doesn't append it at the end of individual sentences. So I think the fix is:

return nmeaSentences.join("\r\n").append("\r\n");   // NMEA-0183 requires CR and LF

The above patch also fixes reading beyond the end of the array.

@srcejon
Copy link
Collaborator

srcejon commented Jul 19, 2022

Should be fixed in 7.5.1.

@srcejon srcejon closed this as completed Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants