diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index ca9f7886..1125ca20 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -148,8 +148,10 @@ Two things to know before relying on any of this: `refuseArrayOnScalar` — pass-through drivers keep their browse-committed scalar-shaped array tags writable; a new sink wrapper must forward that argument, and the reflection guard checks only that the call arrives); `IndexRange` writes ⇒ `BadWriteNotSupported`, `ValueRank > 1` out of scope. An - array node is writable only when the driver can write the array (`DriverArrayWriteSupport`: Modbus bounded - by `maxRegistersPerWrite`/coil cap, S7 non-Bool/non-wide types, OpcUaClient, Galaxy signed types **with a + array node is writable only when the driver can write the array (`DriverArrayWriteSupport`: Modbus keys-addressed tags bounded + by `maxRegistersPerWrite` (never above FC16's 123)/coil cap/address 65535 incl. String/BitInRegister arrays + (BitInRegister also by `maxRegistersPerRead`: its RMW is one unchunked FC03) (#741; a string-only `addressString` tag + stays read-only — family unknown at compose time), S7 non-Bool/non-wide types, OpcUaClient, Galaxy signed types **with a declared `arrayLength`** — an undeclared length stays read-only because a short whole-array write would resize the Galaxy attribute; AbCip atomic non-BOOL types, AbLegacy Int/Long/Float on the matching data file (no `/bit`/`.sub`/indirect/`S:`/`I:`/`O:`, not native PLC-5, ≤ 256), TwinCAT every codec type but a BOOL diff --git a/docs/Uns.md b/docs/Uns.md index 8de7c783..ce3dd85e 100644 --- a/docs/Uns.md +++ b/docs/Uns.md @@ -323,7 +323,7 @@ carried on the plan records (`RawTagPlan.ArrayWritable`, mirrored onto every UNS | Driver | Array writes | Element types | |---|---|---| -| **Modbus** | Yes | `region: Coils` (FC15, any element, ≤ 1968 coils); `region: HoldingRegisters` with `Int16`/`UInt16`/`Bcd16`/`Int32`/`UInt32`/`Bcd32`/`Float32`/`Int64`/`UInt64`/`Float64` (FC16) whose `arrayLength × registers-per-element` fits `maxRegistersPerWrite` (default 123; the smallest value set on the driver or the tag's device wins) — the driver refuses rather than splits an over-cap FC16, so e.g. a 62-element `Int32` array (124 registers) stays read-only. Not BitInRegister / String / Bool-in-register; input regions are read-only | +| **Modbus** | Yes (String / BitInRegister arrays since #741) | Tags addressed by `region` + `address` keys (strict reads: an integer address 0–65535, enum strings that name a member). `region: Coils` (FC15, ≤ 1968 coils) with **any** `dataType` — a bit region is coerced to Bool, never refused. `region: HoldingRegisters` (FC16) with a fixed-width numeric type (`Int16`/`UInt16`/`Bcd16`/`Int32`/`UInt32`/`Bcd32`/`Float32`/`Int64`/`UInt64`/`Float64`), `String` (`(stringLength+1)/2` registers per element, `stringLength >= 1`) or `BitInRegister` (one register per element, read-modify-written in one FC03 + one FC16, `bitIndex` 0–15, and — because of that unchunked FC03 — also within `maxRegistersPerRead`, default and protocol ceiling 125), whose `arrayLength × registers-per-element` fits `maxRegistersPerWrite` (default 123; the smallest value set on the driver or the tag's device wins; a value above FC16's protocol ceiling of 123 never lifts it, since a conformant device refuses a longer FC16) — the driver refuses rather than splits an over-cap FC16, so e.g. a 62-element `Int32` array (124 registers) stays read-only. Every span (registers or coils) must end at or before address 65535. FC23 (opt-in) never changes the answer. Not Bool-in-register; input regions are read-only. A **string-only** tag (`addressString`, no keys) stays read-only: the driver resolves it with its own family, which the composer cannot know — use the editor's Resolve to pin the keys | | **S7** | Yes | `Byte`/`Int16`/`UInt16`/`Int32`/`UInt32`/`Float32` (one contiguous block). Not `Bool` (would clobber neighbouring bits) nor the wide / String / DateTime types | | **OpcUaClient** | Yes | All — forwarded upstream unchanged; the upstream server decides (shape included) | | **Galaxy** | Yes | OPC UA `Boolean`/`Int32`/`Int64`/`Float`/`Double`/`String`/`DateTime` (the `MxValueEncoder` array variants) — **only with a declared `arrayLength`**: an undeclared length materialises an unbounded node, and a short whole-array write would *resize* the Galaxy attribute (mxaccessgw contract), so such a node stays read-only | @@ -340,9 +340,10 @@ stays read-only. **Elements convert; they are not exact-matched.** ScadaBridge writes CLR `int[]` / `List` to Int16 nodes: the node manager converts a numeric array to the node's element type, and the AB CIP, AB Legacy and TwinCAT encoders convert each element again to the controller's type (an element that does not fit -is `BadOutOfRange`). Those rows therefore do not key on an exact node `DataType`; they leave out only -node types whose elements never convert — a `DateTime` or `String` node over a numeric element, and a -`Boolean` node on AB Legacy. An array tag deployed read-only before its driver gained an encoder turns +is `BadOutOfRange`), as does Modbus to the register / coil type. Those rows therefore do not key on an +exact node `DataType`; they leave out only node types whose elements the server cannot promise to convert — a `DateTime` (never converts) or +`String` node over a numeric element (converts only when the text happens to parse, `"1"` / `"true"`; +on Modbus, over a numeric or bit element; a Modbus `String` element takes any node type), and a `Boolean` node on AB Legacy. An array tag deployed read-only before its driver gained an encoder turns writable on the next deploy (a surgical `ArrayWritable` update, no rebuild). Each "Yes" row is pinned against the driver's real encoder by a parity test in that driver's diff --git a/docs/drivers/Modbus.md b/docs/drivers/Modbus.md index 024aaa78..03396c85 100644 --- a/docs/drivers/Modbus.md +++ b/docs/drivers/Modbus.md @@ -138,11 +138,17 @@ numeric, String, and BitInRegister array reads. **Live-verify** — Mac-verifiable against the simulator at `10.100.0.35:5020` (no fixture bring-up required; the sim is always-on on the shared Docker host). -**Array writes** (#610) — a whole-array write goes out as one FC15 (coils) or one FC16 (numeric -holding registers); the driver refuses rather than splits an FC16 longer than `maxRegistersPerWrite` -(default 123). The server mirrors that: an array tag is client-writable only when +**Array writes** (#610, #741) — a whole-array write goes out as one FC15 (coils) or one FC16 +(holding registers: numeric, `String` at `(stringLength+1)/2` registers per element, and +`BitInRegister` as one FC03 + one FC16 read-modify-write of the span); the driver refuses rather than +splits an FC16 longer than `maxRegistersPerWrite` (default 123; a larger value never lifts FC16's +protocol ceiling of 123, and a BitInRegister array's unchunked FC03 also bounds it by +`maxRegistersPerRead`). The server mirrors that: an array tag +is client-writable only when it is addressed by `region` + `address` keys and `arrayLength × registers-per-element` fits the cap (and a coil array ≤ 1968), else its node stays -read-only — see [Uns.md §Array writes](../Uns.md#array-writes-610), including the upgrade note. +read-only. A string-only (`addressString`) array stays read-only, because the composer cannot resolve +it under the driver's family — see [Uns.md §Array writes](../Uns.md#array-writes-610) and +[modbus-addressing.md §Array writes](../v2/modbus-addressing.md#array-writes-610-741). **Deferrals** — multi-dimensional arrays (`arrayLength` is always 1-D); per-element historization. diff --git a/docs/v2/modbus-addressing.md b/docs/v2/modbus-addressing.md index 74fbfe2c..abd367cf 100644 --- a/docs/v2/modbus-addressing.md +++ b/docs/v2/modbus-addressing.md @@ -89,6 +89,13 @@ For 8-byte values (Int64 / Float64) the same labels apply pairwise. `40001:F:5` = `Float32[5]` (consumes HR[0..9]). Array + bit suffix is rejected. Strings are not arrays. +An array written through the grammar string alone (a **string-only** tag — +`addressString` with no `region`/`address` keys) is served and written by the +driver, but its OPC UA node stays **read-only**: whether a write fits is +decided at compose time, where the driver's family is not known. Pin the keys +(the editor's "Resolve with driver family") to make the array writable — see +"Array writes" below. + ### Composition The 3-field shorthand `40001:F:5` is parsed as `(type=F, count=5)` because @@ -279,3 +286,37 @@ than asking users to rewrite spreadsheets. `BitInRegister`). - Exception 01/02/03 are device refusals: per-tag Bad, one Warning per tag, no driver Degraded; reads and writes share one comms-fault health slot. + +## Array writes (#610, #741) + +Whether an array node is client-writable is decided by +`DriverArrayWriteSupport` (Core.Abstractions) at compose time, and it says yes +only for what `ModbusDriver.WriteAsync` encodes end to end +(`ModbusArrayWriteSupportParityTests` pins the two against each other): + +- **Keys only.** The tag must carry `region` + `address` (read as strictly as + the driver reads them). A string-only tag stays read-only — the composer + cannot resolve the grammar under the driver's family. +- **Coils:** one FC15 of at most 1968 coils (the spec ceiling; the driver does + not chunk). Any `dataType` — a bit region is coerced to Bool. +- **HoldingRegisters:** one FC16 whose register count fits + `maxRegistersPerWrite` (default 123; the smallest value on the driver or the + tag's device wins), and never more than FC16's protocol ceiling of 123 — a + larger configured cap does not lift it, because a conformant device refuses + the longer FC16. Registers per element: 1 / 2 / 4 for the numeric types, + `(stringLength+1)/2` for `String` (each element nul-padded to that width), 1 + for `BitInRegister` (the span is read with one FC03, the addressed bit of each + register is set or cleared, and the span is written back with one FC16 — + the other 15 bits are preserved; `bitIndex` 0–15). Because that FC03 is one + unchunked read of the whole span, a BitInRegister array must also fit + `maxRegistersPerRead` (default and protocol ceiling 125). +- **Every span** — registers or coils — must end at or before address 65535 + (a conformant device answers exception 02 past it). FC23 (`useFC23ForMultiRegisterWrites`) is opt-in, + capped at 121 registers and falls back to FC16 above that, so it never + changes which arrays are writable. +- **Never writable:** DiscreteInputs / InputRegisters, Bool in a register + region, a `String` with no `stringLength`, a `bitIndex` above 15, a missing + or zero `arrayLength`, and a `String` / `DateTime` node over a numeric or bit + element (a DateTime never converts; a String converts only when its text + happens to parse, which the server cannot promise). Elements otherwise convert, so a + ScadaBridge `int[]` to an `Int16` node is writable. diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs index b19e20fb..64d5d665 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs @@ -47,7 +47,7 @@ public class HistoryReadCommand : CommandBase /// /// Gets the optional aggregate name used when the operator wants processed history instead of raw samples. /// - [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative")] + [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, TimeAverage, TimeAverage2, Total, Total2, Interpolative, Range, Delta, MinimumActualTime, MaximumActualTime, PercentGood, PercentBad")] public string? Aggregate { get; init; } /// @@ -199,24 +199,33 @@ public class HistoryReadCommand : CommandBase } } + private static readonly string SupportedAggregates = string.Join(", ", Enum.GetNames()); + private static AggregateType ParseAggregateType(string name) { - return name.Trim().ToLowerInvariant() switch + var trimmed = name.Trim(); + var alias = trimmed.ToLowerInvariant() switch { - "average" or "avg" => AggregateType.Average, - "minimum" or "min" => AggregateType.Minimum, - "maximum" or "max" => AggregateType.Maximum, - "count" => AggregateType.Count, - "start" or "first" => AggregateType.Start, - "end" or "last" => AggregateType.End, - "standarddeviation" or "stddev" or "stdev" => AggregateType.StandardDeviation, - "timeaverage" or "timeavg" => AggregateType.TimeAverage, - "timeaverage2" or "timeavg2" => AggregateType.TimeAverage2, - "total" => AggregateType.Total, - "total2" => AggregateType.Total2, - "interpolative" or "interp" => AggregateType.Interpolative, - _ => throw new ArgumentException( - $"Unknown aggregate: '{name}'. Supported: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative") + "avg" => AggregateType.Average, + "min" => AggregateType.Minimum, + "max" => AggregateType.Maximum, + "first" => AggregateType.Start, + "last" => AggregateType.End, + "timeavg" => AggregateType.TimeAverage, + "timeavg2" => AggregateType.TimeAverage2, + "interp" => AggregateType.Interpolative, + "mintime" => AggregateType.MinimumActualTime, + "maxtime" => AggregateType.MaximumActualTime, + _ => (AggregateType?)null }; + if (alias is { } a) return a; + + // Every enum name (the served set; a parity test pins it), case-insensitive. Numeric strings are refused: + // Enum.TryParse would accept "6" or "99" as an undefined value. + if (!int.TryParse(trimmed, out _) && + Enum.TryParse(trimmed, ignoreCase: true, out var parsed) && Enum.IsDefined(parsed)) + return parsed; + + throw new ArgumentException($"Unknown aggregate: '{name}'. Supported: {SupportedAggregates}"); } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index dd871594..b07efa97 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -115,22 +115,6 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter return (nextCp, nextRefs ?? []); } - /// - public async Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - var (_, _, references) = await _session.BrowseAsync( - null, - null, - nodeId, - 1u, - BrowseDirection.Forward, - ReferenceTypeIds.HierarchicalReferences, - true, - 0u); - - return references != null && references.Count > 0; - } - /// public async Task HistoryUpdateAsync(HistoryUpdateDetails details, CancellationToken ct) { @@ -164,46 +148,16 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; - var nodesToRead = new HistoryReadValueIdCollection - { - new HistoryReadValueId { NodeId = nodeId } - }; - - var allValues = new List(); - byte[]? continuationPoint = null; - - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - // Use the async overload so this method is genuinely asynchronous, - // honors the cancellation token, and does not block the caller's thread - // (which would block the UI dispatcher for client.ui consumers). - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Source, - continuationPoint != null, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - // A refused read (BadHistoryOperationUnsupported, BadUserAccessDenied, ...) must surface; only - // BadNoData is an empty result. Swallowing it printed "0 values returned" for a refusal. - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryData historyData) - allValues.AddRange(historyData.DataValues); - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && allValues.Count < maxValues); - - return allValues; + // Continuation pages are READ, never released, until the server finishes or the cap is reached; a read stopped + // by the cap releases the server's cursor (HistoryReadPaging). This path used to pass + // `releaseContinuationPoints: continuationPoint != null`, which RELEASED the second page instead of reading it, + // silently truncating every raw read to its first server page. + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Source, token), + result => result.HistoryData is ExtensionObject { Body: HistoryData historyData } + ? historyData.DataValues + : [], + maxValues, nodeId, ct).ConfigureAwait(false); } /// @@ -219,59 +173,41 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Both, token), + ToModifiedEntries, maxValues, nodeId, ct).ConfigureAwait(false); + } + + private static IEnumerable ToModifiedEntries(HistoryReadResult result) + { + if (result.HistoryData is not ExtensionObject { Body: HistoryModifiedData modified }) yield break; + for (var i = 0; i < modified.DataValues.Count; i++) + { + var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; + yield return new Models.HistoryModifiedEntry( + modified.DataValues[i], + info?.ModificationTime ?? DateTime.MinValue, + info?.UpdateType ?? default, + info?.UserName); + } + } + + /// Issues one single-node HistoryRead (or a release of its continuation point). + private async Task ReadHistoryPageAsync( + ReadRawModifiedDetails details, NodeId nodeId, byte[]? continuationPoint, bool release, + TimestampsToReturn timestamps, CancellationToken ct) + { var nodesToRead = new HistoryReadValueIdCollection { - new HistoryReadValueId { NodeId = nodeId } + new HistoryReadValueId { NodeId = nodeId, ContinuationPoint = continuationPoint } }; - var all = new List(); - byte[]? continuationPoint = null; + // The async overload honours the token and does not block the caller's thread (the UI dispatcher for + // client.ui consumers). + var response = await _session.HistoryReadAsync( + null, new ExtensionObject(details), timestamps, release, nodesToRead, ct).ConfigureAwait(false); - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Both, - false, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryModifiedData modified) - { - for (var i = 0; i < modified.DataValues.Count; i++) - { - var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; - all.Add(new Models.HistoryModifiedEntry( - modified.DataValues[i], - info?.ModificationTime ?? DateTime.MinValue, - info?.UpdateType ?? default, - info?.UserName)); - } - } - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && all.Count < maxValues); - - // A read stopped by the client's own cap leaves a server cursor behind; release it. - if (continuationPoint is { Length: > 0 }) - { - nodesToRead[0].ContinuationPoint = continuationPoint; - await _session.HistoryReadAsync(null, new ExtensionObject(details), TimestampsToReturn.Both, true, - nodesToRead, ct).ConfigureAwait(false); - } - - return all; + return response.Results is { Count: > 0 } results ? results[0] : null; } /// @@ -327,9 +263,22 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter }; _session.AddSubscription(subscription); - await subscription.CreateAsync(ct); + try + { + await subscription.CreateAsync(ct); + } + catch + { + // A subscription the server never created must not linger in Session.Subscriptions either. + try { await _session.RemoveSubscriptionAsync(subscription, CancellationToken.None); } + catch (Exception ex) { Logger.Warning(ex, "Error removing a subscription that failed to create"); } + throw; + } - return new DefaultSubscriptionAdapter(subscription); + // Deleting the adapter also removes the subscription from the SDK session, so a re-targeted alarm/data + // subscription does not accumulate dead Subscription objects in Session.Subscriptions. + return new DefaultSubscriptionAdapter(subscription, + async (s, token) => await _session.RemoveSubscriptionAsync(s, token).ConfigureAwait(false)); } /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs index deb15dd5..6473abf6 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs @@ -12,14 +12,23 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter private static readonly ILogger Logger = Log.ForContext(); private readonly Dictionary _monitoredItems = new(); private readonly Subscription _subscription; + private readonly Func? _detach; + private int _detached; /// /// Wraps a live OPC UA subscription so client code can manage monitored items through a testable abstraction. /// /// The underlying OPC UA subscription that owns monitored items for this client workflow. - public DefaultSubscriptionAdapter(Subscription subscription) + /// + /// Removes the subscription from its owning SDK Session once it is deleted. Without it a deleted + /// subscription stays in Session.Subscriptions until the session is disposed, so a UI that re-targets + /// alarms repeatedly accumulates dead subscriptions the SDK still iterates (transfer / recreate on reconnect). + /// + public DefaultSubscriptionAdapter(Subscription subscription, + Func? detach = null) { _subscription = subscription; + _detach = detach; } /// @@ -113,17 +122,39 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter Logger.Warning(ex, "Error deleting subscription"); } + // Detach whether or not the server-side delete succeeded: the adapter is finished with it either way. + await DetachAsync(ct); _monitoredItems.Clear(); } + private async Task DetachAsync(CancellationToken ct) + { + if (_detach == null || Interlocked.Exchange(ref _detached, 1) != 0) return; + try + { + await _detach(_subscription, ct); + } + catch (Exception ex) + { + Logger.Warning(ex, "Error removing subscription from its session"); + } + } + /// /// Releases the wrapped OPC UA subscription and clears tracked monitored items held by the adapter. /// public void Dispose() { + // One bounded bridge (5 s) for delete + detach, so a dispose never blocks longer than before (review m6). + // DetachAsync swallows its own failures; a failed delete must not skip the detach. try { - DisposeBridge.Run(ct => _subscription.DeleteAsync(true, ct)); + DisposeBridge.Run(async ct => + { + try { await _subscription.DeleteAsync(true, ct); } + catch (Exception ex) { Logger.Warning(ex, "Error deleting subscription"); } + await DetachAsync(ct); + }); } catch { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs index f3d81e64..8734c50b 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs @@ -90,14 +90,6 @@ internal interface ISessionAdapter : IDisposable Task<(byte[]? ContinuationPoint, ReferenceDescriptionCollection References)> BrowseNextAsync( byte[] continuationPoint, CancellationToken ct = default); - /// - /// Checks whether a node has any forward hierarchical child references. - /// - /// The node to inspect for child objects or variables. - /// The cancellation token that aborts the child lookup. - /// true when the node has at least one forward hierarchical child reference; otherwise false. - Task HasChildrenAsync(NodeId nodeId, CancellationToken ct = default); - /// /// Reads raw historical data. /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs index 44462199..23b70ede 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs @@ -23,12 +23,17 @@ public static class AggregateTypeMapper AggregateType.Count => ObjectIds.AggregateFunction_Count, AggregateType.Start => ObjectIds.AggregateFunction_Start, AggregateType.End => ObjectIds.AggregateFunction_End, - AggregateType.StandardDeviation => ObjectIds.AggregateFunction_StandardDeviationPopulation, AggregateType.TimeAverage => ObjectIds.AggregateFunction_TimeAverage, AggregateType.TimeAverage2 => ObjectIds.AggregateFunction_TimeAverage2, AggregateType.Total => ObjectIds.AggregateFunction_Total, AggregateType.Total2 => ObjectIds.AggregateFunction_Total2, AggregateType.Interpolative => ObjectIds.AggregateFunction_Interpolative, + AggregateType.Range => ObjectIds.AggregateFunction_Range, + AggregateType.Delta => ObjectIds.AggregateFunction_Delta, + AggregateType.MinimumActualTime => ObjectIds.AggregateFunction_MinimumActualTime, + AggregateType.MaximumActualTime => ObjectIds.AggregateFunction_MaximumActualTime, + AggregateType.PercentGood => ObjectIds.AggregateFunction_PercentGood, + AggregateType.PercentBad => ObjectIds.AggregateFunction_PercentBad, _ => throw new ArgumentOutOfRangeException(nameof(aggregate), aggregate, "Unknown AggregateType value.") }; } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs new file mode 100644 index 00000000..16bb72b6 --- /dev/null +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs @@ -0,0 +1,96 @@ +using Opc.Ua; +using Serilog; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +/// +/// The client-side HistoryRead paging loop shared by the raw and modified reads. Continuation pages are +/// READ (releaseContinuationPoints = false) while the caller still wants values; a read stopped by the +/// caller's own cap while the server still holds a cursor releases that cursor so it does not occupy one of the +/// session's continuation-point slots until eviction. +/// +internal static class HistoryReadPaging +{ + private static readonly ILogger Logger = Log.ForContext(typeof(HistoryReadPaging)); + + /// Reads (or, with , releases) one page. + /// The cursor to resume, or for the first page. + /// to release the cursor instead of reading. + /// The caller's cancellation token. + /// The per-node result, or when the response carried none. + public delegate Task PageReader( + byte[]? continuationPoint, bool releaseContinuationPoints, CancellationToken ct); + + /// The bound on a best-effort release that does not ride the caller's (possibly cancelled) token. + internal static readonly TimeSpan ReleaseTimeout = TimeSpan.FromSeconds(5); + + /// Reads pages until the server finishes or the caller's cap is reached. + /// The element type extracted from each page. + /// Issues one HistoryRead for the single node. + /// Extracts the page's elements from a result. + /// The caller's cap; must be positive. The result is trimmed to it. + /// The node being read, for refusal messages. + /// The caller's cancellation token. + /// At most elements. + /// is not positive: an uncapped client + /// read would materialise a tag's whole window (the client side of review 08 1.1). + /// A page was refused (see ). + /// was cancelled mid-read; the outstanding + /// cursor is released (best effort) first. + public static async Task> ReadAllAsync( + PageReader readPage, Func> extract, int maxValues, NodeId nodeId, + CancellationToken ct) + { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(maxValues); + + var all = new List(); + byte[]? continuationPoint = null; + + try + { + while (true) + { + var result = await readPage(continuationPoint, false, ct).ConfigureAwait(false); + if (result == null) return Trim(all, maxValues); + + HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); + all.AddRange(extract(result)); + + continuationPoint = result.ContinuationPoint; + if (continuationPoint is not { Length: > 0 }) return Trim(all, maxValues); + if (all.Count >= maxValues) break; + } + } + catch (OperationCanceledException) + { + // Cancelled between pages (or during page N+1) with a cursor outstanding: release it rather than leave it + // occupying one of the session's continuation-point slots, then surface the cancellation. + if (continuationPoint is { Length: > 0 }) await ReleaseAsync(readPage, continuationPoint, nodeId); + throw; + } + + // Stopped by the client's own cap: the server still holds a cursor for this session. Release it — on its own + // bounded token, so a cancellation racing the release cannot lose the values already in hand. + await ReleaseAsync(readPage, continuationPoint, nodeId); + return Trim(all, maxValues); + } + + private static List Trim(List all, int maxValues) + { + if (all.Count > maxValues) all.RemoveRange(maxValues, all.Count - maxValues); + return all; + } + + private static async Task ReleaseAsync(PageReader readPage, byte[] continuationPoint, NodeId nodeId) + { + try + { + using var cts = new CancellationTokenSource(ReleaseTimeout); + await readPage(continuationPoint, true, cts.Token).ConfigureAwait(false); + } + catch (Exception ex) + { + Logger.Warning(ex, "Releasing the HistoryRead continuation point for {NodeId} failed", nodeId); + } + } +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs index b7f48ab7..17dddc51 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs @@ -1,43 +1,64 @@ namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Models; /// -/// Aggregate functions for processed history reads. +/// Aggregate functions for processed history reads — exactly the seventeen the server's SQL historian backends +/// serve (AggregateFamilies.Served; a parity test pins it). The Gateway backend serves a subset and refuses +/// the rest with BadAggregateNotSupported. /// +/// +/// Values are explicit so the pre-existing members keep their numbers. 6 (the former StandardDeviation, +/// which no backend serves) is retired; do not reuse it. +/// public enum AggregateType { /// Average of values in the interval. - Average, + Average = 0, /// Minimum value in the interval. - Minimum, + Minimum = 1, /// Maximum value in the interval. - Maximum, + Maximum = 2, /// Count of values in the interval. - Count, + Count = 3, /// First value in the interval. - Start, + Start = 4, /// Last value in the interval. - End, - - /// Population standard deviation of values in the interval. - StandardDeviation, + End = 5, /// Time-weighted average, interpolated bounds (Part 13 TimeAverage). - TimeAverage, + TimeAverage = 7, /// Time-weighted average, stepped bounds (Part 13 TimeAverage2). - TimeAverage2, + TimeAverage2 = 8, /// Time-weighted integral over the interval (Part 13 Total). - Total, + Total = 9, /// Time-weighted integral, stepped bounds (Part 13 Total2). - Total2, + Total2 = 10, /// Interpolated value at the interval start (Part 13 Interpolative). - Interpolative -} \ No newline at end of file + Interpolative = 11, + + /// Maximum minus minimum in the interval (Part 13 Range). + Range = 12, + + /// Last minus first value in the interval (Part 13 Delta). + Delta = 13, + + /// Minimum value with its own timestamp (Part 13 MinimumActualTime). + MinimumActualTime = 14, + + /// Maximum value with its own timestamp (Part 13 MaximumActualTime). + MaximumActualTime = 15, + + /// Percentage of the interval with Good data (Part 13 PercentGood). + PercentGood = 16, + + /// Percentage of the interval with Bad data (Part 13 PercentBad). + PercentBad = 17 +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs index 4e3658f8..d0b06838 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs @@ -34,7 +34,9 @@ public sealed class BrowseResult public string NodeClass { get; } /// - /// Whether the node has child references. + /// Whether the node MAY have child references: for every Object, confirmed lazily when the + /// caller browses it (an expanded empty folder simply yields no children). Variables and Methods report + /// . /// public bool HasChildren { get; } } \ No newline at end of file diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index c1d87129..272e3695 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -15,16 +15,17 @@ public sealed class OpcUaClientService : IOpcUaClientService { private static readonly ILogger Logger = Log.ForContext(); - // Guards all access to the subscription-bookkeeping state below - // (_activeDataSubscriptions and _activeAlarmSubscription). The dictionary - // and tuple are mutated from the caller thread, the keep-alive failover - // path, and DisconnectAsync, so every read/write must be inside this lock. + // Guards all access to the subscription state below (_activeDataSubscriptions, _activeAlarmSubscription, + // _dataSubscription, _alarmSubscription). They are touched from the caller thread, the keep-alive failover path + // and DisconnectAsync, so every read/write is inside this lock — including failover's null-outs. private readonly object _subscriptionLock = new(); - // Serialises SubscribeAlarmsAsync / UnsubscribeAlarmsAsync so concurrent callers - // cannot both pass the _alarmSubscription == null check and create duplicate event - // subscriptions. Capacity 1 = at most one waiter; async-friendly (no thread-block). - private readonly SemaphoreSlim _alarmSubscribeSemaphore = new(1, 1); + // Serialises every subscription operation that awaits I/O (Subscribe/Unsubscribe, SubscribeAlarms/ + // UnsubscribeAlarms) AND the failover's teardown + reconnect + replay. A subscribe is therefore either + // wholly before a failover (recorded, then replayed onto the new session) or wholly after it (on the new + // session) — never half on the dying session. Async-friendly (no thread block). Deliberately never disposed: + // async-only waits hold no kernel handle, and a failover still running at Dispose releases it afterwards. + private readonly SemaphoreSlim _subscriptionGate = new(1, 1); // Track active data subscriptions for replay after failover private readonly Dictionary _activeDataSubscriptions = new(); @@ -34,6 +35,15 @@ public sealed class OpcUaClientService : IOpcUaClientService // run at a time. 0 = idle, 1 = failover in progress (Interlocked-managed). private int _failoverInProgress; + // The connection epoch (review I1): ConnectAsync and DisconnectAsync each start a new one (under + // _subscriptionLock). A failover runs under the epoch it started in and abandons itself — disposing any session it + // opened, making no state transition — the moment the epoch moves, so a user Disconnect + Connect during a + // failover is never undone by it. A keep-alive from a previous connection's session cannot start one either. + private long _connectionEpoch; + + /// The most recent keep-alive-triggered failover task (test seam: lets a test await it). + internal Task? LastFailover { get; private set; } + private readonly IApplicationConfigurationFactory _configFactory; private readonly IEndpointDiscovery _endpointDiscovery; @@ -99,6 +109,15 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); settings.Validate(); + // Overwriting a live (or reconnecting) session would leak it and orphan its subscriptions and keep-alive + // handler. The caller must disconnect first. + if (_state != ConnectionState.Disconnected || _session != null) + throw new InvalidOperationException( + $"Already {_state.ToString().ToLowerInvariant()}; call DisconnectAsync before connecting again."); + + long epoch; + lock (_subscriptionLock) epoch = ++_connectionEpoch; + _settings = settings; _allEndpointUrls = FailoverUrlParser.Parse(settings.EndpointUrl, settings.FailoverUrls); _currentEndpointIndex = 0; @@ -108,11 +127,11 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var session = await ConnectToEndpointAsync(settings, _allEndpointUrls[0], ct); - _session = session; + lock (_subscriptionLock) _session = session; session.RegisterKeepAliveHandler(isGood => { - if (!isGood) _ = HandleKeepAliveFailureAsync(); + if (!isGood) OnKeepAliveFailed(epoch); }); CurrentConnectionInfo = BuildConnectionInfo(session); @@ -133,6 +152,9 @@ public sealed class OpcUaClientService : IOpcUaClientService if (_state == ConnectionState.Disconnected) return; + // A new epoch first: a failover still connecting abandons itself instead of resurrecting the connection. + lock (_subscriptionLock) _connectionEpoch++; + var endpointUrl = _session?.EndpointUrl ?? _settings?.EndpointUrl ?? string.Empty; // Snapshot the subscription adapter references under the lock so a concurrent @@ -224,18 +246,15 @@ public sealed class OpcUaClientService : IOpcUaClientService while (references.Count > 0) { + // Child detection is lazy: an Object MAY have children and is confirmed when the caller expands or + // recurses into it. Probing each Object child with its own Browse made one level of N folders cost N + 1 + // round trips, and a recursive subscribe over a 50k-node tree ~50k extra Browse calls. foreach (var reference in references) - { - var childNodeId = ExpandedNodeId.ToNodeId(reference.NodeId, _session.NamespaceUris); - var hasChildren = reference.NodeClass == NodeClass.Object && - await _session.HasChildrenAsync(childNodeId, ct); - results.Add(new BrowseResult( reference.NodeId.ToString(), reference.DisplayName?.Text ?? string.Empty, reference.NodeClass.ToString(), - hasChildren)); - } + reference.NodeClass == NodeClass.Object)); if (continuationPoint is not { Length: > 0 } pending) break; (continuationPoint, references) = await _session.BrowseNextAsync(pending, ct); @@ -251,20 +270,39 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfNotConnected(); var nodeIdStr = nodeId.ToString(); - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) - return; // Already subscribed + // A failover may have run (and failed) while this call waited for the gate. + ThrowIfNotConnected(); + + ISubscriptionAdapter? subscription; + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) + return; // Already subscribed + subscription = _dataSubscription; + session = _session!; + } + + if (subscription == null) + { + subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _dataSubscription = subscription; + } + + var handle = await subscription.AddDataChangeMonitoredItemAsync( + nodeId, intervalMs, OnDataChangeNotification, ct); + + lock (_subscriptionLock) + { + _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + } } - - if (_dataSubscription == null) _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); - - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( - nodeId, intervalMs, OnDataChangeNotification, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + _subscriptionGate.Release(); } Logger.Debug("Subscribed to data changes on {NodeId}", nodeId); @@ -276,18 +314,35 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); var nodeIdStr = nodeId.ToString(); - (NodeId NodeId, int IntervalMs, uint Handle) sub; - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) - return; // Not subscribed, safe to ignore + (NodeId NodeId, int IntervalMs, uint Handle) sub; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) + return; // Not subscribed, safe to ignore + subscription = _dataSubscription; + } + + try + { + if (subscription != null) await subscription.RemoveMonitoredItemAsync(sub.Handle, ct); + } + finally + { + // The caller asked to stop: drop the bookkeeping even when the server-side remove failed (a session + // that died under us), or a later failover would replay the item onto the new session (review m5). + lock (_subscriptionLock) + { + _activeDataSubscriptions.Remove(nodeIdStr); + } + } } - - if (_dataSubscription != null) await _dataSubscription.RemoveMonitoredItemAsync(sub.Handle, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions.Remove(nodeIdStr); + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from data changes on {NodeId}", nodeId); @@ -303,17 +358,25 @@ public sealed class OpcUaClientService : IOpcUaClientService // Serialise check-and-create under the semaphore: concurrent // callers cannot both pass the null-check and create duplicate alarm subscriptions. // The semaphore is async-friendly; the subscription I/O runs inside the critical section. - await _alarmSubscribeSemaphore.WaitAsync(ct); + await _subscriptionGate.WaitAsync(ct); try { - if (_alarmSubscription != null) - return; // Already subscribed to alarms + ThrowIfNotConnected(); + + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_alarmSubscription != null) + return; // Already subscribed to alarms + session = _session!; + } var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); + var subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _alarmSubscription = subscription; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await subscription.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, ct); lock (_subscriptionLock) @@ -325,7 +388,7 @@ public sealed class OpcUaClientService : IOpcUaClientService } finally { - _alarmSubscribeSemaphore.Release(); + _subscriptionGate.Release(); } } @@ -334,15 +397,27 @@ public sealed class OpcUaClientService : IOpcUaClientService { ThrowIfDisposed(); - if (_alarmSubscription == null) - return; - - await _alarmSubscription.DeleteAsync(ct); - _alarmSubscription = null; - - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - _activeAlarmSubscription = null; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + subscription = _alarmSubscription; + if (subscription == null) return; + } + + await subscription.DeleteAsync(ct); + + lock (_subscriptionLock) + { + _alarmSubscription = null; + _activeAlarmSubscription = null; + } + } + finally + { + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from alarm events"); @@ -354,10 +429,12 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); ThrowIfNotConnected(); - if (_alarmSubscription == null) + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) subscription = _alarmSubscription; + if (subscription == null) throw new InvalidOperationException("No alarm subscription is active."); - await _alarmSubscription.ConditionRefreshAsync(ct); + await subscription.ConditionRefreshAsync(ct); Logger.Debug("Condition refresh requested"); } @@ -640,13 +717,16 @@ public sealed class OpcUaClientService : IOpcUaClientService /// public void Dispose() { - if (_disposed) return; - _disposed = true; + lock (_subscriptionLock) + { + if (_disposed) return; + _disposed = true; + _connectionEpoch++; // a failover still running abandons itself + } _dataSubscription?.Dispose(); _alarmSubscription?.Dispose(); _session?.Dispose(); - _alarmSubscribeSemaphore.Dispose(); lock (_subscriptionLock) { @@ -688,19 +768,27 @@ public sealed class OpcUaClientService : IOpcUaClientService ct); } - private async Task HandleKeepAliveFailureAsync() + /// A bad keep-alive from the session of connection . + private void OnKeepAliveFailed(long epoch) { - // Serialize failover: the OPC UA stack raises KeepAlive repeatedly - // while a session is down, so multiple bad keep-alives can fire before - // the first failover loop finishes. CompareExchange atomically claims - // the failover slot; a re-entrant call sees 1 and returns immediately, - // guaranteeing exactly one failover loop runs at a time. + // A late keep-alive from a previous connection's session is not about the current one. + if (!IsCurrentEpoch(epoch)) return; + + // Serialize failover: the OPC UA stack raises KeepAlive repeatedly while a session is down, so several bad + // keep-alives can fire before the first failover loop finishes. CompareExchange claims the failover slot; a + // re-entrant call sees 1 and returns, so exactly one loop runs — and only the winner sets the test seam + // (review m8: a re-entrant call used to overwrite it with an already-completed no-op). if (Interlocked.CompareExchange(ref _failoverInProgress, 1, 0) != 0) return; + LastFailover = HandleKeepAliveFailureAsync(epoch); + } + + private async Task HandleKeepAliveFailureAsync(long epoch) + { try { - await RunFailoverAsync(); + await RunFailoverAsync(epoch); } finally { @@ -708,74 +796,136 @@ public sealed class OpcUaClientService : IOpcUaClientService } } - private async Task RunFailoverAsync() + private bool IsCurrentEpoch(long epoch) + { + lock (_subscriptionLock) return _connectionEpoch == epoch && !_disposed; + } + + /// + /// A state transition made only while is still current (checked and applied under + /// , the event raised outside it). + /// + /// when the epoch has moved on (the caller must abandon). + private bool TransitionStateIfCurrent(long epoch, ConnectionState newState, string endpointUrl) + { + ConnectionState oldState; + lock (_subscriptionLock) + { + if (_connectionEpoch != epoch || _disposed) return false; + oldState = _state; + if (oldState == newState) return true; + _state = newState; + } + + ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); + return true; + } + + private async Task RunFailoverAsync(long epoch) { if (_state == ConnectionState.Reconnecting || _state == ConnectionState.Disconnected) return; - var oldEndpoint = _session?.EndpointUrl ?? string.Empty; - TransitionState(ConnectionState.Reconnecting, oldEndpoint); + var oldSession = _session; + var oldEndpoint = oldSession?.EndpointUrl ?? string.Empty; + if (!TransitionStateIfCurrent(epoch, ConnectionState.Reconnecting, oldEndpoint)) return; Logger.Warning("Session lost on {EndpointUrl}. Attempting failover...", oldEndpoint); - // Close old session - if (_session != null) + // Dispose the dead session BEFORE taking the gate: an in-flight subscribe holding the gate is blocked on + // that session, and disposing it is what makes the call fail fast and release the gate. + if (oldSession != null) { try { - _session.Dispose(); + oldSession.Dispose(); } catch { } - - _session = null; } - _dataSubscription = null; - _alarmSubscription = null; - - if (_settings == null || _allEndpointUrls == null) + // The rest runs under the subscription gate (and its null-outs under _subscriptionLock): no subscribe can + // interleave with the teardown, the reconnect or the replay. + await _subscriptionGate.WaitAsync(); + try { - TransitionState(ConnectionState.Disconnected, oldEndpoint); - return; - } - - // Try each endpoint - for (var attempt = 0; attempt < _allEndpointUrls.Length; attempt++) - { - _currentEndpointIndex = (_currentEndpointIndex + 1) % _allEndpointUrls.Length; - var url = _allEndpointUrls[_currentEndpointIndex]; - - try + // The user's settings / endpoint list as of THIS epoch: a Connect during the failover replaces the fields. + ConnectionSettings? settings; + string[]? endpoints; + lock (_subscriptionLock) { - Logger.Information("Failover attempt to {EndpointUrl}", url); - var session = await ConnectToEndpointAsync(_settings, url, CancellationToken.None); - _session = session; + if (_connectionEpoch != epoch || _disposed) return; // the user disconnected / reconnected meanwhile + if (ReferenceEquals(_session, oldSession)) _session = null; + _dataSubscription = null; + _alarmSubscription = null; + settings = _settings; + endpoints = _allEndpointUrls; + } - session.RegisterKeepAliveHandler(isGood => - { - if (!isGood) _ = HandleKeepAliveFailureAsync(); - }); - - CurrentConnectionInfo = BuildConnectionInfo(session); - TransitionState(ConnectionState.Connected, url); - Logger.Information("Failover succeeded to {EndpointUrl}", url); - - // Replay subscriptions - await ReplaySubscriptionsAsync(); + if (settings == null || endpoints == null) + { + TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint); return; } - catch (Exception ex) - { - Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); - } - } - Logger.Error("All failover endpoints unreachable"); - TransitionState(ConnectionState.Disconnected, oldEndpoint); + // Try each endpoint + for (var attempt = 0; attempt < endpoints.Length; attempt++) + { + if (!IsCurrentEpoch(epoch)) return; // stop trying once the user has moved on (m5) + _currentEndpointIndex = (_currentEndpointIndex + 1) % endpoints.Length; + var url = endpoints[_currentEndpointIndex]; + + try + { + Logger.Information("Failover attempt to {EndpointUrl}", url); + var session = await ConnectToEndpointAsync(settings, url, CancellationToken.None); + + // A Disconnect / Connect / Dispose that ran while this attempt was connecting wins: never + // resurrect the old connection or overwrite the user's new session. Checked and installed + // atomically against the epoch. + bool installed; + lock (_subscriptionLock) + { + installed = _connectionEpoch == epoch && !_disposed; + if (installed) _session = session; + } + + if (!installed) + { + session.Dispose(); + return; + } + + session.RegisterKeepAliveHandler(isGood => + { + if (!isGood) OnKeepAliveFailed(epoch); + }); + + CurrentConnectionInfo = BuildConnectionInfo(session); + if (!TransitionStateIfCurrent(epoch, ConnectionState.Connected, url)) return; + Logger.Information("Failover succeeded to {EndpointUrl}", url); + + // Replay subscriptions + await ReplaySubscriptionsAsync(session); + return; + } + catch (Exception ex) + { + Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); + } + } + + if (TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint)) + Logger.Error("All failover endpoints unreachable"); + } + finally + { + _subscriptionGate.Release(); + } } - private async Task ReplaySubscriptionsAsync() + /// Re-creates the recorded subscriptions on . Runs under the subscription gate. + private async Task ReplaySubscriptionsAsync(ISessionAdapter session) { // Snapshot the bookkeeping state under the lock, then clear it so the // replayed handles can be recorded fresh as each monitored item is @@ -791,13 +941,17 @@ public sealed class OpcUaClientService : IOpcUaClientService } // Replay data subscriptions + ISubscriptionAdapter? dataSubscription = null; foreach (var (nodeIdStr, (nodeId, intervalMs, _)) in subscriptions) try { - if (_dataSubscription == null) - _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + if (dataSubscription == null) + { + dataSubscription = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _dataSubscription = dataSubscription; + } - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( + var handle = await dataSubscription.AddDataChangeMonitoredItemAsync( nodeId, intervalMs, OnDataChangeNotification, CancellationToken.None); lock (_subscriptionLock) @@ -817,9 +971,10 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + var replayedAlarms = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _alarmSubscription = replayedAlarms; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await replayedAlarms.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, CancellationToken.None); lock (_subscriptionLock) @@ -937,9 +1092,13 @@ public sealed class OpcUaClientService : IOpcUaClientService private void TransitionState(ConnectionState newState, string endpointUrl) { - var oldState = _state; - if (oldState == newState) return; - _state = newState; + ConnectionState oldState; + lock (_subscriptionLock) + { + oldState = _state; + if (oldState == newState) return; + _state = newState; + } ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs index aab55811..b883be03 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs @@ -43,23 +43,9 @@ public partial class HistoryViewModel : ObservableObject _dispatcher = dispatcher; } - /// Available aggregate types (null means "Raw"). + /// Available aggregate types (null means "Raw"): every client aggregate, which is the served set. public IReadOnlyList AggregateTypes { get; } = - [ - null, - AggregateType.Average, - AggregateType.Minimum, - AggregateType.Maximum, - AggregateType.Count, - AggregateType.Start, - AggregateType.End, - AggregateType.StandardDeviation, - AggregateType.TimeAverage, - AggregateType.TimeAverage2, - AggregateType.Total, - AggregateType.Total2, - AggregateType.Interpolative - ]; + [null, .. Enum.GetValues().Select(a => (AggregateType?)a)]; /// Gets a value indicating whether an aggregate read is selected. public bool IsAggregateRead => SelectedAggregateType != null; diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs index 381719a4..18d2bee4 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs @@ -1,4 +1,5 @@ using System.Collections.ObjectModel; +using System.Collections.Specialized; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using Opc.Ua; @@ -19,6 +20,11 @@ public partial class SubscriptionsViewModel : ObservableObject private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientService _service; + // NodeId -> its rows, kept in step with ActiveSubscriptions by OnActiveSubscriptionsChanged, so a notification is + // one lookup instead of a scan of every row on the UI thread (2 000 items at 1 Hz was 4M string compares/s). + // Touched only on the UI thread, like the collection itself. + private readonly Dictionary> _rowsByNodeId = new(StringComparer.Ordinal); + /// /// Last user-visible status message — set when a subscribe/unsubscribe operation fails so the /// shell can surface the diagnostic instead of silently dropping the error. Cleared on success. @@ -49,9 +55,13 @@ public partial class SubscriptionsViewModel : ObservableObject { _service = service; _dispatcher = dispatcher; + ActiveSubscriptions.CollectionChanged += OnActiveSubscriptionsChanged; _service.DataChanged += OnDataChanged; } + /// The number of distinct NodeIds in the notification index (test seam). + internal int IndexedNodeCount => _rowsByNodeId.Count; + /// Currently active subscriptions. public ObservableCollection ActiveSubscriptions { get; } = []; @@ -62,16 +72,42 @@ public partial class SubscriptionsViewModel : ObservableObject { _dispatcher.Post(() => { - foreach (var item in ActiveSubscriptions) - if (item.NodeId == e.NodeId) - { - item.Value = Helpers.ValueFormatter.Format(e.Value.Value); - item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); - item.Timestamp = e.Value.SourceTimestamp.ToString("O"); - } + if (!_rowsByNodeId.TryGetValue(e.NodeId, out var rows)) return; + foreach (var item in rows) + { + item.Value = Helpers.ValueFormatter.Format(e.Value.Value); + item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); + item.Timestamp = e.Value.SourceTimestamp.ToString("O"); + } }); } + private void OnActiveSubscriptionsChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (e.Action == NotifyCollectionChangedAction.Reset) + { + _rowsByNodeId.Clear(); + foreach (var item in ActiveSubscriptions) Index(item); + return; + } + + if (e.OldItems != null) + foreach (SubscriptionItemViewModel item in e.OldItems) + if (_rowsByNodeId.TryGetValue(item.NodeId, out var rows) && rows.Remove(item) && rows.Count == 0) + _rowsByNodeId.Remove(item.NodeId); + + if (e.NewItems != null) + foreach (SubscriptionItemViewModel item in e.NewItems) + Index(item); + } + + private void Index(SubscriptionItemViewModel item) + { + if (!_rowsByNodeId.TryGetValue(item.NodeId, out var rows)) + _rowsByNodeId[item.NodeId] = rows = []; + rows.Add(item); + } + private bool CanAddSubscription() { return IsConnected && !string.IsNullOrWhiteSpace(NewNodeIdText); @@ -149,7 +185,7 @@ public partial class SubscriptionsViewModel : ObservableObject if (!IsConnected || string.IsNullOrWhiteSpace(nodeIdStr)) return; // Skip if already subscribed - if (ActiveSubscriptions.Any(s => s.NodeId == nodeIdStr)) return; + if (_rowsByNodeId.ContainsKey(nodeIdStr)) return; try { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverArrayWriteSupport.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverArrayWriteSupport.cs index 08b87af5..eca09158 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverArrayWriteSupport.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverArrayWriteSupport.cs @@ -20,11 +20,24 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; /// until this table follows. /// /// -/// Modbus — region: Coils (FC15, any element, at most 1968 coils) and -/// region: HoldingRegisters with a fixed-width numeric dataType (FC16) whose -/// arrayLength × registers-per-element fits the driver's maxRegistersPerWrite (default -/// 123 — the driver refuses rather than chunks a longer FC16). Bit-in-register, String and -/// Bool-in-register arrays have no array encoder; input regions are read-only. +/// Modbus — tags addressed by the region + address KEYS (the +/// ModbusTagAddressResolution rule: keys win whenever both are present), read as strictly as the +/// driver reads them (address an integer 0–65535; a present region / dataType / +/// byteOrder / stringByteOrder string must name a member). region: Coils: FC15, at most +/// 1968 coils, with ANY dataType — the resolver coerces a bit region to Bool and never refuses it. +/// region: HoldingRegisters (FC16), never Bool: a fixed-width numeric dataType, +/// String ((stringLength+1)/2 registers per element, stringLength >= 1) or +/// BitInRegister (one register per element; bitIndex 0–15), whose +/// arrayLength × registers-per-element fits the driver's maxRegistersPerWrite (default 123 — +/// the driver refuses rather than chunks a longer FC16) and never more than FC16's protocol ceiling of 123, +/// however high the cap is configured (a conformant device refuses a longer FC16, so that write always +/// fails). A BitInRegister array is read-modify-written — one unchunked FC03 of the span, then one FC16 — +/// so it is also bounded by maxRegistersPerRead (default and protocol ceiling 125). Every span +/// (registers or coils) ends at or before address 65535. FC23 is opt-in, capped lower and falls back to +/// FC16, so it never changes the answer. Input +/// regions are read-only. A string-only tag (an addressString with no keys) is resolved by the +/// driver with ITS family through a parser this assembly cannot reference, so it cannot be proven +/// writable here and stays read-only. /// S7 — dataType Byte / Int16 / UInt16 / Int32 / UInt32 / Float32 (one contiguous /// block, EncodeArrayBlock). Bool arrays would clobber neighbouring bits; the wide / String / /// DateTime element types have no array encoder. @@ -61,9 +74,12 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; /// Elements convert; they are not exact-matched. ScadaBridge writes CLR arrays such as /// int[] / List<int> to Int16 nodes; the node manager converts a numeric array to the /// node's element type, and the AbCip, AbLegacy and TwinCAT encoders then Convert each element to the -/// controller's type (an element that does not fit is BadOutOfRange). So those rows do not key on an -/// exact node DataType; they leave out only the node types whose elements NEVER convert: a -/// DateTime or String node over a numeric element (AbCip, AbLegacy, TwinCAT — TwinCAT keeps +/// controller's type (an element that does not fit is BadOutOfRange); Modbus Converts each element to +/// the register / coil type the same way. So those rows do not key on an exact node DataType; they leave +/// out only the node types whose elements the server cannot promise to convert: a DateTime node over a +/// numeric element (it never converts) or a String node over one (it converts only when the text happens +/// to parse, "1" / "true") — on Modbus over a numeric or bit element, and a Modbus String element +/// takes any node type; likewise on AbCip, AbLegacy and TwinCAT — TwinCAT keeps /// DATE / DT and WSTRING under a DateTime node, and STRING / WSTRING under a String node; a DateTime's text /// form is culture-dependent, so a STRING under a DateTime node is left out), and a Boolean node on /// AbLegacy, whose encoder refuses bool elements. An unknown DataType name (served as BaseDataType) hands the @@ -74,13 +90,26 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; public static class DriverArrayWriteSupport { /// The Modbus driver's FC16 write cap when maxRegistersPerWrite is absent or 0 - /// (ModbusDriver resolves it the same way). + /// (ModbusDriver resolves it the same way). A configured value only ever LOWERS it — see + /// . public const int ModbusDefaultMaxRegistersPerWrite = 123; /// Modbus FC15's protocol ceiling on coils per request (0x07B0). The driver sends one FC15 for the /// whole array without chunking, so a longer coil array is refused by every conformant device. public const int ModbusMaxCoilsPerWrite = 1968; + /// Modbus FC16's protocol ceiling on registers per request (0x7B). A configured + /// maxRegistersPerWrite above it never lifts it: a conformant device refuses a longer FC16. + public const int ModbusProtocolMaxRegistersPerWrite = 123; + + /// Modbus FC03's protocol ceiling on registers per request (0x7D) — the bound on a BitInRegister[] + /// write's read-modify-write read, together with a configured maxRegistersPerRead. + public const int ModbusProtocolMaxRegistersPerRead = 125; + + /// The last Modbus address (registers and coils alike): a span ending past it is refused by every + /// conformant device (exception 02). + private const long ModbusLastAddress = ushort.MaxValue; + // Element type → registers per element (ModbusDriver.RegisterCount). private static readonly Dictionary ModbusRegisterArrayTypes = new(StringComparer.OrdinalIgnoreCase) { @@ -89,6 +118,30 @@ public static class DriverArrayWriteSupport ["Int64"] = 4, ["UInt64"] = 4, ["Float64"] = 4, }; + // The Modbus enums' member names (ModbusRegion / ModbusDataType / ModbusByteOrder / ModbusStringByteOrder in + // Driver.Modbus.Addressing, which this assembly cannot reference). The parity tests enumerate the real enums, so a + // member added there without being added here fails a test. + private static readonly HashSet ModbusRegions = new(StringComparer.OrdinalIgnoreCase) + { + "Coils", "DiscreteInputs", "InputRegisters", "HoldingRegisters", + }; + + private static readonly HashSet ModbusDataTypes = new(StringComparer.OrdinalIgnoreCase) + { + "Bool", "Int16", "UInt16", "Int32", "UInt32", "Int64", "UInt64", "Float32", "Float64", + "BitInRegister", "String", "Bcd16", "Bcd32", + }; + + private static readonly HashSet ModbusByteOrders = new(StringComparer.OrdinalIgnoreCase) + { + "BigEndian", "WordSwap", "ByteSwap", "FullReverse", + }; + + private static readonly HashSet ModbusStringByteOrders = new(StringComparer.OrdinalIgnoreCase) + { + "HighByteFirst", "LowByteFirst", + }; + private static readonly HashSet S7ArrayTypes = new(StringComparer.OrdinalIgnoreCase) { "Byte", "Int16", "UInt16", "Int32", "UInt32", "Float32", @@ -148,22 +201,7 @@ public static class DriverArrayWriteSupport return dataType is not null && GalaxyArrayDataTypes.Contains(dataType) && TryParseObject(tagConfig, out var galaxyRoot) && ReadArrayLength(galaxyRoot) is not null; case DriverTypeNames.Modbus: - { - // Same reads as ModbusTagDefinitionFactory: a blob with no `region` or no numeric `address` is not - // a Modbus tag at all (the factory refuses it). A present non-string region is read as absent ⇒ - // HoldingRegisters, and an absent dataType ⇒ Int16 (TagConfigJson.TryReadEnumStrict). - if (!TryParseObject(tagConfig, out var root) - || !root.TryGetProperty("region", out _) - || !root.TryGetProperty("address", out var address) || address.ValueKind != JsonValueKind.Number - || ReadArrayLength(root) is not { } count) - return false; - var region = ReadEnumName(root, "region") ?? "HoldingRegisters"; - var native = ReadEnumName(root, "dataType") ?? "Int16"; - if (string.Equals(region, "Coils", StringComparison.OrdinalIgnoreCase)) return count <= ModbusMaxCoilsPerWrite; - return string.Equals(region, "HoldingRegisters", StringComparison.OrdinalIgnoreCase) - && ModbusRegisterArrayTypes.TryGetValue(native, out var registersPerElement) - && (long)count * registersPerElement <= ModbusMaxRegistersPerWrite(driverConfigs); - } + return ModbusSupports(dataType, tagConfig, driverConfigs); case DriverTypeNames.S7: { // S7TagDefinitionFactory requires a dataType key (absent ⇒ not an S7 tag at all); a present @@ -185,6 +223,81 @@ public static class DriverArrayWriteSupport } } + // ModbusTagAddressResolution + ModbusTagDefinitionFactory + ModbusDriver.WriteAsync, KEYS shape only: + // - `region` AND `address` both present (Classify == Keys). Anything else is a string-only (`addressString`) blob the + // driver resolves with ITS family through ModbusAddressParser, which this assembly cannot reference — so it is + // never proven writable here and stays read-only (conservative: a hidden write, never an advertised refusal). + // - the strict key reads: `address` an integer 0–65535; a present string `region` / `dataType` / `byteOrder` / + // `stringByteOrder` must be a member name (a non-string token is read as absent ⇒ the default). + // - Coils: the resolver COERCES any type to Bool (never refuses); one FC15 of at most 1968 coils. + // - HoldingRegisters: never Bool; fixed-width numeric, String ((stringLength+1)/2 registers, stringLength >= 1) or + // BitInRegister (one register each, bitIndex 0–15); arrayLength × registers per element within + // maxRegistersPerWrite clamped to FC16's 123 (the driver refuses rather than chunks; FC23 is opt-in and capped + // lower, falling back to FC16, so it never changes the answer). BitInRegister[] also READS its span first in one + // unchunked FC03, so it is bounded by maxRegistersPerRead (clamped to FC03's 125) too. + // - Every span ends at or before address 65535 (a conformant device refuses one past it with exception 02). + // - Elements CONVERT, so the node type is not exact-matched. A DateTime node over a numeric or bit element never + // converts; a String node over one converts only when the text happens to parse ("1", "true"), which the server + // cannot promise — both are left out. A String element Convert.ToString()s any node type. + private static bool ModbusSupports(string? dataType, string? tagConfig, string?[]? driverConfigs) + { + if (!TryParseObject(tagConfig, out var root) + || !root.TryGetProperty("region", out _) + || !root.TryGetProperty("address", out var addressEl) || addressEl.ValueKind != JsonValueKind.Number + || !addressEl.TryGetInt32(out var address) || address is < 0 or > ushort.MaxValue + || ReadArrayLength(root) is not { } count + || !TryReadModbusEnum(root, "region", ModbusRegions, out var region) + || !TryReadModbusEnum(root, "dataType", ModbusDataTypes, out var native) + || !TryReadModbusEnum(root, "byteOrder", ModbusByteOrders, out _) + || !TryReadModbusEnum(root, "stringByteOrder", ModbusStringByteOrders, out _)) + return false; + region ??= "HoldingRegisters"; + + if (region.Equals("Coils", StringComparison.OrdinalIgnoreCase)) + return count <= ModbusMaxCoilsPerWrite && address + (long)count - 1 <= ModbusLastAddress + && dataType is not ("String" or "DateTime"); + if (!region.Equals("HoldingRegisters", StringComparison.OrdinalIgnoreCase)) return false; // input regions are read-only + + native ??= "Int16"; + int registersPerElement; + if (native.Equals("String", StringComparison.OrdinalIgnoreCase)) + { + // The factory narrows to ushort ((ushort)ReadInt) and refuses a zero length. + var stringLength = (ushort)ReadInt(root, "stringLength"); + if (stringLength < 1) return false; + registersPerElement = (stringLength + 1) / 2; + } + else + { + if (dataType is "String" or "DateTime") return false; + if (native.Equals("BitInRegister", StringComparison.OrdinalIgnoreCase)) + { + // The factory narrows to byte ((byte)ReadInt) and refuses a bit above 15. The RMW reads the whole span + // in ONE unchunked FC03 before writing it, so the read cap bounds it too. + if ((byte)ReadInt(root, "bitIndex") > 15 || count > ModbusMaxRegistersPerRead(driverConfigs)) return false; + registersPerElement = 1; + } + else if (!ModbusRegisterArrayTypes.TryGetValue(native, out registersPerElement)) + { + return false; // Bool in a register region: the factory refuses the pair + } + } + var registers = (long)count * registersPerElement; + return registers <= ModbusMaxRegistersPerWrite(driverConfigs) && address + registers - 1 <= ModbusLastAddress; + } + + // TagConfigJson.TryReadEnumStrict over the Modbus enum's member names: absent or a non-string token ⇒ null (the + // caller applies the default); a string must name a member (trimmed, case-insensitive), else the tag is refused. + // Numeric spellings are not member names, so they are refused exactly as ClassifyEnumText refuses them. + private static bool TryReadModbusEnum(JsonElement root, string name, HashSet members, out string? value) + { + value = ReadEnumName(root, name)?.Trim(); + return value is null || members.Contains(value); + } + + private static int ReadInt(JsonElement root, string name) => + root.TryGetProperty(name, out var el) && el.ValueKind == JsonValueKind.Number && el.TryGetInt32(out var v) ? v : 0; + // AbCipTagDefinitionFactory + AbCipDriver.WriteAsync: a non-blank string tagPath, an atomic element type the codec // has a width for (BOOL arrays are bit-packed DWORDs, STRING / structure elements are structures), a declared // length, and not a safety tag (refused BadNotWritable). Each element is CONVERTED, so any numeric / Boolean node @@ -338,9 +451,24 @@ public static class DriverArrayWriteSupport public static bool RefusesArrayValueOnScalarTag(string? driverType) => driverType is DriverTypeNames.Modbus or DriverTypeNames.S7; - // The smallest Modbus write cap configured in any config; none configured (absent / non-positive) ⇒ the driver - // default (123). The key is matched case-insensitively, like the driver's DTO binding. - private static int ModbusMaxRegistersPerWrite(string?[]? driverConfigs) + // The effective Modbus FC16 write cap: the smallest maxRegistersPerWrite configured in any config (absent / + // non-positive ⇒ the driver default, 123), CLAMPED to FC16's protocol ceiling of 123. The driver trusts a larger + // configured cap and sends the over-ceiling FC16 (past 127 registers its one-byte byte-count field even wraps), which + // every conformant device refuses — so such a write always fails and is never advertised. + private static int ModbusMaxRegistersPerWrite(string?[]? driverConfigs) => + Math.Min(SmallestPositive(driverConfigs, "maxRegistersPerWrite") ?? ModbusDefaultMaxRegistersPerWrite, + ModbusProtocolMaxRegistersPerWrite); + + // The effective Modbus FC03 read cap: the smallest maxRegistersPerRead configured (absent / non-positive ⇒ the driver + // default, 125), clamped to FC03's protocol ceiling of 125. Only the BitInRegister[] write reads (its RMW issues one + // unchunked FC03 of the whole span), so only that shape is bounded by it. + private static int ModbusMaxRegistersPerRead(string?[]? driverConfigs) => + Math.Min(SmallestPositive(driverConfigs, "maxRegistersPerRead") ?? ModbusProtocolMaxRegistersPerRead, + ModbusProtocolMaxRegistersPerRead); + + // The smallest positive Int32 value of `key` (matched case-insensitively, like the driver's DTO binding) at the root + // of any config; null when none carries one. + private static int? SmallestPositive(string?[]? driverConfigs, string key) { int? cap = null; foreach (var config in driverConfigs ?? []) @@ -348,7 +476,7 @@ public static class DriverArrayWriteSupport if (!TryParseObject(config, out var root)) continue; foreach (var prop in root.EnumerateObject()) { - if (string.Equals(prop.Name, "maxRegistersPerWrite", StringComparison.OrdinalIgnoreCase) + if (string.Equals(prop.Name, key, StringComparison.OrdinalIgnoreCase) && prop.Value.ValueKind == JsonValueKind.Number && prop.Value.TryGetInt32(out var configured) && configured > 0) { @@ -356,7 +484,7 @@ public static class DriverArrayWriteSupport } } } - return cap ?? ModbusDefaultMaxRegistersPerWrite; + return cap; } // The declared element count: isArray is the JSON literal true AND arrayLength is an Int32 >= 1 — the rule the diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index 804ff1bc..c914d160 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -44,17 +44,21 @@ public sealed class ProbeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync(["__probe"], ct); - var health = driver.GetHealth(); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync(["__probe"], ct); + var health = driver.GetHealth(); - await console.Output.WriteLineAsync($"Gateway: {Gateway}"); - await console.Output.WriteLineAsync($"Family: {Family}"); - await console.Output.WriteLineAsync($"Health: {health.State}"); - if (health.LastError is { } err) - await console.Output.WriteLineAsync($"Last error: {err}"); - await console.Output.WriteLineAsync(); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + await console.Output.WriteLineAsync($"Gateway: {Gateway}"); + await console.Output.WriteLineAsync($"Family: {Family}"); + await console.Output.WriteLineAsync($"Health: {health.State}"); + if (health.LastError is { } err) + await console.Output.WriteLineAsync($"Last error: {err}"); + await console.Output.WriteLineAsync(); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs index 0d0bbb80..0e4b5ad4 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs @@ -45,9 +45,13 @@ public sealed class ReadCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs index b266fa3d..399b3a0e 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs @@ -50,23 +50,22 @@ public sealed class SubscribeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); ISubscriptionHandle? handle = null; + // The poll engine raises OnDataChange on a background thread: serialise its lines against each other and + // against this thread's own, and never let a failed console write fault the poll loop (review 07 U7). + var writeLock = new object(); try { await driver.InitializeAsync("{}", ct); - await console.Output.WriteLineAsync( - $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); - - driver.OnDataChange += (_, e) => - { - var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + - $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + - $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); - }; + driver.OnDataChange += CreateChangeLineWriter(console.Output, writeLock); handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); + lock (writeLock) + { + console.Output.WriteLine($"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); + } + try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -76,6 +75,10 @@ public sealed class SubscribeCommand : AbCipCommandBase // Expected on Ctrl+C. } } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Ctrl+C while still connecting/subscribing: a clean exit, not a stack trace (review 07 U7). + } finally { if (handle is not null) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs index e4dadd38..b4473e02 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs @@ -57,9 +57,13 @@ public sealed class WriteCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs index 157caf2d..e96b1a75 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs @@ -32,27 +32,6 @@ public abstract class AbLegacyCommandBase : DriverCommandBase "LogixPccc unverified).")] public AbLegacyCliTransport Transport { get; init; } = AbLegacyCliTransport.Default; - /// - /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints - /// Cancelled. instead of an unhandled stack trace (review 07 U7). - /// Any other exception — and an OCE the operator did not ask for — still propagates. - /// - /// The CLI console. - /// The console's Ctrl-C token. - /// The command body. - /// A task that completes when the body has finished or was cancelled. - internal static async ValueTask RunCancellableAsync(CliFx.Infrastructure.IConsole console, CancellationToken ct, Func body) - { - try - { - await body(); - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) - { - await console.Error.WriteLineAsync("Cancelled."); - } - } - /// Gets the per-operation timeout in milliseconds. [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")] public int TimeoutMs { get; init; } = 5000; diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs index d77bff85..d5a087fd 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs @@ -79,5 +79,61 @@ public abstract class DriverCommandBase : ICommand /// protected static void FlushLogging() => Log.CloseAndFlush(); + /// + /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints + /// Cancelled. instead of an unhandled stack trace (review 07 U7). + /// Any other exception — and an OCE the operator did not ask for — still propagates. + /// + /// The CLI console. + /// The console's Ctrl-C token. + /// The command body. + /// A task that completes when the body has finished or was cancelled. + public static async ValueTask RunCancellableAsync(IConsole console, CancellationToken ct, Func body) + { + try + { + await body(); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + await console.Error.WriteLineAsync("Cancelled."); + } + } + + /// + /// Builds the OnDataChange handler a subscribe command attaches: one + /// [HH:mm:ss.fff] ref = value (status) line per change. Writes are serialised on + /// (the poll engine raises from a background thread, concurrently with the + /// command thread's own lines), and a failing console write (closed pipe, redirected stream error) is logged + /// and swallowed so it cannot fault the driver's poll loop (review 07 U7). + /// + /// The console output writer. + /// The lock every console write of the command takes. + /// The event handler. + public static EventHandler CreateChangeLineWriter( + TextWriter output, object writeLock) + { + ArgumentNullException.ThrowIfNull(output); + ArgumentNullException.ThrowIfNull(writeLock); + return (_, e) => + { + try + { + var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; + lock (writeLock) + { + output.WriteLine(line); + } + } + catch (Exception ex) + { + Log.Logger.Warning(ex, "subscribe: console write failed for {Tag}; continuing the poll loop.", + e.FullReference); + } + }; + } + private bool _loggingConfigured; } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index 906fee8e..9a49acdb 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -101,7 +101,11 @@ public static class SnapshotFormatter return sb.ToString().TrimEnd(); } - /// Formats a value for console output, handling null, bool, string, and formattable types. + /// + /// Formats a value for console output, handling null, bool, string, formattable and array/list types. An + /// array renders its elements ([1, 2, 3], each formatted as a scalar) rather than its CLR type name + /// (System.Int32[]). + /// /// The value to format. /// A formatted string representation of the value. public static string FormatValue(object? value) => value switch @@ -110,9 +114,23 @@ public static class SnapshotFormatter bool b => b ? "true" : "false", string s => $"\"{s}\"", IFormattable f => f.ToString(null, CultureInfo.InvariantCulture), + System.Collections.IEnumerable items => FormatSequence(items), _ => value.ToString() ?? "", }; + private static string FormatSequence(System.Collections.IEnumerable items) + { + var sb = new System.Text.StringBuilder("["); + var first = true; + foreach (var item in items) + { + if (!first) sb.Append(", "); + sb.Append(FormatValue(item)); + first = false; + } + return sb.Append(']').ToString(); + } + /// Formats an OPC UA status code as a hexadecimal string with named severity classification. /// The OPC UA status code to format. /// A formatted status string like "0x00000000 (Good)" or "0x80050000 (BadCommunicationError)". diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs new file mode 100644 index 00000000..e68d4daf --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs @@ -0,0 +1,12 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +/// +/// An whose status came OFF THE WIRE: the device (or the bridge's Execute PCCC +/// object) replied with a non-zero CIP envelope status or PCCC STS. It lets a driver tell a refusal the device +/// ANSWERED from the same raised locally before anything was sent — most importantly +/// , which a native runtime also throws for operations it refuses pre-wire +/// (review lane K I2). +/// +/// The status mapped from the reply. +/// The exception message. +public sealed class AbDeviceReplyException(AbStatus status, string message) : AbTransportException(status, message); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs index b9d8d674..39c5b41d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs @@ -1,39 +1,81 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +/// +/// Maps wire statuses (CIP general/extended, PCCC STS/EXT STS) to the transport-neutral the +/// drivers classify. The class an lands in is load-bearing for driver health (#527/#737/#738): +/// an ANSWERED refusal (NotFound / NotAllowed / OutOfBounds / TooLarge / TooSmall / TypeMismatch / +/// DeviceStateConflict, and for AbCip Unsupported) proves the device replied; +/// is the no-reply comms class; / are a reply that +/// cannot be classified. Every code not named below is deliberately . +/// public static class AbStatusCodes { /// Maps a CIP general status (and optional extended status words) to an . /// The CIP general status byte. /// The extended status words, if any. /// The mapped status. + /// + /// 0x01 (connection failure, incl. a routing hop's unconnected-send timeout) and 0x07 (connection lost) stay + /// on purpose: a bridge reports a dead END device that way, so they are neither an + /// answer nor proof of no reply (AbLegacy #738 "opaque"). A Forward Open refused with one of the request-level + /// codes (0x1C / 0x20 / 0xFF-0x2104) stays a connection failure — see + /// ConnectionManagerServices.ParseForwardOpenReply. + /// public static AbStatus FromCip(byte general, ReadOnlySpan extended) => general switch { - 0x00 or 0x0B => AbStatus.Ok, - 0x04 or 0x05 or 0x16 => AbStatus.NotFound, - 0x06 => AbStatus.Partial, - 0x08 => AbStatus.Unsupported, - 0x0A => AbStatus.OutOfBounds, - 0x0C or 0x10 => AbStatus.DeviceStateConflict, - 0x0E or 0x0F => AbStatus.NotAllowed, - 0x13 => AbStatus.TooSmall, - 0x15 => AbStatus.TooLarge, - 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, - 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, + 0x00 or 0x0B => AbStatus.Ok, // success / already in requested mode + 0x04 or 0x05 or 0x16 => AbStatus.NotFound, // path segment error / path unknown / object does not exist + 0x06 => AbStatus.Partial, // partial transfer (fragmented read continues) + 0x08 => AbStatus.Unsupported, // service not supported + 0x0A => AbStatus.OutOfBounds, // attribute list error + 0x0C or 0x10 => AbStatus.DeviceStateConflict, // object state conflict / device state conflict + 0x0E or 0x0F => AbStatus.NotAllowed, // attribute not settable / privilege violation + 0x13 => AbStatus.TooSmall, // not enough data + 0x15 => AbStatus.TooLarge, // too much data + // Lane K F1 (#737): the controller answered and refused this one request — not a garbled reply, so not a + // comms fault that degrades the driver and evicts the runtime. + 0x1C => AbStatus.Unsupported, // missing attribute list entry data + 0x20 => AbStatus.Unsupported, // invalid parameter + 0xFF when extended.Length > 0 && extended[0] == 0x2104 => AbStatus.OutOfBounds, // offset beyond end of tag + 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, // beyond end of the object + 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, // data type mismatch _ => AbStatus.BadReply, }; + /// + /// True for the CIP statuses Lane K F1 moved from to a request-level refusal. + /// A Forward Open refused with one of them is still a connection failure. + /// + /// The CIP general status byte. + /// The extended status words, if any. + /// for 0x1C, 0x20 and 0xFF/0x2104. + internal static bool IsRequestLevelRefusal(byte general, ReadOnlySpan extended) => + general is 0x1C or 0x20 || (general == 0xFF && extended.Length > 0 && extended[0] == 0x2104); + /// Maps a PCCC status byte (and optional extended status) to an . /// The PCCC STS byte. /// The extended status byte, when present. /// The mapped status. + /// + /// Per the DF1/PCCC STS table (1770-6.5.16). Lane K F3 (AbLegacy F8, review 07 r0 I4): 0x20, 0x30 and 0x90 are + /// reported by the HOST or a BRIDGE (a DHRIO, a 1761-NET-ENI) when the END device cannot be reached, so they are + /// the no-reply comms class. Mapping them to an answered refusal made a dead DH+/DH-485 node behind a bridge count + /// as reachable forever — no fail-fast backoff, no comms-loss watchdog. + /// public static AbStatus FromPccc(byte sts, byte? extSts) => sts switch { 0x00 => AbStatus.Ok, - 0x10 => AbStatus.Unsupported, - 0x20 => AbStatus.NotFound, - 0x30 => AbStatus.NotAllowed, - 0x40 or 0x50 => AbStatus.DeviceStateConflict, - 0xF0 when extSts is 0x06 or 0x07 => AbStatus.NotFound, - _ => AbStatus.BadReply, + 0x10 => AbStatus.Unsupported, // illegal command or format + 0x20 => AbStatus.BadConnection, // host has a problem and will not communicate + 0x30 => AbStatus.BadConnection, // remote node host missing, disconnected or shut down + 0x40 => AbStatus.DeviceStateConflict, // host could not complete: hardware fault + 0x50 => AbStatus.NotFound, // addressing problem or memory protect rungs + 0x60 => AbStatus.NotAllowed, // function not allowed due to command protection + 0x70 => AbStatus.DeviceStateConflict, // processor is in Program mode + 0x90 => AbStatus.BadConnection, // remote node cannot buffer command + 0xB0 => AbStatus.DeviceStateConflict, // remote node problem due to download + 0xF0 when extSts is 0x06 => AbStatus.NotFound, // EXT: address does not point to something usable + 0xF0 when extSts is 0x07 => AbStatus.OutOfBounds, // EXT: file is wrong size + _ => AbStatus.BadReply, // 0x80, 0xA0, 0xC0, other EXT STS: unclassified }; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs index 95d2dee7..91d59600 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs @@ -85,8 +85,15 @@ public static class ConnectionManagerServices public static ForwardOpenReply ParseForwardOpenReply(CipReply reply) { if (reply.GeneralStatus != 0) - throw new ForwardOpenRefusedException(reply.Status == AbStatus.BadReply ? AbStatus.BadConnection : reply.Status, - reply.GeneralStatus, reply.ExtendedStatus); + { + // An unclassified refusal, or one of the request-level codes AbStatusCodes maps to a per-request refusal + // for TAG services (0x1C / 0x20 / 0xFF-0x2104), is a connection failure here: no tag can be served. + var status = reply.Status == AbStatus.BadReply + || AbStatusCodes.IsRequestLevelRefusal(reply.GeneralStatus, reply.ExtendedStatus) + ? AbStatus.BadConnection + : reply.Status; + throw new ForwardOpenRefusedException(status, reply.GeneralStatus, reply.ExtendedStatus); + } var r = new CipReader(reply.Data.Span); var ot = r.U32(); var to = r.U32(); r.U16(); r.U16(); r.U32(); return new ForwardOpenReply(ot, to, r.U32(), r.U32()); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs index b746745e..8c911779 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs @@ -47,6 +47,10 @@ public sealed class EncapsulationClient : IAsyncDisposable private readonly SemaphoreSlim _gate = new(1, 1); private long _ctx; private ushort _seq; // connected sequence count; only touched under _gate + + // R07:L5: the 24-byte header of the frame being read. Frames are read only by RunExchangeAsync, which holds _gate + // for the whole exchange, so one buffer per session serves every read (it used to be a fresh array per reply). + private readonly byte[] _head = new byte[EncapsulationHeader.Size]; private volatile bool _faulted; private int _disposed; @@ -282,11 +286,12 @@ public sealed class EncapsulationClient : IAsyncDisposable private async Task ReadFrameAsync(CancellationToken ct) { - var head = new byte[EncapsulationHeader.Size]; - await _stream.ReadExactlyAsync(head, ct).ConfigureAwait(false); - var len = head[2] | (head[3] << 8); + // Only called under _gate (RunExchangeAsync), so the shared header buffer is never read concurrently. The frame + // itself is allocated exactly: the matched reply's CIP payload is returned as a slice of it and outlives the call. + await _stream.ReadExactlyAsync(_head, ct).ConfigureAwait(false); + var len = _head[2] | (_head[3] << 8); var frame = new byte[EncapsulationHeader.Size + len]; - head.CopyTo(frame, 0); + _head.CopyTo(frame, 0); await _stream.ReadExactlyAsync(frame.AsMemory(EncapsulationHeader.Size), ct).ConfigureAwait(false); return frame; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs index ba63cc38..85987cce 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs @@ -1,3 +1,4 @@ +using System.Buffers; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Logix; @@ -52,34 +53,61 @@ public sealed class LogixTagClient(RequestScheduler scheduler, ICipChannel chann var reply = await _scheduler.SendAsync(LogixServices.ReadTagFragmented(path, elements, 0), ct, expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, 0)).ConfigureAwait(false); var (type, handle, first) = LogixServices.ParseReadReply(reply); // throws on any status but 0x00/0x06 - var data = new List(first.Length); - data.AddRange(first.Span); - var lastChunk = first.Length; - while (reply.GeneralStatus == StatusPartial) + // R07:L5: the common single-reply read is one copy of the reply data (it used to go through a List and a + // second ToArray); a fragmented read reassembles in a pooled buffer and copies out once at the end. + if (reply.GeneralStatus != StatusPartial) return (type, handle, first.ToArray()); + + var data = ArrayPool.Shared.Rent(Math.Max(first.Length * 4, 1024)); + var count = 0; + try { - if (lastChunk == 0) - throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); - reply = await _scheduler.SendAsync( - LogixServices.ReadTagFragmented(path, elements, (uint)data.Count), ct, - expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, data.Count)).ConfigureAwait(false); - var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); - if (fragmentType != type) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); - // A mid-read change of the structure handle means the tag's type changed under us (a - // download / online edit); stitching the halves would yield a value of neither type. - if (fragmentHandle != handle) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); - data.AddRange(chunk.Span); - lastChunk = chunk.Length; - if (data.Count > MaxReadBytes) - throw new AbTransportException(AbStatus.TooLarge, - $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); - } + Append(ref data, ref count, first.Span); + var lastChunk = first.Length; - return (type, handle, data.ToArray()); + while (reply.GeneralStatus == StatusPartial) + { + if (lastChunk == 0) + throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); + reply = await _scheduler.SendAsync( + LogixServices.ReadTagFragmented(path, elements, (uint)count), ct, + expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, count)).ConfigureAwait(false); + var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); + if (fragmentType != type) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); + // A mid-read change of the structure handle means the tag's type changed under us (a + // download / online edit); stitching the halves would yield a value of neither type. + if (fragmentHandle != handle) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); + Append(ref data, ref count, chunk.Span); + lastChunk = chunk.Length; + if (count > MaxReadBytes) + throw new AbTransportException(AbStatus.TooLarge, + $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); + } + + return (type, handle, data.AsSpan(0, count).ToArray()); + } + finally + { + ArrayPool.Shared.Return(data); + } + } + + /// Appends to a pooled buffer, growing it (rent bigger, copy, return) as needed. + private static void Append(ref byte[] buffer, ref int count, ReadOnlySpan chunk) + { + if (count + chunk.Length > buffer.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buffer.Length * 2, count + chunk.Length)); + buffer.AsSpan(0, count).CopyTo(bigger); + ArrayPool.Shared.Return(buffer); + buffer = bigger; + } + chunk.CopyTo(buffer.AsSpan(count)); + count += chunk.Length; } private static string FormatHandle(ushort? handle) => handle is { } h ? $"0x{h:X4}" : "(none)"; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs index edd193ee..326dca03 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs @@ -113,7 +113,7 @@ public static class PcccServices public static ReadOnlyMemory ParseReply(CipReply reply, ushort expectedTns, byte[]? expectedRequestorId = null) { if (reply.GeneralStatus != 0) - throw new AbTransportException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); + throw new AbDeviceReplyException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); var r = new CipReader(reply.Data.Span); var idLen = r.U8(); if (idLen < 1) throw new AbTransportException(AbStatus.BadReply, "Empty requestor id in PCCC reply."); @@ -131,7 +131,7 @@ public static class PcccServices throw new AbTransportException(AbStatus.BadReply, $"PCCC TNS {tns} != {expectedTns}."); byte? ext = sts == 0xF0 ? r.U8() : null; if (sts != 0) - throw new AbTransportException(AbStatusCodes.FromPccc(sts, ext), + throw new AbDeviceReplyException(AbStatusCodes.FromPccc(sts, ext), $"PCCC STS 0x{sts:X2}{(ext is { } e ? $" EXT 0x{e:X2}" : "")}."); return reply.Data[r.Position..]; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs index 8c86f186..94b77a79 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs @@ -1,17 +1,38 @@ +using System.Buffers; using System.Buffers.Binary; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Wire; /// Growable little-endian writer for CIP / EIP frames. +/// +/// R07:L5: the working buffer is rented from (a 256-byte array per request used +/// to be allocated and thrown away) and handed back by , which therefore COMPLETES the writer: +/// the result is the caller's own array, a second returns another copy of it, and any further +/// write throws (the buffer belongs to the pool again). A writer abandoned +/// before (a builder that threw) simply leaves its buffer to the GC. +/// public sealed class CipWriter { - private byte[] _buf = new byte[256]; + private const int InitialCapacity = 256; + private byte[]? _buf = ArrayPool.Shared.Rent(InitialCapacity); + private byte[]? _result; + /// Gets the number of bytes written so far. public int Length { get; private set; } + private byte[] Buffer => _buf ?? throw new InvalidOperationException( + "This CipWriter was completed by ToArray(); its buffer has been returned to the pool."); + private Span Take(int n) { - if (Length + n > _buf.Length) Array.Resize(ref _buf, Math.Max(_buf.Length * 2, Length + n)); - var s = _buf.AsSpan(Length, n); + var buf = Buffer; + if (Length + n > buf.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buf.Length * 2, Length + n)); + buf.AsSpan(0, Length).CopyTo(bigger); + ArrayPool.Shared.Return(buf); + _buf = buf = bigger; + } + var s = buf.AsSpan(Length, n); Length += n; return s; } @@ -45,14 +66,27 @@ public sealed class CipWriter /// The replacement value. /// The two bytes at are not already /// written (an offset in the spare buffer capacity would otherwise be patched silently and lost). + /// The writer was completed by . public void PatchU16(int offset, ushort v) { + var buf = Buffer; if (offset < 0 || offset > Length - 2) throw new ArgumentOutOfRangeException(nameof(offset), offset, $"Cannot patch 2 bytes at {offset}: only {Length} bytes written."); - BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v); + BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(offset, 2), v); } - /// Copies the written bytes to a new array. - /// The bytes written so far. - public byte[] ToArray() => _buf.AsSpan(0, Length).ToArray(); + /// + /// Copies the written bytes to a new array and returns the working buffer to the pool, completing the writer + /// (see the remarks on ). + /// + /// The bytes written, in an array the caller owns. + public byte[] ToArray() + { + if (_result is { } done) return (byte[])done.Clone(); + var buf = Buffer; + _result = buf.AsSpan(0, Length).ToArray(); + _buf = null; + ArrayPool.Shared.Return(buf); + return _result; + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs index 334568a1..2bded247 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs @@ -13,10 +13,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// never writes Degraded, never opens the device-wide connect backoff, and warns once per tag. Everything /// else (timeout, bad connection, a reply that failed to parse, partial, abort, …) is a comms fault. /// -/// The native transport currently folds CIP general statuses 0x1C, 0x20 and 0xFF/0x2104 into -/// (AbStatusCodes.FromCip, owned by Driver.Ab.Transport), which -/// this classification cannot tell from a garbled reply; until the transport maps them explicitly they stay -/// comms faults. +/// The native transport maps CIP general statuses 0x1C (missing attribute list entry) and 0x20 (invalid +/// parameter) to and 0xFF/0x2104 (offset beyond end) to +/// (AbStatusCodes.FromCip, Lane K F1), so they classify here as +/// device refusals. A Forward Open refused with one of them stays a connection failure. /// /// internal static class AbCipRefusals 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 2444f3c1..9fdcddf9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -789,9 +789,9 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); return (OpVerdict.Neutral, null); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { - // The native runtime raises a device's refusal (e.g. STS 0x20 "file not found") as an exception. + // The native runtime raises a device's refusal (e.g. STS 0x50 "addressing problem") as an exception. WarnRefusalOnce(device, def, "read", ex.Message, AbLegacyStatusMapper.MapStatus(ex.Status)); results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.MapStatus(ex.Status), null, now); return (OpVerdict.Answered, null); @@ -838,10 +838,10 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// reachability (backoff reset, LastSuccessfulRead, Healthy). /// /// - /// ⚠️ Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc, Lane K): it maps STS 0x30 - /// ("remote node missing" — a dead DH+/DH-485 node behind a bridge) and 0x20 to NotAllowed/NotFound, - /// so on the NATIVE transport such a dead node still reads as answered. Follow-up F8 (review 07 r0 I4); this - /// classification needs no change once it lands. + /// Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc). Since Lane K F3 (F8, review + /// 07 r0 I4) STS 0x20 / 0x30 / 0x90 — the host or a bridge reporting an unreachable END device — map to + /// (no reply), so a dead DH+/DH-485 node behind a bridge is a comms fault, + /// not an answer (NativePcccBridgeStatusHealthTests). /// /// The transport status. /// for a device refusal. @@ -1035,7 +1035,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[i] = new WriteResult(AbLegacyStatusMapper.BadCommunicationError); outcome.FaultMessage ??= tex.Message; } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { device.RecordAnswered(); outcome.AnyAnswered = true; @@ -1210,7 +1210,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; - if (ex is AbTransportException ate && IsDeviceRefusal(ate.Status)) state.RecordAnswered(); + if (IsAnsweredRefusal(ex)) state.RecordAnswered(); else if (ex is not AbTransportException { Status: var st } || IsNoReply(st)) state.RecordCommsFault(Clock()); } @@ -1478,7 +1478,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag { return (null, AbLegacyStatusMapper.BadNotSupported, false, false); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { // The device answered: the index file/element does not exist. Per-tag, not a comms fault. return (null, AbLegacyStatusMapper.MapStatus(ex.Status), false, false); @@ -1617,10 +1617,26 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// all PLC-5 writes), and libplctag's ErrorUnsupported maps to it too. It says nothing about the /// device's reachability, so it must not evict the handle or degrade driver health. /// + /// + /// An Unsupported that came OFF THE WIRE (: CIP 0x08 / 0x1C / 0x20 on the + /// Execute PCCC envelope, PCCC STS 0x10) is NOT local — the device answered — and is classified by + /// instead (review lane K I2). + /// /// The exception to classify. /// if the exception is a local refusal that never reached the wire. internal static bool IsLocalRefusal(Exception ex) => - ex is AbTransportException { Status: AbStatus.Unsupported }; + ex is AbTransportException { Status: AbStatus.Unsupported } and not AbDeviceReplyException; + + /// + /// True for an exception proving the device ANSWERED and refused this operation: a device-refusal status + /// (), or an that came off the wire + /// (). Per-tag Bad, keeps the handle, advances LastSuccessfulRead. + /// + /// The exception to classify. + /// for an answered refusal. + internal static bool IsAnsweredRefusal(Exception ex) => + ex is AbTransportException ate + && (IsDeviceRefusal(ate.Status) || ate is AbDeviceReplyException { Status: AbStatus.Unsupported }); /// /// Initializes a freshly created runtime. A NATIVE runtime whose init (one read) is refused locally is diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 83a28405..36cea8d1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -1603,7 +1603,8 @@ public sealed class ModbusDriver for (var i = 0; i < values.Length; i++) if (values[i]) bitmap[i / 8] |= (byte)(1 << (i % 8)); var qty = (ushort)values.Length; - var pdu15 = new byte[6 + 1 + byteCount]; + // [fc][addr:2][qty:2][byteCount] + data: exactly 6 + byteCount (no trailing byte past the declared data). + var pdu15 = new byte[6 + byteCount]; pdu15[0] = 0x0F; pdu15[1] = (byte)(tag.Address >> 8); pdu15[2] = (byte)(tag.Address & 0xFF); pdu15[3] = (byte)(qty >> 8); pdu15[4] = (byte)(qty & 0xFF); @@ -1638,7 +1639,8 @@ public sealed class ModbusDriver $"Write of {qty} registers to {tag.Name} exceeds MaxRegistersPerWrite={writeCap}. " + $"Split the tag (e.g. shorter StringLength or smaller ArrayCount) — partial FC16 chunks would lose atomicity."); if (await TryWriteViaFc23Async(transport, tag, bytes, qty, ct).ConfigureAwait(false)) return; - var pdu = new byte[6 + 1 + bytes.Length]; + // [fc][addr:2][qty:2][byteCount] + data: exactly 6 + byteCount. + var pdu = new byte[6 + bytes.Length]; pdu[0] = 0x10; pdu[1] = (byte)(tag.Address >> 8); pdu[2] = (byte)(tag.Address & 0xFF); pdu[3] = (byte)(qty >> 8); pdu[4] = (byte)(qty & 0xFF); @@ -1685,7 +1687,8 @@ public sealed class ModbusDriver if (!_options.UseFC23ForMultiRegisterWrites || _options.DisableFC23 || qty < 2 || qty > Fc23MaxWriteRegisters || unsupported.ContainsKey(unit)) return false; - var pdu = new byte[10 + 1 + bytes.Length]; + // [fc][readAddr:2][readQty:2][writeAddr:2][writeQty:2][writeByteCount] + data: exactly 10 + byteCount. + var pdu = new byte[10 + bytes.Length]; pdu[0] = 0x17; pdu[1] = (byte)(tag.Address >> 8); pdu[2] = (byte)(tag.Address & 0xFF); // read start pdu[3] = (byte)(qty >> 8); pdu[4] = (byte)(qty & 0xFF); // read quantity @@ -1824,7 +1827,8 @@ public sealed class ModbusDriver payload[i * 2] = (byte)(reg >> 8); payload[i * 2 + 1] = (byte)(reg & 0xFF); } - var pdu = new byte[6 + 1 + payload.Length]; + // [fc][addr:2][qty:2][byteCount] + data: exactly 6 + byteCount. + var pdu = new byte[6 + payload.Length]; pdu[0] = 0x10; pdu[1] = (byte)(tag.Address >> 8); pdu[2] = (byte)(tag.Address & 0xFF); pdu[3] = (byte)(count >> 8); pdu[4] = (byte)(count & 0xFF); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs index 9fd9c682..bec5055d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs @@ -1,3 +1,4 @@ +using System.Buffers; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; /// @@ -55,12 +56,33 @@ public static class ModbusRtuFraming /// A new array: [unitId, ...pdu, crcLo, crcHi]. public static byte[] BuildAdu(byte unitId, ReadOnlySpan pdu) { - var frame = new byte[1 + pdu.Length]; - frame[0] = unitId; - pdu.CopyTo(frame.AsSpan(1)); - return ModbusCrc.Append(frame); + var frame = new byte[AduLength(pdu.Length)]; + WriteAdu(unitId, pdu, frame); + return frame; } + /// The length of the RTU ADU for a PDU of bytes (unit id + PDU + CRC). + /// The bare PDU length. + /// The ADU length. + internal static int AduLength(int pduLength) => 1 + pduLength + 2; + + /// Writes the RTU ADU into (exactly bytes). + /// The RTU slave/unit address. + /// The bare PDU. + /// The ADU buffer, exactly (.Length) long. + internal static void WriteAdu(byte unitId, ReadOnlySpan pdu, Span destination) + { + destination[0] = unitId; + pdu.CopyTo(destination[1..]); + var body = destination[..(1 + pdu.Length)]; + var crc = ModbusCrc.Compute(body); + destination[1 + pdu.Length] = (byte)(crc & 0xFF); // CRC low byte first + destination[2 + pdu.Length] = (byte)(crc >> 8); + } + + /// The largest RTU response frame: addr + fc + byteCount + 255 data bytes + CRC. + private const int MaxResponseFrame = 3 + 255 + 2; + /// /// Reads a single RTU response frame from and returns its bare PDU /// ([fc, ...data]), with the leading unit-id and trailing CRC stripped. @@ -82,58 +104,56 @@ public static class ModbusRtuFraming public static async Task ReadResponsePduAsync( Stream stream, byte expectedUnit, CancellationToken ct) { - // Header: addr(1) + fc(1). The function code selects how many more bytes to read. - var header = new byte[2]; - await ReadExactlyAsync(stream, header, ct).ConfigureAwait(false); - var fc = header[1]; + // R07:L5: the whole frame is read into ONE pooled buffer and validated in place; only the returned PDU is + // allocated (it used to be five arrays: header, byteCount, rest, the reassembled frame and the PDU). + var frame = ArrayPool.Shared.Rent(MaxResponseFrame); + try + { + // Header: addr(1) + fc(1). The function code selects how many more bytes to read. + await ReadExactlyAsync(stream, frame.AsMemory(0, 2), ct).ConfigureAwait(false); + var fc = frame[1]; - int trailing; // bytes after addr+fc, up to and including the 2 CRC bytes. - if ((fc & 0x80) != 0) - { - // Exception frame: excCode(1) + CRC(2). - trailing = 1 + 2; - } - else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) - { - // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then - // byteCount data bytes + CRC(2). - var bc = new byte[1]; - await ReadExactlyAsync(stream, bc, ct).ConfigureAwait(false); - var byteCount = bc[0]; - var rest = new byte[byteCount + 2]; - await ReadExactlyAsync(stream, rest, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, bc, rest); - } - else - { - // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is - // variable-length and handled above; any NEW variable-length response FC must be added there - // or it is mis-sized here and surfaces as a desync. - trailing = 4 + 2; - } + int length; // the whole frame, addr through CRC + var already = 2; // bytes of the frame read so far + if ((fc & 0x80) != 0) + { + // Exception frame: excCode(1) + CRC(2). + length = 2 + 1 + 2; + } + else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) + { + // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then + // byteCount data bytes + CRC(2). + await ReadExactlyAsync(stream, frame.AsMemory(2, 1), ct).ConfigureAwait(false); + length = 3 + frame[2] + 2; + already = 3; + } + else + { + // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is + // variable-length and handled above; any NEW variable-length response FC must be added there + // or it is mis-sized here and surfaces as a desync. + length = 2 + 4 + 2; + } - var tail = new byte[trailing]; - await ReadExactlyAsync(stream, tail, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, ReadOnlySpan.Empty, tail); + await ReadExactlyAsync(stream, frame.AsMemory(already, length - already), ct).ConfigureAwait(false); + return ValidateAndStrip(expectedUnit, frame.AsSpan(0, length)); + } + finally + { + ArrayPool.Shared.Return(frame); + } } /// - /// Assembles the full frame from its pieces, validates the trailing CRC over everything - /// except the CRC bytes, then validates the response address against - /// and strips addr + CRC to return the bare PDU. + /// Validates the trailing CRC over everything except the CRC bytes, then validates the response address + /// against and strips addr + CRC to return the bare PDU. /// A CRC-valid exception PDU throws . /// - private static byte[] ValidateAndStrip( - byte expectedUnit, ReadOnlySpan header, ReadOnlySpan middle, ReadOnlySpan tail) + private static byte[] ValidateAndStrip(byte expectedUnit, ReadOnlySpan frame) { - // Reassemble the whole frame: header(addr,fc) + middle(optional byteCount) + tail. - var frame = new byte[header.Length + middle.Length + tail.Length]; - header.CopyTo(frame); - middle.CopyTo(frame.AsSpan(header.Length)); - tail.CopyTo(frame.AsSpan(header.Length + middle.Length)); - // CRC covers everything except the trailing 2 CRC bytes. - var body = frame.AsSpan(0, frame.Length - 2); + var body = frame[..^2]; var expected = ModbusCrc.Compute(body); var actual = (ushort)(frame[^2] | (frame[^1] << 8)); if (actual != expected) @@ -172,12 +192,12 @@ public static class ModbusRtuFraming /// so a length-less RTU frame that arrives /// short is classified as a desync rather than a bare . /// - private static async Task ReadExactlyAsync(Stream stream, byte[] buf, CancellationToken ct) + private static async Task ReadExactlyAsync(Stream stream, Memory buf, CancellationToken ct) { var read = 0; while (read < buf.Length) { - var n = await stream.ReadAsync(buf.AsMemory(read), ct).ConfigureAwait(false); + var n = await stream.ReadAsync(buf[read..], ct).ConfigureAwait(false); if (n == 0) throw new ModbusTransportDesyncException( ModbusTransportDesyncException.DesyncReason.TruncatedFrame, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs index 7dcc5dff..e2f4047e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -150,14 +151,17 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport var stream = CurrentStream; if (stream is null) throw new InvalidOperationException("Transport not connected"); - // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. - var adu = ModbusRtuFraming.BuildAdu(unitId, pdu); + // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. Built in a pooled buffer (R07:L5); the same bytes + // ModbusRtuFraming.BuildAdu produces (pinned by a test). + var aduLength = ModbusRtuFraming.AduLength(pdu.Length); + var adu = ArrayPool.Shared.Rent(aduLength); + ModbusRtuFraming.WriteAdu(unitId, pdu, adu.AsSpan(0, aduLength)); using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); // Framing owns the length-less RTU deframe + CRC + unit-address validation. A CRC-valid @@ -181,6 +185,11 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the stream torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } /// Asynchronously disposes the transport and underlying socket resources. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs index f21ce678..6889b1e2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -59,6 +60,10 @@ public sealed class ModbusTcpTransport : IModbusTransport /// The socket lifecycle this transport composes (test seam for its clock). internal ModbusSocketLifecycle Lifecycle => _life; + // R07:L5: the 7-byte MBAP header of the reply being read. SendOnceAsync runs only under _gate (single-flight), so + // one buffer per transport serves every transaction (it used to be a fresh array per reply). + private readonly byte[] _header = new byte[7]; + /// public Task ConnectAsync(CancellationToken ct) => _life.ConnectAsync(ct); @@ -139,11 +144,16 @@ public sealed class ModbusTcpTransport : IModbusTransport if (stream is null) throw new InvalidOperationException("Transport not connected"); var txId = ++_nextTx; - // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU - var adu = new byte[7 + pdu.Length]; + // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU. Built in a pooled buffer (R07:L5): the request + // never escapes, and the write has completed (or been cancelled and the socket torn down) before the finally + // hands it back. Only the reply PDU, which the caller keeps, is allocated. + // (One async method on purpose: a second awaited helper costs a state-machine box per call, which ate the saving.) + var aduLength = 7 + pdu.Length; + var adu = ArrayPool.Shared.Rent(aduLength); adu[0] = (byte)(txId >> 8); adu[1] = (byte)(txId & 0xFF); - // protocol id already zero + adu[2] = 0; // protocol id + adu[3] = 0; var len = (ushort)(1 + pdu.Length); // unit id + pdu adu[4] = (byte)(len >> 8); adu[5] = (byte)(len & 0xFF); @@ -154,10 +164,10 @@ public sealed class ModbusTcpTransport : IModbusTransport cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); - var header = new byte[7]; + var header = _header; await ReadExactlyAsync(stream, header, cts.Token).ConfigureAwait(false); var respTxId = (ushort)((header[0] << 8) | header[1]); if (respTxId != txId) @@ -211,6 +221,11 @@ public sealed class ModbusTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the socket torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } private static async Task ReadExactlyAsync(Stream s, byte[] buf, CancellationToken ct) diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs new file mode 100644 index 00000000..8d150ae2 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs @@ -0,0 +1,87 @@ +using CliFx.Exceptions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// historyread --aggregate accepts every aggregate the client offers (which is exactly the served set), so the +/// gate recipes in docs/Historian.md that need Range / Delta / PercentGood / ... can run from this CLI. +/// +public class HistoryReadAggregateParsingTests +{ + /// Every client aggregate, by its name. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); + + /// Every aggregate name (in any case) reaches the service as that aggregate. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public async Task Every_aggregate_name_parses(AggregateType aggregate) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = aggregate.ToString().ToUpperInvariant() + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(aggregate); + } + + /// The short aliases still parse. + /// The alias. + /// The aggregate it names. + [Theory] + [InlineData("avg", AggregateType.Average)] + [InlineData("min", AggregateType.Minimum)] + [InlineData("last", AggregateType.End)] + [InlineData("interp", AggregateType.Interpolative)] + [InlineData("mintime", AggregateType.MinimumActualTime)] + [InlineData("maxtime", AggregateType.MaximumActualTime)] + public async Task Aliases_parse(string alias, AggregateType expected) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = alias + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(expected); + } + + /// + /// StandardDeviation is no longer offered (no backend serves it): it is refused before connecting, naming the + /// supported set. + /// + [Fact] + public async Task StandardDeviation_is_refused_before_connecting() + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = "stddev" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + + ex.Message.ShouldContain("PercentGood"); + fakeService.HistoryReadAggregateCalls.ShouldBeEmpty(); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs new file mode 100644 index 00000000..07980b36 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs @@ -0,0 +1,57 @@ +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters; + +/// +/// A deleted subscription must also leave the SDK session's Subscriptions list, or every alarm re-target in the UI +/// leaves a dead behind until the session is disposed. +/// +public class DefaultSubscriptionAdapterTests +{ + /// DeleteAsync detaches the wrapped subscription exactly once, even when Dispose follows. + [Fact] + public async Task DeleteAsync_detaches_the_subscription_from_its_session_once() + { + var subscription = new Subscription(); + var detached = new List(); + var adapter = new DefaultSubscriptionAdapter(subscription, (s, _) => + { + lock (detached) detached.Add(s); + return Task.CompletedTask; + }); + + await adapter.DeleteAsync(TestContext.Current.CancellationToken); + adapter.Dispose(); + + detached.ShouldHaveSingleItem().ShouldBeSameAs(subscription); + } + + /// Dispose alone (no prior DeleteAsync) detaches too. + [Fact] + public void Dispose_detaches_the_subscription_from_its_session() + { + var detached = 0; + var adapter = new DefaultSubscriptionAdapter(new Subscription(), (_, _) => + { + Interlocked.Increment(ref detached); + return Task.CompletedTask; + }); + + adapter.Dispose(); + + detached.ShouldBe(1); + } + + /// A failing detach is logged, not thrown: teardown stays best effort. + [Fact] + public async Task A_failing_detach_does_not_throw_from_DeleteAsync() + { + var adapter = new DefaultSubscriptionAdapter(new Subscription(), + (_, _) => throw new InvalidOperationException("boom")); + + await Should.NotThrowAsync(() => adapter.DeleteAsync(TestContext.Current.CancellationToken)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs new file mode 100644 index 00000000..362bc456 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs @@ -0,0 +1,177 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests; + +/// +/// Lane K review I1: a keep-alive failover belongs to the connection it started under. A user Disconnect + Connect +/// while it is still connecting starts a new connection epoch; the stale failover must abandon itself — never +/// overwrite the user's new session, never flip its state. Every step is gate-driven (no sleeps). +/// +public sealed class FailoverEpochTests : IDisposable +{ + private const string Primary = "opc.tcp://primary:4840"; + private const string Backup = "opc.tcp://backup:4840"; + private static readonly TimeSpan Wait = TimeSpan.FromSeconds(10); + + private readonly FakeSessionFactory _factory = new(); + private readonly OpcUaClientService _service; + private readonly List _states = []; + + public FailoverEpochTests() + { + _service = new OpcUaClientService(new FakeApplicationConfigurationFactory(), new FakeEndpointDiscovery(), _factory); + _service.ConnectionStateChanged += (_, e) => { lock (_states) _states.Add(e.NewState); }; + } + + public void Dispose() => _service.Dispose(); + + private static ConnectionSettings Settings() => new() + { + EndpointUrl = Primary, FailoverUrls = [Backup], SessionTimeoutSeconds = 60 + }; + + /// Connects S1, then holds a keep-alive failover inside its connect attempt. + private async Task<(FakeSessionAdapter S1, Task Failover, TaskCompletionSource Gate)> StartHeldFailoverAsync() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + + var gate = new TaskCompletionSource(); + _factory.CreateGate = gate; + s1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + await _factory.CreateEntered.Task.WaitAsync(Wait); + return (s1, failover, gate); + } + + /// The stale failover's attempt SUCCEEDS after the user reconnected: its session is disposed, the user's stays. + [Fact] + public async Task A_stale_failover_that_connects_does_not_replace_the_users_new_session() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + var userSession = new FakeSessionAdapter(); + var failoverSession = new FakeSessionAdapter(); + _factory.EnqueueSession(userSession); // dequeued by the user's Connect (the failover is still held) + _factory.EnqueueSession(failoverSession); // dequeued by the failover once released + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + failoverSession.Disposed.ShouldBeTrue(); + userSession.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Primary); + lock (_states) _states.ShouldBeEmpty(); // the stale failover made no state transition at all + } + + /// The stale failover's attempts all FAIL after the user reconnected: the user's connection stays Connected. + [Fact] + public async Task A_stale_failover_that_fails_does_not_flip_the_users_connection_to_Disconnected() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + var createsBefore = _factory.CreateCallCount; + + _factory.ThrowOnCreate = true; + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + lock (_states) _states.ShouldBeEmpty(); + // It stopped at the abandoned attempt instead of trying every remaining endpoint (review m5). + _factory.CreateCallCount.ShouldBe(createsBefore); + // And the user can still disconnect / reconnect normally afterwards. + _factory.ThrowOnCreate = false; + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + _service.IsConnected.ShouldBeTrue(); + } + + /// Positive control: an undisturbed failover still reconnects to the backup. + [Fact] + public async Task A_genuine_failover_still_reconnects() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Backup); + lock (_states) _states.ShouldContain(ConnectionState.Reconnecting); + } + + /// + /// A late bad keep-alive from the PREVIOUS connection's session must not start a failover against the new one. + /// Positive control: the same signal from the current session does (the genuine-failover test above). + /// + [Fact] + public async Task A_keep_alive_from_a_previous_connection_does_not_fail_over_the_current_one() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + await _service.DisconnectAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + await _service.ConnectAsync(Settings()); + var creates = _factory.CreateCallCount; + lock (_states) _states.Clear(); + + s1.SimulateKeepAlive(false); + if (_service.LastFailover is { } t) await t.WaitAsync(Wait); + + _factory.CreateCallCount.ShouldBe(creates); + lock (_states) _states.ShouldBeEmpty(); + _service.IsConnected.ShouldBeTrue(); + } + + /// Review m8: re-entrant bad keep-alives do not overwrite the seam with an already-completed no-op. + [Fact] + public async Task Reentrant_keep_alives_leave_the_seam_on_the_running_failover() + { + var (s1, failover, gate) = await StartHeldFailoverAsync(); + s1.SimulateKeepAlive(false); + s1.SimulateKeepAlive(false); + + _service.LastFailover.ShouldBeSameAs(failover); + failover.IsCompleted.ShouldBeFalse(); + _factory.EnqueueSession(new FakeSessionAdapter()); + gate.SetResult(); + await failover.WaitAsync(Wait); + } + + /// + /// Review m5: an Unsubscribe whose server-side remove fails (the session died under it) still drops the + /// bookkeeping, so a failover does not replay an item the caller asked to stop. Proven by a re-subscribe of the + /// same node creating a fresh monitored item instead of returning "already subscribed". + /// + [Fact] + public async Task A_failed_unsubscribe_still_forgets_the_item() + { + var sub = new FakeSubscriptionAdapter(); + _factory.EnqueueSession(new FakeSessionAdapter { NextSubscription = sub }); + await _service.ConnectAsync(Settings()); + var node = new Opc.Ua.NodeId("ns=2;s=X"); + await _service.SubscribeAsync(node); + + sub.ThrowOnRemove = true; + await Should.ThrowAsync(() => _service.UnsubscribeAsync(node)); + sub.ThrowOnRemove = false; + + await _service.SubscribeAsync(node); + sub.AddDataChangeCount.ShouldBe(2); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs index a73a9f65..27504765 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs @@ -28,8 +28,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public int BrowseCount { get; private set; } /// Gets the number of times BrowseNextAsync has been called. public int BrowseNextCount { get; private set; } - /// Gets the number of times HasChildrenAsync has been called. - public int HasChildrenCount { get; private set; } /// Gets the number of times HistoryReadRawAsync has been called. public int HistoryReadRawCount { get; private set; } /// Gets the number of times HistoryReadAggregateAsync has been called. @@ -57,8 +55,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public ReferenceDescriptionCollection BrowseNextResponse { get; set; } = []; /// Gets or sets the continuation point for browse-next operations. public byte[]? BrowseNextContinuationPoint { get; set; } - /// Gets or sets the result returned by has-children operations. - public bool HasChildrenResponse { get; set; } = false; /// Gets or sets the historical values returned by raw history-read operations. public List HistoryReadRawResponse { get; set; } = []; @@ -150,13 +146,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult((BrowseNextContinuationPoint, BrowseNextResponse)); } - /// - public Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - HasChildrenCount++; - return Task.FromResult(HasChildrenResponse); - } - /// public Task> HistoryReadRawAsync( NodeId nodeId, DateTime startTime, DateTime endTime, int maxValues, CancellationToken ct) @@ -178,15 +167,31 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult>(HistoryReadAggregateResponse); } + /// + /// Optional gate that, when set, holds open until completed, so a test can keep + /// a subscribe in flight while something else (a failover) happens. Signalled via . + /// + public TaskCompletionSource? CreateSubscriptionGate { get; set; } + + /// Completed when is entered while a gate is set. + public TaskCompletionSource CreateSubscriptionEntered { get; } = + new(TaskCreationOptions.RunContinuationsAsynchronously); + /// - public Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) + public async Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) { + if (CreateSubscriptionGate is { } gate) + { + CreateSubscriptionEntered.TrySetResult(); + await gate.Task; + } + lock (_createdSubscriptions) { var sub = NextSubscription ?? new FakeSubscriptionAdapter(); NextSubscription = null; _createdSubscriptions.Add(sub); - return Task.FromResult(sub); + return sub; } } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs index c1189a78..24cbd13a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs @@ -21,6 +21,9 @@ internal sealed class FakeSessionFactory : ISessionFactory /// public TaskCompletionSource? CreateGate { get; set; } + /// Completed when a call observes . + public TaskCompletionSource CreateEntered { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + /// Gets the list of sessions created via CreateSessionAsync. public IReadOnlyList CreatedSessions => _createdSessions; @@ -39,8 +42,14 @@ internal sealed class FakeSessionFactory : ISessionFactory CreateCallCount++; LastEndpointUrl = endpoint.EndpointUrl; - if (CreateGate != null) - await CreateGate.Task; + // One-shot: the gate holds only the call that observed it, so a later call (e.g. a user's Connect while a + // failover is held) runs freely. + if (CreateGate is { } gate) + { + CreateGate = null; + CreateEntered.TrySetResult(); + await gate.Task; + } if (ThrowOnCreate) throw new InvalidOperationException("FakeSessionFactory configured to fail."); diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs index 0c7c4233..1e442575 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs @@ -37,6 +37,9 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// Gets the number of times AddEventMonitoredItemAsync was called. public int AddEventCount { get; private set; } + /// Gets or sets a value indicating whether RemoveMonitoredItemAsync throws (a session that died). + public bool ThrowOnRemove { get; set; } + /// Gets the number of times RemoveMonitoredItemAsync was called. public int RemoveCount { get; private set; } @@ -70,6 +73,8 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// public Task RemoveMonitoredItemAsync(uint clientHandle, CancellationToken ct) { + if (ThrowOnRemove) + throw new ServiceResultException(StatusCodes.BadSessionClosed, "Session closed"); lock (_itemsLock) { RemoveCount++; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs index 663ab473..267baf02 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs @@ -3,6 +3,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Sql.Common; namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; @@ -17,7 +18,6 @@ public class AggregateTypeMapperTests [InlineData(AggregateType.Count)] [InlineData(AggregateType.Start)] [InlineData(AggregateType.End)] - [InlineData(AggregateType.StandardDeviation)] [InlineData(AggregateType.TimeAverage)] [InlineData(AggregateType.TimeAverage2)] [InlineData(AggregateType.Total)] @@ -72,14 +72,6 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.End).ShouldBe(ObjectIds.AggregateFunction_End); } - /// Verifies that StandardDeviation aggregate type maps to the correct OPC UA node ID. - [Fact] - public void ToNodeId_StandardDeviation_MapsCorrectly() - { - AggregateTypeMapper.ToNodeId(AggregateType.StandardDeviation) - .ShouldBe(ObjectIds.AggregateFunction_StandardDeviationPopulation); - } - /// Verifies that invalid aggregate type value throws ArgumentOutOfRangeException. [Fact] public void ToNodeId_InvalidValue_Throws() @@ -98,4 +90,31 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.Total2).ShouldBe(ObjectIds.AggregateFunction_Total2); AggregateTypeMapper.ToNodeId(AggregateType.Interpolative).ShouldBe(ObjectIds.AggregateFunction_Interpolative); } -} \ No newline at end of file + + /// + /// The client offers exactly the aggregates the server's SQL backends serve (): + /// nothing the server always refuses (the old StandardDeviation), nothing served left unreachable + /// (Range, Delta, Min/MaxActualTime, PercentGood/Bad). + /// + [Fact] + public void Client_aggregates_are_exactly_the_served_set() + { + Enum.GetNames().Order() + .ShouldBe(AggregateFamilies.Served.Select(a => a.ToString()).Order()); + } + + /// Each aggregate maps to the Part 13 ObjectId of the same name. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public void ToNodeId_maps_to_the_same_named_AggregateFunction(AggregateType aggregate) + { + var field = typeof(ObjectIds).GetField("AggregateFunction_" + aggregate); + field.ShouldNotBeNull(); + AggregateTypeMapper.ToNodeId(aggregate).ShouldBe((NodeId)field.GetValue(null)!); + } + + /// Every client aggregate. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs new file mode 100644 index 00000000..bd8c3d1c --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs @@ -0,0 +1,219 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; + +/// +/// Pins the client-side HistoryRead paging loop shared by the raw and modified reads: continuation pages are read +/// (never released) while the caller wants more, and a read stopped by the caller's own cap releases the server cursor. +/// +public class HistoryReadPagingTests +{ + private static readonly NodeId Node = new("t", 2); + + /// One recorded call to the page reader. + private sealed record Call(byte[]? ContinuationPoint, bool Release); + + /// A scripted server: each read returns the next page; a release returns an empty Good result. + private sealed class ScriptedServer + { + private readonly Queue<(int Count, byte[]? Cp)> _pages; + public List Calls { get; } = []; + + public ScriptedServer(params (int Count, byte[]? Cp)[] pages) => _pages = new Queue<(int, byte[]?)>(pages); + + public Task ReadPageAsync(byte[]? cp, bool release, CancellationToken ct) + { + Calls.Add(new Call(cp, release)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + var (count, next) = _pages.Dequeue(); + var data = new HistoryData(); + for (var i = 0; i < count; i++) data.DataValues.Add(new DataValue(new Variant(i))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, + ContinuationPoint = next, + HistoryData = new ExtensionObject(data) + }); + } + } + + private static IEnumerable Values(HistoryReadResult r) => + r.HistoryData?.Body is HistoryData d ? d.DataValues : []; + + /// + /// A server page limit below the caller's cap hands back a continuation point: the follow-up must READ the next + /// page. Releasing it instead (the old raw path passed release = cp != null) silently truncated every raw read + /// to its first server page. + /// + [Fact] + public async Task Continuation_pages_are_read_not_released_until_the_server_finishes() + { + var server = new ScriptedServer((3, [1]), (3, [2]), (2, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, + CancellationToken.None); + + values.Count.ShouldBe(8); + server.Calls.Select(c => c.Release).ShouldAllBe(r => r == false); + server.Calls.Select(c => c.ContinuationPoint?.FirstOrDefault()).ShouldBe(new byte?[] { null, 1, 2 }); + } + + /// A read stopped by the caller's cap while the server still holds a cursor releases that cursor. + [Fact] + public async Task A_read_stopped_by_the_cap_releases_the_outstanding_continuation_point() + { + var server = new ScriptedServer((5, [7])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.Count.ShouldBe(2); + server.Calls[1].Release.ShouldBeTrue(); + server.Calls[1].ContinuationPoint.ShouldBe(new byte[] { 7 }); + } + + /// + /// A read the server finished (no continuation point) has nothing to release — and the positive control above + /// proves a release would have been recorded had one been owed. + /// + [Fact] + public async Task A_read_the_server_finished_issues_no_release() + { + var server = new ScriptedServer((5, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.ShouldHaveSingleItem().Release.ShouldBeFalse(); + } + + /// + /// Review m3: a non-positive cap is refused. An uncapped client read would materialise a tag's whole window + /// (the client side of R08 1.1), and (uint)-1 on the wire asked the server for everything. + /// + /// The cap. + [Theory] + [InlineData(0)] + [InlineData(-1)] + public async Task A_non_positive_cap_is_refused_before_any_read(int max) + { + var server = new ScriptedServer(); + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, max, Node, CancellationToken.None)); + server.Calls.ShouldBeEmpty(); + } + + /// Review m2: server pages that overshoot the cap are trimmed to it (60 + 60 with a cap of 100 is 100). + [Fact] + public async Task The_result_is_trimmed_to_the_cap() + { + var server = new ScriptedServer((60, [1]), (60, [2])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, + CancellationToken.None); + + values.Count.ShouldBe(100); + server.Calls.Last().Release.ShouldBeTrue(); // the outstanding cursor is still released + } + + /// + /// Review m1: a read cancelled between pages releases the outstanding cursor (best effort, not on the caller's + /// cancelled token) and still surfaces the cancellation. Positive control: the release IS recorded. + /// + [Fact] + public async Task A_read_cancelled_mid_paging_releases_the_cursor_and_rethrows() + { + using var cts = new CancellationTokenSource(); + var calls = new List<(byte[]? Cp, bool Release, bool TokenCancelled)>(); + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls.Add((cp, release, ct.IsCancellationRequested)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + if (cp != null) { cts.Cancel(); ct.ThrowIfCancellationRequested(); } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, cts.Token)); + + var release = calls.Where(c => c.Release).ShouldHaveSingleItem(); + release.Cp.ShouldBe(new byte[] { 5 }); + release.TokenCancelled.ShouldBeFalse(); + } + + /// A cancellation that lands while the cap release is in flight does not lose the values read. + [Fact] + public async Task The_cap_release_does_not_ride_the_callers_token() + { + using var cts = new CancellationTokenSource(); + var releaseSawCancelled = (bool?)null; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + if (release) + { + releaseSawCancelled = ct.IsCancellationRequested; + return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + cts.Cancel(); // the caller gives up right as the capped page arrives + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, cts.Token); + + values.Count.ShouldBe(1); + releaseSawCancelled.ShouldBe(false); + } + + /// A failed release is best effort: the values already read are still returned. + [Fact] + public async Task A_failed_release_does_not_lose_the_values_read() + { + var calls = 0; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls++; + if (release) throw new ServiceResultException(StatusCodes.BadSessionClosed); + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [9], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, CancellationToken.None); + + values.Count.ShouldBe(1); + calls.ShouldBe(2); + } + + /// A refused page still surfaces as an exception, not an empty read. + [Fact] + public async Task A_refused_page_throws() + { + Task Read(byte[]? cp, bool release, CancellationToken ct) => + Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.BadHistoryOperationUnsupported + }); + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, CancellationToken.None)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs index 3d3a32a4..a9f01b51 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs @@ -132,10 +132,10 @@ public class ModelConstructionTests ((int)ConnectionState.Reconnecting).ShouldBe(3); } - /// Verifies that AggregateType enum has the expected number of values. + /// AggregateType holds the seventeen served aggregates (the parity test pins which). [Fact] public void AggregateType_Enum_HasExpectedValues() { - Enum.GetValues().Length.ShouldBe(12); + Enum.GetValues().Length.ShouldBe(17); } } \ No newline at end of file diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index a604fb08..a0b89bb9 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -384,10 +384,12 @@ public class OpcUaClientServiceTests : IDisposable } /// - /// Verifies that object nodes trigger child-detection checks so the client can mark expandable branches. + /// R08 3.6: browsing a level issues ONE Browse (plus BrowseNext pages) however many Object children it holds. Child + /// detection is lazy: an Object may have children and is confirmed when it is expanded/recursed, instead of one + /// extra Browse per Object child (~50 000 calls for a recursive subscribe over a 50k-node raw tree). /// [Fact] - public async Task BrowseAsync_ObjectNode_ChecksHasChildren() + public async Task BrowseAsync_ObjectChildren_MayHaveChildren_WithoutAPerChildBrowse() { var session = new FakeSessionAdapter { @@ -398,17 +400,29 @@ public class OpcUaClientServiceTests : IDisposable NodeId = new ExpandedNodeId("ns=2;s=Folder1"), DisplayName = new LocalizedText("Folder1"), NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Folder2"), + DisplayName = new LocalizedText("Folder2"), + NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Tag"), + DisplayName = new LocalizedText("Tag"), + NodeClass = NodeClass.Variable } - ], - HasChildrenResponse = true + ] }; _sessionFactory.EnqueueSession(session); await _service.ConnectAsync(ValidSettings()); var results = await _service.BrowseAsync(); - results[0].HasChildren.ShouldBeTrue(); - session.HasChildrenCount.ShouldBe(1); + results.Select(r => r.HasChildren).ShouldBe([true, true, false]); + session.BrowseCount.ShouldBe(1); + session.BrowseNextCount.ShouldBe(0); } /// @@ -1432,6 +1446,97 @@ public class OpcUaClientServiceTests : IDisposable _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); } + /// + /// R08 4.5: a ConnectAsync on an already-connected client is refused instead of overwriting (and leaking) the live + /// session. The positive control: after a disconnect the same client connects again. + /// + [Fact] + public async Task ConnectAsync_WhenAlreadyConnected_IsRefused_AndKeepsTheLiveSession() + { + var session1 = new FakeSessionAdapter(); + _sessionFactory.EnqueueSession(session1); + await _service.ConnectAsync(ValidSettings()); + + await Should.ThrowAsync(() => _service.ConnectAsync(ValidSettings())); + + _sessionFactory.CreateCallCount.ShouldBe(1); + session1.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(ValidSettings()); + _sessionFactory.CreateCallCount.ShouldBe(2); + _service.IsConnected.ShouldBeTrue(); + } + + /// + /// R08 4.5: a SubscribeAsync in flight when a keep-alive failover starts must not be lost. Failover runs under the + /// subscription gate, so it waits for the in-flight subscribe to record its item and then replays it onto the new + /// session. Before the fix, failover nulled the subscription state outside the lock and finished first; the subscribe + /// then completed onto the dying session's subscription and was never replayed. + /// + [Fact] + public async Task SubscribeAsync_in_flight_during_failover_is_replayed_onto_the_new_session() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + session1.CreateSubscriptionGate = gate; + var subscribe = _service.SubscribeAsync(new NodeId("ns=2;s=Racer"), 1000); + await session1.CreateSubscriptionEntered.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + gate.SetResult(); + await subscribe.WaitAsync(TimeSpan.FromSeconds(10)); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); + var replayed = session2.CreatedSubscriptions.ShouldHaveSingleItem(); + replayed.AddDataChangeCount.ShouldBe(1); + + // And it is the live subscription: unsubscribing removes it from session2's subscription, not session1's. + await _service.UnsubscribeAsync(new NodeId("ns=2;s=Racer")); + replayed.RemoveCount.ShouldBe(1); + replayed.ActiveHandles.ShouldBeEmpty(); + } + + /// + /// A DisconnectAsync issued while a failover is still connecting wins: the failover disposes the session it just + /// opened instead of resurrecting the connection. The positive control is the plain failover test above, where the + /// same gated attempt ends Connected. + /// + [Fact] + public async Task DisconnectAsync_during_failover_is_not_undone_by_the_failover() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + _sessionFactory.CreateGate = gate; + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + await _service.DisconnectAsync(); + gate.SetResult(); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.IsConnected.ShouldBeFalse(); + session2.Disposed.ShouldBeTrue(); + } + /// /// Regression for Client.Shared-005: concurrent subscribe/unsubscribe calls mutating the /// active-subscription bookkeeping must not corrupt the dictionary or throw. diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj index 903b39f9..e588e7af 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj @@ -21,6 +21,8 @@ + + diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs index e056f78b..a333e699 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs @@ -141,7 +141,16 @@ public class HistoryViewModelTests public void AggregateTypes_ContainsNullForRaw() { _vm.AggregateTypes.ShouldContain((AggregateType?)null); - _vm.AggregateTypes.Count.ShouldBe(13); // null + 12 enum values + _vm.AggregateTypes.Count.ShouldBe(Enum.GetValues().Length + 1); // null + every aggregate + } + + /// The picker offers every client aggregate (the served set), including Range/Delta/PercentGood/... + [Fact] + public void AggregateTypes_offers_every_aggregate() + { + _vm.AggregateTypes.Where(a => a != null).Select(a => a!.Value) + .ShouldBe(Enum.GetValues(), ignoreOrder: true); + _vm.AggregateTypes.ShouldContain(AggregateType.PercentGood); } /// Verifies that the Clear method resets the view model state. diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs index 8c4fd3f5..44ca937a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs @@ -111,6 +111,70 @@ public class SubscriptionsViewModelTests _vm.ActiveSubscriptions[0].Value.ShouldBeNull(); } + /// + /// R08 3.6: notifications are routed through a NodeId index, not a scan of every row. The index follows every + /// mutation of (add, remove, replace, clear). + /// + [Fact] + public void NodeId_index_follows_every_collection_mutation() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + var c = new SubscriptionItemViewModel("ns=2;s=C", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Add(c); + _vm.IndexedNodeCount.ShouldBe(3); + + _vm.ActiveSubscriptions.Remove(b); + _vm.IndexedNodeCount.ShouldBe(2); + + _vm.ActiveSubscriptions[0] = new SubscriptionItemViewModel("ns=2;s=D", 1000); // replaces A + Raise("ns=2;s=A", 1); + Raise("ns=2;s=D", 2); + a.Value.ShouldBeNull(); + _vm.ActiveSubscriptions[0].Value.ShouldBe("2"); + + _vm.ActiveSubscriptions.Clear(); + _vm.IndexedNodeCount.ShouldBe(0); + } + + /// A removed row stops receiving values while its sibling (the positive control) still does. + [Fact] + public void DataChanged_after_remove_updates_only_the_remaining_rows() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Remove(a); + + Raise("ns=2;s=A", 7); + Raise("ns=2;s=B", 42); + + a.Value.ShouldBeNull(); + b.Value.ShouldBe("42"); + } + + /// Two rows for the same node (no dedup on the manual-add path) both update, as before. + [Fact] + public void DataChanged_updates_every_row_for_the_node() + { + var first = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var second = new SubscriptionItemViewModel("ns=2;s=A", 500); + _vm.ActiveSubscriptions.Add(first); + _vm.ActiveSubscriptions.Add(second); + + Raise("ns=2;s=A", 5); + + first.Value.ShouldBe("5"); + second.Value.ShouldBe("5"); + } + + private void Raise(string nodeId, int value) => + _service.RaiseDataChanged(new DataChangedEventArgs(nodeId, + new DataValue(new Variant(value), StatusCodes.Good, DateTime.UtcNow))); + /// Verifies that Clear removes all subscriptions. [Fact] public void Clear_RemovesAllSubscriptions() diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverArrayWriteSupportTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverArrayWriteSupportTests.cs index 18137306..8e6c355e 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverArrayWriteSupportTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/DriverArrayWriteSupportTests.cs @@ -15,12 +15,27 @@ public sealed class DriverArrayWriteSupportTests [Theory] [InlineData("{\"region\":\"Coils\",\"address\":0,\"dataType\":\"Bool\"" + Arr2 + "}", true)] - [InlineData("{\"region\":\"Coils\",\"address\":0" + Arr2 + "}", true)] // FC15 ignores dataType + [InlineData("{\"region\":\"Coils\",\"address\":0" + Arr2 + "}", true)] // absent dataType ⇒ Bool + [InlineData("{\"region\":\"Coils\",\"address\":0,\"dataType\":\"Int16\"" + Arr2 + "}", true)] // a bit region COERCES any type to Bool + [InlineData("{\"region\":\"Coils\",\"address\":0,\"dataType\":\"Int17\"" + Arr2 + "}", false)] // ...but a typo refuses the tag [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"Float32\"" + Arr2 + "}", true)] [InlineData("{\"region\":\"holdingregisters\",\"address\":0,\"dataType\":\"bcd16\"" + Arr2 + "}", true)] // case-insensitive, like the driver [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0" + Arr2 + "}", true)] // dataType ⇒ Int16 - [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"BitInRegister\"" + Arr2 + "}", false)] - [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"String\"" + Arr2 + "}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"BitInRegister\"" + Arr2 + "}", true)] // #741 U3 + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"BitInRegister\",\"bitIndex\":15" + Arr2 + "}", true)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"BitInRegister\",\"bitIndex\":16" + Arr2 + "}", false)] // factory refuses + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"String\",\"stringLength\":4" + Arr2 + "}", true)] // #741 U3 + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"String\"" + Arr2 + "}", false)] // no stringLength ⇒ refused + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"String\",\"stringLength\":0" + Arr2 + "}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"Int16\",\"byteOrder\":\"Sideways\"" + Arr2 + "}", false)] // strict enum + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"String\",\"stringLength\":4,\"stringByteOrder\":\"Middle\"" + Arr2 + "}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65536,\"dataType\":\"Int16\"" + Arr2 + "}", false)] // not a register address + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":1.5,\"dataType\":\"Int16\"" + Arr2 + "}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65535,\"dataType\":\"BitInRegister\",\"isArray\":true,\"arrayLength\":1}", true)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65535,\"dataType\":\"BitInRegister\"" + Arr2 + "}", false)] // span runs past 65535 + [InlineData("{\"addressString\":\"40001:S::2\"" + Arr2 + "}", false)] // string-only: family unknown at compose time + [InlineData("{\"region\":\"HoldingRegisters\",\"addressString\":\"40001:S::2\"" + Arr2 + "}", false)] // region alone is not the keys + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"addressString\":\"30001\",\"dataType\":\"Int16\"" + Arr2 + "}", true)] // keys win [InlineData("{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"Bool\"" + Arr2 + "}", false)] [InlineData("{\"region\":\"InputRegisters\",\"address\":0,\"dataType\":\"Int16\"" + Arr2 + "}", false)] // read-only region [InlineData("{\"region\":\"DiscreteInputs\",\"address\":0" + Arr2 + "}", false)] @@ -59,11 +74,50 @@ public sealed class DriverArrayWriteSupportTests DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tag, "{\"maxRegistersPerWrite\":100}", "{\"maxRegistersPerWrite\":9}") .ShouldBeFalse(); DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tag, "{\"maxRegistersPerWrite\":0}", null, "not json").ShouldBeTrue(); - // A larger configured cap lifts the default: 124 registers fit under 200. - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", RegisterArray("Int16", 124), "{\"maxRegistersPerWrite\":200}") + // A configured cap above the FC16 protocol ceiling (123) never lifts it: a conformant device refuses 124 + // registers (and past 127 the driver's one-byte byte-count field wraps), so the write always fails. + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", RegisterArray("Int16", 123), "{\"maxRegistersPerWrite\":200}") .ShouldBeTrue(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", RegisterArray("Int16", 124), "{\"maxRegistersPerWrite\":200}") + .ShouldBeFalse(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", RegisterArray("BitInRegister", 124), "{\"maxRegistersPerWrite\":200}") + .ShouldBeFalse(); } + /// A BitInRegister[] write is a read-modify-write: one unchunked FC03 of the whole span first, so its + /// length is also bounded by maxRegistersPerRead (smallest positive value across the configs; absent / 0 => + /// 125, and never above FC03's protocol ceiling of 125). The other register shapes never read on write. + [Fact] + public void Modbus_bit_in_register_arrays_respect_the_read_cap() + { + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 64), "{\"maxRegistersPerRead\":64}").ShouldBeTrue(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 65), "{\"maxRegistersPerRead\":64}").ShouldBeFalse(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 65), "{\"maxRegistersPerRead\":100}", "{\"MaxRegistersPerRead\":64}") + .ShouldBeFalse(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 123), "{\"maxRegistersPerRead\":0}").ShouldBeTrue(); + // A numeric array of the same length is not read on write, so the read cap does not apply to it. + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", RegisterArray("Int16", 65), "{\"maxRegistersPerRead\":64}").ShouldBeTrue(); + // With the write cap lifted, the read cap is the bound. + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 100), + "{\"maxRegistersPerRead\":100,\"maxRegistersPerWrite\":200}").ShouldBeTrue(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", RegisterArray("BitInRegister", 101), + "{\"maxRegistersPerRead\":100,\"maxRegistersPerWrite\":200}").ShouldBeFalse(); + } + + /// Every shape's span must end at or before address 65535 - a conformant device refuses a write past it + /// (exception 02). + [Theory] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65530,\"dataType\":\"Int16\",\"isArray\":true,\"arrayLength\":6}", true)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65530,\"dataType\":\"Int16\",\"isArray\":true,\"arrayLength\":7}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65532,\"dataType\":\"Int32\",\"isArray\":true,\"arrayLength\":2}", true)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65533,\"dataType\":\"Int32\",\"isArray\":true,\"arrayLength\":2}", false)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65534,\"dataType\":\"String\",\"stringLength\":4,\"isArray\":true,\"arrayLength\":1}", true)] + [InlineData("{\"region\":\"HoldingRegisters\",\"address\":65535,\"dataType\":\"String\",\"stringLength\":4,\"isArray\":true,\"arrayLength\":1}", false)] + [InlineData("{\"region\":\"Coils\",\"address\":65000,\"isArray\":true,\"arrayLength\":536}", true)] + [InlineData("{\"region\":\"Coils\",\"address\":65000,\"isArray\":true,\"arrayLength\":537}", false)] + public void Modbus_array_spans_end_within_the_address_space(string tagConfig, bool expected) => + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tagConfig).ShouldBe(expected); + /// FC15 carries at most 1968 coils and the driver sends one request, so a longer coil array is not /// writable. [Fact] @@ -75,6 +129,40 @@ public sealed class DriverArrayWriteSupportTests "{\"region\":\"Coils\",\"address\":0,\"isArray\":true,\"arrayLength\":1969}").ShouldBeFalse(); } + /// #741 U3 — String[] and BitInRegister[] arrays ride the same FC16 cap: (stringLength+1)/2 registers + /// per String element, one per BitInRegister element. + [Theory] + [InlineData("\"dataType\":\"String\",\"stringLength\":4", 61, true)] // 122 registers + [InlineData("\"dataType\":\"String\",\"stringLength\":4", 62, false)] // 124 + [InlineData("\"dataType\":\"String\",\"stringLength\":5", 41, true)] // 3 registers each: 123 + [InlineData("\"dataType\":\"String\",\"stringLength\":5", 42, false)] // 126 + [InlineData("\"dataType\":\"BitInRegister\"", 123, true)] + [InlineData("\"dataType\":\"BitInRegister\"", 124, false)] + public void Modbus_string_and_bit_arrays_respect_the_write_cap(string typeKeys, int length, bool expected) => + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", + $"{{\"region\":\"HoldingRegisters\",\"address\":0,{typeKeys},\"isArray\":true,\"arrayLength\":{length}}}").ShouldBe(expected); + + /// Elements CONVERT (ScadaBridge's int[] to an Int16 node), so the Modbus row keys on no exact node + /// DataType; it leaves out only a String or DateTime node over a numeric / bit element (a DateTime never converts; a String converts only when its text happens to + /// parse, which the server cannot promise). A + /// String element takes any node type (each element is Convert.ToString'd). + [Theory] + [InlineData("Int16", "Int16", true)] + [InlineData("Int16", "Int32", true)] + [InlineData("Int16", "Double", true)] + [InlineData("Int16", "Boolean", true)] + [InlineData("Int16", "String", false)] + [InlineData("Int16", "DateTime", false)] + [InlineData("BitInRegister", "Boolean", true)] + [InlineData("BitInRegister", "String", false)] + [InlineData("String", "String", true)] + [InlineData("String", "Int16", true)] + [InlineData("String", "DateTime", true)] + [InlineData("String", null, true)] + public void Modbus_rows_leave_out_only_node_types_that_never_convert(string element, string? nodeType, bool expected) => + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, nodeType, + $"{{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"{element}\",\"stringLength\":4" + Arr2 + "}").ShouldBe(expected); + private static string RegisterArray(string dataType, int length) => $"{{\"region\":\"HoldingRegisters\",\"address\":0,\"dataType\":\"{dataType}\",\"isArray\":true,\"arrayLength\":{length}}}"; diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs new file mode 100644 index 00000000..cad52821 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs @@ -0,0 +1,50 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests; + +/// +/// Review 07 U7: the AbCip probe/read/write commands had no arm, so a +/// Ctrl-C printed a stack trace. A command cancelled by the operator now exits cleanly with Cancelled.. +/// The gateway is TEST-NET (never answers), so nothing is sent to a real device. +/// +[Trait("Category", "Unit")] +public sealed class CancellationHandlingTests +{ + private const string Blackhole = "ab://192.0.2.1/1,0"; + + private static async Task RunCancelledAsync(AbCipCommandBase command) + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + await Should.NotThrowAsync(async () => await command.ExecuteAsync(console)); + return console.ReadErrorString(); + } + + /// A cancelled read prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_read_prints_Cancelled() + { + (await RunCancelledAsync(new ReadCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled probe prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_probe_prints_Cancelled() + { + (await RunCancelledAsync(new ProbeCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled write prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_write_prints_Cancelled() + { + (await RunCancelledAsync(new WriteCommand { Gateway = Blackhole, TagPath = "Motor01", Value = "1" })) + .ShouldContain("Cancelled."); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs new file mode 100644 index 00000000..ecdf93f5 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs @@ -0,0 +1,111 @@ +using System.Text; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests; + +/// +/// Review 07 U7, shared half: arrays render their elements (not System.Int32[]), Ctrl-C prints +/// Cancelled., and the subscribe change-line writer is serialised and never throws into the poll loop. +/// +[Trait("Category", "Unit")] +public sealed class CliHelpersU7Tests +{ + private sealed record Handle(string DiagnosticId) : ISubscriptionHandle; + + private static DataChangeEventArgs Change(string reference, object? value) => + new(new Handle("h"), reference, new DataValueSnapshot(value, 0u, DateTime.UtcNow, DateTime.UtcNow)); + + /// An array value renders as its elements, each formatted like a scalar. + [Fact] + public void FormatValue_renders_array_elements() + { + SnapshotFormatter.FormatValue(new[] { 1, 2, 3 }).ShouldBe("[1, 2, 3]"); + SnapshotFormatter.FormatValue(new[] { 1.5f, -2f }).ShouldBe("[1.5, -2]"); + SnapshotFormatter.FormatValue(new[] { true, false }).ShouldBe("[true, false]"); + SnapshotFormatter.FormatValue(new[] { "a", "b" }).ShouldBe("[\"a\", \"b\"]"); + SnapshotFormatter.FormatValue(new List { 7 }).ShouldBe("[7]"); + SnapshotFormatter.FormatValue(Array.Empty()).ShouldBe("[]"); + } + + /// A string is still a scalar, not a sequence of chars (positive control for the array arm). + [Fact] + public void FormatValue_keeps_a_string_scalar() + { + SnapshotFormatter.FormatValue("abc").ShouldBe("\"abc\""); + } + + /// Operator Ctrl-C prints Cancelled. instead of a stack trace. + [Fact] + public async Task RunCancellableAsync_operator_cancellation_prints_Cancelled() + { + using var console = new FakeInMemoryConsole(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Should.NotThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, cts.Token, () => Task.FromCanceled(cts.Token))); + + console.ReadErrorString().ShouldContain("Cancelled."); + } + + /// A cancellation the operator did not request still propagates. + [Fact] + public async Task RunCancellableAsync_unrequested_cancellation_propagates() + { + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, CancellationToken.None, + () => throw new OperationCanceledException())); + } + + /// A change line carries the reference, the formatted value and the status. + [Fact] + public void ChangeLineWriter_writes_one_formatted_line_per_change() + { + var output = new StringWriter(); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + write(this, Change("Tag:DInt", new[] { 4, 5 })); + + var line = output.ToString().TrimEnd(); + line.ShouldContain("Tag:DInt = [4, 5] (0x00000000 (Good))"); + } + + /// Concurrent poll-thread changes produce whole lines, never interleaved fragments. + [Fact] + public async Task ChangeLineWriter_serialises_concurrent_writes() + { + var output = new StringWriter(new StringBuilder()); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(t => Task.Run(() => + { + for (var i = 0; i < 200; i++) write(this, Change($"T{t}", i)); + }))); + + var lines = output.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + lines.Length.ShouldBe(1600); + lines.ShouldAllBe(l => l.StartsWith('[') && l.EndsWith("(Good))")); + } + + /// A failing console (closed pipe) is swallowed so it cannot fault the driver's poll loop. + [Fact] + public void ChangeLineWriter_swallows_a_console_write_failure() + { + var write = DriverCommandBase.CreateChangeLineWriter(new ThrowingWriter(), new object()); + + Should.NotThrow(() => write(this, Change("T", 1))); + } + + private sealed class ThrowingWriter : TextWriter + { + public override Encoding Encoding => Encoding.UTF8; + public override void Write(char value) => throw new IOException("pipe closed"); + public override void WriteLine(string? value) => throw new IOException("pipe closed"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs index 905585bc..76ee84fb 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs @@ -21,10 +21,15 @@ public sealed class AbStatusCodesTests [InlineData((byte)0x0E, AbStatus.NotAllowed)] [InlineData((byte)0x1E, AbStatus.BadReply)] [InlineData((byte)0x01, AbStatus.BadReply)] + [InlineData((byte)0x07, AbStatus.BadReply)] // connection lost: opaque on purpose (AbLegacy #738) + // Lane K F1 (#737): the controller answered and refused this request — not a garbled reply. + [InlineData((byte)0x1C, AbStatus.Unsupported)] // missing attribute list entry data + [InlineData((byte)0x20, AbStatus.Unsupported)] // invalid parameter public void Cip_general_status_maps(byte general, AbStatus expected) => AbStatusCodes.FromCip(general, ReadOnlySpan.Empty).ShouldBe(expected); [Theory] + [InlineData((ushort)0x2104, AbStatus.OutOfBounds)] // offset beyond end of the tag (Lane K F1) [InlineData((ushort)0x2105, AbStatus.OutOfBounds)] [InlineData((ushort)0x2107, AbStatus.TypeMismatch)] [InlineData((ushort)0x9999, AbStatus.BadReply)] @@ -37,16 +42,42 @@ public sealed class AbStatusCodesTests [Theory] [InlineData((byte)0x00, null, AbStatus.Ok)] - [InlineData((byte)0x10, null, AbStatus.Unsupported)] - [InlineData((byte)0x20, null, AbStatus.NotFound)] - [InlineData((byte)0x30, null, AbStatus.NotAllowed)] - [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] - [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] - [InlineData((byte)0xF0, (byte)0x07, AbStatus.NotFound)] + [InlineData((byte)0x10, null, AbStatus.Unsupported)] // illegal command or format + // Lane K F3 (AbLegacy F8, review 07 r0 I4), per the DF1/PCCC STS table (1770-6.5.16): + [InlineData((byte)0x20, null, AbStatus.BadConnection)] // host has a problem and will not communicate + [InlineData((byte)0x30, null, AbStatus.BadConnection)] // remote node host missing / disconnected / shut down + [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] // host hardware fault (the host answered) + [InlineData((byte)0x50, null, AbStatus.NotFound)] // addressing problem or memory protect rungs + [InlineData((byte)0x60, null, AbStatus.NotAllowed)] // function not allowed due to command protection + [InlineData((byte)0x70, null, AbStatus.DeviceStateConflict)] // processor is in Program mode + [InlineData((byte)0x80, null, AbStatus.BadReply)] // compatibility-mode file missing: unclassified + [InlineData((byte)0x90, null, AbStatus.BadConnection)] // remote node cannot buffer command + [InlineData((byte)0xB0, null, AbStatus.DeviceStateConflict)] // remote node problem due to download + [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] // address does not point to something usable + [InlineData((byte)0xF0, (byte)0x07, AbStatus.OutOfBounds)] // file is wrong size [InlineData((byte)0xF0, (byte)0x0A, AbStatus.BadReply)] + [InlineData((byte)0xF0, null, AbStatus.BadReply)] public void Pccc_status_maps(byte sts, byte? ext, AbStatus expected) => AbStatusCodes.FromPccc(sts, ext).ShouldBe(expected); + /// + /// A dead DH+/DH-485 node behind a bridge answers STS 0x30 (and a busy one 0x90). Neither proves the END + /// device answered, so they are the no-reply comms class, never one of the "answered" refusals — that + /// classification made such a device count as reachable forever (no backoff, no comms-loss watchdog). + /// + /// The PCCC STS byte. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + [InlineData((byte)0x90)] + public void Pccc_no_reply_statuses_are_never_answered_refusals(byte sts) + { + var status = AbStatusCodes.FromPccc(sts, null); + status.ShouldBe(AbStatus.BadConnection); + status.ShouldNotBeOneOf(AbStatus.NotFound, AbStatus.NotAllowed, AbStatus.OutOfBounds, + AbStatus.DeviceStateConflict, AbStatus.TooLarge, AbStatus.TooSmall, AbStatus.TypeMismatch); + } + [Fact] public void Ok_is_zero_and_every_error_is_negative() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs index 0d4c2003..ec362206 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs @@ -115,6 +115,30 @@ public sealed class ConnectionManagerServicesTests ex.Status.ShouldBe(AbStatus.BadConnection); // general 0x01 has no specific mapping } + /// + /// Lane K F1 remapped general 0x1C / 0x20 and 0xFF/0x2104 for TAG requests (a per-request refusal). A Forward + /// Open refused with one of them is still a connection failure, exactly as before: the whole connection is + /// unusable, so it must not read as a per-tag refusal. + /// + /// The refused Forward Open reply. + [Theory] + [InlineData("DB001C00")] + [InlineData("DB002000")] + [InlineData("DB00FF010421")] + public void Forward_open_refused_with_a_request_level_status_is_still_BadConnection(string hex) + { + var ex = Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString(hex)))); + ex.Status.ShouldBe(AbStatus.BadConnection); + } + + /// Positive control: a Forward Open refusal that was already specifically mapped keeps its status. + [Fact] + public void Forward_open_refused_service_not_supported_stays_Unsupported() => + Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString("DB000800")))) + .Status.ShouldBe(AbStatus.Unsupported); + [Fact] public void Refused_forward_open_exception_is_not_aliased_to_the_reply_array() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs index b8d36f68..fbac7f56 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs @@ -90,6 +90,32 @@ public sealed class PcccServicesTests [Fact] public void Plc5_write_is_not_shipped_and_flagged() => PcccServices.Plc5WriteVerified.ShouldBeFalse(); + // ---- ParseReply: golden STS replies (Lane K F3 / AbLegacy F8) ---- + + /// + /// Wire-level golden replies: the Execute PCCC reply bytes a device (or a bridge in front of it) sends for each + /// STS, through , to the status the drivers classify. + /// + /// The reply's STS (+ EXT STS) bytes as hex. + /// The status ParseReply raises. + [Theory] + [InlineData("20", AbStatus.BadConnection)] + [InlineData("30", AbStatus.BadConnection)] + [InlineData("90", AbStatus.BadConnection)] + [InlineData("50", AbStatus.NotFound)] + [InlineData("60", AbStatus.NotAllowed)] + [InlineData("70", AbStatus.DeviceStateConflict)] + [InlineData("B0", AbStatus.DeviceStateConflict)] + [InlineData("F0|06", AbStatus.NotFound)] + [InlineData("F0|07", AbStatus.OutOfBounds)] + public void ParseReply_golden_sts(string stsHex, AbStatus expected) + { + var parts = stsHex.Split('|'); + var hex = Id + "4F" + parts[0] + "9987" + (parts.Length > 1 ? parts[1] : ""); + Should.Throw(() => PcccServices.ParseReply(Reply(hex), 0x8799)) + .Status.ShouldBe(expected); + } + // ---- ParseReply ---- private static CipReply Reply(string hex, byte general = 0) => new(0x4B, general, [], Hex(hex)); @@ -122,7 +148,23 @@ public sealed class PcccServicesTests [Fact] public void ParseReply_cip_error_status_throws() => - Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + + /// Review I2: a status the device REPLIED with is marked as such (envelope CIP status and PCCC STS). + [Fact] + public void ParseReply_marks_device_statuses_as_device_replies() + { + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x20), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply(Id + "4F10 9987"), 0x8799)).Status.ShouldBe(AbStatus.Unsupported); + } + + /// Positive control: a refusal raised by a builder before anything is sent is NOT a device reply. + [Fact] + public void A_pre_wire_builder_refusal_is_not_a_device_reply() + { + var ex = Should.Throw(() => PcccServices.SlcTypedRead(1, A("N7:0"), 999)); + ex.ShouldNotBeOfType(); + } [Fact] public void ParseReply_wrong_command_byte_is_BadReply() => diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs index d196d294..cf4a1965 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs @@ -25,3 +25,60 @@ public sealed class CipWriterTests w.ToArray().ShouldBe(new byte[] { 0, 0, 7 }); // nothing was patched } } + +/// +/// R07:L5: works in a buffer rented from and hands +/// it back on . The returned array must be the caller's own (a later writer reusing +/// the pooled buffer cannot change it), growth must keep every byte, and a writer used after ToArray must refuse +/// rather than write into a buffer it no longer owns. +/// +public sealed class CipWriterPoolingTests +{ + [Fact] + public void Growth_past_the_initial_buffer_keeps_every_byte() + { + var w = new CipWriter(); + for (var i = 0; i < 1000; i++) w.U8((byte)i); + var bytes = w.ToArray(); + bytes.Length.ShouldBe(1000); + bytes.Select((b, i) => b == (byte)i).ShouldAllBe(ok => ok); + } + + [Fact] + public void A_returned_array_is_not_affected_by_a_later_writer_reusing_the_pooled_buffer() + { + var first = new CipWriter().U32(0xAABBCCDD).ToArray(); + for (var i = 0; i < 50; i++) new CipWriter().U32(0x11111111).U32(0x22222222).ToArray(); + first.ShouldBe(new byte[] { 0xDD, 0xCC, 0xBB, 0xAA }); + } + + [Fact] + public void ToArray_twice_returns_equal_independent_copies() + { + var w = new CipWriter().U16(0x0102); + var a = w.ToArray(); + var b = w.ToArray(); + b.ShouldBe(a); + b.ShouldNotBeSameAs(a); + } + + [Fact] + public void Writing_after_ToArray_is_refused() + { + var w = new CipWriter().U8(1); + _ = w.ToArray(); + Should.Throw(() => w.U8(2)); + Should.Throw(() => w.PatchU16(0, 1)); + w.Length.ShouldBe(1); + } + + [Fact] + public void Nested_writers_produce_independent_frames() + { + // The Execute PCCC / encapsulation builders build an inner writer while an outer one is live. + var outer = new CipWriter().U8(0xAA); + var inner = new CipWriter().U8(0x01).U8(0x02).ToArray(); + var frame = outer.Bytes(inner).U8(0xBB).ToArray(); + frame.ShouldBe(new byte[] { 0xAA, 0x01, 0x02, 0xBB }); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 028d6e27..03434b0a 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -297,6 +297,11 @@ public sealed class NativeAbCipTagRuntimeTests [InlineData("D2000A00", AbStatus.OutOfBounds)] [InlineData("D2000F00", AbStatus.NotAllowed)] [InlineData("D200FF010521", AbStatus.OutOfBounds)] + // Lane K F1 (#737): these used to arrive as BadReply, i.e. a comms fault (Degraded + evict) for a tag the + // controller had simply refused. AbCipRefusals.IsDeviceRefusal classifies the new statuses with no driver change. + [InlineData("D2001C00", AbStatus.Unsupported)] // missing attribute list entry data + [InlineData("D2002000", AbStatus.Unsupported)] // invalid parameter + [InlineData("D200FF010421", AbStatus.OutOfBounds)] // offset beyond end of the tag public async Task A_refused_tag_completes_initialize_and_reports_the_refusal_on_read(string reply, AbStatus expected) { var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString(reply)) }; 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 8d5185fe..845a3a07 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 @@ -220,14 +220,30 @@ public sealed class NativeAbLegacyTagRuntimeTests } [Fact] - public async Task STS_0x20_on_initialize_is_NotFound() + public async Task STS_0x50_on_initialize_is_NotFound() { - await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x20, null, [])); + await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x50, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotFound); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotFound); } + /// + /// Lane K F3 (F8): STS 0x20 / 0x30 come from the host or a bridge when the END device cannot be reached, so they + /// are the no-reply class (BadConnection), not an answered refusal. + /// + /// The PCCC STS. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + public async Task STS_from_a_bridge_for_an_unreachable_node_is_BadConnection(byte sts) + { + await using var h = new Harness("slc500", "N7:0").Responder((_, _) => (sts, null, [])); + var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); + ex.Status.ShouldBe(AbStatus.BadConnection); + h.Runtime.GetStatus().ShouldBe((int)AbStatus.BadConnection); + } + [Fact] public async Task Initialize_performs_exactly_one_read() { @@ -305,7 +321,7 @@ public sealed class NativeAbLegacyTagRuntimeTests public async Task A_failed_second_write_chunk_is_reported_as_failed_not_Good() { await using var h = new Harness("slc500", "N7:0", count: 130).Responder((_, n) => - n == 1 ? (0x30, null, []) : (0x00, null, [])); + n == 1 ? (0x60, null, []) : (0x00, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.WriteAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotAllowed); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotAllowed); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs new file mode 100644 index 00000000..58eecee7 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs @@ -0,0 +1,131 @@ +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.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Lane K F3 (AbLegacy F8, review 07 r0 I4), end to end over a real socket with the PRODUCTION +/// : a bridge (DHRIO / 1761-NET-ENI) in front of a dead DH+ node answers every +/// PCCC request with STS 0x30 "remote node host missing". That is NOT the device answering, so the driver must treat +/// it as a comms fault — Degraded, LastSuccessfulRead not advanced — or the comms-loss watchdog never fires +/// for it. The positive control is a live device refusing a tag (STS 0x60, command protection): per-tag Bad, Healthy, +/// LastSuccessfulRead advanced. +/// +[Trait("Category", "Unit")] +public sealed class NativePcccBridgeStatusHealthTests +{ + private static readonly byte[] FixedId = [0x07, 0x3D, 0xF3, 0x45, 0x43, 0x50, 0x21]; + + /// A fake whose Execute PCCC envelope itself is refused with CIP general status . + private static FakeEipServer CipEnvelopeServer(byte general) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => FakeEipServer.Command(req) == Ab.Transport.Eip.EncapCommand.SendRRData + ? FakeEipServer.RRReply(req, [0xCB, 0x00, general, 0x00]) + : null; + return server; + } + + /// A PCCC fake that answers every request with one fixed STS. + private static FakeEipServer StsServer(byte sts) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => + { + if (FakeEipServer.Command(req) != Ab.Transport.Eip.EncapCommand.SendRRData) return null; + var cip = FakeEipServer.RequestCip(req); + byte[] head = [0xCB, 0x00, 0x00, 0x00, .. FixedId, 0x4F]; + return FakeEipServer.RRReply(req, [.. head, sts, cip[15], cip[16]]); + }; + return server; + } + +#pragma warning disable OTOPCUA0001 // the driver is called directly to pin its OWN classification (no resilience wrap) + private static async Task DriverAsync(FakeEipServer server) + { + var host = $"ab://127.0.0.1:{server.Port}/1,0"; + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("T", host, "N7:0", AbLegacyDataType.Int)]), + Timeout = TimeSpan.FromSeconds(5), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-pccc-bridge", new NativeAbLegacyTagFactory(new DeviceSessionPool())); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + return driver; + } + + /// STS 0x30 from a bridge: a comms fault, never evidence the device answered. + [Fact] + public async Task A_dead_node_behind_a_bridge_degrades_and_does_not_advance_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x30); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadCommunicationError); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Degraded); + health.LastSuccessfulRead.ShouldBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// Positive control: STS 0x60 is the live device refusing the tag — answered, so Healthy and advanced. + [Fact] + public async Task A_live_device_refusing_the_tag_stays_Healthy_and_advances_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x60); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbOpcUaStatusMap.BadNotWritable); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// + /// Review I2: a refusal that came OFF THE WIRE — the device (or the bridge's Execute PCCC object) answered with + /// CIP 0x08 / 0x1C / 0x20 on the envelope, or PCCC STS 0x10 — is an answered refusal: per-tag BadNotSupported, + /// Healthy, LastSuccessfulRead advanced. It must not land in the "refused before the wire" neutral class + /// (which advances nothing). The local-refusal counterpart stays neutral (NativeAbLegacyRefusedWriteTests). + /// + /// The CIP general status of the envelope reply, or 0 for a PCCC STS 0x10 reply. + [Theory] + [InlineData((byte)0x08)] + [InlineData((byte)0x1C)] + [InlineData((byte)0x20)] + [InlineData((byte)0x00)] + public async Task A_wire_Unsupported_reply_is_an_answered_refusal(byte general) + { + var ct = TestContext.Current.CancellationToken; + await using var server = general == 0 ? StsServer(0x10) : CipEnvelopeServer(general); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } +#pragma warning restore OTOPCUA0001 +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs index 41a20e50..56af4ad4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs @@ -5,131 +5,432 @@ using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; /// -/// Gitea #610 — pins 's Modbus row against the REAL write path. For every -/// region × element type, the server advertises an array node writable exactly when the driver accepts a -/// whole-array write for it. A driver change that gains or loses an array encoder fails here until the table -/// follows — so the server can never advertise a write this driver refuses (nor hide one it accepts). +/// Gitea #610 / #741 — pins 's Modbus row against the REAL write path. For +/// every region × element type × node DataType, and for every shape edge (the FC16 / FC15 caps, the declared +/// length, the type-dependent guards, the address keys), the server advertises an array node writable exactly when +/// encodes the whole array onto the wire. A driver change that gains or loses +/// an array encoder fails here until the table follows — so the server can never advertise a write this driver +/// refuses (nor hide one it accepts). /// +/// +/// "Accepted" means Good status AND a write PDU (FC15 / FC16 / FC23) the device took. The transport is a +/// spec-conformant device: it holds the whole 0–65535 address space and refuses what every conformant device +/// refuses — a quantity over a protocol ceiling (FC15 1968 coils, FC16 123 and FC03 125 registers) or a span past +/// address 65535. The driver sends one FC15 / FC16 (and, for BitInRegister[], one FC03) without chunking, so those +/// ceilings are the device's refusal; a configured MaxRegistersPerWrite below 123 is the driver's own. FC23 +/// is opt-in and never changes the answer (pinned below). +/// [Trait("Category", "Unit")] public sealed class ModbusArrayWriteSupportParityTests { - private static IEnumerable<(ModbusRegion Region, ModbusDataType DataType)> Shapes() => - from region in Enum.GetValues() - from dt in Enum.GetValues() - select (region, dt); + private const string Ref = "cell/modbus/arr"; - public static TheoryData AllShapes() + /// The CLR array the node manager hands the driver for a node of this DataType (it converts a numeric + /// client array to the node's element type first). + private static Array ClrArray(string nodeType, int n) => nodeType switch { - var data = new TheoryData(); - foreach (var (region, dt) in Shapes()) data.Add(region, dt); - return data; - } + "Boolean" => Enumerable.Repeat(true, n).ToArray(), + "Int16" => Enumerable.Repeat((short)1, n).ToArray(), + "UInt16" => Enumerable.Repeat((ushort)1, n).ToArray(), + "Int32" => Enumerable.Repeat(1, n).ToArray(), + "Int64" => Enumerable.Repeat(1L, n).ToArray(), + "Float" => Enumerable.Repeat(1f, n).ToArray(), + "Double" => Enumerable.Repeat(1d, n).ToArray(), + // Non-numeric text: a String node over a numeric / bit element converts only when the text happens to parse, + // which is not a write the server can promise — the table keeps it read-only. + "String" => Enumerable.Repeat("a", n).ToArray(), + "DateTime" => Enumerable.Repeat(DateTime.UtcNow, n).ToArray(), + _ => throw new ArgumentOutOfRangeException(nameof(nodeType)), + }; - /// - /// Shapes where the driver has moved ahead of the server-side table (review 07 P6 / U3, Modbus lane) and - /// — owned outside the Modbus lane — has not followed yet - /// (follow-up F1): the tag factory now REFUSES a Coils tag with a non-Bool type (so the driver serves no - /// such tag), and the driver now encodes String[] and BitInRegister[] holding-register writes. - /// fails the moment the table follows, so this list - /// is removed with it rather than left to rot. - /// - private static bool PendingF1(ModbusRegion region, ModbusDataType dataType) => - (region == ModbusRegion.Coils && dataType != ModbusDataType.Bool) - || (region == ModbusRegion.HoldingRegisters && dataType is ModbusDataType.String or ModbusDataType.BitInRegister); + private static readonly string[] NodeTypes = ["Boolean", "Int16", "UInt16", "Int32", "Int64", "Float", "Double", "String", "DateTime"]; - - public static TheoryData PendingShapes() + /// Drives the real driver: true iff the write is Good AND a write PDU reached the device. + private static async Task<(bool Accepted, uint Status)> DriverEncodesAsync( + string tagConfig, object value, ushort maxRegistersPerWrite = 123, bool fc23 = false, ushort maxRegistersPerRead = 125) { - var data = new TheoryData(); - foreach (var (region, dt) in Shapes().Where(s => PendingF1(s.Region, s.DataType))) data.Add(region, dt); - return data; - } - - [Theory] - [MemberData(nameof(PendingShapes))] - public void Pending_F1_divergences_still_exist(ModbusRegion region, ModbusDataType dataType) - { - var json = $$"""{"region":"{{region}}","address":10,"dataType":"{{dataType}}","stringLength":4,"isArray":true,"arrayLength":2}"""; - var driverAccepts = region == ModbusRegion.HoldingRegisters; // String[] / BitInRegister[] encode; mistyped Coils are refused - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json).ShouldNotBe(driverAccepts, - "F1 landed — DriverArrayWriteSupport now agrees with the driver for this shape; delete it from PendingF1"); - } - - [Theory] - [MemberData(nameof(AllShapes))] - public async Task Table_agrees_with_the_driver_write_path(ModbusRegion region, ModbusDataType dataType) - { - if (PendingF1(region, dataType)) return; // covered by Pending_F1_divergences_still_exist - var json = $$"""{"region":"{{region}}","address":10,"dataType":"{{dataType}}","stringLength":4,"isArray":true,"arrayLength":2}"""; - var fake = new ModbusDriverTests.FakeTransport(); + // The operator configures maxRegistersPerRead because the DEVICE reads no more than that per FC03, so the + // modelled device refuses a longer read (never above FC03's protocol ceiling of 125). + var device = new ConformantDevice { MaxReadRegisters = Math.Min(maxRegistersPerRead == 0 ? 125 : (int)maxRegistersPerRead, 125) }; var drv = new ModbusDriver( - new ModbusDriverOptions { Host = "fake", RawTags = [new RawTagEntry("cell/modbus/arr", json, false)] }, - "modbus-array-parity", _ => fake); + new ModbusDriverOptions + { + Host = "fake", RawTags = [new RawTagEntry(Ref, tagConfig, false)], + MaxRegistersPerWrite = maxRegistersPerWrite, UseFC23ForMultiRegisterWrites = fc23, + MaxRegistersPerRead = maxRegistersPerRead, + }, + "modbus-array-parity", _ => device); await drv.InitializeAsync("{}", CancellationToken.None); - - // A two-element value every element type can convert (Convert.ToBoolean / ToInt16 / ToSingle … of 1). - var result = await drv.WriteAsync([new WriteRequest("cell/modbus/arr", new object[] { 1, 1 })], CancellationToken.None); - var driverAccepts = result.Single().StatusCode == 0u; - - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json).ShouldBe( - driverAccepts, $"region={region} dataType={dataType}: driver status 0x{result.Single().StatusCode:X8}"); + var result = await drv.WriteAsync([new WriteRequest(Ref, value)], CancellationToken.None); + var status = result.Single().StatusCode; + return (status == 0u && device.WritePdus > 0, status); } - /// Positive control: the theory above is not vacuously "everything refused". + public static TheoryData TypeMatrix() + { + var data = new TheoryData(); + foreach (var region in Enum.GetValues()) + foreach (var dt in Enum.GetValues()) + foreach (var node in NodeTypes) + data.Add(region, dt, node); + return data; + } + + /// Every region × element type × node DataType: the table answers exactly what the write path does — + /// including String[] and BitInRegister[] holding-register arrays (#741 U3) and Coils authored with a non-Bool type, + /// which the resolver COERCES to Bool (never refuses). + [Theory] + [MemberData(nameof(TypeMatrix))] + public async Task Table_agrees_with_the_driver_write_path(ModbusRegion region, ModbusDataType dataType, string nodeType) + { + var json = $$"""{"region":"{{region}}","address":10,"dataType":"{{dataType}}","stringLength":4,"bitIndex":3,"isArray":true,"arrayLength":2}"""; + + var (accepted, status) = await DriverEncodesAsync(json, ClrArray(nodeType, 2)); + + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, nodeType, json).ShouldBe( + accepted, $"region={region} dataType={dataType} node={nodeType}: driver status 0x{status:X8}"); + } + + /// Positive control: the matrix above is not vacuously one answer, and it contains the shapes #741 added. [Fact] public void The_matrix_contains_both_answers() { - var answers = Shapes() - .Select(s => DriverArrayWriteSupport.Supports( + var answers = Enum.GetValues() + .SelectMany(r => Enum.GetValues().Select(d => DriverArrayWriteSupport.Supports( DriverTypeNames.Modbus, "Int16", - $$"""{"region":"{{s.Region}}","address":10,"dataType":"{{s.DataType}}","isArray":true,"arrayLength":2}""")) + $$"""{"region":"{{r}}","address":10,"dataType":"{{d}}","stringLength":4,"isArray":true,"arrayLength":2}"""))) .ToHashSet(); answers.ShouldBe(new[] { true, false }, ignoreOrder: true); } /// - /// #610 review MINOR-1/-2 — the table must also agree with the driver on the SHAPE edges: the FC16 write cap - /// (the driver refuses rather than chunks an over-cap array; default 123 registers, or the configured - /// MaxRegistersPerWrite) and a missing / zero arrayLength (the factory maps it to a scalar tag, - /// whose scalar encoder refuses an array value). + /// The shape edges, both directions, against the real write path: the FC16 cap for numeric, String and + /// BitInRegister arrays (default 123, or a configured MaxRegistersPerWrite), the FC15 ceiling, a + /// missing / zero arrayLength (a scalar to the factory, whose scalar encoder refuses an array value), + /// the String length and bit-index guards, the BitInRegister span end, and the strict key reads. /// [Theory] - [InlineData("Int32", "\"arrayLength\":61", 0, 61)] // 122 registers — fits the default cap - [InlineData("Int32", "\"arrayLength\":62", 0, 62)] // 124 registers — over it - [InlineData("Float64", "\"arrayLength\":31", 0, 31)] // 124 registers - [InlineData("Int16", "\"arrayLength\":10", 10, 10)] // exactly a configured cap - [InlineData("Int16", "\"arrayLength\":11", 10, 11)] // one over it - [InlineData("Int16", "\"arrayLength\":0", 0, 2)] // declared length 0 ⇒ scalar to the driver - [InlineData("Int16", "\"isArrayLengthMissing\":true", 0, 2)] - public async Task Table_agrees_with_the_driver_on_write_cap_and_declared_length( - string dataType, string lengthProp, ushort maxRegistersPerWrite, int elements) + // Numeric FC16 cap. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":61}""", 61, 0)] // 122 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":62}""", 62, 0)] // 124 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":123}""", 123, 0)] // exactly 123 + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":124}""", 124, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Float64","isArray":true,"arrayLength":30}""", 30, 0)] // 120 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Float64","isArray":true,"arrayLength":31}""", 31, 0)] // 124 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":10}""", 10, 10)] // exactly a configured cap + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":11}""", 11, 10)] // one over it + // String FC16 cap: (stringLength+1)/2 registers per element. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":4,"isArray":true,"arrayLength":61}""", 61, 0)] // 122 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":4,"isArray":true,"arrayLength":62}""", 62, 0)] // 124 regs + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":5,"isArray":true,"arrayLength":41}""", 41, 0)] // 3 regs each: 123 + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":5,"isArray":true,"arrayLength":42}""", 42, 0)] // 126 + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":1,"isArray":true,"arrayLength":10}""", 10, 10)] // 1 reg each, at a configured cap + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":1,"isArray":true,"arrayLength":11}""", 11, 10)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":0,"isArray":true,"arrayLength":2}""", 2, 0)] // factory refuses + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","isArray":true,"arrayLength":2}""", 2, 0)] // no length ⇒ refused + // BitInRegister FC16 cap: one register per element, plus the bit-index guard and the span end. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","bitIndex":15,"isArray":true,"arrayLength":123}""", 123, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","bitIndex":15,"isArray":true,"arrayLength":124}""", 124, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":10}""", 10, 10)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":11}""", 11, 10)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","bitIndex":16,"isArray":true,"arrayLength":2}""", 2, 0)] // factory refuses + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"BitInRegister","isArray":true,"arrayLength":6}""", 6, 0)] // ends at 65535 + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"BitInRegister","isArray":true,"arrayLength":7}""", 7, 0)] // past the end + // FC15 ceiling (one request, no chunking; a conformant device refuses more than 1968 coils). + [InlineData("""{"region":"Coils","address":0,"isArray":true,"arrayLength":1968}""", 1968, 0)] + [InlineData("""{"region":"Coils","address":0,"isArray":true,"arrayLength":1969}""", 1969, 0)] + [InlineData("""{"region":"Coils","address":0,"dataType":"Float64","isArray":true,"arrayLength":1968}""", 1968, 0)] // coerced to Bool + [InlineData("""{"region":"Coils","address":0,"dataType":"Float64","isArray":true,"arrayLength":1969}""", 1969, 0)] + // Declared length. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":0}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":1}""", 1, 0)] + // Strict key reads: a present-but-invalid enum or an out-of-range address refuses the tag. + [InlineData("""{"region":"Coils","address":0,"dataType":"Int17","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","byteOrder":"Sideways","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":4,"stringByteOrder":"Middle","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","byteOrder":"WordSwap","isArray":true,"arrayLength":2}""", 2, 0)] // valid order: positive control + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":4,"stringByteOrder":"LowByteFirst","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65536,"dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":-1,"dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":1.5,"dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":"10","dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Bool","isArray":true,"arrayLength":2}""", 2, 0)] // register region never carries Bool + [InlineData("""{"region":0,"address":10,"dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] // non-string region ⇒ absent ⇒ HoldingRegisters + // Keys win over an addressString beside them (the driver never reads the string then). + [InlineData("""{"region":"HoldingRegisters","address":10,"addressString":"30011","dataType":"Int16","isArray":true,"arrayLength":2}""", 2, 0)] + public async Task Table_agrees_with_the_driver_on_each_shape_edge(string tagConfig, int elements, ushort maxRegistersPerWrite) { - var json = $$"""{"region":"HoldingRegisters","address":10,"dataType":"{{dataType}}","isArray":true,{{lengthProp}}}"""; var driverConfig = maxRegistersPerWrite == 0 ? "{}" : $$"""{"maxRegistersPerWrite":{{maxRegistersPerWrite}}}"""; - var fake = new ModbusDriverTests.FakeTransport(); - var options = new ModbusDriverOptions - { - Host = "fake", RawTags = [new RawTagEntry("cell/modbus/arr", json, false)], - MaxRegistersPerWrite = maxRegistersPerWrite == 0 ? (ushort)123 : maxRegistersPerWrite, - }; - var drv = new ModbusDriver(options, "modbus-array-cap-parity", _ => fake); - await drv.InitializeAsync("{}", CancellationToken.None); + var cap = maxRegistersPerWrite == 0 ? (ushort)123 : maxRegistersPerWrite; - var value = Enumerable.Repeat(1, elements).ToArray(); - var result = await drv.WriteAsync([new WriteRequest("cell/modbus/arr", value)], CancellationToken.None); - var driverAccepts = result.Single().StatusCode == 0u; + var (accepted, status) = await DriverEncodesAsync(tagConfig, ClrArray("Int16", elements), cap); - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json, driverConfig).ShouldBe( - driverAccepts, $"{dataType} {lengthProp} cap={maxRegistersPerWrite}: driver status 0x{result.Single().StatusCode:X8}"); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tagConfig, driverConfig).ShouldBe( + accepted, $"{tagConfig} cap={cap}: driver status 0x{status:X8}"); } - /// Positive control for the theory above: both answers occur, so it is not vacuously all-refused. + /// The table restates the Modbus enums' member names (Core.Abstractions cannot reference them); every + /// byte order and string byte order the driver accepts must be one it accepts too, so a new member fails here. + [Fact] + public async Task Every_byte_order_member_agrees_with_the_driver() + { + foreach (var order in Enum.GetValues()) + foreach (var stringOrder in Enum.GetValues()) + { + var json = $$"""{"region":"HoldingRegisters","address":10,"dataType":"Int32","byteOrder":"{{order}}","stringByteOrder":"{{stringOrder}}","isArray":true,"arrayLength":2}"""; + var (accepted, _) = await DriverEncodesAsync(json, ClrArray("Int16", 2)); + accepted.ShouldBeTrue(json); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int32", json).ShouldBeTrue(json); + } + } + + /// + /// The protocol ceilings and the configured caps together, against the real write path: + /// + /// a maxRegistersPerWrite above FC16's 123 never lifts it - the driver sends the over-ceiling + /// FC16 and a conformant device refuses it (past 127 the byte-count field even wraps); + /// a BitInRegister[] write reads its span first in ONE unchunked FC03, so it is also bounded by + /// maxRegistersPerRead (and FC03's 125); numeric arrays never read on write; + /// every span ends at or before address 65535 (exception 02 past it). + /// + /// + [Theory] + // Write cap above the FC16 ceiling. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":123}""", 123, 200, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":124}""", 124, 200, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":150}""", 150, 200, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":4,"isArray":true,"arrayLength":62}""", 62, 200, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":123}""", 123, 200, 0)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":124}""", 124, 200, 200)] + // BitInRegister[] RMW read bounded by maxRegistersPerRead. + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":64}""", 64, 0, 64)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":65}""", 65, 0, 64)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":100}""", 100, 0, 64)] + [InlineData("""{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":100}""", 100, 0, 64)] // no read on write + // Span end at 65535. + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"Int16","isArray":true,"arrayLength":6}""", 6, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"Int16","isArray":true,"arrayLength":7}""", 7, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65532,"dataType":"Int32","isArray":true,"arrayLength":2}""", 2, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65533,"dataType":"Int32","isArray":true,"arrayLength":2}""", 2, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65534,"dataType":"String","stringLength":4,"isArray":true,"arrayLength":1}""", 1, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65535,"dataType":"String","stringLength":4,"isArray":true,"arrayLength":1}""", 1, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"BitInRegister","isArray":true,"arrayLength":6}""", 6, 0, 0)] + [InlineData("""{"region":"HoldingRegisters","address":65530,"dataType":"BitInRegister","isArray":true,"arrayLength":7}""", 7, 0, 0)] + [InlineData("""{"region":"Coils","address":65000,"isArray":true,"arrayLength":536}""", 536, 0, 0)] + [InlineData("""{"region":"Coils","address":65000,"isArray":true,"arrayLength":537}""", 537, 0, 0)] + public async Task Table_agrees_with_the_driver_on_protocol_ceilings_and_spans( + string tagConfig, int elements, ushort maxRegistersPerWrite, ushort maxRegistersPerRead) + { + var configs = new List(); + if (maxRegistersPerWrite != 0) configs.Add($$"""{"maxRegistersPerWrite":{{maxRegistersPerWrite}}}"""); + if (maxRegistersPerRead != 0) configs.Add($$"""{"maxRegistersPerRead":{{maxRegistersPerRead}}}"""); + + var (accepted, status) = await DriverEncodesAsync(tagConfig, ClrArray("Int16", elements), + maxRegistersPerWrite == 0 ? (ushort)123 : maxRegistersPerWrite, + maxRegistersPerRead: maxRegistersPerRead == 0 ? (ushort)125 : maxRegistersPerRead); + + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tagConfig, configs.ToArray()).ShouldBe( + accepted, $"{tagConfig} write={maxRegistersPerWrite} read={maxRegistersPerRead}: driver status 0x{status:X8}"); + } + + /// Positive control for the edges: each cap boundary has a writable side and a refused side in the table. [Fact] public void The_shape_edges_contain_both_answers() { - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int32", - """{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":61}""").ShouldBeTrue(); - DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int32", - """{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":62}""").ShouldBeFalse(); + static bool S(string json) => DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json); + S("""{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":61}""").ShouldBeTrue(); + S("""{"region":"HoldingRegisters","address":10,"dataType":"Int32","isArray":true,"arrayLength":62}""").ShouldBeFalse(); + S("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":5,"isArray":true,"arrayLength":41}""").ShouldBeTrue(); + S("""{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":5,"isArray":true,"arrayLength":42}""").ShouldBeFalse(); + S("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":123}""").ShouldBeTrue(); + S("""{"region":"HoldingRegisters","address":10,"dataType":"BitInRegister","isArray":true,"arrayLength":124}""").ShouldBeFalse(); + S("""{"region":"Coils","address":0,"isArray":true,"arrayLength":1968}""").ShouldBeTrue(); + S("""{"region":"Coils","address":0,"isArray":true,"arrayLength":1969}""").ShouldBeFalse(); + } + + /// + /// FC23 is opt-in and never changes writability: with UseFC23ForMultiRegisterWrites on, the driver + /// writes a multi-register array via FC23 up to its 121-register ceiling and falls back to FC16 above it, so + /// the accept boundary is still the FC16 cap the table uses. + /// + [Theory] + [InlineData("Int16", 121)] + [InlineData("Int16", 123)] + [InlineData("Int16", 124)] + [InlineData("String", 61)] + [InlineData("String", 62)] + public async Task FC23_does_not_change_the_answer(string dataType, int elements) + { + var json = $$"""{"region":"HoldingRegisters","address":10,"dataType":"{{dataType}}","stringLength":4,"isArray":true,"arrayLength":{{elements}}}"""; + + var (fc16, _) = await DriverEncodesAsync(json, ClrArray("Int16", elements), fc23: false); + var (fc23, _) = await DriverEncodesAsync(json, ClrArray("Int16", elements), fc23: true); + + fc23.ShouldBe(fc16); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json).ShouldBe(fc23); + } + + /// + /// A string-only tag (addressString, no region + address keys) is resolved by the DRIVER + /// with its own family. Core.Abstractions cannot parse that grammar (it references no driver assembly), so the + /// table cannot prove such an array writable and keeps it read-only — even though the Generic-family driver does + /// write this one. That is the conservative direction (a hidden write, never an advertised refusal); a tag that + /// needs to be array-writable is authored with the keys (the editor's "Resolve" pins them). + /// + [Theory] + [InlineData("""{"addressString":"40011:S::2","isArray":true,"arrayLength":2}""")] + [InlineData("""{"addressString":"40011:F:2"}""")] + [InlineData("""{"region":"HoldingRegisters","addressString":"40011:S::2","isArray":true,"arrayLength":2}""")] // region alone is not the keys + public async Task A_string_only_array_tag_stays_read_only(string tagConfig) + { + var (accepted, status) = await DriverEncodesAsync(tagConfig, ClrArray("Int16", 2)); + + accepted.ShouldBeTrue($"the Generic driver resolves and writes it (status 0x{status:X8}) — this pins the conservative gap, not a refusal"); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", tagConfig).ShouldBeFalse(); + } + + /// + /// The ScadaBridge wire shape: a CLR int[] / List<int> to an Int16 holding-register array on + /// an Int16 node is advertised writable and CONVERTS element by element onto the wire (it is not exact-matched). + /// + [Fact] + public async Task A_ScadaBridge_int_list_to_an_Int16_array_encodes() + { + const string json = """{"region":"HoldingRegisters","address":10,"dataType":"Int16","isArray":true,"arrayLength":3}"""; + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json).ShouldBeTrue(); + + foreach (object value in new object[] { new[] { 1, -2, 300 }, new List { 1, -2, 300 } }) + { + var device = new ConformantDevice(); + var drv = new ModbusDriver( + new ModbusDriverOptions { Host = "fake", RawTags = [new RawTagEntry(Ref, json, false)] }, "modbus-sb", _ => device); + await drv.InitializeAsync("{}", CancellationToken.None); + + (await drv.WriteAsync([new WriteRequest(Ref, value)], CancellationToken.None)).Single().StatusCode.ShouldBe(0u); + device.Holding[10..13].ShouldBe(new ushort[] { 1, 0xFFFE, 300 }); + } + } + + /// The two #741 U3 shapes encode end to end with their own node types: String[] on a String node and + /// BitInRegister[] on a Boolean node, each preserving what the wire must carry. + [Fact] + public async Task String_and_BitInRegister_arrays_encode_on_their_own_node_types() + { + const string strings = """{"region":"HoldingRegisters","address":10,"dataType":"String","stringLength":3,"isArray":true,"arrayLength":2}"""; + const string bits = """{"region":"HoldingRegisters","address":20,"dataType":"BitInRegister","bitIndex":2,"isArray":true,"arrayLength":2}"""; + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "String", strings).ShouldBeTrue(); + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Boolean", bits).ShouldBeTrue(); + + var device = new ConformantDevice(); + device.Holding[20] = 0x0001; + device.Holding[21] = 0x0005; + var drv = new ModbusDriver( + new ModbusDriverOptions { Host = "fake", RawTags = [new RawTagEntry("s", strings, false), new RawTagEntry("b", bits, false)] }, + "modbus-u3", _ => device); + await drv.InitializeAsync("{}", CancellationToken.None); + + var r = await drv.WriteAsync( + [new WriteRequest("s", new[] { "ABC", "de" }), new WriteRequest("b", new[] { true, false })], CancellationToken.None); + + r.Select(x => x.StatusCode).ShouldBe(new[] { 0u, 0u }); + device.Holding[10..14].ShouldBe(new ushort[] { 0x4142, 0x4300, 0x6465, 0x0000 }); // 2 regs per 3-char element, nul-padded + device.Holding[20..22].ShouldBe(new ushort[] { 0x0005, 0x0001 }); // bit 2 set / cleared, other bits kept + } + + /// + /// A spec-conformant Modbus device over the whole 0-65535 address space. It enforces the protocol quantity + /// ceilings (FC01/02 <= 2000, FC03/04 <= 125, FC15 <= 1968, FC16 <= 123, FC23 write <= 121 / read <= 125) and the + /// FC15 / FC16 / FC23 byte-count field AND the exact frame length it implies (a strict device refuses a trailing + /// byte) with exception 03 (Illegal Data Value), and a span past address 65535 with + /// exception 02 (Illegal Data Address) - what a real device answers. Everything else it answers. + /// + private sealed class ConformantDevice : IModbusTransport + { + public readonly ushort[] Holding = new ushort[65536]; + public readonly bool[] Coils = new bool[65536]; + public int WritePdus; + + /// The device's FC03/FC04 quantity limit: the protocol's 125, or a smaller device-specific one. + public int MaxReadRegisters { get; init; } = 125; + + public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask; + + private static Task Refuse(byte fc, byte code) => + Task.FromException(new ModbusException(fc, code, $"fc={fc:X2} exception {code:X2}")); + + // Exception 03 for a quantity outside 1..max, 02 for a span past the last address, else null (accepted). + private static byte? Check(int addr, int qty, int max) => + qty < 1 || qty > max ? (byte)0x03 : addr + qty > 65536 ? (byte)0x02 : null; + + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken ct) + { + var fc = pdu[0]; + var addr = (pdu[1] << 8) | pdu[2]; + var qty = (pdu[3] << 8) | pdu[4]; + switch (fc) + { + case 0x01: + case 0x02: + { + if (Check(addr, qty, 2000) is { } e) return Refuse(fc, e); + var resp = new byte[2 + (qty + 7) / 8]; + resp[0] = fc; resp[1] = (byte)((qty + 7) / 8); + for (var i = 0; i < qty; i++) if (Coils[addr + i]) resp[2 + i / 8] |= (byte)(1 << (i % 8)); + return Task.FromResult(resp); + } + case 0x03: + case 0x04: + if (Check(addr, qty, MaxReadRegisters) is { } re) return Refuse(fc, re); + return Task.FromResult(ReadRegs(fc, addr, qty)); + case 0x05: + WritePdus++; + Coils[addr] = pdu[3] == 0xFF; + return Task.FromResult(pdu); + case 0x06: + WritePdus++; + Holding[addr] = (ushort)((pdu[3] << 8) | pdu[4]); + return Task.FromResult(pdu); + case 0x0F: + if (Check(addr, qty, 1968) is { } ce) return Refuse(fc, ce); + if (pdu[5] != (qty + 7) / 8 || pdu.Length != 6 + pdu[5]) return Refuse(fc, 0x03); // strict frame length + WritePdus++; + for (var i = 0; i < qty; i++) Coils[addr + i] = ((pdu[6 + i / 8] >> (i % 8)) & 1) == 1; + return Task.FromResult(new byte[] { 0x0F, pdu[1], pdu[2], pdu[3], pdu[4] }); + case 0x10: + if (Check(addr, qty, 123) is { } we) return Refuse(fc, we); + if (pdu[5] != qty * 2 || pdu.Length != 6 + pdu[5]) return Refuse(fc, 0x03); // strict frame length + WritePdus++; + for (var i = 0; i < qty; i++) Holding[addr + i] = (ushort)((pdu[6 + i * 2] << 8) | pdu[7 + i * 2]); + return Task.FromResult(new byte[] { 0x10, pdu[1], pdu[2], pdu[3], pdu[4] }); + case 0x17: + { + var writeAddr = (pdu[5] << 8) | pdu[6]; + var writeQty = (pdu[7] << 8) | pdu[8]; + if (Check(addr, qty, 125) is { } r23) return Refuse(fc, r23); + if (Check(writeAddr, writeQty, 121) is { } w23) return Refuse(fc, w23); + if (pdu[9] != writeQty * 2 || pdu.Length != 10 + pdu[9]) return Refuse(fc, 0x03); // strict frame length + WritePdus++; + for (var i = 0; i < writeQty; i++) Holding[writeAddr + i] = (ushort)((pdu[10 + i * 2] << 8) | pdu[11 + i * 2]); + return Task.FromResult(ReadRegs(0x17, addr, qty)); + } + default: + return Refuse(fc, 0x01); + } + } + + private byte[] ReadRegs(byte fc, int addr, int qty) + { + var resp = new byte[2 + qty * 2]; + resp[0] = fc; resp[1] = (byte)(qty * 2); + for (var i = 0; i < qty; i++) + { + resp[2 + i * 2] = (byte)(Holding[addr + i] >> 8); + resp[3 + i * 2] = (byte)(Holding[addr + i] & 0xFF); + } + return resp; + } + + public ValueTask DisposeAsync() => ValueTask.CompletedTask; } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs new file mode 100644 index 00000000..6cf0450b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs @@ -0,0 +1,62 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// R07:L5 (Modbus half): the RTU ADU is now written into a pooled buffer and the response is deframed in one pooled +/// buffer. The wire bytes and the decoded PDUs must be exactly what the allocating code produced, and a reused pooled +/// buffer must never leak a previous frame's bytes into the next result. +/// +[Trait("Category", "Unit")] +public sealed class ModbusPooledFramingTests +{ + /// The pooled ADU writer matches the reference construction (unit + PDU, CRC appended low byte first). + /// The PDU as hex. + [Theory] + [InlineData("0300000001")] + [InlineData("10000A0002040001000200")] + [InlineData("06")] + public void WriteAdu_matches_the_reference_ADU(string hex) + { + var pdu = Convert.FromHexString(hex); + var reference = ModbusCrc.Append([0x11, .. pdu]); + + var pooled = new byte[ModbusRtuFraming.AduLength(pdu.Length) + 7]; // oversized, like a rented array + ModbusRtuFraming.WriteAdu(0x11, pdu, pooled.AsSpan(0, ModbusRtuFraming.AduLength(pdu.Length))); + + pooled.AsSpan(0, reference.Length).ToArray().ShouldBe(reference); + ModbusRtuFraming.BuildAdu(0x11, pdu).ShouldBe(reference); + } + + /// + /// A long read reply followed by a short echo and an exception frame, each deframed through the pooled buffer: + /// every result is exactly its own PDU (no stale tail from the previous, longer frame). + /// + [Fact] + public async Task Consecutive_deframes_through_the_pooled_buffer_do_not_leak_bytes() + { + var ct = TestContext.Current.CancellationToken; + byte[] longRead = [0x03, 200, .. Enumerable.Range(0, 200).Select(i => (byte)(i + 1))]; + byte[] echo = [0x06, 0x00, 0x01, 0x00, 0x2A]; + + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, longRead)), 1, ct)) + .ShouldBe(longRead); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, echo)), 1, ct)) + .ShouldBe(echo); + var ex = await Should.ThrowAsync(() => + ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x83, 0x02])), 1, ct)); + ex.ExceptionCode.ShouldBe((byte)0x02); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x03, 2, 9, 8])), 1, ct)) + .ShouldBe(new byte[] { 0x03, 2, 9, 8 }); + } + + /// A byte-count-255 read (the largest RTU reply) fits the pooled frame. + [Fact] + public async Task The_largest_read_reply_deframes() + { + byte[] max = [0x04, 255, .. new byte[255]]; + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, max)), 1, + TestContext.Current.CancellationToken)).ShouldBe(max); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusWritePduFramingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusWritePduFramingTests.cs new file mode 100644 index 00000000..bc125c3f --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusWritePduFramingTests.cs @@ -0,0 +1,107 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// #741 (Integration-W3 fix round): every multi-write PDU the driver builds is EXACTLY its header plus the bytes its +/// byte-count field declares — FC15 and FC16 are 6 + byteCount, FC23 is 10 + writeByteCount. The +/// builders used to allocate one byte more than they filled, so every FC15 / FC16 / FC23 (and the BitInRegister[] +/// read-modify-write's FC16) carried a trailing zero byte past the declared data — which a strict device or gateway +/// that checks the frame length against the byte count refuses. +/// +[Trait("Category", "Unit")] +public sealed class ModbusWritePduFramingTests +{ + private const string Ref = "arr"; + + /// Records every PDU; answers reads with zeros and multi-writes with the standard echo. + private sealed class CapturingTransport : IModbusTransport + { + public readonly List Sent = []; + + public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask; + + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken ct) + { + Sent.Add(pdu); + var qty = (pdu[3] << 8) | pdu[4]; + return Task.FromResult(pdu[0] switch + { + 0x01 or 0x02 => [pdu[0], (byte)((qty + 7) / 8), .. new byte[(qty + 7) / 8]], + 0x03 or 0x04 or 0x17 => [pdu[0], (byte)(qty * 2), .. new byte[qty * 2]], + 0x0F or 0x10 => new[] { pdu[0], pdu[1], pdu[2], pdu[3], pdu[4] }, + _ => pdu, + }); + } + + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + } + + private static async Task WriteAndCaptureAsync(string tagConfig, object value, byte functionCode, bool fc23 = false) + { + var transport = new CapturingTransport(); + var drv = new ModbusDriver( + new ModbusDriverOptions + { + Host = "fake", RawTags = [new RawTagEntry(Ref, tagConfig, false)], UseFC23ForMultiRegisterWrites = fc23, + }, + "modbus-framing", _ => transport); + await drv.InitializeAsync("{}", CancellationToken.None); + + (await drv.WriteAsync([new WriteRequest(Ref, value)], CancellationToken.None)).Single().StatusCode.ShouldBe(0u); + + return transport.Sent.Single(p => p[0] == functionCode); + } + + [Theory] + [InlineData(1)] + [InlineData(8)] + [InlineData(9)] + public async Task FC15_pdu_is_exactly_header_plus_byte_count(int coils) + { + var pdu = await WriteAndCaptureAsync( + $$"""{"region":"Coils","address":0,"isArray":true,"arrayLength":{{coils}}}""", + Enumerable.Repeat(true, coils).ToArray(), 0x0F); + + pdu[5].ShouldBe((byte)((coils + 7) / 8)); + pdu.Length.ShouldBe(6 + pdu[5]); + } + + [Theory] + [InlineData("""{"region":"HoldingRegisters","address":0,"dataType":"Int16","isArray":true,"arrayLength":3}""", 6)] + [InlineData("""{"region":"HoldingRegisters","address":0,"dataType":"Float64","isArray":true,"arrayLength":2}""", 16)] + [InlineData("""{"region":"HoldingRegisters","address":0,"dataType":"Int32"}""", 4)] // scalar multi-register + public async Task FC16_pdu_is_exactly_header_plus_byte_count(string tagConfig, int byteCount) + { + object value = tagConfig.Contains("isArray") ? new[] { 1, 2, 3 }.Take(tagConfig.Contains("Float64") ? 2 : 3).ToArray() : 7; + + var pdu = await WriteAndCaptureAsync(tagConfig, value, 0x10); + + pdu[5].ShouldBe((byte)byteCount); + pdu.Length.ShouldBe(6 + pdu[5]); + } + + [Fact] + public async Task BitInRegister_array_RMW_FC16_pdu_is_exactly_header_plus_byte_count() + { + var pdu = await WriteAndCaptureAsync( + """{"region":"HoldingRegisters","address":0,"dataType":"BitInRegister","bitIndex":2,"isArray":true,"arrayLength":4}""", + new[] { true, false, true, false }, 0x10); + + pdu[5].ShouldBe((byte)8); + pdu.Length.ShouldBe(6 + pdu[5]); + } + + [Fact] + public async Task FC23_pdu_is_exactly_header_plus_write_byte_count() + { + var pdu = await WriteAndCaptureAsync( + """{"region":"HoldingRegisters","address":0,"dataType":"Int16","isArray":true,"arrayLength":3}""", + new[] { 1, 2, 3 }, 0x17, fc23: true); + + pdu[9].ShouldBe((byte)6); + pdu.Length.ShouldBe(10 + pdu[9]); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceComposerDualNamespaceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceComposerDualNamespaceTests.cs index fbbd41ac..a7baea9a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceComposerDualNamespaceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceComposerDualNamespaceTests.cs @@ -414,4 +414,64 @@ public sealed class AddressSpaceComposerDualNamespaceTests AddressSpaceApplier.UnsReferenceNodeWritable(c.UnsReferenceVariables.Single(v => v.UnsTagReferenceId == id)).ShouldBeTrue(id); AddressSpaceApplier.UnsReferenceNodeWritable(c.UnsReferenceVariables.Single(v => v.UnsTagReferenceId == "r-bits")).ShouldBeFalse(); } + /// + /// #741 (Modbus lane U3, follow-up F1): the Modbus driver encodes String[] and BitInRegister[] holding-register + /// arrays, so an authored-ReadWrite array of each materialises WRITABLE — raw node and UNS mirror alike. A + /// Coils array authored with a non-Bool type (the resolver coerces a bit region to Bool) is writable too. Shapes + /// the driver refuses stay read-only on the SAME driver: a BitInRegister array over the FC16 cap, a String array + /// with no stringLength, and a string-only (addressString) array whose family the composer cannot + /// know — the negative controls. + /// + [Fact] + public void Modbus_string_and_bit_in_register_arrays_materialise_writable_and_refused_shapes_stay_read_only() + { + var folder = new RawFolder { RawFolderId = "f", ClusterId = "c1", Name = "Plant" }; + var mb = new DriverInstance { DriverInstanceId = "drv-mb", ClusterId = "c1", RawFolderId = "f", Name = "Mb", DriverType = "Modbus", DriverConfig = "{}" }; + var dev = new Device { DeviceId = "d1", DriverInstanceId = "drv-mb", Name = "plc", DeviceConfig = "{}" }; + static Tag Arr(string id, string dataType, string tagConfig) => new() + { + TagId = id, DeviceId = "d1", Name = id, DataType = dataType, AccessLevel = TagAccessLevel.ReadWrite, + TagConfig = tagConfig, + }; + var tags = new[] + { + Arr("names", "String", "{\"region\":\"HoldingRegisters\",\"address\":100,\"dataType\":\"String\",\"stringLength\":8,\"isArray\":true,\"arrayLength\":4}"), + Arr("flags", "Boolean", "{\"region\":\"HoldingRegisters\",\"address\":200,\"dataType\":\"BitInRegister\",\"bitIndex\":3,\"isArray\":true,\"arrayLength\":8}"), + Arr("legacyCoils", "Int16", "{\"region\":\"Coils\",\"address\":0,\"dataType\":\"Int16\",\"isArray\":true,\"arrayLength\":8}"), + Arr("overCap", "Boolean", "{\"region\":\"HoldingRegisters\",\"address\":300,\"dataType\":\"BitInRegister\",\"isArray\":true,\"arrayLength\":124}"), + Arr("noLength", "String", "{\"region\":\"HoldingRegisters\",\"address\":400,\"dataType\":\"String\",\"isArray\":true,\"arrayLength\":4}"), + Arr("stringOnly", "Int16", "{\"addressString\":\"40501:S::4\",\"isArray\":true,\"arrayLength\":4}"), + }; + var area = new UnsArea { UnsAreaId = "a", ClusterId = "c1", Name = "filling" }; + var line = new UnsLine { UnsLineId = "l", UnsAreaId = "a", Name = "line-1" }; + var equip = new Equipment { EquipmentId = "e", UnsLineId = "l", Name = "station-1", MachineCode = "S1" }; + var refs = new[] + { + new UnsTagReference { UnsTagReferenceId = "r-names", EquipmentId = "e", TagId = "names" }, + new UnsTagReference { UnsTagReferenceId = "r-flags", EquipmentId = "e", TagId = "flags" }, + new UnsTagReference { UnsTagReferenceId = "r-over", EquipmentId = "e", TagId = "overCap" }, + }; + + var c = AddressSpaceComposer.Compose( + new[] { area }, new[] { line }, new[] { equip }, new[] { mb }, Array.Empty(), + unsTagReferences: refs, tags: tags, rawFolders: new[] { folder }, devices: new[] { dev }); + + foreach (var writable in new[] { "names", "flags", "legacyCoils" }) + { + var plan = c.RawTags.Single(t => t.Name == writable); + plan.ArrayWritable.ShouldBeTrue(writable); + AddressSpaceApplier.RawNodeWritable(plan).ShouldBeTrue(writable); + } + foreach (var refused in new[] { "overCap", "noLength", "stringOnly" }) + { + var plan = c.RawTags.Single(t => t.Name == refused); + plan.IsArray.ShouldBeTrue(refused); // still an array node — just not a writable one + plan.ArrayWritable.ShouldBeFalse(refused); + AddressSpaceApplier.RawNodeWritable(plan).ShouldBeFalse(refused); + } + + foreach (var id in new[] { "r-names", "r-flags" }) + AddressSpaceApplier.UnsReferenceNodeWritable(c.UnsReferenceVariables.Single(v => v.UnsTagReferenceId == id)).ShouldBeTrue(id); + AddressSpaceApplier.UnsReferenceNodeWritable(c.UnsReferenceVariables.Single(v => v.UnsTagReferenceId == "r-over")).ShouldBeFalse(); + } }