Read the device on a worker thread, not on the event loop

CLAUDE.md

@@ -38,7 +38,7 @@ CI's separate `boot` job is what verifies `src/main.py` actually starts against
 
 `controls(odrv)` takes a **live `fibre` remote object**, so it is dynamically typed (`Any`).
 Every attribute it reads (`odrv.axis0.motor.current_control.Iq_measured`, `odrv.axis0.controller.config.pos_gain`, …) mirrors the ODrive 0.5.x/0.6.x object tree.
-Every property read is a blocking USB round-trip on the event loop, so keep timers lean.
+Every property read is a blocking USB round-trip, so after the panel is built all device traffic goes through the single `_usb` worker in `controls.py` (`_Mirror` for bound values, `_on_usb` for timer reads, `_send` for writes and commands) — never read or write the device on the event loop, and keep timers lean.
 When you change the UI, mirror the same attribute path in `tools/mock_odrive.py` so the hardware-free tests keep covering it.
 
 ## Page structure (NiceGUI 3.x)

src/controls.py

@@ -12,18 +12,27 @@
 
 from __future__ import annotations
 
+import asyncio
+import importlib
+import itertools
 import logging
 from collections.abc import Callable
+from concurrent.futures import Future, ThreadPoolExecutor
 from datetime import datetime
 from typing import Any
 
 from nicegui import ui
-from nicegui.events import ValueChangeEventArguments
 from odrive.pyfibre import fibre
 from odrive.utils import dump_errors
 
 log = logging.getLogger('odrive_gui')
 
+# ``import odrive`` loads libfibre a second time, as top-level ``fibre``, and the device objects
+# come from that copy: its ObjectLostError and EmptyInterface are other classes than ours.
+_device_fibre = importlib.import_module('fibre')  # what odrive's own discovery thread imports
+LOST_ERRORS = (fibre.ObjectLostError, _device_fibre.ObjectLostError)
+EMPTY_INTERFACES = (fibre.libfibre.EmptyInterface, _device_fibre.libfibre.EmptyInterface)
+
 MODES: dict[int, str] = {
     0: 'Voltage',
     1: 'Torque',
@@ -41,6 +50,8 @@
     7: 'Mirror',
 }
 
