M17 packet mode needs an update

[sp5wwp]

We have recently updated the M17 protocol's specification document. SDRangel has issues with decoding packets. Could you apply a few minor adjustments to the code to accommodate for that?

The SMS should have the following structure (packet data contents):
0x05 (text message designator), text message (UTF-8 encoded), 0x00 (null-termination), 2-byte CRC (same as for the rest of the protocol).

The message is segmented into 25-byte chunks. If there's not enough data left, the rest is zero-padded. We kept the bit order for the Packet Metadata Field you used before - EOT bit is 1<<7 and the data byte counter goes to <<2. I believe the CRC has wrong endianness though. Most significant byte should be at position [0] of the byte array.

[sp5wwp]

It is possible that only the CRC endianness bug is preventing the messages to be displayed.
Just tested - nope.

[sp5wwp]

Since SDRangel is the most convenient way to send M17 packets, could we have this update?

[srcejon]

SDRangel m17 modem is based on this https://github.com/mobilinkd/m17-cxx-demod/commits/master/include/m17cxx - which doesn't seem to have had any changes since it was forked.

In M17Modulator.h make_lsf, there is:

    std::array<uint8_t, 2> checksum = crc.get_bytes();
    lsf[28] = checksum[0];
    lsf[29] = checksum[1];

And make_packet_frame:

        packet_assembly[packet_size]   = crc_.get_bytes()[1];
        packet_assembly[packet_size+1] = crc_.get_bytes()[0];

Which looks the opposite way around. If we reverse it, in m17demodprocessor.cpp, we'd then also need to reverse:

   uint16_t calcChecksum = crc16.get_bytes()[0] + (crc16.get_bytes()[1]<<8);

[sp5wwp]

m17-cxx-demod software is not maintained anymore, we have a fork named m17-tools.

Packet mode is present only in the reference implementation and the specification, of course :) Can you take a look?

[srcejon]

I don't have a radio that works with m17-tools in order to test - but the above patch should reverse the CRC byte ordering in packet mode, so hopefully you can test it? I just made sure it still worked SDRangel to SDRangel.

[srcejon]

