Add a hardware-free naming API for static PV-name prediction - #65
Open
gilesknap wants to merge 3 commits into
Open
Add a hardware-free naming API for static PV-name prediction#65gilesknap wants to merge 3 commits into
gilesknap wants to merge 3 commits into
Conversation
At runtime the controller names are a side effect of ADS discovery: `_get_ethercat_chains` stamps each slave's ChainLocation and `_resolve_controller_name_and_path` renders it through CATioNameMappings. Both need a live bus. A tool migrating an existing installation onto fastcs-catio needs the same names *before* any hardware exists, so it can rewrite the PV references other IOCs hold. `predict_chain`/`predict_names` derive them from the chain order alone, keyed on terminal type names because that is what a static description of a chain carries, where the runtime keys on (vendor_id, product_code, revision_number) reported over ADS. Purely additive: catio_controller.py is untouched. tests/test_naming.py locks the output against the real runtime path over 6 chain shapes x 3 template shapes, so the two implementations cannot drift apart silently. Written for builder2ibek's `catio` command (epics-containers/builder2ibek#128), which imports predict_names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gui/inline-root-controllers #65 +/- ##
===============================================================
+ Coverage 75.91% 76.30% +0.39%
===============================================================
Files 19 20 +1
Lines 4197 4309 +112
===============================================================
+ Hits 3186 3288 +102
- Misses 1011 1021 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The helper added with these tests read the current event loop so it could put it back afterwards. `asyncio.get_event_loop()` is deprecated from Python 3.12, and `filterwarnings = "error"` turns that warning into an exception raised before the coroutine is ever awaited, so the file dies with "coroutine _get_ethercat_chains was never awaited". It only fires when no loop has been installed yet, which is why CI did not catch it: in a full run an earlier test has already installed one and the lookup returns it without warning. Running just this file -- what a developer does -- fails on 3.12 and 3.13. `new_event_loop()` does not install a loop and `run_until_complete` drives the loop object directly, so nothing needs saving or restoring. That keeps the original property the helper existed for (not clearing the loop test_system.py installs for its ADS simulator, whose sockets would then be collected unclosed and reported against an unrelated test) without reading global asyncio state at all. Verified on 3.11, 3.12 and 3.13, both for this file alone and for the full suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`dox_current_output_current` was the only unselected symbol anyone actually uses. It is the terminal's whole point -- an EL2595 is an LED constant current driver and the output current is the value you set -- while the two symbols that were selected, `dox_status` and `dox_control`, are packed bitfields. DLS's legacy `ethercat` module exposes it as `$(DEVICE):DOXCURRENT:OUTPUTCURRENT` and BL21I writes to it on four terminals (`BL21I-DI-LED-01/02/03` and `BL21I-OP-LED-01`). Without this the migration onto fastcs-catio has nothing to point those references at, because no PV is created for the attribute at all. Hand-edited rather than regenerated, by the maintainer's explicit decision: this is a single boolean selection, exactly what the `catio-terminals edit` GUI toggles, not an import of new Beckhoff XML. Verified as the only selection change in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
fastcs_catio.naming—predict_chain()/predict_names()— which derivethe PV names an EtherCAT chain will get without a bus, a controller, or an
event loop.
No open issue covers this; it is driven by a downstream consumer,
epics-containers/builder2ibek#128.
Why
At runtime the controller names are a side effect of ADS discovery:
FastCSClient._get_ethercat_chainsstamps each slave'sChainLocation, andCATioServerController._resolve_controller_name_and_pathrenders it throughCATioNameMappings. Both need live hardware.A site migrating an existing installation onto fastcs-catio needs those same
names before any hardware exists — other IOCs hold PV references that must be
rewritten ahead of the switchover. DLS is doing exactly this: replacing the
legacy
ethercatscanner beamline by beamline, where the new names differ fromthe old ones and every consumer IOC's database has to be migrated in step.
What it does, and what it deliberately does not
Purely additive.
catio_controller.pyandclient.pyare untouched; theruntime path does not change. The diff is two new files.
Keyed on terminal type names (
"EL3104"), not on(vendor_id, product_code, revision_number). A static description of a chain isall a migration tool has — DLS builder XML carries
type_rev="EL3104 rev 0x00120000"and nothing else — whereas the runtime keys on the identityreported over ADS. Looking up by type name is revision-independent, which
mirrors the vendor+product fallback
get_terminal_type_by_identityalreadyapplies when a rig runs newer firmware than the cached description.
strict=TrueraisesUnknownTerminalTypeErrorfor a terminal missing fromterminal_types.yaml. The runtime is deliberately forgiving here — anunrecognised identity renders as
MOD<position>— but that is the wrong defaultfor a migration tool, which would then rewrite live PV references to names the
IOC never creates. Callers generating names for real hardware should ask for the
error;
strict=Falsereproduces the runtime's behaviour.The part worth reviewing most
tests/test_naming.pyis the anti-drift mechanism. 18 of its 27 tests drive thereal runtime code path —
_get_ethercat_chains,_generate_system_tree,_compute_module_alias_indices,_resolve_controller_name_and_path— over 6chain shapes × 3 template shapes, and assert
predict_names()agrees. Areimplementation that silently drifts from the runtime is the obvious failure
mode for a module like this, so the tests are written to break in this repo,
where the change that caused the drift lives, rather than downstream.
One deliberate oddity: the helper uses a private event loop and restores the
previous one, rather than
asyncio.run().asyncio.runclears theprocess-wide loop on exit, which strands the loop
tests/test_system.pyinstalls for its ADS simulator — that loop and its sockets are then collected
unclosed and pytest raises
PytestUnraisableExceptionWarningagainst anunrelated test. It is invisible when the file runs alone and only shows up in a
full-suite run, so please don't simplify it back.
Base branch — please read before merging
This targets
gui/inline-root-controllers(#63), notmain, becausenaming.pyneedsCATioNameMappingsandget_terminal_type(...).group_alias,and a tree-wide grep of
main'ssrc/returns zero hits for either. Againstmainit does not merely misbehave, it fails at the import line. Both arrivewith #63.
If #63 merges first, retarget this to
main— it should need no other change.Instructions to reviewer on how to test:
uv run pytest— expect 267 passed, 23 skipped (240 on the base branch,plus 27 new tests and 1 new doctest).
uv run pyright src tests— expect 0 errors.git diff gui/inline-root-controllers --stat— confirm only two files areadded and nothing existing is modified.
predict_namesdoctest for the shape of the output:a
(node, position) -> PV prefixmapping.Checks for reviewer