sensor: icm45686: Change watermark threshold irq to equals

[anthony289478]

When using the default configuration of watermark threshold interrupt greater than or equals, extra interrupts are serviced to icm45686_event_handler(). The expected behavior should be only one interrupt is generated per watermark threshold crossing, and until the host drains the fifo, no extra interrupts should be generated.

IMU dt fragment used,

&icm45686 {
	status = "okay";
	accel-odr = <ICM45686_DT_ACCEL_ODR_1600>;
	accel-fs = <ICM45686_DT_ACCEL_FS_16>;
	accel-pwr-mode = <ICM45686_DT_ACCEL_LN>;
	gyro-odr = <ICM45686_DT_GYRO_ODR_1600>;
	gyro-fs = <ICM45686_DT_GYRO_FS_2000>;
	gyro-pwr-mode = <ICM45686_DT_GYRO_LN>;
	fifo-watermark = <64>;
};

This is the observed current behavior of the interrupt pin with the default setting, per datasheet.

1: Write watermark interrupt generated when FIFO data count is greater
than or equal to FIFO watermark threshold

This is the proposed expected behavior of the interrupt pin with the new setting, per datasheet.

0: Write watermark interrupt generated when FIFO data count is equal to
the FIFO watermark threshold

[ubieda]

This change would mute warnings thrown due to system latency on servicing the FIFO Watermark events, wouldn't it?

[anthony289478]

This change would mute warnings thrown due to system latency on servicing the FIFO Watermark events, wouldn't it?

Could you explain a bit more? From what I see, the rtio submission occurs in gpio interrupt context so it is always serviced.
The main issue for me is when there is system latency and the rtio workq does not immediately drain the fifo extra interrupts are generated which allocate more rtio transactions from the context mempool.

[ubieda]

Right - so there's no good reason to have the Workqueue used for every submission. I have this in-flight PR which removes it from the sensor driver. In this case, subsequent FIFO_WM triggers are ignored at the handler level although logged as LOG_WRN. Would you mind looking over? Perhaps it will address your needs: #94832

[anthony289478]

Right - so there's no good reason to have the Workqueue used for every submission.

You still do since icm45686_prep_reg_read_rtio_async() and others still creates async queue's and rtio_submit is called in icm45686_event_handler()
sensor rtio submissions by default are processed here via the rtio_workq

In this case, subsequent FIFO_WM triggers are ignored at the handler level although logged as LOG_WRN.

I see you've added disabling interrupts in icm45686_event_handler but to me that is a software fix for improperly configured hardware.
Could you elaborate based on that PR, without disabling interrupts and configuring wm on equals only, how the system would drop interrupts due to system latency?

From what I understand, once triggered by hardware, a vectored interrupt, even when preempted by higher priority interrupts, will always be serviced by the system eventually so icm45686_gpio_callback() and thus icm45686_event_handler() is guaranteed to be called on every edge when interrupts are enabled.

[ubieda]

Right - so there's no good reason to have the Workqueue used for every submission.

You still do since icm45686_prep_reg_read_rtio_async() and others still creates async queue's and rtio_submit is called in icm45686_event_handler() sensor rtio submissions by default are processed here via the rtio_workq

The sensor_submit_fallback only kicks-in if the sensor driver doesn't implement the .submit API, which in this case it does:

zephyr/drivers/sensor/default_rtio_sensor.c

Lines 32 to 38 in d494f86

	if (api->submit != NULL) {
	api->submit(dev, iodev_sqe);
	} else if (!cfg->is_streaming) {
	sensor_submit_fallback(dev, iodev_sqe);
	} else {
	rtio_iodev_sqe_err(iodev_sqe, -ENOTSUP);
	}

Could you elaborate based on that PR, without disabling interrupts and configuring wm on equals only, how the system would drop interrupts due to system latency?

Take a look at:
https://github.com/CogniPilot/zephyr/blob/a03c53eea78e4bb7c050177a938d214c2f6273d0/drivers/sensor/tdk/icm45686/icm45686_stream.c#L205-L208

[anthony289478]

The sensor_submit_fallback only kicks-in if the sensor driver doesn't implement the .submit API, which in this case it does:

I see this is implemented by the icm45686 as icm45686_stream_submit when streaming is enabled and icm45686_submit_one_shot if not enabled. In both cases it looks like rtio_submit is still called after reg read/write operations are queued and the process repeats. Eventually the jobs will need to go to the rtio_workq to be worked.

For this implementation, it will go to spi_iodev_submit(),

zephyr/include/zephyr/drivers/spi.h

Lines 1272 to 1279 in d494f86

	static inline void spi_iodev_submit(struct rtio_iodev_sqe *iodev_sqe)
	{
	const struct spi_dt_spec *dt_spec = (const struct spi_dt_spec *)iodev_sqe->sqe.iodev->data;
	const struct device *dev = dt_spec->bus;
	const struct spi_driver_api *api = (const struct spi_driver_api *)dev->api;
	api->iodev_submit(dt_spec->bus, iodev_sqe);
	}

From there, the vendor implementation. In most cases, the default implementation is used,

#ifdef CONFIG_SPI_RTIO
	.iodev_submit = spi_rtio_iodev_default_submit,
