hideakitai / MPU9250

Arduino library for MPU9250 Nine-Axis (Gyro + Accelerometer + Compass) MEMS MotionTracking™ Device
MIT License
277 stars 91 forks source link

Intermittent errors in reading mag data #33

Closed obromios closed 3 years ago

obromios commented 3 years ago

I believe that your current code can have intermittent errors when reading mag data. I set up a program based on your simple example, but printed out the mag readings rather than the angles:

#include "MPU9250.h"

MPU9250 mpu;
// MPU9255 mpu; // You can also use MPU9255

void setup() {
    Serial.begin(115200);
    Wire.begin();
    delay(2000);

    mpu.setup(0x68);  // change to your own address
}

void loop() {
    if (mpu.update()) {
        static uint32_t prev_ms = millis();
        if (millis() > prev_ms + 25) {
          Serial.print(mpu.getMagX());
          Serial.print(", ");
          Serial.print(mpu.getMagY());
          Serial.print(", ");
          Serial.print(mpu.getMagZ());
          Serial.println();
          prev_ms = millis();
        }
    }
}

I saw intermittent errors in mag readings, with all error readings being zero e.g.

-201.20, -357.89, -217.94
-199.42, -348.98, -223.09
-229.69, -361.45, -224.81
0.00, 0.00, 0.00
-215.44, -354.32, -221.38
-195.86, -352.54, -223.09
0.00, 0.00, 0.00
-199.42, -363.23, -219.66
-219.00, -354.32, -235.11
-220.79, -356.11, -223.09
0.00, 0.00, 0.00
-222.57, -368.57, -224.81

Upon further examination, it looked like a race condition, so I set up wait for valid data in the read_mag function of MPU9250.h, so the first part of the modified function looks like

    void read_mag(int16_t* destination) {
        uint8_t raw_data[7];                                                 // x/y/z gyro register data, ST2 register stored here, must read ST2 at end of data acquisition
        uint8_t ST1;
        do {
            ST1 = read_byte(AK8963_ADDRESS, AK8963_ST1);
        } while(!(ST1&0x01));
        if (read_byte(AK8963_ADDRESS, AK8963_ST1) & 0x01) {                  // wait for magnetometer data ready bit to be set
            read_bytes(AK8963_ADDRESS, AK8963_XOUT_L, 7, &raw_data[0]);

This modification eliminates the intermittent errors. It is not ideal, because it will cause the program to hang if no valid is provided. This could be solved by adding a timeout to the while condition.

Is there something wrong with the my code or is there is a need to insert a wait for valid data? I note that this code implements such a wait.

By the way, I was very pleased to find your library, and it simplifies many tasks for me, so thank you for this contribution.

I plan to work on this further, including adding a timeout. Would you like me to do a pull request with the changes?

tree32123 commented 3 years ago

I have the same question with you~ Have you ever try to use Yaw? Is it right?

hideakitai commented 3 years ago

@obromios Sorry for the late reply, of course could you make the pull request for this? I don't have time currently but I will check the issue if you can make the PR. Thank you!

@tree32123 In my environment, yaw is stable and maybe it is because yaw is created by filtering raw data by Madgwick filter. I haven't checked raw accel/gyro data, though.

tree32123 commented 3 years ago

@hideakitai Thanks for your request, I think yaw is the direction of geomagnetic north, but when I put the MPU in the desk and turn it, the yaw is stable all the time. I will get the a,g,m data and then use mpu.update code check the Yaw,Pitch,Roll offline.

obromios commented 3 years ago

@hideakitai, thank you for your reply. Over the next few weeks, I will be working on this and other aspects and will then do a pull request for you to consider. It will have a number of suggestions, from which you can pick and choose from.

hideakitai commented 3 years ago

@obromios @tree32123 This issue will be fixed by https://github.com/hideakitai/MPU9250/tree/bugfix_obromious. Please confirm if it works.

obromios commented 3 years ago

@hideakitai, I have checked and the mag readings are not intermittently in error on this branch. I noticed that you took out the code that times-out the mag readings if there is no response from the magnetometer. I have no objection to this, but would be interested to understand why you omitted the timeout code.

hideakitai commented 3 years ago

@obromios Thank you for reviewing! As I mentioned in this commit, the big problem was mag data has been updated even if the reading is not successful (L.497 is false and mag_count remains zero). So using time-out was not a good solution. I thought a better solution is to update mag data only when reading data is successful. In addition to it, because timeout blocks the main thread operation, we should not use it in the polling tasks.

For these reasons, I've made read_mag() to return bool to show if reading data was successful. And only when the reading data was successful, mag data is updated.

obromios commented 3 years ago

That makes sense.

hideakitai commented 3 years ago

Fixed in v0.4.3