Read framed nested sensor data from memory stream

Question

I'm implementing a TCP client to read data from a sensor. The sensor sends messages to connected clients framed with a start sequence, messages, checksum and end sum bytes. Because the sensor is installed at a remote location, I'm currently reading previously captured network data from a pcap dump into a MemoryStream. The TCP data is simulated from a capture file using bittwist (not real-time client-server communication).

Please advise on how to improve the method below that reads TCP messages including framing bytes, through a MemoryStream since I can't use NetworkStream? Is my checksum calculation correct? The sensor documentation indicates that to calculate it you must include all payload data except starting and ending sequence.

I have two while loops, one to first check start sequence (frame start), once done.

The second loop checks for the checksum value, end sequence (frame end) and if not, treats this as a message payload and reads from the stream until a valid end sequence (frame end) is found. Each (n) payload starts with a 3 bytes header (2 bytes identifier + 1 byte length of the succeeding data) and then the actual data.

private static void Decode(MemoryStream memoryStream)
    {
        var frameStartSequenceBuffer = new byte[] { 0xCA, 0xCB, 0xCC, 0xCD };
        var frameChecksumBuffer = new byte[1];
        var frameEndSequenceBuffer = new byte[] { 0xEA, 0xEB, 0xEC, 0xED };
        var startSequenceBuffer = new byte[4];
        var endSequenceBuffer = new byte[4];
        var messages = new List<byte[]>();
        var read = 0;

        while (read < startSequenceBuffer.Length)
        {
            read += memoryStream.Read(startSequenceBuffer, 0, startSequenceBuffer.Length);

            if (Helpers.ByteArrayCompare(startSequenceBuffer, frameStartSequenceBuffer))
                break;
        }

        while (memoryStream.Capacity > read)
        {
            read += memoryStream.Read(frameChecksumBuffer, 0, frameChecksumBuffer.Length);
            read += memoryStream.Read(endSequenceBuffer, 0, endSequenceBuffer.Length);

            if (Helpers.ByteArrayCompare(endSequenceBuffer, frameEndSequenceBuffer))
                break;

            var readPayload = frameChecksumBuffer.Concat(endSequenceBuffer).ToArray();
            var messageBuffer = new byte[3 + endSequenceBuffer[1]];

            readPayload.CopyTo(messageBuffer, 0);
            read += memoryStream.Read(messageBuffer, readPayload.Length, messageBuffer.Length - readPayload.Length);

            messages.Add(messageBuffer);
        }

        var calculatedChecksum = Helpers.CalculateChecksumBitwise(messages.SelectMany(o => o).Concat(frameChecksumBuffer).ToArray());

        if (calculatedChecksum != frameChecksumBuffer[0])
        {
            Console.WriteLine("Messages: {0} - Corrupt", messages.Count);
            return;
        }

        Console.WriteLine("Messages: {0} - Valid", messages.Count);
    }

Each TCP packet from the sensor encapsulates an array of (n) messages delimited with a 3 bytes headers.

The checksum is calculated from all data without the start sequence and the end sequence. The Checksum is a simple XOR Assignment of all n data bytes. Below is the checksum calculation method that I've implemented.

public static byte CalculateChecksumBitwise(IList<byte> byteToCalculate)
    {
        var checksum = 0;

        for (var i = 0; i < byteToCalculate.Count - 1; i++)
        {
            checksum = checksum ^ byteToCalculate[i];
        }

        return (byte)checksum;
    }

Answer 1

I think the first loop is somewhat redundant, because if read < startSequenceBuffer.Length after the first pass, the input memoryStream is shorter than the startSequenceBuffer and then there are no messages to follow anyway. Instead of the first loop you can do:

       int read = memoryStream.Read(startSequenceBuffer, 0, startSequenceBuffer.Length);

        if (!Helpers.ByteArrayCompare(startSequenceBuffer, frameStartSequenceBuffer))
            throw new InvalidDataException();

If the first loop is meant as a search for the start sequence, you'll have to find another stop condition, because it will stop after the first pass, unless the stream is shorter then startSequenceBuffer.

---

while (memoryStream.Capacity > read) {...}

Be aware that MemoryStream.Capacity >= MemoryStream.Length, so the second loop can unintentionally try to read beyond memoryStream.Length which will cause an infinite loop because trying to read from a position greater than Length will add nothing to read. Use memoryStream.Length instead.

---

The meat of the second loop is a little confusing without any data to test on. At first sight it seems to work though. It seems odd that frameChecksumBuffer is written to more than once, because you only use it in one test at the end of the method, but maybe I just misunderstand the name or the format of the stream?

Answer 2

        var frameStartSequenceBuffer = new byte[] { 0xCA, 0xCB, 0xCC, 0xCD };

        var frameEndSequenceBuffer = new byte[] { 0xEA, 0xEB, 0xEC, 0xED };

I would consider pulling these out of the method as private static readonly IReadOnlyList<byte>. That guarantees that you (or the maintainer after you) can't mix up frameStartSequenceBuffer and startSequenceBuffer and write to the wrong one.

---

        while (read < startSequenceBuffer.Length)
        {
            read += memoryStream.Read(startSequenceBuffer, 0, startSequenceBuffer.Length);

            if (Helpers.ByteArrayCompare(startSequenceBuffer, frameStartSequenceBuffer))
                break;
        }

Should the loop guard be read < memoryStream.Length?

Are the sequences guaranteed to be offset by a multiple of 4 bytes?

What if the stream is only 3 bytes long?

As a side note, MemoryStream guarantees to read as many bytes as requested unless there aren't enough. Other Stream implementations might read less than requested even though they're not exhausted. It's handy to have an extension method which uses a loop to read the number of bytes requested or throw an exception if the end of the stream is reached first.

---

        while (memoryStream.Capacity > read)
        {
            read += memoryStream.Read(frameChecksumBuffer, 0, frameChecksumBuffer.Length);
            read += memoryStream.Read(endSequenceBuffer, 0, endSequenceBuffer.Length);

            if (Helpers.ByteArrayCompare(endSequenceBuffer, frameEndSequenceBuffer))
                break;

The scope of endSequenceBuffer is this loop, so you could declare it inside the loop.

---

            var readPayload = frameChecksumBuffer.Concat(endSequenceBuffer).ToArray();
            var messageBuffer = new byte[3 + endSequenceBuffer[1]];

            readPayload.CopyTo(messageBuffer, 0);

Would it not be simpler to declare messageBuffer and then copy frameChecksumBuffer and endSequenceBuffer into the appropriate offsets rather than using Linq?

---

        var calculatedChecksum = Helpers.CalculateChecksumBitwise(messages.SelectMany(o => o).Concat(frameChecksumBuffer).ToArray());

ToArray() does a lot of copying. Maybe make CalculateChecksumBitwise take IEnumerable<byte> rather than byte[] so that you can pass the lazy enumerable.

Alternatively, maybe instead of using MemoryStream and copying chunks of it into byte[] you could refactor the whole method to take a byte[] and select chunks of it as ArraySegment<byte>. Then CalculateChecksumBitwise could also take ArraySegment<byte> and all the copying can be eliminated.

---

        if (calculatedChecksum != frameChecksumBuffer[0])
        {
            Console.WriteLine("Messages: {0} - Corrupt", messages.Count);
            return;
        }

Hang on. The checksum calculation includes frameChecksumBuffer, so the only way it could pass this check is if the messages themselves sum to 0.

---

Each TCP packet from the sensor encapsulates an array of (n) messages delimited with a 3 bytes headers.

Two things. I assume that's a typo for 4 byte headers. And does the layer above this one handle packet fragmentation?