drivers: sensor: icm42670: supports icm42670-P and icm42670-S #71374

afontaine-invn · 2024-04-11T08:52:49Z

dts: supports icm42670-P/-S
samples: sensor: tdk_apex: TDK APEX generic sample
samples: sensor: 6DOF motion DRDY generic sample
module: HAL TDK module

Adds official TDK Invensense Inc. driver in TDK HAL module for icm42670-P/-S sensor.
Adds SPI and I2C interface support.
Adds APEX features, such as Pedometer, Tilt detection, Wake on Motion
and Significant Motion Detector.
Add generic samples supporting this driver.
Integration module test successful with https://github.com/tdk-invn-oss/zephyr.hal_tdk
Validated with custom setup: nrf52dk_nrf52832 + icm42670-P/-S EVB board
Build ok by running:
west twister -T samples\sensor\tdk_apex-T samples\sensor\6dof_motion_drdy -p nrf52dk/nrf52832

Signed-off-by: Aurelie Fontaine aurelie.fontaine@tdk.com

github-actions · 2024-04-11T08:53:26Z

Hello @afontaine-invn, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

gmarull

please, check Zephyr contribution guidelines. There seem to be PDFs and binary files in this PR.

teburd

An initial pass here (and this is large so I'll need time to look at it again) with some high level notable comments but...

I love that this is being submitted as a PR from you, with a TDK email address. I hope this is just the first of many to be frank.

That said there's some issues that will need to be addressed. Blobs are problematic, pdf's are problematic, rewriting the register map but not using the zephyr macros for describing masks and manipulating fields seems like a step backwards. Using the existing trigger API requiring a thread or work queue also feels like a step backwards, but that's not on you.

Is the code under the imu directory here something from a TDK software devkit? If so it might be worth considering looking at how ST packages and uses stmemsc for ST sensor drivers. It does not need to live or be managed then by Zephyr but can be managed entirely but TDK in something like a hal_tdk git repository.

.../invn.algo.sw-integration.aml-42670s-gcc-arm-none-eabi-cm4-fpu-1.6.0/include/invn_algo_aml.h

samples/sensor/icm42670/data_stream/src/main.c

samples/sensor/icm42670/apex/src/main.c

.../invn.algo.sw-integration.aml-42670s-gcc-arm-none-eabi-cm4-fpu-1.6.0/include/invn_algo_aml.h

carlescufi · 2024-04-11T18:11:48Z

Thanks for the PR! Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request.

afontaine-invn · 2024-04-15T08:54:32Z

Hi @teburd, @carlescufi, thank you very much for your review! You point many subjects to address, Find my answers below:

Regarding PDF and blobs I'll remove it for now. We though exposing the AML library (featuring pointing, gesture recognition, ...) but it seems not feasible and compatible with Zephyr rules.
Regarding the trigger API requiring a thread or work queue I kept it configurable by KConfig and I kept the option to not configure any trigger. I can put it as default.
Regarding the possibility to use a hal_tdk git repository to put the imu driver directory, we look at how st handles it with @gjabouley-invn and we'll like the idea. Could you give us more details on how to put it in place? And if there is any rules or guidelines to manage it over the time?
And just as you know I'm not the only one to push with a TDK address lately, @rbuisson-invn also updated ICP drivers with official ones. Find the PR driver: sensor: icp101xx: adds driver and sample #69282

decsny · 2024-04-15T15:59:44Z

dts/bindings/sensor/invensense,icm42670-i2c.yaml

any DT binding property with a default value needs justification of the default value in the description https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html

I updated the DT bindings with default description as required.

decsny · 2024-04-18T15:42:08Z

dts/bindings/sensor/invensense,icm42670-common.yaml

+
+  power-mode:
+    type: string
+    default: "low noise"


missing justification

Thanks for the comment. I added the default value justification for this property also.

afontaine-invn · 2024-04-22T09:56:14Z

I opened an issue to create the tdk_hal repository #71561

teburd · 2024-04-22T15:19:56Z

Sensor WG 04/22/2024

Asked the question around pros/cons of using sensor vendor supplied HALs:

Pros:

Potentially more vendor involvement, with more drivers being pushed to the tree

Questions Posed (Potential Cons):

Unclear how much benefit the HAL provides, e.g. abstracting bus reads/writes is already doable within Zephyr today.
Unclear how the maintainer burden increases when using HALs, who maintains this code long term and moves it forward as sensor APIs evolve and change.
Unclear how we move this code to the asynchronous sensor_read API which is the future of sensors to better support more use cases

addressed

afontaine-invn · 2024-04-25T13:22:09Z

Hi @teburd ,
Thanks for the updates on the tdk_hal repository issue.
Are there any blocking points or any changes expected to move forward with the PR?

teburd · 2024-04-25T13:44:46Z

Hi @teburd , Thanks for the updates on the tdk_hal repository issue. Are there any blocking points or any changes expected to move forward with the PR?

No blocking issues from me no, I expect the TSC will likely approve hal_tdk barring any major concerns, and then this can go in with the tdk hal code removed. I'd expect some more progress in a week or so as the TSC meets weekly, and our sensor maintainer (Maureen Helm at Analog) is traveling this week

afontaine-invn · 2024-09-10T14:23:24Z

Hi @nashif , as I understand the tests don't pass because of a missing test in CMakelist in hal_tdk, meaning part of the files are included. zephyrproject-rtos/hal_tdk#1. How can I push this change?

afontaine-invn · 2024-09-12T14:49:16Z

Hi @ubieda , as I understand the tdk_hal module I added breaks the test. I would like to fix it zephyrproject-rtos/hal_tdk#1. Could you help reviewing it to unlock the build in this PR?

afontaine-invn · 2024-09-24T07:15:29Z

please, check Zephyr contribution guidelines. There seem to be PDFs and binary files in this PR.

Hi @gmarull , the PDF and binary files has been removed, could you have a look at this PR now? Thanks

gmarull · 2024-09-25T08:07:58Z

dts/bindings/sensor/invensense,icm42670.yaml

+    enum:
+      - "low noise"
+      - "low power"


no spaces in enums please

I replaced space by -

gmarull · 2024-09-25T08:08:25Z

dts/bindings/sensor/invensense,icm42670.yaml

+  accel-filt-bw:
+    type: int
+    default: 180


Unit is Hz, I added it in the comment.

property still misses proper suffix

I don't get your point. Which suffix is missing for accel-filt-bw?
The unit is in the comment, and it matches the other properties in the file and the other yaml files. Furthermore, I though the property name needs to be close to the field name in the datasheet (here ACCEL_UI_FILT_BW), it will be easier for the user to find out what is for.

gmarull · 2024-09-25T08:08:35Z

dts/bindings/sensor/invensense,icm42670.yaml

+  gyro-filt-bw:
+    type: int
+    default: 180
+    description: |


Unit is Hz, I added it in the comment.

gmarull · 2024-09-25T08:09:54Z

samples/sensor/6dof_motion_drdy/src/main.c

+	if (dev == NULL) {
+		/* No such node, or the node does not have status "okay". */
+		printk("\nError: no device found.\n");
+		return NULL;
+	}


this is impossible, build will fail if macro doesn't resolve

Ok I removed it. I copied this from the bosh,bme280 example, I though it was needed.

gmarull · 2024-09-25T08:10:22Z

samples/sensor/tdk_apex/VERSION

+VERSION_MAJOR = 1
+VERSION_MINOR = 0
+PATCHLEVEL = 0
+VERSION_TWEAK = 0
+EXTRAVERSION =


I removed the VERSION files. I though these VERSION files were recommended, but I misunderstood the guidelines.

gmarull · 2024-09-25T08:10:49Z

samples/sensor/tdk_apex/src/main.c

+	if (dev == NULL) {
+		/* No such node, or the node does not have status "okay". */
+		printk("\nError: no device found.\n");
+		return NULL;
+	}


Ok I removed it.

afontaine-invn · 2024-09-30T12:18:32Z

Hi @gmarull , I did the changes you requested. Could you update the changes requested status if it's ok for you?

afontaine-invn · 2024-10-07T07:26:37Z

Hello @gmarull
I addressed the change as requested; could you update your review status? Is there something else do you expect?

gmarull · 2024-10-07T08:47:55Z

samples/sensor/6dof_motion_drdy/README.rst

+6DOF Motion Dataready:
+############################################


wrong formatting

I updated both README for 6dof_motion_drdy and tdk_apex samples.

gmarull · 2024-10-07T08:48:47Z

samples/sensor/6dof_motion_drdy/boards/nrf52dk_nrf52832_i2c.overlay

+	compatible = "nordic,nrf-uarte";
+	status = "okay";


why is this needed?

You are right, this is not needed. This is already set in nrf52dk DT, I removed this for both i2c and spi overlay.

gmarull · 2024-10-07T08:49:40Z

samples/sensor/tdk_apex/Kconfig

+# Enable APEX features support for the IMU device selected
+config TDK_APEX
+	default y


this is not a defconfig file, please, use prj.conf

Ok, I moved this to the prj.conf file

drivers/sensor/tdk/icm42670/Kconfig

Prepare to use official TDK Invensense Inc. driver for icm42670-P/-S sensor in tdk_hal module. Simplify I2C and SPI transport files. Driver code moves in hal_tdk module. Adds APEX features, such as Pedometer, Tilt detection, Wake on Motion and Significant Motion Detector. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

Add the power mode, accel and gyro filtering options, variant S support, and apex features. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

It reports IMU 6-axis accelerometer and gyroscope data using DRDY interrupt. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

It reports Advanced Pedometer and Event Detection features, such as Pedometer, Tilt detection, Wake on Motion and Significant Motion Detector. Device tree options. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

afontaine-invn · 2024-10-22T07:40:49Z

Thanks for your comments @pdgendt , it was pertinent and I'm happy with the new version with more options in device tree.

https://github.com/tdk-invn-oss/zephyr.hal_tdk With 6dof_motion_drdy and tdk_apex sample, with custom setup: nrf52dk_nrf52832 + icm42670-P/-S EVB board Build ok by running: west twister -T samples\sensor\tdk_apex -T samples\sensor\6dof_motion_drdy -p nrf52dk/nrf52832 Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Samples Samples area: Sensors Sensors labels Apr 11, 2024

zephyrbot requested review from avisconti, decsny, galak, kartben, MaureenHelm, nashif, teburd, tristan-google and yperess April 11, 2024 08:53

zephyrbot assigned MaureenHelm Apr 11, 2024

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from c22caff to e45513b Compare April 11, 2024 12:50

gmarull requested changes Apr 11, 2024

View reviewed changes

teburd reviewed Apr 11, 2024

View reviewed changes

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from e45513b to 30399c8 Compare April 11, 2024 15:02

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from e3c9a40 to 61896e5 Compare April 15, 2024 15:42

decsny previously requested changes Apr 15, 2024

View reviewed changes

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 61896e5 to a65d60c Compare April 18, 2024 15:13

decsny reviewed Apr 18, 2024

View reviewed changes

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from a65d60c to ed01c24 Compare April 22, 2024 09:49

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 9e9bdcd to 8a24001 Compare September 9, 2024 14:30

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 8a24001 to a86f279 Compare September 10, 2024 14:27

rbuisson-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 9380579 to a86f279 Compare September 17, 2024 13:50

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch 3 times, most recently from 7ef6c0f to 4918388 Compare September 23, 2024 13:34

gmarull requested changes Sep 25, 2024

View reviewed changes

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 4918388 to 9a7fa88 Compare September 26, 2024 07:44

afontaine-invn requested a review from gmarull September 27, 2024 08:30

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch 4 times, most recently from 4f822b8 to f4b66d5 Compare October 4, 2024 07:09

gmarull requested changes Oct 7, 2024

View reviewed changes

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch 2 times, most recently from b8fd338 to e036f89 Compare October 7, 2024 14:23

pdgendt reviewed Oct 10, 2024

View reviewed changes

drivers/sensor/tdk/icm42670/Kconfig Outdated Show resolved Hide resolved

drivers/sensor/tdk/icm42670/Kconfig Outdated Show resolved Hide resolved

afontaine-invn added 4 commits October 22, 2024 09:35

dts: variant, apex, power, and filtering options

70b092a

Add the power mode, accel and gyro filtering options, variant S support, and apex features. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

samples: sensor: 6DOF motion DRDY generic sample

0de0933

It reports IMU 6-axis accelerometer and gyroscope data using DRDY interrupt. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

samples: sensor: TDK APEX generic sample

9b55667

It reports Advanced Pedometer and Event Detection features, such as Pedometer, Tilt detection, Wake on Motion and Significant Motion Detector. Device tree options. Signed-off-by: Aurelie Fontaine <aurelie.fontaine@tdk.com>

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from e036f89 to 7312cfe Compare October 22, 2024 07:37

afontaine-invn force-pushed the TDK_Invensense_ICM42670_Support_S branch from 7312cfe to 1d35bd3 Compare October 22, 2024 12:40

		6DOF Motion Dataready:
		############################################

drivers: sensor: icm42670: supports icm42670-P and icm42670-S #71374

Are you sure you want to change the base?

drivers: sensor: icm42670: supports icm42670-P and icm42670-S #71374

Conversation

afontaine-invn commented Apr 11, 2024 • edited Loading

github-actions bot commented Apr 11, 2024

gmarull left a comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

carlescufi commented Apr 11, 2024

afontaine-invn commented Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

afontaine-invn Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afontaine-invn commented Apr 22, 2024 • edited Loading

teburd commented Apr 22, 2024 • edited Loading

afontaine-invn commented Apr 25, 2024

teburd commented Apr 25, 2024 • edited Loading

afontaine-invn commented Sep 10, 2024 • edited Loading

afontaine-invn commented Sep 12, 2024

afontaine-invn commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afontaine-invn Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afontaine-invn commented Sep 30, 2024

afontaine-invn commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afontaine-invn commented Oct 22, 2024 • edited Loading

afontaine-invn commented Apr 11, 2024 •

edited

Loading

afontaine-invn commented Apr 15, 2024 •

edited

Loading

afontaine-invn Apr 18, 2024 •

edited

Loading

afontaine-invn commented Apr 22, 2024 •

edited

Loading

teburd commented Apr 22, 2024 •

edited

Loading

teburd commented Apr 25, 2024 •

edited

Loading

afontaine-invn commented Sep 10, 2024 •

edited

Loading

afontaine-invn commented Sep 24, 2024 •

edited

Loading

afontaine-invn Sep 30, 2024 •

edited

Loading

afontaine-invn commented Oct 22, 2024 •

edited

Loading