+IDLE = 1
+
 STATES: dict[int, str] = {
     0: 'Undefined',
     1: 'Idle',
@@ -55,7 +66,88 @@ def _lost(obj: Any) -> bool:
     which every attribute read raises ``AttributeError``. The panel's timers can fire in
     the short window before the discovery loop deletes the panel, so they check first.
     """
-    return obj.__class__ is fibre.libfibre.EmptyInterface
+    return obj.__class__ in EMPTY_INTERFACES
+
+
+def is_lost(e: BaseException) -> bool:
+    """Whether ``e`` means "the device left the bus" rather than a bug in our code.
+
+    fibre reports a loss in two ways: a read that fails on the bus raises
+    ``ObjectLostError``, but once libfibre's lost-object callback has run -- on *its own*
+    thread, so at any point, even between two reads of a synchronous panel build -- the
+    object's class is swapped to ``EmptyInterface`` and every attribute read raises a
+    plain ``AttributeError``. Only an ``AttributeError`` on such a lost object counts;
+    a typo'd attribute path in ``controls()`` still surfaces.
+    """
+    if isinstance(e, LOST_ERRORS):
+        return True
+    return isinstance(e, AttributeError) and isinstance(e.obj, EMPTY_INTERFACES)
+
+
+# Every device read or write is a blocking USB round-trip. After the panel is built, all of
+# them run on this one worker, in the order they were issued: the event loop never waits for
+# the bus, and a command can never overtake a setting the user changed before it.
+_usb = ThreadPoolExecutor(max_workers=1, thread_name_prefix='odrive-usb')
+_write_order = itertools.count()
+
+
+def _send(obj: Any, fn: Callable[[], Any]) -> None:
+    """Queue ``fn()`` (a write or command) on the USB worker; log it if it fails."""
+
+    def done(future: Future) -> None:
+        e = future.exception()
+        if e is not None and not (is_lost(e) or _lost(obj)):
+            log.error('ODrive write failed', exc_info=e)
+
+    _usb.submit(fn).add_done_callback(done)
+
+
+async def _on_usb(obj: Any, fn: Callable[[], Any]) -> Any:
+    """``fn()`` on the USB worker; ``None`` if ``obj``'s device left the bus meanwhile.
+
+    ``fn`` may read through sub-objects of ``obj``. libfibre tears a lost device down
+    children first, so such a read can fail while ``obj`` still looks alive: ``is_lost``
+    recognises it by the object the ``AttributeError`` names.
+    """
+    try:
+        return await asyncio.get_running_loop().run_in_executor(_usb, fn)
+    except Exception as e:
+        if is_lost(e) or _lost(obj):
+            return None
+        raise
+
+
+class _Mirror:
+    """A local copy of the attributes widgets bind to on one fibre object.
+
+    NiceGUI cannot be notified by a fibre object, so it polls every bound attribute each
+    0.1 s -- on the event loop. Widgets bind to this mirror instead: reading it is free,
+    ``sync()`` refreshes it on the USB worker, and a widget's change is written through there.
+    """
+
+    def __init__(self, obj: Any) -> None:
+        object.__setattr__(self, '_obj', obj)
+        object.__setattr__(self, '_values', {})
+        object.__setattr__(self, '_written', {})  # name -> _write_order of its last write
+
+    def __getattr__(self, name: str) -> Any:
+        if name not in self._values:
+            self._values[name] = getattr(self._obj, name)  # first read, while building the panel
+        return self._values[name]
+
+    def __setattr__(self, name: str, value: Any) -> None:
+        self._values[name] = value
+        self._written[name] = next(_write_order)
+        _send(self._obj, lambda: setattr(self._obj, name, value))
+
+    async def sync(self) -> None:
+        names = list(self._values)
+        start = next(_write_order)
+        values = await _on_usb(self._obj, lambda: [getattr(self._obj, name) for name in names])
+        for name, value in zip(names, values or [], strict=False):
+            # a write queued after this read started is newer than what the read saw
+            if self._written.get(name, -1) < start:
+                self._values[name] = value
 
 
 def controls(odrv: Any) -> None:
@@ -70,7 +162,7 @@ def controls(odrv: Any) -> None:
     def reboot() -> None:
         try:
             odrv.reboot()
-        except fibre.ObjectLostError:
+        except LOST_ERRORS:
             # the device drops off the USB bus mid-reboot; that specific loss is expected.
             # do NOT touch `odrv` here — a live read on the just-disconnected device raises again.
             log.info('ODrive %x rebooting (connection dropped as expected)', serial)
@@ -81,15 +173,20 @@ def reboot() -> None:
             _chip(f'HW {odrv.hw_version_major}.{odrv.hw_version_minor}.{odrv.hw_version_variant}')
             _chip(f'FW {odrv.fw_version_major}.{odrv.fw_version_minor}.{odrv.fw_version_revision}{" (dev)" if odrv.fw_version_unreleased else ""}')
             voltage = ui.label().classes('text-lg font-medium text-primary')
-            ui.timer(1.0, lambda: _lost(odrv) or voltage.set_text(f'{odrv.vbus_voltage:.2f} V'))
+
+            async def update_voltage() -> None:
+                if (vbus := await _on_usb(odrv, lambda: odrv.vbus_voltage)) is not None:
+                    voltage.set_text(f'{vbus:.2f} V')
+
+            ui.timer(1.0, update_voltage)
         with ui.row().classes('gap-1'):
             # wrap in a lambda (like the original): passing the bare fibre RemoteFunction makes
             # NiceGUI introspect its signature, which is brittle on the C-backed proxy.
-            ui.button(icon='save', on_click=lambda: odrv.save_configuration()).props('dense').tooltip('Save configuration')
-            ui.button(icon='bug_report', on_click=lambda: dump_errors(odrv, hasattr(odrv, 'clear_errors'))).props('dense').tooltip(
-                'Dump and clear errors'
-            )
-            ui.button(icon='restart_alt', on_click=reboot).props('dense').tooltip('Reboot ODrive')
+            ui.button(icon='save', on_click=lambda: _send(odrv, lambda: odrv.save_configuration())).props('dense').tooltip('Save configuration')
+            ui.button(icon='bug_report', on_click=lambda: _send(odrv, lambda: dump_errors(odrv, hasattr(odrv, 'clear_errors')))).props(
+                'dense'
+            ).tooltip('Dump and clear errors')
+            ui.button(icon='restart_alt', on_click=lambda: _send(odrv, reboot)).props('dense').tooltip('Reboot ODrive')
 
     with ui.row().classes('gap-4 items-stretch p-4'):
         for index, axis in enumerate([odrv.axis0, odrv.axis1]):
@@ -117,39 +214,67 @@ def _create_axis_column(index: int, axis: Any) -> None:
             power = ui.label()
             # access via lambda, not `on_click=axis.clear_errors`: the method is optional on
             # older firmware, so it must not be read until the button (if shown) is clicked.
-            button = ui.button(icon='bug_report', color='negative', on_click=lambda: axis.clear_errors()).props('dense').tooltip('Clear errors')
+            button = (
+                ui.button(icon='bug_report', color='negative', on_click=lambda: _send(axis, lambda: axis.clear_errors()))
+                .props('dense')
+                .tooltip('Clear errors')
+            )
             button.set_visibility(hasattr(axis, 'clear_errors'))
 
-    ctr_cfg = axis.controller.config
-    mtr_cfg = axis.motor.config
-    enc_cfg = axis.encoder.config
-    trp_cfg = axis.trap_traj.config
+    mirrors = [_Mirror(obj) for obj in (axis, axis.controller.config, axis.motor.config, axis.encoder.config, axis.trap_traj.config)]
+    axis_state, ctr_cfg, mtr_cfg, enc_cfg, trp_cfg = mirrors
     cc = axis.motor.current_control
 
-    def update_power() -> None:
-        if _lost(axis):
-            return
-        power.set_text(f'{cc.Iq_measured * cc.v_current_control_integral_q:.1f} W')
+    async def sync_mirrors() -> None:
+        for mirror in mirrors:
+            await mirror.sync()
+        show_state(axis_state.current_state)
+
+    async def update_power() -> None:
+        if (watts := await _on_usb(axis, lambda: cc.Iq_measured * cc.v_current_control_integral_q)) is not None:
+            power.set_text(f'{watts:.1f} W')
 
-    # every property read is a blocking USB round-trip on the event loop, so keep the
-    # 10 Hz path to the two values that must feel live; the error flag only enables a
-    # button and can follow at 1 Hz.
+    async def update_error() -> None:
+        if (error := await _on_usb(axis, lambda: axis.error)) is not None:
+            button.set_enabled(error != 0)
+
+    # every property read is a USB round-trip, so keep the 10 Hz path to the two values
+    # that must feel live; the error flag only enables a button and can follow at 1 Hz.
     ui.timer(0.1, update_power)
-    ui.timer(1.0, lambda: _lost(axis) or button.set_enabled(axis.error != 0))
-
-    def request_state(e: ValueChangeEventArguments) -> None:
-        # Only a *user* choice becomes a request. The toggle also changes when the
-        # binding below mirrors a new current_state into it (then value == current_state)
-        # or coerces a state outside STATES (calibration, homing) to None — neither may be
-        # written back: a two-way bind did exactly that and e.g. re-requested CLOSED_LOOP
-        # for an axis that came up in it, which makes 0.5.x firmware leave and re-enter
-        # closed loop (a brief disarm of a live motor).
-        if e.value is not None and e.value != axis.current_state:
-            axis.requested_state = e.value
+    ui.timer(0.1, sync_mirrors)
+    ui.timer(1.0, update_error)
+
+    def request_state(state: int) -> None:
+        if state != IDLE and state == axis_state.current_state:
+            return  # re-requesting e.g. CLOSED_LOOP would briefly disarm the motor (see below)
+        _send(axis, lambda: setattr(axis, 'requested_state', state))
+
+    # The axis state is one button per state, not a toggle: a click on Idle must reach the
+    # device, but a toggle drops a click on the option it already shows -- and the state
+    # shown can be a refresh old (the firmware has not switched yet), so that click can be
+    # the Idle that stops a motor. A toggle's value changes are no better a trigger: they
+    # also fire when a refresh shows the device's state, and writing that back re-requested
+    # e.g. CLOSED_LOOP for an axis already in it, which makes 0.5.x firmware leave and
+    # re-enter closed loop (a brief disarm of a live motor). So clicks request, refreshes show,
+    # and a click on any other state the axis is already in is dropped: at worst a stale
+    # state swallows a click that starts nothing.
+    state_buttons: dict[int, ui.button] = {}
+    shown: list[int | None] = [None]
+
+    def show_state(state: int) -> None:
+        if state == shown[0]:
+            return
+        shown[0] = state
+        for value, state_button in state_buttons.items():
+            state_button.props(remove='flat unelevated color')
+            state_button.props('unelevated color=primary' if value == state else 'flat')
 
     with ui.row().classes('gap-2'):
         mode = ui.toggle(MODES).bind_value(ctr_cfg, 'control_mode')
-        ui.toggle(STATES, on_change=request_state).bind_value_from(axis, 'current_state')
+        with ui.element('div').classes('q-btn-toggle row no-wrap inline'):
+            for value, label in STATES.items():
+                state_buttons[value] = ui.button(label, on_click=lambda value=value: request_state(value)).props(remove='round').props('no-caps')
+        show_state(axis_state.current_state)
 
     with ui.row().classes('gap-4 items-start'):
         for value, name, attr, icons in _MOTION_INPUTS:
@@ -210,7 +335,8 @@ def _motion_input(axis: Any, name: str, attr: str, icons: tuple[str, str, str])
     field = ui.number(f'input {name.lower()}', value=0)
 
     def send(sign: int) -> None:
-        setattr(axis.controller, attr, sign * _field_value(field))
+        value = sign * _field_value(field)
+        _send(axis, lambda: setattr(axis.controller, attr, value))
 
     with ui.row().classes('w-full justify-around gap-0'):
         for icon, sign in zip(icons, (-1, 0, 1), strict=True):
@@ -225,12 +351,13 @@ def _live_plots(axis: Any) -> None:
     plots: list[tuple[ui.checkbox, ui.line_plot, Callable[[], list[list[float]]]]] = []
     timer: ui.timer | None = None
 
-    def push() -> None:
+    async def push() -> None:
         now = datetime.now()
-        for check, plot, sample in plots:
-            if check.value:
-                # line_plot uses a datetime x-axis (matplotlib handles it); the stub types x as float.
-                plot.push([now], sample())  # type: ignore[list-item]
+        shown = [(plot, sample) for check, plot, sample in plots if check.value]
+        samples = await _on_usb(axis, lambda: [sample() for _, sample in shown])
+        for (plot, _), values in zip(shown, samples or [], strict=False):
+            # line_plot uses a datetime x-axis (matplotlib handles it); the stub types x as float.
+            plot.push([now], values)  # type: ignore[list-item]
 
     def update_timer() -> None:
         nonlocal timer

src/main.py

@@ -13,9 +13,8 @@
 
 import odrive
 from nicegui import app, ui
-from odrive.pyfibre import fibre
 
-from controls import controls
+from controls import controls, is_lost
 from theme import apply_theme, header
 
 logging.basicConfig(level=logging.WARNING, format='%(asctime)s %(levelname)s %(message)s')
@@ -30,21 +29,6 @@
 devices: dict[int, Any] = {}
 
 
-def is_lost(e: Exception) -> bool:
-    """Whether ``e`` means "the device left the bus" rather than a bug in our code.
-
-    fibre reports a loss in two ways: a read that fails on the bus raises
-    ``ObjectLostError``, but once libfibre's lost-object callback has run -- on *its own*
-    thread, so at any point, even between two reads of a synchronous panel build -- the
-    object's class is swapped to ``EmptyInterface`` and every attribute read raises a
-    plain ``AttributeError``. Only an ``AttributeError`` on such a lost object counts;
-    a typo'd attribute path in ``controls()`` still surfaces.
-    """
-    if isinstance(e, fibre.ObjectLostError):
-        return True
-    return isinstance(e, AttributeError) and isinstance(e.obj, fibre.libfibre.EmptyInterface)
-
-
 @ui.refreshable
 def device_panels() -> None:
     """One full-width panel per connected device, or a placeholder if there is none.