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
2026-10-22 15:26:59 -04:00
parent 2ef3cd0df2
commit 1adc9d65e1
3 changed files with 146 additions and 5 deletions
@@ -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})"),
    };

+    /// <summary>
+    ///     <c>BitInRegister[]</c> write: element i is bit <c>BitIndex</c> of holding register
+    ///     <c>Address + i</c> (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.
+    /// </summary>
+    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<SemaphoreSlim>(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();
+        }
+    }
+
    /// <summary>
    ///     Read-modify-write one bit in a holding register. FC03 → bit-swap → FC06. Serialised
    ///     against other bit writes targeting the same register via <see cref="GetRmwLock"/>.
@@ -35,10 +35,8 @@ public sealed class ModbusArrayWriteSupportParityTests
    /// </summary>
    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<ModbusRegion, ModbusDataType> PendingShapes()
    {
@@ -0,0 +1,82 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests;
+
+/// <summary>
+///     Review 07 U3: <c>String[]</c> and <c>BitInRegister[]</c> tags read fine; their writes used to fail
+///     <c>BadInternalError</c> (no element width for String, <c>Convert.ToBoolean(bool[])</c> for the bit
+///     array). Both now encode <c>count</c> elements at the tag's element width; a length mismatch is
+///     <c>BadOutOfRange</c>.
+/// </summary>
+[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();
+    }
+}