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

accumulated calibration fixes and improvements (mostly magnetometer) #15235

Merged
merged 23 commits into from
Aug 21, 2020

Conversation

dagar
Copy link
Member

@dagar dagar commented Jun 29, 2020

This pull request combines a few mag sensor pipeline and calibration related improvements and fixes that started to overlap.

New functionality

  1. sensor mag drop all remaining IOCTLS, move calibration downstream to sensors module entirely with parameters, and perform aggregation in standalone WorkItem rather than voted_sensors_update
    • mag calibration is never cleared, only updated after a new successful calibration
    • this largely mirrors the changes down for accel & gyro recently.
  2. magnetometer calibration automatically set the rotation of external magnetometers relative to the first internal (see [WIP] commander: auto set external mag rotation during calibration if possible #15120 for more detail).
  3. per sensor instance configurable priority (CAL_ACCx_PRIO/CAL_GYROx_PRIO/CAL_MAGx_PRIO)
    • this replaces the existing enable/disable parameters (CAL_ACCx_EN/CAL_GYROx_EN/CAL_MAGx_EN) and the uORB priority concept
  4. new helper systemcmds/mag_test that simply runs and prints the heading of each sensor_mag instance until you quit I'll bring this in later.
  5. remove conservative subscription interval during data acquisition
    • this allows you to go through the full mag cal quite a bit faster (I've compared data before and after on a couple setups)
  6. store and use full soft iron calibration matrix (new parameters CAL_MAGx_{X, Y, Z}ODIAG)

Along the way I discovered a number of potential bugs and the need to properly carry forward additional data in calibration "slots" that led to the consolidation of these changes. One problem was again the potential inconsistency between sensor instances (uORB, /dev/magX, calibration slots) that can shift if sensor ordering changes (external mags changed or startup script).

EDIT (@MaEtUgR): fixes #14829

@dagar dagar self-assigned this Jun 29, 2020
@dagar dagar changed the title accumulated calibration fixes and improvements accumulated calibration fixes and improvements (mostly magnetometer) Jun 29, 2020
@dagar dagar force-pushed the pr-calibration_improvements branch from 3360b62 to d6e754e Compare June 29, 2020 01:38
@dagar dagar force-pushed the pr-calibration_improvements branch 9 times, most recently from 0eb2166 to ac94518 Compare June 30, 2020 01:47
@dagar dagar marked this pull request as ready for review June 30, 2020 02:12
@dagar dagar requested review from MaEtUgR and bresch June 30, 2020 02:13
@dagar dagar force-pushed the pr-calibration_improvements branch 4 times, most recently from 5e1e347 to 32b245c Compare June 30, 2020 19:52
@dagar dagar force-pushed the pr-calibration_improvements branch 4 times, most recently from 7eff315 to 6d2d07f Compare July 1, 2020 04:21
 - mag calibration acceptance check sphere/ellipsoid fit status
 - print calibration status when finished
 - print each calibration when finished
 - skip rotations that are identical or very close
 - compute mean squared error (MSE) to skip a sqrt
 - ROTATION_PITCH_9_YAW_180 and ROTATION_ROLL_90_PITCH_68_YAW_293 are
kept if manually configured
 - this keeps the string within a single Mavlink STATUSTEXT
 - fixes #14829
 - reduced now that calibration uses uORB::Subscription
 - can likely be reduced further (perhaps < 8) with additional testing
@dagar dagar force-pushed the pr-calibration_improvements branch from cbc224a to 7832870 Compare August 20, 2020 23:16
Copy link
Member

@bresch bresch left a comment

Choose a reason for hiding this comment

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

All good on my side, I tried a few orientations again and the detection works really well.
Maybe only point for later to improve: you currently don't need to perform a full rotation to complete one axis; doing some small left-right rotations is enough to pass to the next orientation, which isn't ideal.

@MaEtUgR
Copy link
Member

MaEtUgR commented Aug 21, 2020

doing some small left-right rotations is enough to pass to the next orientation, which isn't ideal.

Not ideal but not a regression, tricking it was already easy before.

@dagar
Copy link
Member Author

dagar commented Aug 21, 2020

doing some small left-right rotations is enough to pass to the next orientation, which isn't ideal.

Not ideal but not a regression, tricking it was already easy before.

It's a bit more obvious now because you can wiggle in place and force it through faster. It's also slightly harder though as it must accept samples from all mags in the same update.

https://github.com/PX4/Firmware/blob/bae2589feda03bb379e62f425ce2945caec747e8/src/modules/commander/mag_calibration.cpp#L268-L285

I found that as long as it was the full 6 orientation calibration it's actually pretty hard to mess up even if you squander a few sides completely without rotating. We definitely should encourage people to use the full 6 orientation unless it's not possible, and that's one of the reasons I tried to make it much faster.

I also considered running little attitude estimators in place and really requiring proper clean rotations, but that had implications for auto rotation later that I didn't have time to work through.

@dagar dagar merged commit 2c3441a into master Aug 21, 2020
@dagar dagar deleted the pr-calibration_improvements branch August 21, 2020 14:12
@dagar
Copy link
Member Author

dagar commented Aug 21, 2020

Thanks everyone!

@MaEtUgR
Copy link
Member

MaEtUgR commented Aug 21, 2020

Good timing, I just started testing it and I noticed:

  • Nice UX during mag calibration! Calibration seems like it should be 👍
  • After any calibration (gyro, accel, mag) I had a manual control lost just when it finished successful. Not sure if that's the timing on the autopilot or QGC side (I had virtual joysticks)
  • It added "empty" parameters for an ACC2, MAG2 and MAG3 which are not present

@dagar
Copy link
Member Author

dagar commented Aug 21, 2020

After any calibration (gyro, accel, mag) I had a manual control lost just when it finished successful. Not sure if that's the timing on the autopilot or QGC side (I had virtual joysticks)

I believe this is QGC doing a full param sync after calibration. I'm going to start keeping a closer eye on mavlink performance and latency to see if we can do anything about this and other issues. In particular for manual control I don't yet know if it's the QGC stick broadcast getting blocked or the PX4 mavlink module not receiving/processing them properly.

It added "empty" parameters for an ACC2, MAG2 and MAG3 which are not present

This is an intentional reset of those slots. After you reboot they won't be present. I also plan to update the parameter system at some point so that we're not storing a reset parameter at all.

Thanks for testing.

This pull request was closed.
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.

accel calibration garbage mavlink output on completion
6 participants