fix(twincat): review fixes for sum-read and diagnostics (#645)

Strict width+kind eligibility for sum-batched scalars (others read singly); latch only on ADS
1793/1794, split 1792/1797/1798 without latching; 32 KiB per-command budget and 500-item ceiling;
short sum response handled per item. Diagnostics: opt-in diagnosticsEnabled (default off), reuse the
probe's ReadState, sample after the host-state transition, JitterMs renamed HeadroomMs, compact
per-device line so many devices fit the 16-entry cap, unavailable-symbol latch cleared on reconnect.
AdminUI: min/max on sum-read max, diagnostics checkbox. Pure planner extracted and unit-tested.

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs

@@ -109,10 +109,8 @@ internal class FakeTwinCATClient : ITwinCATClient
         return Task.FromResult<IReadOnlyList<(object? value, uint status)>>(results);
     }
 
-    /// <summary>Simulates reading the ADS device state.</summary>
-    /// <param name="ct">The cancellation token.</param>
-    /// <returns>The configured state text.</returns>
-    public virtual Task<string?> ReadAdsStateAsync(CancellationToken ct) => Task.FromResult(AdsStateText);
+    /// <summary>Gets the ADS state the "probe" last read.</summary>
+    public string? LastAdsState => AdsStateText;
 
     /// <summary>Simulates writing a value to a symbol.</summary>
     /// <param name="symbolPath">The path to the symbol to write.</param>

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATDiagnosticsTests.cs

@@ -12,11 +12,12 @@ public sealed class TwinCATDiagnosticsTests
     private const string Host1 = "ads://5.23.91.23.1.1:851";
     private const string Host2 = "ads://5.23.91.24.1.1:851";
 
-    private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, bool probe, params (string host, string? name)[] devices) =>
+    private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, bool probe, bool diagnostics, params (string host, string? name)[] devices) =>
         new(new TwinCATDriverOptions
         {
             Devices = [.. devices.Select(d => new TwinCATDeviceOptions(d.host, d.name))],
             Probe = new TwinCATProbeOptions { Enabled = probe, Interval = TimeSpan.FromMilliseconds(20) },
+            DiagnosticsEnabled = diagnostics,
         }, "drv-diag", factory);
 
     private static FakeTwinCATClient PlcClient() => new()
