Conversation
Release/9.1 to master
…rate() The function takes the calibration state to operate on as a parameter, but one line wrote to gyroCalibration[0] directly instead of the argument. There is currently a single call site and it passes &gyroCalibration[0], so this has no effect today. It becomes wrong the moment a second call site exists with a different calibration: the wrong one is marked ZERO_CALIBRATION_DONE.
On boards with two IMUs, gyro_to_use selects one of them and only gyroDev[0] is ever sampled. The second sensor is powered but never read. Add gyro_secondary_enabled, which samples the other IMU purely as an instrumentation channel, and the GYRO_2 blackbox include flag, which logs it as gyroRaw2. Both default to off. The two are separate switches because the costs differ: one is an extra SPI transaction per gyro cycle, the other is log bandwidth. The secondary never reaches attitude estimation or the PID loops. It is read before the primary's early return, so a stalled or uncalibrated secondary cannot suppress the sample that flies the aircraft. Calibration ownership had to be made explicit. gyroConfig()->gyro_zero_cal[] is a single shared triple that performGyroCalibration() rewrites on completion, so a second calibrating gyro could persist its own bias over the primary's. performGyroCalibration() now takes persist and only the primary writes the config; gyroUpdateAndCalibrate() takes isPrimary and the secondary always measures its own zero, ignoring init_gyro_cal, whose stored value belongs to the other sensor. Everything is under USE_DUAL_GYRO, including the field definitions, the condition enum entry and the CLI flag name. On single-IMU targets isPrimary is a compile-time constant and LTO removes the parameters: AIKONF7, at 93% flash, does not grow. The condition enum now ends at exactly 64 entries, so this consumes the last bit of the uint64_t condition cache. The STATIC_ASSERT guarding that compared with >= and would have let the next addition through as 1ULL << 64; tightened to >.
Three issues raised in review on the original commit: 1. Secondary sensor selection assumed the two IMU positions are always tagged 0 and 1. They are not: AETH743Basic registers them as 0 and 2, and some targets register both with tag 0, where not even gyro_to_use can reach the second one. Probe the candidate tags and skip the primary's instead of inverting arithmetically, so the feature works on the 0-and-2 layout and stays cleanly disabled where there is nothing to find. 2. performGyroCalibration()'s new persist parameter was referenced only inside #ifndef USE_IMU_FAKE. SITL defines USE_IMU_FAKE and builds with -Wall -Wextra -Werror, so -Wunused-parameter broke that target. Verified by building SITL with WARNINGS_AS_ERRORS=ON before and after. 3. The secondary's zero calibration was started with allowFailure = false. Nothing gates arming on that sensor, so under sustained vibration the calibration restarts forever and every logged sample stays zero for the whole flight. Allow it to fail once and then log the sensor with a zero offset: a constant bias can be removed in post-processing, a column of zeroes cannot.
The movement threshold is a number of raw counts, and raw counts mean different rotation rates on different parts - a dual-IMU board is free to pair two unrelated sensors. Passing the constant unchanged asked the secondary for the same number rather than the same physical stillness, so a more sensitive secondary could fail a calibration the primary passes. Converting through both scales asks for the same stillness. Where the two sensors are the same part the scales cancel and the value is exactly the old constant, so nothing changes on the boards this has been tested on. This is the same class of bug as iNavFlight#11905, which fixes it for the primary by moving the constant into dps. That change and this one both edit gyroStartCalibration(); once it lands, both call sites become the same expression over each sensor's own scale.
The comment claimed a field appended to gyroConfig_t keeps the default pgReset() installed. That is not unconditionally true, and the exception is invisible: pgLoad() copies MIN(stored, current) bytes over the defaults, so a new field that lands inside the old struct's tail padding is still within what an older configuration stored, and comes back as the zero that padding holds - pgResetInstance() copies the reset template whole, padding included. Nothing changes here, because this field defaults to OFF and zero is therefore the right answer either way. The comment now says that, rather than stating a rule that happens to hold for this field and would mislead anyone appending one whose default is not zero. No functional change. SITL builds clean.
One conflict, and the two sides wanted the same thing from different ends. Upstream cached the primary gyro's calibration state in a global to keep a function call out of the hot path, and kept writing gyroCalibration[0] directly. This branch had made the same line honour the zeroCalibrationVector_t it was handed, which is the fix that PR iNavFlight#11932 carried before it was closed as a duplicate. Both survive: inside this branch of the test the pointer is gyroCalibration[0], so the parameterised form is the same write with the secondary case no longer assumed away, and upstream's cache is set alongside it.
A board with two IMUs currently reads one of them. gyro_to_use picks which physical sensor the single driver opens; the other is never sampled, so there is nothing to compare, average or vote on. PR iNavFlight#11933 changed the first half of that by sampling the second gyro as an instrumentation channel and logging it as gyroRaw2. This is the second half: letting it into the loop. gyro_fusion = AVERAGE feeds the mean of the two sensors to the filters and the controller. That is what two gyros buy without a state estimator to weight them: uncorrelated noise falls by about a third. Betaflight arrived at the same answer - it sums the enabled gyros and divides by their count - after deprecating its own gyro_to_use. What it is not is redundancy, and the setting's description says so. Two sensors that disagree establish that one of them is wrong and cannot establish which. PX4, which does run one EKF instance per IMU and arbitrates between them, writes exactly that in EKF2Selector: with three or more sensors the faulty one is the one with the largest accumulated error, and with two "a fault is present, but the faulty sensor identity cannot be determined". Two guards, because averaging is only safe while both sensors are real: - a read that failed leaves zeroes behind, and averaging those would halve the rate the controller sees - an attenuation that presents as a tuning problem rather than as a broken sensor; - a calibration still in progress still has its bias in it. Either one falls back to the first gyro for that cycle, silently and per-sample. Asking for fusion now starts the second gyro on its own rather than requiring gyro_secondary_enabled as well, so the setting cannot be turned on and do nothing. In the log, gyroRaw stays the first sensor as measured. With fusion on, the second sensor follows the existing raw-gyro flag rather than its own: one aircraft with two IMUs reads as the same field twice. The mean the controller saw is their average, so nothing is lost. Draft: flown by nobody yet, and the per-sample fallback is not visible in the log. See the PR for what remains.
git add -A swept in src/main/target/SYNERDUINOH7/CMakeLists.txt, which differs from upstream only in its line endings on this checkout. It has nothing to do with dual-gyro fusion and should not be in the diff.
Averaging two gyros assumes they share a frame. Each device applies its own alignment before anything downstream sees it, so on a correctly described board they do - but a board describing its second IMU wrongly would have two frames blended into one signal, and nothing about the result would look wrong until it flew. Gravity settles it, and only gravity can. A gyro at rest reads zero whichever way it is turned, so a still aircraft says nothing through one; two accelerometers at rest measure the same physical vector, so the angle between their readings is the angle between their frames. The check therefore runs when the gyro calibration completes, which is the one moment the firmware knows the aircraft was held still, reads the second accelerometer once, and never touches it again - there is no cost in flight, and an angle between two mountings does not change. One blind spot, stated wherever the number is: rotations about gravity are invisible this way. A second IMU mounted 90 degrees out in yaw measures the same gravity as the first. That case needs the two gyros compared while the aircraft turns, which is a different measurement and is not in this commit. Reported, not judged. It goes in the Blackbox header - one number for a whole flight, so a header rather than a field - and in `status`, as degrees rather than a verdict: what counts as too far apart depends on the board, and printing "OK" would be deciding that on no evidence. Nothing is gated on it yet, for the same reason. The probe puts back what accDetect() writes into the global sensor tables, which describe the accelerometer the aircraft actually flies on and must not mention a sensor that was read once and dropped.
Member
|
MrScothh probably already realizes this, so partly this is a reminder to any AI models or other humans reading this: Averaging two sensors more than doubles the incidence of failure. Crashes will happen if EITHER sensor has a problem. It also ADDS new failure cases around the fusion of the two (such as an overflow in the averaging code, if the two sensors together use nearly the full range). So it is strictly less reliable than using either one. Averaging makes the failure rate roughly about three times higher. (Failure in A or failure in B or failure in fusion). Therefore, averaging can only be used when there is good evidence that both are healthy, and when that average itself is similar to the reading from each individual sensor. If the average is close to each individual reading, it is likely healthy. |
Two things the last commit left broken, both of which CI caught:
- accMeasureSecondaryMisalignment() saved the sensor tables through a
`sensor_e`, which is not a type anywhere in the tree. requestedSensors[]
and detectedSensors[] are uint8_t, so say uint8_t.
- docs/Settings.md had not been regenerated since gyro_fusion was added.
The alignment of a second IMU is not something this code has to work out. The target declares it, each driver copies it off the bus tag, and gyroUpdateAndCalibrate() applies each sensor's own alignment before anything downstream looks. All three dual-gyro targets in the tree declare it, and any board where someone has ever set gyro_to_use to the second sensor has already flown on that declaration. So the previous commit measured the wrong thing in the wrong way. It re-probed the accelerometer at runtime, saved and restored the global sensor tables to do it, was blind to rotations about gravity - and then printed a number in `status` that nothing read. Ninety-one lines to tell a pilot something they would never look at, about a constant that is already known. What is worth having is a decision. Two gyros averaged into one control signal can be wrong in a way that is hard to attribute: the aircraft flies badly and the tuning gets blamed. So compare them and stop averaging when they disagree. The test is PX4's, because its shape is right. The difference between the two is low-passed, which leaves only what persists and throws away the sampling noise that makes any instantaneous comparison useless. That is then integrated through a dead band, so ordinary disagreement accumulates nothing; what does accumulate is a rate integrated over time, which is an angle, and the pair is tolerated until the disagreement would have cost fifteen degrees of attitude. The thresholds are PX4's own defaults, which are conservative and have flown, rather than tighter numbers invented for a board I do not have. With two sensors a disagreement cannot name the culprit - PX4 says exactly that and stops - so this keeps the first gyro, which is the one every other board flies on, and says so in `status`. It does not go back: a control signal that alternates between one sensor and two is a disturbance in itself.
Author
|
You're right that this is not redundancy, and the PR says so in as many words: two sensors that disagree cannot say which one is wrong. The reason to average is uncorrelated noise, not reliability, and the failure surface does grow. The condition you describe, only averaging when there is evidence both are healthy and when the average stays close to each individual reading, is now enforced rather than assumed. The gate is modelled on PX4's selector. Each sensor's distance from the pair's mean is low-passed first, so sampling noise cannot trigger it, and only the excess over a dead band is integrated. Since that is a rate integrated over time, the accumulator is an angle: the pair is tolerated until the disagreement would have been worth 15 degrees of attitude. The thresholds are PX4's own defaults ( When it trips, averaging stops and the first gyro flies alone until the next boot. With two sensors the faulty one cannot be identified, which is what PX4 states in that case too, so choosing between them would be guesswork. On overflow: the mean is taken between two floats already scaled to deg/s, at most +/-2000 each, so the sum cannot overflow, and there is no integer path in it. More broadly, what I am really after is to find a use for the second IMU at all. INAV already declares it in the targets and applies its alignment, and Averaging is one candidate. A redundancy mode is another, and possibly the more valuable one. It would not be redundancy against silent drift, for exactly the reason you give: with two sensors a disagreement has no verdict. But a sensor that stops responding, saturates, or returns implausible values is detectable without having to choose between two opinions, and falling back to the other one is well defined in that case. That looks like a narrower promise that can actually be kept. If you have a view on which of these is worth having, or on a third use I have not thought of, I would welcome it. |
The description claimed averaging required gyro_secondary_enabled, while the code reads the sensor when either is set. Two places describing one rule, and they had already drifted apart. Say what the code does, and note in the comment that neither switch needs the other, so the next reader of either one is not left to work it out. Also say where the averaging stops: a pair that keeps disagreeing gives up and the first gyro flies alone, which belongs in the setting's own description rather than only in the commit that added it.
Averaging two sensors widens the failure surface rather than narrowing it, as was pointed out on the pull request: a crash follows a fault in either one, plus whatever the fusion itself adds. The answer asked for was that averaging only happen where there is evidence both sensors are healthy. This is that evidence. The checks are PX4's DataValidator, minus the two that need a driver error count the accgyro layer does not keep: has it ever produced a sample, has it produced one within 40 ms, and is it repeating a single value. The thresholds are PX4's own. Health is deliberately a question about the data stream and not about the numbers. Whether a sensor is alive can be decided on its own; whether its numbers are right can only be decided against another sensor, and with two the disagreement names no culprit. The existing consistency gate answers the second question and this answers the first, and averaging now requires both. The frozen test needed one thing PX4 does not: a simulated gyro standing still reads exactly zero sample after sample, so in HITL on the ground an identical-value test would condemn a healthy sensor. It is the same exact zero that makes the Kalman filter produce NaN in iNavFlight#11876. So a sensor counts as frozen only while the other one is moving; if neither is moving, neither is the aircraft.
MrScothh
force-pushed
the
feature/dual-gyro-fusion
branch
from
September 17, 2026 07:43
0bf3e5d to
9833986
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, and stacked on #11933. The code compiles and the reasoning is written down, but nothing has flown with it and the questions at the end are open.
What to review here. This branch is built on #11933, which is where the second gyro starts being sampled at all, so until that merges this diff carries its commits too. The fusion itself is the last commit: 85 lines across
sensors/gyro.c,sensors/gyro.h,settings.yaml, and one condition inblackbox.c. Everything above that belongs to #11933 and reads better there.What it does
gyro_fusion = AVERAGEfeeds the mean of two IMUs to the filters and the controller, instead of reading one and ignoring the other. That is what a second gyro buys without a state estimator able to weight them: uncorrelated sensor noise drops by about a third, and nothing else changes.Betaflight reached the same answer from the other direction - it deprecated
gyro_to_usein favour of a bitmask of enabled gyros, sums the enabled ones and divides by the count (sensors/gyro.c). INAV today only picks which physical device the single driver opens, so the second sensor is never read.What it is not
Redundancy. Two sensors that disagree establish that one of them is wrong, and cannot establish which. PX4 - which runs one EKF instance per IMU and arbitrates between them - puts it plainly in
EKF2Selector.cpp: with three or more sensors the faulty one is the one with the largest accumulated error, and with two, "a fault is present, but the faulty sensor identity cannot be determined". The setting's description says this, because "use both IMUs" reads like fault tolerance to anyone who has not thought about it.Guards
Averaging is only safe while both sensors are real, so two conditions fall back to the first gyro for that cycle:
Asking for fusion also starts the second gyro by itself, rather than silently doing nothing until
gyro_secondary_enabledis set too.Logging
gyroRawstays the first sensor as measured. With fusion on, the second sensor follows the existing raw-gyro flag rather than its own, so one aircraft with two IMUs reads as the same field twice rather than as a measurement plus an optional extra. The mean the controller used is the average of the pair, so nothing is lost by not logging it separately.Open, and why it is still a draft
Nobody has flown it. It needs a dual-IMU board, a log with both sensors, and a comparison of the noise floor with fusion on and off. Bench-testing it is not enough to justify it.
The per-sample fallback is invisible in the log. When a cycle falls back to the first gyro, the log cannot show that it happened - the fields look the same. That wants a bit, not three more fields; whether it is worth one depends on how often the fallback actually fires, which nobody has measured.
Alignment: half measured, half still open.
Assumed to be handled upstream.The two accelerometers are now compared against gravity when the gyro calibration completes - the one moment the aircraft is known to be still - and the angle between their frames goes into the Blackbox header andstatus. One reading, never repeated, nothing in flight.What that cannot see is a rotation about gravity: a second IMU mounted 90 degrees out in yaw measures exactly the same gravity as the first and passes. Catching that needs the two gyros compared while the aircraft turns, which is a separate measurement and is not written yet.
Nothing is gated on the number. What counts as too far apart depends on the board, and picking a threshold without having seen the figure on a healthy one would either disable fusion on good hardware or fail to fire on bad.
Configurator side is not written yet. The intended shape is a "Use dual IMU" switch under Other Features in Configuration, with the separate second-gyro logging checkbox shown only when it is off - since with fusion on, the existing raw-gyro checkbox already covers both.
Built for AOCODARCF7DUAL; SITL does not define
USE_DUAL_GYRO, so it compiles none of this.