From b66972b6f576aacc1a1b69a68e5778ee8311ee4e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 21:34:04 -0400 Subject: [PATCH] =?UTF-8?q?fix(ablegacy):=20final=20review=20I3=20?= =?UTF-8?q?=E2=80=94=20default=20reverts=20to=20libplctag;=20Native=20is?= =?UTF-8?q?=20opt-in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a wire the native PCCC path is proven only for N-file reads/writes and one F8 read (ab_server). An absent transport key now resolves to Libplctag for every family; explicit Native still wins and still warns once per instance on PLC-5/LogixPccc. AdminUI dropdown, CLI help, evidence claims (PcccFileType, AbLegacy.md wire-evidence table, release note) corrected: S:0 (default probe) and B/T/C/R/S/L/ST/A/O/I are unverified natively. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/AbLegacy.md | 106 ++++++++++-------- .../AbLegacyCommandBase.cs | 5 +- .../Pccc/PcccFileType.cs | 9 +- .../AbLegacyDriverOptions.cs | 6 +- .../AbLegacyDriver.cs | 8 +- .../AbLegacyDriverFactoryExtensions.cs | 4 +- .../AbLegacyTransportResolver.cs | 44 +++----- .../Drivers/Forms/AbLegacyDriverForm.razor | 8 +- .../BuildOptionsTests.cs | 17 +-- .../AbLegacyTransportInitLogTests.cs | 7 +- .../AbLegacyTransportSwitchTests.cs | 59 ++++++---- ...bLegacyDriverPageFormSerializationTests.cs | 23 +++- 12 files changed, 166 insertions(+), 130 deletions(-) diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index 71931876..aefdd18c 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -93,7 +93,7 @@ binds from the driver's `DriverConfig` JSON: - **`Devices`** — one `AbLegacyDeviceOptions` per PLC (`HostAddress`, `PlcFamily`, optional `DeviceName`). - **`Tags`** — pre-declared `AbLegacyTagDefinition` list (`Name`, `DeviceHostAddress`, `Address`, `DataType`, `Writable`, `WriteIdempotent`). - **`Probe`** — connectivity-probe `Enabled` / `Interval` / `Timeout` / `ProbeAddress`. -- **`Transport`** — absent (default ⇒ `Native`, but `Libplctag` when any device is PLC-5 / `LogixPccc`), `Native`, or `Libplctag` (fallback); see [Transport](#transport-transport-option). +- **`Transport`** — absent (default ⇒ `Libplctag`), `Native` (opt-in), or `Libplctag`; see [Transport](#transport-transport-option). Full per-field descriptions live in the contracts assembly. The JSON skeleton is reproduced in [docs/v2/driver-specs.md §4](../v2/driver-specs.md). @@ -105,66 +105,72 @@ written as `"Libplctag"` / `"Native"`; "use the default" is encoded by omitting PCCC-over-EtherNet/IP client (`Driver.Ab.Transport`). **Default resolution** (one place, `AbLegacyTransportResolver`, used by the driver, the Test Connect probe and -the CLI): an absent key resolves to `Native`, **unless any configured device has `PlcFamily` `Plc5` or -`LogixPccc`** — then the whole driver resolves to `Libplctag`, because the transport is chosen once per driver, -not per device. An explicit value always wins: explicit `Native` with a PLC-5 / `LogixPccc` device is allowed -(an operator choice) and logs one Warning per driver instance naming the limitation. - -⚠️ **Test Connect in a mixed-family driver can disagree with the runtime.** The AdminUI probe merges the -driver config with **only the device being probed**, so in a defaulted driver holding an SLC 500 and a PLC-5, -Test Connect on the SLC device resolves `Native` while the deployed driver runs every device on `Libplctag`. -SLC 500 works on both stacks, so this is cosmetic; the reverse (probe on libplctag, driver on native) cannot -happen. Set `transport` explicitly to make them agree. +the CLI): an absent key resolves to **`Libplctag` for every PLC family** — unchanged from before the native +transport existed. **`Native` is opt-in** (final review I3: on a wire it is proven only for N-file reads and +writes and one F8 read — see [Wire evidence](#wire-evidence-native-pccc)). An explicit value always wins: +explicit `Native` with a PLC-5 / `LogixPccc` device is allowed (an operator choice) and logs one Warning per +driver instance naming the limitation. (AbCip's node-level `Drivers:AbCip:DefaultTransport` has no AbLegacy +counterpart — the AbLegacy default is libplctag and is not node-configurable.) A change takes effect when the driver instance is constructed: on a deploy that changes `DriverConfig` (which recreates it) **and on every process start**, including the restart that installs this build (see the release note). The -AdminUI form's **Transport** dropdown offers "Default (Native; libplctag for PLC-5 / LogixPccc)" (omits the -key on save), "Native (managed)" and "libplctag (fallback)". The driver logs -`transport= (default|configured)` on every init, e.g. `transport=Libplctag (default; PLC-5/LogixPccc -device present)`. CLI: `--transport default|native|libplctag` (default `default`, so `--plc-type Plc5` with no -`--transport` runs libplctag). See +AdminUI form's **Transport** dropdown offers "Default (libplctag)" (omits the key on save), "Native (managed, +opt-in)" and "libplctag". The driver logs `transport= (default|configured)` on every init, e.g. +`transport=Libplctag (default)`. CLI: `--transport default|native|libplctag` (default `default` = libplctag for +every `--plc-type`). See [AbCip.md §Transport](AbCip.md#transport-transport-option) for the shared behaviour (one pooled connection per device, Forward Open throttle, routing). Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses UnconnectedSend and its SLC framing is **unverified**. -### Release note — native is the default (Task 22) +### Release note — native PCCC transport, opt-in (final review I3) -- ⚠️ **Status-bit writes are refused on the default.** An SLC 500 / MicroLogix driver on the native default - **refuses writes to status-bit sub-elements** — `T4:0.DN`, `T4:0.TT`, `T4:0.EN`, `C5:0.CU`, `C5:0.DN`, - `R6:0.EN`, and the rest. The client sees **`BadNotSupported`**. It is not treated as a comms fault: the - tag's handle is kept and driver health is unchanged. **Nothing is written to the PLC.** libplctag performed a masked bit write here, so an - authored writable status-bit tag that worked before the upgrade fails after it. This is deliberate: a native - read-modify-write of the control word would race the PLC updating TT/DN and could clobber EN/TT/DN, and such - writes are rare. **Drivers that need them must set `"transport": "Libplctag"`.** Reads of status bits are - unaffected. Word sub-elements (`T4:0.PRE`, `T4:0.ACC`, `R6:0.LEN`, `R6:0.POS`) still write natively. -- **The native managed transport is now the default** for SLC 500 and MicroLogix. A driver config with no - `transport` key runs `Native`; libplctag remains available as the fallback. -- **Exclusion:** a defaulted driver with **any PLC-5 or `LogixPccc` device stays on libplctag**. Native PLC-5 - support is N-file reads only, and `LogixPccc` framing is unverified. Explicit `Native` is still honoured - there, with a Warning at init. -- ⚠️ **Client-visible:** native reports **`BadTypeMismatch`** for a mis-authored same-width type, where - libplctag returned Good garbage (see [AbCip.md §Native transport behaviour](AbCip.md#native-transport-behaviour)). -- **ST (string) files:** the native ST file byte order is **unverified** (no capture), so string values read or - written natively may be byte-swapped. Drivers with ST tags whose values matter should fall back. -- **To fall back**, set `"transport": "Libplctag"` in the driver config. -- ⚠️ **When it takes effect: the upgrade restart itself.** Every process start constructs the drivers - (including boot-from-cache on a driver-only node), so **every driver with no `transport` key moves to - Native the moment the upgraded node starts** — no deploy is needed. To keep a driver on libplctag, add - `"transport": "Libplctag"` and deploy it **before upgrading**: older builds ignore the unknown key. After the - upgrade, a transport change takes effect when a deploy recreates the driver (not on `ReinitializeAsync`). -- **Evidence, and its limits:** all of AbLegacy is **simulator-proven only** (`ab_server`). No real SLC, - MicroLogix or PLC-5 has been exercised. (The real-press gate, Z-35162 2026-10-08, covered AbCip reads only.) +- **The default is unchanged: libplctag.** A driver config with no `transport` key runs `Libplctag`, for + every PLC family, exactly as before this release. The upgrade restart moves nothing. (An interim build on + this branch defaulted SLC 500 / MicroLogix to Native; that was reverted before merge.) +- **`"transport": "Native"` opts a driver into the managed PCCC-over-EtherNet/IP client** (`Driver.Ab.Transport`). + It takes effect when the driver is constructed — a deploy that changes `DriverConfig`, or a process start — + not on `ReinitializeAsync`. The AdminUI **Transport** dropdown authors it; remove the key (or pick + "Default (libplctag)") to go back. +- **Known limits of Native** (why it is opt-in): + - ⚠️ **Wire evidence is narrow and simulator-only** — see [Wire evidence](#wire-evidence-native-pccc). Only + N-file reads and writes and one F8 read have touched a wire (`ab_server`). The **default probe address + `S:0`** and the **B, T, C, R, S, L, ST, A, O and I file types** — and the T/C/R sub-element offsets + (`.PRE`/`.ACC`/`.LEN`/`.POS`) — are encoded from the PCCC file-type table and are **unverified natively**. + No real SLC, MicroLogix or PLC-5 has been exercised. Opt in per driver after checking it against the real + controller. + - ⚠️ **Status-bit writes are refused** — `T4:0.DN`, `T4:0.TT`, `T4:0.EN`, `C5:0.CU`, `C5:0.DN`, `R6:0.EN` + and the rest return **`BadNotSupported`**; nothing is written to the PLC; the handle is kept and health is + unchanged. libplctag performs a masked bit write here; a native read-modify-write of the control word would + race the PLC updating TT/DN. Reads of status bits are unaffected. + - **PLC-5 native is N-file reads only**; every other PLC-5 operation is refused with `BadNotSupported`. + `LogixPccc` framing is unverified. Explicit Native on either logs a Warning at init. + - **ST (string) byte order is unverified** (no capture); string values may be byte-swapped. + - Unlike AbCip, the native AbLegacy runtime has **no element-type check**, so a mis-authored same-width + type reads as reinterpreted data (as on libplctag), not `BadTypeMismatch`. + +### Wire evidence (native PCCC) + +Everything below is against the `ab_server` simulator; no real PCCC controller has been exercised. + +| What | Evidence | +|---|---| +| N-file read (`N7:0`) | libplctag captures `slc500-read-n7`, `micrologix-read-n7`, `plc5-read-n7` (codec byte parity) + native `AbLegacyReadSmokeTests` read on all three profiles | +| N-file write (`N7:5`) | capture `slc500-write-n7` + native `AbLegacyReadSmokeTests` write/read round trip (SLC 500) | +| F-file read (`F8`) | one capture, `slc500-read-f8` (codec parity only; no native F write on a wire) | +| S (incl. the default probe `S:0`), B, T, C, R, L, ST, A, O, I | **none** — encoded from the PCCC file-type table | +| T/C/R sub-elements (`.PRE`/`.ACC`/`.LEN`/`.POS`), 6-byte T/C/R element layout | **none** | ### Native known gaps -- ⚠️ **Simulator-only.** PCCC reads and writes are verified against `ab_server` only. No real SLC/MicroLogix/ - PLC-5 has been exercised; the real-press gate (Task 21) was read-only and AbCip-only. +- ⚠️ **Simulator-only, and narrow.** Only N-file reads/writes and one F8 read are on a wire (`ab_server`); see + [Wire evidence](#wire-evidence-native-pccc). No real SLC/MicroLogix/PLC-5 has been exercised; the real-press + gate (Task 21) was read-only and AbCip-only. - **PLC-5 native is N-file reads only.** Every other file type and all PLC-5 writes are refused before the - wire and reach the client as `BadNotSupported` (handle kept, health unchanged; #571 closed). The default excludes PLC-5 and - `logixpccc` (they resolve to libplctag unless `Native` is set explicitly). + wire and reach the client as `BadNotSupported` (handle kept, health unchanged; #571 closed). Native is opt-in + for every family; an explicit Native on PLC-5 / `logixpccc` logs a Warning at init. - Status-bit sub-element writes (e.g. `T4:0.DN`, `C5:0.DN`) are Unsupported natively (they would clobber the control word); nothing goes on the wire. Sub-element arrays are Unsupported natively. L-file bit indexes above 15 are allowed. @@ -184,14 +190,16 @@ An AB Legacy tag becomes a **1-D OPC UA array node** when its `TagConfig` JSON c `"isArray": true` and `"arrayLength": N` (N ≥ 1, capped at 256). The canonical rule: `isArray: true` + `arrayLength >= 1` → array; `isArray: false` (any length) → scalar. -**Read mechanism** — the driver issues a **PCCC multi-element file read** via libplctag. -The tag's file address (e.g. `N7:0`) is extended by `N` element count so libplctag fetches +**Read mechanism** — the driver issues a **PCCC multi-element file read** (libplctag by default; the opt-in +native transport issues the same typed read, chunked under the PCCC payload limit). +The tag's file address (e.g. `N7:0`) is extended by `N` element count so the transport fetches `N` consecutive PCCC file elements in one EIP/PCCC request. The response is decoded into N values of the declared `AbLegacyDataType`. Maximum element count is 256 (PCCC payload limit). This closes the previously-noted limitation ("single-element addressing today") for the -array read path. Array *writes* remain a follow-up. +array read path. The opt-in native transport also writes multi-element arrays (chunked); on libplctag +array *writes* remain a follow-up. **Unit test coverage** — `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/` covers the multi-element read path via the fake tag runtime. The AB Legacy integration fixture is diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs index 2354b91a..e96b1a75 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs @@ -27,8 +27,9 @@ public abstract class AbLegacyCommandBase : DriverCommandBase /// Gets the transport stack used to reach the PLC. [CommandOption("transport", Description = - "Transport stack: default, native or libplctag. default = native, except libplctag for --plc-type " + - "Plc5 / LogixPccc (native there is PLC-5 N-file reads only, no status-bit writes, LogixPccc unverified).")] + "Transport stack: default, native or libplctag. default = libplctag for every --plc-type; native is " + + "opt-in (wire-proven only for N-file reads/writes and an F8 read; PLC-5 native is N-file reads only, " + + "LogixPccc unverified).")] public AbLegacyCliTransport Transport { get; init; } = AbLegacyCliTransport.Default; /// Gets the per-operation timeout in milliseconds. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccFileType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccFileType.cs index c39684ab..fd7bcc8e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccFileType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccFileType.cs @@ -1,8 +1,13 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Pccc; /// PCCC file-type codes for the protected typed logical read/write commands (0xA2/0xAA). -/// N (0x89) and F (0x8A) are verified against the golden captures; the rest follow the PCCC -/// data-table file-type table. +/// Wire evidence (final review I3), all against the ab_server simulator, never a real PLC: +/// N (0x89) — byte parity with libplctag captures (SLC 500 read + write, MicroLogix read, PLC-5 read) and +/// native ab_server round trips (N7:0 read on all three profiles, N7:5 write on SLC 500); F (0x8A) — one +/// SLC 500 F8 read capture only (no native write on a wire). +/// S, B, T, C, R, ST, A, L, O and I are encoded from the PCCC data-table file-type table and are +/// UNVERIFIED natively — no capture and no simulator test. That includes S:0, the default probe address, +/// and the T/C/R sub-element (PRE/ACC/LEN/POS) word offsets. public static class PcccFileType { /// Returns the file-type code for a file letter (case-insensitive), or null when unknown. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs index 781bd0f1..ab5b9d09 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs @@ -27,9 +27,9 @@ public sealed class AbLegacyDriverOptions /// /// Which transport stack the driver, its Test Connect probe and the CLI use. null (the - /// transport key absent) means "use the default": , UNLESS - /// any configured device is a PLC-5 or LogixPccc (limited native support), in which case the whole - /// driver resolves to . An explicit value always wins. The + /// transport key absent) means "use the default": for + /// every family. is opt-in (final review I3: on a wire it is proven + /// only for N-file reads/writes and one F8 read). An explicit value always wins. The /// effective transport is resolved in one place, AbLegacyTransportResolver. /// Changing this takes effect when the driver instance is RECREATED, not on /// ReinitializeAsync: the tag factory is chosen once in the constructor and held across diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index aef9f6bd..57c6ba1d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -72,7 +72,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // Chosen once here and held across ReinitializeAsync (Shutdown+Initialize): a changed // Transport in a reinit config is inert; a deploy recreates the instance instead. _transport = AbLegacyTransportResolver.Resolve(options); - _transportDescription = AbLegacyTransportResolver.Describe(options.Transport, _transport); + _transportDescription = AbLegacyTransportResolver.Describe(options.Transport); _tagFactory = tagFactory ?? (_transport == AbLegacyTransport.Native ? new NativeAbLegacyTagFactory() : new LibplctagLegacyTagFactory()); @@ -103,7 +103,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // from the node log which stack serves this instance. private readonly AbLegacyTransport _transport; - // Configured-vs-effective rendering of _transport for the init log, e.g. "Native (default)". + // Configured-vs-effective rendering of _transport for the init log, e.g. "Libplctag (default)". private readonly string _transportDescription; // Set once the explicit-Native-on-a-limited-family Warning has been logged for this instance. @@ -157,8 +157,8 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover if (HasConfigBody(driverConfigJson)) _options = AbLegacyDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); - // The resolver only defaults a PLC-5 / LogixPccc driver to libplctag; an operator may still pick - // Native explicitly. Allowed, but the limitation is client-visible, so name it ONCE per instance — + // Native is opt-in for every family (the default is libplctag); an operator may pick it explicitly + // even for PLC-5 / LogixPccc. Allowed, but the limitation is client-visible, so name it ONCE per instance — // not on every init, which includes every comms-loss reconnect retry (Task 22 review r1 M4). if (!_limitedFamilyWarned && _transport == AbLegacyTransport.Native diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs index 90c7c989..e7093f76 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs @@ -97,7 +97,7 @@ public static class AbLegacyDriverFactoryExtensions ProbeAddress = dto.Probe?.ProbeAddress ?? "S:0", }, Timeout = PositiveTimeoutOrDefault(dto.TimeoutMs, 2_000), - // Absent ⇒ null ("use the default", resolved by AbLegacyTransportResolver against the devices); + // Absent ⇒ null ("use the default" = Libplctag, resolved by AbLegacyTransportResolver); // a bad value still fails exactly as any other bad enum field does. Transport = string.IsNullOrWhiteSpace(dto.Transport) ? null @@ -153,7 +153,7 @@ public static class AbLegacyDriverFactoryExtensions public int? TimeoutMs { get; init; } /// - /// Gets or sets the transport (Libplctag or Native, case-insensitive); absent means Libplctag. + /// Gets or sets the transport (Libplctag or Native, case-insensitive); absent means Libplctag (the default). /// public string? Transport { get; init; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs index 66e1bacf..58c14e74 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs @@ -4,16 +4,13 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; /// /// The ONE place an AbLegacy driver's effective transport is decided from its configured -/// and its devices. The driver constructor, the Test -/// Connect probe and the CLI (through the driver) all resolve through here. -/// An absent key means — UNLESS any configured device is a -/// or , whose native support -/// is limited (PLC-5: N-file reads only; LogixPccc: framing unverified). The tag factory is chosen once -/// per DRIVER, not per device, so one such device resolves the whole driver to -/// . An explicit value always wins (an operator choice). -/// The probe resolves from the config it is GIVEN. The AdminUI merges only the probed device into -/// that config, so in a mixed-family defaulted driver Test Connect on an SLC device can resolve Native -/// while the deployed driver resolves Libplctag (documented in docs/drivers/AbLegacy.md). +/// . The driver constructor, the Test Connect probe and the +/// CLI (through the driver) all resolve through here. +/// An absent key means = for EVERY +/// family (final review I3): on a wire the native PCCC path is proven only for N-file reads and writes +/// and one F8 read, so Native stays opt-in. An explicit value always wins (an operator choice). An +/// explicit Native on a PLC-5 / LogixPccc device is allowed but logs a once-per-instance Warning +/// (). /// public static class AbLegacyTransportResolver { @@ -32,16 +29,13 @@ public static class AbLegacyTransportResolver return [.. devices.Select(d => d.PlcFamily).Where(IsNativeLimited).Distinct()]; } - /// Returns the effective transport for a configured value and device set. + /// The transport an absent / null transport key selects (all families). + public const AbLegacyTransport Default = AbLegacyTransport.Libplctag; + + /// Returns the effective transport: an explicit value always wins; null means . /// The configured transport, or null for "use the default". - /// The configured devices. /// The transport the driver runs on. - public static AbLegacyTransport Resolve(AbLegacyTransport? configured, IEnumerable devices) - { - ArgumentNullException.ThrowIfNull(devices); - if (configured is { } explicitValue) return explicitValue; - return devices.Any(d => IsNativeLimited(d.PlcFamily)) ? AbLegacyTransport.Libplctag : AbLegacyTransport.Native; - } + public static AbLegacyTransport Resolve(AbLegacyTransport? configured) => configured ?? Default; /// Returns the effective transport for . /// The driver options. @@ -49,19 +43,15 @@ public static class AbLegacyTransportResolver public static AbLegacyTransport Resolve(AbLegacyDriverOptions options) { ArgumentNullException.ThrowIfNull(options); - return Resolve(options.Transport, options.Devices); + return Resolve(options.Transport); } /// - /// Operator-facing description of the configured-vs-effective transport for the init log line, e.g. - /// Native (default), Libplctag (default; PLC-5/LogixPccc device present) or - /// Native (configured). + /// Operator-facing description of the configured-vs-effective transport for the init log line: + /// Libplctag (default) when the key was absent, Native (configured) when explicit. /// /// The configured transport, or null. - /// The effective transport. /// A short description. - public static string Describe(AbLegacyTransport? configured, AbLegacyTransport effective) => - configured is not null ? $"{effective} (configured)" - : effective == AbLegacyTransport.Libplctag ? $"{effective} (default; PLC-5/LogixPccc device present)" - : $"{effective} (default)"; + public static string Describe(AbLegacyTransport? configured) => + configured is null ? $"{Resolve(configured)} (default)" : $"{configured} (configured)"; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor index b5397a76..1a91c666 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbLegacyDriverForm.razor @@ -22,11 +22,11 @@
- - - + + + -
Which protocol stack talks to the controller. Default omits the setting so the driver picks; takes effect on the next deploy.
+
Which protocol stack talks to the controller. Default omits the setting (libplctag). Native is opt-in: wire-proven only for N-file reads/writes and F8 reads. Takes effect on the next deploy.
diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/BuildOptionsTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/BuildOptionsTests.cs index a806eebf..3fa11d6e 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/BuildOptionsTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/BuildOptionsTests.cs @@ -162,13 +162,12 @@ public sealed class BuildOptionsTests } /// - /// With no --transport the CLI leaves the option null and the driver's resolver decides: Native - /// for an SLC 500, libplctag for --plc-type Plc5 / LogixPccc (Task 22 exclusion). An explicit - /// choice is forwarded verbatim and wins. + /// With no --transport the CLI leaves the option null and the driver's resolver decides: libplctag + /// for every family (final review I3; Native is opt-in). An explicit choice is forwarded verbatim and wins. /// [Theory] - [InlineData(AbLegacyPlcFamily.Slc500, AbLegacyTransport.Native)] - [InlineData(AbLegacyPlcFamily.MicroLogix, AbLegacyTransport.Native)] + [InlineData(AbLegacyPlcFamily.Slc500, AbLegacyTransport.Libplctag)] + [InlineData(AbLegacyPlcFamily.MicroLogix, AbLegacyTransport.Libplctag)] [InlineData(AbLegacyPlcFamily.Plc5, AbLegacyTransport.Libplctag)] [InlineData(AbLegacyPlcFamily.LogixPccc, AbLegacyTransport.Libplctag)] public void BuildOptions_default_transport_goes_through_the_resolver(AbLegacyPlcFamily family, AbLegacyTransport expected) @@ -186,13 +185,15 @@ public sealed class BuildOptionsTests } /// - /// The plan's CLI case end to end through CliFx: --plc-type plc5 with no --transport - /// resolves to libplctag; the same invocation for an SLC resolves Native (falsification control). + /// End to end through CliFx: no --transport resolves to libplctag for every --plc-type + /// (final review I3). The slc500 / micrologix rows are the falsification rows — they resolved Native + /// before the revert. /// [Theory] [InlineData("plc5", AbLegacyTransport.Libplctag)] [InlineData("logixpccc", AbLegacyTransport.Libplctag)] - [InlineData("slc500", AbLegacyTransport.Native)] + [InlineData("slc500", AbLegacyTransport.Libplctag)] + [InlineData("micrologix", AbLegacyTransport.Libplctag)] public async Task Plc_type_without_transport_flag_resolves_through_the_resolver(string plcType, AbLegacyTransport expected) { CapturingCommand.Options = null; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportInitLogTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportInitLogTests.cs index 13d2ba24..e85a45d0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportInitLogTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportInitLogTests.cs @@ -15,12 +15,12 @@ public sealed class AbLegacyTransportInitLogTests private static string Config(string? transport, string family = "Slc500") => $$"""{{{(transport is null ? "" : $"\"transport\":\"{transport}\",")}}"probe":{"enabled":false},"devices":[{"hostAddress":"ab://10.0.0.1:44818/1,0","plcFamily":"{{family}}"}]}"""; - /// The init line names the configured-vs-effective transport (Task 22). + /// The init line names the configured-vs-effective transport (final review I3: default = Libplctag). [Theory] [InlineData("Native", "Slc500", "Native (configured)")] [InlineData("Libplctag", "Slc500", "Libplctag (configured)")] - [InlineData(null, "Slc500", "Native (default)")] - [InlineData(null, "Plc5", "Libplctag (default; PLC-5/LogixPccc device present)")] + [InlineData(null, "Slc500", "Libplctag (default)")] + [InlineData(null, "Plc5", "Libplctag (default)")] public async Task Init_logs_the_transport_through_the_registered_logger_factory( string? json, string family, string expected) { @@ -37,6 +37,7 @@ public sealed class AbLegacyTransportInitLogTests [InlineData("Native", "Plc5", true)] [InlineData("Native", "LogixPccc", true)] [InlineData(null, "Plc5", false)] + [InlineData(null, "Slc500", false)] [InlineData("Libplctag", "LogixPccc", false)] [InlineData("Native", "Slc500", false)] public async Task Explicit_Native_on_a_limited_family_warns_once(string? json, string family, bool warns) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportSwitchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportSwitchTests.cs index 5c9486bc..249c4b8c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportSwitchTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyTransportSwitchTests.cs @@ -24,13 +24,13 @@ public sealed class AbLegacyTransportSwitchTests /// /// Transport values select the factory type; absent parses to null ("use the default") and the driver - /// resolves that to Native for an SLC 500 (Task 22). An explicit Libplctag still wins. + /// resolves that to Libplctag (final review I3 — Native is opt-in). An explicit Native still wins. /// [Theory] [InlineData("\"Native\"", AbLegacyTransport.Native)] [InlineData("\"native\"", AbLegacyTransport.Native)] [InlineData("\"Libplctag\"", AbLegacyTransport.Libplctag)] - [InlineData(null, AbLegacyTransport.Native)] + [InlineData(null, AbLegacyTransport.Libplctag)] public void Config_transport_selects_the_factory(string? json, AbLegacyTransport expected) { var parsed = AbLegacyDriverFactoryExtensions.ParseOptions("ab1", Config(json)); @@ -46,35 +46,42 @@ public sealed class AbLegacyTransportSwitchTests } /// - /// The exclusion: an absent key resolves the WHOLE driver to libplctag when any device is a PLC-5 or a - /// LogixPccc (limited native support); the factory is driver-level, so one such device among SLCs - /// is enough. The SLC/MicroLogix-only rows are the falsification control — they must resolve Native. + /// Final review I3: an absent key resolves EVERY family to libplctag — the SLC/MicroLogix rows are the + /// ones that change (they defaulted Native before), so they are the falsification rows: reverting the + /// resolver to "Native unless PLC-5/LogixPccc" turns them red. /// [Theory] - [InlineData(AbLegacyTransport.Libplctag, "Plc5")] - [InlineData(AbLegacyTransport.Libplctag, "LogixPccc")] - [InlineData(AbLegacyTransport.Libplctag, "Slc500", "Plc5")] - [InlineData(AbLegacyTransport.Libplctag, "MicroLogix", "LogixPccc")] - [InlineData(AbLegacyTransport.Native, "Slc500")] - [InlineData(AbLegacyTransport.Native, "MicroLogix")] - [InlineData(AbLegacyTransport.Native, "Slc500", "MicroLogix")] - public void Absent_transport_excludes_PLC5_and_LogixPccc(AbLegacyTransport expected, params string[] families) + [InlineData("Plc5")] + [InlineData("LogixPccc")] + [InlineData("Slc500", "Plc5")] + [InlineData("Slc500")] + [InlineData("MicroLogix")] + [InlineData("Slc500", "MicroLogix")] + public void Absent_transport_resolves_libplctag_for_every_family(params string[] families) { var json = Config(null, families); - AbLegacyTransportResolver.Resolve(AbLegacyDriverFactoryExtensions.ParseOptions("ab1", json)).ShouldBe(expected); + AbLegacyTransportResolver.Resolve(AbLegacyDriverFactoryExtensions.ParseOptions("ab1", json)) + .ShouldBe(AbLegacyTransport.Libplctag); var driver = AbLegacyDriverFactoryExtensions.CreateInstance("ab1", json); - driver.EffectiveTransport.ShouldBe(expected); - if (expected == AbLegacyTransport.Native) - driver.TagFactory.ShouldBeOfType(); - else - driver.TagFactory.ShouldBeOfType(); + driver.EffectiveTransport.ShouldBe(AbLegacyTransport.Libplctag); + driver.TagFactory.ShouldBeOfType(); } - /// An explicit value always wins over the exclusion, in both directions. + /// The init-log rendering names the default as such, and an explicit choice as configured. + [Fact] + public void Describe_names_default_and_configured() + { + AbLegacyTransportResolver.Describe(null).ShouldBe("Libplctag (default)"); + AbLegacyTransportResolver.Describe(AbLegacyTransport.Native).ShouldBe("Native (configured)"); + AbLegacyTransportResolver.Describe(AbLegacyTransport.Libplctag).ShouldBe("Libplctag (configured)"); + } + + /// An explicit value always wins, for every family. [Theory] [InlineData("\"Native\"", "Plc5", AbLegacyTransport.Native)] [InlineData("\"Native\"", "LogixPccc", AbLegacyTransport.Native)] + [InlineData("\"Native\"", "Slc500", AbLegacyTransport.Native)] [InlineData("\"Libplctag\"", "Slc500", AbLegacyTransport.Libplctag)] public void Explicit_transport_wins_over_the_exclusion(string json, string family, AbLegacyTransport expected) { @@ -171,8 +178,8 @@ public sealed class AbLegacyTransportSwitchTests /// /// The probe resolves an ABSENT transport through the same resolver as the driver: an SLC 500 with no - /// key takes the native path (native failure text), a PLC-5 with no key takes libplctag (no native - /// text) — against the same listener, so only the resolution differs. + /// key takes libplctag (no native failure text) exactly as a PLC-5 does, while an explicit Native on the + /// same listener produces the native text — the control that proves the text discriminates. /// [Fact] public async Task Probe_with_no_transport_resolves_like_the_driver() @@ -191,19 +198,23 @@ public sealed class AbLegacyTransportSwitchTests string Cfg(string family) => $$"""{"devices":[{"hostAddress":"ab://127.0.0.1:{{port}}/1,0","plcFamily":"{{family}}"}]}"""; + var nativeCfg = + $$"""{"transport":"Native","devices":[{"hostAddress":"ab://127.0.0.1:{{port}}/1,0","plcFamily":"Slc500"}]}"""; var probe = new AbLegacyDriverProbe(); var slc = await probe.ProbeAsync(Cfg("Slc500"), TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); var plc5 = await probe.ProbeAsync(Cfg("Plc5"), TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); + var native = await probe.ProbeAsync(nativeCfg, TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); listener.Stop(); AbLegacyTransportResolver.Resolve(AbLegacyDriverFactoryExtensions.ParseOptions("p", Cfg("Slc500"))) - .ShouldBe(AbLegacyTransport.Native); + .ShouldBe(AbLegacyTransport.Libplctag); AbLegacyTransportResolver.Resolve(AbLegacyDriverFactoryExtensions.ParseOptions("p", Cfg("Plc5"))) .ShouldBe(AbLegacyTransport.Libplctag); slc.Ok.ShouldBeFalse(); - slc.Message!.ShouldContain("PCCC handshake failed: Connection lost"); + slc.Message!.ShouldNotContain("Connection lost"); plc5.Message!.ShouldNotContain("Connection lost"); + native.Message!.ShouldContain("PCCC handshake failed: Connection lost"); } /// The probe deserialises with its own string-enum converter; a bad transport is a config error. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs index 2cfa184d..ec8b74d3 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AbLegacyDriverPageFormSerializationTests.cs @@ -144,8 +144,7 @@ public sealed class AbLegacyDriverPageFormSerializationTests public void A_config_without_transport_loads_as_Default_and_an_unrelated_edit_keeps_the_key_absent() { // Task 22 reverses Task 19's "re-emit an explicit Libplctag": that would pin every edited driver to - // a concrete transport and bypass the driver-side default (and AbLegacy's PLC-5 / LogixPccc - // exclusion). Absent loads as Default (null) and a save that edits something else still omits it. + // a concrete transport and bypass the driver-side default (Libplctag since final review I3). Absent loads as Default (null) and a save that edits something else still omits it. var loaded = AbLegacyDriverForm.TryDeserialize("""{"probeTimeoutSeconds":5}"""); loaded.ShouldNotBeNull(); var form = AbLegacyDriverForm.FormModel.FromOptions(loaded); @@ -168,4 +167,24 @@ public sealed class AbLegacyDriverPageFormSerializationTests form.Transport = null; // the "Default (...)" dropdown option AbLegacyDriverForm.Serialize(form.ToOptions(loaded.Devices)).ShouldNotContain("transport", Case.Insensitive); } + + /// + /// Final review I3: the dropdown labels must say what an absent key does (libplctag) and that Native is + /// opt-in. Source scan (no bUnit in this repo); the labels are static option text. + /// + [Fact] + public void The_transport_dropdown_labels_name_libplctag_as_the_default_and_native_as_opt_in() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull("could not locate the repository root"); + var razor = File.ReadAllText(Path.Combine(dir!.FullName, "src", "Server", "ZB.MOM.WW.OtOpcUa.AdminUI", + "Components", "Shared", "Drivers", "Forms", "AbLegacyDriverForm.razor")); + + razor.ShouldContain(""); + razor.ShouldContain(""); + razor.ShouldContain(""); + razor.ShouldNotContain("Default (Native"); + } }