From 1adc9d65e1f6dfb9560e35861ae35b680241a0d4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:26:59 -0400 Subject: [PATCH] feat(modbus): String[] and BitInRegister[] writes U3: both array shapes read fine but every write failed BadInternalError. String[] now encodes each element at its register width (StringLength chars, 2 per register); BitInRegister[] reads the span once, flips the addressed bit of each register and writes the span back with FC16 (per-register RMW locks taken in ascending order). A length mismatch is BadOutOfRange and sends nothing. DriverArrayWriteSupport (outside this lane) still advertises these nodes read-only - recorded as follow-up F1 in the parity test. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ModbusDriver.cs | 65 ++++++++++++++- .../ModbusArrayWriteSupportParityTests.cs | 4 +- .../ModbusStringAndBitArrayWriteTests.cs | 82 +++++++++++++++++++ 3 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusStringAndBitArrayWriteTests.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 cc252539..4aa312d3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -1554,7 +1554,10 @@ public sealed class ModbusDriver if (tag.DataType == ModbusDataType.BitInRegister && tag.Region is ModbusRegion.HoldingRegisters) { - await WriteBitInRegisterAsync(transport, tag, value, ct).ConfigureAwait(false); + if (tag.ArrayCount.HasValue) + await WriteBitInRegisterArrayAsync(transport, tag, value, ct).ConfigureAwait(false); + else + await WriteBitInRegisterAsync(transport, tag, value, ct).ConfigureAwait(false); return; } @@ -1680,9 +1683,67 @@ public sealed class ModbusDriver ModbusDataType.Int16 or ModbusDataType.UInt16 or ModbusDataType.Bcd16 => 2, ModbusDataType.Int32 or ModbusDataType.UInt32 or ModbusDataType.Float32 or ModbusDataType.Bcd32 => 4, ModbusDataType.Int64 or ModbusDataType.UInt64 or ModbusDataType.Float64 => 8, - _ => throw new InvalidOperationException($"Element byte count not defined for {tag.DataType} in array context"), + // A String element is StringLength chars at 2 per register, rounded up — the read path's width. + ModbusDataType.String => RegisterCount(tag) * 2, + _ => throw new ModbusWriteRefusedException(StatusBadNotSupported, + $"Array writes are not supported for {tag.DataType} tags ({tag.Name})"), }; + /// + /// BitInRegister[] write: element i is bit BitIndex of holding register + /// Address + i (the read path's layout). One FC03 of the whole span, flip the addressed bit of + /// each register, one FC16 back — so the other 15 bits of every register are preserved. Serialised + /// per register with the scalar bit-write path (locks taken in ascending address order, so two + /// overlapping array writes cannot deadlock). Not atomic against the PLC's own writes between the + /// read and the write, exactly like the scalar RMW. + /// + private async Task WriteBitInRegisterArrayAsync( + IModbusTransport transport, ModbusTagDefinition tag, object? value, CancellationToken ct) + { + var count = tag.ArrayCount!.Value; + var bits = ToBoolArray(value, count, tag.Name); + var writeCap = _options.MaxRegistersPerWrite == 0 ? (ushort)123 : _options.MaxRegistersPerWrite; + if (count > writeCap || tag.Address + count - 1 > ushort.MaxValue) + throw new ModbusWriteRefusedException(StatusBadOutOfRange, + $"BitInRegister array write of {count} registers to {tag.Name} exceeds MaxRegistersPerWrite={writeCap}"); + if (tag.BitIndex > 15) + throw new ModbusWriteRefusedException(StatusBadOutOfRange, + $"BitInRegister bit index {tag.BitIndex} out of range (0-15) for tag {tag.Name}."); + + var locks = new List(count); + try + { + for (var i = 0; i < count; i++) + { + var l = GetRmwLock((ushort)(tag.Address + i)); + await l.WaitAsync(ct).ConfigureAwait(false); + locks.Add(l); + } + + var unit = ResolveUnitId(tag); + var current = await ReadRegisterBlockAsync(transport, unit, 0x03, tag.Address, (ushort)count, ct).ConfigureAwait(false); + var payload = new byte[count * 2]; + for (var i = 0; i < count; i++) + { + var reg = (ushort)((current[i * 2] << 8) | current[i * 2 + 1]); + reg = bits[i] ? (ushort)(reg | (1 << tag.BitIndex)) : (ushort)(reg & ~(1 << tag.BitIndex)); + payload[i * 2] = (byte)(reg >> 8); + payload[i * 2 + 1] = (byte)(reg & 0xFF); + } + var pdu = new byte[6 + 1 + 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); + pdu[5] = (byte)payload.Length; + Buffer.BlockCopy(payload, 0, pdu, 6, payload.Length); + await transport.SendAsync(unit, pdu, ct).ConfigureAwait(false); + } + finally + { + for (var i = locks.Count - 1; i >= 0; i--) locks[i].Release(); + } + } + /// /// Read-modify-write one bit in a holding register. FC03 → bit-swap → FC06. Serialised /// against other bit writes targeting the same register via . 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 6d2363b1..41a20e50 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 @@ -35,10 +35,8 @@ public sealed class ModbusArrayWriteSupportParityTests /// private static bool PendingF1(ModbusRegion region, ModbusDataType dataType) => (region == ModbusRegion.Coils && dataType != ModbusDataType.Bool) - || (region == ModbusRegion.HoldingRegisters && dataType is ModbusDataType.String or ModbusDataType.BitInRegister && PendingU3); + || (region == ModbusRegion.HoldingRegisters && dataType is ModbusDataType.String or ModbusDataType.BitInRegister); - // Flipped to true by the U3 commit (String[] / BitInRegister[] encoders). - private const bool PendingU3 = false; public static TheoryData PendingShapes() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusStringAndBitArrayWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusStringAndBitArrayWriteTests.cs new file mode 100644 index 00000000..8cdfa7d6 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusStringAndBitArrayWriteTests.cs @@ -0,0 +1,82 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// Review 07 U3: String[] and BitInRegister[] tags read fine; their writes used to fail +/// BadInternalError (no element width for String, Convert.ToBoolean(bool[]) for the bit +/// array). Both now encode count elements at the tag's element width; a length mismatch is +/// BadOutOfRange. +/// +[Trait("Category", "Unit")] +public sealed class ModbusStringAndBitArrayWriteTests +{ + private static async Task<(ModbusDriver, ScriptedTransport)> StartAsync(params ModbusTagDefinition[] tags) + { + var t = new ScriptedTransport(); + var d = new ModbusDriver(new ModbusDriverOptions + { + Host = "f", RawTags = ModbusRawTags.Entries(tags), Probe = new ModbusProbeOptions { Enabled = false }, + }, "m1", _ => t); + await d.InitializeAsync("{}", TestContext.Current.CancellationToken); + return (d, t); + } + + [Fact] + public async Task String_array_write_encodes_each_element_at_its_register_width() + { + var tag = new ModbusTagDefinition("S", ModbusRegion.HoldingRegisters, 10, ModbusDataType.String, + StringLength: 3, ArrayCount: 2); // 2 registers per element + var (drv, t) = await StartAsync(tag); + await using var _ = drv; + + var w = await drv.WriteAsync([new WriteRequest("S", new[] { "AB", "XYZ" })], TestContext.Current.CancellationToken); + + w[0].StatusCode.ShouldBe(0u); + t.Bank.HoldingRegisters[10].ShouldBe((ushort)(('A' << 8) | 'B')); + t.Bank.HoldingRegisters[11].ShouldBe((ushort)0); + t.Bank.HoldingRegisters[12].ShouldBe((ushort)(('X' << 8) | 'Y')); + t.Bank.HoldingRegisters[13].ShouldBe((ushort)('Z' << 8)); + // Round trip through the read path. + (await drv.ReadAsync(["S"], TestContext.Current.CancellationToken))[0].Value.ShouldBe(new[] { "AB", "XYZ" }); + } + + [Fact] + public async Task BitInRegister_array_write_sets_only_the_addressed_bit_of_each_register() + { + var tag = new ModbusTagDefinition("B", ModbusRegion.HoldingRegisters, 30, ModbusDataType.BitInRegister, + BitIndex: 4, ArrayCount: 3); + var (drv, t) = await StartAsync(tag); + await using var _ = drv; + t.Bank.HoldingRegisters[30] = 0xFFFF; + t.Bank.HoldingRegisters[31] = 0x0001; + t.Bank.HoldingRegisters[32] = 0x8000; + + var w = await drv.WriteAsync([new WriteRequest("B", new object[] { false, true, 1 })], TestContext.Current.CancellationToken); + + w[0].StatusCode.ShouldBe(0u); + t.Bank.HoldingRegisters[30].ShouldBe((ushort)0xFFEF, "bit 4 cleared, neighbours kept"); + t.Bank.HoldingRegisters[31].ShouldBe((ushort)0x0011); + t.Bank.HoldingRegisters[32].ShouldBe((ushort)0x8010); + (await drv.ReadAsync(["B"], TestContext.Current.CancellationToken))[0].Value.ShouldBe(new[] { false, true, true }); + } + + [Theory] + [InlineData("S")] + [InlineData("B")] + public async Task A_length_mismatch_is_BadOutOfRange_and_sends_nothing(string tagName) + { + var (drv, t) = await StartAsync( + new ModbusTagDefinition("S", ModbusRegion.HoldingRegisters, 10, ModbusDataType.String, StringLength: 2, ArrayCount: 2), + new ModbusTagDefinition("B", ModbusRegion.HoldingRegisters, 30, ModbusDataType.BitInRegister, BitIndex: 1, ArrayCount: 3)); + await using var _ = drv; + object value = tagName == "S" ? new[] { "only one" } : new[] { true }; + + var w = await drv.WriteAsync([new WriteRequest(tagName, value)], TestContext.Current.CancellationToken); + + w[0].StatusCode.ShouldBe(0x803C0000u); + t.Sent.Where(s => s.Pdu[0] is 0x06 or 0x10).ShouldBeEmpty(); + } +}