-
Notifications
You must be signed in to change notification settings - Fork 7.7k
sensor: Add ICM42686 Driver Support #92579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
sensor: Add ICM42686 Driver Support #92579
Conversation
0f64e78
to
7477da4
Compare
Looks like the issues flagged by SonarQube are due to moving existing in-tree files. I wonder if we're good with addressing them on follow-up PRs instead of blocking this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition! I also tested this on real hardware, both targets (42688 and 42686) work perfectly.
As a first step to enable the similar variants (e.g: ICM42686), refactor common functionality into icm4268x files. As a result, applications using the icm42688 will need to have both compatible properties: "invensense,icm42688" and "invensense,icm4268x" defined. In-tree boards have been modified to comply with this pattern. This patch does not contain functional changes. The driver should work the same as before. Signed-off-by: Luis Ubieda <[email protected]>
Addressing low-hanging fruits. Put in a separate commit in order to make it easier to keep track of changes. Signed-off-by: Luis Ubieda <[email protected]>
Now this driver supports both ICM42688 and ICM42686. Tested with read-decode as well as streaming mode. Signed-off-by: Luis Ubieda <[email protected]>
Additionally, remove overlay from sensor-shell sample, which is redundant. Signed-off-by: Luis Ubieda <[email protected]>
For build-time validation. Signed-off-by: Luis Ubieda <[email protected]>
Users now requiring to define both dt compatibles: both icm4268x and icm42688 to work. Signed-off-by: Luis Ubieda <[email protected]>
f135b0c
7477da4
to
f135b0c
Compare
|
I just rebased to resolve the conflicts and move the migration guide to 4.3.0. WRT SonarQube issues: I gave another pass at the issues pointed out, and they're now reduced to two types:
After this round, I've repeated the test-verification listed in the PR description and I've got the same results. I'll stick around to see all CI checks pass, then I'll stand by for another round of reviews. |
zephyr_library_sources_ifdef(CONFIG_SENSOR_ASYNC_API icm4268x_rtio.c) | ||
zephyr_library_sources_ifdef(CONFIG_ICM4268X_DECODER icm4268x_decoder.c) | ||
zephyr_library_sources_ifdef(CONFIG_ICM4268X_STREAM icm4268x_rtio_stream.c) | ||
zephyr_library_sources_ifdef(CONFIG_ICM4268X_TRIGGER icm4268x_trigger.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of all the APIs the Sensor API has currently, I believe the trigger API is the least usable among them and conflicts the most with the stream API. I'm wondering if perhaps this API is one we really should aim to deprecate.
Supporting the polling read and stream is fairly easy I think. Supporting trigger and stream seems conflict ridden.
Description
This PR adds support for ICM42686 driver, by extending and refactoring existing ICM42688 driver.
The new variant contains feature parity with the original driver:
Testing
This was tested on VMU RT1170, which contains both ICM42688 and ICM42686, running the sensor-shell sample to set up and obtain sensor readings: