diff --git a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index 2043857..b6a0e8d 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -24,6 +24,11 @@ internal sealed class AdsTwinCATClient : ITwinCATClient private readonly AdsClient _client = new(); private readonly ConcurrentDictionary _notifications = new(); + // Per-parent-symbol RMW locks. Keys are bounded by the writable-bit-tag cardinality + // and are intentionally never removed — a leaking-but-bounded dictionary is simpler + // than tracking liveness, matching the AbCip / Modbus / FOCAS pattern from #181. + private readonly ConcurrentDictionary _bitWriteLocks = new(); + public AdsTwinCATClient() { _client.AdsNotificationEx += OnAdsNotificationEx; @@ -75,9 +80,9 @@ 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."); + if (bitIndex is int bit && type == TwinCATDataType.Bool) + return await WriteBitInWordAsync(symbolPath, bit, value, cancellationToken) + .ConfigureAwait(false); try { @@ -94,6 +99,69 @@ internal sealed class AdsTwinCATClient : ITwinCATClient } } + /// + /// Read-modify-write a single bit within an integer parent word. + /// is the bit-selector path (e.g. Flags.3); the parent is the same path with the + /// .N suffix stripped and is read/written as a UDINT — TwinCAT handles narrower + /// parents (BYTE/WORD) implicitly through the UDINT projection. + /// + /// + /// Concurrent bit writers against the same parent are serialised through a per-parent + /// to prevent torn reads/writes. Mirrors the AbCip / Modbus / + /// FOCAS bit-RMW pattern. + /// + private async Task WriteBitInWordAsync( + string symbolPath, int bit, object? value, CancellationToken cancellationToken) + { + var parentPath = TryGetParentSymbolPath(symbolPath); + if (parentPath is null) return TwinCATStatusMapper.BadNotSupported; + + var setBit = Convert.ToBoolean(value); + var rmwLock = _bitWriteLocks.GetOrAdd(parentPath, _ => new SemaphoreSlim(1, 1)); + await rmwLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + var read = await _client.ReadValueAsync(parentPath, typeof(uint), cancellationToken) + .ConfigureAwait(false); + if (read.ErrorCode != AdsErrorCode.NoError) + return TwinCATStatusMapper.MapAdsError((uint)read.ErrorCode); + + var current = Convert.ToUInt32(read.Value ?? 0u); + var updated = ApplyBit(current, bit, setBit); + + var write = await _client.WriteValueAsync(parentPath, updated, cancellationToken) + .ConfigureAwait(false); + return write.ErrorCode == AdsErrorCode.NoError + ? TwinCATStatusMapper.Good + : TwinCATStatusMapper.MapAdsError((uint)write.ErrorCode); + } + catch (AdsErrorException ex) + { + return TwinCATStatusMapper.MapAdsError((uint)ex.ErrorCode); + } + finally + { + rmwLock.Release(); + } + } + + /// + /// Strip the trailing .N bit selector from a TwinCAT symbol path. Returns + /// null when the path has no parent (single segment / leading dot). + /// + internal static string? TryGetParentSymbolPath(string symbolPath) + { + var dot = symbolPath.LastIndexOf('.'); + return dot <= 0 ? null : symbolPath.Substring(0, dot); + } + + /// Set or clear bit in . + internal static uint ApplyBit(uint word, int bit, bool setBit) + { + var mask = 1u << bit; + return setBit ? (word | mask) : (word & ~mask); + } + public async Task ProbeAsync(CancellationToken cancellationToken) { try diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs index 9ff7b01..1594437 100644 --- a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs @@ -41,7 +41,25 @@ internal class FakeTwinCATClient : ITwinCATClient { if (ThrowOnWrite) throw Exception ?? new InvalidOperationException(); WriteLog.Add((symbolPath, type, bitIndex, value)); - Values[symbolPath] = value; + + // Model the parent-word RMW path the production AdsTwinCATClient performs for + // bit-indexed BOOL writes so driver-level tests can assert the resulting parent state. + if (bitIndex is int bit && type == TwinCATDataType.Bool) + { + var parentPath = AdsTwinCATClient.TryGetParentSymbolPath(symbolPath); + if (parentPath is not null) + { + var current = Values.TryGetValue(parentPath, out var p) && p is not null + ? Convert.ToUInt32(p) : 0u; + Values[parentPath] = AdsTwinCATClient.ApplyBit( + current, bit, Convert.ToBoolean(value)); + } + } + else + { + Values[symbolPath] = value; + } + var status = WriteStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good; return Task.FromResult(status); } diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitWriteTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitWriteTests.cs new file mode 100644 index 0000000..bee2e02 --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitWriteTests.cs @@ -0,0 +1,105 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +[Trait("Category", "Unit")] +public sealed class TwinCATBitWriteTests +{ + // ---- Helper unit tests ---- + + [Theory] + [InlineData("Flags.3", "Flags")] + [InlineData("MAIN.Status.7", "MAIN.Status")] + [InlineData("GVL.Motors[0].Status.5", "GVL.Motors[0].Status")] + public void TryGetParentSymbolPath_strips_trailing_bit_selector(string input, string expected) + { + AdsTwinCATClient.TryGetParentSymbolPath(input).ShouldBe(expected); + } + + [Theory] + [InlineData("Counter")] // single segment — no parent + [InlineData(".Bad")] // leading dot + public void TryGetParentSymbolPath_returns_null_for_pathless(string input) + { + AdsTwinCATClient.TryGetParentSymbolPath(input).ShouldBeNull(); + } + + [Theory] + [InlineData(0u, 0, true, 0x0000_0001u)] + [InlineData(0u, 31, true, 0x8000_0000u)] + [InlineData(0xFFFF_FFFFu, 0, false, 0xFFFF_FFFEu)] + [InlineData(0xFFFF_FFFFu, 31, false, 0x7FFF_FFFFu)] + [InlineData(0x0000_0008u, 3, true, 0x0000_0008u)] // already set — idempotent + [InlineData(0x0000_0000u, 3, false, 0x0000_0000u)] // already clear — idempotent + public void ApplyBit_sets_or_clears_bit(uint word, int bit, bool setBit, uint expected) + { + AdsTwinCATClient.ApplyBit(word, bit, setBit).ShouldBe(expected); + } + + // ---- Driver-level round-trip ---- + + private static (TwinCATDriver drv, FakeTwinCATClientFactory factory) NewDriver(params TwinCATTagDefinition[] tags) + { + var factory = new FakeTwinCATClientFactory(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions("ads://5.23.91.23.1.1:851")], + Tags = tags, + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", factory); + return (drv, factory); + } + + [Fact] + public async Task Bit_write_sets_bit_in_parent_word() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("FlagBit3", "ads://5.23.91.23.1.1:851", "Flags.3", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", CancellationToken.None); + // Parent word starts at 0. + factory.Customise = () => new FakeTwinCATClient { Values = { ["Flags"] = 0u } }; + + var results = await drv.WriteAsync( + [new Core.Abstractions.WriteRequest("FlagBit3", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + // Parent should now have bit 3 set. + Convert.ToUInt32(factory.Clients[0].Values["Flags"]!).ShouldBe(0x0000_0008u); + } + + [Fact] + public async Task Bit_write_clears_bit_without_disturbing_neighbours() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("FlagBit3", "ads://5.23.91.23.1.1:851", "Flags.3", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", CancellationToken.None); + // Parent word has bits 0, 3, 7 set initially. + factory.Customise = () => new FakeTwinCATClient { Values = { ["Flags"] = 0x0000_0089u } }; + + var results = await drv.WriteAsync( + [new Core.Abstractions.WriteRequest("FlagBit3", false)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + // Bit 3 cleared; bits 0 and 7 untouched. + Convert.ToUInt32(factory.Clients[0].Values["Flags"]!).ShouldBe(0x0000_0081u); + } + + [Fact] + public async Task Bit_write_does_not_throw_NotSupported() + { + // Regression: AdsTwinCATClient previously threw NotSupportedException for bit writes. + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Bit", "ads://5.23.91.23.1.1:851", "GVL.Word.0", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Word"] = 0u } }; + + var results = await drv.WriteAsync( + [new Core.Abstractions.WriteRequest("Bit", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + // Status should be Good, not BadNotSupported (which is what the catch block produced). + results.Single().StatusCode.ShouldNotBe(TwinCATStatusMapper.BadNotSupported); + } +}