Skip to content

drivers/sensors/sensor: fix poll()/read() for fetch()-only sensors - #19596

Open
FelipeMdeO wants to merge 3 commits into
apache:masterfrom
FelipeMdeO:fix/uorb-sensor-fetch-only-pollin
Open

drivers/sensors/sensor: fix poll()/read() for fetch()-only sensors#19596
FelipeMdeO wants to merge 3 commits into
apache:masterfrom
FelipeMdeO:fix/uorb-sensor-fetch-only-pollin

Conversation

@FelipeMdeO

Copy link
Copy Markdown
Contributor

Summary

A fetch()-only lower half reads the device on demand, so it's always
ready, nothing to wait for. The upper half didn't reflect that:
sensor_poll() only reported POLLIN when opened O_NONBLOCK, and a
blocking read() waited on buffersem, which only a driver-owned
interrupt (notify_event) posts. A fetch()-only sensor with no
interrupt therefore never satisfied poll()/read() at all — e.g.
in-tree nucleo-h563zi:dts hangs on a blocking read() today.
Applications worked around this with O_NONBLOCK (apache/nuttx-apps#3686);
this PR fixes it at the root instead, per reviewer feedback there.

Three commits:

  1. l3gd20 → push-only. Its old BUFFER_SIZE == 0 mode kept fetch()
    set while also driving notify_event from the data-ready interrupt,
    and that combination was itself buggy, not just inconsistent:
    sensor_poll()'s fetch-only branch reported POLLIN whenever the
    descriptor was O_NONBLOCK, regardless of whether the interrupt had
    actually fired, so a multi-descriptor poll() loop could read stale
    or duplicate data off the l3gd20 while correctly waiting on other
    sensors. Push-only fixes that, and generalizes to every
    interrupt-driven sensor in the tree: the interrupt handler reads the
    sample and calls push_event(), landing it in the upper half's
    circular buffer and waking any poller through sensor_pollnotify().
    POLLIN is then reported only when sensor_is_updated() sees a
    generation the caller hasn't consumed yet — acquisition (the
    interrupt reading the chip) and delivery (the app's read()) are
    decoupled and correctly synchronized, instead of read() racing the
    next interrupt. read() never touches the bus at that point, it just
    copies out of the buffer. This is the existing, already-correct model
    for every other push sensor in tree; l3gd20 was the one exception.
  2. sensor_poll()/sensor_read() always ready for fetch()-only.
    The actual fix. notify_event becomes dead code for fetch()-only
    lower halves as a result; left the API in place, open to deprecating
    it in a follow-up.
  3. Pace POLLIN at the requested interval. Without it, a rate
    request (-r) got no help from poll(), and apps sleeping it out
    themselves serialize across multiple topics (three at 10 Hz → 3.3 Hz
    each). A per-subscriber work_s timer, armed in sensor_poll(),
    paces each one at its own interval — the fetch() counterpart of what
    sensor_is_updated() already does for push. Falls back to
    always-ready without CONFIG_SCHED_LPWORK.

Impact

Any fetch()-only uORB sensor (on-demand I2C/SPI, no interrupt) now
works with poll()/read() without the app forcing O_NONBLOCK. Every
interrupt-driven sensor, including l3gd20 after commit 1, keeps blocking
correctly until real new data arrives — nothing about push semantics
changes for them, verified both by code and on hardware (Testing).

Testing

Host: Ubuntu 24.04.3 LTS. checkpatch.sh (style + -m commit messages)
clean on all 3 commits.

Compiles clean on 3 architectures touching the changed
drivers/sensors/sensor.c: xtensa-esp-elf-gcc 14.2.0 (esp32s3-devkit),
arm-none-eabi-gcc 13.2.1 (stm32f4discovery), and sim (x86_64,
UORB_LISTENER + CONFIG_SCHED_LPWORK).

Fetch-only sensor on hardware — ESP32-S3-DevKitC + MPU6050 (GY-521)
on I2C0, fetch()-only with no interrupt line, no O_NONBLOCK anywhere
in the unmodified apps/system/uorb/listener.c:

nsh> uorb_listener -n 5 sensor_accel0
Monitor objects num:1
object_name:sensor_accel, object_instance:0
sensor_accel(now:37860000):timestamp:37860000,x:-0.205901,y:-0.122104,z:10.323798,temperature:23.824116
sensor_accel(now:37860000):timestamp:37860000,x:-0.205901,y:-0.122104,z:10.323798,temperature:23.777058
sensor_accel(now:37860000):timestamp:37860000,x:-0.205901,y:-0.122104,z:10.323798,temperature:23.730000
sensor_accel(now:37860000):timestamp:37860000,x:-0.205901,y:-0.122104,z:10.323798,temperature:23.918234
sensor_accel(now:37860000):timestamp:37860000,x:-0.131681,y:-0.134075,z:10.299856,temperature:23.824116
Object name:sensor_accel0, received:5
Total number of received Message:5/5

Per-subscriber pacing, same hardware: two uorb_listener processes
on sensor_accel0 at once, -r 20 and -r 5. Samples landed ~60ms and
~210ms apart respectively — each subscriber paced at its own rate, not
the minimum across both (the failure mode of an earlier per-device-timer
prototype).

Teardown under stress: 8 rounds of uorb_listener -r 5 sensor_accel0 &
followed by SIGINT mid-poll. ps afterward shows no leaked task and no
stuck lpwork item; the sensor still answers normally right after.

Push sensor on hardware — STM32F4Discovery onboard accelerometer via
CONFIG_SENSORS_LIS3DSH_UORB: turned out to be an LIS302DL (older board
revision, confirmed by WHO_AM_I=0x3b vs LIS3DSH's 0x3f), a
pre-existing hardware mismatch unrelated to this change, so no live data
from this one. Push path correctness instead verified by inspection: the
else branch in sensor_read()/sensor_poll() that push sensors take
is untouched by this series.

@github-actions github-actions Bot added Area: Sensors Sensors issues Size: M The size of the change in this PR is medium labels Aug 1, 2026
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 <moura.fmo@gmail.com>
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.

Assisted-by: Claude:claude-sonnet-5

Signed-off-by: Felipe Moura <moura.fmo@gmail.com>
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 timer 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 work_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. Teardown
clears fds before cancelling so a running worker can neither notify nor
re-arm, and the cancel itself happens outside upper->lock, which the
worker takes. sensor_close() does the same, since work_cancel_sync() waits
out a running worker but does not undo a re-arm it performed.

Without CONFIG_SCHED_LPWORK there is no timer to pace with, so poll()
keeps reporting ready every time, as it does today.

Assisted-by: Claude:claude-sonnet-5

Signed-off-by: Felipe Moura <moura.fmo@gmail.com>
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

Comment thread drivers/sensors/sensor.c
}
else
{
work_queue(LPWORK, &user->work, sensor_fetch_worker, user,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we avoid the work queue? the benefit from fetch is avoiding the work or thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Sensors Sensors issues Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants