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 69553586..78b7fd81 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 @@ -1,11 +1,34 @@ -# OMNY open issues (as of 2026-08-26) +# OMNY open issues (as of 2026-09-02) -Tracks what the first OMNY live-smoke-test session found: bugs (fixed and unfixed) and -what's still untested. Written so a future session can pick this up without re-deriving -it. Companion docs: `../../flomni/AI_docs/SIMULATED_ENDSTATIONS.md` (the sim architecture -and the fullest write-up of the rotation-offset bug below) and -`../../flomni/AI_docs/FLOMNI_TO_LAMNI_COMPARISON.md` (the flomni→lamni migration this -OMNY work is meant to mirror). +Tracks the live-testing effort for bringing OMNY up to flomni's/BEC's current standards on +branch `update-omny`: bugs found (fixed and unfixed) and what's still untested. Written so a +future session can pick this up without re-deriving it. Companion docs: +`../../flomni/AI_docs/SIMULATED_ENDSTATIONS.md` (the sim architecture) and +`../../flomni/AI_docs/FLOMNI_TO_LAMNI_COMPARISON.md` (the flomni→lamni migration this OMNY work +is meant to mirror). + +## Current status + +A full `omny_fermat_scan` (via `omny.tomo_scan_projection()`) now runs end to end against +`simulated_omny.yaml` without crashing — rotation, feedback, and the scan itself all complete. +Getting there took several rounds of live-tested fixes; see the dated sections below for the +full trace of each. GUI flows (`omny.omnygui_show_progress()`, `omnygui_show_xeyealign()`, +`omnygui_show_omnycam_parking()`/`omnygui_show_omnycam_samplestage()`) also work now, ported onto +flomni's current `gui_tools.py` API. + +**Not yet started**: testing the advanced tomo modes (golden ratio, 4/8 sub-tomogram) added +earlier this branch — this is the next thing being tested live. `holler` (Mirko's account) was +added to `_SUPERUSER_ACCOUNTS` (`omny.py`) alongside `gac-x01dc` so `tomo_parameters()` shows the +advanced options when testing from his own machine, where `bec.active_account` is unset and the +superuser check falls back to the OS login. + +**Still open / deferred** (unchanged from before, not touched this pass): +- `x_ray_eye_align.py` is still LamNI-derived, unadapted code beyond the two call-site renames + that unblocked the GUI-open crash — see its own section below. The GUI itself opens fine now; + the alignment *logic* underneath it is still wrong for OMNY. +- The full gripper sample-transfer chain (`otransfer_put_sample()`/`otransfer_get_sample()`), + dewar/temperature methods, sample storage bookkeeping, and `omny_alignment_mixin.py`/ + `omny_optics_mixin.py` beyond what scans exercise — see "Checklist" section below. ## Standing question: OMNY should follow flOMNI, not LamNI @@ -18,41 +41,42 @@ like LamNI, and is a general lens to apply going forward: if fixing an OMNY bug choosing what the "correct" shape looks like, default to whatever flOMNI already does, not whatever LamNI does — a LamNI-shaped OMNY file is more likely a leftover copy-paste than a deliberate design choice. See the `x_ray_eye_align.py` finding below for a concrete -example already found this way. +example already found this way, and the `omny_fermat_scan` rotation-crash section further +down for a case where following flomni's exact pattern was the fix. -## Bug found, NOT fixed — needs Mirko's call before touching it +## `osamroy` rotation offset — resolved (real hardware code was correct; only the simulator was wrong) -**`osamroy` (rotation axis) permanently fails every `.move()`/`umv()` call.** Full -technical write-up: `SIMULATED_ENDSTATIONS.md`'s OMNY live-test section. Short version: -`OMNYGalilReadbackSignal._socket_get()` (`devices/omny/galil/ogalil_ophyd.py:56-60`) adds -a documented `+25`° offset on *read* only ("we introduce an offset of 25 to the rotation -axis... that way we just do tomography from 0 to 180 degrees"). Nothing subtracts it back -out on the *write* side (`OMNYGalilMotor.move()`, same file) or in that method's own -success check, so `readback (= raw + 25)` is compared against the raw target passed to -`.move()` — always off by exactly 25, for any target. This is real, unmodified hardware- -facing code (not a sim artifact — the sim just relays it faithfully), and it -unconditionally blocks `feedback_enable_with_reset()` (`rt_omny_ophyd.py:526-533`), which -blocks every scan needing interferometer feedback, i.e. effectively all of them. +Originally logged here as an unresolved, hardware-review-only question: whether +`OMNYGalilMotor.move()` (`devices/omny/galil/ogalil_ophyd.py`) or `feedback_enable_with_reset()` +(`rt_omny_ophyd.py`) had the wrong sign/space for the documented `+25`° read-side offset on the +rotation axis (`OMNYGalilReadbackSignal._socket_get()`), since nothing appeared to subtract it +back out on the write side. **Resolved once the real Galil DMC controller source became +available** (`controller1.dmc`, `contrID=3` handler): the real controller firmware already +applies `ntarget=ntarget-25` on write for this axis — `OMNYGalilMotor.move()` was never buggy. +Only the **simulator** (`csaxs_bec/devices/sim/sim_galil.py`) was missing this, silently passing +through raw un-offset moves. Fixed in `SimGalilState.start_move_from_newpar()`: applies the same +`-25` when the port/axis matches the OMNY rotation stage, mirroring the real controller exactly. +Also seeded `osamroy`'s `sim_initial_position: -25` in `simulated_omny.yaml` so a fresh sim +session starts at displayed 0° instead of 25° (round-trips through the same offset via +`OMNYGalilReadbackSignal`'s `+25` read-side conversion). -Two plausible fixes, and only Mirko can say which matches real hardware intent: -1. `OMNYGalilMotor.move()` should convert the caller's display-space target to raw - (subtract 25) before sending it over the wire, for this axis — i.e. `.move(0)` means - "go to displayed zero." -2. Callers are expected to already pass raw targets, in which case - `feedback_enable_with_reset()`'s `osamroy.obj.move(0, wait=True)` and its - `np.fabs(readback) > 0.1` guard (which reads *display*-space) are the ones using the - wrong space, and should target/compare against `-25` instead of `0`. +Separately, and unrelated to this offset: `omny_rotation()`'s own `.set()` call had a different, +real bug (targeting `osamroy.user_setpoint` instead of the root `osamroy` device), which caused a +readback-progressbar crash later in the scan. See the `omny_fermat_scan` section below — already +fixed, do not confuse with this offset issue. -Do not guess-fix this one — a wrong sign/direction on a real rotation-stage offset is the -kind of thing that should get hardware-review, not silent inference from code reading. - -## `x_ray_eye_align.py` — worse than the crash that got fixed +## `x_ray_eye_align.py` — GUI now opens, underlying logic still wrong The construction-time crash (`self.lamni.align.reset_correction()` called on an object still mid-construction) is fixed (matches the fix LamNI's own copy of this file already -got — see its `__init__` for the rationale comment). But that only unblocks -*construction*. The rest of `omny/x_ray_eye_align.py` is still an **unadapted copy of -`LamNI/x_ray_eye_align.py`**, not code actually written for OMNY: +got — see its `__init__` for the rationale comment). The two GUI-open call sites +(`self.lamni.lamnigui_show_xeyealign()`/`lamnigui_show_xeyealign_fittab()` — LamNI-only method +names that don't exist on `OMNYGuiTools`) were also fixed, renamed to +`omnygui_show_xeyealign()`/`omnygui_show_xeyealign_fittab()`, so `omny.align.align()` now gets +past the GUI-open step without crashing. + +The rest of `omny/x_ray_eye_align.py` is still an **unadapted copy of `LamNI/x_ray_eye_align.py`**, +not code actually written for OMNY: - Docstring: `"""BEC-GUI-based X-ray eye alignment for LamNI. ... The key difference from the FlOMNI alignment is that LamNI needs *both* x and y fits..."""` — describes LamNI vs flOMNI, doesn't mention OMNY at all. @@ -62,38 +86,36 @@ got — see its `__init__` for the rationale comment). But that only unblocks - `LAMNI_ALIGNMENT_ANGLES = [k * 45 for k in range(8)]` (8 angles, full 360°, laminography- specific) — presumably wrong for OMNY, which (per the flOMNI/LamNI comparison doc) is the kind of full-rotation tomography flOMNI's alignment already handles differently. +- References devices/methods (`lsamrot`, `lfzp_in`, `omnyfsh`, `omny_xray_gui`) that mix LamNI + and OMNY naming conventions — needs a careful pass against OMNY's actual device inventory. -None of this was exercised in the live smoke test beyond the constructor no longer -crashing — `start_x_ray_eye_alignment()` / `xrayeye_update_frame()` were never called. -Given the "OMNY ≈ flOMNI, not LamNI" principle above, the right fix is almost certainly to -port OMNY's version from `flomni/x_ray_eye_align.py` (440 lines, the flOMNI-specific -implementation) the way `LamNI/x_ray_eye_align.py` was itself adapted from flOMNI's, -rather than patching the current LamNI-derived file in place. +None of the alignment logic itself has been exercised live yet — only the constructor and the +GUI-open step. Given the "OMNY ≈ flOMNI, not LamNI" principle above, the right fix is almost +certainly to port OMNY's version from `flomni/x_ray_eye_align.py` (440 lines, the flOMNI-specific +implementation) the way `LamNI/x_ray_eye_align.py` was itself adapted from flOMNI's, rather than +patching the current LamNI-derived file in place. Not started. -## Checklist items not yet run (smoke test only covered 3 of 4) +## Checklist items not yet run -From the live session against `simulated_omny.yaml` / `bec --session omny`: -- **Passed**: config load, a translation-axis move (`osamx`, `osamy`), one lightweight - transfer (`omny.otransfer_park_slot(6)`). -- **Blocked**: any scan (see rotation-offset bug above) — includes `omny.tomo_scan()` - itself, never reached. +- **Passed** (live, against `simulated_omny.yaml` / `bec --session omny`): config load, + translation-axis moves (`osamx`, `osamy`), a lightweight transfer (`omny.otransfer_park_slot(6)`), + `feedback_enable_with_reset()`, a full `omny_fermat_scan` via `omny.tomo_scan_projection()` + end to end, and the GUI flows listed under "Current status" above. -Still completely untested (not attempted yet, not known-broken — just unexplored): +Still untested (not attempted yet, not known-broken — just unexplored): +- **Advanced tomo modes** (golden ratio, 4/8 sub-tomogram, `tomo_parameters()`'s superuser-gated + options) — about to be tested next. - The **full gripper sample-transfer chain**: `otransfer_put_sample()` / `otransfer_get_sample()` (the actual pin pickup/dropoff with the `confirm`/`atconfir` - handshake), as opposed to the lighter `otransfer_park_slot()` used for the smoke test. -- **X-ray eye alignment** end-to-end (blocked in practice by the section above, but worth - retesting once that file is properly ported). + handshake), as opposed to the lighter `otransfer_park_slot()` used so far. +- **X-ray eye alignment** end-to-end alignment logic (GUI now opens fine, see section above — + the actual click-through alignment procedure itself hasn't been exercised). - **Dewar / temperatures** (`omny_dewar`, `omny_temperatures` devices) — they load and construct (`SimOMNYDewar`/`SimOMNYTemperatures`), but no `omny.py`-level method that - reads/uses them was called. + reads/uses them has been called live. - **Sample storage** (`omny_samples`) beyond it loading with seeded slots — no `omny.py`-level storage-bookkeeping method exercised. -- `omny_alignment_mixin.py`, `omny_optics_mixin.py`, and the rest of `omny_rt.py` beyond - `feedback_enable_with_reset`/`feedback_disable`/`feedback_status` — none of these were - called. -- GUI-dependent flows (`omnygui_show_gui()` etc.) — the smoke test ran `--nogui`; nothing - that needs a real BEC-widgets window was exercised. +- `omny_alignment_mixin.py`, `omny_optics_mixin.py` beyond what a fermat scan's own setup touches. ## Known, already-documented, separate from this session diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/omny.py b/csaxs_bec/bec_ipython_client/plugins/omny/omny.py index b0df048f..35368497 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/omny.py +++ b/csaxs_bec/bec_ipython_client/plugins/omny/omny.py @@ -46,9 +46,12 @@ class OMNYError(Exception): # modes (golden ratio, 4/8 sub-tomogram) in OMNY.tomo_parameters(). Regular users only # ever see "1: 2 equally spaced sub-tomograms". active_account is a Redis-backed # pgroup value, set explicitly via `bec-set-account ` -- it is NOT derived -# from the OS login, so testing as this account requires running -# `bec-set-account gac-x01dc` first (or wiring pgroup "gac-x01dc" some other way). -_SUPERUSER_ACCOUNTS: frozenset[str] = frozenset({"gac-x01dc"}) +# from the OS login, so testing as one of these accounts requires either running +# `bec-set-account ` first, or (dev/test sessions where active_account is +# unset) simply being logged in as that OS user, since _is_superuser_account() +# falls back to os.getenv("USER") when active_account is blank. "holler" is +# Mirko's account, used for live testing this branch. +_SUPERUSER_ACCOUNTS: frozenset[str] = frozenset({"gac-x01dc", "holler"}) class OMNYInitStagesMixin: