tockn / MPU6050_tockn

Arduino library for easy communication with MPU6050
218 stars 84 forks source link

Unreliable endianness of retrieved data #42

Open edgar-bonet opened 4 years ago

edgar-bonet commented 4 years ago

The method void MPU6050::calcGyroOffsets() contains the following line:

rx = wire->read() << 8 | wire->read();

and similar lines for ry and rz. The same idiom is used in MPU6050::update().

This kind of expression should be avoided, as it does not guarantee the endianness of the retrieved data. This is because, in C++, the order of evaluation of the arguments of the | operator is unspecified.

Presumably, the current version of gcc, when invoked with the options passed by the current version of the Arduino IDE, generates assembly that orders the bytes in the expected way. But this cannot be relied upon, and can very well depend on the compiler, compiler version, compiler options, or even surrounding code. This is the kind of software error that cannot be unveiled by testing, as the code can “just work” even if it is incorrect.

The simplest solution is to issue the calls to wire->read() within separate statements. Unlike the arguments of the | operator, separate statements have a well define order of evaluation:

rx = wire->read() << 8;
rx |= wire->read();

See also this post on the Arduino forum.

rfetick commented 4 years ago

Thanks for your comment @edgar-bonet , however it seems that @tockn didn't update his library for several months :( I forked the Tockn's library and took your comment into account in my own repository. Your comments are welcome.