Skip to content

AP_AHRS: use direct active_backend access in place of state structure entries - #33608

Closed
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr/ahrs-remove-state-indirection-for-active-estimates
Closed

peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr/ahrs-remove-state-indirection-for-active-estimates

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

Stops copying the data simply available from the backend pointer into state, just take it straight from the pointer

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Description

We don't need this indirection - remove it.

The matrix identity wasn't surviving long anyway, so remove it.

@peterbarker

Copy link
Copy Markdown
Contributor Author
Board                    AP_Periph  antennatracker  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *                                 *                                                   
Durandal                            -240            -200   *           -552    -544              -200   -104   -280
Hitec-Airspeed           *                                 *                                                   
KakuteH7-bdshot                     -256            -224   *           8       -8                424    64     -72
MatekF405                           -360            -248   *           -224    -224              520    -200   -288
Pixhawk1-1M-bdshot                  -272            -360               -64     -72               32     -96    -200
SITL_x86_64_linux_gnu               -144            -144               16      16                16     16     16
YJUAV_A6SE                          -248            -224   *           16      24                56     -112   -216
f103-QiotekPeriph        *                                 *                                                   
f303-MatekGPS            *                                 *                                                   
f303-Universal           *                                 *                                                   
iomcu                                                                                *                         
revo-mini                           -288            -256   *           -160    -176              -104   -264   -312
skyviper-v2450                                                         -80                                     
speedybeef4                         -328            -256   *           -208    -208              960    -240   -296

@peterbarker
peterbarker force-pushed the pr/ahrs-remove-state-indirection-for-active-estimates branch 2 times, most recently from c476053 to c30ce1e Compare June 30, 2026 10:53
@peterbarker
peterbarker force-pushed the pr/ahrs-remove-state-indirection-for-active-estimates branch from c30ce1e to e6f8326 Compare October 18, 2026 01:18
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Global Size Summary (Total Flash delta in bytes)

Built from a3b3a84 rebased onto afeb9c4

Board Periph tracker blimp bootloader copter heli iofw plane rover sub
DrotekP3Pro _ _ _ * _ _ _ _ _ _
Durandal _ -160 -124 _ -4 +8 _ +48 -52 -272
Hitec-Airspeed * _ _ _ _ _ _ _ _ _
KakuteF7 _ -252 -176 _ -32 +260 _ -196 -200 -260
MambaH743v4 _ -272 -288 _ -64 -32 _ +8 -140 -332
MatekF405-CAN _ _ _ * _ _ _ _ _ _
MatekF405 _ -252 -176 _ -32 +260 _ -196 -200 -260
MatekH743-bdshot _ -160 -124 _ +60 +32 _ +84 -80 -292
Pixhawk1-1M _ -224 -188 _ -20 -52 _ -40 -140 -292
disco _ -284 -272 _ -180 -136 _ -952 -348 -388
f103-GPS * _ _ _ _ _ _ _ _ _

@AP-Review

AP-Review commented Oct 18, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-18)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: REQUEST CHANGES

Reviewed at head e6f83268c4.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33608/1.html#pr33608

Thanks. Replacing the state copies with direct active_estimates reads looks right for the main-thread and semaphore-holding callers. One problem needs fixing first, though.

get_gyro_drift() and get_gyro() (AP_AHRS.h:95, AP_AHRS.h:92) now alias the backend Estimates. AP_AHRS::update() zeroes those (libraries/AP_AHRS/AP_AHRS.cpp:572/583) before get_results() refills them. Copter's fast-rate thread reads ahrs.get_gyro_drift() without the AHRS semaphore (ArduCopter/rate_thread.cpp:265), and so does the Pixart flow callback for get_gyro() (libraries/AP_OpticalFlow/AP_OpticalFlow_Pixart.cpp:291). Either can pick up zeros in that window, which the old state copy prevented. Could you keep a published copy of those two values, or stage the results so these readers never see the cleared struct?

Minor, no change needed: get_velocity_NED() now leaves the caller's vector untouched when velocity is invalid, where it used to copy the last valid value. About ten callers ignore the return value, for example ArduPlane/quadplane.cpp:3682. It may be worth a line in the commit message, since 'Non-functional change' is unticked.

I also checked the identity removal. Only the AHRS_View constructor sees the zero matrix before the first update, which agrees with your note.

@peterbarker
peterbarker force-pushed the pr/ahrs-remove-state-indirection-for-active-estimates branch from e6f8326 to a3b3a84 Compare October 18, 2026 07:04
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head a3b3a8495b.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/33608/2.html#pr33608

Thanks for restoring the gyro copies. get_gyro() and get_gyro_drift() now return state again (AP_AHRS.h:92, AP_AHRS.h:95), so the fast-rate thread and Pixart reads are fine.

Two smaller cases of the same reset window remain. Neither is blocking.

  • iBus telemetry runs as a timer process, above main priority on ChibiOS. It reads get_accel()/get_accel_bias() without the AHRS semaphore (libraries/AP_IBus_Telem/AP_IBus_Telem.cpp:599-607). If it pre-empts update() between *configured_estimates = {} and get_results() (libraries/AP_AHRS/AP_AHRS.cpp:577-578), it can send IMU 0's accel with zero bias (AP_AHRS.h:465, AP_AHRS.h:712). This is cosmetic, and only reachable when ACC_X/Y/Z fall within iBus IDs 1-15.
  • get_rotation_body_to_ned() (AP_AHRS.h:669) is read unlocked by DDS body_to_earth() (libraries/AP_DDS/AP_DDS_ExternalControl.cpp:72) and by the DroneCAN moving-baseline gate (libraries/AP_GPS/GPS_Backend.cpp:406). Those threads sit below main on ChibiOS, so it is only a problem on multi-core Linux and SITL. There, a body-frame DDS command could be turned into zero velocity.

Filling a temporary in update() and then assigning it to the stored Estimates would avoid the zero window for all of these without bringing the state copies back.

The get_velocity_NED() note from last time still stands: it no longer writes the output when the estimate is invalid. It may be worth a line in the commit message.

@peterbarker
peterbarker deleted the pr/ahrs-remove-state-indirection-for-active-estimates branch October 19, 2026 16:57
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.

3 participants