HAL module request: hal_tdk

[afontaine-invn]

Origin

The new repository will contain TDK sensor drivers, which is currently available for registered user only. As example, find icm42670p driver here: https://invensense.tdk.com/products/motion-tracking/6-axis/icm-42670-p/#documentation
This is part of our current effort to put in place a public git repository.

Purpose

Support TDK sensor drivers.

Mode of integration

The module should be integrated as an external HAL module. The proposed name is hal_tdk.

Maintainership

tdk_hal will be maintained by:

• maintainer: @afontaine-invn
• maintainer: @rbuisson-invn
• collaborator: @gjabouley-invn

Pull Request

#71374

Description

This provides TDK official sensor drivers. Initially for IMUs.
As first step, we proposed this tree layout:

├── CMakeLists.txt
├── icm42x7x/  
│   └── imu/
│       └── inv_imu_driver.c , ...
│   └── icm42670p_h/
│       └── imu/inv_imu.h
│   └── icm42670s_h/
│       └── imu/inv_imu.h
├── common/
│   └── Invn/
│       └── InvError.h
└── zephyr/
    └── module.yml

Other chip reference could be added; icm42671s, icm42671p, icm42370
And in next step we could update existing icm drivers to use this hal

The demo repository is available:
https://github.com/tdk-invn-oss/zephyr.hal_tdk

Dependencies

None

Revision

7f0f547109b8db79722ee2f8c2854577ccb610af
Initial push with separate PR.

License

BSD-3-Clause

[github-actions]

Hi @afontaine-invn! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[teburd]

From my perspective I think this could be really nice. I do want to caution though that Zephyr is moving towards an asynchronous bus API, and asynchronous sensor API using a completion queue API.

There are many reasons for this, but maybe some of the large ones are... memory management should be done by the application not the driver, manipulating the sensor readings should be optional, and ideally don't want to have threads dealing with gpio interrupts. It adds latency, it adds memory cost, it prevents potential hardware optimizations. @PetervdPerk-NXP noted at EOSS2024 that simply going to that gpio ISR is time consuming when instead a DMA could be preprogrammed to react and start a SPI/I2C transfer. We want Zephyr to be as optimal of a solution possible for sensing while still being generalized. The newer API gets us, I think, a lot closer.

So I guess my question for this and actually stmemsc for @avisconti as well is... can we still take advantage of the code in these sensor libraries here with the async bus APIs, desired power management functionality (saving state on power mode swaps potentially), and more?

I think providing register maps is great. Providing utilities around converting configuration values is maybe nice. But the bus API abstraction that is in these libraries is usually making some assumptions that I don't know work well with the goals Zephyr has for sensing and that should be talked about.

[afontaine-invn]

Hi @teburd ,
Thank you for your inputs. But I'm a bit confuse with them, could you clarify some items below:

1. Memory management
   You mentioned "memory management should be done by the application not the driver", do you mean our driver memory usage is the issue?
2. DMA vs Thread
   You mentioned "don't want to have threads dealing with gpio interrupts", this point can be fixed. This is not part of our driver package. We based our implementation on other sensor drivers in Zephyr (Bosh & ST).
3. IT
   Would you remove any interrupt handling with the async API? There are used to wake up the system and needed for IoT product and for low power optimization. Or would you that the interrupt simply triggering some DMA transfer?
4. HAL
   You mentioned "bus API abstraction [...]don't [...] work well", do you mean the issue is the SPI/I2C bus abstraction layer in our driver? Would you prefer to only keep this abstraction handled by the overlays?

I opened this issue to create the tdk_hal repository to match how competitor's sensor driver are supported in Zephyr. Indeed, I based the driver implementation on competitor drivers (Bosh & ST). Most of them have the same implementation regarding interrupt, thread, etc ...

For sure, I'm available to talk about this if we want to schedule a meeting.

[teburd]

Hi @teburd , Thank you for your inputs. But I'm a bit confuse with them, could you clarify some items below:

1. Memory management
   You mentioned "memory management should be done by the application not the driver", do you mean our driver memory usage is the issue?

This has more to do with the existing sensor_fetch + sensor_get API pair, where the model was to have the driver read the sensor, then provide individual channel readings. The memory to hold those readings was kept in the sensor driver instance.

The newer API expects to be given a buffer that the driver can dump the sensor encodeded (perhaps the entire FIFO worth) of readings into.

E.g. https://github.com/zephyrproject-rtos/zephyr/pull/71160/files#diff-6e8a0a6aba3e176f4b97af47b854b803193c02676ae42ffd8559554fcee25b1aR1020

From there you can convert vectors of the data into fixed point values that are more amenable to things like cmsis dsp filters.

2. DMA vs Thread
   You mentioned "don't want to have threads dealing with gpio interrupts", this point can be fixed. This is not part of our driver package. We based our implementation on other sensor drivers in Zephyr (Bosh & ST).

I get that, but I want to ensure whatever HAL code we pull in doesn't try and dictate how interrupts are dealt with. For example from the application API with sensors, the newer API does not provide any callback mechanisms. This means, potentially, the hardware could be programmed to trigger DMA directly from a gpio interrupt entirely avoiding the issue of ISR latencies.

3. IT
   Would you remove any interrupt handling with the async API? There are used to wake up the system and needed for IoT product and for low power optimization. Or would you that the interrupt simply triggering some DMA transfer?

No interrupts would be supported, but the way interrupts are reported back to the application changes from callbacks to queued events. We could absolutely reconsider this, but I'd note that callbacks as a whole in Zephyr are a bit complicated.

