Drive ADS symbol expansion from terminal YAML (closes #54, #53)#59
Open
gilesknap wants to merge 23 commits into
Open
Drive ADS symbol expansion from terminal YAML (closes #54, #53)#59gilesknap wants to merge 23 commits into
gilesknap wants to merge 23 commits into
Conversation
Collapse the two parallel pipelines that decided what symbols existed: delete the hardcoded AdsSymbolTypePattern LUT in symbols.py and drive bus-side expansion from the terminal YAML. The XML parser now records each entry's bit_offset within its parent struct on the SymbolNode it emits, so the runtime can compute every sub-symbol's ADS offset without a per-struct case-arm. selected: false rows are no longer subscribed. Also drops the _unreferenced_ carve-out (closes #53) since the seam it papered over no longer exists, and updates the beckhoff-xml skill to remove the "two things" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Beckhoff bumps revision numbers on backward-compatible firmware/silicon updates while keeping the PDO layout identical. The rig at 172.23.242.39 hit identity-not-found warnings because its EL3314 is at rev 0x180000 while the cached XML produced a YAML pinned to 0x100000. Match on (vendor_id, product_code) when no exact revision match exists — same physical layout, same bit_offsets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EA.md and index.md are generated by the IOC at runtime as FastCS attribute dumps in the working directory. They snuck in via `git add -A` in the previous commit. Add them to .gitignore so this doesn't recur. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ADS addresses are byte-granular. YAML rows with `bit_offset % 8 != 0` (e.g. EL3104's `.Status__Limit 1` at bit 2, EL3314's `.Limit 1` at bit 2) represent bit fields within a parent status byte and can't be subscribed as standalone notifications — the ADS server rejects them with ADSERR_DEVICE_SYMBOLVERSIONINVALID, crashing IOC startup. The data is still available via the parent row's subscription; reading individual bit fields is a FastCS-attribute-layer concern, not a bus-side one. Match the old LUT's behavior by simply not emitting AdsSymbols for non-byte-aligned rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bare parent row of a struct terminal (e.g. EL3314's
`TC Inputs Channel {channel}` with children `.Value`, `.Limit 1`) has
type metadata sized for only the bit-collapsed status (typically 1
byte) but the bus reports the full parent struct size (4 bytes for
AI16/TC). Subscribing succeeds but the notification stream parser then
hits `assert symbol.nbytes == sample.size` (no message) in
messages.py, surfacing as "Notification flushing error:" with an empty
detail and breaking the flush loop.
Only emit AdsSymbols for "leaf" YAML rows — rows whose name_template
isn't a strict prefix of any other row's. Sub-fields cover the data
the user actually cares about; the bit-field bits that lived only in
the bare parent are not yet exposed as PVs (and weren't in the old
LUT path either for terminals like EL3314).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bare 'assert symbol.nbytes == sample.size' in messages.py (both get_notification_dtype and get_combined_notifications_dtype) and 'assert streams_dtype.fields' in client.py fire with empty AssertionError detail, producing log lines like "Notification flushing error:" with nothing useful after the colon. Add messages that name the symbol and the conflicting sizes so the next failure is diagnosable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_dtype_for_type_name was treating _DTYPE_MAP's element byte size as the element count, so e.g. type INT (2 bytes per int16) emitted AdsSymbol(dtype=int16, size=2) → nbytes=4, while the server reported size=2 for the actual symbol. The notification stream then failed the sample-size assertion on every flush. For scalar primitives, count is always 1; only the ARRAY [a..b] OF X branch uses an element count derived from the array bounds. Fix the scalar branch to return count=1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The IOC was subscribing to every symbol the bus discovered, including master-device housekeeping like _SyncUnits._default_._unreferenced_.* that has no PV consumer. After the _unreferenced_ carve-out was removed in c65d2dd these notifications surfaced as per-cycle warnings. Pass the server controller's full attribute_map keys into the connection right after the map is built, then skip any AdsSymbol whose f"_{name}" doesn't appear in that key set when add_notifications runs. The connection falls back to subscribing everything if the filter is never set (preserves the pre-#54 behaviour for callers that bypass CATioServerController). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A YAML row whose name_template (after channel substitution) doesn't correspond to any bus-discovered symbol or parent struct silently disappeared from _ecsymbols. The PV still got created via the IOC path, but every read/write failed with "No match for controller N and ADS Symbol ...". Log a warning at expansion time so the gap is visible at startup instead of on first PV write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a YAML row's parent isn't on the bus and the terminal has dynamic PDO groups, the most likely cause is a mismatch between the YAML's selected_pdo_group and the rig's actual PDO configuration. EL3314 is the canonical example: YAML selects "with ColdJunction Compensation" but the rig runs in the default "Inputs only" mode, so TC Outputs Channel N never appears on the bus. Surface that hypothesis in the warning message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 73.91% 75.26% +1.35%
==========================================
Files 19 19
Lines 4129 4096 -33
==========================================
+ Hits 3052 3083 +31
+ Misses 1077 1013 -64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 tasks
symbols.py was the largest gap in #59's patch coverage (45%). The new tests exercise every entry point — _dtype_for_type_name, _find_parent_node, expand_symbols_for_slave (including the unselected/non-leaf/sub-byte/ multi-channel/missing-parent/dynamic-PDO-hint branches), expand_device_symbols (Inputs, Outputs, bare names, non-BIGTYPE skip), expand_primitive_node, and build_symbols_for_device end-to-end. Also covers the new get_terminal_type_by_identity exact-match, vendor+product fallback, and no-match paths in terminal_config.py. Coverage: symbols.py 45% → 99%, terminal_config.py 84% → 90%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Combo I/O terminals like EP4374-0002 number their AO channels 3 and 4
(continuing on from AI channels 1, 2). The parser was storing only
channels=N (count), and the four iterators that resolve {channel} all
walked range(1, N+1) — fabricating Channel 1/2 names that don't exist
on the bus and silently dropping the write path.
SymbolNode now carries channel_indices: list[int]; the parser
populates it from sorted(set(channel_nums)); the bus expander and IOC
PV creator walk it directly. Legacy YAMLs without the field stay
1-based via a model validator. to_yaml omits the field when it equals
the implicit default.
EP4374-0002 rev 0x00100002 is the only DLS-shipped terminal where
this matters for the YAML as written; #60 tracks multi-revision
support for the case where the same product code has different PDO
naming across firmware revisions (EP2338-0002, and now EP4374-0002's
own newer revisions where the PDO names also changed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The terminal-details panel now offers a dropdown of all firmware revisions present in the cached ESI XML for the selected terminal. Picking a different revision mutates `identity.revision_number` and re-runs `merge_xml_for_terminal`, then surfaces a toast summarising how many YAML rows were dropped, added unselected, or preserved. This unblocks the EP2338-0002 case (and any future Beckhoff PDO rename) by letting users pin the matching revision without hand-editing the YAML. While verifying the dropdown on the rig, the re-merge surfaced a long-standing idempotency bug in `merge_xml_for_terminal`: it forced `selected=True` on every entry that survived merge, which on a second pass promoted XML-only rows that the first pass had added unselected. The downstream effect was the IOC trying to read CoE 0x1011 (Restore default parameters, a write-only command CoE) and crashing on the empty response. Fix: - Drop the `yaml_sym.selected = True` / `yaml_coe.selected = True` force-assignments in both branches of `merge_xml_for_terminal`. - `TerminalConfig.from_yaml` now marks loaded CoE objects as `selected=True` so the round-trip through YAML preserves intent (`to_yaml` already filters unselected CoE out on save). Also pin EP4374-0002 to revision 0x00130002 so the channel-3/4 AO writes resolve against the renamed PDOs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Capture the merge-idempotency invariant and the to_yaml / from_yaml CoE-selection asymmetry that broke the rig on 2026-05-21, so future sessions touching the merge or YAML round-trip code don't re-introduce the bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EL3314's TC Input TxPDOs (#x1a00..#x1a03) are listed in both the `Inputs only` AlternativeSmMapping and `with ColdJunction Compensation`. The previous last-writer-wins reverse map in assign_symbols_to_groups let the second group claim those PDOs exclusively, leaving `Inputs only` with an empty symbol_indices list. Selecting the default group in the GUI therefore showed no symbols. Collect every group a PDO belongs to and append the symbol to each. The re-merged EL3314 YAML at rev 0x00180000 now populates both groups correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The startup EtherCAT chain dump showed each slave's terminal type and name but not its identity revision. Without the revision it's awkward to tell e.g. an EL3202-0010 from an EL3202-0020 (which share a ProductCode and only differ in the low word of RevisionNo), or to spot a firmware revision mismatch between the rig and the cached XML. Append `rev=0xHHHHLLLL` to each slave line — high word firmware, low word variant suffix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A command called clean-yaml shouldn't quietly throw away every CoE setting in the file — yet that's what running it without flags used to do. Hours of GUI configuration could vanish in one command. The repo's checked-in YAMLs all carry settings-range CoEs, so the no-flag output also didn't match how the file is actually used. Defaults flipped: - No flag: keep CoE in 0x8000-0x8FFF (was: strip everything). - --include-all-coe: keep every CoE from XML (unchanged). - --remove-coe: strip all CoE objects (the old default, now opt-in). - --include-coe is gone — equivalent to the new default. Print a one-line warning at the top of a --remove-coe run so the destructive path is loud. Verified: clean-yaml with no flags now produces byte-identical output to the previously-committed re-merge (which used --include-coe). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… skill Two new sections lift this session's lessons out of memory and into the skill so they surface automatically next time someone edits the XML parser or runs clean-yaml: - PDO groups can share PDOs across `AlternativeSmMapping` blocks (with the 8eb4247 last-writer-wins bug shape and the regression test that nails the invariant). - `clean-yaml` selection contract: which symbols get selected after re-merge, the new CoE flag scheme (default keeps 0x8000-0x8FFF, --include-all-coe / --remove-coe), and the empty-diff verification trick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bare-struct parent rows (like EL3314's `TC Inputs Channel {channel}`)
whose name_template is a strict prefix of a sibling's were producing
orphan PVs: the FastCS attribute pipeline (driven by `selected`) created
them, but the bus-side `expand_symbols_for_slave` skipped them as
non-leaf — so the PV existed but never got a subscription.
Fix at the source: `_drop_non_leaf_parents` filters them out at the end
of `create_symbol_nodes` and remaps `symbol_index_to_pdo` so PDO group
assignments still bind to the surviving rows. Regenerating
`terminal_types.yaml` via `clean-yaml --all` removes 16 such rows across
12 terminals (-201/+38).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promotes the durable rule introduced by f6fbfbc into the skill: the non-leaf bare-struct filter now runs at XML→YAML generation as well as on the bus side. Bus-side gate stays in place as defense-in-depth against stale or hand-edited YAML. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CoESubIndex/CoEObject validators now fill fastcs_name, primitive bit_size, and (implicitly) all-zero default_data on load; to_yaml's new _clean_coe_for_yaml strips them on save only when stored == derived, so any future hand-edit survives the round-trip. Removes the now-dead fastcs_name patch-up in merge_xml_for_terminal and documents the convention in the beckhoff-xml skill. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Rebased / cleaner-history alternative to #56. Same target: collapse the two parallel pipelines #54 describes, with the cleanups #53 was a band-aid for.
Summary
AdsSymbolTypePatternLUT insrc/fastcs_catio/symbols.pywith a YAML-driven expander. For each bus-discoveredAdsSymbolNodewe look up the matching terminal by(vendor_id, product_code, revision_number)(with vendor+product fallback for revision drift) and emit oneAdsSymbolperselected: trueleaf YAML row atparent.offset + row.bit_offset // 8.SymbolNode.bit_offsetfield; XML parser (src/catio_terminals/xml/pdo.py) now records cumulative bit position per entry within its parent struct.attribute_mapmembership — the IOC only subscribes to symbols a PV consumes. Closes the seam Spurious "No reference to _SyncUnits._default_._unreferenced_.WcState.WcState" warnings on real hardware #53 papered over.Inputs/Outputsframe state (Frm0State, SlaveCount, ...) kept as a tiny built-in layout — not user-extensible, doesn't belong in YAML..claude/skills/beckhoff-xml/SKILL.mdupdated to drop the "two things" wording.EP4374-0002 follow-ups (latest pushes)
SymbolNode.channel_indices: list[int]so combo terminals like EP4374-0002 (AI on ch.1-2, AO on ch.3-4) can pin non-1-based channel numbering; parser populates it, expanders walk it, YAML row patched withchannel_indices: [3, 4].src/catio_terminals/ui_components/terminal_details.py). Picking a different revision re-runsmerge_xml_for_terminaland surfaces a toast with the drop/add/preserve counts. Lets users pin the right revision when Beckhoff renames PDOs across revisions, without hand-editing YAML. Partial step toward Support multiple firmware revisions of the same terminal in one YAML #60.merge_xml_for_terminalsurfaced by the revision dropdown: it force-setselected=Trueon every surviving entry, so a second merge promoted XML-only rows the first merge had added unselected — which silently selected CoE 0x1011 ("Restore default parameters", write-only command) and crashed the IOC at startup. Merge now preserves selection state;TerminalConfig.from_yamlmarks loaded CoEsselected=Trueto keep round-trips correct (regression tests intests/test_new_terminal_coe_selection.py).0x00130002(was0x00100002) to match the renamedAI Inputs Channel …/AO Outputs Channel …PDOs.Hardware verification
Verified against the rig at 172.23.242.39 (EK1100 + EK1110 + 3× EL3314). One hardware-only issue surfaced and was filed as #58 — the YAML's
selected_pdo_groupis per-terminal-class but the bus PDO config is per-slave-instance, so mixed-mode terminals on one chain need follow-up work.Also verified the revision-dropdown + AO-write fix against an EP4374-0002 on the rig.
Test plan
uv run pytest— 198 passed, 23 skipped.uv run ruff check,uv run pyright src tests— clean.0x00130002.Related
🤖 Generated with Claude Code