Skip to content

Conversation

@ubieda
Copy link
Member

@ubieda ubieda commented Feb 18, 2025

Description

This PR introduces support for TDK's ICM45686 IMU sensor, including:

  • Fetch/Get API.
  • Read/Decode API.
  • Configuration through device-tree properties.
  • Bus support for SPI (although easily extensible to others as
    based on RTIO).
  • Trigger for SENSOR_TRIG_DATA_READY.

Testing

  • Fetch/Get API tested with accel_polling sample.
  • Read/Decode API tested with sensor_shell sample.
  • Trigger with sensor_shell sample.

@ubieda ubieda force-pushed the icm45686-basic-functionality branch 3 times, most recently from c77bbcd to 72bed35 Compare February 19, 2025 00:37
@ubieda ubieda added this to the v4.2.0 milestone Feb 19, 2025
@ubieda ubieda marked this pull request as ready for review February 19, 2025 04:04
@zephyrbot zephyrbot added area: Sensors Sensors platform: ADI Analog Devices, Inc. area: SPI SPI bus labels Feb 19, 2025
@ubieda ubieda added the DNM This PR should not be merged (Do Not Merge) label Feb 19, 2025
@ubieda
Copy link
Member Author

ubieda commented Feb 19, 2025

Marked as DNM until in-flight PRs this depends upon are merged.

@ubieda ubieda force-pushed the icm45686-basic-functionality branch 2 times, most recently from 55f1b5c to 9e790cf Compare February 22, 2025 03:26
@ubieda ubieda force-pushed the icm45686-basic-functionality branch 3 times, most recently from 1c34f00 to f540f40 Compare February 25, 2025 00:29
ubieda added 3 commits March 10, 2025 14:25
Both for Gyro and Accelerometer.

Signed-off-by: Luis Ubieda <[email protected]>
Only working with SENSOR_TRIG_DATA_READY so far.

Signed-off-by: Luis Ubieda <[email protected]>
Only supported for SPI so far.

Signed-off-by: Luis Ubieda <[email protected]>
@ubieda ubieda dismissed stale reviews from teburd, PetervdPerk-NXP, and bperseghetti via 90c12b5 March 10, 2025 18:30
@ubieda ubieda force-pushed the icm45686-basic-functionality branch from 04b9820 to 90c12b5 Compare March 10, 2025 18:30
@ubieda ubieda requested a review from teburd March 10, 2025 18:30
@kartben kartben merged commit 052c76b into zephyrproject-rtos:main Mar 11, 2025
21 checks passed
@afontaine-invn
Copy link
Contributor

Hi @MaureenHelm , Hi @ubieda , Hi @kartben ,
You asked from TDK that we put in place a HAL to support our sensors. We are currently working at supporting this ICM-45686 device. I would appreciate to be notified by such PR. This driver does not support our drivers from the hal. I don't get it, it's not inline with what you asked from us. Why do you merge such drivers?

FYI @sriccardi-invn @JHKim8212

@ubieda
Copy link
Member Author

ubieda commented Mar 14, 2025

Hi @afontaine-invn,

I'm adding some comments here:

  • I've developed and submitted this driver as a community contribution, given I had a need for it to work with the Streaming API (Sensor Model v2).
  • WRT the conflict of stepping on each others' toes: Being completely transparent, I was not aware you were working on it. Please note this PR was open about 3-weeks from now; but even so, I still don't see how I or the other Sensor collaborators/maintainers would have known so, given I still don't see an upstream PR filed to support this sensor functionality.
  • As far as not being on the loop when the PR was submitted:
    1. The contribution followed its natural process. The automatic reviewers are Collaborators and Maintainers of the different areas. If you want to be notified on every PR filed for any TDK driver sensor driver, I encourage you to follow the process and nominate yourselves as collaborators/maintainers for your sensors.
    2. This driver was brought up on at least a Sensor WG meeting, where we discussed details on Triggers vs Streaming. I encourage you to join us these meetings as well to stay in the loop of what's going on with the WG and how we could collaborate each other.
  • WRT Why merging such drivers, I really don't know what to say other than the Zephyr Project being open-source: individuals contribute source code and it follows the same process as companies, members and organization contributing code to support their products. I don't see what is wrong with that.
  • As far as TDK being able to support their sensors: I personally have no preference of this driver using or not using a HAL, as long as it still supports my use-case: Streaming mode with Watermark interrupts on native RTIO support (just like the already existing ICM42688). Feel free to submit a PR with your tweaks and it will go through the proper review process.

