Skip to content

Read the device on a worker thread, not on the event loop - #16

Merged
falkoschindler merged 5 commits into
zauberzeug:mainfrom
evnchn:fix/poll-off-event-loop-upstream
Oct 26, 2026
Merged

falkoschindler merged 5 commits into
zauberzeug:mainfrom
evnchn:fix/poll-off-event-loop-upstream

Conversation

@evnchn

@evnchn evnchn commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

The GUI read the ODrive on the event loop: NiceGUI polls every widget bound to a fibre object every 0.1 s, and each read is a blocking USB round-trip. On a real board, main logs binding propagation … took 0.016–0.020 s on every tick. With this PR it logged one such warning in about 4 minutes.

Two things on main this also fixes, both found on the bench:

  • An Idle click could be lost. The state toggle drops a click on the option it already shows, and the state shown can be a refresh old. So: click Loop, a refresh still reads Idle, the firmware then enters Loop, and the click on Idle is never sent. The state is now three buttons that look the same, and every click is sent.
  • Save and reboot logged a traceback. import odrive loads libfibre twice (odrive.pyfibre.fibre and top-level fibre). Device objects come from the second copy, so the GUI's ObjectLostError and EmptyInterface checks never matched on hardware. It now checks both.

Would you rather have the libfibre fix as its own PR? It is the last two commits and stands alone.

Bench run: ODrive 3.6 clone, real USB, motor never energized

Board: ODrive 3.6 clone, FW 0.0.0 (dev), 19.96 V DC, both axes Idle throughout. Step 4 was skipped on purpose: this clone has no hardware overcurrent protection.

step result
read latency 0.328 ms per read (200 × Iq_measured)
page load, live values pass. One binding propagation warning (0.013 s) in about 4 min, where main logs 231 in about 37 s, one per tick at 16–20 ms each
write path pass. pos_gain 5.0 → 5.5 survived refreshes and Save (this firmware reboots on save); the panel came back reading 5.5 from the device; restored to 5.0
Loop/Idle on hardware skipped (see above); covered by the tests and a Chromium run against the mock
device drop partial: every save/reboot drops the device with the page open; app and page stayed up, panel back within ~1.5 s. USB was not physically unplugged
tracebacks on save/reboot main: logged, e.g. fibre.libfibre.ObjectLostError: the object disappeared from reboot(), then 'EmptyInterface' object has no attribute 'Iq_measured' from the power timer. Fixed by the libfibre commits
How the device is read now
  • Widgets bind to a small local mirror per fibre object instead of the fibre object.
  • One worker thread (_usb) refreshes the mirrors at 10 Hz. The power/voltage/error timers and the plot sampler read there too.
  • Every write and command (setting edits, state requests, motion inputs, save, reboot, clear errors) is queued on that worker in click order. So "lower current_lim, then Loop" reaches the device in that order.
  • A refresh that read a value before the user edited it does not overwrite the edit.
  • Unchanged: the reads made while a panel is built still happen on the loop, once per panel.
Tests: each new one fails without its fix
test without the fix
test_live_values_are_not_read_on_the_event_loop main: 604 device reads on the loop thread in 1.2 s
test_a_refresh_that_read_before_an_edit_does_not_undo_it fails with the write-order check removed
test_a_quick_loop_then_idle_is_not_dropped fails when clicks are compared with the cached state
test_a_command_does_not_overtake_a_setting_changed_before_it fails when commands bypass the worker queue
test_idle_is_requested_even_while_idle_is_the_state_shown the toggle version on main sends [8] instead of [8, 1]
test_a_device_dropping_off_mid_save_or_reboot_is_not_an_error ODrive write failed logged
test_lost_device_drops_only_its_panel (existing) fails once the mock's lost device uses the top-level EmptyInterface, as on hardware

The mock now has a separate top-level fibre with its own classes, like the real package. It also counts reads per thread and can give each read a latency (MOCK_ODRIVE_READ_MS).

The browser half of the lost Idle click (Quasar ignoring a click on the selected option) is outside NiceGUI's User simulation, so it was checked in Chromium against the mock, with a firmware that never switches: click Loop, wait 2.5 s, click Idle. main sent only 8; this branch sends 8, then 1.

Gates: ruff, ruff format, mypy clean; pytest 18 passed (repeated, also with 1 ms mock reads). src/main.py boots against the real locked odrive and serves the page.

Known limits
  • One worker for all devices: a hung device stalls every panel's updates, but the page stays responsive (before, it froze the app).
  • pyfibre is called from the worker while panel builds read on the loop. The bench showed no problem; that is not proof.
  • A failed write is logged; later commands are not held back, as before.
  • Each open browser tab polls on its own, as before.
Review

Five Codex passes on the worker change; each blocker was reproduced, then fixed with a test. They are the ordering, lost-click and stale-state cases in the test table. Last verdict: "suitable for supervised bench testing … 8/10". The bench above followed.

The libfibre fix was reviewed by GLM-5.3 (Codex was out of quota). It flagged an unguarded sys.modules['fibre'] lookup that could fail on older odrive releases; that is now importlib.import_module('fibre'), the same import odrive's discovery thread does. Its other high finding, that the new test asserts nothing, does not hold: NiceGUI's user fixture fails a test on any ERROR log, and the test fails with the fix reverted (ODrive write failed). .trigger('click') fires every visible button.

🤖 Generated with Claude Code

evnchn and others added 3 commits October 25, 2026 23:05
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@falkoschindler
falkoschindler self-requested a review October 25, 2026 15:42
@falkoschindler falkoschindler added this to the 0.3 milestone Oct 25, 2026
@falkoschindler falkoschindler added the bug Something isn't working label Oct 25, 2026
@falkoschindler falkoschindler self-assigned this Oct 25, 2026
falkoschindler and others added 2 commits October 26, 2026 15:55
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WwBCFK9zGwtgjTpcB9sq4G
_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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WwBCFK9zGwtgjTpcB9sq4G

@falkoschindler falkoschindler left a comment

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.

@evnchn Thanks, this is a clean way to get the USB traffic off the event loop. The bench run and a test for every change that fails without it make it easy to trust. Keeping the libfibre fix in this PR is fine, since it sits in its own commits.

One thing for later, not for this PR: sync_mirrors still re-reads every bound config value at 10 Hz, although only current_state needs to feel live. Syncing the config mirrors at about 1 Hz would remove most of the worker's load. That load is not new, because NiceGUI polled the same bindings before, so it can wait for a follow-up.

Two small fixes pushed to the branch
  • No repeated Loop request (d8583b7): The state buttons sent every click, so a click on Loop while the axis was already in closed loop wrote CLOSED_LOOP again. As your own comment notes, 0.5.x firmware then leaves and re-enters closed loop. Idle is still sent on every click, but any other state is now only requested when the mirrored state differs. At worst a stale mirror swallows a Loop click, which starts nothing.
  • Lost sub-objects (e4b79d0): _send and _on_usb only checked whether the object passed in was lost, but the power label reads the current controller and the plots read the encoder. libfibre tears a lost device down children first, so such a read could fail while the axis still looked alive and log a traceback. is_lost now lives in controls.py and is used there and in main.py, so there is one definition of "the device left the bus" again.

Both come with a test that fails without the fix.

※

@falkoschindler
falkoschindler merged commit 87c3f24 into zauberzeug:main Oct 26, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants