2
\$\begingroup\$

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;
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ For us to understand the code we need at the very least to know the types of the variables. Can you edit to give the whole method? And if dataStream isn't a System.IO.Stream we'll need the implementation of dataStream.Read too: without more context it's not clear whether or not the way it's being called is introducing a lot of bugs. \$\endgroup\$ – Peter Taylor Sep 12 '18 at 8:31
  • \$\begingroup\$ @PeterTaylor thanks for your feedback. I've updated the question to indicate the data type for dataStream. All other variables have implicit data types based on the assigned type. \$\endgroup\$ – Thabiso Mofokeng Sep 12 '18 at 9:01
3
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ I did remove the first while loop and changed memoryStream.Capacity to memoryStream.length and it is working properly. \$\endgroup\$ – Thabiso Mofokeng Sep 21 '18 at 9:19
2
\$\begingroup\$
        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?

\$\endgroup\$
  • \$\begingroup\$ Should the loop guard be read < memoryStream.Length? Nope, I just need to read the first 4 bytes and exit the loop. \$\endgroup\$ – Thabiso Mofokeng Sep 21 '18 at 9:24
  • \$\begingroup\$ I have moved the expected framing byte arrays from the method and made them read only as per your advise. I used MemoryStream because eventually I'll need to switch NetworkStream once I can TCP connect to the remote sensor but I do think you suggest to read into one byte array and then select chunks would be great and make the code more readable. Thanks \$\endgroup\$ – Thabiso Mofokeng Sep 21 '18 at 9:26
  • \$\begingroup\$ Regarding the checksum, the manufacturer says exclude start and ending bytes but does not mention including checksum value, however excluding it all my checks fail. Am I wrong? \$\endgroup\$ – Thabiso Mofokeng Sep 21 '18 at 9:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.