feat(imu): add BMI220 sensor support for TUYA_T5AI_PIXEL board#560
feat(imu): add BMI220 sensor support for TUYA_T5AI_PIXEL board#560robeortZ wants to merge 2 commits intotuya:masterfrom
Conversation
The PIXEL board ships with a BMI220 (chip_id 0x26) instead of BMI270 (chip_id 0x24). BMI220 requires its own 8192-byte config firmware (sourced from ChromeOS EC v2.47.1) — the BMI270 firmware is incompatible. Changes: - Add BMI220 driver (board_bmi220_api.c/h) using Bosch BMI2 library - Add BMI220-specific config firmware (bmi260_config_file.c) - Patch bmi2.c chip_id check to accept 0x26 - Add IMU abstraction layer in tuya_main.c (BMI220 preferred, BMI270 fallback) - Add ENABLE_IMU_BMI220 Kconfig option Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
BMI270和220经常混料,所以pixel这个项目增加BMI220的驱动 |
shiliu-yang
left a comment
There was a problem hiding this comment.
PR #560 Code Review: feat(imu): add BMI220 sensor support for TUYA_T5AI_PIXEL board
Author: robeortZ | +1318 / -22 | 8 files changed
Overview
This PR adds support for the BMI220 IMU chip (chip_id 0x26) that the PIXEL board actually ships with, instead of BMI270 (chip_id 0x24). The approach: add a new BMI220 driver, add BMI220-specific config firmware, patch the Bosch BMI2 library's chip_id check, and introduce an abstraction layer in tuya_main.c that tries BMI220 first and falls back to BMI270.
The functional goal is valid and the sensor data shown in the migration doc confirms it works. However, there are several correctness and maintainability issues that need attention.
Issues
HIGH — Must Fix
1. Magic number in shared library code (bmi2.c:1904)
// Current
if (chip_id == dev->chip_id || chip_id == 0x26)bmi2.c is a shared Bosch library used by ALL boards. This hardcoded 0x26 is a global side-effect — it weakens the chip_id guard for every consumer of the library, not just BMI220. Should at minimum use a named constant, and ideally be gated behind a #ifdef or passed via the dev struct.
2. I2C resource leak on BMI220 init failure
In board_bmi220_init(), tkl_i2c_init() is called but there is no matching tkl_i2c_deinit() in any error path. When BMI220 init fails and the code falls back to BMI270 (which uses the same TUYA_I2C_NUM_0/GPIO20+21), the BMI270 driver will call tkl_i2c_init() on an already-initialized port. The behavior is platform-dependent and may silently fail or cause undefined behavior.
3. Comment contradicts reality in board_bmi220_api.c
/* External: BMI270 config file (we reuse it for chip ID 0x26) */
extern const uint8_t bmi260_config_file[];The comment says "BMI270 config file" — this is wrong. bmi260_config_file[] is the BMI220-specific firmware sourced from ChromeOS EC, not the BMI270 firmware. This will mislead future maintainers into thinking the wrong firmware is intentionally reused when it is in fact a different, correct firmware.
MEDIUM — Should Fix
4. Misleading naming: bmi260_config_file.c placed in bmi270/ directory
The BMI220 firmware blob is placed at src/peripherals/imu/bmi270/bmi260_config_file.c. Storing BMI220 firmware inside the bmi270/ directory creates confusing ownership. Should live in its own bmi220/ subdirectory or at least the existing imu/ level.
5. chip_id field in bmi220_dev_t is never populated
typedef struct {
...
uint8_t chip_id; /* Read-back chip ID */
...
} bmi220_dev_t;board_bmi220_init() never writes back the actual read chip_id to dev->chip_id. This field is dead — either populate it or remove it to avoid confusion.
6. Production debug logging left in sand_update_physics()
static uint32_t imu_dbg_cnt = 0;
if (imu_dbg_cnt++ % 500 == 0) {
PR_NOTICE("IMU data: acc(%.2f, %.2f, %.2f) ...");
}Periodic PR_NOTICE/PR_ERR/PR_WARN every 500 iterations will fire continuously in production. This is a debugging aid that should be removed or gated behind a debug build flag.
7. board_bmi220_scan_i2c() is unused
The function is declared, implemented, and exported in the header but never called anywhere. Either wire it into the init flow (e.g., detect presence before full init) or remove it.
8. BMI270 API functions called from BMI220 driver without explanation
rslt = bmi270_get_sensor_config(config, 2, dev);
rslt = bmi270_set_sensor_config(config, 2, dev);
rslt = bmi270_sensor_enable(sensor_list_220, 2, &bmi2_dev_220);These bmi270_* calls inside board_bmi220_api.c are valid due to register-level compatibility, but there's no comment explaining why BMI270 API functions are appropriate for a BMI220 chip. This will look like a copy-paste bug to future reviewers.
LOW — Suggestions
9. Migration doc is entirely in Chinese
BMI270_to_BMI220_migration.md is 108 lines of Chinese. As an open-source project, docs committed to the repo should be in English (or bilingual). The PR template also calls for English comments.
10. PR quality checklist is entirely unchecked
All five checklist items in the PR description are [ ] (unchecked). Per the template, these must be reviewed and checked before merge: English comments, file header format, Doxygen function headers, coding style guide compliance, and code formatting tool.
11. board_bmi220_read_accel() / board_bmi220_read_gyro() are unused
These two functions are implemented and exported but never called. Either use them or don't export them.
Summary
| Severity | Count |
|---|---|
| HIGH | 3 |
| MEDIUM | 5 |
| LOW | 3 |
The core approach is correct and the working sensor data validates the hardware fix. But the shared library patch (bmi2.c) needs proper scoping, the I2C leak on failure needs fixing, and the misleading comments need correction before this should merge.
Reviewed with Claude Code
HIGH fixes: - Revert bmi2.c chip_id hack: remove hardcoded 0x26 from shared library. The BMI220 driver sets dev->chip_id = 0x26 before calling bmi2_sec_init(), so the original check (chip_id == dev->chip_id) passes naturally. - Add tkl_i2c_deinit() on all BMI220 init error paths to prevent I2C leak - Fix misleading comments about config firmware source MEDIUM fixes: - Move bmi260_config_file.c from bmi270/ to bmi220/ directory - Add CONFIG_ENABLE_IMU_BMI220 section in imu/CMakeLists.txt - Populate dev->chip_id field after successful init - Remove production debug logging in sand_update_physics() - Remove unused board_bmi220_scan_i2c() and board_bmi220_read_gyro() - Add comments explaining why bmi270_* APIs are valid for BMI220 - Fix inaccurate Kconfig descriptions LOW fixes: - Convert migration doc from Chinese to English Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The PIXEL board ships with a BMI220 (chip_id 0x26) instead of BMI270 (chip_id 0x24). BMI220 requires its own 8192-byte config firmware (sourced from ChromeOS EC v2.47.1) — the BMI270 firmware is incompatible.
Changes:
PR 描述/PR description
[在此详细描述 PR 的内容]/[Describe the PR content in detail here]
代码质量/Code Quality:
在本次拉取请求中,我已考虑以下事项 As part of this pull request, I've considered the following: