ICM45686: Streaming Performance Improvements #94832

ubieda · 2025-08-22T00:37:55Z

Various performance improvements after stress-testing driver: 2X instances, running at 3200-Hz ODR, results at 800-Hz (4 results every 1.25-ms).

sonarqubecloud · 2025-08-22T01:02:29Z

Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ C)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

anthony289478 · 2025-09-19T17:25:40Z

icm45686_prep_reg_read_rtio_async() is mostly implemented via rtio_read_regs_async

It might be better to expand on it and perhapse add an additional API rtio_write_regs_async to fulfill the use case of icm45686_prep_reg_write_rtio_async so other sensors can use them too.

ubieda · 2025-09-19T19:30:52Z

icm45686_prep_reg_read_rtio_async() is mostly implemented via rtio_read_regs_async

It might be better to expand on it and perhapse add an additional API rtio_write_regs_async to fulfill the use case of icm45686_prep_reg_write_rtio_async so other sensors can use them too.

Makes sense. I'll unify with common APIs before marking as ready for review.

drivers/sensor/tdk/icm45686/icm45686_stream.c

anthony289478 · 2025-09-19T21:03:00Z

drivers/sensor/tdk/icm45686/icm45686_stream.c

icm45686: stream: Only enable GPIO interrupts when stream is active
Otherwise a lot of spurious callbacks may trigger shortly after
rebooting.

This looks related to #96246

when FIFO_CONFIG0 is configured to stream mode,

01: Stream mode - Frames are overwritten when the FIFO full condition is reached.

and FIFO_WR_WM_GT_TH is set to 1 in FIFO_CONFIG2 you will get an interrupt every time a frame is written to the fifo if you are over the watermark threshold.
This occurring during init is because the fifo is full and not being emptied or reset yet.

The problem is that when the application boots-up it has not configured the IMU yet and it has no knowledge or control on its state prior to the reset. For that reason, the right choice IMO is to not have the GPIO INT enabled until it matters (e.g: when a stream submission is requested).

Whether that comes from an interrupt due to a greater-than or exactly equals to the FIFO WM seems orthogonal to the problem.

If you disable interrupts and read the fifo but due to system latency you do not re enable before the next equals wm threshold is hit you will never get another interrupt. So having the driver be greater-equals with irq disabled or equals with irq always on are mutually exclusive.

drivers/sensor/tdk/icm45686/icm45686_stream.c

drivers/sensor/tdk/icm45686/icm45686_bus.c

ubieda · 2025-09-24T18:48:28Z

FYI - Picking this back up

ubieda · 2025-09-25T18:28:29Z

icm45686_prep_reg_read_rtio_async() is mostly implemented via rtio_read_regs_async
It might be better to expand on it and perhapse add an additional API rtio_write_regs_async to fulfill the use case of icm45686_prep_reg_write_rtio_async so other sensors can use them too.

Makes sense. I'll unify with common APIs before marking as ready for review.

After looking at the reg-map API, I notice rtio_read_regs_async() does not match the needs for this driver. Since other sensor drivers also follow this same pattern, I'll follow-up with a separate PR to unify them.

Similar pattern applied to other in-tree sensor drivers, where preparing a set of SQEs for writing/reading on registers is a recurrent syntax. Signed-off-by: Luis Ubieda <luisf@croxel.com>

In order to allow the error-handling scheme to take action, instead of crashing when asserts are enabled. Now possible since we have RTIO error handling for streaming mode. Signed-off-by: Luis Ubieda <luisf@croxel.com>

In order to simplify code-logic. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Not handled otherwise. Signed-off-by: Luis Ubieda <luisf@croxel.com>

In order to detect when events are being missed. Signed-off-by: Luis Ubieda <luisf@croxel.com>

As it's not required, so we rather spare the cycles. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Since the driver knows how many samples it wants (because of the watermark threshold), stick to that per event in order to facilitate batches of N number of samples. Signed-off-by: Luis Ubieda <luisf@croxel.com>

By introducing three states: Off -> On -> Busy -> (Offf) This allows us to more clearly guard on-going events and detect overlapping triggers. Signed-off-by: Luis Ubieda <luisf@croxel.com>

To handle streaming under stress scenario. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Otherwise a lot of spurious callbacks may trigger shortly after rebooting. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Process all in one callback, in order to reduce latency. The following changes have been done: - Process FIFO read-out and/or Data-ready in GPIO callback. - FIFO Full is handled on completion if needed. - Omit fifo-count fetching, to optimize cycles. The watermark count is the number of samples we're going for. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Multi-shot request should re-enable it shortly. Signed-off-by: Luis Ubieda <luisf@croxel.com>

