adding icm42670p driver - #201
Conversation
|
Looks promising, can you prepare any sort of demo? I mean YT video for example and cli |
|
i will post it as fast as i can |
📝 WalkthroughWalkthroughThis PR adds support for the ICM-42670-P gyro sensor to the ESP-FC flight controller firmware. A new device type enum is defined, a complete device driver class is implemented with register initialization and fast sensor reads, and the device is integrated into the hardware auto-detection system for both SPI and I2C buses. ChangesICM-42670-P Gyro Device Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
⚔️ Resolve merge conflicts
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/Espfc/src/Device/GyroICM42760P.h (1)
51-64: 💤 Low valueConsider verifying bus write success during initialization.
The initialization writes to multiple registers but doesn't check return values from
writeByte. If a write fails (e.g., due to bus error), the device may not be properly configured, yetbegin()will return success. Consider checking at least the critical power management write.🤖 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 `@lib/Espfc/src/Device/GyroICM42760P.h` around lines 51 - 64, The init sequence calls _bus->writeByte repeatedly (e.g., for ICM42670P_REG_DEVICE_CONFIG, ICM42670P_REG_PWR_MGMT0, ICM42670P_REG_GYRO_CONFIG0, ICM42670P_REG_ACCEL_CONFIG0) but ignores return values; update begin() to check the boolean/return result of at least the critical power management write (_bus->writeByte(_addr, ICM42670P_REG_PWR_MGMT0, ICM42670P_PWR_LN_MODE)) and fail early if it indicates an error (log or set an error state and return false from begin()), and optionally propagate similar checks for the other writes (retry or abort) so device initialization cannot report success when bus writes fail.
🤖 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 `@lib/Espfc/src/Device/GyroICM42760P.h`:
- Around line 1-2: The header file is misnamed GyroICM42760P.h but the device
and symbols use ICM42670P; rename the file to GyroICM42670P.h to match the
include guard/class naming and ensure consistency, then update any includes that
reference the old name (notably the include in Hardware.cpp) to the new filename
GyroICM42670P.h so compilation and symbol resolution remain correct.
In `@lib/Espfc/src/Hardware.cpp`:
- Line 9: Update the include in Hardware.cpp to reference the corrected header
name: replace the old include of "Device/GyroICM42760P.h" with the corrected
"Device/GyroICM42670P.h" so that the compilation uses the fixed GyroICM42670P.h
header (search for the include line containing GyroICM42760P and change the
filename to GyroICM42670P).
---
Nitpick comments:
In `@lib/Espfc/src/Device/GyroICM42760P.h`:
- Around line 51-64: The init sequence calls _bus->writeByte repeatedly (e.g.,
for ICM42670P_REG_DEVICE_CONFIG, ICM42670P_REG_PWR_MGMT0,
ICM42670P_REG_GYRO_CONFIG0, ICM42670P_REG_ACCEL_CONFIG0) but ignores return
values; update begin() to check the boolean/return result of at least the
critical power management write (_bus->writeByte(_addr, ICM42670P_REG_PWR_MGMT0,
ICM42670P_PWR_LN_MODE)) and fail early if it indicates an error (log or set an
error state and return false from begin()), and optionally propagate similar
checks for the other writes (retry or abort) so device initialization cannot
report success when bus writes fail.
🪄 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: edc4ce9b-6947-4055-afa7-91022df3b3dc
📒 Files selected for processing (3)
lib/Espfc/src/Device/GyroDevice.hlib/Espfc/src/Device/GyroICM42760P.hlib/Espfc/src/Hardware.cpp
| #ifndef _ESPFC_DEVICE_GYRO_ICM42670P_H_ | ||
| #define _ESPFC_DEVICE_GYRO_ICM42670P_H_ |
There was a problem hiding this comment.
Filename typo: GyroICM42760P.h should be GyroICM42670P.h.
The file is named GyroICM42760P.h but the device is ICM-42670-P (note: 42670, not 42760). The include guard and class name correctly use 42670P. Rename the file to GyroICM42670P.h for consistency, and update the include in Hardware.cpp accordingly.
🤖 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 `@lib/Espfc/src/Device/GyroICM42760P.h` around lines 1 - 2, The header file is
misnamed GyroICM42760P.h but the device and symbols use ICM42670P; rename the
file to GyroICM42670P.h to match the include guard/class naming and ensure
consistency, then update any includes that reference the old name (notably the
include in Hardware.cpp) to the new filename GyroICM42670P.h so compilation and
symbol resolution remain correct.
| #include "Device/GyroLSM6DSO.h" | ||
| #include "Device/GyroICM20602.h" | ||
| #include "Device/GyroBMI160.h" | ||
| #include "Device/GyroICM42760P.h" |
There was a problem hiding this comment.
Update include path when filename is corrected.
This include path references the typo'd filename (GyroICM42760P.h). When the filename is corrected to GyroICM42670P.h, update this include accordingly.
🤖 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 `@lib/Espfc/src/Hardware.cpp` at line 9, Update the include in Hardware.cpp to
reference the corrected header name: replace the old include of
"Device/GyroICM42760P.h" with the corrected "Device/GyroICM42670P.h" so that the
compilation uses the fixed GyroICM42670P.h header (search for the include line
containing GyroICM42760P and change the filename to GyroICM42670P).
rtlopez
left a comment
There was a problem hiding this comment.
Hi @r-u-t-r-A To proceed I need all issues to be addressed, Also you need to rebase your changes. Thank you.
| GYRO_ICM20602 = 7, | ||
| GYRO_BMI160 = 8, | ||
| GYRO_ICM42670P = 9, | ||
| GYRO_MAX |
There was a problem hiding this comment.
I think You missed this place
https://github.com/r-u-t-r-A/esp-fc/blob/385dc0453495c6b8dc2090d9318657e02d0c9e71/lib/Espfc/src/Device/GyroDevice.cpp#L9
lib/Espfc/src/Device/GyroDevice.cpp:9
static const char* devChoices[] = { PSTR("AUTO"), PSTR("NONE"), PSTR("MPU6000"), PSTR("MPU6050"), PSTR("MPU6500"), PSTR("MPU9250"), PSTR("LSM6DSO"), PSTR("ICM20602"),PSTR("BMI160"), NULL };
Hi, ive created a driver for icm42670p since all MPU IMUs are end of life products, code compiles, tested on esp32s3 with gyro connected over i2c
Summary by CodeRabbit
Release Notes