DiamondLightSource / DiamondLightSource/fastcs-catio

Loose-match identity lookup conflates suffix variants that share a ProductCode

Open
#61 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
1
Forks
0
PR merge metrics
No merged PRs in 30d

Description

## Problem

`get_terminal_type_by_identity` in `src/fastcs_catio/terminal_config.py:197-218` prefers an exact `(vendor, product, revision)` match but falls back to `(vendor, product)` if the revision differs. The fallback comment notes that Beckhoff bumps revision on backward-compatible firmware updates — true, but **incomplete**. Auditing the ESI cache against `terminal_types.yaml` shows the ProductCode is also shared across **suffix variants** (`-0010`, `-0020`, `-0030`, `-0090`, `-0092`, etc.), which are *not* firmware revisions of the same hardware — they are different products. The variant is encoded in the low word of `RevisionNo`.

| Type string | ProductCode | RevisionNo | low word |
|---|---|---|---|
| `EL3202` | 0xC823052 | 0x00100000 | 0x00 (base) |
| `EL3202-0010` | 0xC823052 | 0x0010000A | 0x0A → \"0010\" |
| `EL3202-0020` | 0xC823052 | 0x00110014 | 0x14 → \"0020\" |
| `EL3202-0030` | 0xC823052 | 0x0016001E | 0x1E → \"0030\" |

Same shape in every multi-variant family in the cache:

- **EL1004** family: base / `-0010` / `-0020` — all PC `0x03EC3052`.
- **EL3314** family: base / `-0002` / `-0010` / `-0030` / `-0090` / `-0092` — all PC `0x0CF23052`.
- **EP3174** family: `-0002` / `-0092` — both PC `0x0C664052`.

The high word of RevisionNo is the firmware/silicon revision (`0x0010` → `0x0011` → ...); the low word is the variant suffix as decimal (`0x0A = 10`, `0x14 = 20`, `0x1E = 30`, `0x5A = 90`, `0x5C = 92`, `0x02 = 2`).

## How this is worse than #58 / #60

- #58 (per-slave PDO group) and #60 (multi-firmware-revision YAML) both deal with chains carrying **mixed** revisions of the same physical product.
- This bug bites a **homogeneous chain with one revision per class** — if the YAML's entry is for `EL3202-0010` but the rig has `EL3202-0020`, the loose `(vendor, product)` fallback matches silently, the `bit_offset`s and `name_template`s from the wrong variant are applied, and the symptoms are the same "no bus parent" / "no match for AO write" warnings recorded in [[project_ao-write-no-match]].

The PDO layout often happens to be similar across suffixes (Beckhoff reuses sync-manager indices), so the bug can lie dormant until a variant where it actually differs lands on a rig.

## Suggested fix

Split RevisionNo into high (firmware) and low (variant) words at the comparison site:

```python
def _variant(rev: int) -> int:
return rev & 0xFFFF

def _firmware(rev: int) -> int:
return rev >> 16
```

Change the fallback policy from "first `(vendor, product)` hit wins" to:

1. Prefer exact `(vendor, product, revision)`.
2. Else prefer `(vendor, product, variant)` with the highest firmware ≤ slave's firmware.
3. Else no match — raise/warn rather than silently fall back to a different variant.

This composes cleanly with #60's "highest revision ≤ slave's revision" rule (#60 just becomes the firmware-tier selector once variant matches).

## Files in scope

- `src/fastcs_catio/terminal_config.py::get_terminal_type_by_identity` — split match into variant + firmware tiers.
- `tests/test_symbol_expansion.py::TestGetTerminalTypeByIdentity` — add cases proving a `-0010` YAML does **not** match a `-0020` rig identity.
- `src/fastcs_catio/symbols.py` — the warning at `expand_symbols_for_slave` should distinguish "no terminal type for this product code" (truly unknown) from "wrong variant matched" (loose fallback fired).

Worth confirming on rig: any chain whose YAML entry suffix doesn't exactly match the physical label. Most current YAML entries already pin to a specific suffix (`EL3202-0010`, `EL3356-0010`, `EP3174-0002`), so a rig swap to e.g. `EL3202-0030` would expose the bug without any other change.

## Related

- #60 — adds multi-firmware-revision support per variant. This issue is the variant-vs-firmware decomposition that #60's lookup logic should sit on top of.
- #58 — per-slave PDO group selection (orthogonal: same product, different group choice per slave).
- #54 — the YAML-driven symbol expansion this all extends.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.