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

ICM426XX - Disable AFSR #933

Merged

Conversation

nerdCopter
Copy link
Member

@nerdCopter nerdCopter commented Oct 24, 2023

  • ICM426XX - Disable AFSR
  • Betaflight: Disable ICM426XX AFSR feature to prevent stalls
  • Ardupilot: AP_InertialSensor: fix for ICM42688 stuck gyro issue
    these undocumented bits in register 0x4d control the "adaptive full
    scale range" mode of the ICM42688. The feature is enabled by default
    but has a bug where it gives "stuck" gyro values for short periods
    (between 1ms and 2ms):, leading to a significant gyro bias at longer
    time scales, enough to in some cases cause a vehicle to crash if it is
    unable to switch to an alternative IMU
  • Thanks to all participants here: Artefacts in ICM-42688-P gyro output betaflight/betaflight#12970
  • Thanks to @DutchFPV for quick-turnaround testing
  • Thanks to Betaflight devs communications

Co-authored-by: Andrew Tridgell andrew@tridgell.net
Co-authored-by: Steve Evans Steve@SCEvans.com

Betaflight: Disable ICM426XX AFSR feature to prevent stalls
Ardupilot: AP_InertialSensor: fix for ICM42688 stuck gyro issue
these undocumented bits in register 0x4d control the "adaptive full
scale range" mode of the ICM42688. The feature is enabled by default
but has a bug where it gives "stuck" gyro values for short periods
(between 1ms and 2ms):, leading to a significant gyro bias at longer
time scales, enough to in some cases cause a vehicle to crash if it is
unable to switch to an alternative IMU

Co-authored-by: Andrew Tridgell <andrew@tridgell.net>
Co-authored-by: Steve Evans <Steve@SCEvans.com>
Thanks to all participants here: betaflight/betaflight#12970
@nerdCopter

This comment was marked as outdated.

@nerdCopter nerdCopter marked this pull request as draft October 24, 2023 15:56
@nerdCopter
Copy link
Member Author

nerdCopter commented Oct 24, 2023

i'm unsure what to use: spiBusReadRegister or spiReadRegMsk. both seem to work. but BBL shows some different axis behavior. I don't have this FC mounted so i cannot flight-test :(

left=spiBusReadRegister ; right=spiReadRegMsk (debug, X,Y,Z seemingly re-arranged or maybe just scaled differently)
image

EmuFlight did not have spiReadRegMsk before ICM426xx, which seems to only "wait" before calling spiBusReadRegister.

// Wait for bus to become free, then read a byte of data where the register is ORed with 0x80
uint8_t spiReadRegMsk(const busDevice_t *bus, uint8_t reg)
{
return spiBusReadRegister(bus, reg | 0x80);
}

Copy link
Member

@Quick-Flash Quick-Flash left a comment

Choose a reason for hiding this comment

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

This looks like it should fix the issue, before we merge it we should see if we can replicate the issue on the bench and then show it not happening after this fix.

@Quick-Flash
Copy link
Member

i'm unsure what to use: spiBusReadRegister or spiReadRegMsk. both seem to work. but BBL shows some different axis behavior. I don't have this FC mounted so i cannot flight-test :(

left=spiBusReadRegister ; right=spiReadRegMsk (debug, X,Y,Z seemingly re-arranged or maybe just scaled differently) image

EmuFlight did not have spiReadRegMsk before ICM426xx, which seems to only "wait" before calling spiBusReadRegister.

Since you have an FC that works with this code, If your unsure what scaling or whatever it needs try using the gui to see what rotating the quad does. If you tilt the fc 90 degrees you should see the quad respond correctly to that and not take some extra time to correct, or see it overshoot. If thats the case then we know its working right.

@nerdCopter
Copy link
Member Author

@Quick-Flash , 90 tilts seem to work fine, with the caveat that our Configurator can become out-of-sync with physical position if moved too rapidly. (tested with mpu6000 and bmi270, this is still true, but maybe worse with icm42688p, both this PR and master)

master vs fix (where fix uses spiReadRegMsk)
obvious ticks left, not on right
image

@Quick-Flash
Copy link
Member

@nerdCopter can you test 90 degree pitch with dcm_kp and dcm_ki to zero? This will disable the accelerometer fusion. If 90 degree pitch without going to crazy works fine then I'll say it looks good to merge.

@nerdCopter
Copy link
Member Author

nerdCopter commented Oct 26, 2023

still does it, but sensors tab looks good. maybe i'll wait for flight-test. or maybe just know/consider taht Configurator is obsolete on setup page render.

@nerdCopter
Copy link
Member Author

nerdCopter commented Oct 31, 2023

findings:

  • Flight-tested as good. (@DutchFPV tested multiple builds on JHEF7DUAL)
  • Generally, logging at 8k will cause undesired effect. flight-control is less responsive to stick-input. (not a factor of this PR)
  • ICM4266p Configurator 3D render becomes unsynchronized in both EmuFlight and Betaflight. All gyro's seem to do but icm426xx is a bit more drastic. Does not affect flight.
  • Sensors tab seems to be fine.
  • I had concerns about using spiReadRegMsk versus spiBusReadRegister because they both had a bitwise OR operator (reg | 0x80), but it turns out that reg | x80 is equal to reg | 0x80 | 0x80. Ultimately choosing to call the old function spiBusReadRegister.

@nerdCopter nerdCopter marked this pull request as ready for review October 31, 2023 20:20
@nerdCopter nerdCopter merged commit 3fd882d into emuflight:master Oct 31, 2023
6 checks passed
@nerdCopter nerdCopter deleted the 20231023_disable_icm426xx_AFSR branch October 31, 2023 20:21
@nerdCopter nerdCopter restored the 20231023_disable_icm426xx_AFSR branch June 26, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants