[Solved] Simple XOR'ing goes wrong
-
I have an issue about a simple XOR operation. I have the following code (which is simplified to norrow down the issue).
@#include <QCoreApplication>
#include <QByteArray>unsigned short CalculateCRC_ISO13239(QByteArray* data)
{
unsigned short crc_val = 0xFFFF;
int i,j;for (i = 0; i < data->length(); i++) { crc_val = (unsigned short)(crc_val ^ (unsigned short)(*data)[i]); for (j = 0; j < 8; j++) { if ((crc_val & 0x0001) != 0) { crc_val = (unsigned short)((crc_val >> 1) ^ 0x8408); } else { crc_val = (unsigned short)(crc_val >> 1); } } } crc_val = (unsigned short)(~crc_val); return crc_val;
}
int main(int argc, char *argv[])
{
QCoreApplication a(argc, argv);unsigned char ref[] = { 0, 1, 12, 5, 182 }; QByteArray* data = new QByteArray((char*)ref, 5); unsigned char crc = CalculateCRC_ISO13239(data); return a.exec();
}@
This calculation function does not work as expected. Followed the process step by step and seen that the XOR operation does not have the right result at step i=4. (I have a breakpoint at the "crc_val = (unsigned short)(crc_val ^ (unsigned short)(*data)[i]);" line. I'm pressing F5 4 times and then press F10 to see the line's step.
The expected calculation is 0xa7f0 ^ 0xb6 = 0xa746. But somehow it gives the 0x5846 result. If you look bitwise, you'll see that the LSB is correct but the MSB is the opposite what it should be.
I must be blind for not being able to see what's wrong, but here I am. Tried both MinGW and MSVC. The result is the same.
What's wrong with this code (or with me :) ?
P.S: A very similar code is running fine on C#.
Tool: Qt 5.3.1
IDE: QtCreator
Compiler: MSVC 2012 & MinGW 4.8.2 -
Hi,
Some notes:
ISO 13239 supports both CRC-16 and CRC-32. Your function name could be more specific.
char is 8 bits but short is 16 bits.
When you XOR, both values must have the same number of bits.
In your main() function, you converted your CRC result into an 8-bit value. Are you sure this is what you want?
Why do you have so many casts? Just use QVector<unsigned short> instead of QByteArray.
Anyway, if you really must use casts, don't use a C-style cast. These are unsafe because you might get a reinterpret_cast without realizing (this means your data gets corrupted).
@
// Don't use a C-style cast...
unsigned short value2 = (unsigned short)(value1);// ... use a static_cast instead
unsigned short value2 = static_cast<unsigned short>(value1);
@ -
Hi,
Thanks for the suggestions. Good to know that ISO13239 have two CRC lengths. Since I've only used 16 bits I didn't know there were a 32 bits option.
If I want to XOR same length bits I have to use QVector like you said and cast from byte array to ushort with a padding byte. But as far as I know this validation does not need word aligning, i.e. 16 bits. And everything comes down to a simple XOR operation. The first four operation is correct but the fifth is not. Tried the static casting, the result is the same.
"unsigned char crc" is just a quick typo while trying to create a simple project for the issue.
And the heavy type casting is coming from my embedded development background. It gave me lots of headaches in the past and now I'm maybe using just too many. :)
Long story short, problem is still there and I have no idea what's going wrong.
-
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?
-
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;
}@
-
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?
-
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.
-
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.
-
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);
@
-
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.
-
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)
-
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 :)
-
You're most welcome :) All the best with your project!