fix(twincat): BOOL-within-word writes RMW the parent at its own width (H18)
The bit write read and wrote the parent word as a 32-bit UDInt with no parent check: a bit beyond a 16-bit parent was written into neighbouring bytes, and a signed parent with its sign bit set read back >= 2^31, so the checked conversion turned any bit write into BadOutOfRange. The write now goes through ITwinCATClient.WriteBitAsync: the ADS client resolves the parent symbol, checks the bit against it (TwinCATBitAccess. CheckParent, as the notification path already did), reads exactly parent.ByteSize raw bytes, flips one bit (TwinCATBitAccess.ApplyBitWrite) and writes the bytes back by index group/offset or variable handle. A bit beyond the parent is BadOutOfRange, a non-integer parent BadTypeMismatch, a short parent read BadCommunicationError (never written back), all without a health write. The per-parent RMW lock stays in the driver. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
@@ -136,6 +136,40 @@ internal class FakeTwinCATClient : ITwinCATClient
|
||||
return Task.FromResult(status);
|
||||
}
|
||||
|
||||
/// <summary>Raw bytes of bit-write parent words, by parent path (#739 H18). The byte count is the parent's width.</summary>
|
||||
public Dictionary<string, byte[]> ParentWords { get; } = new(StringComparer.OrdinalIgnoreCase);
|
||||
/// <summary>Optional parent shapes; absent = an unsigned integer word as wide as its <see cref="ParentWords"/> entry.</summary>
|
||||
public Dictionary<string, TwinCATPlcSymbolShape> ParentShapes { get; } = new(StringComparer.OrdinalIgnoreCase);
|
||||
/// <summary>Gets the log of every <see cref="WriteBitAsync"/> call.</summary>
|
||||
public List<(string parent, int bit, bool value)> BitWriteLog { get; } = new();
|
||||
/// <summary>When set, <see cref="WriteBitAsync"/> yields between its read and its write so a missing
|
||||
/// per-parent lock in the driver loses updates.</summary>
|
||||
public bool YieldInBitWrite { get; set; }
|
||||
|
||||
/// <summary>Simulates the parent-width bit RMW with the driver's own <see cref="TwinCATBitAccess.ApplyBitWrite"/>.</summary>
|
||||
/// <param name="parentPath">The parent word's symbol path.</param>
|
||||
/// <param name="bit">The bit index.</param>
|
||||
/// <param name="value">The bit value.</param>
|
||||
/// <param name="ct">The cancellation token.</param>
|
||||
/// <returns>The write status.</returns>
|
||||
public virtual async Task<uint> WriteBitAsync(string parentPath, int bit, bool value, CancellationToken ct)
|
||||
{
|
||||
if (ThrowOnWrite) throw Exception ?? new InvalidOperationException();
|
||||
lock (BitWriteLog) BitWriteLog.Add((parentPath, bit, value));
|
||||
if (ReadStatuses.TryGetValue(parentPath, out var readStatus) && readStatus != TwinCATStatusMapper.Good)
|
||||
return readStatus;
|
||||
byte[] current;
|
||||
lock (ParentWords) current = ParentWords.TryGetValue(parentPath, out var w) ? [.. w] : [];
|
||||
var shape = ParentShapes.TryGetValue(parentPath, out var sh)
|
||||
? sh
|
||||
: new TwinCATPlcSymbolShape(TwinCATPlcKind.Scalar, current.Length == 0 ? 4 : current.Length, false, "UDINT");
|
||||
if (YieldInBitWrite) await Task.Yield();
|
||||
var status = TwinCATBitAccess.ApplyBitWrite(shape, current, bit, value, out var updated);
|
||||
if (status != TwinCATStatusMapper.Good) return status;
|
||||
lock (ParentWords) ParentWords[parentPath] = updated;
|
||||
return WriteStatuses.TryGetValue(parentPath, out var s) ? s : TwinCATStatusMapper.Good;
|
||||
}
|
||||
|
||||
/// <summary>Simulates probing the connection status.</summary>
|
||||
/// <param name="ct">The cancellation token.</param>
|
||||
/// <returns>A task that returns the probe result.</returns>
|
||||
|
||||
@@ -55,4 +55,50 @@ public sealed class TwinCATBitAccessTests
|
||||
TwinCATBitAccess.CheckParent(0, TwinCATPlcSymbolShape.Classify("ST_X", DataTypeCategory.Struct, 8)).ShouldNotBeNull();
|
||||
TwinCATBitAccess.CheckParent(31, TwinCATPlcSymbolShape.Classify("DWORD", DataTypeCategory.Primitive, 4)).ShouldBeNull();
|
||||
}
|
||||
|
||||
// ---- #739 H18: the bit WRITE is a read-modify-write at the parent's own width ----
|
||||
|
||||
/// <summary>A bit write flips exactly one bit of the parent's raw bytes at the parent's width.</summary>
|
||||
[Fact]
|
||||
public void ApplyBitWrite_flips_one_bit_at_the_parent_width()
|
||||
{
|
||||
var word = TwinCATPlcSymbolShape.Classify("WORD", DataTypeCategory.Primitive, 2);
|
||||
TwinCATBitAccess.ApplyBitWrite(word, [0x01, 0x00], 9, true, out var set).ShouldBe(TwinCATStatusMapper.Good);
|
||||
set.ShouldBe(new byte[] { 0x01, 0x02 });
|
||||
TwinCATBitAccess.ApplyBitWrite(word, [0xFF, 0xFF], 3, false, out var cleared).ShouldBe(TwinCATStatusMapper.Good);
|
||||
cleared.ShouldBe(new byte[] { 0xF7, 0xFF });
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A signed parent with its sign bit set is plain bytes — the old UDInt round-trip read it back as
|
||||
/// a value ≥ 2³¹ and a checked conversion turned ANY bit write into BadOutOfRange.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ApplyBitWrite_on_a_negative_signed_parent_preserves_the_sign_bit()
|
||||
{
|
||||
var intWord = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, 2);
|
||||
TwinCATBitAccess.ApplyBitWrite(intWord, [0x00, 0x80], 0, true, out var updated).ShouldBe(TwinCATStatusMapper.Good);
|
||||
updated.ShouldBe(new byte[] { 0x01, 0x80 });
|
||||
}
|
||||
|
||||
/// <summary>A bit beyond the parent is BadOutOfRange; a non-integer parent BadTypeMismatch; nothing is produced.</summary>
|
||||
[Fact]
|
||||
public void ApplyBitWrite_refuses_a_bit_the_parent_does_not_have()
|
||||
{
|
||||
var word = TwinCATPlcSymbolShape.Classify("UINT", DataTypeCategory.Primitive, 2);
|
||||
TwinCATBitAccess.ApplyBitWrite(word, [0, 0], 16, true, out var none).ShouldBe(TwinCATStatusMapper.BadOutOfRange);
|
||||
none.ShouldBeEmpty();
|
||||
var real = TwinCATPlcSymbolShape.Classify("REAL", DataTypeCategory.Primitive, 4);
|
||||
TwinCATBitAccess.ApplyBitWrite(real, [0, 0, 0, 0], 1, true, out _).ShouldBe(TwinCATStatusMapper.BadTypeMismatch);
|
||||
}
|
||||
|
||||
/// <summary>A parent read shorter than the parent's width is never written back (it would zero bytes).</summary>
|
||||
[Fact]
|
||||
public void ApplyBitWrite_refuses_a_short_parent_buffer()
|
||||
{
|
||||
var dword = TwinCATPlcSymbolShape.Classify("DWORD", DataTypeCategory.Primitive, 4);
|
||||
TwinCATBitAccess.ApplyBitWrite(dword, [0xFF, 0xFF], 1, true, out var none)
|
||||
.ShouldBe(TwinCATStatusMapper.BadCommunicationError);
|
||||
none.ShouldBeEmpty();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -296,23 +296,23 @@ public sealed class TwinCATReadWriteTests
|
||||
factory.Clients[0].DisposeCount.ShouldBe(1);
|
||||
}
|
||||
|
||||
// ---- BOOL-within-word RMW writes ----
|
||||
// ---- BOOL-within-word RMW writes (#739 H18: at the parent's own width, on raw bytes) ----
|
||||
|
||||
/// <summary>A BOOL-within-word set reads the parent word, ORs the bit, writes it back as UDInt.</summary>
|
||||
/// <summary>A BOOL-within-word set read-modify-writes the parent word's raw bytes through WriteBitAsync.</summary>
|
||||
[Fact]
|
||||
public async Task Bit_set_RMWs_parent_word_as_UDInt()
|
||||
public async Task Bit_set_RMWs_the_parent_word_bytes()
|
||||
{
|
||||
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 } };
|
||||
factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Flags"] = [0b0001, 0, 0, 0] } };
|
||||
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);
|
||||
factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0b1001, 0, 0, 0 });
|
||||
factory.Clients[0].BitWriteLog.ShouldBe([("MAIN.Flags", 3, true)]);
|
||||
factory.Clients[0].WriteLog.ShouldBeEmpty(); // no 32-bit UDInt round-trip any more
|
||||
}
|
||||
|
||||
/// <summary>A BOOL-within-word clear preserves the other bits in the parent word.</summary>
|
||||
@@ -321,12 +321,12 @@ public sealed class TwinCATReadWriteTests
|
||||
{
|
||||
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 } };
|
||||
factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Flags"] = [0xFF, 0xFF] } };
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
|
||||
await drv.WriteAsync([new WriteRequest("Flag", false)], CancellationToken.None);
|
||||
|
||||
factory.Clients[0].Values["MAIN.Flags"].ShouldBe(0xFFF7u);
|
||||
factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xF7, 0xFF });
|
||||
}
|
||||
|
||||
/// <summary>RMW works on a DWORD parent (bit 20 set above the 16-bit boundary).</summary>
|
||||
@@ -335,12 +335,31 @@ public sealed class TwinCATReadWriteTests
|
||||
{
|
||||
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 } };
|
||||
factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["GVL.Status"] = [0, 0, 0, 0] } };
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
|
||||
await drv.WriteAsync([new WriteRequest("Hi", true)], CancellationToken.None);
|
||||
|
||||
factory.Clients[0].Values["GVL.Status"].ShouldBe(1u << 20);
|
||||
BitConverter.ToUInt32(factory.Clients[0].ParentWords["GVL.Status"]).ShouldBe(1u << 20);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A bit beyond a 16-bit parent is refused BadOutOfRange and nothing is written — before, the hardcoded
|
||||
/// 32-bit RMW wrote into the neighbouring bytes. No health write: a refusal is not a comms fault.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task Bit_beyond_a_16_bit_parent_is_refused_without_writing()
|
||||
{
|
||||
var (drv, factory) = NewDriver(
|
||||
new TwinCATTagDefinition("Hi", "ads://5.23.91.23.1.1:851", "MAIN.Word.20", TwinCATDataType.Bool));
|
||||
factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Word"] = [0, 0] } };
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
|
||||
var results = await drv.WriteAsync([new WriteRequest("Hi", true)], CancellationToken.None);
|
||||
|
||||
results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange);
|
||||
factory.Clients[0].ParentWords["MAIN.Word"].ShouldBe(new byte[] { 0, 0 });
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
}
|
||||
|
||||
/// <summary>A failed parent read short-circuits the RMW and surfaces the read status.</summary>
|
||||
@@ -351,6 +370,7 @@ public sealed class TwinCATReadWriteTests
|
||||
new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool));
|
||||
factory.Customise = () => new FakeTwinCATClient
|
||||
{
|
||||
ParentWords = { ["MAIN.Flags"] = [0xAA, 0] },
|
||||
ReadStatuses = { ["MAIN.Flags"] = TwinCATStatusMapper.BadNodeIdUnknown },
|
||||
};
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
@@ -358,25 +378,24 @@ public sealed class TwinCATReadWriteTests
|
||||
var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None);
|
||||
|
||||
results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown);
|
||||
factory.Clients[0].WriteLog.ShouldBeEmpty();
|
||||
factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xAA, 0 });
|
||||
}
|
||||
|
||||
/// <summary>A Good parent read that yields a null value must NOT zero the word — it surfaces
|
||||
/// a Bad status and writes nothing (treating null as 0 would clear every set bit).</summary>
|
||||
/// <summary>A Good parent read that yields no bytes must NOT zero the word — it surfaces a Bad status
|
||||
/// and writes nothing (treating it as 0 would clear every set bit).</summary>
|
||||
[Fact]
|
||||
public async Task Bit_write_with_null_parent_value_does_not_zero_the_word()
|
||||
public async Task Bit_write_with_no_parent_bytes_does_not_zero_the_word()
|
||||
{
|
||||
var (drv, factory) = NewDriver(
|
||||
new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool));
|
||||
// Do NOT seed Values["MAIN.Flags"] — the fake returns (null, Good) for the parent read.
|
||||
// Do NOT seed ParentWords["MAIN.Flags"] — the fake answers the parent read Good with no bytes.
|
||||
factory.Customise = () => new FakeTwinCATClient();
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
|
||||
var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None);
|
||||
|
||||
results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError);
|
||||
// No parent write attempted — the word is left untouched on the device.
|
||||
factory.Clients[0].WriteLog.ShouldBeEmpty();
|
||||
factory.Clients[0].ParentWords.ShouldNotContainKey("MAIN.Flags");
|
||||
}
|
||||
|
||||
/// <summary>Concurrent bit writes to the same word compose correctly (per-parent lock).</summary>
|
||||
@@ -387,12 +406,12 @@ public sealed class TwinCATReadWriteTests
|
||||
.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 } };
|
||||
factory.Customise = () => new FakeTwinCATClient { YieldInBitWrite = true, ParentWords = { ["MAIN.Flags"] = [0, 0] } };
|
||||
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);
|
||||
factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xFF, 0 });
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user