Merge pull request '[s7] S7 — PDU size negotiation surfaced' (#374) from auto/s7/PR-S7-C1 into auto/driver-gaps

docs/drivers/S7-Test-Fixture.md

@@ -44,6 +44,10 @@ The driver ctor change that made this possible:
   bool-with-bit in one batch call; proves typed decode per S7DataType
 - `S7_1500SmokeTests.Driver_write_then_read_round_trip_on_scratch_word` —
   `DB1.DBW100` write → read-back; proves write path + buffer visibility
+- `S7_1500DiagnosticsTests.Driver_exposes_negotiated_pdu_size_post_init` —
+  asserts `DriverHealth.Diagnostics["S7.NegotiatedPduSize"]` is non-zero
+  after `InitializeAsync`; proves the negotiated PDU size surfaces in
+  driver health (Snap7 fixture pins this at 240 bytes — see fixture README)
 
 ### Unit
 

docs/v2/s7.md

@@ -548,6 +548,31 @@ under the standard `<DriverType>.<Counter>` naming convention:
 Observe via the `driver-diagnostics` RPC (`/api/v2/drivers/{id}/diagnostics`)
 or the Admin UI's per-driver dashboard.
 
+### Diagnostics surfacing
+
+Beyond the coalescing counters above, the S7 driver also surfaces the
+**negotiated PDU size** captured during the COTP/S7comm handshake under the
+same `<DriverType>.<Counter>` naming convention:
+
+- `S7.NegotiatedPduSize` — the PDU envelope size advertised by the CPU
+  during `Plc.OpenAsync`. Default S7-1500 CPUs negotiate **240 bytes**;
+  CPUs running the extended PDU advertise **480 or 960 bytes**. The value
+  is `0` before the first successful connect and is reset to `0` on
+  driver shutdown so an operator inspecting the Admin UI dashboard can
+  immediately tell whether the driver is currently online.
+
+Together these counters answer the most common operator questions about
+S7 driver health without reaching for a Wireshark capture:
+
+- "Is the driver actually connected?" → `S7.NegotiatedPduSize > 0`
+- "Is coalescing working?" → `S7.TotalBlockReads` climbing while
+  `S7.TotalMultiVarBatches` stays flat
+- "Why is throughput poor?" → `S7.NegotiatedPduSize` is 240 instead of 960
+  (operator can switch the CPU to extended PDU if the project allows)
+
+The values render alongside Modbus / OPC UA Client metrics in the Admin
+UI driver-diagnostics panel — same RPC, same dashboard row layout.
+
 ## References
 
 1. Siemens Industry Online Support, *Modbus/TCP Communication between SIMATIC S7-1500 / S7-1200 and Modbus/TCP Controllers with Instructions `MB_CLIENT` and `MB_SERVER`*, Entry ID 102020340, V6 (Feb 2021). https://cache.industry.siemens.com/dl/files/340/102020340/att_118119/v6/net_modbus_tcp_s7-1500_s7-1200_en.pdf

src/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs

@@ -98,6 +98,21 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
     private long _totalMultiVarBatches;   // Plc.ReadMultipleVarsAsync calls issued
     private long _totalSingleReads;       // per-tag ReadOneAsync fallbacks
 
+    /// <summary>
+    ///     Negotiated PDU size from the most recent <see cref="Plc.OpenAsync"/>. Snapshotted
+    ///     once into a field so the diagnostics dictionary keeps a stable reading even after
+    ///     the underlying <c>Plc</c> instance is closed (e.g. mid-reinit). Resets to 0 on
+    ///     <see cref="ShutdownAsync"/> so a stale post-disconnect reading never confuses an
+    ///     operator inspecting the driver-diagnostics panel.
+    /// </summary>
+    private int _negotiatedPduSize;
+
+    /// <summary>
+    ///     Test-only entry point for the negotiated PDU size that's surfaced via
+    ///     <see cref="DriverHealth.Diagnostics"/> as <c>S7.NegotiatedPduSize</c>.
+    /// </summary>
+    internal int NegotiatedPduSize => _negotiatedPduSize;
+
     /// <summary>
     ///     Total <c>Plc.ReadBytesAsync</c> calls the coalesced byte-range path issued.
     ///     Test-only entry point for the integration assertion that 50 contiguous DBWs
@@ -160,8 +175,13 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
             await plc.OpenAsync(cts.Token).ConfigureAwait(false);
 
             Plc = plc;
+            // S7netplus exposes the PDU size negotiated during the COTP/S7comm handshake on
+            // Plc.MaxPDUSize. Snapshot once so the diagnostics surface (S7.NegotiatedPduSize)
+            // doesn't have to dereference Plc on every BuildDiagnostics() call. Default S7-1500
+            // CPUs negotiate 240 bytes; CPUs running the extended PDU advertise 480 or 960.
+            _negotiatedPduSize = plc.MaxPDUSize;
 
-            _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null);
+            _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null, BuildDiagnostics());
 
             // Kick off the probe loop once the connection is up. Initial HostState stays
             // Unknown until the first probe tick succeeds — avoids broadcasting a premature
