fix(twincat): read-singly fallback for any non-comms overall sum error, once-per-device warning, wording (#645 re-review)
Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
@@ -1133,7 +1133,7 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa
|
||||
- **TwinCAT (#645, #310/#314):** opt-in ADS sum-read (`useSumRead`, `sumReadMaxItems` default 200 / ceiling 500, 32 KiB payload
|
||||
budget). It reads raw bytes and decodes as the declared type, so only a scalar passing the strict width+kind check
|
||||
(`CheckNotification`) rides a batch; value-converted pairs (DInt over INT, Real over LREAL/DINT) read singly. Only overall
|
||||
ADS 1793/1794 latch the device to single reads; 1792/1797/1798 split that chunk without latching; a comms failure neither.
|
||||
ADS 1793/1794 latch the device to single reads; any other non-comms overall code (1792/1795/1797/1798...) reads that chunk singly for the poll without latching, with a once-per-device Warning; a comms failure neither.
|
||||
Opt-in `diagnosticsEnabled` (default off) puts ADS state/app/online-change/cycle/exec/`HeadroomMs` on the `/hosts`
|
||||
`IDriverDiagnostics` bag (up to 4 extra reads per device per probe tick). Unit-tested only; live gate pending a
|
||||
user-approved route (the presses' routes name the Mac's old VPN IP). Sum-write not ported.
|
||||
|
||||
@@ -208,11 +208,12 @@ command is unverified, hence opt-in.
|
||||
without joining the batch; a bit-indexed BOOL and a handle-addressed symbol (`0xF005`) are read singly.
|
||||
- Per-item ADS error codes map to that tag only (`MapAdsError`); a response shorter than asked is a size error
|
||||
for that item, not a failed chunk. The overall code is the command's.
|
||||
- **Latch vs split.** Only overall ADS **1793 / 1794** (service / index group not supported) throws
|
||||
- **Latch vs read-singly.** Only overall ADS **1793 / 1794** (service / index group not supported) throws
|
||||
`TwinCATSumReadRefusedException`; the driver logs once, marks the device `SumReadUnsupported` and reads
|
||||
singly until re-initialise. **1792 / 1797 / 1798** (general error, bad size, bad data) mean *this request*
|
||||
was rejected — typically too big — so that chunk is read singly for this poll and nothing is latched. A
|
||||
comms-level failure is neither: that chunk goes `BadCommunicationError` and the next poll tries again.
|
||||
singly until re-initialise. **Any other non-comms overall code** (1792 general error, 1795 offset, 1797 size, 1798 data, ...) means
|
||||
*this request* was rejected — typically too big — so that chunk is read singly for this poll, nothing is
|
||||
latched, and a Warning is logged once per device so the live gate can see sum-read is not working there. A
|
||||
comms-level failure (router / port / timeout) is neither: that chunk goes `BadCommunicationError` and the next poll tries again.
|
||||
- A one-item chunk is just a single read. Sum-**write** is not implemented (writes stay per-tag).
|
||||
|
||||
### `/hosts` diagnostics (`IDriverDiagnostics`, opt-in)
|
||||
|
||||
@@ -289,6 +289,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
|
||||
{
|
||||
// The library slices the response by the REQUESTED lengths and throws when a target answers with
|
||||
// less. That is this target mangling this response, not a comms fault: read the group singly.
|
||||
NoteSumFallback(0);
|
||||
await FallBackAsync().ConfigureAwait(false);
|
||||
return;
|
||||
}
|
||||
@@ -301,6 +302,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
|
||||
throw new TwinCATSumReadRefusedException(overall);
|
||||
if (TwinCATSumReadPlanner.IsRequestRejected(overall))
|
||||
{
|
||||
NoteSumFallback(overall);
|
||||
await FallBackAsync().ConfigureAwait(false); // too big / bad size for THIS request: no latch
|
||||
return;
|
||||
}
|
||||
@@ -325,6 +327,20 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
|
||||
private readonly record struct PlannedSumRead(int Index, uint IndexGroup, uint IndexOffset, TwinCATArrayLayout Layout, bool IsArray);
|
||||
|
||||
private string? _lastAdsState;
|
||||
private long _sumFallbackCount;
|
||||
private long _lastSumFallbackCode;
|
||||
|
||||
private void NoteSumFallback(uint code)
|
||||
{
|
||||
Interlocked.Exchange(ref _lastSumFallbackCode, code);
|
||||
Interlocked.Increment(ref _sumFallbackCount);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public long SumFallbackCount => Interlocked.Read(ref _sumFallbackCount);
|
||||
|
||||
/// <inheritdoc />
|
||||
public uint LastSumFallbackCode => (uint)Interlocked.Read(ref _lastSumFallbackCode);
|
||||
|
||||
/// <inheritdoc />
|
||||
public string? LastAdsState => Volatile.Read(ref _lastAdsState);
|
||||
|
||||
@@ -92,6 +92,16 @@ public interface ITwinCATClient : IDisposable
|
||||
/// </summary>
|
||||
string? LastAdsState => null;
|
||||
|
||||
/// <summary>
|
||||
/// How many sum-read chunks this client has read singly because the target rejected the sum request
|
||||
/// without refusing the service (non-latching). The driver warns once per device when it grows, so the
|
||||
/// live gate can see sum-read is not working on that target. Default 0.
|
||||
/// </summary>
|
||||
long SumFallbackCount => 0;
|
||||
|
||||
/// <summary>The overall ADS code of the most recent such fallback (0 = a malformed response).</summary>
|
||||
uint LastSumFallbackCode => 0;
|
||||
|
||||
/// <summary>
|
||||
/// Write a symbolic value. Returns the mapped OPC UA status for the operation
|
||||
/// (0 = Good, non-zero = error mapped via <see cref="TwinCATStatusMapper"/>).
|
||||
|
||||
@@ -393,7 +393,17 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
|
||||
for (var k = 0; k < chunk.Count; k++)
|
||||
items[k] = new TwinCATBulkReadItem(chunk[k].SymbolName, chunk[k].Def.DataType, chunk[k].BitIndex, chunk[k].Def.ArrayLength);
|
||||
|
||||
var fallbacksBefore = client.SumFallbackCount;
|
||||
var bulk = await client.ReadValuesAsync(items, cancellationToken).ConfigureAwait(false);
|
||||
if (client.SumFallbackCount > fallbacksBefore && !device.SumFallbackWarned)
|
||||
{
|
||||
device.SumFallbackWarned = true;
|
||||
_logger.LogWarning(
|
||||
"TwinCAT driver '{DriverInstanceId}' device '{Device}': the target rejected an ADS sum-read " +
|
||||
"request (ADS error {AdsError}; 0 = malformed response) and the chunk was read singly. Sum-read " +
|
||||
"is not working on this target; this warning is logged once per device",
|
||||
_driverInstanceId, device.Options.HostAddress, client.LastSumFallbackCode);
|
||||
}
|
||||
for (var k = 0; k < chunk.Count; k++)
|
||||
{
|
||||
var e = chunk[k];
|
||||
@@ -994,7 +1004,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
|
||||
|
||||
/// <summary>
|
||||
/// The <c>/hosts</c> diagnostics bag (Gitea #640 channel): per device, ADS state, PLC app name, online
|
||||
/// change count, task cycle time, last execution time and jitter (last exec − cycle). A single-device
|
||||
/// change count, task cycle time, last execution time and headroom (cycle − last exec). A single-device
|
||||
/// driver uses flat <c>TwinCAT.*</c> keys; several devices prefix the device name (or host address).
|
||||
/// A pure in-memory read — the probe loop does the sampling. The host bounds the bag
|
||||
/// (<see cref="DriverDiagnostics.MaxEntries"/>), so a many-device driver keeps the first keys in
|
||||
@@ -1464,6 +1474,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
|
||||
/// <see cref="DeviceState"/> (driver re-initialise).</summary>
|
||||
public volatile bool SumReadUnsupported;
|
||||
|
||||
/// <summary>True once the "sum request rejected, chunk read singly" warning was logged for this device.</summary>
|
||||
public volatile bool SumFallbackWarned;
|
||||
|
||||
/// <summary>Latest probe-sampled PLC diagnostics (#645 / #314); null before the first sample.</summary>
|
||||
public TwinCATDeviceDiagnostics? LastDiagnostics { get; set; }
|
||||
|
||||
|
||||
@@ -37,11 +37,21 @@ internal static class TwinCATSumReadPlanner
|
||||
/// <returns>True for 1793 (service not supported) and 1794 (invalid index group).</returns>
|
||||
public static bool IsRefusal(uint adsError) => adsError is 1793 or 1794;
|
||||
|
||||
/// <summary>The target rejected THIS request (general error / bad size / bad data), typically because it was
|
||||
/// too big. Not a device property: read the chunk singly for this poll, do not latch.</summary>
|
||||
/// <summary>A transport-level failure (router / port / timeout), not an answer from the sum service.</summary>
|
||||
/// <param name="adsError">The overall ADS code.</param>
|
||||
/// <returns>True for 1792, 1797 and 1798.</returns>
|
||||
public static bool IsRequestRejected(uint adsError) => adsError is 1792 or 1797 or 1798;
|
||||
/// <returns>True when the code maps to a communication error or timeout.</returns>
|
||||
public static bool IsCommsFailure(uint adsError) =>
|
||||
TwinCATStatusMapper.MapAdsError(adsError) is var s
|
||||
&& (s == TwinCATStatusMapper.BadCommunicationError || s == TwinCATStatusMapper.BadTimeout);
|
||||
|
||||
/// <summary>
|
||||
/// The sum service answered with an overall error that is neither "unsupported" nor a comms failure
|
||||
/// (general error, bad size / data / offset, ...): it rejected THIS request. Read the chunk singly for this
|
||||
/// poll, do not latch.
|
||||
/// </summary>
|
||||
/// <param name="adsError">The overall ADS code.</param>
|
||||
/// <returns>True for any such code.</returns>
|
||||
public static bool IsRequestRejected(uint adsError) => !IsRefusal(adsError) && !IsCommsFailure(adsError);
|
||||
|
||||
/// <summary>
|
||||
/// Splits items (in order) into groups whose summed <c>length + PerItemOverhead</c> stays within
|
||||
|
||||
@@ -85,7 +85,7 @@ internal class FakeTwinCATClient : ITwinCATClient
|
||||
/// <summary>Gets or sets an exception <see cref="ReadValuesAsync"/> throws instead of answering — a
|
||||
/// <see cref="TwinCATSumReadRefusedException"/> models a target without sum commands.</summary>
|
||||
public Exception? SumReadException { get; set; }
|
||||
/// <summary>Gets or sets the ADS state text <see cref="ReadAdsStateAsync"/> returns.</summary>
|
||||
/// <summary>Gets or sets the ADS state text <see cref="LastAdsState"/> returns.</summary>
|
||||
public string? AdsStateText { get; set; }
|
||||
|
||||
/// <summary>Simulates an ADS sum-read: records the batch size, then answers each item from the same
|
||||
@@ -99,6 +99,7 @@ internal class FakeTwinCATClient : ITwinCATClient
|
||||
if (ThrowOnRead) throw Exception ?? new InvalidOperationException();
|
||||
SumReadBatches.Add(reads.Count);
|
||||
if (SumReadException is { } ex) throw ex;
|
||||
if (SimulateSumFallback) SumFallbackCount++;
|
||||
var results = new (object? value, uint status)[reads.Count];
|
||||
for (var i = 0; i < reads.Count; i++)
|
||||
{
|
||||
@@ -109,6 +110,12 @@ internal class FakeTwinCATClient : ITwinCATClient
|
||||
return Task.FromResult<IReadOnlyList<(object? value, uint status)>>(results);
|
||||
}
|
||||
|
||||
/// <summary>Gets or sets whether each batch counts as a non-latching sum fallback.</summary>
|
||||
public bool SimulateSumFallback { get; set; }
|
||||
/// <summary>Gets the simulated count of non-latching sum fallbacks.</summary>
|
||||
public long SumFallbackCount { get; private set; }
|
||||
/// <summary>Gets the simulated overall code of the last fallback.</summary>
|
||||
public uint LastSumFallbackCode => 1795;
|
||||
/// <summary>Gets the ADS state the "probe" last read.</summary>
|
||||
public string? LastAdsState => AdsStateText;
|
||||
|
||||
|
||||
@@ -49,7 +49,7 @@ public sealed class TwinCATDiagnosticsTests
|
||||
diag.GetDiagnostics().ShouldBeEmpty();
|
||||
}
|
||||
|
||||
/// <summary>Verifies a single-device driver publishes flat keys with state, app, cycle, exec and jitter.</summary>
|
||||
/// <summary>Verifies a single-device driver publishes flat keys with state, app, cycle, exec and headroom.</summary>
|
||||
[Fact]
|
||||
public async Task Single_device_publishes_flat_keys()
|
||||
{
|
||||
|
||||
@@ -46,10 +46,12 @@ public sealed class TwinCATSumReadPlannerTests
|
||||
[InlineData(1792u, false, true)]
|
||||
[InlineData(1797u, false, true)]
|
||||
[InlineData(1798u, false, true)]
|
||||
[InlineData(1795u, false, false)]
|
||||
[InlineData(6u, false, false)]
|
||||
[InlineData(1795u, false, true)] // invalid offset: this request rejected, read singly
|
||||
[InlineData(1796u, false, true)]
|
||||
[InlineData(1808u, false, true)]
|
||||
[InlineData(6u, false, false)] // comms: every item goes Bad
|
||||
[InlineData(7u, false, false)]
|
||||
[InlineData(1861u, false, false)]
|
||||
[InlineData(1808u, false, false)]
|
||||
public void Overall_ads_codes_classify(uint code, bool refusal, bool rejected)
|
||||
{
|
||||
TwinCATSumReadPlanner.IsRefusal(code).ShouldBe(refusal);
|
||||
|
||||
@@ -156,6 +156,22 @@ public sealed class TwinCATSumReadTests
|
||||
factory.Clients[0].SumReadBatches.ShouldBe([2, 2]);
|
||||
}
|
||||
|
||||
/// <summary>Verifies a non-latching fallback is surfaced once per device, not once per poll.</summary>
|
||||
[Fact]
|
||||
public async Task Non_latching_fallback_warns_once_per_device()
|
||||
{
|
||||
var (drv, factory) = NewDriver(true, 200, Tag("A"), Tag("B"));
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.A"] = 1, ["MAIN.B"] = 2 }, SimulateSumFallback = true };
|
||||
|
||||
await drv.ReadAsync(["A", "B"], CancellationToken.None);
|
||||
var state = drv.GetDeviceState(Host)!;
|
||||
state.SumFallbackWarned.ShouldBeTrue();
|
||||
state.SumReadUnsupported.ShouldBeFalse(); // not latched: the next poll tries the sum command again
|
||||
await drv.ReadAsync(["A", "B"], CancellationToken.None);
|
||||
factory.Clients[0].SumReadBatches.ShouldBe([2, 2]);
|
||||
}
|
||||
|
||||
/// <summary>Verifies unknown references do not disturb result order.</summary>
|
||||
[Fact]
|
||||
public async Task Unknown_reference_keeps_its_slot_in_a_batched_poll()
|
||||
|
||||
Reference in New Issue
Block a user