PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.32k stars 13.43k forks source link

HMC5883 report x, y doesn't match direction of x_raw, y_raw nor x, y direction of chip #7929

Closed ksschwabe closed 7 years ago

ksschwabe commented 7 years ago

@LorenzMeier, @tridge, @julianoes I was having a problem understanding the data coming from my magnetometer on my GPS. It wasn't making sense to me, so I had to dig into the driver and found some nastiness.

The x, and y values for the x, y, output are swapped and "y" is negated to rotate the frame to the GPS module frame.

      /* the standard external mag by 3DR has x pointing to the
     * right, y pointing backwards, and z down, therefore switch x
     * and y and invert y */
    xraw_f = -report.y;
    yraw_f = report.x;
    zraw_f = report.z;
...
        new_report.x = ((xraw_f * _range_scale) - _scale.x_offset) * _scale.x_scale;
    /* flip axes and negate value for y */
    new_report.y = ((yraw_f * _range_scale) - _scale.y_offset) * _scale.y_scale;
    /* z remains z */
    new_report.z = ((zraw_f * _range_scale) - _scale.z_offset) * _scale.z_scale;

But when you set the "raw" uint16_t values from the chip, you rotate the frame in the opposite 90 degree direction (x and y swapped but this time x is negated).

        /*
     * RAW outputs
     *
     * to align the sensor axes with the board, x and y need to be flipped
     * and y needs to be negated
     */
    new_report.x_raw = report.y;
    new_report.y_raw = -report.x;

If you have no rotation (_rotation - ROTATION_NONE) and your scale offsets were 0 and your calibration scales were 1, then your raw values are off by 180 degrees from your final new_report.x, y,z values.

Here is the full section of the code from the collect() function in the hmc5883 driver:

       /*
     * RAW outputs
     *
     * to align the sensor axes with the board, x and y need to be flipped
     * and y needs to be negated
     */
    new_report.x_raw = report.y;
    new_report.y_raw = -report.x;
    /* z remains z */
    new_report.z_raw = report.z;

    /* scale values for output */

    // XXX revisit for SPI part, might require a bus type IOCTL
    unsigned dummy;
    sensor_is_onboard = !_interface->ioctl(MAGIOCGEXTERNAL, dummy);
    new_report.is_external = !sensor_is_onboard;

    if (sensor_is_onboard) {
        // convert onboard so it matches offboard for the
        // scaling below
        report.y = -report.y;
        report.x = -report.x;
    }

    /* the standard external mag by 3DR has x pointing to the
     * right, y pointing backwards, and z down, therefore switch x
     * and y and invert y */
    xraw_f = -report.y;
    yraw_f = report.x;
    zraw_f = report.z;

    // apply user specified rotation
    rotate_3f(_rotation, xraw_f, yraw_f, zraw_f);

    new_report.x = ((xraw_f * _range_scale) - _scale.x_offset) * _scale.x_scale;
    /* flip axes and negate value for y */
    new_report.y = ((yraw_f * _range_scale) - _scale.y_offset) * _scale.y_scale;
    /* z remains z */
new_report.z = ((zraw_f * _range_scale) - _scale.z_offset) * _scale.z_scale;

Also, the negating of the axes for when the sensor is an onboard sensor is rather horrible, because this is just correcting the rotation that you have performed to cope with the 90 degree rotation of the magnetometer on the 3DR GPS modules. How do you cope with GPS modules that have magnetometers at different rotations than the 90 degrees on the 3DR GPS modules? I have noticed that the Drotek GPS modules have different rotations.

        if (sensor_is_onboard) {
        // convert onboard so it matches offboard for the
        // scaling below
        report.y = -report.y;
        report.x = -report.x;
    }

Is there any reason why we should keep this hard-coded rotation in the driver?

In any event, the negation of the x_raw and y_raw values must definitely be corrected so that they are rotated in the same direction as the "final reported" x, y values.

dagar commented 7 years ago

I agree, it's unnecessarily confusing. I was going clean this up a few months ago, but decided to leave it alone as these modules are EOL. If you'd like to improve it I'll happily review.

I don't know how to cope with the "3DR rotation convention". The user has no way to see the real chip orientation, and we have nothing to distinguish a 3DR module from a Drotek module.

julianoes commented 7 years ago

I don't know how to cope with the "3DR rotation convention". The user has no way to see the real chip orientation, and we have nothing to distinguish a 3DR module from a Drotek module.

One thing I've seen is that most modules out there are designed like the 3DR module, so for most cases this was correct.

ksschwabe commented 7 years ago

@dagar: Why is this module heading to EOL? What is going to be replacing it?

I'll issue a PR so that at least the x_raw, y_raw are in the same direction as x and y if the rotation is set to ROTATION_NONE. (I'll hopefully do this by tomorrow).

@julianoes : From personal experience (I have the two GPS modules in front of me), the Ublox NEO-M8T GPS + HMC5983 compass (XL) has the magnetometer rotated at 90 degrees to their Ublox NEO-M8N GPS + HMC5983 compass (XXL) (which is the same as the 3DR rotation).