chore(v16): add support for more IMU hardware#7482
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds V16 IMU build wiring, updates IMU bus selection, and revises ICM42627 initialization timing and register sequencing. ChangesHorus V16 IMU wiring and initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/drivers/icm42627.cpp`:
- Around line 95-97: The reset command write failure in the if statement
checking write_cmd(ICM42627_DEVICE_CONFIG_REG, ICM42627_RESET) is currently only
logging a trace message and continuing execution, which leaves the sensor in a
partially configured state. Instead of just logging the error with TRACE, the
function gyro42627Init() should return a failure status immediately when
write_cmd returns a value less than 0, ensuring that a failed reset is treated
as a fatal error that prevents the initialization from succeeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69dca07d-df01-4e69-9876-7b5e910f91fa
📒 Files selected for processing (5)
radio/src/drivers/icm42627.cppradio/src/drivers/icm42627.hradio/src/targets/horus/CMakeLists.txtradio/src/targets/horus/board.cppradio/src/targets/horus/hal.h
|
Is this duplicated #7232? |
Well, some modifications were made to make it compatible with more gyroscope models |
|
The problem is that it is a duplicate of #7232, while also extending it ... i.e. if I merge #7232 first (which I was intending to do before I saw Richard's mention of this PR), there would most likely be conflicts for merging this one (spoiler, there were). Because the intention was to enhance on #7232, I'll still merge that first and then update this so it can be merged after. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
radio/src/targets/horus/CMakeLists.txt (1)
119-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign SC7U22 source gating with macro gating.
Line 119-120 enables
IMU_SC7U22underPCBREVconditions, while Line 301-303 compilesdrivers/sc7u22.cppunderFLAVOURconditions. Keeping these predicates identical avoids config drift between symbol usage and source inclusion.Suggested diff
-if(FLAVOUR STREQUAL v16) +if(PCB STREQUAL X10 AND PCBREV STREQUAL V16) target_sources(board PRIVATE drivers/sc7u22.cpp) endif()Also applies to: 301-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/horus/CMakeLists.txt` around lines 119 - 120, The SC7U22 macro definition and source file compilation are gated by different CMake conditions, creating a mismatch between symbol definition and actual source inclusion. The add_definitions(-DIMU_SC7U22) uses PCBREV conditions (line 119-120), while the drivers/sc7u22.cpp compilation uses FLAVOUR conditions (around line 301-303). Update the conditional check for compiling drivers/sc7u22.cpp to use the same PCBREV condition as the macro definition to ensure consistency between when the symbol is defined and when the source is actually compiled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@radio/src/targets/horus/CMakeLists.txt`:
- Around line 119-120: The SC7U22 macro definition and source file compilation
are gated by different CMake conditions, creating a mismatch between symbol
definition and actual source inclusion. The add_definitions(-DIMU_SC7U22) uses
PCBREV conditions (line 119-120), while the drivers/sc7u22.cpp compilation uses
FLAVOUR conditions (around line 301-303). Update the conditional check for
compiling drivers/sc7u22.cpp to use the same PCBREV condition as the macro
definition to ensure consistency between when the symbol is defined and when the
source is actually compiled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef937371-3c40-42dd-99ca-e65522192f7d
📒 Files selected for processing (4)
radio/src/drivers/icm42627.hradio/src/targets/horus/CMakeLists.txtradio/src/targets/horus/board.cppradio/src/targets/horus/hal.h
✅ Files skipped from review due to trivial changes (1)
- radio/src/drivers/icm42627.h
🚧 Files skipped from review as they are similar to previous changes (1)
- radio/src/targets/horus/board.cpp
…pecific Replace the RADIO_V16-gated multi-chip candidate table and HAS_IMU check with a single table driven by IMU_xxx chip defines (IMU_ICM42627, IMU_SC7U22, IMU_LSM6DS33), with a fallback for boards that only set the legacy IMU_I2C_ADDRESS (PCBX12S). Lays groundwork for unifying rm-h750/taranis/pa01/st16 onto the same pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/targets/horus/board.cpp`:
- Around line 56-57: The IMU feature guard in board.cpp is too permissive for
IMU_LSM6DS33, allowing that path to compile without an IMU_I2C_ADDRESS even
though the later IMU setup still depends on it. Update the conditional checks
around the IMU support block and the IMU_I2C_ADDRESS usage so that IMU_LSM6DS33
only enables the relevant code when an I2C address is present, keeping the guard
consistent across the definitions near the board feature macros and the IMU
initialization logic in the Horus board target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 005c105a-3ce8-4f2a-a0ee-f745c2cb0ac5
📒 Files selected for processing (1)
radio/src/targets/horus/board.cpp
…MU drivers drivers/sc7u22.cpp had no internal guard, so gating it on FLAVOUR STREQUAL v16 was redundant and inconsistent with lsm6ds.cpp and icm42627.cpp, which are always compiled and rely on --gc-sections to drop unused code when their IMU_xxx define isn't set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Configure the gyro/accel signal path while powered off per datasheet section 12.6, then power on last with the required 200us/45ms timing windows. Also retry the initial I2C device-ready probe instead of a single quick check, since the chip can take longer than that to start acknowledging after power-up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@helloradiosky @gismo2004 Would you be so kind as to test the latest state of this PR on the V16. I think I finally pinned down the intermittent ICM42627 detection to the chip sometimes not responding as soon as we expect it to on power on (and fixed up a few initialisation order inconsistencies while at it)... once it starts responding... all is good. @philmoz If you want to check also, this should hopefully work for your V16 now also... mine is consistently working now, rather than still being somewhat intermittent in detecting the IMU. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/drivers/icm42627.cpp`:
- Around line 120-123: The GYRO_STATIC2 failure path in icm42627::init leaves
the device in bank 1 because it returns immediately after the write_cmd call, so
update this error branch to switch the register bank back to 0 before returning.
Make the fix in the same init sequence that selects bank 1 earlier, ensuring any
transient failure still restores the default bank state for later retries and
subsequent register access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9edd107e-fdaf-48a9-853f-4d8573ea1689
📒 Files selected for processing (1)
radio/src/drivers/icm42627.cpp
Alright, I'll test it right away |
Tested one of the V16 radios and it now works - thanks. |
|
@pfeerick It also works for me, but it worked before too :-). Thanks for taking a look at it and fixing it properly! BTW, where did you get the docs from? |
|
Thanks @philmoz ! :) Fantastic! @gismo2004 Excellent! Yeah, it worked for me for a short while after resetting the IMU a few times, for several reboots, and then stopped again. HRS sent through the datasheets they had ;) |
@pfeerick I have repeatedly tested it, and it is working perfectly. Thank you |
@pfeerick Sorry, if there are still unstable issues, I think it might be a problem with the gyro hardware on your machine. Please keep an eye on it. If it's indeed unstable, I'll arrange for another prototype for you to test |
If the GYRO_STATIC2 write fails during ICM42627 init, the function returned early while still in bank 1, leaving WHO_AM_I/DEVICE_CONFIG (bank 0 registers) unreadable on any subsequent init retry or candidate probe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pfeerick
added
bug 🪲
5 participants
The gyroscope of V16 has been enhanced with multi-chip dynamic loading functionality
The future should support more devices
Summary by CodeRabbit