-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
AP_Arming: Allow memorizing ICE state #25905
base: master
Are you sure you want to change the base?
AP_Arming: Allow memorizing ICE state #25905
Conversation
needs rebase to re-trigger CI checks, as the above esp32 issue is now fixed. |
1ea495d
to
c66972c
Compare
This has now been flown. Found a slight bug where I was comparing to if the state was starting or higher, but I needed to change it to start height delay or start height to cope with the problems from the engine still bouncing between starts when it doesn't fire up the first time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it would be simpler to skip that test in pre-arm if ICE reports the engine is running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the simpler option of just skipping the test when ICE runnig
@@ -142,7 +143,7 @@ const AP_Param::GroupInfo AP_Arming::var_info[] = { | |||
// @Param: OPTIONS | |||
// @DisplayName: Arming options | |||
// @Description: Options that can be applied to change arming behaviour | |||
// @Values: 0:None,1:Disable prearm display,2:Do not send status text on state change | |||
// @Bitmask: 0:None,1:Disable prearm display,2:Do not send status text on state change,3:Use the last passing value for INS consistency check if ICE is running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this to skip INS consistency check if ICE is running
I thought about it, what I liked about this method is it lets me still keep a protection level for a fundamentally bad set of IMU's. The check was clearly added for a reason, and it seemed like it was worth keeping. I could also have just raised the accelerometer threshold used in the test until it passed, but I did want to sanity check the accelerometers first, which if we skip the test means we don't get the protection anymore. |
@WickedShell another possibility is that we offer another data product from AP_InertialSensor or AP_AHRS which is a much lower frequency filtered IMU. Maybe 3Hz or so? This could be calculated in AP_AHRS in the update() function, filtering the current filtered IMU data but at a fixed low frequency. Using 3Hz should be enough to avoid engine RPM, which would usually be above 30Hz |
@WickedShell can you upload a log where the issue is shown? I can apply a 3Hz filter to it and see if it would work |
With the options to start the engine while disarmed I've had some intermittent arming failures due to vibration on the ground causing a problem with the cube between the vibe and non vibe isolated IMU. This is amplified with ground resonance, which is triggering the accel inconsistent check.
This allows us to keep the benefit of having a reasonably tight check for the accelerometer consistency, but once the engine is started we simply remember if we were previously passing the check and keep the check passed.
This was tested on the plane that exhibits the problem, with the option set I can arm while the engine is running, even with a tight threshold on accelerometer consistency.