4
\$\begingroup\$

I'm reading serial data in binary format from a stream, and I'm not happy with how I extract the data because it takes so much operations to extract the binary data from a simple line of binary data.

Each line starts with a character indicating the type of data, and afterwards follow a few 16 bit integers (big endian), followed by a checksum character and a newline.

Here's a sample of what line would be after reading:

line = "F{3x 16 bit int big endian}{checksum character}\n"

This is the simplified code in question:

  byte[] b = new byte[2];
  System.Text.ASCIIEncoding enc = new System.Text.ASCIIEncoding();

  // Check that the port is open. If not skip and do nothing
    if (SerialPort.IsOpen)
    {
        while(SerialPort.BytesToRead > 50){

            // read line and turn string in byte array
            string line = SerialPort.ReadLine();
            byte[] encoded = enc.GetBytes(line);

            if (line[0]=='F'){

                // copy 16 bit int into byte-array
                Array.Copy(encoded, 1, b, 0, 2);
                // -> big endian
                Array.Reverse(b);

                // turn into integer
                short value1 = BitConverter.ToInt16(b, 0);

                Array.Copy(encoded, 3, b, 0, 2);
                Array.Reverse(b);
                short value1 = BitConverter.ToInt16(b, 0);


                Array.Copy(encoded, 5, b, 0, 2);
                Array.Reverse(b);
                short value3 = BitConverter.ToInt16(b, 0);
            }

        }
        this.bytesAvailable = SerialPort.BytesToRead;

Is there a smarter way to accomplish the result of my code above?

\$\endgroup\$
14
\$\begingroup\$

Data Acquisition

You asked:

Is there a smarter way to accomplish the result of my code above?

So, I have another suggestion, to work on top of the suggestions by Heslacher:

Eliminate the BitConverter work. This is easily done with boolean operators (most specifically the shift operators).

short value1 = (short)((short)encoded[1] << 8 | encoded[2]);
short value2 = (short)((short)encoded[3] << 8 | encoded[4]);
short value3 = (short)((short)encoded[5] << 8 | encoded[6]);

Why? It's fast as hell. And this tends to make it more clear what position each value represents. There's no need to place them backwards as you have to with the BitConverter. Looking at this code, it's extremely easy to determine that each pair consists of the high-byte, followed by the low-byte. With the BitConverter, the lowest byte must be followed by the subsequently higher bytes.

Can it really be faster, with all those casts, shifting (<<) and or's (|), and the fact that Microsoft themselves wrote the BitConverter?

You're damn right it can. Run the following programme:

byte[] encoded = new byte[7];
encoded[0] = 0xF0;
encoded[1] = 0xF1;
encoded[2] = 0xF2;
encoded[3] = 0xF3;
encoded[4] = 0xF4;
encoded[5] = 0xF5;
encoded[6] = 0xF6;

Stopwatch sw1 = new Stopwatch();

short value1 = 0, value2 = 0, value3 = 0;

sw1.Start();
for (int i = 0; i < 100000000; i++)
{
    value1 = BitConverter.ToInt16(new byte[] { encoded[2], encoded[1] }, 0);
    value2 = BitConverter.ToInt16(new byte[] { encoded[4], encoded[3] }, 0);
    value3 = BitConverter.ToInt16(new byte[] { encoded[6], encoded[5] }, 0);
}
sw1.Stop();
Console.WriteLine(sw1.ElapsedTicks.ToString());
sw1.Reset();

Console.WriteLine(value1.ToString("X"));
Console.WriteLine(value2.ToString("X"));
Console.WriteLine(value3.ToString("X"));

sw1.Start();
for (int i = 0; i < 100000000; i++)
{
    value1 = (short)((short)encoded[1] << 8 | encoded[2]);
    value2 = (short)((short)encoded[3] << 8 | encoded[4]);
    value3 = (short)((short)encoded[5] << 8 | encoded[6]);
}
sw1.Stop();
Console.WriteLine(sw1.ElapsedTicks.ToString());
sw1.Reset();

Console.WriteLine(value1.ToString("X"));
Console.WriteLine(value2.ToString("X"));
Console.WriteLine(value3.ToString("X"));

Results:

12956403
F1F2
F3F4
F5F6
1505318
F1F2
F3F4
F5F6

You'll notice that the bitwise-operator method takes 12% of the time the BitConverter method does. And in this situation it makes a great deal of sense. It's easier to reason how the value1, value2 and value3 variables are being filled with the bitwise-operators. It's not so obvious with the BitConverter.

Yes, this is obviously a worst-case environment, you will likely have other bottlenecks and JITer that will influence it, but this at least goes to show that there are other just as good solutions to the same problem. Personally, I've never touched the BitConverter, I always do my own bitwise-arithmetic. To me, it's easier to read and more obvious. To you, it may not be.

I wouldn't suggest going so far as to make a loop. I did just that, and it's actually slower.

short[] myValues = new short[3];

sw1.Start();
for (int i = 0; i < 100000000; i++)
{
    for (int t = 0; t < myValues.Length; t++)
        myValues[t] = (short)((short)encoded[1 + t * 2] << 8 | encoded[2 + t * 2]);
}
sw1.Stop();
Console.WriteLine(sw1.ElapsedTicks.ToString());
sw1.Reset();

for (int t = 0; t < myValues.Length; t++)
    Console.WriteLine(myValues[t].ToString("X"));

Results:

3759655
F1F2
F3F4
F5F6

Twice the time the non-loop method took. With a dynamic List you can expect that it will be even slower. (Haven't done any experiments with that method, but one can estimate it would take longer.)

Commenting // when is too much and not enough

Another suggestion, based on his answer: put a comment on the while(SerialPort.BytesToRead > 50){ line describing why we only want it if there are more than 50 bytes left to read. From your current code it's impossible to reason why.

Comments are very powerful; and though I am very liberal with my comments (placing one every 2-10 lines), you don't have to be. But they should be often enough that you don't have to examine a 200+ line file to get the relevant bits. Add minor information on where these values are used, what are they for, if you are using indices like this, then markup what each index represents. You don't have to go ape-s*** on them. Just describe what it is you are doing, and why.

\$\endgroup\$
  • 1
    \$\begingroup\$ Great you changed from comment to a very good answer. \$\endgroup\$ – Heslacher Jul 6 '15 at 20:43
  • \$\begingroup\$ @Heslacher My intention was never to undercut your answer - but to provide more information and explanation as to how your answer works. Both answers have valid merit, which I believe is the purpose of Code Review. (To generate multiple opinions.) \$\endgroup\$ – Der Kommissar Jul 6 '15 at 22:04
7
\$\begingroup\$
  • You only need to get the bytes if line[0]=='F' so you should move it inside the if block.

  • If you only need to get these 3 Int16 values, IMHO using Array.Copy, Array.Reverse is a little bit too much.

    while(SerialPort.BytesToRead > 50){
    
        string line = SerialPort.ReadLine();
    
        if (line[0]=='F'){
            byte[] encoded = enc.GetBytes(line);
    
            short value1 = BitConverter.ToInt16(new byte[] {encoded[2] , encoded [1]}, 0);
            short value2 = BitConverter.ToInt16(new byte[] {encoded[4] , encoded [3]}, 0);
            short value3 = BitConverter.ToInt16(new byte[] {encoded[6] , encoded [5]}, 0);
        }
    }
    
  • naming variables value1, value2 and value3 screams for new meaningful names and for a List<T> or any other collection. In this way the shown conversation could be done using a loop.

  • comments like // read line and turn string in byte array does not add any value to the code, because the comment is describing what is done instead of why something is done.

    A very good read about comments: https://codereview.stackexchange.com/a/90113/29371

\$\endgroup\$
  • \$\begingroup\$ @BCdotWeb that had screamed for an edit, hadn't it ? Thanks. \$\endgroup\$ – Heslacher Jul 7 '15 at 8:10

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.