Handling the temperature correction bit properly

[gllmbernard]

Hello,

Thank you for your code, it is very clear.

Here is a small improvement that can be done in function calibrateMPU9250. In this function, there is this comment:

// Apparently this is not working for the acceleration biases in the MPU-9250
// Are we handling the temperature correction bit properly?

You should modify the lines before the comment to use 0xFE instead of 0xFF for the low bytes. By doing so, you do not keep the last bias bit and you preserve the temperature compensation bit.

  data[0] = (accel_bias_reg[0] >> 8) & 0xFF;
  data[1] = (accel_bias_reg[0])      & 0xFE;
  data[1] = data[1] | mask_bit[0]; // preserve temperature compensation bit when writing back to accelerometer bias registers
  data[2] = (accel_bias_reg[1] >> 8) & 0xFF;
  data[3] = (accel_bias_reg[1])      & 0xFE;
  data[3] = data[3] | mask_bit[1]; // preserve temperature compensation bit when writing back to accelerometer bias registers
  data[4] = (accel_bias_reg[2] >> 8) & 0xFF;
  data[5] = (accel_bias_reg[2])      & 0xFE;
  data[5] = data[5] | mask_bit[2]; // preserve temperature compensation bit when writing back to accelerometer bias registers

[kriswiner]

Have you found that this works all of the time?

On Wed, Dec 13, 2017 at 6:06 AM, gllmbernard notifications@github.com wrote:

Hello,

Thank you for your code, it is very clear.

Here is a small improvement that can be done in function calibrateMPU9250. In this function, there is this comment:

// Apparently this is not working for the acceleration biases in the MPU-9250// Are we handling the temperature correction bit properly?

You should modify the lines before the comment to use 0xFE instead of 0xFF for the low bytes. By doing so, you do not keep the last bias bit and you preserve the temperature compensation bit.

data[0] = (accel_bias_reg[0] >> 8) & 0xFF; data[1] = (accel_bias_reg[0]) & 0xFE; data[1] = data[1] | mask_bit[0]; // preserve temperature compensation bit when writing back to accelerometer bias registers data[2] = (accel_bias_reg[1] >> 8) & 0xFF; data[3] = (accel_bias_reg[1]) & 0xFE; data[3] = data[3] | mask_bit[1]; // preserve temperature compensation bit when writing back to accelerometer bias registers data[4] = (accel_bias_reg[2] >> 8) & 0xFF; data[5] = (accel_bias_reg[2]) & 0xFE; data[5] = data[5] | mask_bit[2]; // preserve temperature compensation bit when writing back to accelerometer bias registers

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kriswiner/MPU9250/issues/215, or mute the thread https://github.com/notifications/unsubscribe-auth/AGY1quJnCd7SoCCx_Yuz1Bv4bdjAicvRks5s_9n6gaJpZM4RAmMj .

[gllmbernard]

I was inspired by your code, my case is different as I am loading bias values from an SD card. But as far as I tested it yesterday, it was working without error.

While writing this message, I had a second look at the code and I think it can be simplified by the following:

  // I did not modify the lines to read from the registry
  int32_t accel_bias_reg[3] = {0, 0, 0}; // A place to hold the factory accelerometer trim biases
  readBytes(MPU9250_ADDRESS, XA_OFFSET_H, 2, &data[0]); // Read factory accelerometer trim values
  accel_bias_reg[0] = (int32_t) (((int16_t)data[0] << 8) | data[1]);
  readBytes(MPU9250_ADDRESS, YA_OFFSET_H, 2, &data[0]);
  accel_bias_reg[1] = (int32_t) (((int16_t)data[0] << 8) | data[1]);
  readBytes(MPU9250_ADDRESS, ZA_OFFSET_H, 2, &data[0]);
  accel_bias_reg[2] = (int32_t) (((int16_t)data[0] << 8) | data[1]);

  // I remove the code that keeps track of the value of the last bit

  // I added & 0xFFFFFFFE to leave the last bit unchanged by the computation
  accel_bias_reg[0] -= ((accel_bias[0]/8) & 0xFFFFFFFE);
  accel_bias_reg[1] -= ((accel_bias[1]/8) & 0xFFFFFFFE); 
  accel_bias_reg[2] -= ((accel_bias[2]/8) & 0xFFFFFFFE);

  // no modification on the next lines
  data[0] = (accel_bias_reg[0] >> 8) & 0xFF;
  data[1] = (accel_bias_reg[0])      & 0xFF;
  data[2] = (accel_bias_reg[1] >> 8) & 0xFF;
  data[3] = (accel_bias_reg[1])      & 0xFF;
  data[4] = (accel_bias_reg[2] >> 8) & 0xFF;
  data[5] = (accel_bias_reg[2])      & 0xFF;