#endif

which ends up at the rtio_workq

zephyr/drivers/spi/spi_rtio.c

Lines 137 to 153 in d494f86

	void spi_rtio_iodev_default_submit(const struct device *dev,
	struct rtio_iodev_sqe *iodev_sqe)
	{
	LOG_DBG("Executing fallback for dev: %p, sqe: %p", (void *)dev, (void *)iodev_sqe);
	struct rtio_work_req *req = rtio_work_req_alloc();
	if (req == NULL) {
	LOG_ERR("RTIO work item allocation failed. Consider to increase "
	"CONFIG_RTIO_WORKQ_POOL_ITEMS.");
	rtio_iodev_sqe_err(iodev_sqe, -ENOMEM);
	return;
	}
	rtio_work_req_submit(req, iodev_sqe, spi_rtio_iodev_default_submit_sync);
	}

Take a look at:
https://github.com/CogniPilot/zephyr/blob/a03c53eea78e4bb7c050177a938d214c2f6273d0/drivers/sensor/tdk/icm45686/icm45686_stream.c#L205-L208

It looks like that warning might exist to suppress the extra interrupts from watermark because the default configuration is set greater than or equals rather than just equals.

I would say the warning should stay as a sanity check but the configuration should still be updated to only generate one interrupt per watermark.

Other TDK IMU's which have the same interrupt behavior have watermark interrupt only once per threshold crossing so it is not an unusual request.

In the case of icm42688 wm_gt_th is disabled, and interrupts are not disabled in gpio callback because it is not needed.

zephyr/drivers/sensor/tdk/icm4268x/icm4268x_common.c

Lines 315 to 325 in 216fc8f

	uint8_t fifo_cfg1 =
	FIELD_PREP(BIT_FIFO_TEMP_EN, 1) |
	FIELD_PREP(BIT_FIFO_GYRO_EN, 1) |
	FIELD_PREP(BIT_FIFO_ACCEL_EN, 1) |
	FIELD_PREP(BIT_FIFO_TMST_FSYNC_EN, 1) |
	FIELD_PREP(BIT_FIFO_HIRES_EN, cfg->fifo_hires);
	LOG_DBG("HIRES MODE ENABLED?: %d", cfg->fifo_hires);
	LOG_DBG("FIFO_CONFIG1 (0x%x) 0x%x", REG_FIFO_CONFIG1, fifo_cfg1);
	res = icm4268x_spi_single_write(&dev_cfg->spi, REG_FIFO_CONFIG1, fifo_cfg1);

zephyr/drivers/sensor/tdk/icm4268x/icm4268x_trigger.c

Lines 21 to 37 in 216fc8f

	static void icm4268x_gpio_callback(const struct device *dev, struct gpio_callback *cb,
	uint32_t pins)
	{
	struct icm4268x_dev_data *data = CONTAINER_OF(cb, struct icm4268x_dev_data, gpio_cb);
	ARG_UNUSED(dev);
	ARG_UNUSED(pins);
	#if defined(CONFIG_ICM4268X_TRIGGER_OWN_THREAD)
	k_sem_give(&data->gpio_sem);
	#elif defined(CONFIG_ICM4268X_TRIGGER_GLOBAL_THREAD)
	k_work_submit(&data->work);
	#endif
	if (IS_ENABLED(CONFIG_ICM4268X_STREAM)) {
	icm4268x_fifo_event(data->dev);
	}
	}

on older drivers, such as icm42605, wm_gt_th is misconfigured as enabled and to suppress spurious interrupts, gpio interrupts are disabled in gpio callback.

zephyr/drivers/sensor/tdk/icm42605/icm42605_setup.c

Line 240 in 216fc8f

uint8_t fifo_en = BIT_FIFO_ACCEL_EN | BIT_FIFO_GYRO_EN | BIT_FIFO_WM_TH;

zephyr/drivers/sensor/tdk/icm42605/icm42605_trigger.c

Line 69 in 216fc8f

gpio_pin_interrupt_configure_dt(&cfg->gpio_int, GPIO_INT_DISABLE);

[anthony289478]

Closed because this change will conflict fundamentally with #94832 interrupt handling

[anthony289478]

I am revisiting this with the latest changes from #94832 and #97564

During periods of system latency am getting console log messages of spurious interrupts being triggered (and ignored) from the watermark >= threshold with high data rates.

[00:02:12.482,574] <wrn> ICM45686_STREAM: Event handler triggered while a stream is in progress! Ignoring
[00:02:12.522,338] <wrn> ICM45686_STREAM: Event handler triggered while a stream is in progress! Ignoring
[00:02:12.562,133] <wrn> ICM45686_STREAM: Event handler triggered while a stream is in progress! Ignoring
[00:02:12.601,898] <wrn> ICM45686_STREAM: Event handler triggered while a stream is in progress! Ignoring

With the current main branch driver, the IMU is being reset on init to a known state, interrupts are being enabled on stream enable, and only disabled on stream result error or callback trigger without stream submission.
I believe it would be a good time to re-visit this updated PR as a possible solution to suppress the interrupt spam at a hardware level.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud