diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index a19da231..38895924 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -175,7 +175,9 @@ internal sealed class AdsTwinCATClient : ITwinCATClient /// /// The ADS symbol path to write to. /// The TwinCAT data type. - /// Optional bit index for BOOL values (not supported for writes). + /// Optional bit index for BOOL values. BOOL-within-word writes are handled + /// upstream by as a parent-word read-modify-write, so a + /// bit index does not reach this method on the write path. /// The value to write. /// The cancellation token. /// The OPC UA status code of the write operation. @@ -186,10 +188,6 @@ internal sealed class AdsTwinCATClient : ITwinCATClient object? value, CancellationToken cancellationToken) { - if (bitIndex is int && type == TwinCATDataType.Bool) - throw new NotSupportedException( - "BOOL-within-word writes require read-modify-write; tracked in task #181."); - try { var converted = ConvertForWrite(type, value); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index 4baa97b3..3d5747ba 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -28,6 +28,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private readonly ConcurrentDictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); + // Per-parent-word RMW gate for BOOL-within-word writes: a single-bit write is a + // read-modify-write of the parent word (TwinCAT's symbol table doesn't expose "Word.N"), + // so concurrent bit-writers to the same word must serialise or they lose each other's + // updates. Keyed by device + parent symbol. (Cannot guard against the PLC program writing + // the word between our read and write — inherent to RMW.) + private readonly ConcurrentDictionary _bitRmwLocks = + new(StringComparer.OrdinalIgnoreCase); + // Resolves a read/write/subscribe fullReference to a tag definition, bridging the two // authoring models: an authored tag-table entry (by name) OR an equipment tag whose // reference is its raw TagConfig JSON (parsed once via TwinCATEquipmentTagParser, cached). @@ -293,6 +301,40 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var parsed = TwinCATSymbolPath.TryParse(def.SymbolPath); + + // BOOL-within-word write — read-modify-write of the parent word. Mirrors the bit-read + // (AdsTwinCATClient.ReadValueAsync) which reads the parent as uint: read the parent as + // UDInt (-> uint), flip the bit, write it back, all under a per-parent lock. + if (def.DataType == TwinCATDataType.Bool && parsed?.BitIndex is int bit) + { + var parentPath = (parsed with { BitIndex = null }).ToAdsSymbolName(); + var gate = _bitRmwLocks.GetOrAdd( + $"{def.DeviceHostAddress}|{parentPath}", static _ => new SemaphoreSlim(1, 1)); + await gate.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + var (parentValue, readStatus) = await client.ReadValueAsync( + parentPath, TwinCATDataType.UDInt, null, null, cancellationToken).ConfigureAwait(false); + if (readStatus != TwinCATStatusMapper.Good) + { + results[i] = new WriteResult(readStatus); + } + else + { + var word = Convert.ToUInt32(parentValue ?? 0u); + var updated = Convert.ToBoolean(w.Value) ? word | (1u << bit) : word & ~(1u << bit); + var writeStatus = await client.WriteValueAsync( + parentPath, TwinCATDataType.UDInt, null, updated, cancellationToken).ConfigureAwait(false); + results[i] = new WriteResult(writeStatus); + } + } + finally + { + gate.Release(); + } + continue; + } + var symbolName = parsed?.ToAdsSymbolName() ?? def.SymbolPath; var status = await client.WriteValueAsync( symbolName, def.DataType, parsed?.BitIndex, w.Value, cancellationToken).ConfigureAwait(false); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs index e97d9409..aef00412 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs @@ -266,4 +266,86 @@ public sealed class TwinCATReadWriteTests factory.Clients[0].DisposeCount.ShouldBe(1); } + + // ---- BOOL-within-word RMW writes ---- + + /// A BOOL-within-word set reads the parent word, ORs the bit, writes it back as UDInt. + [Fact] + public async Task Bit_set_RMWs_parent_word_as_UDInt() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); + factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0b0001u } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + factory.Clients[0].Values["MAIN.Flags"].ShouldBe(0b1001u); + factory.Clients[0].WriteLog.ShouldContain(e => + e.symbol == "MAIN.Flags" && e.type == TwinCATDataType.UDInt && e.bit == null); + } + + /// A BOOL-within-word clear preserves the other bits in the parent word. + [Fact] + public async Task Bit_clear_preserves_other_bits() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); + factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0xFFFFu } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.WriteAsync([new WriteRequest("Flag", false)], CancellationToken.None); + + factory.Clients[0].Values["MAIN.Flags"].ShouldBe(0xFFF7u); + } + + /// RMW works on a DWORD parent (bit 20 set above the 16-bit boundary). + [Fact] + public async Task Bit_set_on_DWORD_parent_sets_high_bit() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Hi", "ads://5.23.91.23.1.1:851", "GVL.Status.20", TwinCATDataType.Bool)); + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Status"] = 0u } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.WriteAsync([new WriteRequest("Hi", true)], CancellationToken.None); + + factory.Clients[0].Values["GVL.Status"].ShouldBe(1u << 20); + } + + /// A failed parent read short-circuits the RMW and surfaces the read status. + [Fact] + public async Task Bit_write_surfaces_parent_read_failure() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); + factory.Customise = () => new FakeTwinCATClient + { + ReadStatuses = { ["MAIN.Flags"] = TwinCATStatusMapper.BadNodeIdUnknown }, + }; + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + factory.Clients[0].WriteLog.ShouldBeEmpty(); + } + + /// Concurrent bit writes to the same word compose correctly (per-parent lock). + [Fact] + public async Task Concurrent_bit_writes_to_same_word_compose_correctly() + { + var tags = Enumerable.Range(0, 8) + .Select(b => new TwinCATTagDefinition($"Bit{b}", "ads://5.23.91.23.1.1:851", $"MAIN.Flags.{b}", TwinCATDataType.Bool)) + .ToArray(); + var (drv, factory) = NewDriver(tags); + factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0u } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(b => + drv.WriteAsync([new WriteRequest($"Bit{b}", true)], CancellationToken.None))); + + Convert.ToUInt32(factory.Clients[0].Values["MAIN.Flags"]).ShouldBe(0xFFu); + } }