1. Read the data from the IMU registry.
2. Remove the last bit from the computed accel_bias. You can use ^ 0x00000001 instead of & 0xFFFFFFFE.
3. The computation of the updated bias, do not affect the value of the last bit. I think this was the source of the error in the current code.
4. Write the updated value to the IMU registry.

[kriswiner]

The buffers are int16_t whereas 0xFFFFFFFE is a 32-bit integer. Maybe you mean 0xFFFE?

This is such a can of worms (yes, you would think it would be straightforward) that the simplest thing to do is subtract the offsets in the main loop manually, which is what I do with good success.

On Thu, Dec 14, 2017 at 6:38 AM, gllmbernard notifications@github.com wrote:

I was inspired by your code, my case is different as I am loading bias values from an SD card. But as far as I tested it yesterday, it was working without error.

While writing this message, I had a second look at the code and I think it can be simplified by the following:

// I did not modify the lines to read from the registry int32_t accel_bias_reg[3] = {0, 0, 0}; // A place to hold the factory accelerometer trim biases readBytes(MPU9250_ADDRESS, XA_OFFSET_H, 2, &data[0]); // Read factory accelerometer trim values accel_bias_reg[0] = (int32_t) (((int16_t)data[0] << 8) | data[1]); readBytes(MPU9250_ADDRESS, YA_OFFSET_H, 2, &data[0]); accel_bias_reg[1] = (int32_t) (((int16_t)data[0] << 8) | data[1]); readBytes(MPU9250_ADDRESS, ZA_OFFSET_H, 2, &data[0]); accel_bias_reg[2] = (int32_t) (((int16_t)data[0] << 8) | data[1]);

// I remove the code that keeps track of the value of the last bit

// I added & 0xFFFFFFFE to leave the last bit unchanged by the computation accel_bias_reg[0] -= ((accel_bias[0]/8) & 0xFFFFFFFE); accel_bias_reg[1] -= ((accel_bias[1]/8) & 0xFFFFFFFE); accel_bias_reg[2] -= ((accel_bias[2]/8) & 0xFFFFFFFE);

// no modification on the next lines data[0] = (accel_bias_reg[0] >> 8) & 0xFF; data[1] = (accel_bias_reg[0]) & 0xFF; data[2] = (accel_bias_reg[1] >> 8) & 0xFF; data[3] = (accel_bias_reg[1]) & 0xFF; data[4] = (accel_bias_reg[2] >> 8) & 0xFF; data[5] = (accel_bias_reg[2]) & 0xFF;

1. Read the data from the IMU registry.
2. Remove the last bit from the computed accel_bias. You can use ^ 0x00000001 instead of & 0xFFFFFFFE.
3. The computation of the updated bias, do not affect the value of the last bit. I think this was the source of the error in the current code.
4. Write the updated value to the IMU registry.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kriswiner/MPU9250/issues/215#issuecomment-351728573, or mute the thread https://github.com/notifications/unsubscribe-auth/AGY1qhqwJn6ZYGC3U-C4QzatkLgy6h5-ks5tATLcgaJpZM4RAmMj .

[gllmbernard]

You are right. 0xFFFE is better.