@@ -204,6 +224,9 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
 
         try { Plc?.Close(); } catch { /* best-effort — tearing down anyway */ }
         Plc = null;
+        // Reset the snapshot so a post-shutdown diagnostics read doesn't display a stale
+        // PDU size from the previous connection. Reinit will repopulate after OpenAsync.
+        _negotiatedPduSize = 0;
         _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
         return Task.CompletedTask;
     }
@@ -422,6 +445,10 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
         ["S7.TotalBlockReads"] = Interlocked.Read(ref _totalBlockReads),
         ["S7.TotalMultiVarBatches"] = Interlocked.Read(ref _totalMultiVarBatches),
         ["S7.TotalSingleReads"] = Interlocked.Read(ref _totalSingleReads),
+        // Negotiated PDU size from the COTP/S7comm handshake — 240 bytes on a default
+        // S7-1500 CPU, 480 or 960 on CPUs running the extended PDU. 0 before connect /
+        // after shutdown so an operator can tell the driver isn't currently online.
+        ["S7.NegotiatedPduSize"] = _negotiatedPduSize,
     };
 
     /// <summary>

tests/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md

@@ -67,6 +67,18 @@ DB1 (1024 bytes) + MB (256 bytes) with typed seeds at known offsets:
 Seed types supported: `u8`, `i8`, `u16`, `i16`, `u32`, `i32`, `f32`,
 `bool` (with `"bit": 0..7`), `ascii` (S7 STRING).
 
+## Negotiated PDU size
+
+Snap7's `Server` always negotiates a **fixed 240-byte PDU** during the
+COTP/S7comm handshake (the legacy default — Snap7 does not implement the
+S7-1500 extended-PDU negotiation). The S7 driver surfaces this value
+through `DriverHealth.Diagnostics["S7.NegotiatedPduSize"]`, exercised by
+`S7_1500DiagnosticsTests.Driver_exposes_negotiated_pdu_size_post_init`.
+
+Operators inspecting a driver against this fixture should therefore see
+`S7.NegotiatedPduSize = 240`. Real S7-1500 CPUs running the extended PDU
+report 480 or 960; that branch is hardware-only and not exercised here.
+
 ## Known limitations
 
 From the `snap7.server.Server` docstring upstream:

tests/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500DiagnosticsTests.cs

