blackbox: optionally log the second gyro on dual-IMU boards - #11933
sensei-hacker merged 14 commits into
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.
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
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 >.
f23c02d to
0386632
Compare
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoOptionally log secondary gyro data on dual-IMU boards
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
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.
|
All three review findings were real. Addressed in the commit above. 1. Secondary sensor selection assumed tags 0 and 1. Confirmed — Now the candidate tags are probed, skipping the one the primary claimed, bounded by the highest tag any in-tree target uses. It picks up the 0-and-2 layout and stays cleanly disabled where there is genuinely nothing to find. I deliberately did not go as far as a target-declared slot mapping — that touches every dual-IMU target and felt out of scope for this PR, but say the word if you would rather have it done properly in one go. As a side note, this also corrected my own reading of 2. SITL build break. Confirmed and reproduced, not just reasoned about:
3. Secondary calibration could restart forever. Confirmed by construction. Changed to Build status after the fixes
The single-IMU control build not changing size at all is the check I care about most, since AIKONF7 sits at 93% flash. The hardware timing measurement is still outstanding and still the thing that should gate merging. |
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.
|
@MrScothh Thanks - you are right, and I had missed #11933 entirely. I only checked #11932 because that was the one named, which is exactly the kind of thing that bites later. Verified both of your points:
And your Offer, your call since #11933 is yours: I can add a small helper to #11905 so the second call site is a one-liner rather than a merge conflict: static float gyroMovementThreshold(uint8_t index)
{
return CALIBRATING_GYRO_MORON_THRESHOLD_DPS / gyroDev[index].scale;
}Then #11933's secondary becomes Either way I will note the dependency in #11905's description so whoever merges first sees it coming. (No hardware or dual-IMU board here, and the firmware CI has not been released for #11905, so none of this is built on my side.) |
|
Test firmware build ready — commit Download firmware for PR #11933 249 targets built. Find your board's
|
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
@Raffi1202 Yes please, add the helper — that is the better shape and I would rather converge on it than leave two spellings of the same idea in one function. One thing I would ask, purely about sequencing. I will keep this PR's self-contained form until #11905 actually lands: CALIBRATING_GYRO_MORON_THRESHOLD * gyroDev[0].scale / gyroDev[1].scalenot out of reluctance, but because So the reconciliation is: you add Noting the dependency in #11905's description is exactly right — I have just done the same here. While I was at it: #11932 was closed as a duplicate of this PR, so its fix now lives only in this PR's first commit. I have said so in the description, and quoted your "worth merging regardless", because with #11932 gone that one line could quietly disappear if someone dropped the first commit. Thanks for looking at it independently — that is the second time in this exchange that checking the PR that was not named turned out to be the useful thing. On your closing note, for symmetry: I do have a dual-IMU board (TBS Lucid H7 Wing) and have built this branch on it, but the measurement that would actually justify merging — what the extra SPI transaction costs the gyro task — I have not run yet. So the arithmetic here is verified by construction and by build, not on hardware, same as yours. |
Asks each gyro for its own scale by index instead of hardcoding the primary. A dual-IMU board may pair two unrelated parts, so the primary's sensitivity is the wrong reference for a secondary sensor. Requested on iNavFlight#11933, which adds a second calibration call site in this function.
|
@MrScothh Helper is in, /* Zero calibration works on raw gyro readings, so the movement threshold has to be
* expressed in that sensor's LSB. Each gyro is asked for its own scale: a dual-IMU
* board may pair two unrelated parts, and the primary's sensitivity would be the
* wrong reference for the secondary. */
static float gyroMovementThreshold(uint8_t index)
{
return CALIBRATING_GYRO_MORON_THRESHOLD_DPS / gyroDev[index].scale;
}The primary call site is now Your sequencing is right and I would not have you do it differently: Good catch on #11932 being closed as a duplicate - I had not seen that. Worth stating plainly for whoever merges this: with #11932 gone, On the hardware point, noted and appreciated - your dual-IMU build is a good deal more than I have. My side is unbuilt upstream (CI still And agreed on the pattern: twice now the useful thing was the PR nobody named. I have started checking who else touches a function before claiming independence. |
|
Build is green: 22 jobs, all 15 target shards and the four SITL platforms, on So Scaffold is cleaned up; nothing extra left on the fork. |
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.
|
This looks interesting. Please let us know when you have flown it. |
|
@sensei-hacker thanks - I'll fly it this weekend and report back here with the results. |
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.
SYNERDUINOH7/CMakeLists.txt is committed upstream with a CRLF ending while .gitattributes marks it as text, so git reports it modified on every Windows checkout and `git add -A` carries it along. It has nothing to do with the second gyro; put the upstream blob back.
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.
f6a9d57 to
e5a4301
Compare
…tead of two All of this lives in the path that only exists once the second IMU is enabled. The cached "calibration complete" flag was a single bool shared by both sensors. The secondary starts its window first and is read first, so it always reached the end of the window first and raised the flag; the primary then skipped its own final sample, kept the zero it had been handed while still calibrating, and left gyroCalibration[0] in progress for good. That is what gyroIsCalibrationComplete() reports, and what areSensorsCalibrating() blocks arming on, so a board with the second gyro enabled would never arm. There is now one flag per sensor. Reproduced in SITL: with the setting off the arming flags clear after a second, with it on the sensors-calibrating bit used to stay up for as long as you cared to watch, and now clears in the same second. The feature also had two switches, gyro_secondary_enabled and a GYRO_2 Blackbox include flag. With the first on and the second off the board detected, calibrated and read the second sensor every gyro cycle and threw the numbers away. The include flag is gone and the setting decides both: on a board where the sensor is not sampled there is nothing to include, and while it is off the second IMU is not initialised at all. Finally, the secondary's calibration is asked to succeed rather than allowed to fail, so a window that ends on a moving aircraft restarts exactly as the primary's does and the two finish together on a still one. A channel meant to be compared against the primary should have its zero measured the same way. Nothing arms on this sensor, so the retry cannot keep the aircraft on the ground; what it costs is that a model which never sits still logs zeroes rather than a biased column.
|
Code review by qodo was updated up to the latest commit a524043 |
The second IMU feeds nothing but gyroRaw2, yet with gyro_secondary_enabled on it was read on every gyro cycle from power-up: on the bench, disarmed, with no log device, after the log had ended. It is now read while it measures its zero and while the blackbox has a log open. The blackbox says so where it changes state, so the gyro loop only tests a flag. The rest is tidying with no change in behaviour. gyroUpdateAndCalibrate() and performGyroCalibration() take the sensor index alone and find the device and the calibration from it, instead of taking pointers and an index that had to agree. The two sensors are named GYRO_PRIMARY and GYRO_SECONDARY rather than 0 and 1, and both start through gyroDevStart(), so their settings are the same by construction and the two channels in a log compare like for like. The condition cache assert explains why it needs LAST + 1 bits: a dual-gyro target now uses the 64th. The setting and docs/Blackbox.md say when the sensor is read, and that GYRO_RAW is needed alongside to compare the two.
The field condition asked whether a second sensor was running, which today is the same thing as the setting being on, but need not stay that way: anything else that starts the second gyro for its own reasons would have put its trace in every log with no way to leave it out. It now asks for the setting as well.
|
Fixed, though not with a flag of its own. The condition asked whether a second sensor was running. Today that is the same thing as A Bench unchanged, on SITL with a second gyro that moves only while armed:
|
The calibration threshold helper this branch asked for arrived upstream in the meantime, so the secondary's threshold comes from gyroMovementThreshold() now instead of being scaled by hand.
GEPRCF745_BT_HD has two IMUs and under a hundred bytes of ITCM to spare, so this branch overflowed it by eight. The secondary is read only while it measures its zero and while a log is being written, which is no reason to spend fast memory, so its path moves into its own function outside FAST_CODE.
Parameterising the read by gyro index cost the fast section 176 bytes, which on a GEPRCF745_BT_HD is most of what it has left. The body is now inlined into two copies: the primary's keeps the constant addresses it had before this branch and stays in the fast section, the secondary's sits in ordinary flash, where the read it does while a log is being written is nobody's hurry.
|
Which milestone would you like this one in? By what you wrote on #11947 it is a feature, so 10.1, but this pull request has no milestone yet and I would rather ask than assume while you are cutting RC1. Where it stands: merged with What is not done is the flight you asked for. I have the test build from this pull request and will report back with a log: what I want to show is So if the flight is what decides it, 10.1 is the honest answer. If you would rather have it in 10.0 on the strength of the bench work, say so today and I will fly it as soon as the weather allows either way. |
|
I'm going to merge it for this RC1 so it makes the "new feature" deadline, then it can be adjusted as needed during the RC phases before final release. |
| #ifdef USE_DUAL_GYRO | ||
| // The second gyro is read only for the log, so only while there is one | ||
| gyroSetSecondaryLogging(newState > BLACKBOX_STATE_STOPPED); |
There was a problem hiding this comment.
4. Paused logs still read the second gyro 🐞 Bug ➹ Performance
blackboxSetState() enables gyroSecondaryLogging for every state numerically above BLACKBOX_STATE_STOPPED, rather than only BLACKBOX_STATE_RUNNING. Header generation, paused logging, and shutdown therefore keep gyroUpdateSecondary() issuing SPI reads even though paused and shutdown paths write no main frames that could contain gyroRaw2.
Agent Prompt
## Issue description
`gyroSetSecondaryLogging(newState > BLACKBOX_STATE_STOPPED)` enables the secondary IMU while headers are emitted, while Blackbox is paused, and while it is shutting down. Those states do not write main log frames, so the extra gyro transaction cannot produce `gyroRaw2` data and contradicts the intended logging-only sampling behavior.
## Fix Focus Areas
- src/main/blackbox/blackbox.c[942-944]
## Recommended Fix
Enable secondary sampling only when entering `BLACKBOX_STATE_RUNNING`, and disable it for every other state. Preserve the existing calibration behavior: `gyroUpdateSecondary()` already continues reading an incomplete secondary calibration even when logging is disabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 15aaf4c |
3 participants
The latent bug this feature turns into a real one was proposed separately as
#11932, which has since been closed as a duplicate of this PR. So the first
commit here is now the only place that fix lives - please keep it rather than
dropping it.
It is one line in
gyroUpdateAndCalibrate(): the function took azeroCalibrationVector_t *and then wrote togyroCalibration[0]regardless.That changes no behaviour today, because the single call site passes
&gyroCalibration[0], and it becomes a real bug the moment a second calibrationslot exists - which is what this PR adds. @Raffi1202 reviewed it independently
while checking #11905 against it and reached the same conclusion: "Worth merging
#11932 regardless - passing a parameter and then ignoring it is the kind of thing
that bites exactly once, later."
Later in this PR
gyroUpdateAndCalibrate()goes one step further and takes thesensor index instead of pointers, so the device and the calibration it works on
come from the same place and cannot disagree at all.
What this adds
On boards that carry two IMUs, INAV currently selects one of them:
gyro_to_usepicks a bus tag,MAX_GYRO_COUNTis 1, and onlygyroDev[0]isever read. The second sensor is present and powered but never sampled.
This PR adds the option to sample it as well, purely as an instrumentation
channel, and to log it to Blackbox as
gyroRaw2.One switch:
set gyro_secondary_enabled = ON. The second IMU is then initialisedat boot, measures its own zero, and is read while a Blackbox log is open, one
extra SPI transaction per gyro cycle; the rest of the time it is not read. It adds
gyroRaw2to the log; includeGYRO_RAWas well to compare the two sensors,since
gyroADCis filtered andgyroRaw2is not. DefaultOFF.An earlier revision also had a
blackbox GYRO_2include flag. I removed it ina524043: with the setting on and the flag off, the board detected, calibrated
and read the second sensor every gyro cycle and threw the numbers away. The
setting exists only to produce that column, so it decides both.
Why
With a single gyro you cannot separate sensor noise from real airframe motion:
whatever you see might be the aircraft. Two sensors rigidly attached to the same
airframe let you separate them by coherence: what correlates between the two is
motion, what does not is noise. That is the number you actually want when
choosing a gyro LPF cutoff, and today it cannot be measured from an INAV log.
The same data makes it possible to characterise a board's two IMUs against each
other, which is useful when deciding what
gyro_to_useshould be.Safety
The secondary never reaches attitude estimation or the PID loops.
gyro.gyroRaw2is read only by
blackbox.c, and the secondary itself is only initialised andread inside the
USE_DUAL_GYROblocks ingyro.c.Three specific points:
1. Read ordering. The secondary is read before the primary's early return,
so a stalled or uncalibrated secondary can never suppress the sample that flies
the aircraft.
2. Calibration ownership. This was the one place where the two sensors
genuinely touched.
gyroConfig()->gyro_zero_cal[]is a single shared triple andperformGyroCalibration()rewrites it on completion. With a second gyrocalibrating, whichever finishes last wins, so the secondary could write its own
bias into the primary's persisted calibration, the one used in flight when
init_gyro_cal = OFF.performGyroCalibration()andgyroUpdateAndCalibrate()now take the sensor index,
GYRO_PRIMARYorGYRO_SECONDARY, and only theprimary writes the config. Likewise only the primary honours
init_gyro_cal: thesecondary always measures its own zero, because the stored calibration belongs to
the other sensor and applying it here would bias the logged samples. Each sensor
also has its own cached "calibration complete" flag. With a shared one, the
secondary finished first, the primary skipped its own final sample and stayed
"calibrating" for good, and the board would not arm; reproduced in SITL before the
fix, and gone after it.
3. Config compatibility: why the new field sits at the end of the struct.
pgLoad()compares only the parameter group version, never the size, and thenmemcpy()sMIN(stored, current)bytes over a freshlypgReset()instance:So a field added mid-struct without a version bump would silently shift every
following field of an existing saved configuration; on
gyroConfig_tthat isthe whole set of gyro filter cutoffs. Appending at the end instead makes the
change purely additive: existing settings keep their offsets, the new field keeps
the default
pgReset()installed, andPG_GYRO_CONFIGdoes not need a versionbump, which means upgrading does not discard anyone's gyro configuration. There
is a comment in
gyro.hsaying so, so the next person does not undo it.If you would rather have the version bumped anyway as a matter of policy, say so
and I will add it; I avoided it specifically to not reset users' settings.
Single-IMU targets are untouched
Everything is under
#ifdef USE_DUAL_GYRO, including the Blackbox fielddefinitions and the condition enum entry, string tables that would otherwise land
in the binary regardless. On a single-IMU target
gyroConfig_tdoes not changeshape at all.
Verified on the ELFs built from the current head rather than by reading the
source:
And against the same two targets built from this PR's merge base:
AIKONF7 was the deliberate control build because it sits at about 93% of FLASH1,
with no room for an accidental regression; it comes out the same size in every
section.
One unrelated line, and why it is here
The Blackbox condition cache is a
uint64_t, and on a dual-gyro target thecondition enum now ends at exactly 64 entries:
FLIGHT_LOG_FIELD_CONDITION_NEVERis 63, the last usable bit. This PR consumes the last free slot. (The enum entry
is under
#ifdef, so single-IMU targets keep upstream's numbering and still haveone spare.)
The assertion that is supposed to catch that is off by one:
FLIGHT_LOG_FIELD_CONDITION_LASTis inclusive:blackboxBuildConditionCache()loops
cond <= LASTand shifts1ULL << cond, so 64 bits hold conditions 0..63and the correct comparison is
>, not>=. With>=the 65th condition passesthe check and produces
1ULL << 64, which is undefined behaviour, silently.Tightened to
>, with a comment next to it saying why. I would normally send thisseparately, but it is the guard on the exact resource this PR exhausts, so it
seemed wrong to leave it for the next person to discover the hard way. Happy to
split it out if you prefer.
What has NOT been verified yet
Being explicit so nobody has to guess:
in the gyro task while a log is open, and I have not measured the task execution
time with and without the option on a physical board. On the target I am using
(TBS_LUCID_H7_WING) the two IMUs sit on separate SPI buses, so there is no bus
contention; but that is an argument, not a measurement. I will post numbers.
Please do not merge on my word alone.
flying wing taking off, climbing and turning both ways with the log open for the
whole flight. SITL's fake IMU answers for both sensors, so the two columns should
agree, and they did: 1746 rows, all three axes, with one row 1 deg/s apart where
the simulator updated between the two reads. That exercises the logging path, not
a second physical sensor.
above is read off
pgLoad(); I have not yet flashed over an existingconfiguration on a dual-gyro board and confirmed the gyro settings survive.
Review of the design and of the safety argument is very welcome in the meantime.
Testing done
TBS_LUCID_H7_WING(dual IMU) andAIKONF7(single IMU), andthe SITL builds clean with and without a second IMU.
only moves while armed, so a stale
gyroRaw2would show as a flat line:init_gyro_cal = OFF;calibration and while a log is open, none otherwise; before 23c4a8e they ran
at about 95/s all the time;
gyroRaw2equal togyroRawon every row of every log, with the gyro moving;docs/Settings.mdregenerated withsrc/utils/update_cli_docs.py, so the"Make sure docs are updated" workflow passes.
docs/Blackbox.mdupdated by hand to describegyroRaw2and when it is read.