From 8a55180a39125f93e56f20fa8dcab748326f2909 Mon Sep 17 00:00:00 2001 From: Felipe Moura Date: Sat, 1 Aug 2026 12:36:49 -0300 Subject: [PATCH 1/3] drivers/sensors/l3gd20: always deliver samples with push_event The driver had two modes selected by CONFIG_SENSORS_L3GD20_BUFFER_SIZE: with a buffer it pushed samples from a work queue, and without one it exposed fetch() while still using the data ready interrupt to signal readiness through notify_event. That second mode misuses the fetch interface. fetch() means the data is read from the device on demand and is therefore always available, while an interrupt driven sensor is exactly what push_event is for. Mixing the two forces the upper half to guess whether a fetch() only lower half will ever notify, and it makes poll() unusable in a multi descriptor loop, because the descriptor reports ready while the read still has to wait for the next interrupt. Drop the fetch path and always use the work queue and push_event, which is what the driver already did by default since BUFFER_SIZE defaults to 1. CONFIG_SENSORS_L3GD20_BUFFER_SIZE gains a range of 1 to 32, as a zero sized buffer no longer has a meaning, and SCHED_HPWORK is now selected unconditionally because the work queue is always used. No in tree configuration enables this driver and the previous default already took the push path, so no defconfig changes are needed. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Felipe Moura --- drivers/sensors/Kconfig | 8 +++--- drivers/sensors/l3gd20_uorb.c | 49 ----------------------------------- 2 files changed, 5 insertions(+), 52 deletions(-) diff --git a/drivers/sensors/Kconfig b/drivers/sensors/Kconfig index 6de33906550c8..c963c07596f07 100644 --- a/drivers/sensors/Kconfig +++ b/drivers/sensors/Kconfig @@ -861,17 +861,19 @@ config SENSORS_L3GD20 bool "STMicro L3GD20 Gyroscope Sensor support" default n select SPI - select SCHED_HPWORK if SENSORS_L3GD20_BUFFER_SIZE > 0 + select SCHED_HPWORK ---help--- Enable driver support for the STMicro L3GD20 gyroscope sensor. config SENSORS_L3GD20_BUFFER_SIZE int "size of buffer" default 1 + range 1 32 depends on SENSORS_L3GD20 ---help--- - The size of the circular buffer used. If the value equal to zero, - indicates that the circular buffer is disabled. + The number of events that the circular buffer can hold. The data + ready interrupt pushes each sample into it, so at least one event + is required. config SENSOR_KXTJ9 bool "Kionix KXTJ9 Accelerometer support" diff --git a/drivers/sensors/l3gd20_uorb.c b/drivers/sensors/l3gd20_uorb.c index 26d3f5b5070fd..c27c348a36233 100644 --- a/drivers/sensors/l3gd20_uorb.c +++ b/drivers/sensors/l3gd20_uorb.c @@ -69,12 +69,10 @@ struct l3gd20_dev_s * L3GD20 sensor */ uint64_t timestamp; /* Units is microseconds */ struct sensor_lowerhalf_s lower; /* The struct of lower half driver */ -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 struct work_s work; /* The work queue is responsible for * retrieving the data from the sensor * after the arrival of new data was * signalled in an interrupt */ -#endif }; /**************************************************************************** @@ -100,13 +98,7 @@ static int l3gd20_interrupt_handler(int irq, FAR void *context, FAR void *arg); static int l3gd20_activate(FAR struct sensor_lowerhalf_s *lower, FAR struct file *filep, bool enable); -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 static void l3gd20_worker(FAR void *arg); -#else -static int l3gd20_fetch(FAR struct sensor_lowerhalf_s *lower, - FAR struct file *filep, - FAR char *buffer, size_t buflen); -#endif /**************************************************************************** * Private Data @@ -119,11 +111,7 @@ static const struct sensor_ops_s g_l2gd20_ops = .activate = l3gd20_activate, .set_interval = NULL, .batch = NULL, -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 .fetch = NULL, -#else - .fetch = l3gd20_fetch, -#endif .control = NULL }; @@ -359,7 +347,6 @@ static int l3gd20_interrupt_handler(int irq, FAR void *context, priv->timestamp = sensor_get_timestamp(); -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 /* Task the worker with retrieving the latest sensor data. We should not do * this in a interrupt since it might take too long. Also we cannot lock * the SPI bus from within an interrupt. @@ -373,17 +360,10 @@ static int l3gd20_interrupt_handler(int irq, FAR void *context, snerr("ERROR: Failed to queue work: %d\n", ret); return ret; } -#else - - /* notify event to upper half driver */ - priv->lower.notify_event(priv->lower.priv); - -#endif return OK; } -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 /**************************************************************************** * Name: l3gd20_worker ****************************************************************************/ @@ -405,33 +385,6 @@ static void l3gd20_worker(FAR void *arg) sizeof(struct sensor_gyro_uncal)); } -#else - -/**************************************************************************** - * Name: l3gd20_fetch - ****************************************************************************/ - -static int l3gd20_fetch(FAR struct sensor_lowerhalf_s *lower, - FAR struct file *filep, - FAR char *buffer, size_t buflen) -{ - FAR struct l3gd20_dev_s *priv = container_of(lower, - FAR struct l3gd20_dev_s, - lower); - - if (buflen != sizeof(struct sensor_gyro_uncal)) - return 0; - - DEBUGASSERT(priv != NULL); - - /* Read out the latest sensor data */ - - l3gd20_read_measurement_data(priv, (FAR struct sensor_gyro_uncal *)buffer); - - return sizeof(struct sensor_gyro_uncal); -} -#endif - /**************************************************************************** * Name: l3gd20_activate ****************************************************************************/ @@ -563,9 +516,7 @@ int l3gd20_register(int devno, FAR struct spi_dev_s *spi, priv->spi = spi; priv->config = config; -#if CONFIG_SENSORS_L3GD20_BUFFER_SIZE > 0 priv->work.worker = NULL; -#endif priv->timestamp = 0; priv->lower.type = SENSOR_TYPE_GYROSCOPE_UNCALIBRATED; From 585014346b46a1df66eb308c85e0d5b573fb8fdc Mon Sep 17 00:00:00 2001 From: Felipe Moura Date: Sat, 1 Aug 2026 12:37:00 -0300 Subject: [PATCH 2/3] drivers/sensors/sensor: always report POLLIN for fetch only sensor A fetch() only lower half reads the device on demand, so its data is always available and there is never anything to wait for. The upper half did not reflect that: poll() only reported POLLIN when the descriptor was opened O_NONBLOCK, and a blocking read() waited on buffersem, which is only posted when the lower half drives notify_event from an interrupt of its own. A fetch() only sensor with no interrupt therefore never satisfied poll()/read() at all. This is not hypothetical: in the in tree nucleo-h563zi:dts configuration CONFIG_STM32_DTS_TRIGGER defaults to 0, which selects stm32_dts_fetch(), and no CONFIG_STM32_DTS_ITEN_* option is enabled, so the DTS interrupt never fires. A blocking read() on that sensor waits forever, even though stm32_dts_fetch() performs a complete software triggered measurement on its own and needs no interrupt at all. Applications had to work around this by forcing O_NONBLOCK on the descriptor themselves, see apache/nuttx-apps#3686. Drop the O_NONBLOCK special case in both paths: sensor_poll() now always reports POLLIN for a fetch only sensor and sensor_read() calls fetch() directly instead of waiting. Update the sensor_ops_s::fetch documentation, which described the old contract. With the wait gone, buffersem has no waiters left. Its only two readers were the ones removed here, both in the fetch path: the wait in sensor_read() and the nxsem_get_value() in sensor_poll(). The remaining nxsem_post() calls in sensor_push_event() and sensor_notify_event() had nothing left to wake, so drop the semaphore and those posts as well. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Felipe Moura --- drivers/sensors/sensor.c | 54 +++++++--------------------------- include/nuttx/sensors/sensor.h | 8 ++--- 2 files changed, 15 insertions(+), 47 deletions(-) diff --git a/drivers/sensors/sensor.c b/drivers/sensors/sensor.c index cb88cb33e1709..ef08737ae8269 100644 --- a/drivers/sensors/sensor.c +++ b/drivers/sensors/sensor.c @@ -101,7 +101,6 @@ struct sensor_user_s */ unsigned int event; /* The event of this sensor, eg: SENSOR_EVENT_FLUSH_COMPLETE. */ bool flushing; /* The is used to indicate user is flushing */ - sem_t buffersem; /* Wakeup user waiting for data in circular buffer */ size_t bufferpos; /* The index of user generation in buffer */ /* The subscriber info @@ -774,7 +773,6 @@ static int sensor_open(FAR struct file *filep) user->state.interval = UINT32_MAX; user->state.esize = upper->state.esize; user->state.nonwakeup = true; - nxsem_init(&user->buffersem, 0, 0); list_add_tail(&upper->userlist, &user->node); /* The new user generation, notify to other users */ @@ -835,7 +833,6 @@ static int sensor_close(FAR struct file *filep) } list_delete(&user->node); - nxsem_destroy(&user->buffersem); /* The user is closed, notify to other users */ @@ -871,24 +868,18 @@ static ssize_t sensor_read(FAR struct file *filep, FAR char *buffer, return -EINVAL; } - if (!(filep->f_oflags & O_NONBLOCK)) - { - nxrmutex_unlock(&upper->lock); - ret = nxsem_wait_uninterruptible(&user->buffersem); - if (ret < 0) - { - return ret; - } + /* Fetch the data from the device directly, there is nothing to wait + * for. This matches the POLLIN sensor_poll() always reports for a + * fetch only sensor. + */ - nxrmutex_lock(&upper->lock); - } - else if (!upper->state.nsubscribers) + if (!upper->state.nsubscribers) { ret = -EAGAIN; goto out; } - ret = lower->ops->fetch(lower, filep, buffer, len); + ret = lower->ops->fetch(lower, filep, buffer, len); } else if (circbuf_is_empty(&upper->buffer)) { @@ -1166,7 +1157,6 @@ static int sensor_poll(FAR struct file *filep, FAR struct sensor_lowerhalf_s *lower = upper->lower; FAR struct sensor_user_s *user = filep->f_priv; pollevent_t eventset = 0; - int semcount; int ret = 0; nxrmutex_lock(&upper->lock); @@ -1184,20 +1174,12 @@ static int sensor_poll(FAR struct file *filep, fds->priv = filep; if (lower->ops->fetch) { - /* Always return POLLIN for fetch data directly(non-block) */ + /* Always return POLLIN for fetch only sensor: the data is read + * from the device on demand by sensor_read(), so there is never + * anything to wait for. + */ - if (filep->f_oflags & O_NONBLOCK) - { - eventset |= POLLIN; - } - else - { - nxsem_get_value(&user->buffersem, &semcount); - if (semcount > 0) - { - eventset |= POLLIN; - } - } + eventset |= POLLIN; } else if (sensor_is_updated(upper, user)) { @@ -1229,7 +1211,6 @@ static ssize_t sensor_push_event(FAR void *priv, FAR const void *data, FAR struct sensor_lowerhalf_s *lower = upper->lower; FAR struct sensor_user_s *user; unsigned long envcount; - int semcount; int ret; nxrmutex_lock(&upper->lock); @@ -1287,12 +1268,6 @@ static ssize_t sensor_push_event(FAR void *priv, FAR const void *data, { if (sensor_is_updated(upper, user)) { - nxsem_get_value(&user->buffersem, &semcount); - if (semcount < 1) - { - nxsem_post(&user->buffersem); - } - sensor_pollnotify_one(user, POLLIN, SENSOR_ROLE_RD); } } @@ -1305,17 +1280,10 @@ static void sensor_notify_event(FAR void *priv) { FAR struct sensor_upperhalf_s *upper = priv; FAR struct sensor_user_s *user; - int semcount; nxrmutex_lock(&upper->lock); list_for_every_entry(&upper->userlist, user, struct sensor_user_s, node) { - nxsem_get_value(&user->buffersem, &semcount); - if (semcount < 1) - { - nxsem_post(&user->buffersem); - } - sensor_pollnotify_one(user, POLLIN, SENSOR_ROLE_RD); } diff --git a/include/nuttx/sensors/sensor.h b/include/nuttx/sensors/sensor.h index e675be50a673a..f266eb8cdd3c8 100644 --- a/include/nuttx/sensors/sensor.h +++ b/include/nuttx/sensors/sensor.h @@ -353,10 +353,10 @@ struct sensor_ops_s * If fetch isn't NULL, upper half driver will disable intermediate * buffer and userspace can't set buffer size by ioctl. * - * You can call this function to read sensor register data by I2C/SPI bus - * when open mode is non-block, and poll are always successful. - * When you call this function and open mode is block, you will wait - * until sensor data ready, then read sensor data. + * You can call this function to read sensor register data by I2C/SPI + * bus. The data is read from the device on demand, so it is always + * available: poll() always reports POLLIN and read() never blocks, + * whether or not the open mode is non-block. * * Input Parameters: * lower - The instance of lower half sensor driver. From c1e9f81e5419430d7866b7bbd0b8cacd382749c9 Mon Sep 17 00:00:00 2001 From: Felipe Moura Date: Sat, 1 Aug 2026 13:00:29 -0300 Subject: [PATCH 3/3] drivers/sensors/sensor: pace POLLIN at the requested interval A fetch() only lower half is always ready, so a subscriber that asked for a rate with SNIOC_SET_INTERVAL got no pacing from poll(): the descriptor reported POLLIN on every pass and the application had to sleep out the period itself. That does not compose. An application polling several topics reads them sequentially from one thread, so per read sleeps serialize: three topics at 10 Hz sleeping 100 ms each yield 3.3 Hz per topic rather than 10. Pace it where poll() can act on it instead. A subscriber that never requested a rate stays always ready, and one that did becomes ready once per its own interval, driven by a watchdog armed in sensor_poll(). This is the fetch() side of what sensor_is_updated() already does for a pushing lower half, so both models now honor a requested rate the same way. The wdog_s lives in sensor_user_s rather than in the device, so each subscriber is paced at its own interval instead of at the minimum across all of them, and the timer only runs while somebody is polling. The expiry runs in timer context and takes no lock: poll_notify() is safe from an interrupt handler, and a teardown that raced it has already cleared fds, which makes both the notify and the re-arm no-ops. Teardown therefore just cancels the watchdog where it clears fds, and a watchdog keeps this off the work queue entirely, which a fetch() only sensor exists to avoid. sensor_close() needs nothing of its own: poll_setup() holds a reference on the file for the duration of the poll, so file_close() cannot run until poll_teardown() has called sensor_poll() with setup false, and that already cancelled the watchdog. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Felipe Moura --- drivers/sensors/sensor.c | 54 ++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/drivers/sensors/sensor.c b/drivers/sensors/sensor.c index ef08737ae8269..fb6b89d99f0c9 100644 --- a/drivers/sensors/sensor.c +++ b/drivers/sensors/sensor.c @@ -43,6 +43,7 @@ #include #include #include +#include /**************************************************************************** * Pre-processor Definitions @@ -96,6 +97,8 @@ struct sensor_user_s struct list_node node; /* Node of users list */ struct pollfd *fds; /* The poll structure of thread waiting events */ sensor_role_t role; /* The is used to indicate user's role based on open flags */ + struct wdog_s wdog; /* Paces POLLIN at the requested interval */ + uint64_t fetched; /* When POLLIN was last reported, in usec */ bool changed; /* This is used to indicate event happens and need to * asynchronous notify other users */ @@ -142,6 +145,7 @@ static int sensor_poll(FAR struct file *filep, FAR struct pollfd *fds, bool setup); static ssize_t sensor_push_event(FAR void *priv, FAR const void *data, size_t bytes); +static void sensor_fetch_expired(wdparm_t arg); /**************************************************************************** * Private Data @@ -687,6 +691,23 @@ static void sensor_pollnotify_one(FAR struct sensor_user_s *user, poll_notify(&user->fds, 1, eventset); } +static void sensor_fetch_expired(wdparm_t arg) +{ + FAR struct sensor_user_s *user = (FAR struct sensor_user_s *)arg; + + /* Timer context, so no lock: a teardown that raced us cleared fds, which + * makes both the notify and the re-arm below no-ops. + */ + + if (user->fds != NULL) + { + user->fetched = sensor_get_timestamp(); + sensor_pollnotify_one(user, POLLIN, SENSOR_ROLE_RD); + wd_start(&user->wdog, USEC2TICK(user->state.interval), + sensor_fetch_expired, arg); + } +} + static void sensor_pollnotify(FAR struct sensor_upperhalf_s *upper, pollevent_t eventset, sensor_role_t role) { @@ -868,10 +889,7 @@ static ssize_t sensor_read(FAR struct file *filep, FAR char *buffer, return -EINVAL; } - /* Fetch the data from the device directly, there is nothing to wait - * for. This matches the POLLIN sensor_poll() always reports for a - * fetch only sensor. - */ + /* Read the device directly, there is nothing to wait for */ if (!upper->state.nsubscribers) { @@ -1174,12 +1192,31 @@ static int sensor_poll(FAR struct file *filep, fds->priv = filep; if (lower->ops->fetch) { - /* Always return POLLIN for fetch only sensor: the data is read - * from the device on demand by sensor_read(), so there is never - * anything to wait for. + /* Always ready, unless a rate was requested: then once per + * interval, woken by sensor_fetch_expired(). */ - eventset |= POLLIN; + if (user->state.interval == UINT32_MAX) + { + eventset |= POLLIN; + } + else + { + uint64_t now = sensor_get_timestamp(); + uint64_t elapsed = now - user->fetched; + + if (elapsed >= user->state.interval) + { + user->fetched = now; + eventset |= POLLIN; + } + else + { + wd_start(&user->wdog, + USEC2TICK(user->state.interval - elapsed), + sensor_fetch_expired, (wdparm_t)user); + } + } } else if (sensor_is_updated(upper, user)) { @@ -1197,6 +1234,7 @@ static int sensor_poll(FAR struct file *filep, { user->fds = NULL; fds->priv = NULL; + wd_cancel(&user->wdog); } errout: