fix(abcip): review fix round 1 - BOOL-array fallback, local refusals, minors
I1: the U11 Bits[n] -> Bits[n/32] rewrite was unconditional. The native runtime now keeps the original element path until the first reply settles the layout: 0xD3 confirms DWORD addressing (bit n%32); any other reply type (e.g. a 0xC1 per-element BOOL array) or a refusal of the DWORD index falls back to Bits[n] and re-reads, and a write settles the layout by a read first, so it can never decode or write element n/32 of an element-addressed array. I2: libplctag ErrorNoMatch/ErrorBadDevice no longer fold into NotFound (they now read as BadConnection: comms, Degraded, LastSuccessfulRead preserved). ErrorNotImplemented - and the native runtime's own pre-send refusals - raise AbCipLocalRefusalException: BadNotSupported, no health, LastSuccessfulRead NOT advanced, no backoff record, not reachable for the probe or Test Connect. M1: the bit RMW's parent read/write status words follow the health contract (refusal: per-tag Bad + Warning; comms: evict the parent + Degraded). M2: device Unsupported read refusals return Refused (advance LastSuccessfulRead); device write refusals call RecordWriteAnswered (not the pre-send native type gate). M3: a good write clears a Degraded a write comms fault recorded (a read-side Degraded is still left for the read path). M4: a superseded init throws AbCipInitializationSupersededException (not an OCE); ShutdownAsync stamps Unknown only if no newer init published meanwhile. M5: the sync-Dispose timing assert is 2 s against a 4 s probe read; the unencodable-array test asserts health. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
@@ -123,6 +123,7 @@ public sealed class AbCipArrayWriteTests
|
||||
|
||||
r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported);
|
||||
factory.Tags.ShouldNotContainKey("Arr");
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy); // M5: a refusal writes no health
|
||||
}
|
||||
|
||||
/// <summary>The controller holding a different shape than authored (size ≠ width × count) is BadTypeMismatch, nothing sent.</summary>
|
||||
|
||||
@@ -124,4 +124,38 @@ public sealed class AbCipBitInWordWidthTests
|
||||
await drv.DisposeAsync();
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// M1: a parent read that fails with a COMMS status evicts the parent runtime and degrades (LastSuccessfulRead
|
||||
/// preserved); one the controller REFUSES is per-tag Bad, keeps the runtime and writes no health.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task A_failed_parent_read_follows_the_health_contract()
|
||||
{
|
||||
var (drv, factory) = await BuildAsync(4, 0, "Motor.Flags.3");
|
||||
factory.Customise = p => new FakeAbCipTag(p) { Status = (int)AbStatus.BadConnection };
|
||||
var before = drv.GetHealth();
|
||||
|
||||
var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None);
|
||||
|
||||
r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError);
|
||||
factory.Tags["Motor.Flags"].Disposed.ShouldBeTrue();
|
||||
factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0);
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Degraded);
|
||||
drv.GetHealth().LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead);
|
||||
}
|
||||
|
||||
/// <summary>M1 positive control: a refused parent read keeps the runtime, no health write.</summary>
|
||||
[Fact]
|
||||
public async Task A_refused_parent_read_keeps_the_runtime_and_health()
|
||||
{
|
||||
var (drv, factory) = await BuildAsync(4, 0, "Motor.Flags.3");
|
||||
factory.Customise = p => new FakeAbCipTag(p) { Status = (int)AbStatus.NotAllowed };
|
||||
|
||||
var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None);
|
||||
|
||||
r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable);
|
||||
factory.Tags["Motor.Flags"].Disposed.ShouldBeFalse();
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -225,4 +225,74 @@ public sealed class AbCipDeviceRefusalTests
|
||||
{
|
||||
public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// #737 I2: a LOCAL refusal (libplctag ErrorNotImplemented — no I/O) is per-tag BadNotSupported, keeps health
|
||||
/// state, and does NOT advance LastSuccessfulRead (nothing proves the device answered). Positive control: the
|
||||
/// device's own Unsupported refusal does advance it (M2).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task A_local_refusal_does_not_advance_LastSuccessfulRead_but_a_device_one_does()
|
||||
{
|
||||
var (drv, _) = Build(p => p.TagName == "Arr[999]"
|
||||
? new LocalRefusingTag(p)
|
||||
: new RefusingTag(p) { ReadThrows = AbStatus.Unsupported });
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
var before = drv.GetHealth().LastSuccessfulRead!.Value;
|
||||
await Task.Delay(20);
|
||||
|
||||
(await drv.ReadAsync(["Bad"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported);
|
||||
drv.GetHealth().LastSuccessfulRead.ShouldBe(before);
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
|
||||
(await drv.ReadAsync(["Good"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported);
|
||||
drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(before);
|
||||
}
|
||||
|
||||
/// <summary>#737 I2: a local refusal at create opens no backoff — a sibling cold create still connects.</summary>
|
||||
[Fact]
|
||||
public async Task A_local_refusal_at_create_does_not_throttle_or_count_as_answered()
|
||||
{
|
||||
var (drv, _) = Build(p => p.TagName == "Arr[999]"
|
||||
? new FakeAbCipTag(p) { ThrowOnInitialize = true, Exception = new AbCipLocalRefusalException("ErrorNotImplemented") }
|
||||
: new FakeAbCipTag(p) { Value = 3 });
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
|
||||
var r = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None);
|
||||
|
||||
r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported);
|
||||
r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good);
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
}
|
||||
|
||||
/// <summary>M2: a write the device refuses proves it answered — LastSuccessfulRead advances.</summary>
|
||||
[Fact]
|
||||
public async Task A_refused_write_advances_LastSuccessfulRead()
|
||||
{
|
||||
var factory = new FakeAbCipTagFactory { Customise = p => new RefusingTag(p) { WriteThrows = AbStatus.NotAllowed } };
|
||||
var drv = new AbCipDriver(new AbCipDriverOptions
|
||||
{
|
||||
Devices = [new AbCipDeviceOptions(Device)],
|
||||
RawTags = AbCipRawTags.From(new AbCipTagDefinition("Ro", Device, "ReadOnlyTag", AbCipDataType.DInt)),
|
||||
Probe = new AbCipProbeOptions { Enabled = false },
|
||||
}, "abcip-refusal", factory);
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
var before = drv.GetHealth().LastSuccessfulRead!.Value;
|
||||
await Task.Delay(20);
|
||||
|
||||
(await drv.WriteAsync([new WriteRequest("Ro", 5)], CancellationToken.None))[0].StatusCode
|
||||
.ShouldBe(AbCipStatusMapper.BadNotWritable);
|
||||
|
||||
drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(before);
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
}
|
||||
|
||||
private sealed class LocalRefusingTag(AbCipTagCreateParams p) : FakeAbCipTag(p)
|
||||
{
|
||||
public override async Task ReadAsync(CancellationToken cancellationToken)
|
||||
{
|
||||
await base.ReadAsync(cancellationToken);
|
||||
throw new AbCipLocalRefusalException("ErrorNotImplemented");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+30
-7
@@ -56,9 +56,9 @@ public sealed class AbCipLifecycleConcurrencyTests
|
||||
return (drv, factory);
|
||||
}
|
||||
|
||||
private static async Task WaitUntilAsync(Func<bool> condition, string what)
|
||||
private static async Task WaitUntilAsync(Func<bool> condition, string what, int seconds = 5)
|
||||
{
|
||||
var deadline = DateTime.UtcNow.AddSeconds(5);
|
||||
var deadline = DateTime.UtcNow.AddSeconds(seconds);
|
||||
while (!condition())
|
||||
{
|
||||
if (DateTime.UtcNow > deadline) throw new TimeoutException(what);
|
||||
@@ -106,7 +106,8 @@ public sealed class AbCipLifecycleConcurrencyTests
|
||||
var init = drv.InitializeAsync("{}", CancellationToken.None);
|
||||
await Task.Delay(50);
|
||||
await drv.ShutdownAsync(CancellationToken.None);
|
||||
await Should.ThrowAsync<OperationCanceledException>(() => init);
|
||||
var superseded = await Should.ThrowAsync<AbCipInitializationSupersededException>(() => init);
|
||||
superseded.ShouldNotBeAssignableTo<OperationCanceledException>(); // M4: distinct from a caller cancellation
|
||||
|
||||
drv.GetDeviceState(Host).ShouldBeNull();
|
||||
drv.GetHostStatuses().ShouldBeEmpty();
|
||||
@@ -199,18 +200,40 @@ public sealed class AbCipLifecycleConcurrencyTests
|
||||
[Fact]
|
||||
public async Task Sync_Dispose_does_not_block_on_the_teardown_but_still_completes_it()
|
||||
{
|
||||
var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(500));
|
||||
var (drv, factory) = Build(probeReadDelay: TimeSpan.FromSeconds(4));
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started");
|
||||
var loop = drv.GetDeviceState(Host)!.ProbeTask!;
|
||||
await Task.Delay(20); // the probe is inside its slow, cancellation-deaf read
|
||||
await Task.Delay(50); // the probe is inside its slow, cancellation-deaf read
|
||||
|
||||
var sw = System.Diagnostics.Stopwatch.StartNew();
|
||||
((IDisposable)drv).Dispose();
|
||||
sw.Stop();
|
||||
|
||||
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(250)); // did not wait out the 500 ms probe read
|
||||
await WaitUntilAsync(() => loop.IsCompleted && factory.Tags["ProbeTag"].Disposed, "teardown never completed");
|
||||
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(2)); // did not wait out the 4 s probe read (CI-tolerant margin)
|
||||
await WaitUntilAsync(() => loop.IsCompleted && factory.Tags["ProbeTag"].Disposed, "teardown never completed", seconds: 15);
|
||||
drv.GetDeviceState(Host).ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// M4: a ShutdownAsync overtaken by a NEWER InitializeAsync (its probe join is slow) must not stamp Unknown over
|
||||
/// the newer init's Healthy when it finally finishes.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task A_slow_shutdown_does_not_overwrite_a_newer_inits_health()
|
||||
{
|
||||
var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(400));
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started");
|
||||
await Task.Delay(30); // inside the slow probe read
|
||||
|
||||
var shutdown = drv.ShutdownAsync(CancellationToken.None);
|
||||
await Task.Delay(30);
|
||||
await drv.InitializeAsync("{}", CancellationToken.None); // newer: publishes while the shutdown still joins
|
||||
await shutdown;
|
||||
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
drv.GetDeviceState(Host).ShouldNotBeNull();
|
||||
await drv.DisposeAsync();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -7,7 +7,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Task 22 review r1: a runtime that refuses an operation with <see cref="AbStatus.Unsupported"/> (the native
|
||||
/// transport does this before the wire, e.g. a write it cannot type) reaches the client as BadNotSupported —
|
||||
/// transport does this before the wire, e.g. a write it cannot type — an <see cref="AbCipLocalRefusalException"/>
|
||||
/// since #737 I2) reaches the client as BadNotSupported —
|
||||
/// NOT as a comms fault. The handle is kept (one create, never disposed) and driver health is untouched.
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
@@ -19,12 +20,12 @@ public sealed class AbCipLocalRefusalTests
|
||||
{
|
||||
public override Task ReadAsync(CancellationToken cancellationToken) =>
|
||||
refuseRead
|
||||
? Task.FromException(new AbTransportException(AbStatus.Unsupported, "refused read"))
|
||||
? Task.FromException(new AbCipLocalRefusalException("refused read"))
|
||||
: base.ReadAsync(cancellationToken);
|
||||
|
||||
public override Task WriteAsync(CancellationToken cancellationToken) =>
|
||||
refuseWrite
|
||||
? Task.FromException(new AbTransportException(AbStatus.Unsupported, "refused write"))
|
||||
? Task.FromException(new AbCipLocalRefusalException("refused write"))
|
||||
: base.WriteAsync(cancellationToken);
|
||||
}
|
||||
|
||||
|
||||
@@ -77,7 +77,7 @@ public sealed class AbCipStatusMapperTests
|
||||
Status.Ok => AbCipStatusMapper.Good,
|
||||
Status.Pending => AbCipStatusMapper.GoodMoreData,
|
||||
Status.ErrorTimeout => AbCipStatusMapper.BadTimeout,
|
||||
Status.ErrorNotFound or Status.ErrorNoMatch or Status.ErrorBadDevice => AbCipStatusMapper.BadNodeIdUnknown,
|
||||
Status.ErrorNotFound => AbCipStatusMapper.BadNodeIdUnknown, // #737 I2: NoMatch/BadDevice are comms
|
||||
Status.ErrorNotAllowed => AbCipStatusMapper.BadNotWritable,
|
||||
Status.ErrorOutOfBounds or Status.ErrorTooLarge or Status.ErrorTooSmall => AbCipStatusMapper.BadOutOfRange,
|
||||
Status.ErrorUnsupported or Status.ErrorNotImplemented => AbCipStatusMapper.BadNotSupported,
|
||||
|
||||
@@ -110,4 +110,22 @@ public sealed class AbCipWriteHealthTests
|
||||
after.LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead);
|
||||
factory.Tags["D"].Disposed.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>M3: a good write clears a Degraded that a WRITE comms fault recorded.</summary>
|
||||
[Fact]
|
||||
public async Task A_good_write_clears_a_write_side_Degraded()
|
||||
{
|
||||
var fail = true;
|
||||
var (drv, _) = await BuildAsync(p => fail ? new ThrowingWriteTag(p) : new RealEncodingTag(p));
|
||||
(await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None))[0].StatusCode
|
||||
.ShouldBe(AbCipStatusMapper.BadCommunicationError);
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Degraded);
|
||||
|
||||
fail = false; // the evicted runtime is re-created healthy
|
||||
(await drv.WriteAsync([new WriteRequest("D", 6)], CancellationToken.None))[0].StatusCode
|
||||
.ShouldBe(AbCipStatusMapper.Good);
|
||||
|
||||
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
|
||||
drv.GetHealth().LastError.ShouldBeNull();
|
||||
}
|
||||
}
|
||||
|
||||
+22
-2
@@ -16,8 +16,8 @@ public sealed class LibplctagStatusTranslationTests
|
||||
[InlineData(Status.Pending, AbStatus.Pending)]
|
||||
[InlineData(Status.ErrorTimeout, AbStatus.Timeout)]
|
||||
[InlineData(Status.ErrorNotFound, AbStatus.NotFound)]
|
||||
[InlineData(Status.ErrorNoMatch, AbStatus.NotFound)]
|
||||
[InlineData(Status.ErrorBadDevice, AbStatus.NotFound)]
|
||||
[InlineData(Status.ErrorNoMatch, AbStatus.BadConnection)] // #737 I2: not proof the controller answered
|
||||
[InlineData(Status.ErrorBadDevice, AbStatus.BadConnection)]
|
||||
[InlineData(Status.ErrorNotAllowed, AbStatus.NotAllowed)]
|
||||
[InlineData(Status.ErrorOutOfBounds, AbStatus.OutOfBounds)]
|
||||
[InlineData(Status.ErrorTooLarge, AbStatus.TooLarge)]
|
||||
@@ -148,4 +148,24 @@ public sealed class LibplctagStatusTranslationTests
|
||||
LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException(Status.ErrorBadParam)));
|
||||
thrown.Status.ShouldBe(AbStatus.BadConnection);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// #737 I2: libplctag's <c>ErrorNotImplemented</c> is the LIBRARY refusing (no I/O): a local refusal, not a device
|
||||
/// refusal — it must not count as "the controller answered".
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task NotImplemented_is_a_local_refusal_not_a_device_refusal()
|
||||
{
|
||||
var ex = await Should.ThrowAsync<AbTransportException>(() =>
|
||||
LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException("ErrorNotImplemented")));
|
||||
ex.ShouldBeOfType<AbCipLocalRefusalException>();
|
||||
ex.Status.ShouldBe(AbStatus.Unsupported);
|
||||
AbCipRefusals.IsDeviceRefusal(ex).ShouldBeFalse();
|
||||
AbCipDriverProbe.IsReachableException(ex).ShouldBeFalse();
|
||||
|
||||
// Positive control: the device's own Unsupported refusal still is one.
|
||||
var device = await Should.ThrowAsync<AbTransportException>(() =>
|
||||
LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException("ErrorUnsupported")));
|
||||
AbCipRefusals.IsDeviceRefusal(device).ShouldBeTrue();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -135,6 +135,30 @@ public sealed class NativeAbCipTagRuntimeTests
|
||||
ch.Sent[0].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "28" + dword.ToString("X2") + "0100" + "00000000"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// #737 I1: a family whose BOOL array is element-addressed (the controller answers the DWORD-index request with a
|
||||
/// plain BOOL, 0xC1) falls back to the ORIGINAL element path <c>Bits[n]</c> — it must neither decode nor write
|
||||
/// element <c>n / 32</c>. Positive control: the 0xD3 theory above still decodes bit <c>n % 32</c>.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task A_per_element_BOOL_array_falls_back_to_the_element_path_for_read_and_write()
|
||||
{
|
||||
var ch = new FakeCipChannel
|
||||
{
|
||||
Respond = req => Task.FromResult(Convert.FromHexString(req[0] == 0x4D ? "CD000000" : "D2000000C10001")),
|
||||
};
|
||||
using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[37]", AbCipDataType.Bool));
|
||||
await rt.InitializeAsync(CancellationToken.None);
|
||||
await rt.ReadAsync(CancellationToken.None);
|
||||
|
||||
rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true);
|
||||
ch.Sent[^1].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "2825" + "0100" + "00000000")); // Bits[37]
|
||||
|
||||
rt.EncodeValue(AbCipDataType.Bool, null, false);
|
||||
await rt.WriteAsync(CancellationToken.None);
|
||||
ch.Sent[^1].ShouldBe(Convert.FromHexString("4D04" + "910442697473" + "2825" + "C100" + "0100" + "00")); // Bits[37]
|
||||
}
|
||||
|
||||
/// <summary>Positive control: a BOOL answered as a plain BOOL (0xC1) still decodes its byte.</summary>
|
||||
[Fact]
|
||||
public async Task A_BOOL_element_answered_as_BOOL_decodes_the_byte()
|
||||
|
||||
Reference in New Issue
Block a user