@@ -41,7 +42,7 @@ public sealed class TwinCATDiagnosticsTests
     [Fact]
     public async Task Bag_is_empty_before_the_first_sample()
     {
-        var drv = NewDriver(new FakeTwinCATClientFactory(), false, (Host1, null));
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, (Host1, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
 
         var diag = drv.ShouldBeAssignableTo<IDriverDiagnostics>()!;
@@ -52,7 +53,7 @@ public sealed class TwinCATDiagnosticsTests
     [Fact]
     public async Task Single_device_publishes_flat_keys()
     {
-        var drv = NewDriver(new FakeTwinCATClientFactory(), false, (Host1, null));
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, (Host1, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
         await Sample(drv, PlcClient(), Host1);
 
@@ -63,7 +64,7 @@ public sealed class TwinCATDiagnosticsTests
         bag["TwinCAT.OnlineChangeCnt"].ShouldBe("7");
         bag["TwinCAT.CycleTimeMs"].ShouldBe("10.0");
         bag["TwinCAT.LastExecTimeMs"].ShouldBe("2.5");
-        bag["TwinCAT.JitterMs"].ShouldBe("-7.5");
+        bag["TwinCAT.HeadroomMs"].ShouldBe("7.5");
         DriverDiagnostics.Bound(bag).Count.ShouldBe(bag.Count); // within the host's 16-entry cap
     }
 
@@ -71,15 +72,15 @@ public sealed class TwinCATDiagnosticsTests
     [Fact]
     public async Task Multiple_devices_are_prefixed()
     {
-        var drv = NewDriver(new FakeTwinCATClientFactory(), false, (Host1, "Elma01"), (Host2, null));
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, (Host1, "Elma01"), (Host2, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
         await Sample(drv, PlcClient(), Host1);
         await Sample(drv, PlcClient(), Host2);
 
         var bag = ((IDriverDiagnostics)drv).GetDiagnostics();
 
-        bag["TwinCAT.Elma01.AdsState"].ShouldBe("Run");
-        bag[$"TwinCAT.{Host2}.CycleTimeMs"].ShouldBe("10.0");
+        bag["TwinCAT.Elma01"].ShouldBe("Run; Plc30 App; online-changes 7; cycle 10.0 ms; exec 2.5 ms; headroom 7.5 ms");
+        bag[$"TwinCAT.{Host2}"].ShouldContain("cycle 10.0 ms");
         bag.ContainsKey("TwinCAT.AdsState").ShouldBeFalse();
     }
 
@@ -88,7 +89,7 @@ public sealed class TwinCATDiagnosticsTests
     [Fact]
     public async Task Missing_task_info_gives_a_partial_bag_and_is_not_re_asked()
     {
-        var drv = NewDriver(new FakeTwinCATClientFactory(), false, (Host1, null));
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, (Host1, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
         var client = new FakeTwinCATClient { AdsStateText = "Run" };
         client.Values[TwinCATDriver.SymAppName] = "Plc1";
@@ -104,7 +105,7 @@ public sealed class TwinCATDiagnosticsTests
         bag["TwinCAT.AdsState"].ShouldBe("Run");
         bag["TwinCAT.AppName"].ShouldBe("Plc1");
         bag.Keys.ShouldNotContain("TwinCAT.CycleTimeMs");
-        bag.Keys.ShouldNotContain("TwinCAT.JitterMs");
+        bag.Keys.ShouldNotContain("TwinCAT.HeadroomMs");
         client.SingleReadCount.ShouldBe(readsAfterFirst + 1); // only AppName is read again
     }
 
@@ -112,7 +113,7 @@ public sealed class TwinCATDiagnosticsTests
     [Fact]
     public async Task Failed_sample_keeps_the_last_values()
     {
-        var drv = NewDriver(new FakeTwinCATClientFactory(), false, (Host1, null));
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, (Host1, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
         var client = PlcClient();
         await Sample(drv, client, Host1);
@@ -131,7 +132,7 @@ public sealed class TwinCATDiagnosticsTests
     public async Task Probe_loop_populates_the_bag()
     {
         var factory = new FakeTwinCATClientFactory { Customise = PlcClient };
-        var drv = NewDriver(factory, true, (Host1, null));
+        var drv = NewDriver(factory, true, true, (Host1, null));
         await drv.InitializeAsync("{}", CancellationToken.None);
         try
         {
@@ -145,4 +146,64 @@ public sealed class TwinCATDiagnosticsTests
         }
         finally { await drv.ShutdownAsync(CancellationToken.None); }
     }
+
+    /// <summary>Verifies a bag for the maximum number of devices fits the host's cap (one line per device).</summary>
+    [Fact]
+    public async Task Many_devices_all_fit_the_bag_cap()
+    {
+        var hosts = Enumerable.Range(1, 12).Select(i => ($"ads://5.23.91.{i}.1.1:851", (string?)$"D{i}")).ToArray();
+        var drv = NewDriver(new FakeTwinCATClientFactory(), false, true, hosts);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        foreach (var (host, _) in hosts) await Sample(drv, PlcClient(), host);
+
+        var bag = DriverDiagnostics.Bound(((IDriverDiagnostics)drv).GetDiagnostics());
+
+        bag.Count.ShouldBe(12);
+    }
+
+    /// <summary>Verifies sampling is opt-in: with it off the probe loop adds no reads and the bag stays empty.</summary>
+    [Fact]
+    public async Task Disabled_by_default_adds_no_reads()
+    {
+        var factory = new FakeTwinCATClientFactory { Customise = PlcClient };
+        var drv = NewDriver(factory, true, false, (Host1, null));
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        try
+        {
+            for (var i = 0; i < 100 && drv.GetHostStatuses().All(h => h.State != HostState.Running); i++)
+                await Task.Delay(20);
+            await Task.Delay(100); // several more probe ticks
+        }
+        finally { await drv.ShutdownAsync(CancellationToken.None); }
+
+        factory.Clients[0].SingleReadCount.ShouldBe(0);
+        ((IDriverDiagnostics)drv).GetDiagnostics().ShouldBeEmpty();
+    }
+
+    /// <summary>Verifies a symbol the PLC did not export is asked for again once the client is rebuilt.</summary>
+    [Fact]
+    public async Task Unavailable_symbols_are_cleared_when_a_new_client_is_installed()
+    {
+        var factory = new FakeTwinCATClientFactory();
+        var drv = new TwinCATDriver(new TwinCATDriverOptions
+        {
+            Devices = [new TwinCATDeviceOptions(Host1)],
+            RawTags = TwinCATRawTags.Entries([new TwinCATTagDefinition("A", Host1, "MAIN.A", TwinCATDataType.DInt)]),
+            Probe = new TwinCATProbeOptions { Enabled = false },
+            DiagnosticsEnabled = true,
+        }, "drv-diag", factory);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        var first = new FakeTwinCATClient();
+        first.ReadStatuses[TwinCATDriver.SymCycleTime] = TwinCATStatusMapper.BadNodeIdUnknown;
+        factory.Customise = () => first;
+        await drv.ReadAsync(["A"], CancellationToken.None); // installs the first client
+        await Sample(drv, first, Host1);
+        drv.GetDeviceState(Host1)!.UnavailableDiagSymbols.ContainsKey(TwinCATDriver.SymCycleTime).ShouldBeTrue();
+
+        first.SimulateWireDrop();
+        factory.Customise = () => new FakeTwinCATClient();
+        await drv.ReadAsync(["A"], CancellationToken.None); // reconnect installs a new client
+
+        drv.GetDeviceState(Host1)!.UnavailableDiagSymbols.ShouldBeEmpty();
+    }
 }

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumReadPlannerTests.cs

@@ -0,0 +1,107 @@
+using Shouldly;
+using TwinCAT.Ads;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests;
+
+/// <summary>The pure sum-read decisions (Gitea #645) — the part of the wire path a fake client cannot reach.</summary>
+[Trait("Category", "Unit")]
+public sealed class TwinCATSumReadPlannerTests
+{
+    private static TwinCATPlcSymbolShape Plc(string name, int size, bool isFloat = false) =>
+        new(TwinCATPlcKind.Scalar, size, isFloat, name);
+
+    /// <summary>Verifies the strict width + kind rule: only an exact match rides a sum command, the lenient
+    /// CheckRead pairs (value-converted by Beckhoff on a single read) are read singly instead.</summary>
+    [Theory]
+    [InlineData(TwinCATDataType.DInt, "DINT", 4, false, true)]
+    [InlineData(TwinCATDataType.DInt, "UDINT", 4, false, true)]    // same width + kind: tolerated
+    [InlineData(TwinCATDataType.Real, "REAL", 4, true, true)]
+    [InlineData(TwinCATDataType.Bool, "BOOL", 1, false, true)]
+    [InlineData(TwinCATDataType.DInt, "INT", 2, false, false)]     // would over-read into the neighbour
+    [InlineData(TwinCATDataType.Real, "LREAL", 8, true, false)]    // half a double
+    [InlineData(TwinCATDataType.Real, "DINT", 4, false, false)]    // bit pattern reinterpreted
+    [InlineData(TwinCATDataType.DInt, "REAL", 4, true, false)]
+    [InlineData(TwinCATDataType.Int, "DINT", 4, false, false)]     // truncation
+    public void Scalar_eligibility_needs_exact_width_and_kind(
+        TwinCATDataType declared, string plcType, int plcSize, bool plcFloat, bool eligible)
+    {
+        var shape = Plc(plcType, plcSize, plcFloat);
+
+        TwinCATSumReadPlanner.ScalarEligible(declared, shape).ShouldBe(eligible);
+        // The mismatching pairs are exactly the ones the lenient single-read check lets through.
+        if (!eligible) TwinCATTypeGuard.CheckRead(declared, shape).ShouldBeNull();
+    }
+
+    /// <summary>Verifies a string tag over a numeric symbol is not eligible.</summary>
+    [Fact]
+    public void A_string_tag_over_a_numeric_symbol_is_not_eligible() =>
+        TwinCATSumReadPlanner.ScalarEligible(TwinCATDataType.String, Plc("DINT", 4)).ShouldBeFalse();
+
+    /// <summary>Verifies only service / index-group errors latch; request-size errors split without latching.</summary>
+    [Theory]
+    [InlineData(1793u, true, false)]
+    [InlineData(1794u, true, false)]
+    [InlineData(1792u, false, true)]
+    [InlineData(1797u, false, true)]
+    [InlineData(1798u, false, true)]
+    [InlineData(1795u, false, false)]
+    [InlineData(6u, false, false)]
+    [InlineData(1861u, false, false)]
+    [InlineData(1808u, false, false)]
+    public void Overall_ads_codes_classify(uint code, bool refusal, bool rejected)
+    {
+        TwinCATSumReadPlanner.IsRefusal(code).ShouldBe(refusal);
+        TwinCATSumReadPlanner.IsRequestRejected(code).ShouldBe(rejected);
+    }
+
+    /// <summary>Verifies the byte budget splits a poll and gives an oversized item a group of its own.</summary>
+    [Fact]
+    public void Partition_respects_the_byte_budget()
+    {
+        // cost = length + 4:   104, 104, 104 | 1004 (own group) | 14
+        var groups = TwinCATSumReadPlanner.Partition([100, 100, 100, 1000, 10], 330);
+
+        groups.Select(g => g.ToArray()).ToArray().ShouldBe([[0, 1, 2], [3], [4]]);
+    }
+
+    /// <summary>Verifies an empty poll partitions to nothing.</summary>
+    [Fact]
+    public void Partition_of_nothing_is_empty() =>
+        TwinCATSumReadPlanner.Partition([], 1000).ShouldBeEmpty();
+
+    /// <summary>Verifies a Good block decodes to a scalar / an array.</summary>
+    [Fact]
+    public void Decode_returns_scalar_and_array_values()
+    {
+        byte[] four = [0x2A, 0, 0, 0];
+        var scalar = TwinCATSumReadPlanner.DecodeItem(TwinCATDataType.DInt, AdsErrorCode.NoError, four,
+            new TwinCATArrayLayout(1, 4), false, false, TwinCATStatusMapper.MapAdsError);
+        scalar.ShouldBe((42, TwinCATStatusMapper.Good));
+
+        byte[] arr = [1, 0, 2, 0];
+        var array = TwinCATSumReadPlanner.DecodeItem(TwinCATDataType.Int, AdsErrorCode.NoError, arr,
+            new TwinCATArrayLayout(2, 2), true, false, TwinCATStatusMapper.MapAdsError);
+        array.status.ShouldBe(TwinCATStatusMapper.Good);
+        array.value.ShouldBe(new short[] { 1, 2 });
+    }
+
+    /// <summary>Verifies a per-item ADS error maps to that item.</summary>
+    [Fact]
+    public void Decode_maps_a_sub_error() =>
+        TwinCATSumReadPlanner.DecodeItem(TwinCATDataType.DInt, (AdsErrorCode)1808, null,
+            new TwinCATArrayLayout(1, 4), false, false, TwinCATStatusMapper.MapAdsError)
+            .ShouldBe((null, TwinCATStatusMapper.BadNodeIdUnknown));
+
+    /// <summary>Verifies a short or missing block is a size error for that item, never an uncaught throw.</summary>
+    [Fact]
+    public void Decode_of_a_short_or_missing_block_is_a_per_item_size_error()
+    {
+        var expected = (default(object), TwinCATStatusMapper.MapAdsError(1797));
+        TwinCATSumReadPlanner.DecodeItem(TwinCATDataType.DInt, AdsErrorCode.NoError, new byte[] { 1, 2 },
+            new TwinCATArrayLayout(1, 4), false, false, TwinCATStatusMapper.MapAdsError).ShouldBe(expected);
+        TwinCATSumReadPlanner.DecodeItem(TwinCATDataType.DInt, AdsErrorCode.NoError, null,
+            new TwinCATArrayLayout(1, 4), false, false, TwinCATStatusMapper.MapAdsError).ShouldBe(expected);
+    }
+}

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumReadTests.cs

@@ -211,20 +211,6 @@ public sealed class TwinCATSumReadTests
         off.SumReadMaxItems.ShouldBe(200);
     }
 
-    /// <summary>Verifies which overall ADS codes mean "no sum commands here" rather than a comms fault.</summary>
-    [Theory]
-    [InlineData(1792u, true)]
-    [InlineData(1793u, true)]
-    [InlineData(1794u, true)]
-    [InlineData(1795u, true)]
-    [InlineData(1797u, true)]
-    [InlineData(1798u, true)]
-    [InlineData(6u, false)]    // TargetPortNotFound
-    [InlineData(1861u, false)] // ClientSyncTimeOut
-    [InlineData(1808u, false)] // symbol not found: a per-item matter
-    public void Overall_ads_codes_classify_as_refusal_only_for_service_errors(uint code, bool refusal) =>
-        AdsTwinCATClient.IsSumRefusal(code).ShouldBe(refusal);
-
     private sealed class CapturingClient(List<TwinCATBulkReadItem> sink) : FakeTwinCATClient
     {
         public override Task<IReadOnlyList<(object? value, uint status)>> ReadValuesAsync(

tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/TwinCATDriverPageFormSerializationTests.cs

@@ -74,6 +74,7 @@ public sealed class TwinCATDriverPageFormSerializationTests
             NotificationMaxDelayMs = 100,
             UseSumRead = true,
             SumReadMaxItems = 64,
+            DiagnosticsEnabled = true,
             Probe = new TwinCATProbeOptions
             {
                 Enabled = true,
@@ -92,6 +93,7 @@ public sealed class TwinCATDriverPageFormSerializationTests
         roundTripped.NotificationMaxDelayMs.ShouldBe(100);
         roundTripped.UseSumRead.ShouldBeTrue();
         roundTripped.SumReadMaxItems.ShouldBe(64);
+        roundTripped.DiagnosticsEnabled.ShouldBeTrue();
         roundTripped.Probe.Enabled.ShouldBeTrue();
         roundTripped.Probe.Interval.ShouldBe(TimeSpan.FromSeconds(6));
         roundTripped.Probe.Timeout.ShouldBe(TimeSpan.FromSeconds(2));