In the case where the measurement can overflow the range measured by the accelerometer or gyroscope, it is preferable to set the value in the IMU. But this is an extreme case.

[kriswiner]

When I tried to use the accel offset registers, it would produce reasonable results about half the time. I think this is due to not handling the temperature bit properly. I had no trouble with the gyro offset registers and still use these. Can you confirm that this fix results in accurate accel offset handling 100% of the time?

On Fri, Dec 15, 2017 at 12:23 AM, Guillaume Bernard < notifications@github.com> wrote:

You are right. 0xFFFE is better.

In the case where the measurement can overflow the range measured by the accelerometer or gyroscope, it is preferable to set the value in the IMU. But this is an extreme case.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kriswiner/MPU9250/issues/215#issuecomment-351943206, or mute the thread https://github.com/notifications/unsubscribe-auth/AGY1qgrnU9Yr1IXK6-aD6CNBPH4gHcY4ks5tAiyBgaJpZM4RAmMj .

[gllmbernard]

Yes. I tested it in my implementation and had no error.

A short explanation of the error.

Line 886:

accel_bias_reg[0] -= (accel_bias[0]/8);

This code switches the last bit half of the time because the last bit of accel_bias[0]/8 can be 1.

Lines 890-892:

  data[0] = (accel_bias_reg[0] >> 8) & 0xFF;
  data[1] = (accel_bias_reg[0])      & 0xFF;
  data[1] = data[1] | mask_bit[0]; // preserve temperature compensation bit when writing back to accelerometer bias registers

If the last bit of accel_bias_reg[0] is 1 and mask_bit is 0, you keep the 1 for the last bit data[1]. But you should have kept 0. The piece of code from yesterday removes the last bit from the computation to avoid this error.

[kriswiner]

I'll give it a try

On Fri, Dec 15, 2017 at 9:12 AM, Guillaume Bernard <notifications@github.com

wrote:

Yes. I tested it in my implementation and had no error.

A short explanation of the error.

Line 886 https://github.com/kriswiner/MPU9250/blob/master/MPU9250BasicAHRS.ino#L886 :

accel_bias_reg[0] -= (accel_bias[0]/8);

This code switches the last bit half of the time because the last bit of accel_bias[0]/8 can be 1.

Lines 890-892 https://github.com/kriswiner/MPU9250/blob/master/MPU9250BasicAHRS.ino#L890 :

data[0] = (accel_bias_reg[0] >> 8) & 0xFF; data[1] = (accel_bias_reg[0]) & 0xFF; data[1] = data[1] | mask_bit[0]; // preserve temperature compensation bit when writing back to accelerometer bias registers

If the last bit of accel_bias_reg[0] is 1 and mask_bit is 0, you keep the 1 for the last bit data[1]. But you should have kept 0. The piece of code from yesterday removes the last bit from the computation to avoid this error.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kriswiner/MPU9250/issues/215#issuecomment-352060315, or mute the thread https://github.com/notifications/unsubscribe-auth/AGY1qt5U_efnaS8p2zV-584MH4gLIzpmks5tAqiVgaJpZM4RAmMj .

[jmagan]

Hello,

I found this issue and I think it resolve the accel_bias problem. Based on gllmbernard's comments, the code isn't preserving the LSB bit. I'll try to explain it:

Actual code:

data[1] = (accel_bias_reg[0]) & 0xFF; 11111111 & 11111111 = 11111111 data[1] = data[1] | mask_bit[0]; let's say mask was a 0. So 11111111 | 00000000 = 11111111 Result: 11111111 you should preserve the last bit as 0.

With the gllmbernard's solution you set LSB to 0 and then with the or operator you preserve the LSB. So:

data[1] = (accel_bias_reg[0]) & 0xFE; 11111111 & 11111110 = 11111110 data[1] = data[1] | mask_bit[0]; let's say mask was a 0. So 11111110 | 00000000 = 11111110 Result: 11111110

I hope it helps. I'll try it by myself.

Best regards. Juan