From 1806af54f06969753f6a75f02c1c0a140378ed07 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 21:06:40 -0400 Subject: [PATCH] =?UTF-8?q?fix(ab):=20Task=2022=20review=20r1=20=E2=80=94?= =?UTF-8?q?=20release-note=20timing=20and=20AbLegacy=20regressions,=20prob?= =?UTF-8?q?e=20E2E=20on=20both=20transports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - I1: the upgrade restart itself moves every no-key AbCip/AbLegacy driver to Native (every process start constructs drivers, incl. boot-from-cache). Docs + CLAUDE.md now say so and tell operators to set "transport": "Libplctag" BEFORE upgrading (older builds ignore the unknown key). - I2: AbLegacy release note leads with the status-bit write refusal (T4:0.DN, C5:0.CU, R6:0.EN, ...) on defaulted SLC/MicroLogix drivers and the fallback. The real client status is BadCommunicationError, not BadNotSupported (the driver's write catch-all maps the runtime's AbTransportException like any transport failure, as #571 does for PLC-5), with handle eviction and Degraded health; pinned by a driver-level test (no 0xAA write on the wire, with a positive control). ST string byte order stated as unverified. Also corrects the docs' "T4:0/DN" examples: the driver's address grammar is "T4:0.DN" ("/DN" is a malformed address). - I3: DriverProbeHandshakeE2eTests' two AbCip probe tests are a [Theory] over Native and Libplctag. Run against ab_server controllogix: all 4 rows pass with identical exact messages. - M1: documented (docs + resolver XML doc): the AdminUI merges only the probed device, so a mixed-family defaulted AbLegacy driver can probe Native while running Libplctag. - M2: factory ParseEnum rejects undefined integer strings ("7") via Enum.IsDefined (both drivers). - M3: AbCip driver-level bad value reads "AB CIP config in '' has unknown Transport ..."; the named constant is gone (tagName: null now means driver-level). - M4: the explicit-Native limited-family Warning is once per instance, not per (re)init. - Nit: EffectiveTransport docs note that injected factories bypass it. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 6 +- docs/drivers/AbCip.md | 12 ++-- docs/drivers/AbLegacy.md | 33 +++++++-- .../AbCipDriver.cs | 3 +- .../AbCipDriverFactoryExtensions.cs | 18 ++--- .../AbLegacyDriver.cs | 14 ++-- .../AbLegacyDriverFactoryExtensions.cs | 3 +- .../AbLegacyTransportResolver.cs | 3 + .../AbCipTransportSwitchTests.cs | 22 ++++++ .../AbLegacyTransportInitLogTests.cs | 28 ++++++++ .../AbLegacyTransportSwitchTests.cs | 13 ++++ .../NativeAbLegacyTagRuntimeTests.cs | 67 +++++++++++++++++++ .../DriverProbeHandshakeE2eTests.cs | 23 ++++--- 13 files changed, 211 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6b595018..aec3bf30 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -977,12 +977,14 @@ Dev/test LDAP is the **shared GLAuth** running on the Linux Docker host at `10.1 AbCip and AbLegacy have a second, managed wire transport (`Driver.Ab.Transport`) beside libplctag, chosen by the driver-config key `transport` = `Native` | `Libplctag` (string; takes effect when the driver is -recreated by a deploy; AdminUI "Transport" dropdown; CLIs `--transport`). **Native is the default (Task 22):** +constructed — every process start, so **the upgrade restart alone moves no-key drivers to Native**; set +`"transport": "Libplctag"` BEFORE upgrading to stay put; AdminUI "Transport" dropdown; CLIs `--transport`). **Native is the default (Task 22):** an ABSENT key resolves to `Native` via `AbCipTransportResolver` / `AbLegacyTransportResolver` (driver, probe and CLI share them) — except an AbLegacy driver with any PLC-5 / `LogixPccc` device, which resolves to `Libplctag`. "Default" is encoded by omitting the key; the AdminUI must never re-emit a concrete default. Evidence: AbCip reads proven on a real CompactLogix (Z-35162, read-only); AbCip writes and all AbLegacy are `ab_server`-only. -Fall back with `"transport": "Libplctag"`. Client-visible: native reports `BadTypeMismatch` where libplctag returned +Fall back with `"transport": "Libplctag"`. ⚠️ AbLegacy native refuses status-bit sub-element +writes (`T4:0.DN`…, client sees `BadCommunicationError`) and its ST byte order is unverified. Client-visible: native reports `BadTypeMismatch` where libplctag returned Good garbage for a same-width mis-authored type (DINT authored, REAL returned). libplctag is confined to `Libplctag*.cs` adapters by `LibplctagConfinementGuardTests`; do not reference it elsewhere. See `docs/drivers/AbCip.md` §Transport, `docs/drivers/AbLegacy.md`, and diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 7246db88..dd253d3f 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -97,7 +97,8 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi - An absent key means "use the default". It is resolved in one place, `AbCipTransportResolver`, which the driver, the Test Connect probe and the CLI all go through. An explicit value always wins. - A bad value fails like a bad `PlcFamily`; the Test Connect probe reports "Config JSON is invalid". -- A change takes effect when the driver instance is **recreated**. A deploy that changes `DriverConfig` +- A change takes effect when the driver instance is **constructed**: on every process start (so the upgrade + restart moves no-key drivers to Native — see the release note) and when a deploy that changes `DriverConfig` does this (`DriverHostActor` respawns the child, #516). `ReinitializeAsync` alone keeps the transport it was constructed with. - AdminUI: the **Transport** dropdown in the advanced section of the AbCip driver form ("Default (Native)" / @@ -120,8 +121,11 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi authored type; see [Native transport behaviour](#native-transport-behaviour). - **To fall back**, set `"transport": "Libplctag"` in the driver config (or pick "libplctag (fallback)" in the AdminUI dropdown). -- **When it takes effect:** a change of transport — including this default flip for an existing driver with - no key — takes effect when a deploy recreates the driver instance, not on `ReinitializeAsync`. +- ⚠️ **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:** AbCip **reads** (plus browse and templates) were proven on a production CompactLogix 5069-L320ERS2 (**Z-35162**, read-only, 2026-10-08; see [gate results](../plans/2026-10-07-native-ab-transport-gate-results.md) §Real press). AbCip **writes** are @@ -153,7 +157,7 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi writes are gated on a mismatch too. - Malformed tag names put the device into backoff, as under libplctag (#570). STRING capacity includes the alignment pad (84, not 82) (#570). -- Status-bit sub-element writes (e.g. `T4:0/DN`) are Unsupported natively (they would clobber the control +- Status-bit sub-element writes (e.g. `T4:0.DN`) are Unsupported natively (they would clobber the control word); nothing goes on the wire. Sub-element arrays are Unsupported natively. ## Known gaps diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index 5479deb0..45ea0438 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -108,9 +108,17 @@ PCCC-over-EtherNet/IP client (`Driver.Ab.Transport`). 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 at init naming the limitation. +(an operator choice) and logs one Warning per driver instance naming the limitation. -A change takes effect when the driver instance is recreated (a deploy that changes `DriverConfig`). The +⚠️ **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. + +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 @@ -124,6 +132,16 @@ Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses Unconnected ### Release note — native is the default (Task 22) +- ⚠️ **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 **`BadCommunicationError`** (not `BadNotSupported`: the driver's + write path maps the refusal like any transport failure, as for the PLC-5 refusal in #571), the tag's handle + is evicted, and driver health goes `Degraded` with "Writes to a status-bit sub-element … are not supported by + the native transport". **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 @@ -131,9 +149,14 @@ Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses Unconnected 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:** when a deploy recreates the driver instance (not on `ReinitializeAsync`). An existing - driver with no key moves to Native on its next recreate. +- ⚠️ **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.) @@ -144,7 +167,7 @@ Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses Unconnected - **PLC-5 native is N-file reads only.** Every other file type and all PLC-5 writes are refused, currently surfacing as `BadCommunicationError` rather than `BadNotSupported` (#571). The default excludes PLC-5 and `logixpccc` (they resolve to libplctag unless `Native` is set explicitly). -- Status-bit sub-element writes (e.g. `T4:0/DN`, `C5:0/DN`) are Unsupported natively (they would clobber the +- 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. - **ST (string) file layout is uncaptured**, so the string byte order is unverified natively. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 534f90bc..5bf613ca 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -180,7 +180,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// The tag factory chosen at construction; exposed so tests can pin transport selection. internal IAbCipTagFactory TagFactory => _tagFactory; - /// The effective transport resolved at construction (see ). + /// The effective transport resolved at construction (see ). + /// Describes the OPTION only: injected factories (tests) are used regardless. internal AbCipTransport EffectiveTransport => _transport; // The transport chosen at construction (see the ctor); logged at every init so an operator can tell diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs index 4016d6d5..db8d501a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs @@ -100,7 +100,7 @@ public static class AbCipDriverFactoryExtensions // fails exactly as any other bad enum field does. Transport = string.IsNullOrWhiteSpace(dto.Transport) ? null - : ParseEnum(dto.Transport, DriverConfigOwner, driverInstanceId, "Transport"), + : ParseEnum(dto.Transport, tagName: null, driverInstanceId, "Transport"), EnableControllerBrowse = dto.EnableControllerBrowse ?? false, EnableAlarmProjection = dto.EnableAlarmProjection ?? false, AlarmPollInterval = TimeSpan.FromMilliseconds(dto.AlarmPollIntervalMs ?? 1_000), @@ -123,23 +123,25 @@ public static class AbCipDriverFactoryExtensions private static TimeSpan PositiveTimeoutOrDefault(int? ms, int defaultMs) => TimeSpan.FromMilliseconds(ms is int v and > 0 ? v : defaultMs); - // Names the owner of a driver-level enum field in ParseEnum's error text. A named constant rather - // than an inline literal: RoleAuthorityGuardTests forbids a bare role-name string literal outside RoleParser. - private const string DriverConfigOwner = "(driver config)"; - + // tagName null = a driver-level field: the error text names the driver config, not a tag. private static T ParseEnum(string? raw, string? tagName, string driverInstanceId, string field, T? fallback = null) where T : struct, Enum { + var subject = tagName is null ? $"config in '{driverInstanceId}'" : $"tag '{tagName}'"; if (string.IsNullOrWhiteSpace(raw)) { if (fallback.HasValue) return fallback.Value; throw new InvalidOperationException( - $"AB CIP tag '{tagName ?? ""}' in '{driverInstanceId}' missing {field}"); + tagName is null + ? $"AB CIP {subject} missing {field}" + : $"AB CIP {subject} in '{driverInstanceId}' missing {field}"); } - return Enum.TryParse(raw, ignoreCase: true, out var v) + // Enum.TryParse also accepts any integer string ("7"), defined or not; an undefined value would + // otherwise fall through to a switch's default arm silently (Task 22 review r1 M2). + return Enum.TryParse(raw, ignoreCase: true, out var v) && Enum.IsDefined(v) ? v : throw new InvalidOperationException( - $"AB CIP tag '{tagName}' has unknown {field} '{raw}'. " + + $"AB CIP {subject} has unknown {field} '{raw}'. " + $"Expected one of {string.Join(", ", Enum.GetNames())}"); } 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 6f132660..510e588a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -94,7 +94,8 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// The tag factory chosen at construction; exposed so tests can pin transport selection. internal IAbLegacyTagFactory TagFactory => _tagFactory; - /// The effective transport resolved at construction (see ). + /// The effective transport resolved at construction (see ). + /// Describes the OPTION only: an injected tagFactory (tests) is used regardless. internal AbLegacyTransport EffectiveTransport => _transport; // The transport chosen at construction (see the ctor); logged at every init so an operator can tell @@ -104,6 +105,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // Configured-vs-effective rendering of _transport for the init log, e.g. "Native (default)". private readonly string _transportDescription; + // Set once the explicit-Native-on-a-limited-family Warning has been logged for this instance. + private bool _limitedFamilyWarned; + /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); @@ -153,11 +157,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover _options = AbLegacyDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); // The resolver only defaults a PLC-5 / LogixPccc driver to libplctag; an operator may still pick - // Native explicitly (or a reinit may add such a device to a driver resolved Native). Allowed, but - // the limitation is client-visible, so name it once per init. - if (_transport == AbLegacyTransport.Native + // Native explicitly. 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 && AbLegacyTransportResolver.NativeLimitedFamilies(_options.Devices) is { Count: > 0 } limited) { + _limitedFamilyWarned = true; _logger.LogWarning( "AbLegacy driver {DriverInstanceId} runs the Native transport against {Families} device(s); native " + "support is limited there (PLC-5: N-file reads only; LogixPccc: framing unverified). Set " + 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 f6963b4d..90c7c989 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs @@ -130,7 +130,8 @@ public static class AbLegacyDriverFactoryExtensions throw new InvalidOperationException( $"AB Legacy {(tagName is null ? "config" : $"tag '{tagName}'")} in '{driverInstanceId}' missing {field}"); } - return Enum.TryParse(raw, ignoreCase: true, out var v) + // Enum.TryParse also accepts any integer string ("7"), defined or not (Task 22 review r1 M2). + return Enum.TryParse(raw, ignoreCase: true, out var v) && Enum.IsDefined(v) ? v : throw new InvalidOperationException( $"AB Legacy {(tagName is null ? "config" : $"tag '{tagName}'")} has unknown {field} '{raw}'. " + 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 fdadc7c5..66e1bacf 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyTransportResolver.cs @@ -11,6 +11,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; /// 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). /// public static class AbLegacyTransportResolver { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs index 5375101d..24db4996 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTransportSwitchTests.cs @@ -66,6 +66,28 @@ public sealed class AbCipTransportSwitchTests existing.Message.ShouldContain("unknown PlcFamily 'Bogus'"); } + /// + /// An undefined integer string is rejected, not silently run on a switch's default arm (Task 22 review + /// r1 M2): Enum.TryParse alone accepts "7". A defined numeric ("1") still parses, as before. + /// + [Fact] + public void An_undefined_numeric_transport_is_rejected() + { + Should.Throw( + () => AbCipDriverFactoryExtensions.ParseOptions("ab1", Config("\"7\""))) + .Message.ShouldContain("unknown Transport '7'"); + AbCipDriverFactoryExtensions.ParseOptions("ab1", Config("\"Native\"")).Transport.ShouldBe(AbCipTransport.Native); + } + + /// A bad driver-level value names the driver config, not a "tag" (Task 22 review r1 M3). + [Fact] + public void A_bad_transport_names_the_driver_config() + { + var ex = Should.Throw( + () => AbCipDriverFactoryExtensions.ParseOptions("ab1", Config("\"Bogus\""))); + ex.Message.ShouldStartWith("AB CIP config in 'ab1' has unknown Transport 'Bogus'"); + } + /// An explicitly injected factory wins over the option. [Fact] public void Injected_factory_wins_over_the_option() 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 223656d5..13d2ba24 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 @@ -48,6 +48,34 @@ public sealed class AbLegacyTransportInitLogTests if (warns) warnings[0].Message.ShouldContain(family); } + /// + /// The Warning is once per INSTANCE, not per init: every comms-loss reconnect retry re-runs init + /// (Task 22 review r1 M4). Three inits, one warning. + /// + [Fact] + public async Task The_limited_family_warning_is_logged_once_per_instance_across_reinits() + { + var config = Config("Native", "Plc5"); + var captured = new List<(LogLevel, string)>(); + using var factory = new CapturingLoggerFactory(captured); + var driver = AbLegacyDriverFactoryExtensions.CreateInstance("drv-t19", config, factory); + try + { + await driver.InitializeAsync(config, TestContext.Current.CancellationToken); + await driver.ReinitializeAsync(config, TestContext.Current.CancellationToken); + await driver.ReinitializeAsync(config, TestContext.Current.CancellationToken); + } + finally + { + await driver.ShutdownAsync(TestContext.Current.CancellationToken); + } + + List<(LogLevel Level, string Message)> snapshot; + lock (captured) snapshot = [.. captured]; + snapshot.Count(m => m.Message.Contains("initializing")).ShouldBe(3); // positive control: three inits ran + snapshot.Count(m => m.Level == LogLevel.Warning && m.Message.Contains("native support is limited")).ShouldBe(1); + } + private static async Task> InitAndCaptureAsync(string config) { var captured = new List<(LogLevel, string)>(); 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 3705e60d..5c9486bc 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 @@ -96,6 +96,19 @@ public sealed class AbLegacyTransportSwitchTests existing.Message.ShouldContain("unknown PlcFamily 'Bogus'"); } + /// + /// An undefined integer string is rejected, not silently run on a switch's default arm (Task 22 review + /// r1 M2): Enum.TryParse alone accepts "7". A defined numeric ("1") still parses, as before. + /// + [Fact] + public void An_undefined_numeric_transport_is_rejected() + { + Should.Throw( + () => AbLegacyDriverFactoryExtensions.ParseOptions("ab1", Config("\"7\""))) + .Message.ShouldContain("unknown Transport '7'"); + AbLegacyDriverFactoryExtensions.ParseOptions("ab1", Config("\"Native\"")).Transport.ShouldBe(AbLegacyTransport.Native); + } + /// An explicitly injected factory wins over the option. [Fact] public void Injected_factory_wins_over_the_option() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs index a9b479f7..84070fb9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs @@ -1,6 +1,8 @@ using System.Buffers.Binary; using Shouldly; using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Captures; @@ -380,6 +382,71 @@ public sealed class NativeAbLegacyTagRuntimeTests h.Channel.SentCount.ShouldBe(0); } + // These two tests call the driver's WriteAsync directly to pin its OWN status mapping; resilience wrapping + // (CapabilityInvoker) is out of scope and would retry/mask the very status under test. +#pragma warning disable OTOPCUA0001 + private sealed class HarnessFactory(Harness h) : IAbLegacyTagFactory + { + public IAbLegacyTagRuntime Create(AbLegacyTagCreateParams p) => + new NativeAbLegacyTagRuntime(p, h.Scheduler, Vendor, Serial); + } + + /// + /// Pins the CLIENT-VISIBLE status of the native status-bit write refusal, through the driver (Task 22 + /// review r1 I2 — the release note states it). The runtime throws AbTransportException(Unsupported); + /// the driver's write path does not special-case it, so the client sees the generic transport + /// status, and nothing reaches the wire. + /// + [Theory] + [InlineData("T4:0.DN")] + [InlineData("C5:0.CU")] + [InlineData("R6:0.EN")] + public async Task A_status_bit_write_through_the_driver_reports_the_documented_status(string address) + { + await using var h = new Harness("slc500", "N7:0").Replies(Words(0)); + const string host = "ab://10.0.0.1:44818/1,0"; + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("Bit", host, address, AbLegacyDataType.Bit)]), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-sb", new HarnessFactory(h)); + await driver.InitializeAsync("{}", CancellationToken.None); + var results = await driver.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + // BadCommunicationError, NOT BadNotSupported: the driver's catch-all maps any runtime exception to + // it (same as the PLC-5 write refusal, #571), evicts the handle and marks health Degraded. + results.Single().StatusCode.ShouldBe(AbLegacyStatusMapper.BadCommunicationError); + driver.GetHealth().LastError.ShouldNotBeNull().ShouldContain("status-bit sub-element"); + // Refused before the wire: the lazy runtime's init read may go out, but never a write (fnc 0xAA). + lock (h.Channel.Sent) h.Channel.Sent.ShouldNotContain(r => FncOf(r) == 0xAA); + await driver.ShutdownAsync(CancellationToken.None); + } + + /// Positive control for the "no 0xAA on the wire" assertion above: the same driver path for a WORD + /// sub-element does send a typed write, so the detector can see one. + [Fact] + public async Task A_word_sub_element_write_through_the_driver_sends_a_typed_write() + { + await using var h = new Harness("slc500", "N7:0").Replies(Words(0)); + const string host = "ab://10.0.0.1:44818/1,0"; + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("Pre", host, "T4:0.PRE", AbLegacyDataType.Int)]), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-sb", new HarnessFactory(h)); + await driver.InitializeAsync("{}", CancellationToken.None); + + var results = await driver.WriteAsync([new WriteRequest("Pre", (short)5)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + lock (h.Channel.Sent) h.Channel.Sent.ShouldContain(r => FncOf(r) == 0xAA); + await driver.ShutdownAsync(CancellationToken.None); + } + +#pragma warning restore OTOPCUA0001 + [Theory] [InlineData("T4:0.PRE")] [InlineData("T4:0.ACC")] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs index 7ce24bce..b0e3da0a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs @@ -168,8 +168,10 @@ public sealed class DriverProbeHandshakeE2eTests // ---- AbCip : libplctag CIP session handshake (skips unless the sim fixture is up) ---- - [Fact] - public async Task AbCip_Green_AgainstSim() + [Theory] + [InlineData("Native")] + [InlineData("Libplctag")] + public async Task AbCip_Green_AgainstSim(string transport) { SkipUnless(DockerHost, AbCipPort); // Probe an explicit tag that the ab_server ControlLogix sim actually defines @@ -182,9 +184,9 @@ public sealed class DriverProbeHandshakeE2eTests // exercising the one path the comment above says it does not exercise — and reporting it as a // failure of the fixture rather than of the config shape. var result = await new AbCipDriverProbe().ProbeAsync( - // Pinned to libplctag (Task 22 flipped the default to Native): the exact messages below were - // proven against ab_server on libplctag; the native path's ab_server coverage is TransportMatrix. - $"{{\"Transport\":\"Libplctag\",\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}]," + + // Run on BOTH transports (Task 22 review r1 I3): Native is the production default, and the + // probe's green messages are transport-neutral strings, so each row asserts the same text. + $"{{\"Transport\":\"{transport}\",\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}]," + $"\"RawTags\":[{{\"RawPath\":\"Plant/AbCip/dev1/TestDINT\"," + $"\"TagConfig\":\"{{\\\"tagPath\\\":\\\"TestDINT\\\",\\\"dataType\\\":\\\"DInt\\\"}}\"," + $"\"WriteIdempotent\":false,\"DeviceName\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}]}}", @@ -214,13 +216,16 @@ public sealed class DriverProbeHandshakeE2eTests /// Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch — so this test /// needs the fixture rebuilt from that patch, not merely running. /// - [Fact] - public async Task AbCip_WithNoAuthoredTags_ReportsTheControllerReachable() + [Theory] + [InlineData("Native")] + [InlineData("Libplctag")] + public async Task AbCip_WithNoAuthoredTags_ReportsTheControllerReachable(string transport) { SkipUnless(DockerHost, AbCipPort); var result = await new AbCipDriverProbe().ProbeAsync( - // Pinned to libplctag (Task 22): the 0x04 → ErrorNotFound reasoning above is libplctag's. - $"{{\"Transport\":\"Libplctag\",\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}],\"RawTags\":[]}}", + // Both transports (Task 22 review r1 I3): native maps CIP 0x04 to AbStatus.NotFound exactly as + // libplctag's ErrorNotFound does, so the reachable classification is the same. + $"{{\"Transport\":\"{transport}\",\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}],\"RawTags\":[]}}", Timeout, Ct); result.Ok.ShouldBeTrue($"Probe message: {result.Message}");