fix(omny): rotate the root osamroy device, not its user_setpoint signal
CI for csaxs_bec / test (push) Successful in 1m53s
CI for csaxs_bec / test (push) Successful in 1m53s
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QLrD7sVYGLAzsQjLVJpCgt
This commit is contained in:
@@ -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: '<some device>' 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)
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user