dohertj2/lmxopcua
1
0
Fork 0
Code Issues 43 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(abcip): bit-in-word writes take the parent width from its read

Browse Source 
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
This commit is contained in:
Joseph Doherty
2026-10-22 14:21:43 -04:00
parent 4e9bd9d966
commit cb16ebc75e
2 changed files with 189 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs
+62 -11
View File
@@ -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,
}
/// <summary>
/// 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 <see cref="SemaphoreSlim"/>.
/// Matches the Modbus BitInRegister + FOCAS PMC Bit pattern.
/// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows <c>.n</c> 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 <see cref="SemaphoreSlim"/>. 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.
/// </summary>
private async Task<uint> 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,
}
}
/// <summary>Evicts + disposes a bit-write parent runtime so the next read-modify-write re-creates it.</summary>
private static void EvictParentRuntime(DeviceState device, string parentName)
{
if (device.ParentRuntimes.TryRemove(parentName, out var stale))
{
try { stale.Dispose(); } catch { }
}
}
/// <inheritdoc />
public DriverHealth GetHealth() => _health;
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs
+127
View File
@@ -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;
/// <summary>
/// Review 07 H17: Logix allows a <c>.n</c> bit selector on SINT / INT / DINT (and LINT) hosts, and reads of
/// <c>Flags.3</c> on an INT worked — but the bit write's read-modify-write hard-coded a DINT parent, so its
/// <c>DecodeValue(DInt)</c> 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.
/// </summary>
[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);
}
/// <summary>SINT / INT / DINT hosts: the bit is set, every other bit is preserved, nothing degrades.</summary>
/// <param name="size">The parent's wire width.</param>
/// <param name="before">The parent's value before the write.</param>
/// <param name="bit">The bit written true.</param>
/// <param name="after">The value the parent must be written back with.</param>
[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);
}
/// <summary>A bit past the host's width is refused before anything is written, with no health write.</summary>
[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);
}
/// <summary>A host whose width is no integer's is a type mismatch, refused before writing, no health write.</summary>
[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);
}
/// <summary>
/// End to end over the REAL native runtime against a fake CIP target whose <c>Flags</c> is an INT: the write
/// goes out as ONE INT (type 0xC3, two data bytes) with the bit set and the other bits preserved.
/// </summary>
[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<byte[]?>(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();
}
}
}