fix(ab): Task 22 review r1 — release-note timing and AbLegacy regressions, probe E2E on both transports

- 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 '<id>' 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
This commit is contained in:
Joseph Doherty
2026-10-07 21:06:40 -04:00
parent ed0881d672
commit 1806af54f0
13 changed files with 211 additions and 34 deletions
@@ -180,7 +180,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
/// <summary>The tag factory chosen at construction; exposed so tests can pin transport selection.</summary>
internal IAbCipTagFactory TagFactory => _tagFactory;
/// <summary>The effective transport resolved at construction (see <see cref="AbCipTransportResolver"/>).</summary>
/// <summary>The effective transport resolved at construction (see <see cref="AbCipTransportResolver"/>).
/// Describes the OPTION only: injected factories (tests) are used regardless.</summary>
internal AbCipTransport EffectiveTransport => _transport;
// The transport chosen at construction (see the ctor); logged at every init so an operator can tell
@@ -100,7 +100,7 @@ public static class AbCipDriverFactoryExtensions
// fails exactly as any other bad enum field does.
Transport = string.IsNullOrWhiteSpace(dto.Transport)
? null
: ParseEnum<AbCipTransport>(dto.Transport, DriverConfigOwner, driverInstanceId, "Transport"),
: ParseEnum<AbCipTransport>(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<T>(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 ?? "<unnamed>"}' in '{driverInstanceId}' missing {field}");
tagName is null
? $"AB CIP {subject} missing {field}"
: $"AB CIP {subject} in '{driverInstanceId}' missing {field}");
}
return Enum.TryParse<T>(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<T>(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<T>())}");
}
@@ -94,7 +94,8 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
/// <summary>The tag factory chosen at construction; exposed so tests can pin transport selection.</summary>
internal IAbLegacyTagFactory TagFactory => _tagFactory;
/// <summary>The effective transport resolved at construction (see <see cref="AbLegacyTransportResolver"/>).</summary>
/// <summary>The effective transport resolved at construction (see <see cref="AbLegacyTransportResolver"/>).
/// Describes the OPTION only: an injected <c>tagFactory</c> (tests) is used regardless.</summary>
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;
/// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary>
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 " +
@@ -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<T>(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<T>(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}'. " +
@@ -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
/// <see cref="AbLegacyTransport.Libplctag"/>. An explicit value always wins (an operator choice).</para>
/// <para>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).</para>
/// </summary>
public static class AbLegacyTransportResolver
{