diff --git a/.claude/skills/beckhoff-xml/SKILL.md b/.claude/skills/beckhoff-xml/SKILL.md index c7cb91c..43c58fc 100644 --- a/.claude/skills/beckhoff-xml/SKILL.md +++ b/.claude/skills/beckhoff-xml/SKILL.md @@ -285,6 +285,40 @@ Beckhoff bumps the `revision_number` on backward-compatible firmware `(vendor, product)` if the rig's revision differs from the cached XML's. Don't pin matching to all three components without a fallback. +### Multi-revision YAML entries (issue #60) + +Some terminals do rename PDOs across revisions (e.g. EP2338-0002 +renames the RxPDOs `Channel 1..8` → `Channel 9..16` at revision +`0x00120002`). A single YAML entry per product would silently +misname ADS symbols on rigs running the newer firmware. The fix is +to ship multiple entries for the same `(vendor, product)`, each +pinned to a different `revision_number`. + +Keying convention: +- The lowest-revision entry keeps the bare product ID (e.g. + `EP2338-0002`) — existing single-rig deployments don't need + renaming. +- Additional entries use the suffix `__rev<8 hex digits>` (lowercase + `rev`, no `@` — that character would flow into GUI DOM IDs in + `ui_components/tree_data_builder.py`). Example: + `EP2338-0002__rev00120002`. + +Lookup precedence in `get_terminal_type_by_identity`: +1. Exact `(vendor, product, revision)` match. +2. Among `(vendor, product)` candidates with + `revision_number ≤ rig revision`, pick the highest (the most + recent layout the rig is compatible with). +3. Otherwise pick the lowest-revision candidate as a degraded + fallback (the rig is older than every cached entry) and log a + warning. +4. Otherwise return `None`. + +When `clean-yaml` re-merges from XML, `service_file.py` strips the +`__rev...` suffix to find the ESI XML device, and passes the YAML +entry's pinned `revision_number` as `target_revision` so the parser +picks the right `` element. The PDO renames flow into +`channel_indices` on the affected `symbol_nodes`. + ## Subscription gating `CATioConnection.set_wanted_attribute_keys(...)` (called from the diff --git a/src/catio_terminals/models.py b/src/catio_terminals/models.py index 0458dc1..75f8edf 100644 --- a/src/catio_terminals/models.py +++ b/src/catio_terminals/models.py @@ -1,5 +1,6 @@ """Data models for terminal description YAML files.""" +import re from pathlib import Path from pydantic import BaseModel, Field, computed_field, model_validator @@ -36,6 +37,28 @@ def _coe_primitive_bit_size(type_name: str | None) -> int | None: return _COE_PRIMITIVE_BIT_SIZES.get(type_name.upper()) +# Issue #60: multi-revision YAML entries are keyed +# "__rev<8 hex digits>", lowercase ``rev``, no ``@`` (that +# character would flow into the GUI's DOM IDs in +# ui_components/tree_data_builder.py). The bare-id entry (no suffix) +# is the lowest cached revision for the product. +_REVISION_SUFFIX_RE = re.compile(r"__rev[0-9a-fA-F]{8}$") + + +def strip_revision_suffix(terminal_id: str) -> str: + """Strip the ``__revXXXXXXXX`` suffix from a multi-revision YAML key. + + Returns the bare terminal ID used to look up the ESI XML device. + Keys without the suffix pass through unchanged. + + >>> strip_revision_suffix("EP2338-0002") + 'EP2338-0002' + >>> strip_revision_suffix("EP2338-0002__rev00120002") + 'EP2338-0002' + """ + return _REVISION_SUFFIX_RE.sub("", terminal_id) + + def _coe_default_data_zero(bit_size: int | None) -> str | None: """Zero-valued ``default_data`` string for a given bit size, or None.""" if not bit_size: diff --git a/src/catio_terminals/service_file.py b/src/catio_terminals/service_file.py index 445e331..7afb6cb 100644 --- a/src/catio_terminals/service_file.py +++ b/src/catio_terminals/service_file.py @@ -6,7 +6,7 @@ from typing import TYPE_CHECKING from catio_terminals.beckhoff import BeckhoffClient -from catio_terminals.models import TerminalConfig +from catio_terminals.models import TerminalConfig, strip_revision_suffix if TYPE_CHECKING: from catio_terminals.models import TerminalType @@ -99,8 +99,13 @@ async def merge_xml_for_terminal( """ logger.debug(f"Loading XML for terminal: {terminal_id}") + # Issue #60: multi-revision entries are keyed + # "__rev<8 hex>" but the ESI XML only knows the bare ID. + # Strip the suffix before fetching/parsing. + xml_terminal_id = strip_revision_suffix(terminal_id) + # Fetch XML for this terminal - xml_content = await beckhoff_client.fetch_terminal_xml(terminal_id) + xml_content = await beckhoff_client.fetch_terminal_xml(xml_terminal_id) if not xml_content: logger.warning(f"No XML found for {terminal_id}") return False @@ -110,7 +115,7 @@ async def merge_xml_for_terminal( # Pass target_revision to match the terminal's existing revision target_revision = terminal.identity.revision_number xml_terminal, composite_types = beckhoff_client.parse_terminal_xml( - xml_content, terminal_id, terminal.group_type, target_revision + xml_content, xml_terminal_id, terminal.group_type, target_revision ) # Merge composite types into the config diff --git a/src/catio_terminals/terminals/terminal_types.yaml b/src/catio_terminals/terminals/terminal_types.yaml index 99a7d22..a5358a9 100644 --- a/src/catio_terminals/terminals/terminal_types.yaml +++ b/src/catio_terminals/terminals/terminal_types.yaml @@ -7474,6 +7474,40 @@ terminal_types: bit_offset: 0 coe_objects: [] group_type: DigOut + EP2338-0002__rev00120002: + description: 8 Ch. Dig. Input/Output 24V, 0,5A, M12 + identity: + vendor_id: 2 + product_code: 153239634 + revision_number: 1179650 + symbol_nodes: + - name_template: Channel {channel} + index_group: 61489 + type_name: InputBits + channels: 8 + access: Read-only + fastcs_name: channel_{channel} + selected: true + bit_offset: 0 + - name_template: Channel {channel} + index_group: 61473 + type_name: InputBits + channels: 8 + channel_indices: + - 9 + - 10 + - 11 + - 12 + - 13 + - 14 + - 15 + - 16 + access: Read/Write + fastcs_name: channel_{channel} + selected: true + bit_offset: 0 + coe_objects: [] + group_type: DigOut EP2624-0002: description: 4Ch. Relay Output, NO (125V AC / 30V DC) identity: diff --git a/src/fastcs_catio/terminal_config.py b/src/fastcs_catio/terminal_config.py index 632071a..530e2ec 100644 --- a/src/fastcs_catio/terminal_config.py +++ b/src/fastcs_catio/terminal_config.py @@ -199,20 +199,53 @@ def get_terminal_type_by_identity( ) -> TerminalType | None: """Look up a terminal definition by its EtherCAT identity. - Used by the bus-side symbol expansion. Beckhoff bumps the revision - number on backward-compatible firmware/silicon updates, so the - physical layout (the only thing #54 needs) stays the same. We - therefore prefer an exact match but fall back to vendor+product - when the rig is at a newer revision than the cached XML. + Used by the bus-side symbol expansion. Beckhoff usually bumps the + revision number on backward-compatible firmware/silicon updates, so + one cached YAML entry per product is enough. Occasionally though + (e.g. EP2338-0002 at rev 0x00120002) Beckhoff renames PDOs across + a revision, which fabricates wrong ADS symbol names if the rig is + matched against the wrong YAML entry. Issue #60 lets a product ship + multiple YAML entries pinned to different revisions; this lookup + picks the right one. + + Resolution order: + 1. Exact ``(vendor, product, revision)`` match. + 2. Among ``(vendor, product)`` candidates with ``revision <= rig``, + the one with the highest revision (most recent layout the rig is + compatible with). + 3. Any ``(vendor, product)`` candidate (degraded fallback for rigs + older than every cached entry) — logged as a warning. + 4. ``None``. """ config = load_terminal_config() - fallback: TerminalType | None = None - for terminal in config.terminal_types.values(): - ident = terminal.identity - if ident.vendor_id != vendor_id or ident.product_code != product_code: - continue - if ident.revision_number == revision_number: + candidates: list[TerminalType] = [ + t + for t in config.terminal_types.values() + if t.identity.vendor_id == vendor_id and t.identity.product_code == product_code + ] + if not candidates: + return None + + # 1. Exact match wins. + for terminal in candidates: + if terminal.identity.revision_number == revision_number: return terminal - if fallback is None: - fallback = terminal + + # 2. Highest revision <= rig (most recent compatible layout). + le_candidates = [ + t for t in candidates if t.identity.revision_number <= revision_number + ] + if le_candidates: + return max(le_candidates, key=lambda t: t.identity.revision_number) + + # 3. Degraded fallback: rig is older than every cached entry. Pick + # the lowest-revision candidate (closest to the rig) and warn. + fallback = min(candidates, key=lambda t: t.identity.revision_number) + logger.warning( + f"No cached YAML entry has revision <= rig revision 0x{revision_number:08x} " + f"for vendor 0x{vendor_id:x} product 0x{product_code:x}; " + f"falling back to YAML entry pinned at " + f"0x{fallback.identity.revision_number:08x}. " + "Symbol layout may not match the rig." + ) return fallback diff --git a/tests/test_symbol_expansion.py b/tests/test_symbol_expansion.py index 27fd0f9..bff85e1 100644 --- a/tests/test_symbol_expansion.py +++ b/tests/test_symbol_expansion.py @@ -513,6 +513,9 @@ def test_exact_match(self, install_terminal_config): assert get_terminal_type_by_identity(2, 10, 5) is t def test_vendor_product_fallback_on_revision_drift(self, install_terminal_config): + # Single-entry products still loose-match: rig at a newer revision + # than the cached YAML falls through to the only candidate (since + # its revision is <= the rig's). This preserves pre-#60 behaviour. t = TerminalType( description="EL", identity=Identity(vendor_id=2, product_code=10, revision_number=5), @@ -528,3 +531,64 @@ def test_no_match_returns_none(self, install_terminal_config): install_terminal_config({"EL": t}) assert get_terminal_type_by_identity(2, 11, 5) is None assert get_terminal_type_by_identity(3, 10, 5) is None + + def test_multi_revision_exact_match_wins(self, install_terminal_config): + a = TerminalType( + description="EL rev A", + identity=Identity(vendor_id=2, product_code=10, revision_number=5), + ) + b = TerminalType( + description="EL rev B", + identity=Identity(vendor_id=2, product_code=10, revision_number=20), + ) + install_terminal_config({"EL": a, "EL__rev00000014": b}) + # Exact match on A + assert get_terminal_type_by_identity(2, 10, 5) is a + # Exact match on B + assert get_terminal_type_by_identity(2, 10, 20) is b + + def test_multi_revision_picks_highest_below_rig(self, install_terminal_config): + a = TerminalType( + description="EL rev A", + identity=Identity(vendor_id=2, product_code=10, revision_number=5), + ) + b = TerminalType( + description="EL rev B", + identity=Identity(vendor_id=2, product_code=10, revision_number=20), + ) + install_terminal_config({"EL": a, "EL__rev00000014": b}) + # Rig at rev 15 (> A, < B) picks A (the highest <= 15). + assert get_terminal_type_by_identity(2, 10, 15) is a + # Rig at rev 99 (> B) picks B (the highest cached compatible rev). + assert get_terminal_type_by_identity(2, 10, 99) is b + + def test_multi_revision_rig_below_all_falls_back_with_warning( + self, install_terminal_config + ): + from fastcs.logging import logger as _fastcs_logger + + a = TerminalType( + description="EL rev A", + identity=Identity(vendor_id=2, product_code=10, revision_number=5), + ) + b = TerminalType( + description="EL rev B", + identity=Identity(vendor_id=2, product_code=10, revision_number=20), + ) + install_terminal_config({"EL": a, "EL__rev00000014": b}) + + # terminal_config uses fastcs's loguru logger, which doesn't feed + # pytest's caplog. Add a sink we can drain. + messages: list[str] = [] + sink_id = _fastcs_logger.add( + lambda msg: messages.append(str(msg)), level="WARNING" + ) + try: + # Rig at rev 1 is older than every cached entry. Degraded + # fallback: pick the lowest-revision YAML (closest) and warn. + result = get_terminal_type_by_identity(2, 10, 1) + finally: + _fastcs_logger.remove(sink_id) + assert result is a + joined = " ".join(messages).lower() + assert "falling back" in joined