@@ -0,0 +1,42 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500;
+
+/// <summary>
+///     End-to-end coverage for the S7 driver's diagnostics surface against the python-snap7
+///     S7-1500 simulator. The simulator runs the upstream Snap7 server which negotiates a
+///     fixed 240-byte PDU during the COTP handshake; this suite asserts the driver captures
+///     and surfaces that value through <c>DriverHealth.Diagnostics["S7.NegotiatedPduSize"]</c>
+///     so the Admin UI driver-diagnostics panel can render it.
+/// </summary>
+[Collection(Snap7ServerCollection.Name)]
+[Trait("Category", "Integration")]
+[Trait("Device", "S7_1500")]
+public sealed class S7_1500DiagnosticsTests(Snap7ServerFixture sim)
+{
+    [Fact]
+    public async Task Driver_exposes_negotiated_pdu_size_post_init()
+    {
+        if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason);
+
+        var options = S7_1500Profile.BuildOptions(sim.Host, sim.Port);
+        await using var drv = new S7Driver(options, driverInstanceId: "s7-pdu-diag");
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+
+        // Issue a read so BuildDiagnostics() flows into _health.Diagnostics through the
+        // Healthy state-transition (the post-OpenAsync construction also seeds it, but we
+        // exercise the read path too so the assertion is robust against future refactors
+        // that move the seed point).
+        var snapshots = await drv.ReadAsync(
+            [S7_1500Profile.ProbeTag], TestContext.Current.CancellationToken);
+        snapshots[0].StatusCode.ShouldBe(0u, "probe read must succeed before checking diagnostics");
+
+        var diagnostics = drv.GetHealth().DiagnosticsOrEmpty;
+        diagnostics.ContainsKey("S7.NegotiatedPduSize").ShouldBeTrue(
+            "S7 driver must surface negotiated PDU size as S7.NegotiatedPduSize");
+        diagnostics["S7.NegotiatedPduSize"].ShouldBeGreaterThan(0,
+            "Snap7 negotiates a 240-byte PDU on every COTP handshake — anything ≤ 0 means the snapshot was missed");
+    }
+}

tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverDiagnosticsTests.cs

@@ -0,0 +1,54 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests;
+
+/// <summary>
+///     Unit-level coverage for the <c>DriverHealth.Diagnostics</c> dictionary the S7 driver
+///     exposes through <see cref="S7Driver.GetHealth"/>. Wire-level reads of
+///     <c>S7.NegotiatedPduSize</c> against a real handshake live in the Snap7 integration
+///     suite (where the simulator negotiates a fixed 240-byte PDU); this suite covers the
+///     before-connect / after-shutdown contract on the same key without booting a fixture.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class S7DriverDiagnosticsTests
+{
+    [Fact]
+    public void NegotiatedPduSize_diagnostics_key_starts_at_zero_before_initialize()
+    {
+        // Before InitializeAsync the driver hasn't talked to a PLC, so the negotiated PDU
+        // is unknown. We surface 0 (rather than omitting the key) so the Admin UI driver-
+        // diagnostics panel can render a stable row even pre-connect — operators see "0"
+        // and immediately know the driver hasn't completed a handshake yet.
+        using var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-pre-init");
+
+        var health = drv.GetHealth();
+        // Pre-init the driver's state is Unknown and Diagnostics is null (the empty dict);
+        // the field-backed counter is what we directly assert here.
+        drv.NegotiatedPduSize.ShouldBe(0);
+        // The DriverHealth surface should also be safe to consume.
+        health.DiagnosticsOrEmpty.ContainsKey("S7.NegotiatedPduSize").ShouldBeFalse(
+            "pre-init Diagnostics is null and DiagnosticsOrEmpty returns the empty dict");
+    }
+
+    [Fact]
+    public async Task NegotiatedPduSize_resets_to_zero_after_shutdown()
+    {
+        // Even when InitializeAsync fails (unreachable host), the field must remain at 0
+        // so a subsequent ShutdownAsync doesn't carry a stale value forward into the next
+        // re-init attempt. RFC 5737 documentation IP guarantees a fast TCP timeout.
+        var opts = new S7DriverOptions { Host = "192.0.2.1", Timeout = TimeSpan.FromMilliseconds(250) };
+        using var drv = new S7Driver(opts, "s7-shutdown");
+
+        await Should.ThrowAsync<Exception>(async () =>
+            await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
+
+        // The Faulted path in InitializeAsync's catch block doesn't touch _negotiatedPduSize
+        // (we never reached the post-OpenAsync capture) so it should still be 0.
+        drv.NegotiatedPduSize.ShouldBe(0);
+
+        await drv.ShutdownAsync(TestContext.Current.CancellationToken);
+        drv.NegotiatedPduSize.ShouldBe(0, "ShutdownAsync must zero the negotiated PDU snapshot");
+    }
+}