From bb5ff0cedf1d5ffa4e9d648d1b1e22460439c93a Mon Sep 17 00:00:00 2001 From: x01dc Date: Wed, 2 Sep 2026 04:28:50 +0200 Subject: [PATCH] fix(omny): rotate the root osamroy device, not its user_setpoint signal Live-reproduced (local BEC deployment, real client) that the readoutPriority fix (c37bfc7) didn't actually fix the readback-progressbar crash -- it just moved it to a different device (rt_positions instead of cam200), confirming the earlier diagnosis was incomplete. Real root cause: omny_rotation() called self.actions.set(self.dev.osamroy.user_setpoint, angle, wait=False), passing the user_setpoint Signal sub-component instead of the root device. ScanActions._normalize_device_name (bec_server) normalizes a sub-signal to its dotted name ("osamroy.user_setpoint"), which never matches the plain "osamroy" registered as requiring a response by the preceding add_scan_report_instruction_readback(devices=["osamroy"], ...) call. So the rotation's own completion is never reported, and the readback progressbar's request-status listener stays subscribed for the rest of the scan -- until post_scan()'s complete_all_devices() batches many unrelated owned devices into one instruction later, which DOES trigger a response (since the literal "osamroy" is also in that batch), reporting every device in it and crashing the still-alive progressbar on whichever one happens to complete first. Confirmed live this is omny-specific, not a flomni-shared exposure as previously assumed: flomni_fermat_scan's equivalent flomni_rotation() calls self.actions.set(self.dev.fsamroy, angle, wait=False) -- the whole root device -- so its own completion reports correctly and the progressbar exits cleanly well before complete_all_devices() runs. A live flomni_fermat_scan run with cam_xeye still async+enabled completed without incident, disproving the earlier "flomni has the same latent exposure" assumption. Fixed by matching flomni's pattern exactly. OMNYGalilMotor.move() (ogalil_ophyd.py) already does self.user_setpoint.put(...) plus proper completion bookkeeping internally, so setting the root device is a strict superset of the previous behavior, not a functional change to the move itself. The readoutPriority change from the previous commit is kept (still correct, matches flomni's camera convention) even though it wasn't the actual fix. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01QLrD7sVYGLAzsQjLVJpCgt --- .../plugins/omny/AI_docs/OPEN_ISSUES.md | 75 +++++++++++-------- csaxs_bec/scans/omny_fermat_scan.py | 4 +- tests/tests_scans/test_omny_fermat_scan.py | 11 ++- 3 files changed, 54 insertions(+), 36 deletions(-) diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md index 7e5b3243..0a192188 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md +++ b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md @@ -139,42 +139,53 @@ hardware use:** `sim_velocity`/`sim_initial_position` hand-additions elsewhere in this file, already present before this session). -## cam200-203 readoutPriority: async caused a scan crash (fixed, bec-core bug found along the way) +## omny_fermat_scan rotation crashed on an arbitrary device (readback progressbar) (fixed) -`omny.tomo_scan_projection(1)` -> `omny_fermat_scan` crashed with `ValueError: 'cam200' is not in -list` in `bec_ipython_client`'s `DeviceProgressBar.set_finished()`, once a scan actually did a -readback-tracked osamroy rotation (the first time this branch got that far). Root cause, traced -through bec_server + bec_ipython_client core code: +`omny.tomo_scan_projection(1)` -> `omny_fermat_scan` crashed with `ValueError: '' is +not in list` in `bec_ipython_client`'s `DeviceProgressBar.set_finished()`, once a scan actually did +a readback-tracked osamroy rotation. First seen as `cam200`; changing cam200-203/cam_xeye's +`readoutPriority` to `on_request` (below) did **not** fix it -- it just changed which device the +crash landed on (`rt_positions`, confirmed by live reproduction). The `readoutPriority` change is +still correct/kept (matches flomni's convention for view-only cameras), but it was treating a +symptom, not the actual bug. -- `omny_fermat_scan.py`'s `add_scan_report_instruction_readback(devices=["osamroy"], ...)` - registers `osamroy` as requiring a response. -- `post_scan()`'s `complete_all_devices()` batches **every enabled+claimable-owned device** into - one instruction -- including `cam200`, because `readoutPriority: async` + `enabled: true` default - to `ownership_mode: "claimable"`, and claimable async-priority devices are auto-acquired for any - scan **unless** their `readoutPriority` is `"on_request"` (`bec_server/.../scan_actions.py: - 1148-1188`). -- The batched instruction's `response=True` flag is set because *any* device in the batch needs a - response (`scan_actions.py:_send()`, not scoped per-device) -- so the server publishes a - `DeviceReqStatusMessage` for cam200 too, under the same RID the osamroy-only readback progressbar - is listening on. -- `ReadbackDataHandler.on_req_status()` (`bec_ipython_client/.../move_device.py`) filters by RID - only, not by device, so it records cam200's status even though this progressbar only tracks - `["osamroy"]`, and `DeviceProgressBar.set_finished()` crashes on `self.devices.index("cam200")`. +**Real root cause, confirmed via live reproduction (local BEC deployment, not just static +reading)**: `omny_fermat_scan.py`'s `omny_rotation()` called +`self.actions.set(self.dev.osamroy.user_setpoint, angle, wait=False)` -- passing the +`user_setpoint` **signal sub-component**, not the root device. `ScanActions._normalize_device_name` +(`bec_server/.../scan_actions.py:1106-1107`) normalizes a `DeviceBase` to its **dotted name** for a +sub-signal, i.e. `"osamroy.user_setpoint"`, while `add_scan_report_instruction_readback(devices= +["osamroy"], ...)` (line above) registers the plain string `"osamroy"` as requiring a response. +Since `{"osamroy.user_setpoint"} & {"osamroy"}` is empty, `_send()` never sets `response=True` on +the rotation's own instruction -- the device server never reports `"osamroy"` done, so the readback +progressbar's `ReadbackDataHandler` never sees its request finish and stays subscribed to that RID +for the rest of the scan. Later, `post_scan()`'s `complete_all_devices()` batches many unrelated +owned devices into one instruction; because the literal string `"osamroy"` *is* in that later batch +and still needs a response, `response=True` fires on the **whole batch**, and the device server +reports every device in it -- which the still-alive progressbar (only tracking `["osamroy"]`) +receives and crashes on, for whichever device happens to report first (arbitrary -- hence `cam200` +for Mirko, `rt_positions` in the live repro here). -**Fixed** by changing `cam200`/`cam201`/`cam202`/`cam203` from `readoutPriority: async` to -`on_request` in both `simulated_omny.yaml` and `ptycho_omny.yaml` (also applied to the `cam_xeye` -added earlier this session) -- `on_request` is explicitly excluded from the claimable/owned -auto-acquire set, and is also the semantically correct value for these view-only cameras (never -referenced by any scan code), matching flomni's own `cam_flomni_gripper`/`cam_flomni_overview` -convention. Live-mode GUI streaming (`start_live_mode()`) is unaffected -- it's a dedicated -background thread, not driven by readout-priority scheduling. +**Confirmed this is omny-specific, not a general/flomni-shared bug** (contrary to my earlier +assumption): `flomni_fermat_scan.py`'s equivalent `flomni_rotation()` calls +`self.actions.set(self.dev.fsamroy, angle, wait=False)` -- the whole root device, not a sub-signal +-- so `_normalize_device_name` returns `"fsamroy"`, matching the required-response registration +exactly. Live-reproduced: `flomni_fermat_scan` with `cam_xeye` still `readoutPriority: async` and +enabled completed cleanly (no crash), because the rotation's own completion reports correctly and +the progressbar exits well before `complete_all_devices()` ever runs. -**Not fixed, out of scope here**: this is a genuine bec-core bug (steps 3-4 above -- an -instruction-batch-scoped response flag combined with device-unfiltered status handling -client-side), not something specific to omny. flomni's own `cam_xeye` (`ptycho_flomni.yaml`, -`readoutPriority: async`, untouched by this session) has the exact same latent exposure and would -hit the same crash the first time a flomni scan does an actual (non-"already at angle") rotation -with cam_xeye enabled. Reported upstream as feedback; not something csaxs_bec can fix directly. +**Fixed** in `omny_fermat_scan.py`'s `omny_rotation()`: `self.actions.set(self.dev.osamroy, angle, +wait=False)`, matching flomni's pattern exactly. `OMNYGalilMotor.move()` +(`csaxs_bec/devices/omny/galil/ogalil_ophyd.py`) already does `self.user_setpoint.put(...)` plus +proper completion bookkeeping internally, so setting the root device is a strict superset of the +previous behavior, not a functional change to the move itself. + +**Not fixed, out of scope, reported upstream as feedback (defense-in-depth, framework-level)**: +`ReadbackDataHandler.on_req_status()` and `DeviceProgressBar.set_finished()` +(`bec_ipython_client`) should guard against/ignore a device outside their tracked list rather than +crash, and `scan_actions.py`'s `_send()` should scope the `response` flag per-device rather than +per-batched-instruction -- any scan with this same signal-vs-root-device mismatch pattern would hit +the same failure mode. ## Environment note (not code, but will bite again if forgotten) diff --git a/csaxs_bec/scans/omny_fermat_scan.py b/csaxs_bec/scans/omny_fermat_scan.py index 0a40d64b..660bd588 100644 --- a/csaxs_bec/scans/omny_fermat_scan.py +++ b/csaxs_bec/scans/omny_fermat_scan.py @@ -391,9 +391,7 @@ class OmnyFermatScan(ScanBase): self.actions.add_scan_report_instruction_readback( devices=["osamroy"], start=[osamroy_current_setpoint], stop=[angle] ) - self.omny_rotation_status = self.actions.set( - self.dev.osamroy.user_setpoint, angle, wait=False - ) + self.omny_rotation_status = self.actions.set(self.dev.osamroy, angle, wait=False) def prepare_setup_part2(self): if self.omny_rotation_status is not None: diff --git a/tests/tests_scans/test_omny_fermat_scan.py b/tests/tests_scans/test_omny_fermat_scan.py index a7187b55..f403fcad 100644 --- a/tests/tests_scans/test_omny_fermat_scan.py +++ b/tests/tests_scans/test_omny_fermat_scan.py @@ -23,7 +23,16 @@ def test_omny_rotation_moves_when_setpoint_differs(): scan.actions.set.assert_called_once() args, kwargs = scan.actions.set.call_args - assert args[0] is scan.dev.osamroy.user_setpoint + # Must target the root osamroy device, not the user_setpoint sub-signal: + # add_scan_report_instruction_readback(devices=["osamroy"]) registers the + # plain device name as requiring a response, but self.actions.set() on a + # Signal normalizes to a dotted name ("osamroy.user_setpoint") that never + # matches "osamroy" -- the rotation's own completion is never reported, + # leaving the readback progressbar's request-status listener alive until + # complete_all_devices() batches many unrelated devices together later + # and crashes on an arbitrary one of them (confirmed live; see + # omny/AI_docs/OPEN_ISSUES.md). + assert args[0] is scan.dev.osamroy assert args[1] == 10.0 assert kwargs.get("wait") is False