From 5533de62814f04d261dba1ccaddf17c64da585c0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:10:00 -0400 Subject: [PATCH] feat(arrays): Modbus String[]/BitInRegister[] array writes reachable; table follows the resolver The Modbus lane (#741 U3) gave the driver String[] and BitInRegister[] holding-register array encoders, and moved address resolution to ModbusTagAddressResolution (keys first, a bit region coerced to Bool, strict key reads), but DriverArrayWriteSupport still listed both array shapes read-only (follow-up F1), so a client write was BadNotWritable before it reached the driver. Modbus row now mirrors the driver end to end: - Keys shape only (region + address both present). A string-only addressString tag is resolved by the driver with its own family through a parser Core.Abstractions cannot reference, so it stays read-only (pinned: the conservative direction). - Strict reads as the resolver: address an integer 0-65535; a present region / dataType / byteOrder / stringByteOrder string must name a member. - Coils: any dataType (coerced to Bool), <= 1968 coils (FC15). - HoldingRegisters (FC16, never Bool): numeric, String ((stringLength+1)/2 registers, stringLength >= 1), BitInRegister (1 register, bitIndex 0-15, span ending <= 65535), all within maxRegistersPerWrite. FC23 is opt-in and never changes the answer. - Elements convert (ScadaBridge int[] -> Int16 stays writable); only a String / DateTime node over a numeric or bit element is left out, since such an element never converts. Tests: the parity test's PendingF1 exemption and its guard are gone; the parity matrix is now region x element type x node type against the real WriteAsync with a spec-conformant device (FC15 > 1968 refused), plus shape edges at every cap boundary (numeric, String, BitInRegister, coil), strict key reads, FC23 on/off, string-only, the ScadaBridge int[] shape and U3 wire bytes. Core.Abstractions row tests and a composer test (String[] and BitInRegister[] writable; over-cap, no-stringLength and string-only read-only). Docs: table XML doc, Uns.md, modbus-addressing.md, drivers/Modbus.md, data-plane rules. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .claude/rules/data-plane-auth.md | 5 +- docs/Uns.md | 9 +- docs/drivers/Modbus.md | 12 +- docs/v2/modbus-addressing.md | 35 ++ .../DriverArrayWriteSupport.cs | 138 +++++- .../DriverArrayWriteSupportTests.cs | 54 ++- .../ModbusArrayWriteSupportParityTests.cs | 414 ++++++++++++++---- .../AddressSpaceComposerDualNamespaceTests.cs | 60 +++ 8 files changed, 597 insertions(+), 130 deletions(-) diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index ca9f7886..0287d536 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -148,8 +148,9 @@ 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`/coil cap incl. String/BitInRegister arrays (#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..e6c07878 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, span ending at or before register 65535), 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. 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 never convert — a `DateTime` or +`String` node over a numeric element (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..0276404c 100644 --- a/docs/drivers/Modbus.md +++ b/docs/drivers/Modbus.md @@ -138,11 +138,15 @@ 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). 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..2eb2e73e 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,31 @@ 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). 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, the span must end at or + before register 65535). 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 (such an element never converts). Elements otherwise convert, so a + ScadaBridge `int[]` to an `Int16` node is writable. 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..7b723ed2 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,20 @@ 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, read-modify-written; bitIndex 0–15, the span +/// ending at or before register 65535), whose arrayLength × registers-per-element fits the +/// driver's maxRegistersPerWrite (default 123 — the driver refuses rather than chunks a longer +/// FC16). 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 +70,11 @@ 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 NEVER convert: a DateTime or String node over a numeric +/// element (Modbus — over a numeric or bit element; a Modbus String element takes any node type — AbCip, +/// AbLegacy, 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 @@ -89,6 +100,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 +183,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 +205,76 @@ 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, span ending at or before 65535); arrayLength × registers per + // element within maxRegistersPerWrite (the driver refuses rather than chunks; FC23 is opt-in and capped lower, + // falling back to FC16, so it never changes the answer). + // - Elements CONVERT, so the node type is not exact-matched; a String / DateTime node over a numeric or bit element + // never converts and is 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 && 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 array RMW refuses a span + // past the last register. + if ((byte)ReadInt(root, "bitIndex") > 15 || (long)address + count - 1 > ushort.MaxValue) return false; + registersPerElement = 1; + } + else if (!ModbusRegisterArrayTypes.TryGetValue(native, out registersPerElement)) + { + return false; // Bool in a register region: the factory refuses the pair + } + } + return (long)count * registersPerElement <= ModbusMaxRegistersPerWrite(driverConfigs); + } + + // 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 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..f410c8bb 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)] @@ -75,6 +90,39 @@ 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, which never converts. 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/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs index 41a20e50..df481e42 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,359 @@ 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) reached the transport. The transport is a +/// spec-conformant device: it holds the whole 0–65535 address space and answers an FC15 of more than 1968 coils +/// with exception 03, as every conformant device does — the driver itself sends one FC15 without chunking, so the +/// table's 1968 ceiling is the device's refusal, not the driver's. The FC16 cap is the driver's own +/// (MaxRegistersPerWrite). 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) { - 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(); + var device = new ConformantDevice(); 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, + }, + "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); + } + } + + /// 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 refuses an FC15 of more than + /// 1968 coils with exception 03 (Illegal Data Value), as the spec requires; 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; + + public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask; + + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken ct) + { + var addr = (pdu[1] << 8) | pdu[2]; + var qty = (pdu[3] << 8) | pdu[4]; + switch (pdu[0]) + { + case 0x01: + case 0x02: + { + var resp = new byte[2 + (qty + 7) / 8]; + resp[0] = pdu[0]; 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: + return Task.FromResult(ReadRegs(pdu[0], 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 (qty is < 1 or > 1968) + return Task.FromException(new ModbusException(0x0F, 0x03, "FC15 quantity outside 1..1968")); + 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: + 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: + { + WritePdus++; + var writeAddr = (pdu[5] << 8) | pdu[6]; + var writeQty = (pdu[7] << 8) | pdu[8]; + for (var i = 0; i < writeQty; i++) Holding[writeAddr + i] = (ushort)((pdu[10 + i * 2] << 8) | pdu[11 + i * 2]); + var resp = ReadRegs(0x17, addr, qty); + return Task.FromResult(resp); + } + default: + return Task.FromException(new ModbusException(pdu[0], 0x01, "unsupported")); + } + } + + 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/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(); + } }