fix(ablegacy): review fixes - one family-aware address authority in Contracts, deploy gate + editor check, malformed address -> BadNodeIdUnknown warned once, probe normalised, octal/migration docs (#244 #245 #247)

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 14:38:25 -04:00
parent 0dc09329b7
commit d8f3dd7394
10 changed files with 291 additions and 15 deletions
@@ -49,7 +49,10 @@ public sealed record AbLegacyAddress(

    /// <summary>
    ///     Converts the address to the libplctag library address format. Numeric indices are always
-    ///     emitted <b>decimal</b> (a PLC-5 octal I/O address is normalised at parse time). An indirect
+    ///     emitted <b>decimal</b> (a PLC-5 octal I/O address is normalised at parse time). This is correct, not a
+    ///     double conversion: libplctag's <c>pccc.c</c> (<c>parse_pccc_elem_num</c> / <c>parse_pccc_bit_num</c>) parses
+    ///     decimal numerals only (a FIXME there notes octal is unsupported), so handing it the RSLogix octal text would
+    ///     read the wrong word. An indirect
    ///     address re-emits its bracket form, which neither transport accepts — check
    ///     <see cref="IsIndirect"/> first.
    /// </summary>
@@ -86,6 +89,36 @@ public sealed record AbLegacyAddress(
        return ParseCore(value.Trim(), profile, allowIndirect: true);
    }

+    /// <summary>
+    ///     The single authoring / deploy-gate / runtime address check. With a known <paramref name="family"/> the
+    ///     address must parse for that family; with an unknown one (<see langword="null"/>) it must parse for at
+    ///     least one family, so a caller that cannot see the device never rejects what the runtime would accept.
+    /// </summary>
+    /// <param name="address">The authored address text.</param>
+    /// <param name="family">The device's PLC family, or <see langword="null"/> when it is not known.</param>
+    /// <returns>An error message, or <see langword="null"/> when the address is acceptable.</returns>
+    public static string? Validate(string? address, AbLegacyPlcFamily? family)
+    {
+        if (string.IsNullOrWhiteSpace(address)) return "Address is required.";
+        if (family is { } f)
+        {
+            if (TryParse(address, f) is not null) return null;
+            var hint = f == AbLegacyPlcFamily.Plc5 && TryParse(address, AbLegacyPlcFamily.Slc500) is not null
+                ? " (PLC-5 I:/O: word and bit indices are octal - 8 and 9 are invalid)"
+                : IsFunctionLetterPrefix(address) ? " (function files are MicroLogix-only)" : "";
+            return $"'{address}' is not a valid address for a {f} device{hint}.";
+        }
+        foreach (var candidate in Enum.GetValues<AbLegacyPlcFamily>())
+            if (TryParse(address, candidate) is not null) return null;
+        return $"'{address}' is not a valid PCCC file address for any PLC family.";
+    }
+
+    private static bool IsFunctionLetterPrefix(string address)
+    {
+        var colon = address.IndexOf(':');
+        return colon > 0 && IsFunctionFileLetter(address[..colon].Trim().ToUpperInvariant());
+    }
+
    private static AbLegacyAddress? ParseCore(string src, AbLegacyPlcFamilyProfile? profile, bool allowIndirect)
    {
        // BitIndex: trailing /N outside any bracketed inner address. The numeric parse is deferred until the
@@ -254,9 +287,9 @@ public sealed record AbLegacyAddress(

    /// <summary>
    ///     MicroLogix 1100/1400 function-file prefixes — each a single fixed instance with a sub-element
-    ///     catalogue (<see cref="AbLegacyFunctionFile"/>).
+    ///     catalogue (<c>AbLegacyFunctionFile</c>).
    /// </summary>
-    internal static bool IsFunctionFileLetter(string letter) => letter switch
+    public static bool IsFunctionFileLetter(string letter) => letter switch
    {
        "RTC" or "HSC" or "DLS" or "MMI" or "PTO" or "PWM" or "STI" or "EII" or "IOS" or "BHI" => true,
        _ => false,
@@ -1,3 +1,4 @@
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
 using System.Text.Json;
 using System.Text.Json.Nodes;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
@@ -122,6 +123,12 @@ public static class AbLegacyTagDefinitionFactory
    ///     The RawPath handed to <see cref="FromTagConfig"/> during inspection — the mapper only copies it
    ///     onto <see cref="AbLegacyTagDefinition.Name"/> and never validates it.
    /// </summary>
+    private static bool IsIndirectFor(string address, AbLegacyPlcFamily? family)
+    {
+        if (family is { } f) return AbLegacyAddress.TryParse(address, f)?.IsIndirect == true;
+        return Enum.GetValues<AbLegacyPlcFamily>().Any(c => AbLegacyAddress.TryParse(address, c)?.IsIndirect == true);
+    }
+
    private const string InspectionRawPath = "(inspect)";

    /// <summary>
@@ -145,7 +152,18 @@ public static class AbLegacyTagDefinitionFactory
    /// </remarks>
    /// <param name="reference">The equipment tag's TagConfig JSON.</param>
    /// <returns>The warnings; empty when clean.</returns>
-    public static IReadOnlyList<string> Inspect(string reference)
+    public static IReadOnlyList<string> Inspect(string reference) => Inspect(reference, family: null);
+
+    /// <summary>
+    ///     <see cref="Inspect(string)"/> plus the family-aware address check (<see cref="AbLegacyAddress.Validate"/>,
+    ///     the same authority the driver runtime uses). <paramref name="family"/> is the tag's device's PLC family, or
+    ///     <see langword="null"/> when unknown (then only an address no family accepts is flagged). An indirect
+    ///     address parses but the runtime always answers it <c>BadNotSupported</c>, so it is warned about too.
+    /// </summary>
+    /// <param name="reference">The equipment tag's TagConfig JSON.</param>
+    /// <param name="family">The owning device's PLC family, when known.</param>
+    /// <returns>The warnings; empty when clean.</returns>
+    public static IReadOnlyList<string> Inspect(string reference, AbLegacyPlcFamily? family)
    {
        var warnings = new List<string>();
        // Gitea #556 item 1: the framing check TRIMS FIRST. FromTagConfig hands the blob straight to
@@ -157,7 +175,14 @@ public static class AbLegacyTagDefinitionFactory

        // The accept gate: whatever the driver reads draws nothing. The RAW reference is handed to the
        // mapper — the driver reads the stored blob, not a trimmed copy of it.
-        if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings;
+        if (FromTagConfig(reference, InspectionRawPath, out var accepted))
+        {
+            if (AbLegacyAddress.Validate(accepted.Address, family) is { } addressError)
+                warnings.Add($"AbLegacy tag address: {addressError} The driver reads this tag as BadNodeIdUnknown.");
+            else if (IsIndirectFor(accepted.Address, family))
+                warnings.Add($"AbLegacy address '{accepted.Address}' is indirect; indirect addressing is not supported and the tag reads BadNotSupported.");
+            return warnings;
+        }

        try
        {
@@ -357,7 +357,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            {
                // Indirect addresses (N7:[N7:0]) parse but are not resolved at runtime (#247) — answer the
                // refusal before a runtime is created for a name neither transport can encode.
-                if (AbLegacyAddress.TryParse(def.Address, device.Options.PlcFamily) is { IsIndirect: true })
+                var preParsed = AbLegacyAddress.TryParse(def.Address, device.Options.PlcFamily);
+                if (preParsed is null)
+                {
+                    WarnMalformedOnce(device, def);
+                    results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNodeIdUnknown, null, now);
+                    continue;
+                }
+                if (preParsed.IsIndirect)
                {
                    results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now);
                    continue;
@@ -465,8 +472,16 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            {
                var parsed = AbLegacyAddress.TryParse(def.Address, device.Options.PlcFamily);

+                // A malformed address is a config error, not a comms fault (BadNodeIdUnknown, warned once per tag).
+                if (parsed is null)
+                {
+                    WarnMalformedOnce(device, def);
+                    results[i] = new WriteResult(AbLegacyStatusMapper.BadNodeIdUnknown);
+                    continue;
+                }
+
                // Indirect addressing is parse-only (#247): refuse before any runtime or RMW read is created.
-                if (parsed is { IsIndirect: true })
+                if (parsed.IsIndirect)
                {
                    results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported);
                    continue;
@@ -625,7 +640,10 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            Port: state.ParsedAddress.Port,
            CipPath: state.EffectiveCipPath,
            LibplctagPlcAttribute: state.Profile.LibplctagPlcAttribute,
-            TagName: _options.Probe.ProbeAddress!,
+            // Normalised exactly like tag paths (PLC-5 octal I/O -> decimal); an unparseable probe address is passed through
+            // so the transport reports the failure as it always did.
+            TagName: AbLegacyAddress.TryParse(_options.Probe.ProbeAddress, state.Options.PlcFamily)?.ToLibplctagName()
+                     ?? _options.Probe.ProbeAddress!,
            Timeout: _options.Probe.Timeout);

        IAbLegacyTagRuntime? probeRuntime = null;
@@ -873,6 +891,19 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
        keep();
    }

+    /// <summary>
+    ///     A tag whose address does not parse for its device's PLC family (e.g. a PLC-5 octal I/O digit 8/9, a
+    ///     function-file letter off MicroLogix) is a configuration error: answered <c>BadNodeIdUnknown</c> with the
+    ///     handle/health untouched, and warned once per tag so a polled tag does not flood the log.
+    /// </summary>
+    private void WarnMalformedOnce(DeviceState device, AbLegacyTagDefinition def)
+    {
+        if (!device.MalformedAddressLogged.TryAdd(def.Name, 0)) return;
+        _logger.LogWarning(
+            "AbLegacy tag address is not valid for the device's PLC family. Driver={DriverInstanceId} Device={DeviceHostAddress} Reference={Reference} Address={Address} Family={Family}",
+            _driverInstanceId, def.DeviceHostAddress, def.Name, def.Address, device.Options.PlcFamily);
+    }
+
    /// <summary>The create parameters for a tag's own runtime — shared by creation and the write-refusal check, so
    /// the refusal is asked about exactly the runtime a write would otherwise create.</summary>
    private AbLegacyTagCreateParams CreateParamsFor(DeviceState device, AbLegacyTagDefinition def, AbLegacyAddress parsed) =>
@@ -1105,6 +1136,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
        /// </summary>
        public bool FirstNonZeroStatusLogged { get; set; }

+        /// <summary>Tags already warned about for a malformed address (once-per-tag log).</summary>
+        public System.Collections.Concurrent.ConcurrentDictionary<string, byte> MalformedAddressLogged { get; } = new();
+
        /// <summary>
        ///     Disposes all cached tag runtimes.
        /// </summary>
@@ -68,8 +68,11 @@ public sealed class AbLegacyTagConfigModel
        if (TagConfigJson.DescribeInvalidEnum<AbLegacyDataType>(_bag, "dataType") is { } dtError)
            return dtError;

-        if (string.IsNullOrWhiteSpace(Address))
-            return "Address is required.";
+        // The editor cannot see the device's PLC family, so it checks only what no family accepts (the same
+        // AbLegacyAddress authority the deploy gate and runtime use) — never stricter than the gate, which adds the
+        // family-specific verdict (PLC-5 octal I/O, MicroLogix-only function files).
+        if (AbLegacyAddress.Validate(Address, family: null) is { } addressError)
+            return addressError;

        // DELIBERATELY STRICTER THAN THE RUNTIME, and re-confirmed as such by the Gitea #554 survey — do not
        // "align" it. PCCC data-file elements are limited to AbLegacyArray.MaxElements (256). The reader
@@ -262,12 +262,16 @@ public sealed class AdminOperationsActor : ReceiveActor
            var driverByDevice = draft.Devices
                .GroupBy(d => d.DeviceId, StringComparer.Ordinal)
                .ToDictionary(g => g.Key, g => g.First().DriverInstanceId, StringComparer.Ordinal);
+            var deviceConfigById = draft.Devices
+                .GroupBy(d => d.DeviceId, StringComparer.Ordinal)
+                .ToDictionary(g => g.Key, g => g.First().DeviceConfig, StringComparer.Ordinal);
            var tagConfigWarnings = new List<(string TagId, string Text)>();
            foreach (var t in draft.Tags)
            {
                if (!driverByDevice.TryGetValue(t.DeviceId, out var driverInstanceId)) continue;
                if (!driverTypeById.TryGetValue(driverInstanceId, out var driverType)) continue;
-                foreach (var w in EquipmentTagConfigInspector.Inspect(driverType, t.TagConfig))
+                foreach (var w in EquipmentTagConfigInspector.Inspect(
+                    driverType, t.TagConfig, deviceConfigById.GetValueOrDefault(t.DeviceId)))
                    tagConfigWarnings.Add((t.TagId, $"tag '{t.TagId}': {w}"));
            }
            if (tagConfigWarnings.Count > 0)
@@ -1,6 +1,7 @@
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
 using ZB.MOM.WW.OtOpcUa.Driver.AbCip;
 using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy;
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
 using ZB.MOM.WW.OtOpcUa.Driver.Calculation;
 using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
 using ZB.MOM.WW.OtOpcUa.Driver.Modbus;
@@ -78,6 +79,47 @@ public static class EquipmentTagConfigInspector
        return Inspectors.TryGetValue(driverType, out var inspect) ? inspect(tagConfig) : Array.Empty<string>();
    }

+    /// <summary>
+    ///     <see cref="Inspect(string?, string?)"/> with the owning device's <c>DeviceConfig</c>, so a driver whose
+    ///     address grammar depends on the device (AbLegacy: the <c>PlcFamily</c>) is checked against the family the
+    ///     runtime will use. Other drivers ignore the device config. A missing family key is the runtime default
+    ///     (SLC 500); an unrecognised value is "unknown" and only the family-independent check applies — the gate
+    ///     is never stricter than the runtime.
+    /// </summary>
+    /// <param name="driverType">The bound driver's <c>DriverType</c>.</param>
+    /// <param name="tagConfig">The equipment tag's <c>TagConfig</c> JSON.</param>
+    /// <param name="deviceConfig">The owning device's <c>DeviceConfig</c> JSON (may be null).</param>
+    /// <returns>The warnings; empty when clean.</returns>
+    public static IReadOnlyList<string> Inspect(string? driverType, string? tagConfig, string? deviceConfig)
+    {
+        if (string.IsNullOrEmpty(driverType) || tagConfig is null) return Array.Empty<string>();
+        if (string.Equals(driverType, DriverTypeNames.AbLegacy, StringComparison.OrdinalIgnoreCase))
+            return AbLegacyTagDefinitionFactory.Inspect(tagConfig, ReadAbLegacyFamily(deviceConfig));
+        return Inspect(driverType, tagConfig);
+    }
+
+    private static AbLegacyPlcFamily? ReadAbLegacyFamily(string? deviceConfig)
+    {
+        if (string.IsNullOrWhiteSpace(deviceConfig)) return AbLegacyPlcFamily.Slc500;
+        try
+        {
+            using var doc = System.Text.Json.JsonDocument.Parse(deviceConfig);
+            if (doc.RootElement.ValueKind != System.Text.Json.JsonValueKind.Object) return AbLegacyPlcFamily.Slc500;
+            foreach (var prop in doc.RootElement.EnumerateObject())
+            {
+                if (!string.Equals(prop.Name, "PlcFamily", StringComparison.OrdinalIgnoreCase)) continue;
+                var raw = prop.Value.ValueKind == System.Text.Json.JsonValueKind.String ? prop.Value.GetString() : null;
+                if (string.IsNullOrWhiteSpace(raw)) return AbLegacyPlcFamily.Slc500;
+                return Enum.TryParse<AbLegacyPlcFamily>(raw, ignoreCase: true, out var f) && Enum.IsDefined(f) ? f : null;
+            }
+            return AbLegacyPlcFamily.Slc500;
+        }
+        catch (System.Text.Json.JsonException)
+        {
+            return null;
+        }
+    }
+
    /// <summary>Whether the given driver type has a mapped inspector.</summary>
    /// <param name="driverType">The bound driver's <c>DriverType</c>.</param>
    /// <returns><see langword="true"/> when an inspector is registered for the driver type.</returns>