4. HAL
   You mentioned "bus API abstraction [...]don't [...] work well", do you mean the issue is the SPI/I2C bus abstraction layer in our driver? Would you prefer to only keep this abstraction handled by the overlays?

I'd prefer if we didn't do this in a sensor HAL, it forces a specific flow of bus usages. Today the way sensors handle triggers is by starting a work queue task or waking up a thread dedicated to the sensor to handle it. As noted in our docs they have tradeoffs between the options, one has a latency implication, the other has a memory implication. Both actually are less than ideal, when really all we wanted to do 90% of the time or more is start an I2C or SPI read.

The newer API uses an asynchronous operation queue. The driver is expected, in the ISR, request a bus read with an attached callback. On completion, report back to the sensor driver caller than their read is done with a completion queue item being added. No threads needed, no context switches needed, etc.

I opened this issue to create the tdk_hal repository to match how competitor's sensor driver are supported in Zephyr. Indeed, I based the driver implementation on competitor drivers (Bosh & ST). Most of them have the same implementation regarding interrupt, thread, etc ...

For sure, I'm available to talk about this if we want to schedule a meeting.

I think that might help, perhaps you'd be interested in joining the next sensor WG meeting to discuss? https://github.com/zephyrproject-rtos/zephyr/wiki/Sensors-Working-Group

If you haven't looked at the section on async sensors in the docs that would be a good place to start. I won't block this HAL or the driver by any means, but I do want to make sure there's some critical thinking and consideration for where we want to go taken into account. https://docs.zephyrproject.org/latest/hardware/peripherals/sensor.html#async-read

The first driver we did for this newer API was in fact... the ICM42688p on using the TDK Robokit1 to develop it!
#52911
#60063
(likely other PRs)

[afontaine-invn]

Hi @nashif, Hi @teburd, how should we move forward? I expected a "go" from the community, but should I create a "test" hal_tdk repository first and update the related PR?

[afontaine-invn]

Hi @nashif , hi @ubieda ,
I tried to connect just few minutes after the TSC meeting begins, and now I can't join as guest. Do you have any chance to change these parameter if you have any question regarding this issue? I really expected we could move on with this module hal_tdk.

[ubieda]

Hi @nashif , hi @ubieda , I tried to connect just few minutes after the TSC meeting begins, and now I can't join as guest. Do you have any chance to change these parameter if you have any question regarding this issue? I really expected we could move on with this module hal_tdk.

Hi @afontaine-invn, today's meeting was cancelled due to the US holiday. I suggest joining next week to go over this and move forward with the process.

[afontaine-invn]

Hi @nashif , Hi @MaureenHelm ,
Thanks for pointing me the license issue on last TSC meeting, I updated the files with the required SDPX-identifier and checked the compatibility with BSD-3 license.
Could we move forward with this issue? Or should we wait to the next TSC meeting?

[nashif]

@afontaine-invn why do you still have the original license in the code?

The text below copyright looks like ISC license....

/* SPDX-License-Identifier: BSD 3-Clause */
/*
 *
 * Copyright (c) [2018] by InvenSense, Inc.
 * 
 * Permission to use, copy, modify, and/or distribute this software for any
 * purpose with or without fee is hereby granted.
 * 
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
 * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
 * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
 * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 *
 */

In general it should be enough to have:

/* Copyright (c) [2018] by InvenSense, Inc.
 * SPDX-License-Identifier: BSD 3-Clause
 */

[afontaine-invn]

The text below the SPDX Identifier is validated and provided by our legals to provide open-source code. I don't understand why it's not compliant. The hal_nordic https://github.com/zephyrproject-rtos/hal_nordic for example has the same licence header in the files.
But if it's rejected by Zephyr community, I'll update the files as proposed.

[MaureenHelm]

The SPDX Identifier on the first line indicates a BSD 3-Clause license, but full text of the license on the following lines aren't actually a BSD 3-Clause license. If you want to include both the SPDX identifier and the full text of the license, they need to be consistent.

[afontaine-invn]

Hi @nashif , Hi @MaureenHelm ,
Thanks for following up on this topic, I updated the license header as you suggest, without any text below the SPDX identifier.
And I'll loop back with our legals regarding this topic.
Is it ok to move forward with the TDK HAL repo? Or are there any other remarks to address?

[afontaine-invn]

Hi @nashif , I'm glad the motion is passed on last TSC meeting. What's the next step now?

[ubieda]

Hi @nashif , I'm glad the motion is passed on last TSC meeting. What's the next step now?

@stephanosio May you help on creating the module repo in @zephyrproject-rtos? Thanks!

[afontaine-invn]

Hi @stephanosio , sorry to ping you directly, could we move forward to create the repository? Or do you have an ETA for this task? Thanks

[afontaine-invn]

Hi @ubieda , Hi @nashif ,
Could you help me to move forward with this issue? It has been approved, how to create the TDK HAL repository now?

[afontaine-invn]

Hi @henrikbrixandersen, Hi @carlescufi
You made the motion passes on the 10th July TSC meeting for this issue and thanks a lot. What's the next step now? How to create the hal_tdk module as planned?

[nashif]

@afontaine-invn repo is created: https://github.com/zephyrproject-rtos/hal_tdk please submit a PR to add it to the manifest and other files needed for it to be used.

[nashif]

also add an entry to the MAINTAINER file

[afontaine-invn]

Thank you @nashif ,
I updated the PR #71374 linked to this issue.

[afontaine-invn]

Hi @nashif , we have a PR opened in the tdk hal repository to fix the integration in the zephyr-rtos project. zephyrproject-rtos/hal_tdk#1
No one have the write access to merge this in the repository. No reviewers are notified. How can we configure this? Should the maintainers have the write access? How can we move forward?