fix(arrays): Modbus row honours the FC16/FC03 protocol ceilings, the RMW read cap and the address-space end
Review r0 of Integration-W3: - I1: the effective FC16 cap is min(configured maxRegistersPerWrite, 123). The driver trusts a larger configured cap and sends an over-ceiling FC16 (past 127 registers its byte-count byte wraps), which every conformant device refuses, so such an array is never advertised writable. - I2: a BitInRegister[] write reads its span first in one unchunked FC03, so it is also bounded by min(configured maxRegistersPerRead, 125) (absent/0 = 125). - m2: every span (registers and coils) must end at or before address 65535. - m1: the String-node wording says it converts only when the text parses, which the server cannot promise (DateTime never converts). - m4: the parity device now enforces FC01/02 2000, FC03/04 125 (or a smaller device read limit), FC15 1968, FC16 123, FC23 121/125, the FC15/FC16 byte-count field (exception 03) and the address-space end (exception 02). Tests: parity rows for each ceiling/cap/span boundary against the real write path, and Core.Abstractions row tests. The configured-cap test that asserted a cap of 200 lifts the ceiling now asserts the opposite. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
+43
-3
@@ -74,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();
|
||||
}
|
||||
|
||||
/// <summary>A BitInRegister[] write is a read-modify-write: one unchunked FC03 of the whole span first, so its
|
||||
/// length is also bounded by <c>maxRegistersPerRead</c> (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.</summary>
|
||||
[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();
|
||||
}
|
||||
|
||||
/// <summary>Every shape's span must end at or before address 65535 - a conformant device refuses a write past it
|
||||
/// (exception 02).</summary>
|
||||
[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);
|
||||
|
||||
/// <summary>FC15 carries at most 1968 coils and the driver sends one request, so a longer coil array is not
|
||||
/// writable.</summary>
|
||||
[Fact]
|
||||
@@ -104,7 +143,8 @@ public sealed class DriverArrayWriteSupportTests
|
||||
$"{{\"region\":\"HoldingRegisters\",\"address\":0,{typeKeys},\"isArray\":true,\"arrayLength\":{length}}}").ShouldBe(expected);
|
||||
|
||||
/// <summary>Elements CONVERT (ScadaBridge's <c>int[]</c> 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
|
||||
/// 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 <c>Convert.ToString</c>'d).</summary>
|
||||
[Theory]
|
||||
[InlineData("Int16", "Int16", true)]
|
||||
|
||||
+89
-18
@@ -13,11 +13,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests;
|
||||
/// refuses (nor hide one it accepts).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// "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
|
||||
/// (<c>MaxRegistersPerWrite</c>). FC23 is opt-in and never changes the answer (pinned below).
|
||||
/// "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 <c>MaxRegistersPerWrite</c> below 123 is the driver's own. FC23
|
||||
/// is opt-in and never changes the answer (pinned below).
|
||||
/// </remarks>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class ModbusArrayWriteSupportParityTests
|
||||
@@ -46,14 +47,17 @@ public sealed class ModbusArrayWriteSupportParityTests
|
||||
|
||||
/// <summary>Drives the real driver: true iff the write is Good AND a write PDU reached the device.</summary>
|
||||
private static async Task<(bool Accepted, uint Status)> DriverEncodesAsync(
|
||||
string tagConfig, object value, ushort maxRegistersPerWrite = 123, bool fc23 = false)
|
||||
string tagConfig, object value, ushort maxRegistersPerWrite = 123, bool fc23 = false, ushort maxRegistersPerRead = 125)
|
||||
{
|
||||
var device = new ConformantDevice();
|
||||
// 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(Ref, tagConfig, false)],
|
||||
MaxRegistersPerWrite = maxRegistersPerWrite, UseFC23ForMultiRegisterWrites = fc23,
|
||||
MaxRegistersPerRead = maxRegistersPerRead,
|
||||
},
|
||||
"modbus-array-parity", _ => device);
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
@@ -181,6 +185,55 @@ public sealed class ModbusArrayWriteSupportParityTests
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The protocol ceilings and the configured caps together, against the real write path:
|
||||
/// <list type="bullet">
|
||||
/// <item>a <c>maxRegistersPerWrite</c> 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);</item>
|
||||
/// <item>a BitInRegister[] write reads its span first in ONE unchunked FC03, so it is also bounded by
|
||||
/// <c>maxRegistersPerRead</c> (and FC03's 125); numeric arrays never read on write;</item>
|
||||
/// <item>every span ends at or before address 65535 (exception 02 past it).</item>
|
||||
/// </list>
|
||||
/// </summary>
|
||||
[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<string>();
|
||||
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}");
|
||||
}
|
||||
|
||||
/// <summary>Positive control for the edges: each cap boundary has a writable side and a refused side in the table.</summary>
|
||||
[Fact]
|
||||
public void The_shape_edges_contain_both_answers()
|
||||
@@ -286,8 +339,10 @@ public sealed class ModbusArrayWriteSupportParityTests
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// 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 byte-count field 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.
|
||||
/// </summary>
|
||||
private sealed class ConformantDevice : IModbusTransport
|
||||
{
|
||||
@@ -295,25 +350,38 @@ public sealed class ModbusArrayWriteSupportParityTests
|
||||
public readonly bool[] Coils = new bool[65536];
|
||||
public int WritePdus;
|
||||
|
||||
/// <summary>The device's FC03/FC04 quantity limit: the protocol's 125, or a smaller device-specific one.</summary>
|
||||
public int MaxReadRegisters { get; init; } = 125;
|
||||
|
||||
public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask;
|
||||
|
||||
private static Task<byte[]> Refuse(byte fc, byte code) =>
|
||||
Task.FromException<byte[]>(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<byte[]> 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 (pdu[0])
|
||||
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] = pdu[0]; resp[1] = (byte)((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:
|
||||
return Task.FromResult(ReadRegs(pdu[0], addr, qty));
|
||||
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;
|
||||
@@ -323,26 +391,29 @@ public sealed class ModbusArrayWriteSupportParityTests
|
||||
Holding[addr] = (ushort)((pdu[3] << 8) | pdu[4]);
|
||||
return Task.FromResult(pdu);
|
||||
case 0x0F:
|
||||
if (qty is < 1 or > 1968)
|
||||
return Task.FromException<byte[]>(new ModbusException(0x0F, 0x03, "FC15 quantity outside 1..1968"));
|
||||
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);
|
||||
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);
|
||||
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];
|
||||
if (Check(addr, qty, 125) is { } r23) return Refuse(fc, r23);
|
||||
if (Check(writeAddr, writeQty, 121) is { } w23) return Refuse(fc, w23);
|
||||
WritePdus++;
|
||||
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);
|
||||
return Task.FromResult(ReadRegs(0x17, addr, qty));
|
||||
}
|
||||
default:
|
||||
return Task.FromException<byte[]>(new ModbusException(pdu[0], 0x01, "unsupported"));
|
||||
return Refuse(fc, 0x01);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user