f4exb / sdrangel

SDR Rx/Tx software for Airspy, Airspy HF+, BladeRF, HackRF, LimeSDR, PlutoSDR, RTL-SDR, SDRplay and FunCube
GNU General Public License v3.0
2.85k stars 432 forks source link

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

Closed traegerdev closed 2 years ago

traegerdev commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 - c6c0d2dc5e87b97c348337e0edd22e251b3befae.

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 commented 2 years ago

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 commented 2 years ago

Should be fixed in 7.5.1.