wollewald / MPU9250_WE

An Arduino library for the 9-axis accelerometer, gyroscope and magnetometer MPU9250 and MPU6500. It contains many example sketches make it easy to use.
https://wolles-elektronikkiste.de/en/mpu9250-9-axis-sensor-module-part-1
MIT License
56 stars 26 forks source link

Conversion error in FIFO readout #2

Closed savejeff closed 3 years ago

savejeff commented 3 years ago

There is a bug in the readMPU9250xyzValFromFifo function. a the conversion from uin16_t to float does not work with negative values. this code is working:

Vec3f MPU9250_WE::readMPU9250xyzValFromFifo(){
    uint8_t MSByte = 0, LSByte = 0;
    xyzFloat xyzResult = {0.0, 0.0, 0.0};
    MSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    LSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    xyzResult.x = ((int16_t) ((MSByte<<8) + LSByte) * 1.0);
    MSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    LSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    xyzResult.y = ((int16_t) ((MSByte<<8) + LSByte) * 1.0);
    MSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    LSByte = readMPU9250Register8(MPU9250_FIFO_R_W);
    xyzResult.z = ((int16_t) ((MSByte<<8) + LSByte) * 1.0);
    return xyzResult; 
}

i also recommend reading 6 bytes in one go and then convert. this will be much faster

roughly like this:

uint8_t data[12];
read_bytes(MPU9250_FIFO_R_W, data, 12);
uint8_t index = 0;
a_sen_raw.x = ((int16_t) (data[index+0] << 8) | data[index+1]) * 1.0;
a_sen_raw.y = ((int16_t) (data[index+2] << 8) | data[index+3]) * 1.0;
a_sen_raw.z = ((int16_t) (data[index+4] << 8) | data[index+5]) * 1.0;
index += 6;
g_sen_raw.x = ((int16_t) (data[index+0] << 8) | data[index+1]) * 1.0;
g_sen_raw.y = ((int16_t) (data[index+2] << 8) | data[index+3]) * 1.0;
g_sen_raw.z = ((int16_t) (data[index+4] << 8) | data[index+5]) * 1.0;
wollewald commented 3 years ago

@savejeff , I have just tried it and I obtain also negative values. So, somehow it works as is. And the compiler does not complain. MSByte and LSByte are read as uint8_t, that's right. But xyzResult.x = (MSByte<<8) + LSByte; in conjunction with xyzFloat xyzResult = {0.0, 0.0, 0.0}; works. The implicite change of varriable type seems to work.

At least it works with my compiler and I have to admit it's not best coding. Have you tried it and you don't get negative values or is it just an observation you made when you read the code? Just my interest.

I will do the quick fix now, but I think the brackets should be different: You proposed: xyzResult.x = ((int16_t) ((MSByte<<8) + LSByte) * 1.0);

But I think cleaner is: xyzResult.x = ((int16_t) ((MSByte<<8) + LSByte)) * 1.0;

The recommendation to improve the speed by reading all bytes in one go is also good, but I will do this at a later stage.

savejeff commented 3 years ago

Moin Moin,

i run the code on the new Raspberry Pi Pico. i think this is caused by different compilers. If you don't specify the explicit conversion from unit to int the behaviour is undefined. some compiler thus just convert unsigned in the corresponding float value.

i only get negative numbers with my proposed code. without I got high postitiv numbers.

i also found the same "bug" in the ICM library.

otherwise very nicely designed code. alternative library from sparkfun have c code and do not compile correctly and are very confusing to modify

wollewald commented 3 years ago

Nice to hear a "moin moin" since I grew up in northern Germany.

I have changed it now and do the same in the ICM20948 lib.

savejeff commented 3 years ago

I'm from Germany (new Frankfurt) and I saw the link to the Germany website ;]