Oh, just re-read your second comment, and it seems there may be another change needed. Can you clarify what you think is wrong? (I don't actually use it...)

[sp5wwp]

You can TX M17 using rpitx if you have a Raspberry Pi around. We need to compare what SDRangel expects in a packet (what is extracted from it per frame) and what the reference code actually puts there.

[srcejon]

Added a hex dump to m17-packet-encode, gives the following:

./m17-packet-encode -S M7RCE1 -D M7RCE2 -n 6 -C 0 -o out.raw < packet.bin

packet data:05 68 65 6c 6c 6f e7 14
DST: M7RCE2     0000B1C733DD
SRC: M7RCE1     0000ABACB3DD
Data CRC:       E714
LSF CRC:        018D
LSF: 00 00 b1 c7 33 dd 00 00 ab ac b3 dd 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 8d
pkt_chunk: 05 68 65 6c 6c 6f e7 14 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a0
FN:-- (ending frame)

SDRangel modulator (with flipped CRC gives)

LSF: 00 00 b1 c7 33 dd 00 00 ab ac b3 dd 00 64 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c3 d1
pkt_chunk: 05 68 65 6c 6c 6f 14 e7 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a0

So it doesn't look like the CRC needed to be flipped!

I think you'll need to give an example of what is wrong or not working (an IQ file, for example)

[srcejon]

The SMS should have the following structure (packet data contents): 0x05 (text message designator), text message (UTF-8 encoded), 0x00 (null-termination), 2-byte CRC (same as for the rest of the protocol).

I do see above that there isn't a null terminator between the text and CRC. But I don't see that in the M17 spec either. Where does it come from?

[sp5wwp]

Ok, I'm going to take this step by step. LSF TYPE of 00 02 indicates packet mode with data payload. SDRangel sets this to 00 64 which doesn't fit.
Every string should be null-terminated, I can add that to the spec later. I will also take a look at the implementation and the missing 0x00.

[sp5wwp]

I use this script to generate baseband. It properly adds \0 at the end of the string. If you want to use ... < file.bin, you need to provide the ending 0x00 and add it to the total byte count. The contents of my test.bit are therefore 05 68 65 6C 6C 6F 00 (7 bytes, not 6). I think the null terminator causes problems - SDRangel takes wrong bytes for CRC calculation and that causes a mismatch.

[sp5wwp]

Spec fixed. It now defines SMS content as null terminated, UTF-8 encoded string.

[srcejon]

Ok, I'm going to take this step by step. LSF TYPE of 00 02 indicates packet mode with data payload. SDRangel sets this to 00 64 which doesn't fit.

Code for that is:

   lsf[13] = (streamElsePacket ? 5 : 4) | ((can_ & 1) << 7);

   if (gnss_on_)
   {
       lsf[13] |= (1<<5);
       std::copy(gnss_.begin(), gnss_.end(), &lsf[14]);
   }
   else
   {
       lsf[13] |= (3<<5);
   }

I guess the first change is:

   lsf[13] = (streamElsePacket ? 5 : 2) | ((can_ & 1) << 7);

to set Data type to data rather than voice.

This:

       lsf[13] |= (3<<5);

appears to set the encryption subtype to reserved - Perhaps this was set to reserved to indicate that the META field doesn't contain text or GNSS position?

[sp5wwp]

Perhaps this was set to reserved to indicate that the META field doesn't contain text or GNSS position?

Does any part of the spec suggest that? If subtype=reserved then META field carries no GNSS data.

[srcejon]

Perhaps this was set to reserved to indicate that the META field doesn't contain text or GNSS position?

Does any part of the spec suggest that? If subtype=reserved then META field carries no GNSS data.

I'm just guessing what the author has tried to do. In the spec you have:

m17-packet-encode appears to set subtype to Text, with META data being a null string.
This code appears to set subtype to reserved, to perhaps indicate that META doesn't contain anything valid (i.e. unused rather than reserved).

I suspect that isn't a good idea, in case the value of 11 is actually used for something else later on. Can't think what else it might be, unless it was defined differently in an earlier spec - but as far as I can see, it wasn't.

[sp5wwp]

Ah, yes, I see now. I found it at the same time :) I'd leave subtype at 00 and fill META with zeroes (I do that now).
BTW streamElsePacket ? 5 : 4? Why not streamElsePacket ? 5 : 2?

[srcejon]

SDRangel takes wrong bytes for CRC calculation and that causes a mismatch

Yeah - CRC calculation itself was fine - it's just that it doesn't null terminate the string. If we add the null, and fix LSF as above, it matches:

LSF: 00 00 b1 c7 33 dd 00 00 ab ac b3 dd 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 8d
pkt_chunk: 05 68 65 6c 6c 6f 00 2b 52 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a4

[srcejon]

BTW streamElsePacket ? 5 : 4? Why not streamElsePacket ? 1 : 0?

I presume that is setting datatype to voice, when streaming.

[sp5wwp]

I edited that line.

[srcejon]

Yep, I think it should be 5 : 2.

[sp5wwp]

Don't forget to remove

else
 {
     lsf[13] |= (3<<5);
 }

[srcejon]

Yep - have removed that.

In the demod, we have:

m_typeInfo = "PKT:"; // Packet mode
switch ((type & 6) >> 1) // bits 1..2
{
case 0:
m_typeInfo += "UNK";
break;
case 1:
m_typeInfo += "RAW";
break;
case 2:
m_typeInfo += "ENC";
break;
case 3:
m_typeInfo += "UNK";
break;
}

For packet, should data type only ever be 1? Any idea what the difference between RAW and ENC is meant to be?

[sp5wwp]

No idea what the difference is. I would set these to UNK/DTA/UNK/UNK and call it a day for now.

[sp5wwp]

The true content is encoded in the first bytes of the payload, anyway.

[srcejon]

No idea what the difference is. I would set these to UNK/DTA/UNK/UNK and call it a day for now.

It seems in M17DemodProcessor::decode_lsf:

 switch (packet_type)
 {
 case 1: // RAW -- ignore LSF.
      break;
 case 2: // ENCAPSULATED
     append_packet(m_currentPacket, lsf);
     break;

The LSF is included in what is passed up the stack.