From 0f305f18273d788cea8be8f4e0518e280a3cae9c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:35:40 -0400 Subject: [PATCH] fix(modbus): multi-write PDUs carry no trailing byte past their byte count The FC15, FC16, BitInRegister[] RMW FC16 and FC23 builders allocated one byte more than they filled (6 + 1 + n / 10 + 1 + n, data copied at offset 6 / 10), so every multi-write request ended in a stray zero byte beyond the length its byte-count field declares. A strict device or gateway that checks the frame length against the byte count refuses it. Each PDU is now exactly 6 + n (FC15/FC16) or 10 + n (FC23). Tests: ModbusWritePduFramingTests asserts the exact length and byte-count field for FC15 (1/8/9 coils), FC16 (array, Float64 array, scalar multi-register), the BitInRegister RMW FC16 and FC23 (all red before the fix); the parity suite's conformant device now refuses a frame-length mismatch on FC15/FC16/FC23. Live: Driver.Modbus.IntegrationTests against 10.100.0.35:5020 (standard profile) passes, including the FC16 and FC23 multi-register round trips. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ModbusDriver.cs | 12 +- .../ModbusArrayWriteSupportParityTests.cs | 8 +- .../ModbusWritePduFramingTests.cs | 107 ++++++++++++++++++ 3 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusWritePduFramingTests.cs 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/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs index cca64cf5..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 @@ -341,7 +341,8 @@ public sealed class ModbusArrayWriteSupportParityTests /// /// 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 + /// 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 @@ -392,13 +393,13 @@ public sealed class ModbusArrayWriteSupportParityTests 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); + 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); + 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] }); @@ -408,6 +409,7 @@ public sealed class ModbusArrayWriteSupportParityTests 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)); 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]); + } +}