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

ekf2: mag control cleanup mag reset #21305

Merged
merged 3 commits into from
Jul 24, 2023
Merged

Conversation

dagar
Copy link
Member

@dagar dagar commented Mar 13, 2023

current/old

  • if yaw estimator available
    • reset heading to yaw estimator
    • use WMM to reset mag_I
    • use WMM (and new yaw estimator heading) to reset mag_B
  • otherwise reset mag_B to zero and mag_I to mag field

PR

  • always use WMM to reset mag_I if available
  • if yaw estimator available use it to reset mag_B (so that existing mag will produce desired heading)
  • perform resetMagHeading() (using mag bias if available)

This was broken out of #21212 (somewhat).

@dagar dagar force-pushed the pr-ekf2_mag_control_mag_reset_cleanup branch 2 times, most recently from 94a11bf to d37a927 Compare March 13, 2023 22:39
@dagar dagar added the EKF2 label Mar 13, 2023
@Sidzeppelin95
Copy link

not sure about the first one will have to look through the log once more but for
./platforms/nuttx/Debug/jlink_gdb_backtrace_simple.sh build/cuav_x7pro_test/cuav_x7pro_test.elf || true
probably the stack has identical or similar objects (could be just names or the object itself). Could also be a deleted object. You can add an exception while tracing this track for similar objects or delete the identical one altogether if it is not related to anything important that's running.

@dagar
Copy link
Member Author

dagar commented Mar 14, 2023

@Sidzeppelin95 I don't think I'm following what you're saying, but can you please open a new dedicated issue and add more detail?

@dagar dagar force-pushed the pr-ekf2_mag_control_mag_reset_cleanup branch from d37a927 to 5276a72 Compare May 30, 2023 18:10
@dagar dagar marked this pull request as ready for review May 30, 2023 18:11
src/modules/ekf2/EKF/mag_control.cpp Outdated Show resolved Hide resolved
src/modules/ekf2/EKF/mag_control.cpp Outdated Show resolved Hide resolved
src/modules/ekf2/EKF/mag_control.cpp Show resolved Hide resolved
@dagar dagar force-pushed the pr-ekf2_mag_control_mag_reset_cleanup branch from 7054ed4 to 7d4defb Compare June 13, 2023 00:41
@dagar dagar requested a review from bresch June 13, 2023 00:42
@bresch bresch force-pushed the pr-ekf2_mag_control_mag_reset_cleanup branch 2 times, most recently from acb6e3d to aefcdf8 Compare July 14, 2023 12:33
@bresch
Copy link
Member

bresch commented Jul 14, 2023

Squashed, rebased and f-puched

@dagar
Copy link
Member Author

dagar commented Jul 18, 2023

@bresch we need to take a closer look (large change indication diff).

@bresch
Copy link
Member

bresch commented Jul 19, 2023

I had a look at the diffs in the change indicator; they are related to the earth field states and with the prints I found what's happening:
With this PR, the mag field states are initialized using the WMM if available (which make sense). However, the GNSS coordinates of the change indicator flight lead to an inconsistent inclination

reset mag heading 0.374 -> 0.376 rad (bias:[0.000, 0.000, 0.000], declination:0.0)
reset to WMM. decl: 3.917351 deg, incl: 0.573786 deg
meas inc = 64.794960 deg
resetting mag I [0.203, 0.002, 0.435] -> [0.367, 0.004, 0.025]

In summary, the earth field state is different in the replay because the measured field doesn't match the WMM (or the GNSS coordinates are incorrect).

bresch
bresch previously approved these changes Jul 19, 2023

resetMagCov();
// mag_I: reset to WMM
_state.mag_I = mag_earth_pred;
Copy link
Member

Choose a reason for hiding this comment

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

If you change this line by

_state.mag_I = _R_to_earth * mag;

the whole change indicator diff disappears. So I'm pretty sure it's all good.

dagar and others added 3 commits July 21, 2023 11:02
There is some actual changes because the earth mag field states are now
reset using the WMM when available. In the replay log, the measured mag
field does not match the WMM and this is why there is a large diff. It
is however more correct now than before.
@bresch bresch force-pushed the pr-ekf2_mag_control_mag_reset_cleanup branch from 8ad9069 to 906fc0b Compare July 21, 2023 09:06
@bresch
Copy link
Member

bresch commented Jul 21, 2023

Added unit test for mag earth field reset to WMM. Will add an additional check on the inclination in a next PR.

@bresch bresch merged commit eb9bcb0 into main Jul 24, 2023
87 checks passed
@bresch bresch deleted the pr-ekf2_mag_control_mag_reset_cleanup branch July 24, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants