feat(radio): add gyro support for HelloRadio V16#7232
Conversation
|
Nice, you start contributing. 👏 |
📝 WalkthroughWalkthroughAdds a new ICM42627 IMU driver ( ChangesICM42627 IMU Driver for Horus/RADIO_V16
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@gismo2004 Sorry for the delay on this one. Can you double check with the firmware built by github @ https://github.com/EdgeTX/edgetx/actions/runs/27930666702 once it is done. I've not made any changes, only rebased the code on main and there weren't any conflicts. It is just that my V16 doesn't show the IMU, so either it doesn't have one, isn't working, or a different part was used (entirely possible, due to gyro supply chain shortages) - the later option would mean it needs to be able to check for more than just the ICM42627. |
![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 ICM42627
initialization is only being logged via TRACE but the init continues execution,
leaving the device in an undefined state. Instead of continuing after a failed
write_cmd call with ICM42627_DEVICE_CONFIG_REG and ICM42627_RESET, the
initialization should fail fast by returning an error code (similar to how
subsequent config writes are handled), ensuring the device is only considered
successfully initialized when the reset command actually succeeds.
🪄 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: 658f24f3-6939-44ad-b9bf-771ebd98e5a4
📒 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
| if (write_cmd(ICM42627_DEVICE_CONFIG_REG, ICM42627_RESET) < 0) { | ||
| TRACE("ICM42627: failed to write reset command"); | ||
| } |
There was a problem hiding this comment.
Fail init when the reset command write is rejected.
At Line 95, a failed reset write is logged but init continues. This can report successful init with an undefined device state; it should fail fast like the subsequent config writes.
Suggested fix
if (write_cmd(ICM42627_DEVICE_CONFIG_REG, ICM42627_RESET) < 0) {
TRACE("ICM42627: failed to write reset command");
+ return -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (write_cmd(ICM42627_DEVICE_CONFIG_REG, ICM42627_RESET) < 0) { | |
| TRACE("ICM42627: failed to write reset command"); | |
| } | |
| if (write_cmd(ICM42627_DEVICE_CONFIG_REG, ICM42627_RESET) < 0) { | |
| TRACE("ICM42627: failed to write reset command"); | |
| return -1; | |
| } |
🤖 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/drivers/icm42627.cpp` around lines 95 - 97, The reset command write
failure in the ICM42627 initialization is only being logged via TRACE but the
init continues execution, leaving the device in an undefined state. Instead of
continuing after a failed write_cmd call with ICM42627_DEVICE_CONFIG_REG and
ICM42627_RESET, the initialization should fail fast by returning an error code
(similar to how subsequent config writes are handled), ensuring the device is
only considered successfully initialized when the reset command actually
succeeds.
|
Yep, I will give it a go when I am back home from work! |
|
Also works for me... Can you check your gyro chip if there is something written on it? |
|
@helloradiosky can you confirm that there are different gyro chip types for the V16? |
@gismo2004 The gyroscope in V16 has always used the ICM42627 chip, but in the future, it may be changed due to unstable chip supply. |
@pfeerick Thank you very much, V16 uses ICM42627, and it has not changed so far. I will test it later. |
|
Did some V16's ship without it? Neither of the V16 radios I have show the gyro with this build. |
The earliest prototypes might not have had the gyroscope firmware installed since the firmware did not yet support the IMU, although the hardware was supported |
I installed the firmware from this PR - no IMU showing in the debug page for either radio. |
I'm very sorry, I will come later to test and see what's going on. |
|
I tested it and confirmed that the edgetx main branch does not have a gyroscope, likely because the code has not been added. |
|
@gismo2004 static const etx_imu_t _imu_candidates[] = {
#if defined(IMU_ICM4207C)
{ &imu_icm42607_driver, IMU_I2C_BUS, ICM426xx_I2C_BASE_ADDR },
{ &imu_icm42607_driver, IMU_I2C_BUS, ICM426xx_I2C_BASE_ADDR + 1 },
#endif
#if defined(IMU_SC7U22)
{ &imu_sc7u22_driver, IMU_I2C_BUS, SC7U22_I2C_BASE_ADDR },
{ &imu_sc7u22_driver, IMU_I2C_BUS, SC7U22_I2C_BASE_ADDR + 1 },
#endif
}; |
|
Ok, that is weird... I checked both my production V16 and early development board, and they both seem to have the ICM42627 populated. So I dug into the gyro code a little more again to make it show diagnostic info and allow me to reset it via cli, and I then realised it was working (as in, giving values, working in the mixer) but not showing on the hardware debug screen. So I restarted the hardset after that, and it has worked perfectly since (debug screen also) with the PR firmware. So I am none the wiser, other than to think maybe something was stuck in the gyro registers/detection (or it could be the first start after a DFU flash causing issues again), but now it is working as expected. So, since we know @gismo2004 has it working, and ICM42627 is the only gyro used at present, this will do for now. I think later down the track we need to tweak how the IMUs are done generally for horus targets, as it is a bit all over the place atm. 
|
5 participants
Summary of changes:
This adds gyro support for HelloRadio V16. Since I was not able to find documentation about the used chip, I took the code used by @helloradiosky as the basis.
The gyro is working as expected:
Let me know if this needs to be done differently or if it is even wanted.
Summary by CodeRabbit
New Features