From b0b0ce9c77d623eb8b3eaf722b17db8213708370 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 16:53:56 -0400 Subject: [PATCH 1/2] feat(ablegacy): resolve indirect addresses at read/write time (#646) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 5 +- docs/drivers/AbLegacy.md | 22 ++- .../AbLegacyAddress.cs | 6 +- .../AbLegacyTagDefinitionFactory.cs | 8 - .../AbLegacyDriver.cs | 162 +++++++++++++++++- .../AbLegacyIndirectAddressingTests.cs | 55 ++++++ .../AbLegacyFamilyAddressingTests.cs | 22 --- .../AbLegacyIndirectAddressingTests.cs | 131 ++++++++++++++ .../Uns/AbLegacyFamilyAddressGateTests.cs | 4 +- 9 files changed, 364 insertions(+), 51 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.IntegrationTests/AbLegacyIndirectAddressingTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyIndirectAddressingTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 1c68f757..ca385748 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1118,8 +1118,9 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa - **TwinCAT (#637):** TIME/TOD publish Float64 ms, DATE/DT a UTC DateTime; a tag authored `UDInt` keeps the raw counter (and keeps historizing — DateTime is not historized). - **AbLegacy (#636):** `AbLegacyAddress` (in `AbLegacy.Contracts`) is family-aware: PLC-5 `I:`/`O:` indices are OCTAL - and are emitted to libplctag as decimal (libplctag's `pccc.c` parses decimal only). MicroLogix function files and - indirect `N7:[N7:0]` addresses parse but read `BadNotSupported`. One check (`AbLegacyAddress.Validate`) backs the + and are emitted to libplctag as decimal (libplctag's `pccc.c` parses decimal only). MicroLogix function files parse + but read `BadNotSupported`. Indirect `N7:[N7:0]` (#646) is resolved by the driver at read/write time (index re-read + every call, never cached; bad index = `BadOutOfRange`; indirect writes are not atomic). One check (`AbLegacyAddress.Validate`) backs the editor (family-independent) and the deploy gate (family from the tag's `DeviceConfig.PlcFamily`); an unparseable address reads `BadNodeIdUnknown` for that tag only. - **OpcUaClient (#635):** OperationLimits are honoured by the SDK session (`SessionClientBatched`) — do not re-add diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index 09b46101..4ce9d5ac 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -69,7 +69,7 @@ pieces for driver-side routing (e.g. deciding a bit needs read-modify-write): | `I:0/0` / `O:1/2` / `S:1` | Input / output / status system files (no file number) | | `I:017/07` | **PLC-5 only:** `I:`/`O:` word and bit indices are **octal** (#244); a digit 8/9 is a parse error | | `RTC:0.HR` / `HSC:0.ACC` | **MicroLogix only:** function files RTC, HSC, DLS, MMI, PTO, PWM, STI, EII, IOS, BHI (#245) | -| `N7:[N7:0]` / `N[N7:0]:5` | Indirect (index word read from the PLC) — parsed, **not supported at runtime** (#247) | +| `N7:[N7:0]` / `N[N7:0]:5` | Indirect (index word read from the PLC) — resolved at read/write time (#646) | `AbLegacyDataType` covers the corresponding PCCC types: `Bit`, `Int` (N), `Long` (L), `Float` (F), `AnalogInt` (A), `String` (ST), and the `TimerElement` / @@ -118,11 +118,18 @@ physical inputs and may reject the write. members default to Int32). ⚠️ **Wire-unverified on both transports:** there is no capture and no simulator coverage; the libplctag path passes the name through and the native transport has no file-type code for them, so it refuses the tag as `Unsupported` (`BadNotSupported` to the client; see Wire evidence). -- **Indirect addressing (#247).** `N7:[N7:0]` (word from another address) and `N[N7:0]:5` (file from another - address) parse, one level deep (the inner address must be direct, and carries the family's octal rule). - **Runtime resolution is not implemented** (it needs a pre-read of the index word and per-resolved-address - runtimes): reads and writes of an indirect tag answer `BadNotSupported` before any runtime is created, on both - transports. +- **Indirect addressing (#247 parse, #646 runtime).** `N7:[N7:0]` (word from another address) and `N[N7:0]:5` (file + from another address) parse, one level deep (the inner address must be direct, and carries the family's octal rule). + At runtime the driver reads the inner address first (same runtime factory, so libplctag and Native alike), requires + an integer in range (word/element `0..255`, file `0..999`), composes the concrete address and runs the normal + read/write path against it. **The index is re-read on every read/write - never cached** - and the previous + resolution's handle is released when the index moves. A bad index (negative, too large, float/string source) is + `BadOutOfRange` for that tag only (no health impact); an inner-read failure propagates its own status (a wire + failure also degrades health like any read). **Writes** resolve then write and are **not atomic**: the index can + change between the two PLC accesses. Bit RMW writes (`B3:[N7:0]/4`) resolve first, then RMW the concrete word. + The Native transport never sees the bracket form (it receives the composed name), so N-file indirect stays inside + the wire-proven envelope; other file types are as (un)verified as their direct forms. Live-gated on `ab_server` + by `AbLegacyIndirectAddressingTests` (`N7:0=5`, `N7:5=42`, `N7:[N7:0]` reads 42). ## Configuration @@ -211,7 +218,8 @@ Everything below is against the `ab_server` simulator; no real PCCC controller h | N-file write (`N7:5`) | capture `slc500-write-n7` + native `AbLegacyReadSmokeTests` write/read round trip (SLC 500) | | F-file read (`F8`) | one capture, `slc500-read-f8` (codec parity only; no native F write on a wire) | | S (incl. the default probe `S:0`), B, T, C, R, L, ST, A, O, I | **none** — encoded from the PCCC file-type table | -| MicroLogix function files (RTC/HSC/...), PLC-5 octal I/O, indirect addressing | **none** — parsed only; native refuses function files and indirect names as Unsupported | +| MicroLogix function files (RTC/HSC/...), PLC-5 octal I/O | **none** — parsed only; native refuses function files as Unsupported | +| Indirect addressing (`N7:[N7:0]`) | `ab_server` N-file, both transports (#646) - resolved by the driver, the transport sees the composed address | | T/C/R sub-elements (`.PRE`/`.ACC`/`.LEN`/`.POS`), 6-byte T/C/R element layout | **none** | ### Native known gaps diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyAddress.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyAddress.cs index aac67c41..576cb062 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyAddress.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyAddress.cs @@ -23,7 +23,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; /// L9:0 — long-integer file (SLC 5/05+, 32-bit). /// I:017/07 — PLC-5 only: I/O word + bit indices are octal (#244); stored decimal. /// RTC:0.HR — MicroLogix only: function file (RTC/HSC/DLS/MMI/PTO/PWM/STI/EII/IOS/BHI, #245). -/// N7:[N7:0], N[N7:0]:5 — indirect addressing (#247): parsed, not resolved at runtime. +/// N7:[N7:0], N[N7:0]:5 — indirect addressing (#247 parse, #646 runtime): the driver reads the index first and composes the concrete address. /// /// The libplctag transport takes as its name=... attribute and the /// native transport re-parses the same string, so both are handed identical (decimal) numbers. This parser @@ -42,8 +42,8 @@ public sealed record AbLegacyAddress( /// /// True when the file number or the word number is sourced from another PCCC address evaluated /// at runtime (N7:[N7:0] or N[N7:0]:5). Neither transport can encode the bracket - /// form, and resolving it needs a pre-read of the index word, which is not implemented — the - /// driver answers an indirect tag BadNotSupported. + /// form, so the driver (#646) pre-reads the index, substitutes it, and hands the transport the + /// concrete address; an indirect name never reaches a transport. /// public bool IsIndirect => IndirectFileSource is not null || IndirectWordSource is not null; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs index 1bbfa9af..b76a47b4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs @@ -123,12 +123,6 @@ public static class AbLegacyTagDefinitionFactory /// The RawPath handed to during inspection — the mapper only copies it /// onto and never validates it. /// - private static bool IsIndirectFor(string address, AbLegacyPlcFamily? family) - { - if (family is { } f) return AbLegacyAddress.TryParse(address, f)?.IsIndirect == true; - return Enum.GetValues().Any(c => AbLegacyAddress.TryParse(address, c)?.IsIndirect == true); - } - private const string InspectionRawPath = "(inspect)"; /// @@ -179,8 +173,6 @@ public static class AbLegacyTagDefinitionFactory { if (AbLegacyAddress.Validate(accepted.Address, family) is { } addressError) warnings.Add($"AbLegacy tag address: {addressError} The driver reads this tag as BadNodeIdUnknown."); - else if (IsIndirectFor(accepted.Address, family)) - warnings.Add($"AbLegacy address '{accepted.Address}' is indirect; indirect addressing is not supported and the tag reads BadNotSupported."); return warnings; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index f1ea75e7..80dcc136 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -388,8 +388,6 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover try { - // Indirect addresses (N7:[N7:0]) parse but are not resolved at runtime (#247) — answer the - // refusal before a runtime is created for a name neither transport can encode. var preParsed = AbLegacyAddress.TryParse(def.Address, device.Options.PlcFamily); if (preParsed is null) { @@ -399,8 +397,18 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover } if (preParsed.IsIndirect) { - results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); - continue; + // Indirect (#646): read the index word(s) NOW (never cached - the index can change between + // polls), compose the concrete address, then run the normal path against it. + var res = await ResolveIndirectAsync(device, def, preParsed, cancellationToken).ConfigureAwait(false); + if (res.Def is null) + { + results[i] = new DataValueSnapshot(null, res.Status, null, now); + if (res.TransportFault) + _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, + $"transport fault resolving indirect address for {reference}"); + continue; + } + def = res.Def; } var runtime = await EnsureTagRuntimeAsync(device, def, cancellationToken).ConfigureAwait(false); @@ -513,11 +521,21 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover continue; } - // Indirect addressing is parse-only (#247): refuse before any runtime or RMW read is created. + // Indirect (#646): resolve the index first (non-atomic with the write - the index may change + // between the two PLC accesses), then write the concrete address through the normal path. if (parsed.IsIndirect) { - results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported); - continue; + var res = await ResolveIndirectAsync(device, def, parsed, cancellationToken).ConfigureAwait(false); + if (res.Def is null) + { + results[i] = new WriteResult(res.Status); + if (res.TransportFault) + _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, + $"transport fault resolving indirect address for {w.FullReference}"); + continue; + } + def = res.Def; + parsed = AbLegacyAddress.TryParse(def.Address, device.Options.PlcFamily)!; } // A write the transport refuses outright (native: a T/C/R status-bit sub-element such as T4:0.DN, @@ -841,6 +859,132 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover } } + /// Largest word/element index an indirect source may yield (PCCC element offset is one byte). + private const int MaxIndirectElement = AbLegacyArray.MaxElements - 1; + + /// Largest data-file number an indirect source may yield (PLC-5 allows up to 999). + private const int MaxIndirectFile = 999; + + /// + /// Resolves an indirect address (#646). Reads each index source through the same runtime/transport path + /// (same factory, so libplctag and Native alike), range-checks it, and composes a concrete tag definition + /// whose Name is keyed by the resolved address so the normal runtime cache applies. Nothing about the + /// index is cached: every call re-reads it. A bad index is BadOutOfRange (per-tag, no health impact); + /// an inner-read failure propagates its mapped status (TransportFault true for a wire failure). + /// + private async Task ResolveIndirectAsync( + DeviceState device, AbLegacyTagDefinition def, AbLegacyAddress parsed, CancellationToken ct) + { + var fileNumber = parsed.FileNumber; + var word = parsed.WordNumber; + + if (parsed.IndirectFileSource is { } fileSrc) + { + var r = await ReadIndexAsync(device, fileSrc, ct).ConfigureAwait(false); + if (r.Status != 0) return new IndirectResolution(null, r.Status, r.TransportFault); + if (r.Value is < 0 or > MaxIndirectFile) + return new IndirectResolution(null, AbLegacyStatusMapper.BadOutOfRange, false); + fileNumber = (int)r.Value!; + } + if (parsed.IndirectWordSource is { } wordSrc) + { + var r = await ReadIndexAsync(device, wordSrc, ct).ConfigureAwait(false); + if (r.Status != 0) return new IndirectResolution(null, r.Status, r.TransportFault); + if (r.Value is < 0 or > MaxIndirectElement) + return new IndirectResolution(null, AbLegacyStatusMapper.BadOutOfRange, false); + word = (int)r.Value!; + } + + var concrete = parsed with + { + FileNumber = fileNumber, WordNumber = word, IndirectFileSource = null, IndirectWordSource = null, + }; + var concreteText = concrete.ToLibplctagName(); + var resolvedName = $"{def.Name}=>{concreteText}"; + + // The index moved: drop the previous resolution's handle so a sweeping index does not accumulate one + // libplctag handle per value it ever took. + var previous = device.IndirectLastName.TryGetValue(def.Name, out var prior) ? prior : null; + device.IndirectLastName[def.Name] = resolvedName; + if (previous is not null && !string.Equals(previous, resolvedName, StringComparison.OrdinalIgnoreCase)) + { + var oldLock = device.GetRuntimeLock(previous); + await oldLock.WaitAsync(ct).ConfigureAwait(false); + try { EvictRuntime(device, previous); } finally { oldLock.Release(); } + } + return new IndirectResolution(def with { Name = resolvedName, Address = concreteText }, 0, false); + } + + /// Reads one direct index address as an integer through the shared parent-runtime cache. + private async Task<(long? Value, uint Status, bool TransportFault)> ReadIndexAsync( + DeviceState device, AbLegacyAddress inner, CancellationToken ct) + { + AbLegacyDataType type; + if (inner.BitIndex is not null) type = AbLegacyDataType.Bit; + else if (inner.FileLetter == "L") type = AbLegacyDataType.Long; + else if (inner.FileLetter is "F" or "ST") return (null, AbLegacyStatusMapper.BadOutOfRange, false); + else type = AbLegacyDataType.Int; + + // A bit-addressed index reads its parent word and decodes the bit, like the RMW path does. + var runtimeName = (inner with { BitIndex = null }).ToLibplctagName(); + var gate = device.GetRmwLock(runtimeName); + await gate.WaitAsync(ct).ConfigureAwait(false); + try + { + IAbLegacyTagRuntime runtime; + try + { + runtime = await EnsureParentRuntimeAsync(device, runtimeName, ct).ConfigureAwait(false); + await runtime.ReadAsync(ct).ConfigureAwait(false); + } + catch (OperationCanceledException) { throw; } + catch (AbTransportException ex) when (IsLocalRefusal(ex)) + { + return (null, AbLegacyStatusMapper.BadNotSupported, false); + } + catch + { + EvictParentRuntime(device, runtimeName); + return (null, AbLegacyStatusMapper.BadCommunicationError, true); + } + + var status = runtime.GetStatus(); + if (status != 0) + { + EvictParentRuntime(device, runtimeName); + return (null, AbLegacyStatusMapper.MapStatus(status), true); + } + var raw = runtime.DecodeValue(type, inner.BitIndex); + try + { + return raw switch + { + bool b => (b ? 1L : 0L, 0u, false), + null => (null, AbLegacyStatusMapper.BadOutOfRange, false), + _ => (Convert.ToInt64(raw), 0u, false), + }; + } + catch (Exception ex) when (ex is FormatException or InvalidCastException or OverflowException) + { + return (null, AbLegacyStatusMapper.BadOutOfRange, false); + } + } + finally + { + gate.Release(); + } + } + + private static void EvictParentRuntime(DeviceState device, string name) + { + if (device.ParentRuntimes.TryRemove(name, out var stale)) + { + try { stale.Dispose(); } catch { /* best-effort */ } + } + } + + private sealed record IndirectResolution(AbLegacyTagDefinition? Def, uint Status, bool TransportFault); + private async Task EnsureParentRuntimeAsync( AbLegacyDriver.DeviceState device, string parentName, CancellationToken ct) { @@ -1086,6 +1230,10 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover public System.Collections.Concurrent.ConcurrentDictionary ParentRuntimes { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Indirect tag name to the resolved-address runtime key it last used (#646), so a moved index evicts its stale handle. + public System.Collections.Concurrent.ConcurrentDictionary IndirectLastName { get; } = + new(StringComparer.OrdinalIgnoreCase); + /// /// Per-key creation locks for and . /// A caller holds this before the TryGetValue + Create + InitializeAsync + TryAdd diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.IntegrationTests/AbLegacyIndirectAddressingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.IntegrationTests/AbLegacyIndirectAddressingTests.cs new file mode 100644 index 00000000..1b12a781 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.IntegrationTests/AbLegacyIndirectAddressingTests.cs @@ -0,0 +1,55 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.IntegrationTests; + +/// +/// #646 against ab_server: N7:[N7:0] reads the index word, then the word it names, on both +/// transports. Writes N7:0 and N7:5 (simulator-only, gated by ). +/// +[Collection(AbLegacyServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Simulator", "ab_server-PCCC")] +public sealed class AbLegacyIndirectAddressingTests(AbLegacyServerFixture sim) +{ + /// Seeds N7:0=5 and N7:5=42, then reads and writes through N7:[N7:0]. + /// The transport stack the driver runs on. + [AbLegacyTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task Slc500_indirect_word_read_and_write_resolve_through_the_index(AbLegacyTransport transport) + { + if (sim.SkipReasonFor(KnownProfiles.Slc500) is { } skip) Assert.Skip(skip); + IntegrationWriteGate.SkipUnlessWritesAllowed(); + + var deviceUri = $"ab://{sim.Host}:{sim.PortFor(KnownProfiles.Slc500)}/{sim.CipPath}"; + await using var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(deviceUri, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries( + new AbLegacyTagDefinition("Index", deviceUri, "N7:0", AbLegacyDataType.Int, Writable: true), + new AbLegacyTagDefinition("Target", deviceUri, "N7:5", AbLegacyDataType.Int, Writable: true), + new AbLegacyTagDefinition("Indirect", deviceUri, "N7:[N7:0]", AbLegacyDataType.Int, Writable: true)), + Timeout = TimeSpan.FromSeconds(5), + Probe = new AbLegacyProbeOptions { Enabled = false }, + Transport = transport, + }, driverInstanceId: $"ablegacy-indirect-{transport}"); + var ct = TestContext.Current.CancellationToken; + await drv.InitializeAsync("{}", ct); + + var seeded = await drv.WriteAsync( + [new WriteRequest("Index", (short)5), new WriteRequest("Target", (short)42)], ct); + seeded.ShouldAllBe(r => r.StatusCode == AbLegacyStatusMapper.Good); + + var read = (await drv.ReadAsync(["Indirect"], ct)).Single(); + read.StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + Convert.ToInt32(read.Value).ShouldBe(42); + + // Write through the indirection lands on N7:5. + (await drv.WriteAsync([new WriteRequest("Indirect", (short)77)], ct)).Single() + .StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + var direct = (await drv.ReadAsync(["Target"], ct)).Single(); + Convert.ToInt32(direct.Value).ShouldBe(77); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyFamilyAddressingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyFamilyAddressingTests.cs index 595b4797..58716228 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyFamilyAddressingTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyFamilyAddressingTests.cs @@ -181,28 +181,6 @@ public sealed class AbLegacyFamilyAddressingTests factory.Tags["I:15/15"].ReadCount.ShouldBe(1); } - /// An indirect tag is answered BadNotSupported on read and write, and no runtime is ever created for it. - [Fact] - public async Task Indirect_tag_is_BadNotSupported_and_creates_no_runtime() - { - var (drv, factory) = NewDriver(AbLegacyPlcFamily.Slc500, - new AbLegacyTagDefinition("Ind", "ab://10.0.0.5/1,0", "N7:[N7:0]", AbLegacyDataType.Int, Writable: true), - new AbLegacyTagDefinition("Ok", "ab://10.0.0.5/1,0", "N7:1", AbLegacyDataType.Int, Writable: true)); - await drv.InitializeAsync("{}", CancellationToken.None); - factory.Customise = p => new FakeAbLegacyTag(p) { Value = 5 }; - - var read = await drv.ReadAsync(["Ind", "Ok"], CancellationToken.None); - var write = await drv.WriteAsync( - [new WriteRequest("Ind", 1), new WriteRequest("Ok", 1)], CancellationToken.None); - - read[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); - write[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); - // positive control: the neighbouring direct tag in the same batch still works, so the refusal is per-tag - read[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); - write[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); - factory.Tags.Keys.ShouldNotContain(k => k.Contains('[')); - } - /// A MicroLogix function-file tag reaches the transport under its libplctag name. [Fact] public async Task MicroLogix_function_file_tag_reaches_the_transport() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyIndirectAddressingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyIndirectAddressingTests.cs new file mode 100644 index 00000000..6baf2165 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyIndirectAddressingTests.cs @@ -0,0 +1,131 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// #646: indirect addresses (N7:[N7:0], N[N7:0]:5) resolve at read/write time. +[Trait("Category", "Unit")] +public sealed class AbLegacyIndirectAddressingTests +{ + private const string Host = "ab://10.0.0.5/1,0"; + + private static async Task<(AbLegacyDriver drv, FakeAbLegacyTagFactory factory)> NewDriverAsync( + params AbLegacyTagDefinition[] tags) + { + var factory = new FakeAbLegacyTagFactory(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(Host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries(tags), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + private static void Seed(FakeAbLegacyTagFactory f, params (string name, object value)[] values) + { + var map = values.ToDictionary(v => v.name, v => v.value, StringComparer.OrdinalIgnoreCase); + f.Customise = p => new FakeAbLegacyTag(p) { Value = map.TryGetValue(p.TagName, out var v) ? v : 0 }; + } + + [Fact] + public async Task Word_indirect_reads_index_then_composed_address() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int)); + Seed(f, ("N7:0", (short)5), ("N7:5", (short)42)); + + var r = (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single(); + + r.StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + r.Value.ShouldBe((short)42); + f.Tags["N7:0"].ReadCount.ShouldBe(1); + f.Tags["N7:5"].ReadCount.ShouldBe(1); + f.Tags.Keys.ShouldNotContain(k => k.Contains('[')); + } + + [Fact] + public async Task File_indirect_resolves_the_file_number() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N[N7:0]:5", AbLegacyDataType.Int)); + Seed(f, ("N7:0", (short)9), ("N9:5", (short)77)); + + var r = (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single(); + + r.StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + r.Value.ShouldBe((short)77); + f.Tags.ContainsKey("N9:5").ShouldBeTrue(); + } + + [Fact] + public async Task Index_is_re_read_every_poll_never_cached() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int)); + Seed(f, ("N7:0", (short)5), ("N7:5", (short)42), ("N7:6", (short)43)); + + (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single().Value.ShouldBe((short)42); + f.Tags["N7:0"].Value = (short)6; + (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single().Value.ShouldBe((short)43); + + f.Tags["N7:0"].ReadCount.ShouldBe(2); + // the stale resolution's handle is released when the index moves + f.Tags["N7:5"].Disposed.ShouldBeTrue(); + } + + [Theory] + [InlineData(-1)] + [InlineData(256)] + [InlineData(30000)] + public async Task Out_of_range_index_is_BadOutOfRange_without_health_impact(int index) + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int)); + Seed(f, ("N7:0", (short)index)); + + var r = (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single(); + + r.StatusCode.ShouldBe(AbLegacyStatusMapper.BadOutOfRange); + drv.GetHealth().State.ShouldNotBe(DriverState.Degraded); + f.Tags.Keys.ShouldNotContain(k => k.StartsWith("N7:") && k != "N7:0"); + } + + [Fact] + public async Task Inner_read_failure_propagates_its_status() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int)); + f.Customise = p => new FakeAbLegacyTag(p) { Value = (short)5, Status = p.TagName == "N7:0" ? -14 : 0 }; + + var r = (await drv.ReadAsync(["Ind"], CancellationToken.None)).Single(); + + r.StatusCode.ShouldNotBe(AbLegacyStatusMapper.Good); + r.StatusCode.ShouldNotBe(AbLegacyStatusMapper.BadOutOfRange); + f.Tags.ContainsKey("N7:5").ShouldBeFalse(); + } + + [Fact] + public async Task Write_resolves_index_then_writes_the_composed_address() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int, Writable: true)); + Seed(f, ("N7:0", (short)5)); + + var w = (await drv.WriteAsync([new WriteRequest("Ind", 99)], CancellationToken.None)).Single(); + + w.StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + f.Tags["N7:5"].WriteCount.ShouldBe(1); + f.Tags["N7:5"].Value.ShouldBe((short)99); + f.Tags["N7:0"].WriteCount.ShouldBe(0); + } + + [Fact] + public async Task Write_with_bad_index_is_BadOutOfRange_and_writes_nothing() + { + var (drv, f) = await NewDriverAsync(new AbLegacyTagDefinition("Ind", Host, "N7:[N7:0]", AbLegacyDataType.Int, Writable: true)); + Seed(f, ("N7:0", (short)-3)); + + var w = (await drv.WriteAsync([new WriteRequest("Ind", 1)], CancellationToken.None)).Single(); + + w.StatusCode.ShouldBe(AbLegacyStatusMapper.BadOutOfRange); + f.Tags.Values.ShouldAllBe(t => t.WriteCount == 0); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyFamilyAddressGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyFamilyAddressGateTests.cs index b0404868..cd29213f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyFamilyAddressGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyFamilyAddressGateTests.cs @@ -28,10 +28,10 @@ public sealed class AbLegacyFamilyAddressGateTests } [Fact] - public void Gate_warns_about_indirect_addresses_which_never_read() + public void Gate_does_not_warn_about_indirect_addresses_which_the_driver_resolves() { EquipmentTagConfigInspector.Inspect(DriverTypeNames.AbLegacy, Cfg("N7:[N7:0]"), "{\"PlcFamily\":\"Slc500\"}") - .ShouldContain(x => x.Contains("indirect")); + .ShouldBeEmpty(); } [Theory] From 73fdb8015737be51f540556bccf4a28fe4f09cd3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 16:56:58 -0400 Subject: [PATCH 2/2] fix(ablegacy): indirect element index up to 999 on PLC-5; clear IndirectLastName on dispose Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/AbLegacy.md | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index 4ce9d5ac..ddf566c2 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -121,7 +121,7 @@ physical inputs and may reject the write. - **Indirect addressing (#247 parse, #646 runtime).** `N7:[N7:0]` (word from another address) and `N[N7:0]:5` (file from another address) parse, one level deep (the inner address must be direct, and carries the family's octal rule). At runtime the driver reads the inner address first (same runtime factory, so libplctag and Native alike), requires - an integer in range (word/element `0..255`, file `0..999`), composes the concrete address and runs the normal + an integer in range (word/element `0..255`, or `0..999` on PLC-5/LogixPccc; file `0..999`), composes the concrete address and runs the normal read/write path against it. **The index is re-read on every read/write - never cached** - and the previous resolution's handle is released when the index moves. A bad index (negative, too large, float/string source) is `BadOutOfRange` for that tag only (no health impact); an inner-read failure propagates its own status (a wire diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 80dcc136..4afe4fe7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -862,6 +862,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// Largest word/element index an indirect source may yield (PCCC element offset is one byte). private const int MaxIndirectElement = AbLegacyArray.MaxElements - 1; + /// PLC-5 data files hold up to 1000 words (extended PCCC offset), unlike SLC/MicroLogix (256). + private const int MaxIndirectElementPlc5 = 999; + /// Largest data-file number an indirect source may yield (PLC-5 allows up to 999). private const int MaxIndirectFile = 999; @@ -877,6 +880,8 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { var fileNumber = parsed.FileNumber; var word = parsed.WordNumber; + var maxElement = device.Options.PlcFamily is AbLegacyPlcFamily.Plc5 or AbLegacyPlcFamily.LogixPccc + ? MaxIndirectElementPlc5 : MaxIndirectElement; if (parsed.IndirectFileSource is { } fileSrc) { @@ -890,7 +895,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { var r = await ReadIndexAsync(device, wordSrc, ct).ConfigureAwait(false); if (r.Status != 0) return new IndirectResolution(null, r.Status, r.TransportFault); - if (r.Value is < 0 or > MaxIndirectElement) + if (r.Value < 0 || r.Value > maxElement) return new IndirectResolution(null, AbLegacyStatusMapper.BadOutOfRange, false); word = (int)r.Value!; } @@ -1329,6 +1334,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover Runtimes.Clear(); foreach (var r in ParentRuntimes.Values) r.Dispose(); ParentRuntimes.Clear(); + IndirectLastName.Clear(); // Dispose + clear the per-parent RMW gates and the per-runtime/creation locks so // ReinitializeAsync cycles don't orphan their SemaphoreSlim instances (each leaks a // wait handle once contended).