From cb16ebc75e757cc1129f56b41900117c5b55dd93 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:21:43 -0400 Subject: [PATCH] fix(abcip): bit-in-word writes take the parent width from its read H17: the .n read-modify-write hard-coded a DINT parent. On an INT or SINT host (both legal in Logix, and reads of Flags.3 there already worked) the DINT decode of a 2-byte buffer threw OutOfBounds, the generic catch evicted the BIT tag's (non-existent) runtime and reported BadCommunicationError + Degraded. The parent width now comes from the parent read's size (1 SINT / 2 INT / 4 DINT / 8 LINT; unknown keeps the DINT assumption). Narrow hosts use unsigned arithmetic so a set top bit encodes without overflow; a bit past the host's width is BadOutOfRange and a non-integer width BadTypeMismatch, both refused before anything is written, warned once, no health write. A transport failure in the RMW now evicts the parent runtime it actually used. Native end-to-end: an INT host is written as ONE INT (type 0xC3, 2 bytes). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 73 ++++++++-- .../AbCipBitInWordWidthTests.cs | 127 ++++++++++++++++++ 2 files changed, 189 insertions(+), 11 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index b48a619c..89f06793 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1374,6 +1374,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, continue; } + string? rmwParent = null; try { var parsedPath = AbCipTagPath.TryParse(def.TagPath); @@ -1384,6 +1385,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // losing one another's update. if (def.DataType == AbCipDataType.Bool && parsedPath?.BitIndex is int bit) { + rmwParent = (parsedPath with { BitIndex = null }).ToLibplctagName(); results[i] = new WriteResult( await WriteBitInDIntAsync(device, parsedPath, bit, w.Value, cancellationToken) .ConfigureAwait(false)); @@ -1542,8 +1544,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { - // Transport / wire error — evict so the next write creates a fresh handle. - EvictRuntime(device, def.Name); + // Transport / wire error — evict so the next write creates a fresh handle (for a bit write, the + // parent runtime the read-modify-write used; the bit tag itself holds none). + if (rmwParent is not null) EvictParentRuntime(device, rmwParent); + else EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); _logger.LogWarning(ex, @@ -1556,10 +1560,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// - /// Read-modify-write one bit within a DINT parent. Creates / reuses a parallel - /// parent-DINT runtime (distinct from the bit-selector handle) + serialises concurrent - /// writers against the same parent via a per-parent . - /// Matches the Modbus BitInRegister + FOCAS PMC Bit pattern. + /// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows .n on + /// each). Creates / reuses a parallel parent runtime (distinct from the bit-selector handle) + serialises + /// concurrent writers against the same parent via a per-parent . The parent's + /// width comes from the size its read returned (H17 — a hard-coded DINT decode threw OutOfBounds on an INT + /// host and was reported as a comms fault). Matches the Modbus BitInRegister + FOCAS PMC Bit pattern. /// private async Task WriteBitInDIntAsync( DeviceState device, AbCipTagPath bitPath, int bit, object? value, CancellationToken ct) @@ -1576,12 +1581,49 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var readStatus = parentRuntime.GetStatus(); if (readStatus != 0) return AbCipStatusMapper.MapStatus(readStatus); - var current = Convert.ToInt32(parentRuntime.DecodeValue(AbCipDataType.DInt, bitIndex: null) ?? 0); - var updated = Convert.ToBoolean(value) - ? current | (1 << bit) - : current & ~(1 << bit); + // Unsigned at the narrow widths so a set top bit (SINT bit 7, INT bit 15) encodes without overflowing the + // signed type; DINT keeps its signed int arithmetic (1 << 31 is int.MinValue, which a DINT encodes as is). + var size = parentRuntime.GetSize(); + AbCipDataType? hostType = size switch + { + 1 => AbCipDataType.USInt, + 2 => AbCipDataType.UInt, + 8 => AbCipDataType.LInt, + 4 or <= 0 => AbCipDataType.DInt, // <= 0: the runtime cannot say — the historical DINT assumption + _ => null, + }; + if (hostType is not { } type) + { + WarnTypeMismatchOnce(parentName, + "AbCip bit write to {Parent}.{Bit} on device {Device} refused: the parent is {Size} byte(s), not an integer host (logged once per tag)", + parentName, bit, device.Options.HostAddress, size); + return AbCipStatusMapper.BadTypeMismatch; + } + var widthBits = type switch { AbCipDataType.USInt => 8, AbCipDataType.UInt => 16, AbCipDataType.LInt => 64, _ => 32 }; + if (bit >= widthBits) + { + WarnTypeMismatchOnce(parentName, + "AbCip bit write to {Parent}.{Bit} on device {Device} refused: the parent holds only {Bits} bits (logged once per tag)", + parentName, bit, device.Options.HostAddress, widthBits); + return AbCipStatusMapper.BadOutOfRange; + } - parentRuntime.EncodeValue(AbCipDataType.DInt, bitIndex: null, updated); + var set = Convert.ToBoolean(value); + var decoded = parentRuntime.DecodeValue(type, bitIndex: null) ?? 0; + object updated; + if (type == AbCipDataType.LInt) + { + var current = Convert.ToInt64(decoded); + updated = set ? current | (1L << bit) : current & ~(1L << bit); + } + else + { + var mask = type switch { AbCipDataType.USInt => 0xFF, AbCipDataType.UInt => 0xFFFF, _ => -1 }; + var current = Convert.ToInt32(decoded) & mask; + updated = (set ? current | (1 << bit) : current & ~(1 << bit)) & mask; + } + + parentRuntime.EncodeValue(type, bitIndex: null, updated); await parentRuntime.WriteAsync(ct).ConfigureAwait(false); var writeStatus = parentRuntime.GetStatus(); return writeStatus == 0 @@ -1763,6 +1805,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } + /// Evicts + disposes a bit-write parent runtime so the next read-modify-write re-creates it. + private static void EvictParentRuntime(DeviceState device, string parentName) + { + if (device.ParentRuntimes.TryRemove(parentName, out var stale)) + { + try { stale.Dispose(); } catch { } + } + } + /// public DriverHealth GetHealth() => _health; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs new file mode 100644 index 00000000..11f7b736 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs @@ -0,0 +1,127 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H17: Logix allows a .n bit selector on SINT / INT / DINT (and LINT) hosts, and reads of +/// Flags.3 on an INT worked — but the bit write's read-modify-write hard-coded a DINT parent, so its +/// DecodeValue(DInt) on a 2-byte buffer threw OutOfBounds and the write was reported as a comms fault +/// (BadCommunicationError + Degraded). The parent width now comes from the parent read's size. +/// +[Trait("Category", "Unit")] +public sealed class AbCipBitInWordWidthTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory)> BuildAsync(int parentSize, object parentValue, string bitPath) + { + var factory = new FakeAbCipTagFactory + { + Customise = p => new FakeAbCipTag(p) { Value = parentValue, Size = parentSize }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Bit", Device, bitPath, AbCipDataType.Bool)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-bit", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + /// SINT / INT / DINT hosts: the bit is set, every other bit is preserved, nothing degrades. + /// The parent's wire width. + /// The parent's value before the write. + /// The bit written true. + /// The value the parent must be written back with. + [Theory] + [InlineData(1, 0x81, 3, 0x89)] // SINT 0x81 (-127): bit 7 set stays set, written as its unsigned byte + [InlineData(1, 0x00, 7, 0x80)] // SINT top bit: 128 would overflow an sbyte encode + [InlineData(2, 0x8001, 3, 0x8009)] // INT top bit preserved + [InlineData(2, 0x0000, 15, 0x8000)] // INT top bit: 32768 would overflow a short encode + [InlineData(4, 0x00000001, 3, 0x00000009)] + public async Task A_bit_write_uses_the_parent_width(int size, int before, int bit, int after) + { + var (drv, factory) = await BuildAsync(size, before, $"Motor.Flags.{bit}"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + Convert.ToInt64(factory.Tags["Motor.Flags"].Value).ShouldBe(after); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A bit past the host's width is refused before anything is written, with no health write. + [Fact] + public async Task A_bit_past_the_host_width_is_BadOutOfRange_and_nothing_is_written() + { + var (drv, factory) = await BuildAsync(2, 0, "Motor.Flags.20"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A host whose width is no integer's is a type mismatch, refused before writing, no health write. + [Fact] + public async Task A_host_of_no_integer_width_is_BadTypeMismatch() + { + var (drv, factory) = await BuildAsync(3, 0, "Motor.Flags.1"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// End to end over the REAL native runtime against a fake CIP target whose Flags is an INT: the write + /// goes out as ONE INT (type 0xC3, two data bytes) with the bit set and the other bits preserved. + /// + [Fact] + public async Task A_native_bit_write_on_an_INT_host_writes_an_INT() + { + await using var target = new FakeCipTarget(); + // Read Tag Fragmented (0x52) → INT (0x00C3) 0x8001; Write Tag (0x4D) → success. + target.ConnectedHandler = req => Task.FromResult(req[0] switch + { + 0x52 => [0xD2, 0x00, 0x00, 0x00, 0xC3, 0x00, 0x01, 0x80], + 0x4D => [0xCD, 0x00, 0x00, 0x00], + _ => target.ServiceReply(req), + }); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Bit", host, "Flags.3", AbCipDataType.Bool)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "abcip-bit-native", new NativeAbCipTagFactory(new DeviceSessionPool())); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var write = target.ConnectedCips.Single(c => c[0] == 0x4D); + var typeAt = 2 + write[1] * 2; + write[typeAt].ShouldBe((byte)0xC3); // INT, not DINT + write.Length.ShouldBe(typeAt + 2 + 2 + 2); // type, element count, ONE 2-byte element + write[^2].ShouldBe((byte)0x09); // 0x8001 | bit 3 + write[^1].ShouldBe((byte)0x80); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await drv.DisposeAsync(); + } + } +}