Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mag offset correction from estimated bias #21812

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Conversation

bresch
Copy link
Member

@bresch bresch commented Jul 6, 2023

Solved Problem

I realized that the estimated mag bias was never applied back to the calibration. I found the following bugs:

  1. since last_us is set to 0 every time the bias is not observable, the total time was also reset -> needed 30 consecutive seconds in mag 3D to be declared "stable"
  2. after landing, the mag_aligned_in_flight flag is reset. Using this for bias validity makes it invalid before we have a chance to save it to the calibration.
  3. the change in parameter is (at least in sim) < 0.01gs when the parameter is wrong by 0.1 gs, making it being rejected in set_offset

Solution

Fix 1.; now it's truly a period of accumulated time, no need to be consecutive anymore
Remove check for mag_aligned_in_flight; we anyway don't enter mag 3D before that
Reduce the minimum mag offset param change to 0.001gs

Additionally:

  • Reduce the required stability period to 10s. For the mag, 30s of bias learning is a lot, given that it is only active during turns and that it usually converges in < 5 seconds (and we start counting after convergence)

  • Increase uncertainty of calibration parameters: in air bias estimation is usually really accurate and should be weighted more heavily compared to the calibration parameters that are often more approximate given the worse magnetic environment near the ground.

Changelog Entry

For release notes:

Fix mag offset correction from estimated bias

Alternatives

Test coverage

SITL

@@ -110,7 +110,7 @@ void Magnetometer::SensorCorrectionsUpdate(bool force)

bool Magnetometer::set_offset(const Vector3f &offset)
{
if (Vector3f(_offset - offset).longerThan(0.01f)) {
if (Vector3f(_offset - offset).longerThan(0.001f)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done to minimize writing out the parameters after every flight for negligible changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can revert this if you're fine with changing magb_vref (as modified in this PR). Otherwise the change is so small that even an offset of 0.1gs is never getting corrected (the change in offset is ~0.003gs in sim)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's set it to 0.005 so it gets applied if the change in offset is > 1% of the usual mag field

@@ -329,7 +329,7 @@ void VehicleMagnetometer::UpdateMagCalibration()

if (_calibration[mag_index].set_offset(mag_cal_offset)) {

PX4_INFO("%d (%" PRIu32 ") EST:%d offset: [%.2f, %.2f, %.2f]->[%.2f, %.2f, %.2f] (full [%.3f, %.3f, %.3f])",
PX4_INFO("%d (%" PRIu32 ") EST:%d offset: [%.3f, %.3f, %.3f]->[%.3f, %.3f, %.3f] (full [%.3f, %.3f, %.3f])",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these strings weren't being fully captured, maybe check a quick example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work
mag_offset_commit

@dagar
Copy link
Member

dagar commented Jul 7, 2023

  1. we definitely want this part fixed for v1.14
  2. shouldn't matter, the in flight calibration is held before this happens.


if (cal.total_time_us > 30_s) {
if (cal.total_time_us > 10_s) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this magic number will also apply for accel and gyro.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't think it's a problem for those either.

- since last_us is set to 0 every time the bias is not observable, the
  total time was also reset -> needed 30 consecutive seconds in mag 3D
  to be declared "stable"
- after landing, the mag_aligned_in_flight flag is reset. Using this for
  bias validity makes it invalid before we have a chance to save it to
  the calibration.
For the mag, 30s of bias learning is a lot, given that it is only active
during turns and that it usually converges in < 5 seconds
in air bias estimation is usually really accurate and should be weighted
more heavily compared to the calibration parameters that are often
more approximate given the worse magnetic environment near the ground.
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-july-11-2023/33057/1

@dagar dagar merged commit 9ebfed0 into main Jul 11, 2023
81 of 84 checks passed
@dagar dagar deleted the pr-mag-bias-to-cal-fix branch July 11, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants