Force compasses to use SI units

[staroselskii]

A replacement for #2397. This one needs extensive testing.

The code for HMC was slightly refactored. Nothing fancy, just extracted a couple of methods. If that's not needed, let me know!) . There's a couple of places in the HMC's code that I don't entirely understand. Especially this:

    _mag_x = -rx;
    _mag_y =  ry;
    _mag_z = -rz;

Why do we do such a thing?

And also this one:

if (_product_id == AP_COMPASS_TYPE_HMC5883L) {
        field.rotate(ROTATION_YAW_90);
}

Is it absolutely necessary to rotate on the low level? Cannot it be done on the higher one?

Magnetometer offsets also need to be adjusted (I suggest using something like 50 uT. That's the magnetic field vector around the Earth's surface).
I also need some insight on PX4 compass driver.

Updated (9d9068d is the previous HEAD):

• Improved constness in HMC code

Rebased on master (9391266 is the previous HEAD)

[staroselskii]

@lucasdemarchi

What's our policy on the PR reviewing once again? I can update this commit with a rebase but it'll purge all our comment. (This is where git rebase comes short.)

[lucasdemarchi]

What's our policy on the PR reviewing once again? I can update this commit with a rebase but it'll purge all our comment. (This is where git rebase comes short.)

The problem is with github, not git ;-). The final patch set should be clean with no fixups on top, i.e. a rebase should be done. You can do now opening a new pr and linking to this one, or you can put the fixups on top, wait for others to comment and then do the final rebase.

[lucasdemarchi]

Another option if opening a new pr is too troublesome: do a push -f and update the description of this pr with a link to the last commit of the previous version. Github doesn't actually remove the comments, the problem is that the comments are "attached" to the commits and since their hashes change when you rebase we lose their reference.

Example: for this pr even if you force push I'd still be able to see the comments by going to
https://github.com/emlid/ardupilot/commits/19d9eed1e1a2a355997a1596bf874c149d74d50c

[staroselskii]

@lucasdemarchi

Thanks for the clarification! I've updated the PR accordingly.

[staroselskii]

The Travis PX4 build fail is unrelated.

[staroselskii]

@tridge @rmackay9 @lucasdemarchi

I've updated the PR and would like to get a comment on this. Too many Navio users complain about failing pre-arm check that verifies that offsets are in a range suitable for HMC5883 rather than performing a more generic check.

[lucasdemarchi]

I made some comments on the individual patches I cherry-picked from this pr to mine. I did this in order not to cause headache with conflicts.

For the pr, I think we could do it in 2 steps because it's a long time already and I don't think @rmackay9 wants to push this to 3.3. It would be much easier to accept if it was done like:

1. bug fix;
2. conversion to SI units; not tying one to the other.

So we could convert the values to the same range HMC5883 is using (let's call it hmc-unit). This already fixes the bug. After 3.3 is out we convert all sensors to SI and deal with all the drivers, PX4, changes to params, etc.

@staroselskii @rmackay9 do you agree?

[staroselskii]

@lucasdemarchi

Thanks for the review and bug fixes!

If I understand correctly, we need to introduce a new method for converting compass-specific units to common hmc-units for now, right? I hope your PR will be merged soon so I can keep up and continue the work on this issue.

I'd been hoping to get it done before AC3.3 but I understand that this is a vast change that needs to be tested thoroughly. The major concern for me is getting compass pre-arm checks working for Navio without making code more coupled.

I also encourage to use Tesla rather than Gauss as it is a SI unit.

[lucasdemarchi]

If I understand correctly, we need to introduce a new method for converting compass-specific units to common hmc-units for now, right?

So... after the call yesterday we decided to go a little beyond that. It's not hmc-units, but milligauss. It's still not SI, but better than hmc-units.

So the idea is that there's a public API in the AP_Compass library to get the measurements in milligauss. The pre-arm check uses this API. The saved parameter with offsets however uses the sensor-specific data. This way we don't worry about previously saved values, units, etc.

When we get the compass calibration inside ardupilot (and would be great if someone volunteers for this effort), then we are allowed to change the unit we save and publish (maybe microtesla, dunno).

I hope your PR will be merged soon so I can keep up and continue the work on this issue.

It's a big pullrequest that maybe has to wait a little bit to be merged. Don't wait that pullrequest. I pushed a branch with only the patches that would conflict with yours. You can use that one to work on top: https://github.com/lucasdemarchi/ardupilot/commits/hmc-cleanup

I'd been hoping to get it done before AC3.3 but I understand that this is a vast change that needs to be tested thoroughly. The major concern for me is getting compass pre-arm checks working for Navio without making code more coupled.

yep... if we can make these changes quickly I think it can still go to 3.3... or at least would be easy to cherry-pick for an eventual 3.3.1. That's up to @rmackay9 though.

I also encourage to use Tesla rather than Gauss as it is a SI unit.

we can do that after the compass calibration.... Right now it won't be visible to the user and the saved values will be in sensor-specific units, so it doesn't really matter.

[staroselskii]

@lucasdemarchi

It's a big pullrequest that maybe has to wait a little bit to be merged. Don't wait that pullrequest. I pushed a branch with only the patches that would conflict with yours. You can use that one to work on top:

Thanks, Lucas!

So the idea is that there's a public API in the AP_Compass library to get the measurements in milligauss. The pre-arm check uses this API. The saved parameter with offsets however uses the sensor-specific data. This way we don't worry about previously saved values, units, etc.

I'd like to get everything right before starting to implement it once more.

So, everything is converted to milligauss. get_field() and get_offsets() from now will output milligauss. The only place where old units are used is the set_and_save_offsets() where an additional conversion from milligauss to the compass units takes place. The defines used in motors.cpp like COMPASS_OFFSETS_MAX will also be in milligauss. The users won't be affected by this change at all. If I understand correctly, we don't need a new set of methods like get_offsets_milligaus() and the like.

To sum up, the only thing we need to add to the public compass backend API is the convert_to_old_units method. At first glance it looks like a decision to our problem. Is there a flaw in my logic?

This way the AK8963's problem will be also solved.

[lucasdemarchi]

I think it would be easier to add get_field_milligauss() and get_offsets_milligauss, leaving get_field() and get_offsetsas is. So while the first pair of methods returns the value converted to milligauss the second returns the raw value, in sensor units. Then you update the callers in the places that a) are not saved to storage and b) are not sent via mavlink.

The constants in motors.cpp can be updated, as long as you update compass.get_offsets() to be compas.get_offsets_milligauss().

@tridge @rmackay9. Does it seem reasonable to you?

[staroselskii]

Closed in favor of #2748.