From fea05ce3629644ad15830721dac6f1101c27f057 Mon Sep 17 00:00:00 2001 From: evnchn Date: Sun, 25 Oct 2026 23:05:56 +0800 Subject: [PATCH 1/5] Read the device on a worker thread, not on the event loop Widgets were bound straight to fibre objects. NiceGUI cannot be notified by those, so its binding loop polled every bound attribute every 0.1 s on the event loop, each poll a blocking USB round-trip (44 per tick with two calibrated axes). The power, voltage and error timers and the plot sampler read on the loop too. Widgets now bind to a local mirror per fibre object. One worker thread refreshes the mirrors at 10 Hz and carries every timer read, write and command, in the order the user made them, so "lower current_lim, then enter closed loop" reaches the device in that order. A refresh that read a value before the user edited it does not undo the edit. The axis state is now one button per state instead of a toggle. A toggle drops a click on the option it already shows, and the state shown can be a refresh old, so a click on Idle meant to stop the motor could be lost. The mock counts reads per thread and can give each read a latency (MOCK_ODRIVE_READ_MS). Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 2 +- src/controls.py | 168 +++++++++++++++++++++++++++++++++---------- tests/test_main.py | 144 +++++++++++++++++++++++++++++++++---- tests/test_render.py | 38 ++++++++++ tools/mock_odrive.py | 39 +++++++--- 5 files changed, 329 insertions(+), 62 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index aec3ac4..5da2714 100644 --- a/CLAUDE.md +++ b/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) diff --git a/src/controls.py b/src/controls.py index e98e5de..77b9942 100644 --- a/src/controls.py +++ b/src/controls.py @@ -12,13 +12,15 @@ from __future__ import annotations +import asyncio +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 @@ -58,6 +60,67 @@ def _lost(obj: Any) -> bool: return obj.__class__ is fibre.libfibre.EmptyInterface +# 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 (isinstance(e, fibre.ObjectLostError) 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.""" + try: + return await asyncio.get_running_loop().run_in_executor(_usb, fn) + except Exception as e: + if isinstance(e, fibre.ObjectLostError) 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: """Render the control panel for a single connected ODrive device.""" @@ -81,15 +144,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 +185,63 @@ 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) - # 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_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') + + 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: + _send(axis, lambda: setattr(axis, 'requested_state', state)) + + # The axis state is one button per state, not a toggle: every click 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. + 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 +302,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 +318,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 diff --git a/tests/test_main.py b/tests/test_main.py index d8de470..094a0e7 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -8,13 +8,34 @@ """ import asyncio +import threading import pytest from mock_odrive import arrive_mid_scan, lose_mock_odrive, make_mock_odrive, set_connected_devices from nicegui import ui from nicegui.testing import User -from controls import STATES +from controls import STATES, _usb + + +def state_buttons(user: User) -> dict[int, ui.button]: + return {state: user.find(kind=ui.button, content=label).elements.pop() for state, label in STATES.items()} + + +def pick(user: User, state: int) -> None: + """Click the axis-state button for ``state``.""" + user.find(kind=ui.button, content=STATES[state]).click() + + +def shown_state(user: User) -> int | None: + """The state the axis-state buttons highlight.""" + return next((state for state, button in state_buttons(user).items() if button.props.get('color') == 'primary'), None) + + +async def drain() -> None: + """Wait until every device write queued so far has run on the USB worker.""" + await asyncio.wrap_future(_usb.submit(lambda: None)) + pytestmark = pytest.mark.nicegui_main_file('src/main.py') @@ -92,33 +113,33 @@ async def test_every_client_sees_the_devices(user: User, create_user) -> None: await other.should_see('SN 3333') -async def test_state_toggle_only_writes_user_choices(user: User) -> None: - """The axis-state toggle mirrors ``current_state`` but must not echo it back as a - request: only a state picked in the UI is written to ``requested_state``.""" - dev = make_mock_odrive(two_axes=False) # one axis -> exactly one state toggle on the page +async def test_state_buttons_show_the_device_state_and_only_clicks_request_one(user: User) -> None: + """The highlighted state follows ``current_state``, but only a click writes + ``requested_state`` -- a refresh never echoes the device's state back as a request.""" + dev = make_mock_odrive(two_axes=False) # one axis -> exactly one set of state buttons axis = dev.axis0 axis.current_state = 8 # came up in CLOSED_LOOP_CONTROL axis.requested_state = 'untouched' set_connected_devices([dev]) await user.open('/') await user.should_see('Axis 0') - await asyncio.sleep(0.3) # a few binding refresh cycles + await asyncio.sleep(0.3) # a few refreshes assert axis.requested_state == 'untouched' # rendering did not request anything + assert shown_state(user) == 8 - state_toggle = next(t for t in user.find(ui.toggle).elements if t.options == STATES) - assert state_toggle.value == 8 - state_toggle.set_value(1) # the user picks Idle + pick(user, 1) # the user clicks Idle + await drain() assert axis.requested_state == 1 axis.requested_state = 'untouched' - axis.current_state = 1 # the firmware followed; the toggle mirrors it … + axis.current_state = 1 # the firmware followed; the buttons show it … await asyncio.sleep(0.3) - assert state_toggle.value == 1 + assert shown_state(user) == 1 assert axis.requested_state == 'untouched' # … without writing it back - axis.current_state = 3 # a state the toggle has no option for (calibration) … + axis.current_state = 3 # a state without a button (calibration) … await asyncio.sleep(0.3) - assert state_toggle.value is None - assert axis.requested_state == 'untouched' # … is not written back as 0 either + assert shown_state(user) is None + assert axis.requested_state == 'untouched' # … is not written back either async def test_stop_with_cleared_input_field_still_writes_zero(user: User) -> None: @@ -132,4 +153,99 @@ async def test_stop_with_cleared_input_field_still_writes_zero(user: User) -> No field = next(n for n in user.find(ui.number).elements if n.props['label'] == 'input torque') field.set_value(None) # the user cleared the box user.find(ui.button).trigger('click') # includes the torque stop button (visible buttons only) + await drain() assert dev.axis0.controller.input_torque == 0.0 + + +async def test_gain_edits_reach_the_device_and_device_changes_reach_the_ui(user: User) -> None: + """The widgets bind to local mirrors, so both directions must still get through: an + edit is written to the device (and not undone by a refresh that read the old value + in the meantime), and a change made on the device shows up in the UI.""" + device = make_mock_odrive(two_axes=False) # one axis -> exactly one pos_gain field + set_connected_devices([device]) + await user.open('/') + await user.should_see('pos_gain') + field = next(n for n in user.find(ui.number).elements if n.props.get('label') == 'pos_gain') + config = device.axis0.controller.config + + field.set_value(42.0) + await asyncio.sleep(1.2) + assert config.pos_gain == 42.0 + assert field.value == 42.0 + + config.pos_gain = 7.0 # changed behind the UI's back (odrivetool, another client) + await asyncio.sleep(1.2) + assert field.value == 7.0 + + +def _record_state_requests(axis, follow: bool = True) -> list[tuple[int, float]]: + """Record every ``requested_state`` write, with the ``current_lim`` the device had then.""" + seen: list[tuple[int, float]] = [] + + def _request(self, state: int) -> None: + seen.append((state, self.motor.config.current_lim)) + if follow: + self.current_state = state # the firmware follows at once + + axis.__class__ = type( + 'RecordingAxis', + (type(axis),), + {'requested_state': property(lambda self: seen[-1][0] if seen else 1, _request)}, + ) + return seen + + +async def test_a_quick_loop_then_idle_is_not_dropped(user: User) -> None: + """Loop then Idle before any refresh: both are requested, in order.""" + dev = make_mock_odrive(two_axes=False) + seen = _record_state_requests(dev.axis0) + set_connected_devices([dev]) + await user.open('/') + await user.should_see('Axis 0') + gate = threading.Event() + _usb.submit(gate.wait) # hold the worker: no refresh can run in between + pick(user, 8) + pick(user, 1) + gate.set() + await drain() + assert [state for state, _ in seen] == [8, 1] + + +async def test_a_command_does_not_overtake_a_setting_changed_before_it(user: User) -> None: + """Lowering ``current_lim`` and then entering closed loop must reach the device in that + order, even while the USB worker is busy: the limit is the motor's protection.""" + dev = make_mock_odrive(two_axes=False) + seen = _record_state_requests(dev.axis0) + set_connected_devices([dev]) + await user.open('/') + await user.should_see('current_lim') + limit = next(n for n in user.find(ui.number).elements if n.props.get('label') == 'current_lim') + gate = threading.Event() + _usb.submit(gate.wait) # the worker is busy (a slow read, a refresh) + limit.set_value(1.0) + pick(user, 8) + gate.set() + await drain() + assert seen == [(8, 1.0)] + + +async def test_idle_is_requested_even_while_idle_is_the_state_shown(user: User) -> None: + """The user clicks Loop; refreshes still read Idle because the firmware has not switched + yet, so Idle is what is shown; then the firmware enters Loop. The user's click on the + Idle shown must still be requested -- a toggle dropped it (on the server and in the + browser alike) and left the axis in closed loop.""" + dev = make_mock_odrive(two_axes=False) + seen = _record_state_requests(dev.axis0, follow=False) + set_connected_devices([dev]) + await user.open('/') + await user.should_see('Axis 0') + pick(user, 8) + await asyncio.sleep(0.3) # refreshes still read Idle + assert shown_state(user) == 1 + gate = threading.Event() + _usb.submit(gate.wait) # no refresh shows the switch before the click + dev.axis0.current_state = 8 # now the firmware switches + pick(user, 1) + gate.set() + await drain() + assert [state for state, _ in seen] == [8, 1] diff --git a/tests/test_render.py b/tests/test_render.py index 1da92b4..bfab7e7 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -8,10 +8,14 @@ """ import asyncio +import threading +from mock_odrive import READS from nicegui import ui from nicegui.testing import User +from controls import _Mirror, _usb + async def test_panel_renders(user: User) -> None: await user.open('/') @@ -61,3 +65,37 @@ async def test_live_plots_and_telemetry_read_the_mock(user: User) -> None: for check in checks: check.set_value(False) await asyncio.sleep(0.1) # the per-axis plot timers are torn down without error + + +async def test_live_values_are_not_read_on_the_event_loop(user: User) -> None: + """Every device read is a blocking USB round-trip, so after the panel is built none may + happen on the event loop: widgets bind to local mirrors that a worker thread refreshes. + Bound straight to the fibre objects instead, NiceGUI's binding loop polls each of them + on the loop every 0.1 s (two calibrated axes: ~440 USB reads per second).""" + await user.open('/') + await user.should_see('Axis 1') + loop_thread = threading.current_thread().name + before = READS.copy() + await asyncio.sleep(1.2) # several binding refreshes and mirror syncs, every 10 Hz/1 Hz timer + assert READS[loop_thread] == before[loop_thread] + assert READS['odrive-usb_0'] > before['odrive-usb_0'] # the live values are still read, on the worker + + +async def test_a_refresh_that_read_before_an_edit_does_not_undo_it(user: User, mock_odrive) -> None: + """A refresh reads on the USB worker while the user may edit the same value: the read + sees the old device value, and applying it would snap the widget back -- whose change + handler then writes the old value to the device too. Holding the worker makes the + read-queued-before-the-write order deterministic.""" + config = mock_odrive.axis0.controller.config + mirror = _Mirror(config) + assert mirror.pos_gain == 20.0 # registers pos_gain for refreshes + gate = threading.Event() + _usb.submit(gate.wait) + sync = asyncio.create_task(mirror.sync()) + await asyncio.sleep(0.01) # the refresh's read is now queued behind the gate + mirror.pos_gain = 42.0 # and the write behind it + gate.set() + await sync + await asyncio.sleep(0.05) + assert mirror.pos_gain == 42.0 + assert config.pos_gain == 42.0 diff --git a/tools/mock_odrive.py b/tools/mock_odrive.py index 8e262c6..7333581 100644 --- a/tools/mock_odrive.py +++ b/tools/mock_odrive.py @@ -15,27 +15,46 @@ from __future__ import annotations +import collections import concurrent.futures import math +import os import sys +import threading import time import types _T0 = time.time() +# device attribute reads per thread name, so tests can tell which thread talks to the "bus" +READS: collections.Counter[str] = collections.Counter() +# a real read is a USB round-trip; MOCK_ODRIVE_READ_MS makes every mock read cost one too +READ_LATENCY = float(os.environ.get('MOCK_ODRIVE_READ_MS', '0')) / 1000 + def _wave(period: float, amp: float, offset: float = 0.0) -> float: return offset + amp * math.sin(2 * math.pi * (time.time() - _T0) / period) -class _Config: +class _Recorded: + """Counts every public attribute read (see ``READS``), each costing ``READ_LATENCY``.""" + + def __getattribute__(self, name: str): + if not name.startswith('_'): + READS[threading.current_thread().name] += 1 + if READ_LATENCY: + time.sleep(READ_LATENCY) + return object.__getattribute__(self, name) + + +class _Config(_Recorded): """A plain settable namespace -- NiceGUI bindings get/set attributes here.""" def __init__(self, **kw: object) -> None: self.__dict__.update(kw) -class _CurrentControl: +class _CurrentControl(_Recorded): Iq_setpoint = 0.4 Id_setpoint = 0.0 @@ -52,7 +71,7 @@ def v_current_control_integral_q(self) -> float: return _wave(3.0, 0.2, 1.1) -class _Thermistor: +class _Thermistor(_Recorded): """The FET thermistor, mirroring ``odrv.axisN.motor.fet_thermistor``. Exposes only ``.temperature`` — deliberately its own class rather than aliasing the @@ -66,18 +85,18 @@ def temperature(self) -> float: return _wave(20.0, 4.0, 32.0) -class _MockAxis: +class _MockAxis(_Recorded): def __init__(self, calibrated: bool = True, control_mode: int = 2) -> None: self.error = 0 self.requested_state = 1 self.current_state = 1 - self.motor = types.SimpleNamespace( + self.motor = _Config( is_calibrated=calibrated, current_control=_CurrentControl(), # lives on the motor only, as on hardware config=_Config(current_lim=10.0, current_control_bandwidth=1000.0, torque_lim=float('inf'), requested_current_range=60.0), fet_thermistor=_Thermistor(), ) - self.controller = types.SimpleNamespace( + self.controller = _Config( input_torque=0.0, input_vel=0.0, input_pos=0.0, @@ -98,13 +117,13 @@ def __init__(self, calibrated: bool = True, control_mode: int = 2) -> None: ), ) self.encoder = _Encoder() - self.trap_traj = types.SimpleNamespace(config=_Config(vel_limit=2.0, accel_limit=0.5, decel_limit=0.5)) + self.trap_traj = _Config(config=_Config(vel_limit=2.0, accel_limit=0.5, decel_limit=0.5)) def clear_errors(self) -> None: self.error = 0 -class _Encoder: +class _Encoder(_Recorded): """Encoder with a settable config and animated position/velocity estimates.""" def __init__(self) -> None: @@ -128,7 +147,7 @@ def make_mock_odrive(serial: int = 0x208E39855253, two_axes: bool = True, contro ax0 = _MockAxis(calibrated=True, control_mode=control_mode) ax1 = _MockAxis(calibrated=two_axes, control_mode=control_mode) - class _Dev: + class _Dev(_Recorded): serial_number = serial hw_version_major, hw_version_minor, hw_version_variant = 3, 6, 56 fw_version_major, fw_version_minor, fw_version_revision = 0, 5, 6 @@ -264,7 +283,7 @@ def arrive_mid_scan(during: object, arriving: object) -> dict: def read_once(self) -> int: type(self).serial_number = serial # fire on the first read only - scan = sys._getframe(1) # the frame whose read sprang the trap + scan = sys._getframe(2) # the frame whose read sprang the trap (1 is _Recorded.__getattribute__) report['fired_mid_scan'] = scan.f_code.co_name == 'discovery_loop' and serial not in scan.f_globals.get('devices', {}) set_connected_devices([during, arriving]) return serial From 6dce88289605366e895668a034861f941d2c5438 Mon Sep 17 00:00:00 2001 From: evnchn Date: Sun, 25 Oct 2026 23:05:49 +0800 Subject: [PATCH 2/5] Recognise a lost device by the classes the device actually uses `import odrive` loads libfibre twice: as `odrive.pyfibre.fibre` and, via the path it puts on sys.path, as top-level `fibre`. The device objects come from the top-level copy, so their ObjectLostError and EmptyInterface are other classes than the ones the GUI checked. On real hardware every save and reboot (both drop the device) therefore logged a traceback, and the lost-object checks never matched. Check both copies' classes. The mock now has a separate top-level `fibre` with its own classes, as the real package does, so the tests see this. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/controls.py | 14 ++++++++++---- src/main.py | 7 +++---- tests/test_main.py | 20 +++++++++++++++++++- tools/mock_odrive.py | 17 ++++++++++++++--- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/controls.py b/src/controls.py index 77b9942..0421170 100644 --- a/src/controls.py +++ b/src/controls.py @@ -15,6 +15,7 @@ import asyncio import itertools import logging +import sys from collections.abc import Callable from concurrent.futures import Future, ThreadPoolExecutor from datetime import datetime @@ -26,6 +27,11 @@ 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. +LOST_ERRORS = (fibre.ObjectLostError, sys.modules['fibre'].ObjectLostError) +EMPTY_INTERFACES = (fibre.libfibre.EmptyInterface, sys.modules['fibre'].libfibre.EmptyInterface) + MODES: dict[int, str] = { 0: 'Voltage', 1: 'Torque', @@ -57,7 +63,7 @@ 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 # Every device read or write is a blocking USB round-trip. After the panel is built, all of @@ -72,7 +78,7 @@ def _send(obj: Any, fn: Callable[[], Any]) -> None: def done(future: Future) -> None: e = future.exception() - if e is not None and not (isinstance(e, fibre.ObjectLostError) or _lost(obj)): + if e is not None and not (isinstance(e, LOST_ERRORS) or _lost(obj)): log.error('ODrive write failed', exc_info=e) _usb.submit(fn).add_done_callback(done) @@ -83,7 +89,7 @@ async def _on_usb(obj: Any, fn: Callable[[], Any]) -> Any: try: return await asyncio.get_running_loop().run_in_executor(_usb, fn) except Exception as e: - if isinstance(e, fibre.ObjectLostError) or _lost(obj): + if isinstance(e, LOST_ERRORS) or _lost(obj): return None raise @@ -133,7 +139,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) diff --git a/src/main.py b/src/main.py index 0ef6b2e..a4f37a4 100755 --- a/src/main.py +++ b/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 EMPTY_INTERFACES, LOST_ERRORS, controls from theme import apply_theme, header logging.basicConfig(level=logging.WARNING, format='%(asctime)s %(levelname)s %(message)s') @@ -40,9 +39,9 @@ def is_lost(e: Exception) -> bool: 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): + if isinstance(e, LOST_ERRORS): return True - return isinstance(e, AttributeError) and isinstance(e.obj, fibre.libfibre.EmptyInterface) + return isinstance(e, AttributeError) and isinstance(e.obj, EMPTY_INTERFACES) @ui.refreshable diff --git a/tests/test_main.py b/tests/test_main.py index 094a0e7..788f4cd 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -11,7 +11,7 @@ import threading import pytest -from mock_odrive import arrive_mid_scan, lose_mock_odrive, make_mock_odrive, set_connected_devices +from mock_odrive import arrive_mid_scan, lose_mock_odrive, make_mock_odrive, object_lost_error, set_connected_devices from nicegui import ui from nicegui.testing import User @@ -249,3 +249,21 @@ async def test_idle_is_requested_even_while_idle_is_the_state_shown(user: User) gate.set() await drain() assert [state for state, _ in seen] == [8, 1] + + +async def test_a_device_dropping_off_mid_save_or_reboot_is_not_an_error(user: User) -> None: + """This firmware reboots on save, and reboot drops the device: both calls then raise the + device's ``ObjectLostError``. That comes from ``odrive``'s second, top-level copy of + libfibre, not ``odrive.pyfibre.fibre``, and must not be logged as a failed write.""" + dev = make_mock_odrive(two_axes=False) + + def lost() -> None: + raise object_lost_error() + + dev.save_configuration = lost + dev.reboot = lost + set_connected_devices([dev]) + await user.open('/') + await user.should_see('Axis 0') + user.find(ui.button).trigger('click') # includes save and reboot; any ERROR log fails the test + await drain() diff --git a/tools/mock_odrive.py b/tools/mock_odrive.py index 7333581..0234a4d 100644 --- a/tools/mock_odrive.py +++ b/tools/mock_odrive.py @@ -214,6 +214,11 @@ def install_odrive_stub() -> None: fibre.ObjectLostError = type('ObjectLostError', (Exception,), {}) # type: ignore[attr-defined] fibre.libfibre = types.SimpleNamespace(EmptyInterface=type('EmptyInterface', (), {})) pyfibre.fibre = fibre + # ``import odrive`` also loads libfibre as top-level ``fibre``, and the device objects come + # from that copy, so its classes are not ``odrive.pyfibre.fibre``'s -- keep them distinct here. + device_fibre = types.ModuleType('fibre') + device_fibre.ObjectLostError = type('ObjectLostError', (Exception,), {}) # type: ignore[attr-defined] + device_fibre.libfibre = types.SimpleNamespace(EmptyInterface=type('EmptyInterface', (), {})) # type: ignore[attr-defined] utils = types.ModuleType('odrive.utils') utils.dump_errors = lambda odrv, clear=False: None # type: ignore[attr-defined] odrive.pyfibre = pyfibre # type: ignore[attr-defined] @@ -224,6 +229,7 @@ def install_odrive_stub() -> None: 'odrive.pyfibre': pyfibre, 'odrive.pyfibre.fibre': fibre, 'odrive.utils': utils, + 'fibre': device_fibre, } ) @@ -248,10 +254,15 @@ def lose_mock_odrive(device: object) -> None: """Make ``device`` behave like a fibre object whose USB connection was lost. Mirrors ``RemoteObject._destroy()``: the instance's class is swapped to - ``EmptyInterface``, so every attribute read from now on raises ``AttributeError``. + ``EmptyInterface`` (of the top-level ``fibre`` copy, as on hardware), so every attribute + read from now on raises ``AttributeError``. """ - fibre = sys.modules['odrive.pyfibre.fibre'] - device.__class__ = fibre.libfibre.EmptyInterface # type: ignore[attr-defined] + device.__class__ = sys.modules['fibre'].libfibre.EmptyInterface # type: ignore[attr-defined] + + +def object_lost_error() -> Exception: + """The ``ObjectLostError`` a real device raises when it drops off the bus mid-call.""" + return sys.modules['fibre'].ObjectLostError('the object disappeared') # type: ignore[attr-defined] def reset_odrive_stub() -> None: From 672342d4edf245b93345dcd0cdb54e8f9d8483e5 Mon Sep 17 00:00:00 2001 From: evnchn Date: Sun, 25 Oct 2026 23:11:10 +0800 Subject: [PATCH 3/5] Look up the top-level fibre with import_module, not sys.modules Reading sys.modules['fibre'] relies on `import odrive` having already imported it, which the pinned odrive range does not promise. Importing it is what odrive's own discovery thread does, so it always resolves to the copy the device objects come from. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/controls.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/controls.py b/src/controls.py index 0421170..6386705 100644 --- a/src/controls.py +++ b/src/controls.py @@ -13,9 +13,9 @@ from __future__ import annotations import asyncio +import importlib import itertools import logging -import sys from collections.abc import Callable from concurrent.futures import Future, ThreadPoolExecutor from datetime import datetime @@ -29,8 +29,9 @@ # ``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. -LOST_ERRORS = (fibre.ObjectLostError, sys.modules['fibre'].ObjectLostError) -EMPTY_INTERFACES = (fibre.libfibre.EmptyInterface, sys.modules['fibre'].libfibre.EmptyInterface) +_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', From d8583b7f07a36cd87f0b72fef9760cd946eeea29 Mon Sep 17 00:00:00 2001 From: Falko Schindler Date: Mon, 26 Oct 2026 15:55:18 +0100 Subject: [PATCH 4/5] Do not re-request the state the axis is already in, except Idle The state buttons sent every click. A click on Loop while the axis is already in closed loop therefore wrote CLOSED_LOOP to requested_state again, which makes 0.5.x firmware leave and re-enter closed loop: a brief disarm of a live motor. The old toggle dropped that click. Idle is still sent on every click, whatever the state shown. Any other state is only requested when the mirrored state differs. At worst a stale mirror swallows a Loop click, which starts nothing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WwBCFK9zGwtgjTpcB9sq4G --- src/controls.py | 10 ++++++++-- tests/test_main.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/controls.py b/src/controls.py index 6386705..033844b 100644 --- a/src/controls.py +++ b/src/controls.py @@ -50,6 +50,8 @@ 7: 'Mirror', } +IDLE = 1 + STATES: dict[int, str] = { 0: 'Undefined', 1: 'Idle', @@ -223,15 +225,19 @@ async def update_error() -> None: 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: every click must reach the + # 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. + # 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] diff --git a/tests/test_main.py b/tests/test_main.py index 788f4cd..ef4b97b 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -251,6 +251,23 @@ async def test_idle_is_requested_even_while_idle_is_the_state_shown(user: User) assert [state for state, _ in seen] == [8, 1] +async def test_a_click_on_the_state_the_axis_is_already_in_is_not_requested(user: User) -> None: + """Re-requesting CLOSED_LOOP for an axis already in it makes 0.5.x firmware leave and + re-enter closed loop (a brief disarm of a live motor), so a second click on Loop is + dropped. Idle is still requested, whatever the state shown.""" + dev = make_mock_odrive(two_axes=False) + dev.axis0.current_state = 8 + seen = _record_state_requests(dev.axis0, follow=False) + set_connected_devices([dev]) + await user.open('/') + await user.should_see('Axis 0') + assert shown_state(user) == 8 + pick(user, 8) + pick(user, 1) + await drain() + assert [state for state, _ in seen] == [1] + + async def test_a_device_dropping_off_mid_save_or_reboot_is_not_an_error(user: User) -> None: """This firmware reboots on save, and reboot drops the device: both calls then raise the device's ``ObjectLostError``. That comes from ``odrive``'s second, top-level copy of From e4b79d0e95e938fdef8af4f886cc10943d28df2b Mon Sep 17 00:00:00 2001 From: Falko Schindler Date: Mon, 26 Oct 2026 15:55:28 +0100 Subject: [PATCH 5/5] Recognise a lost device in worker reads through sub-objects _send and _on_usb only checked whether the object passed in was lost, but their reads go through sub-objects too: the power label reads the current controller, the plots read the encoder. libfibre tears a lost device down children first, on its own thread, so such a read could fail while the axis still looked alive, and the timer logged a traceback. Move is_lost from main.py next to the fibre classes in controls.py and use it there too. It recognises the AttributeError by the lost object it names, and there is one definition of "the device left the bus" again. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WwBCFK9zGwtgjTpcB9sq4G --- src/controls.py | 26 +++++++++++++++++++++++--- src/main.py | 17 +---------------- tests/test_main.py | 12 ++++++++++++ 3 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/controls.py b/src/controls.py index 033844b..9afaef5 100644 --- a/src/controls.py +++ b/src/controls.py @@ -69,6 +69,21 @@ def _lost(obj: Any) -> bool: 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. @@ -81,18 +96,23 @@ def _send(obj: Any, fn: Callable[[], Any]) -> None: def done(future: Future) -> None: e = future.exception() - if e is not None and not (isinstance(e, LOST_ERRORS) or _lost(obj)): + 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()`` 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 isinstance(e, LOST_ERRORS) or _lost(obj): + if is_lost(e) or _lost(obj): return None raise diff --git a/src/main.py b/src/main.py index a4f37a4..8ea7ffc 100755 --- a/src/main.py +++ b/src/main.py @@ -14,7 +14,7 @@ import odrive from nicegui import app, ui -from controls import EMPTY_INTERFACES, LOST_ERRORS, 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') @@ -29,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, LOST_ERRORS): - return True - return isinstance(e, AttributeError) and isinstance(e.obj, EMPTY_INTERFACES) - - @ui.refreshable def device_panels() -> None: """One full-width panel per connected device, or a placeholder if there is none. diff --git a/tests/test_main.py b/tests/test_main.py index ef4b97b..65bce0a 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -268,6 +268,18 @@ async def test_a_click_on_the_state_the_axis_is_already_in_is_not_requested(user assert [state for state, _ in seen] == [1] +async def test_a_sub_object_lost_before_its_axis_is_not_an_error(user: User) -> None: + """libfibre tears a lost device down children first, on its own thread, so a timer can + read through an already lost sub-object (the current controller for the power label) + while the axis it checks still looks alive. That read is a lost device, not a bug.""" + dev = make_mock_odrive(two_axes=False) + set_connected_devices([dev]) + await user.open('/') + await user.should_see('Axis 0') + lose_mock_odrive(dev.axis0.motor.current_control) + await asyncio.sleep(0.3) # several 10 Hz power-label updates + + async def test_a_device_dropping_off_mid_save_or_reboot_is_not_an_error(user: User) -> None: """This firmware reboots on save, and reboot drops the device: both calls then raise the device's ``ObjectLostError``. That comes from ``odrive``'s second, top-level copy of