Add ICM40609D IMU driver - #11765
Open
sensei-hacker wants to merge 2 commits into
Open
Conversation
Wire in a gyro/accel driver for the TDK ICM40609D, modeled on the existing ICM42605 driver. Register map, WHO_AM_I, and scale factors verified directly against the ICM-40609-D datasheet (DS-000272 rev 0.8) rather than trusted from Betaflight's driver, which mislabels ACCEL_FS_SEL=0 as +/-16g when the datasheet specifies +/-32g, and originally reset via the wrong register (0x4C instead of 0x11, fixed upstream in betaflight/betaflight#14415).
Add a rate-config table (mirroring ICM42605's) so the driver selects GYRO_CONFIG0/ACCEL_CONFIG0 ODR from gyro->requestedSampleIntervalUs instead of always running at 1kHz, which would have silently capped the PID loop rate on any board using this chip. Also document why the ICM42605-style INT_CONFIG1/ASYNC_RESET clear step is omitted: that register isn't present in the ICM-40609-D register map.
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
PR Summary by QodoAdd TDK ICM40609D IMU driver and wire into sensor autodetect
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Contributor
Code Review by Qodo
1. Enum value compatibility break
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
47
to
50
| ACC_ICM45686, | ||
| ACC_ICM40609D, | ||
| ACC_FAKE, | ||
| ACC_MAX = ACC_FAKE |
Contributor
There was a problem hiding this comment.
1. Enum value compatibility break 🐞 Bug ≡ Correctness
Adding ACC_ICM40609D before ACC_FAKE shifts the numeric value of ACC_FAKE, so configs saved with acc_hardware=FAKE will deserialize as ACC_ICM40609D after upgrade and attempt to initialize the wrong sensor driver.
Agent Prompt
### Issue description
`ACC_ICM40609D` was inserted before `ACC_FAKE`, which changes the implicit numeric value of `ACC_FAKE`. Because `accelerometerConfig()->acc_hardware` is persisted as a raw PG value, existing saved configs using `ACC_FAKE` will be misinterpreted as `ACC_ICM40609D` after flashing this firmware, causing incorrect sensor init.
### Issue Context
- `accelerometerConfig` is a persisted PG and `acc_hardware` is used directly for detection.
- The YAML table is order-synced with the enum, so the numeric index change is real.
### Fix Focus Areas
- src/main/sensors/acceleration.h[34-51]
- src/main/sensors/acceleration.c[90-101]
- src/main/fc/settings.yaml[1-6]
- src/main/config/config_eeprom.c[56-72]
### Suggested fix approach
- Implement a backward-compatibility migration:
- Bump the `accelerometerConfig` PG version.
- On load of older versions, remap the old numeric value that previously meant `ACC_FAKE` to the new `ACC_FAKE` value.
- Alternatively (less preferred if many enums rely on ordering), assign explicit stable numeric values for the enum members and update any loops/range checks accordingly, ensuring `ACC_MAX` reflects the true maximum.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+192
to
+195
| if (tmp == ICM40609D_WHO_AM_I_CONST) { | ||
| return true; | ||
| } | ||
| } while (attemptsRemaining--); |
Contributor
There was a problem hiding this comment.
2. Retry loop off-by-one 🐞 Bug ☼ Reliability
icm40609dDeviceDetect() uses a uint8_t post-decrement do/while which performs 6 WHO_AM_I reads for attemptsRemaining=5, adding an extra 150ms delay and making retry behavior misleading.
Agent Prompt
### Issue description
The retry loop in `icm40609dDeviceDetect()` runs one extra time due to `do { ... } while (attemptsRemaining--);` with an unsigned counter. This adds an extra delay on failed detection and obscures intent.
### Issue Context
This does not cause an infinite loop, but it *does* perform 6 attempts when initialized to 5.
### Fix Focus Areas
- src/main/drivers/accgyro/accgyro_icm40609d.c[174-198]
### Suggested fix approach
Replace with an explicit bounded loop, e.g.:
```c
for (uint8_t i = 0; i < 5; i++) {
delay(150);
if (busRead(dev, MPU_RA_WHO_AM_I, &tmp) && tmp == ICM40609D_WHO_AM_I_CONST) {
return true;
}
}
return false;
```
(or keep the existing structure but use `while (attemptsRemaining-- > 0)` and ensure the intended attempt count).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
Adds a gyro/accel driver for the TDK ICM-40609-D, modeled structurally on the existing
accgyro_icm42605.cdriver, and wires it into sensor autodetect.Changes
drivers/accgyro/accgyro_icm40609d.c/.hDEVHW_ICM40609Dbus enum, centralizedICM40609D_WHO_AM_I_CONSTinaccgyro_mpu.hsensors/gyro.c/sensors/acceleration.cautodetect dispatch,gyro.h/acceleration.henumsfc/settings.yamlacc_hardwaretable andfc/cli.cgyroNames[]updated (both order-synced with the enums)CMakeLists.txtsource listAll register addresses, the WHO_AM_I value (0x75 / 0x3B), power-mode bits, ODR/full-scale-range encodings, and the soft-reset sequence were verified directly against the TDK ICM-40609-D datasheet (DS-000272 rev 0.8), not copied verbatim from Betaflight's driver. Betaflight's reference implementation (betaflight/betaflight#14367) had two issues that are not carried into this driver:
0x4Cinstead of0x11), fixed upstream in Fix ICM40609D reset register betaflight/betaflight#14415 — confirmed here against the datasheet directly.ACCEL_FS_SEL=0macro is labeled+-16g, but the datasheet specifiesACCEL_FS_SEL=0is+-32g(FS_SEL=1is+-16g) — this driver usesFS_SEL=1for+-16g/2048 LSB/g.This chip's Bank 0 register map also does not include an
INT_CONFIG1(0x64) register — confirmed absent from the datasheet, register map goesINT_CONFIG0(0x63) straight toINT_SOURCE0(0x65) — so the ICM42605-style "clear ASYNC_RESET" erratum step is intentionally omitted (see code comment).Gyro sample rate is selected via a rate-config table (mirroring ICM42605's) driven by
gyro->requestedSampleIntervalUs, rather than a fixed rate, so it doesn't cap the PID loop rate below what's configured.Recommended test target
No shipping target currently defines
USE_IMU_ICM40609D. #11701 "Add ORBITH743v2 target" (open) is the natural candidate — that board pairs an ICM42688P with an ICM40609D as its second IMU, and currently registers the ICM40609D underDEVHW_ICM42605as a stand-in since no dedicated driver existed. Once this PR merges, that target'sbusdev_icm40609registration should switch toDEVHW_ICM40609Dand enableUSE_IMU_ICM40609D, then be flashed/tested on real ORBITH743v2 hardware.Testing
USE_IMU_FAKE; confirms the enum/CMake/settings wiring doesn't break the build)AETH743Basic, withUSE_IMU_ICM40609Dtemporarily scaffolded in to exercise the driver's actual code path — compiles and links cleanly, no warnings. That scaffold edit was reverted before this PR; no shipping target'starget.his changed here.Code Review
Reviewed with inav-code-review agent. Two IMPORTANT findings were addressed (hard-coded 1kHz ODR replaced with a proper rate-config table; INT_CONFIG1 omission documented with a code comment) and a minor CMakeLists.txt ordering nit was fixed.