@kartben @MaureenHelm please chime in if there's anything else to add or if you think otherwise.

@teburd
Copy link
Contributor

teburd commented Mar 14, 2025

I'd welcome you to take maintainership of the TDK drivers, I only maintain them today because no one else has volunteered to do so.

@ubieda ubieda deleted the icm45686-basic-functionality branch March 14, 2025 15:44
@kartben
Copy link
Contributor

kartben commented Mar 14, 2025

Great summary, @ubieda, thank you! The obvious thing that comes to mind is indeed that maybe TDK wants to step up to become maintainers of their sensors?
This will help ensure you folks are in the loop, and will make you more directly responsible for handling community contributions (which may or may not use your HAL!)
Putting thumbs down emojis when/if more of such contributions come will likely not be the best way to move forward though, as I'm sure you can imagine :) (cc @gjabouley-invn)

@afontaine-invn
Copy link
Contributor

Thanks for your reply. For sure we would like to be maintainers of TDK drivers. What is the process to being so?

@teburd
Copy link
Contributor

teburd commented Mar 14, 2025

Thanks for your reply. For sure we would like to be maintainers of TDK drivers. What is the process to being so?

Send a PR with your desired maintainer/collaborator status changes to the MAINTAINERS file in this tree.

See for example:
https://github.com/zephyrproject-rtos/zephyr/blob/main/MAINTAINERS.yml#L4445

And an example PR:
#85528

I'll reiterate a few points here..

Please do refrain from leaving thumbs down emojis. This does not help anyone.

Please do join the working group and the bi-weekly meeting, we're a friendly bunch I promise. I like that you and TDK are contributing to Zephyr. I very much doubt anyone views this as a negative. That said being involved in driver reviews and working group discussions is important if you want to have a good understanding and a voice in the direction sensors in Zephyr are going.

Speaking of reviews, if you add your name to the collaborators list here you will see all sensor driver pull requests, and quality reviews are very much appreciated https://github.com/zephyrproject-rtos/zephyr/blob/main/MAINTAINERS.yml#L2047

@gjabouley-invn
Copy link
Contributor

gjabouley-invn commented Mar 14, 2025

Thanks all for clarifying this point
we though initially that during the hal_tdk creation story we had expressed our intention to step-up to maintain/extend TDK sensors support in Zephyr ecosystem (#71374 (comment))
And seeing such PR (and some others) related to recent TDK sensors merged without being informed created some misunderstanding as it is also being finalized internally...
But turns out we are not yet properly identified as maintainers for TDK sensors, only for hal_tdk.
As recommended, we will create the PR to solved this gap and ensure we can provide quality feedbacks to community, also avoiding spread of alternate implementation of our drivers when it can be addressed as part of the HAL.

note i also removed my initial thumbs down reaction, and thumbs up (I definitely prefer to use this one btw) your comment @teburd

afontaine-invn added a commit to InvenSenseInc/fork.zephyr-rtos that referenced this pull request Mar 17, 2025
We would like to take maintainership of TDK drivers as discussed:
 zephyrproject-rtos#85963

Signed-off-by: Aurélie Fontaine <[email protected]>
@afontaine-invn
Copy link
Contributor

Thanks for your explanation @teburd ,

I opened #87191.
And we added the sensor WG meeting in our agenda.

Regarding your proposition, we rather start with only our sensor maintainership and see what it is implied before committed in the full sensor review.

afontaine-invn added a commit to InvenSenseInc/fork.zephyr-rtos that referenced this pull request Mar 17, 2025
We would like to take maintainership of TDK drivers as discussed:
 zephyrproject-rtos#85963

Signed-off-by: Aurelie Fontaine <[email protected]>
afontaine-invn added a commit to InvenSenseInc/fork.zephyr-rtos that referenced this pull request Apr 4, 2025
We would like to take maintainership of TDK drivers as discussed:
 zephyrproject-rtos#85963
teburd and MaureenHelm moves to collaborators

Signed-off-by: Aurelie Fontaine <[email protected]>
kartben pushed a commit that referenced this pull request Apr 4, 2025
We would like to take maintainership of TDK drivers as discussed:
 #85963
teburd and MaureenHelm moves to collaborators

Signed-off-by: Aurelie Fontaine <[email protected]>
abrarnasirjaffari pushed a commit to abrarnasirjaffari/zephyr that referenced this pull request Apr 9, 2025
We would like to take maintainership of TDK drivers as discussed:
 zephyrproject-rtos#85963
teburd and MaureenHelm moves to collaborators

Signed-off-by: Aurelie Fontaine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants