[Solved] Simple XOR'ing goes wrong

MuratUrsavas

I've seen the issue via using an intermediate variable. The last operand "0xb6" (*data[4]) comes as "0xffb6" from the byte array. This makes the MSB reversed.

How is that even possible?

MuratUrsavas

Well, it is really strange. I've investigated the issue at instruction level and seen that one instruction is acting weird at fifth step, which is "cbtw: convert byte to word".

Code gets the QByteArray member from the array correctly but fills extra "0xff" for the second byte at "cbtw" call. This does not happen for the first four loop runs.

Removing the casting and using directly the byte value solved the issue (since it removed the cbtw instruction) but I'm still suspecting from an errata (even at chip level).

Anyway, here's the correct function.

@unsigned short CalculateCRC_ISO13239_16(QByteArray* data)
{
unsigned short crc_val = 0xFFFF;
int i,j;

for (i = 0; i < data->length(); i++)
{
    crc_val ^= (*data)[i];
    for (j = 0; j < 8; j++)
    {
        if ((crc_val & 0x0001) != 0)
        {
            crc_val = (crc_val >> 1) ^ 0x8408;
        }
        else
        {
            crc_val >>= 1;
        }
    }
}

crc_val = ~crc_val;

return crc_val;

}@

JKSH

I'm glad to hear that you've resolved the issue. Thanks for sharing your detailed investigations and solutions with the community!

[quote]Code gets the QByteArray member from the array correctly but fills extra “0xff” for the second byte at “cbtw” call. This does not happen for the first four loop runs.

Removing the casting and using directly the byte value solved the issue (since it removed the cbtw instruction) but I’m still suspecting from an errata (even at chip level).[/quote]That's an interesting quirk. What compiler/chip did you use?

MuratUrsavas

I've used both MSVC 11.0 and MinGW 4.8.2. The results were the same. That's why I'm suspecting from an errata at chip level.

My processor is an Intel Core i7-2675QM. I couldn't find the errata sheet of Sandy Bridge processors therefore couldn't find a proof related to this issue.

MuratUrsavas

Please note that the function above works for MSVC. MinGW still has the same problem probably because not removing the cbtw instruction.

For a workaround you can change this line:
@crc_val ^= (*data)[i];@

with this:
@crc_val ^= (*data)[i] & 0xFF;@

It will solve the issue with a little performance penalty.

JKSH

What happens if you do either one of the following?

Cast using static_cast<unsigned short>(*data[i]) instead of the C-style cast (or having no cast).
Replace the QByteArray with a QVector<unsigned short>.

Some other general tips:

Qt provides nice typedefs: You can write "quint16" instead of "unsigned short".
You shouldn't create QByteArray, QVector, or other Qt containers using new, as they are "implicitly shared":http://qt-project.org/doc/qt-5/implicit-sharing.html. Create the container on the stack, and pass it into the function by const-reference instead:
@
// Function prototype
quint16 CalculateCRC_ISO13239_16(const QByteArray& data);
@
@
// Function call
QByteArray data = ...;
quint16 crc = CalculateCRC_ISO13239_16(data);
@

MuratUrsavas

As expected, QVector solution works fine, since there is no requirement for converting a byte to word (just ushort to long). The effected byte is the second one which is already covered by the vector class.

The static_cast didn't work. It was the first thing I tried after your first reply.

bq. You shouldn’t create QByteArray, QVector, or other Qt containers using new, as they are implicitly shared [qt-project.org]. Create the container on the stack, and pass it into the function by const-reference instead:

This is a good suggestion for me, as I'm new to Qt and C++ :) Thanks again.

JKSH

Thank you too for trying out my suggestions and reporting back :)

By the way, I've just realized the reason for your error: unsigned char can hold values from 0 to 255, but signed char can only hold values from -128 to 127.

The value 182 doesn't fit in a signed char, but you tried to cast it anyway in line #35 of your original post. 0xB6 is 182 (uchar) or -74 (char). When you cast that to 16 bit, you get the 16-bit representation of -74, which is 0xFFB6

(And this is another example of why we need to be careful with casting)

MuratUrsavas

Yes, also had realized that while investigating the issue and covered that with changing the type char to unsigned char (not just the casting). The result was the same.

Thanks for the support, again :)

JKSH

You're most welcome :) All the best with your project!