ubieda · 2025-09-25T20:58:41Z

@anthony289478 your comments have been addressed. Please revisit

sonarqubecloud · 2025-09-25T21:01:32Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
8.8% Duplication on New Code

See analysis details on SonarQube Cloud

anthony289478 · 2025-09-26T08:39:24Z

@anthony289478 your comments have been addressed. Please revisit

I will be able to test this earliest October 15 as I am on vacation.

ubieda · 2025-09-26T12:03:50Z

@anthony289478 your comments have been addressed. Please revisit

I will be able to test this earliest October 15 as I am on vacation.

Ok - since your NACK was about the p4wq issue which is not present in upstream/zephyr (see #92462). I suggest dismissing it and letting the regular review process kick-in.

anthony289478 · 2025-09-26T14:54:38Z

Ok - since your NACK was about the p4wq issue which is not present in upstream/zephyr (see #92462). I suggest dismissing it and letting the regular review process kick-in.

I do not see an option to dismiss the request for changes. In fact, none of my comments have the "resolve conversation" button.
The only thing I can do is delete my comments.

ubieda · 2025-09-26T14:56:47Z

@anthony289478 PTAL here:

anthony289478 · 2025-09-26T15:04:34Z

@anthony289478 PTAL here

I do not have those ellipses nor those options on my side.

Dismissing NACK since Anthony is not able to do so.

anthony289478 · 2025-10-14T18:45:01Z

@ubieda Am I missing something or is this definition wrong for its usage?
I get the following errors when compiling these patches from main

/home/ubuntu/zephyrprojectrtos/zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c:256:20: error: too few arguments to function 'should_read_data'
  256 |         } else if (should_read_data(read_cfg)) {
      |                    ^~~~~~~~~~~~~~~~
/home/ubuntu/zephyrprojectrtos/zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c:89:20: note: declared here
   89 | static inline bool should_read_data(const struct sensor_read_config *read_cfg,
      |                    ^~~~~~~~~~~~~~~~
/home/ubuntu/zephyrprojectrtos/zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c:294:20: error: too few arguments to function 'should_read_data'
  294 |         } else if (should_read_data(read_cfg)) {
      |                    ^~~~~~~~~~~~~~~~
/home/ubuntu/zephyrprojectrtos/zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c:89:20: note: declared here
   89 | static inline bool should_read_data(const struct sensor_read_config *read_cfg,
      |                    ^~~~~~~~~~~~~~~~

This was correct on the original commit but was reverted on the rebase from d59b736 to f455e0c

https://github.com/zephyrproject-rtos/zephyr/compare/d59b73668528d4d04e3a5af127df68971de73e6d..f455e0c67e934c276f77358864cc2bed1db10e32#diff-a9718e5a998674de0d95c1a5f255169848ee689f88f2b123d47ca258b21fd16bL80

Current main definition:

zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c

Lines 89 to 90 in 91c57f1

    
           static inline bool should_read_data(const struct sensor_read_config *read_cfg, 
        
           				    uint8_t int_status)

Current main usage:

zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c

Line 256 in 91c57f1

} else if (should_read_data(read_cfg)) {

zephyr/drivers/sensor/tdk/icm45686/icm45686_stream.c

Line 294 in 91c57f1

} else if (should_read_data(read_cfg)) {

ubieda · 2025-10-14T19:15:36Z

@anthony289478 you're right - I just revisited the ICM45686 driver and there's a couple fixes in #97564. I tested on MCXN947 and it seems to work OK again.

Thanks for reporting it.

ubieda added the DNM This PR should not be merged (Do Not Merge) label Aug 22, 2025

ubieda changed the title ~~icm45686: Update RTIO bus API to simplify async transfers~~ icm45686: Streaming Performance Improvements Aug 22, 2025

ubieda changed the title ~~icm45686: Streaming Performance Improvements~~ ICM45686: Streaming Performance Improvements Aug 22, 2025

ubieda mentioned this pull request Sep 19, 2025

sensor: icm45686: Change watermark threshold irq to equals #96246

Merged

anthony289478 reviewed Sep 19, 2025

View reviewed changes

drivers/sensor/tdk/icm45686/icm45686_stream.c Outdated Show resolved Hide resolved

anthony289478 reviewed Sep 19, 2025

View reviewed changes

anthony289478 reviewed Sep 22, 2025

View reviewed changes

drivers/sensor/tdk/icm45686/icm45686_stream.c Show resolved Hide resolved

anthony289478 previously requested changes Sep 22, 2025

View reviewed changes

drivers/sensor/tdk/icm45686/icm45686_bus.c Show resolved Hide resolved

ubieda force-pushed the icm45686-performance-optimizations branch 2 times, most recently from 1398df1 to d9e6f10 Compare September 25, 2025 19:29

ubieda added 6 commits September 25, 2025 15:36

icm45686: Update RTIO bus API to simplify async transfers

51b91ce

Similar pattern applied to other in-tree sensor drivers, where preparing a set of SQEs for writing/reading on registers is a recurrent syntax. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: Replace assert for check-if when buffer alloc fails

6180147

In order to allow the error-handling scheme to take action, instead of crashing when asserts are enabled. Now possible since we have RTIO error handling for streaming mode. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Refactor completion actions through helper function

a9d3cdf

In order to simplify code-logic. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Add missing call to drop RTIO Bus queue when full

4a73733

Not handled otherwise. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: Add warning log when event triggered while busy

647e4e6

In order to detect when events are being missed. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Remove memset when completing stream event

691e1ae

As it's not required, so we rather spare the cycles. Signed-off-by: Luis Ubieda <luisf@croxel.com>

ubieda force-pushed the icm45686-performance-optimizations branch 2 times, most recently from 10d5071 to d59b736 Compare September 25, 2025 19:52

ubieda added 6 commits September 25, 2025 16:52

icm45686: stream: Remove run-time variability of samples on FIFO WM

bc4a517

Since the driver knows how many samples it wants (because of the watermark threshold), stick to that per event in order to facilitate batches of N number of samples. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Improve tracking of state transitions

ff39000

By introducing three states: Off -> On -> Busy -> (Offf) This allows us to more clearly guard on-going events and detect overlapping triggers. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: rtio: Expand bus RTIO queue size

868d2c2

To handle streaming under stress scenario. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Only enable GPIO interrupts when stream is active

da25845

Otherwise a lot of spurious callbacks may trigger shortly after rebooting. Signed-off-by: Luis Ubieda <luisf@croxel.com>

icm45686: stream: Add missing INT disable on error completion

f455e0c

Multi-shot request should re-enable it shortly. Signed-off-by: Luis Ubieda <luisf@croxel.com>

ubieda force-pushed the icm45686-performance-optimizations branch from d59b736 to f455e0c Compare September 25, 2025 20:57

ubieda removed the DNM This PR should not be merged (Do Not Merge) label Sep 25, 2025

ubieda marked this pull request as ready for review September 25, 2025 21:44

zephyrbot added the area: Sensors Sensors label Sep 25, 2025

zephyrbot requested review from MaureenHelm, afontaine-invn, asemjonovs, avisconti, gjabouley-invn, jeppenodgaard, rbuisson-invn, sriccardi-invn, teburd, tristan-google and yperess September 25, 2025 21:46

zephyrbot assigned MaureenHelm Sep 25, 2025

teburd approved these changes Sep 26, 2025

View reviewed changes

MaureenHelm approved these changes Sep 29, 2025

View reviewed changes

kartben merged commit 91c57f1 into zephyrproject-rtos:main Sep 30, 2025
29 of 30 checks passed

ubieda mentioned this pull request Oct 14, 2025

ICM45686: Address issues coming from a sloppy rebase on #94832 #97564

Merged

ubieda deleted the icm45686-performance-optimizations branch October 15, 2025 18:11

ICM45686: Streaming Performance Improvements #94832

ICM45686: Streaming Performance Improvements #94832

Uh oh!

Conversation

ubieda commented Aug 22, 2025

Uh oh!

sonarqubecloud bot commented Aug 22, 2025

Quality Gate failed

Uh oh!

anthony289478 commented Sep 19, 2025

Uh oh!

ubieda commented Sep 19, 2025

Uh oh!

Uh oh!

anthony289478 Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

ubieda Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

anthony289478 Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ubieda commented Sep 24, 2025

Uh oh!

ubieda commented Sep 25, 2025

Uh oh!

ubieda commented Sep 25, 2025

Uh oh!

sonarqubecloud bot commented Sep 25, 2025

Quality Gate passed

Uh oh!

anthony289478 commented Sep 26, 2025

Uh oh!

ubieda commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anthony289478 commented Sep 26, 2025

Uh oh!

ubieda commented Sep 26, 2025

Uh oh!

anthony289478 commented Sep 26, 2025

Uh oh!

Uh oh!

anthony289478 commented Oct 14, 2025

Uh oh!

ubieda commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ubieda commented Sep 26, 2025 •

edited

Loading