From 5018627b35563ccb1d7da5872a96161411e65029 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 9 Oct 2026 04:23:59 -0400 Subject: [PATCH 1/2] feat(abcip): #611 discovered UDT member writes + whole-UDT refusal by decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 2 — controller-discovered UDT members are writable on both transports once browse-committed and authored ReadWrite (writability is the operator's: the commit lands AccessLevel Read with no writable key). The write names the member (one CIP symbolic segment per member), its own type, its own bytes. BIT members are supported: Z-35162 (read-only probe) exposes a UDT BIT member as a 1-byte 0xC1 BOOL, so the controller applies the bit and the hidden host's other bits are never in the request. - AbCipDriver: pre-write width check on BOTH transports (libplctag patched part of its read buffer and sent it all): BadTypeMismatch, Bool over a wider value BadNotSupported; nothing sent, handle kept, health untouched, warn once. - A runtime NotSupportedException is a refusal: no longer latches Degraded. Item 3 — whole-UDT writes refused BadNotSupported before any runtime exists, by decision (a structure write overwrites hidden hosts + padding), documented. ab_server patch 0003: serve member paths from dotted flat tags (UdtSeg.*), and accept libplctag's pad byte on 1-byte writes (stock ab_server refused every libplctag BOOL/SINT write with 0x03). New AbCipUdtMemberWriteTests (both transports), AbCipDiscoveredMemberWriteTests, native request-byte theory, and a browse-commit mapper pin. Docs: AbCip.md Member writes / Whole-UDT writes / Known gaps, fixture docs, CLAUDE.md AB section. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 6 +- docs/drivers/AbCip.md | 55 +++- docs/drivers/AbServer-Test-Fixture.md | 16 +- .../AbCipDriver.cs | 100 ++++++- .../LibplctagTagRuntime.cs | 7 +- .../AbCipUdtMemberWriteTests.cs | 222 +++++++++++++++ .../Docker/README.md | 22 +- .../Docker/docker-compose.yml | 14 +- ...ember-paths-against-dotted-flat-tags.patch | 93 +++++++ .../AbCipDiscoveredMemberWriteTests.cs | 259 ++++++++++++++++++ .../NativeAbCipTagRuntimeTests.cs | 25 ++ .../Uns/RawBrowseCommitMapperTests.cs | 37 +++ 12 files changed, 832 insertions(+), 24 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipUdtMemberWriteTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0003-ab_server-resolve-member-paths-against-dotted-flat-tags.patch create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index cda3152a..9d1513cf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1098,7 +1098,11 @@ re-emit a concrete default. The defaults differ per driver (final review, 2026-1 Refusals are not comms faults: an Unsupported refusal (`BadNotSupported`) and an AbCip native **type mismatch** (`BadTypeMismatch`, where libplctag returned Good garbage for a same-width mis-authored type) both keep the tag handle, leave driver health alone (no `Degraded`, nothing for the comms-loss watchdog) and warn -once per tag — only genuine transport failures evict and degrade. `DeviceSessionPool.Shared` logs through the +once per tag — only genuine transport failures evict and degrade (a runtime `NotSupportedException` too, #611 — +it used to latch `Degraded`). AbCip width-checks every scalar **write** against the controller's bytes before +sending, on both transports (#611; libplctag used to patch part of its buffer and send it all). Controller-discovered +UDT members are writable once browse-committed and authored `ReadWrite` (BIT members included — the controller +applies the bit); whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT factories' `Register(registry, ILoggerFactory?)` take one too, #602 CONV-3). The AbCip `browsePageDelayMs` knob has an AdminUI field (`AbCipDriverForm` "Browse page delay (ms)"). libplctag is confined to `Libplctag*.cs` adapters by `LibplctagConfinementGuardTests`; do not reference it elsewhere. See `docs/drivers/AbCip.md` §Transport, `docs/drivers/AbLegacy.md` §Transport + §Wire evidence, `docs/Configuration.md` §`Drivers:AbCip`, and diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 276a1735..98cb235d 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -242,6 +242,9 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi (2026-10-08) and nothing else. Results: [native-ab-transport-gate-results](../plans/2026-10-07-native-ab-transport-gate-results.md). - **libplctag cannot be deleted** until a bench controller allows write verification. +- Discovered UDT **member writes** (#611) are likewise `ab_server`-only on both transports; the + BIT-member argument rests on a read-only hardware observation (the member reads as a 1-byte `0xC1` + BOOL). See [Member writes](#member-writes) for the candidate gate tag. - ⚠️ RESIDUAL (#567): wide-struct **fragmented writes** (8-byte alignment) are unit-tested only. Production presses are read-only and there is no bench controller, so this cannot be proven; it stays open until a bench controller exists. @@ -374,8 +377,54 @@ Reading the UDT container tag itself (without a member suffix) returns ### Member writes -UDT member **writes** discovered via controller browse are **deferred** in this release. -Pre-declared `Members` with `Writable: true` retain full read/write support as before. +**Discovered UDT members are writable, on both transports (Gitea #611).** There is no separate +"discovered member" write path to enable — under v3 a browsed member leaf becomes an ordinary raw tag +when it is browse-committed in `/raw`, and that tag writes like any other atomic tag: + +- **Writability is the operator's, never the browse's.** Browse-commit (`RawBrowseCommitMapper`) writes + `tagPath` = the member path (`Seg.RampTime`, nested `Seg.Sub.Code`, program-scoped + `Program:Main.Seg.ID`) and `dataType` = the template's atomic type, at **AccessLevel `Read`** and with no + `writable` key. The node is writable only once the operator sets the raw tag to `ReadWrite` (the + node-manager gate); an authored `"writable": false` still refuses at the driver (`BadNotWritable`). The + browse's `SecurityClass` (members inherit the parent symbol's read-only bit) is not consulted by the + commit. An element of a member array is authored by hand as `Seg.Points[2]`; a whole member array is + not writable (AbCip has no array encoder, `DriverArrayWriteSupport`). +- **The request names the member.** One CIP symbolic segment per member (plus an element segment), the + member's own CIP type, the member's own bytes — never the parent UDT, never a hidden host. Pinned at the + request-byte level by `NativeAbCipTagRuntimeTests.A_member_path_write_names_the_member_and_sends_only_its_own_bytes` + and end to end on ab_server by `AbCipUdtMemberWriteTests` (both transports, value read back through a fresh + driver). +- **BIT members** (a UDT `BIT`, which Logix packs into a hidden `ZZZZZZZZZZ…` SINT host) **are supported.** + A controller exposes the member as a standalone `BOOL`: on Z-35162, `…_LastSegment.Enable` reads type + `0xC1`, **one byte**, while the hidden host at the same offset holds that bit (read-only probe, 2026-10-09). + So a `BOOL` write to `Seg.Enable` is a symbolic write of one BOOL byte to the member, and the controller — + not the driver — applies the bit; the host's other bits are never in the request. No driver-side + read-modify-write is involved (that is reserved for `Tag.N` bit selectors on an integer, below), and the + hidden host itself is never browsed, so it cannot be authored by browse-commit. +- **Type checking, before anything is sent, on both transports.** The driver compares the authored type + with the bytes the controller holds at the path (the runtime's last read) and refuses a mismatch without + writing: a narrower/wider atomic type is `BadTypeMismatch` (libplctag would otherwise patch part of its read + buffer and send the whole buffer back — e.g. an `Int` over a DINT member wrote four bytes with a stale high + word); a `Bool` over a wider value (a packed BOOL-array DWORD, a mistyped integer) is `BadNotSupported`, + because writing it would rewrite every neighbouring bit. Native additionally refuses a same-width + mismatch (REAL vs DINT) from the CIP type code — see [Native transport behaviour](#native-transport-behaviour). + A refusal keeps the handle, warns once per tag and **does not touch driver health**. +- **Not verified on a real controller.** Production presses are read-only, so member writes are proven on + `ab_server` only (Docker patch 0003 serves member paths). A candidate for a future *approved* write gate: + Z-35162 controller tag `_EmptyTemperatureSegment` (`UDT_TemperatureProfile_Segment`), which no logic + references — its members (`ID`, `Enable`, `RampTime`, …) are all zero and resolve as described above. + +Pre-declared `Members` with `Writable: true` keep working exactly as before (same write path). + +### Whole-UDT writes — refused, by decision + +A write to a UDT **container** tag (`dataType: Structure`) is refused `BadNotSupported` before any runtime is +created or anything is sent, with no health change and one Warning per tag. This is a **decision, not a +deferral** (#611): a whole-structure write overwrites every member of the controller's instance — hidden BOOL +hosts and alignment padding included — with whatever bytes the client supplied, and the member writes above +cover the use case without that blast radius. A runtime-level `NotSupportedException` (the libplctag and +native encoders both refuse `Structure`) is likewise a refusal: `BadNotSupported`, handle kept, health untouched +(it used to latch `Degraded`). ### Configuration @@ -388,7 +437,7 @@ UDT-typed tags found in the walk. - **Native heap is invisible to the GC.** libplctag's native `Tag` heap does not show up in CLR allocation figures. Watch whole-process RSS, and use `ReinitializeAsync` (tears down + re-creates every device's libplctag handles) as the remediation for native-heap growth. (There is no per-driver footprint number to consult: `IDriver.GetMemoryFootprint()` was deleted as unconsumed in Gitea #525.) - **Handle eviction on failure** — a non-zero libplctag status or a transport exception evicts the cached tag runtime so the next read/write re-creates a fresh handle, mirroring the probe loop's recreate-on-failure behaviour. - **Read statuses carry the libplctag cause** — a tag the controller does not have reads `BadNodeIdUnknown` (not `BadCommunicationError`), and it does **not** open the device's connect-backoff window (the controller answered; before this, one typo throttled every sibling tag on the device). A genuine connection failure still opens it. -- **Width check on scalar reads** — libplctag never checks the CIP type the controller returns. A scalar atomic read whose buffer is not the authored type's width (e.g. a REAL authored as INT — the legacy Kepware export does exactly this for some press tags) reads `BadTypeMismatch` instead of Good garbage. Same-width mistakes (DINT vs REAL) are undetectable under libplctag; the `Native` transport reports them as `BadTypeMismatch` (see [Native transport behaviour](#native-transport-behaviour)). The width-mismatch refusal is pinned by `AbCipTypeMismatchTests.A_width_mismatched_read_keeps_the_handle_leaves_health_alone_and_warns_once`: `BadTypeMismatch`, handle kept, no `Degraded`, one Warning per tag. *Benign edge (#585):* a driver left `Degraded` by an earlier outage whose only remaining tags are mis-typed gets one extra comms-loss-watchdog reconnect ~90 s later before settling, because a type refusal deliberately writes no health and so never clears the stale `Degraded`. +- **Width check on scalar reads** — libplctag never checks the CIP type the controller returns. A scalar atomic read whose buffer is not the authored type's width (e.g. a REAL authored as INT — the legacy Kepware export does exactly this for some press tags) reads `BadTypeMismatch` instead of Good garbage. Same-width mistakes (DINT vs REAL) are undetectable under libplctag; the `Native` transport reports them as `BadTypeMismatch` (see [Native transport behaviour](#native-transport-behaviour)). The width-mismatch refusal is pinned by `AbCipTypeMismatchTests.A_width_mismatched_read_keeps_the_handle_leaves_health_alone_and_warns_once`: `BadTypeMismatch`, handle kept, no `Degraded`, one Warning per tag. The same width check runs **before every scalar write, on both transports** (#611): a mismatch is refused `BadTypeMismatch` (a `Bool` over a wider value `BadNotSupported`) with nothing sent — see [Member writes](#member-writes). *Benign edge (#585):* a driver left `Degraded` by an earlier outage whose only remaining tags are mis-typed gets one extra comms-loss-watchdog reconnect ~90 s later before settling, because a type refusal deliberately writes no health and so never clears the stale `Degraded`. - **Module-defined I/O tags** — `Local:1:I.Data`, `ABB_OmniCore:I.ConnectionFaulted` and similar colon-bearing base names are valid tag paths (controller scope only). - **User string arrays** — `STR0016[N]` and similar arrays stride by the actual element size (read buffer ÷ count); libplctag.NET 1.5.2 leaves `ElementSize` unset after a read. - **Declaration-only UDT grouping is a footgun unless verified** — only enable `EnableDeclarationOnlyUdtGrouping` when every UDT's member declaration order has been hand-verified against the controller's compiled layout. diff --git a/docs/drivers/AbServer-Test-Fixture.md b/docs/drivers/AbServer-Test-Fixture.md index 7c22622e..e198c6c2 100644 --- a/docs/drivers/AbServer-Test-Fixture.md +++ b/docs/drivers/AbServer-Test-Fixture.md @@ -43,16 +43,20 @@ libplctag passed on all seven profiles; on `micro800` libplctag failed on the em smoke test passes on both transports. Evidence: [native-ab-transport-gate-results](../plans/2026-10-07-native-ab-transport-gate-results.md). -- Write-capable tests (the Logix Emulate ALMD test) require `AB_SERVER_ALLOW_WRITES=1` and refuse while - `AB_DIFF_ENDPOINT` is set. +- Write-capable tests (the Logix Emulate ALMD test, and `AbCipUdtMemberWriteTests` — #611 discovered-member + writes, both transports, against the `UdtSeg.*` member tags of the `controllogix` profile) require + `AB_SERVER_ALLOW_WRITES=1` and refuse while `AB_DIFF_ENDPOINT` is set. - **Differential harness** (`AbCip.IntegrationTests/Differential`, `LiveIntegration`) compares libplctag and native on browse, template and read parity against one endpoint. It is read-only by construction, aborts on the first connection failure, never retries, and caps the native phase at one Forward Open. Env: `AB_DIFF_ENDPOINT`, `AB_DIFF_PATH`, `AB_DIFF_FAMILY`, `AB_DIFF_TAGS`, `AB_DIFF_MAX_TAGS` (at most 500), `AB_DIFF_MAX_TEMPLATES`, `AB_DIFF_PACE_MS` (default 20), `AB_DIFF_COUNT_OPENS`. Always run with `--filter "FullyQualifiedName~Differential"` and `AB_SERVER_ENDPOINT=127.0.0.1:1`. -- ab_server serves no UDTs and refuses 1-byte writes, so template parity is vacuous there and BOOL/SINT - parity is weak evidence. Native writes remain simulator-only. +- ab_server serves no UDTs, so template parity is vacuous there. Docker patch 0003 serves a UDT **member + path** from a flat tag named by that dotted path (`UdtSeg.ID`, `UdtSeg.Sub.Code`, `UdtSeg.Points[2]`) — the + wire of a member read/write, not a structure — and accepts the pad byte libplctag adds to a 1-byte write, + which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Native writes remain + simulator-only. - **Golden-frame capture:** `scripts/ab-capture/capture.sh` (capture client in `scripts/ab-capture/client`); fixtures live in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/` with a README. @@ -76,7 +80,9 @@ ControlLogix profile `Notes`: *"ab_server lacks full UDT emulation."* Unverified against `ab_server`: -- PR 6 structured read/write (`AbCipStructureMember` fan-out) +- PR 6 structured read/write (`AbCipStructureMember` fan-out) — except the **member-path wire** of a + write, which patch 0003 serves (`AbCipUdtMemberWriteTests`, #611); offsets and BIT-member semantics are + not emulated - #179 Template Object shape reader (`CipTemplateObjectDecoder` + `FetchUdtShapeAsync`) - #194 whole-UDT read optimization (`AbCipUdtReadPlanner` + `AbCipUdtMemberLayout` + the `ReadGroupAsync` path in `AbCipDriver`) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 2fade80c..64a22ca0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -218,6 +218,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, _logger.LogWarning(message, args); } + // Tags whose write-refusal Warning has already been logged (Gitea #611) — a client may retry forever. + private readonly System.Collections.Concurrent.ConcurrentDictionary _writeRefusalWarned = + new(StringComparer.OrdinalIgnoreCase); + + /// Logs a write-refusal Warning the first time reports one on this instance. + private void WarnWriteRefusedOnce(string tagName, string message, params object?[] args) + { + if (_writeRefusalWarned.TryAdd(tagName, 0)) + _logger.LogWarning(message, args); + } + // Tags whose malformed-name Warning has already been logged (Gitea #570) — polled forever, warned once. private readonly System.Collections.Concurrent.ConcurrentDictionary _configErrorWarned = new(StringComparer.OrdinalIgnoreCase); @@ -1137,6 +1148,19 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, continue; } + // Gitea #611 item 3 — whole-UDT writes are refused by DECISION, not deferred: a structure write + // overwrites every member of the controller's instance, hidden BOOL hosts and padding included, + // with whatever the client sent. Address the members instead (each is its own raw tag). Refused + // before a runtime exists, so nothing is read or sent; a refusal is not a fault — no health change. + if (def.DataType == AbCipDataType.Structure) + { + results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device} refused: whole-UDT writes are not supported — write its members (logged once per tag)", + w.FullReference, def.DeviceHostAddress); + continue; + } + try { var parsedPath = AbCipTagPath.TryParse(def.TagPath); @@ -1162,18 +1186,43 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var opLock = device.GetRuntimeLock(def.Name); await opLock.WaitAsync(cancellationToken).ConfigureAwait(false); int status; + uint? refusal; + int size; try { - runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); - await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); - status = runtime.GetStatus(); + // Gitea #611: the bytes the controller holds for this path must be what the authored type + // encodes, BEFORE anything is sent — on BOTH transports. libplctag never checks: it encodes + // into the buffer the last read sized and sends the whole buffer, so an INT authored over a + // DINT patched two bytes and wrote four (stale high word), and a BOOL over a packed + // BOOL-array DWORD wrote all 32 bits. Discovered UDT members are where a mis-authored type + // is most likely (browse-commit fills dataType from the template, an operator may retype). + size = runtime.GetSize(); + refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size); + if (refusal is null) + { + runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); + await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); + status = runtime.GetStatus(); + } + else + { + status = 0; + } } finally { opLock.Release(); } - if (status == (int)AbStatus.TypeMismatch) + if (refusal is { } refused) + { + // Deterministic, nothing sent: keep the handle, leave health alone, warn once per tag. + results[i] = new WriteResult(refused); + WarnTypeMismatchOnce(def.Name, + "AbCip write to {Tag} on device {Device} refused before sending: the controller holds {Size} byte(s) there but the authored {DataType} encodes {Width} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, size, def.DataType, ExpectedScalarWriteWidth(def.DataType)); + } + else if (status == (int)AbStatus.TypeMismatch) { // Final review I4: deterministic — keep the handle, leave health, warn once per tag. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); @@ -1202,12 +1251,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (NotSupportedException nse) { - // Type/protocol error — not a transport fault; don't evict the handle. + // A deterministic local refusal (a runtime that cannot encode this write without clobbering + // something — e.g. a BOOL-array element as a single BOOL) — not a transport fault: keep the + // handle and leave driver health alone (Gitea #611; same rule as the Unsupported refusal + // below). It used to latch Degraded, which a refusal the device never saw must not do. results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); - _logger.LogWarning(nse, - "AbCip write not supported for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write not supported for tag {Tag} on device {Device}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, nse.Message); } catch (FormatException fe) { @@ -1642,6 +1693,37 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, _ => null, }; + /// + /// Bytes one scalar write of encodes: the read-side width, plus BOOL (one byte — + /// a Logix BOOL tag or UDT BIT member reads back as one byte, type 0xC1). null where the encoded + /// size is not fixed (STRING, DT, structures). + /// + /// The authored type. + /// The width in bytes, or null. + internal static int? ExpectedScalarWriteWidth(AbCipDataType type) => + type == AbCipDataType.Bool ? 1 : ExpectedScalarWidth(type); + + /// + /// Gitea #611 — the refusal status for a scalar write whose authored type does not fit what the controller + /// holds at that path ( bytes, from the runtime's last read), or null to + /// write. A BOOL over a wider value (a packed BOOL-array DWORD) is BadNotSupported — writing it + /// would clobber the neighbouring bits, the same refusal the native runtime raises; any other width + /// mismatch is BadTypeMismatch. Arrays, bit selectors (Flags.3, served by the + /// read-modify-write path) and an unknown size (<= 0) are not judged here. + /// + /// The tag being written. + /// The parsed bit selector, if any. + /// The runtime's read-buffer size in bytes. + /// The refusal status, or null. + internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size) + { + if (IsArrayTag(def) || bitIndex is not null || size <= 0) return null; + if (ExpectedScalarWriteWidth(def.DataType) is not { } width || size == width) return null; + return def.DataType == AbCipDataType.Bool + ? AbCipStatusMapper.BadNotSupported + : AbCipStatusMapper.BadTypeMismatch; + } + /// /// Recursively fan a controller-discovered UDT shape out into Variable nodes — one per /// ATOMIC leaf — under the folder produced by . Mirrors the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index 1b4d051e..0aa19df2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -277,9 +277,10 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime _tag.SetInt32(0, Convert.ToInt32(value)); break; case AbCipDataType.Structure: - // Whole-UDT writes are not implemented — operators address individual UDT - // members via dotted tag paths, which route per-member through the atomic - // encode cases above. A whole-UDT writer is a deferred follow-up. + // Whole-UDT writes are refused by decision (Gitea #611), not deferred: a structure + // write overwrites every member, hidden BOOL hosts and padding included. Members are + // written via their own dotted tag paths through the atomic cases above. The driver + // refuses a Structure tag before it ever reaches here; this is the backstop. throw new NotSupportedException( "Whole-UDT writes are not supported — address individual member paths instead."); default: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipUdtMemberWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipUdtMemberWriteTests.cs new file mode 100644 index 00000000..87bd4371 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipUdtMemberWriteTests.cs @@ -0,0 +1,222 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests; + +/// +/// Gitea #611 item 2 — writes to controller-discovered UDT members, end to end against ab_server on both +/// transports. A discovered member reaches the driver the way browse-commit authors it: an ordinary raw tag +/// whose TagConfig carries the member path as tagPath and the browsed atomic type as +/// dataType (RawBrowseCommitMapper), with no writable key. ab_server has no structures, +/// so the UdtSeg.* members are flat tags served on their member path by Docker patch 0003 — what is +/// proved here is the wire: one symbolic segment per member (plus the element index for +/// UdtSeg.Points[2]), the member's own CIP type, and a value that reads back through a fresh handle. +/// +/// +/// Writes: skipped unless AB_SERVER_ALLOW_WRITES=1 (), so pointing +/// AB_SERVER_ENDPOINT at a real controller never writes to it. BIT-member neighbour preservation is +/// NOT claimed here: ab_server's UdtSeg.Enable / UdtSeg.Ready are separate tags, not bits of +/// one host SINT, so such a test would pass whatever the driver did. The BIT-member argument rests on the +/// driver addressing the member (never its hidden host) — pinned at the request-byte level by +/// NativeAbCipTagRuntimeTests — see docs/drivers/AbCip.md §"Member writes". +/// +[Trait("Category", "Integration")] +public sealed class AbCipUdtMemberWriteTests +{ + /// A discovered member authored writable writes, and the value reads back through a fresh driver. + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_discovered_member_write_reaches_the_member_and_reads_back(AbCipTransport transport) + { + IntegrationWriteGate.SkipUnlessWritesAllowed(); + var fixture = new AbServerFixture(); + var device = Device(fixture); + // A per-run value so a stale server value from an earlier run cannot satisfy the read-back. + var seed = Random.Shared.Next(1_000, 30_000); + + var writer = await StartAsync(device, transport, "writer", + Member("RampTime", "UdtSeg.RampTime", "Real"), + Member("Id", "UdtSeg.ID", "DInt"), + Member("Code", "UdtSeg.Sub.Code", "Int"), + Member("Point2", "UdtSeg.Points[2]", "DInt"), + Member("Enable", "UdtSeg.Enable", "Bool")); + await RequireMemberTagsAsync(writer); + try + { + var results = await writer.WriteAsync( + [ + new WriteRequest("RampTime", seed + 0.5f), + new WriteRequest("Id", seed), + new WriteRequest("Code", (short)(seed % 30_000)), + new WriteRequest("Point2", -seed), + new WriteRequest("Enable", seed % 2 == 0), + ], CancellationToken.None); + string[] names = ["RampTime", "Id", "Code", "Point2", "Enable"]; + results.Select((r, i) => $"{names[i]}=0x{r.StatusCode:X8}") + .ShouldBe(names.Select(n => $"{n}=0x{AbCipStatusMapper.Good:X8}")); + writer.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await writer.ShutdownAsync(CancellationToken.None); + } + + // A FRESH driver — new handles, nothing cached from the writer — proves the server holds the values. + var reader = await StartAsync(device, transport, "reader", + Member("RampTime", "UdtSeg.RampTime", "Real"), + Member("Id", "UdtSeg.ID", "DInt"), + Member("Code", "UdtSeg.Sub.Code", "Int"), + Member("Point2", "UdtSeg.Points[2]", "DInt"), + Member("Enable", "UdtSeg.Enable", "Bool")); + try + { + var read = await reader.ReadAsync(["RampTime", "Id", "Code", "Point2", "Enable"], CancellationToken.None); + read.Select(r => r.StatusCode).ShouldAllBe(s => s == AbCipStatusMapper.Good); + Convert.ToSingle(read[0].Value).ShouldBe(seed + 0.5f); + Convert.ToInt32(read[1].Value).ShouldBe(seed); + Convert.ToInt32(read[2].Value).ShouldBe(seed % 30_000); + Convert.ToInt32(read[3].Value).ShouldBe(-seed); + Convert.ToBoolean(read[4].Value).ShouldBe(seed % 2 == 0); + } + finally + { + await reader.ShutdownAsync(CancellationToken.None); + } + } + + /// + /// An element write to a member array touches that element only: the neighbours read back unchanged. + /// + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_member_array_element_write_leaves_the_other_elements_alone(AbCipTransport transport) + { + IntegrationWriteGate.SkipUnlessWritesAllowed(); + var device = Device(new AbServerFixture()); + var seed = Random.Shared.Next(1_000, 30_000); + var drv = await StartAsync(device, transport, "elements", + Member("P0", "UdtSeg.Points[0]", "DInt"), + Member("P1", "UdtSeg.Points[1]", "DInt"), + Member("P2", "UdtSeg.Points[2]", "DInt"), + Member("P3", "UdtSeg.Points[3]", "DInt")); + await RequireMemberTagsAsync(drv, "P0"); + try + { + (await drv.WriteAsync( + [new WriteRequest("P0", seed), new WriteRequest("P1", seed + 1), + new WriteRequest("P2", seed + 2), new WriteRequest("P3", seed + 3)], CancellationToken.None)) + .Select(r => r.StatusCode).ShouldAllBe(s => s == AbCipStatusMapper.Good); + + (await drv.WriteAsync([new WriteRequest("P2", -7)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + + var read = await drv.ReadAsync(["P0", "P1", "P2", "P3"], CancellationToken.None); + read.Select(r => Convert.ToInt32(r.Value)).ShouldBe([seed, seed + 1, -7, seed + 3]); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } + + /// + /// A member authored narrower than the controller's type (INT over the DINT UdtSeg.ID) is refused + /// BadTypeMismatch with nothing written, on BOTH transports — libplctag included, which would + /// otherwise patch the low two bytes of the 4-byte buffer and send the DINT back. + /// + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_width_mistyped_member_write_is_refused_and_writes_nothing(AbCipTransport transport) + { + IntegrationWriteGate.SkipUnlessWritesAllowed(); + var device = Device(new AbServerFixture()); + var seed = Random.Shared.Next(100_000, 1_000_000); // needs the high word, so a 2-byte patch would show + var drv = await StartAsync(device, transport, "mistyped", + Member("IdRight", "UdtSeg.ID", "DInt"), + Member("IdWrong", "UdtSeg.ID", "Int")); + await RequireMemberTagsAsync(drv, "IdRight"); + try + { + (await drv.WriteAsync([new WriteRequest("IdRight", seed)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + + (await drv.WriteAsync([new WriteRequest("IdWrong", (short)5)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + Convert.ToInt32((await drv.ReadAsync(["IdRight"], CancellationToken.None)).Single().Value).ShouldBe(seed); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } + + /// A discovered member the operator authored read-only (writable:false) is never written. + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_member_authored_read_only_is_not_written(AbCipTransport transport) + { + IntegrationWriteGate.SkipUnlessWritesAllowed(); + var device = Device(new AbServerFixture()); + var seed = Random.Shared.Next(1_000, 30_000); + var drv = await StartAsync(device, transport, "readonly", + Member("Id", "UdtSeg.ID", "DInt"), + Member("IdRo", "UdtSeg.ID", "DInt", writable: false)); + await RequireMemberTagsAsync(drv); + try + { + (await drv.WriteAsync([new WriteRequest("Id", seed)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + (await drv.WriteAsync([new WriteRequest("IdRo", seed + 1)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable); + Convert.ToInt32((await drv.ReadAsync(["Id"], CancellationToken.None)).Single().Value).ShouldBe(seed); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } + + private static string Device(AbServerFixture fixture) => $"ab://{fixture.Host}:{fixture.Port}/1,0"; + + /// A raw tag exactly as browse-commit authors a discovered member: tagPath + dataType, no writable key. + private static RawTagEntry Member(string rawPath, string tagPath, string dataType, bool? writable = null) => + new(rawPath, + writable is { } w + ? $$"""{"tagPath":"{{tagPath}}","dataType":"{{dataType}}","writable":{{(w ? "true" : "false")}}}""" + : $$"""{"tagPath":"{{tagPath}}","dataType":"{{dataType}}"}""", + WriteIdempotent: false); + + private static async Task StartAsync( + string device, AbCipTransport transport, string id, params RawTagEntry[] tags) + { + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(device, AbCipPlcFamily.ControlLogix)], + RawTags = [.. tags.Select(t => t with { DeviceName = device })], + Timeout = TimeSpan.FromSeconds(5), + Transport = transport, + Probe = new AbCipProbeOptions { Enabled = false }, + }, $"drv-udtwrite-{id}-{transport}"); + await drv.InitializeAsync("{}", CancellationToken.None); + return drv; + } + + /// + /// Fails (not skips) with a rebuild hint when the running ab_server predates patch 0003: write tests are + /// opt-in, so whoever opted in is pointing at a simulator that should carry the member tags. + /// + private static async Task RequireMemberTagsAsync(AbCipDriver drv, string probe = "Id") + { + var status = (await drv.ReadAsync([probe], CancellationToken.None)).Single().StatusCode; + if (status == AbCipStatusMapper.BadNodeIdUnknown) + Assert.Fail("ab_server does not serve the UdtSeg.* member tags — rebuild the image from Docker/ " + + "(patch 0003) and run the controllogix profile."); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md index 3a6bad3f..c0ebb5f0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md @@ -72,7 +72,25 @@ all). `AbCip_WithNoAuthoredTags_ReportsTheControllerReachable` in `Host.IntegrationTests` is the pin, and it needs the fixture **rebuilt** from this patch, not merely running. -**Bumping `LIBPLCTAG_TAG` means re-checking both patches against the new tag.** It +### 0003 — member paths, and libplctag's pad byte + +`patches/0003-ab_server-resolve-member-paths-against-dotted-flat-tags.patch` does two +things for the discovered-UDT-member write suite (`AbCipUdtMemberWriteTests`, Gitea #611): + +- **Member paths.** A Logix member path (`Seg.Sub.Code`) arrives as one CIP symbolic + segment per member. ab_server has no structures, so the patch joins consecutive symbolic + segments with `.` and looks up a flat tag whose `--tag` name is that dotted path (the name + charset now admits `.`); element segments after the last member work as before + (`UdtSeg.Points[2]`). This proves the wire of a member read/write — not template offsets + and not BIT-member semantics (each `UdtSeg.*` BOOL is its own byte, not a bit of a hidden + host). The `controllogix` profile seeds `UdtSeg.ID` / `.Enable` / `.Ready` / `.RampTime` / + `.Sub.Code` / `.Points[4]`, modelled on the Z-35162 press's `UDT_TemperatureProfile_Segment`. +- **libplctag's pad byte.** libplctag pads an odd-length write payload (a 1-byte SINT / BOOL) + with one byte; stock ab_server counted it as data and refused every such write with CIP + `0x03` (libplctag `ErrorBadParam`), so no libplctag BOOL/SINT write could pass here. The + patch drops exactly one trailing pad byte when it alone would overrun an odd-sized target. + +**Bumping `LIBPLCTAG_TAG` means re-checking every patch against the new tag.** It is a concrete tag rather than the `release` branch precisely so that this stays a deliberate act — the branch moves, and the Dockerfile previously called it "pinned" while tracking it. @@ -141,7 +159,7 @@ place means updating both. | Family | Seeded tags | Notes | |---|---|---| -| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | +| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | | CompactLogix | `TestDINT` `TestREAL` `TestBOOL` | Narrow ConnectionSize cap enforced driver-side; ab_server accepts any size | | Micro800 | `TestDINT` `TestREAL` | ab_server has no `micro800` mode; falls back to `controllogix` emulation | | GuardLogix | `TestDINT` `SafetyDINT_S` | ab_server has no safety subsystem; `_S` suffix triggers driver-side classification only | diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml index 27e4ef18..8e6e45ae 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml @@ -22,6 +22,12 @@ services: - "44818:44818" # The last three are here for the controller-browse gate (Gitea #468) and # are additive — the read/write suites key off the Test* tags above. + # The UdtSeg.* tags (Gitea #611) stand in for the members of a UDT tag `UdtSeg` + # (modelled on the Z-35162 press's UDT_TemperatureProfile_Segment): ab_server has + # no structures, so patch 0003 serves a member path (one CIP symbolic segment per + # member) from the flat tag whose --tag name is that dotted path. They exist for + # the discovered-member write suite (AbCipUdtMemberWriteTests); a bare `UdtSeg` + # read stays unknown, as does any member not listed here. # Odd-length names are deliberate: Symbol Object entries are packed with no # padding, so a name with an odd byte count puts every following entry on an # odd offset. A decoder that assumes even alignment reads all six correctly @@ -39,7 +45,13 @@ services: "--tag=TestArray:DINT[16]", "--tag=Pump_1_Speed:REAL[1]", "--tag=Line1_Count:DINT[1]", - "--tag=Motor_Run:BOOL[1]" + "--tag=Motor_Run:BOOL[1]", + "--tag=UdtSeg.ID:DINT[1]", + "--tag=UdtSeg.Enable:BOOL[1]", + "--tag=UdtSeg.Ready:BOOL[1]", + "--tag=UdtSeg.RampTime:REAL[1]", + "--tag=UdtSeg.Sub.Code:INT[1]", + "--tag=UdtSeg.Points:DINT[4]" ] compactlogix: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0003-ab_server-resolve-member-paths-against-dotted-flat-tags.patch b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0003-ab_server-resolve-member-paths-against-dotted-flat-tags.patch new file mode 100644 index 00000000..629a0caa --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0003-ab_server-resolve-member-paths-against-dotted-flat-tags.patch @@ -0,0 +1,93 @@ +diff --git a/src/tools/ab_server/cip.c b/src/tools/ab_server/cip.c +index db44932..31a23b5 100644 +--- a/src/tools/ab_server/cip.c ++++ b/src/tools/ab_server/cip.c +@@ -1149,6 +1149,16 @@ slice_s handle_write_request(uint8_t cip_service, slice_s cip_service_path, slic + /* what is the actual starting byte offset for this specific request which might be a fragment. */ + request_start_byte_offset += request_fragment_start_byte_offset; + ++ /* OtOpcUa patch 0003: libplctag pads an odd-length write payload (a 1-byte SINT / BOOL) with one ++ * byte to keep the CIP message 16-bit aligned. A Logix controller ignores that pad byte; stock ++ * ab_server counted it as data and refused every 1-byte write from libplctag with 0x03. Drop a ++ * single trailing pad byte when exactly one would overrun an odd-sized target. */ ++ if((slice_len(write_request_payload_slice) + request_start_byte_offset) == request_end_byte_offset + 1 ++ && ((request_end_byte_offset - request_start_byte_offset) % 2) == 1) { ++ write_request_payload_slice = ++ slice_from_slice(write_request_payload_slice, 0, slice_len(write_request_payload_slice) - 1); ++ } ++ + /* Make sure we are not trying to write too much. */ + if((slice_len(write_request_payload_slice) + request_start_byte_offset) > request_end_byte_offset) { + log_info("Too much data in the write request!"); +@@ -1266,11 +1276,40 @@ bool parse_tag_path(slice_s tag_path, plc_s *plc, tag_def_s **tag, uint32_t *num + /* Align to 16-bit boundary if necessary */ + if(offset % 2 != 0) { offset++; } + ++ /* OtOpcUa patch 0003: a Logix member path ("Seg.ID", "Seg.Sub.Code") arrives as one symbolic ++ * segment per member. ab_server has no structures, so a member is served by a flat tag whose ++ * --tag name is the dotted path; join consecutive symbolic segments with '.' and look that up. ++ * A single-segment path takes exactly the original lookup below. */ ++ char joined_name[256] = {0}; ++ size_t joined_len = 0; ++ bool has_members = false; ++ if(offset < slice_len(tag_path) && slice_get_uint8(tag_path, offset) == CIP_SYMBOLIC_SEGMENT_MARKER) { ++ has_members = true; ++ if(name_len >= sizeof(joined_name)) { return false; } ++ memcpy(joined_name, tag_name_slice.data, name_len); ++ joined_len = name_len; ++ while(offset + 2 <= slice_len(tag_path) && slice_get_uint8(tag_path, offset) == CIP_SYMBOLIC_SEGMENT_MARKER) { ++ uint8_t member_len = slice_get_uint8(tag_path, offset + 1); ++ offset += 2; ++ if(offset + member_len > slice_len(tag_path) || joined_len + 1 + member_len >= sizeof(joined_name)) { ++ log_info("Member segment overruns the tag path!"); ++ return false; ++ } ++ joined_name[joined_len++] = '.'; ++ memcpy(joined_name + joined_len, tag_path.data + offset, member_len); ++ joined_len += member_len; ++ offset += member_len; ++ if(offset % 2 != 0) { offset++; } ++ } ++ joined_name[joined_len] = 0; ++ } ++ + /* find the tag */ + *tag = plc->tags; + + while(*tag) { +- if(slice_match_string_exact(tag_name_slice, (*tag)->name)) { ++ if(has_members ? (strcmp(joined_name, (*tag)->name) == 0) ++ : slice_match_string_exact(tag_name_slice, (*tag)->name)) { + log_info("Found tag %s", (*tag)->name); + break; + } +@@ -1279,7 +1318,11 @@ bool parse_tag_path(slice_s tag_path, plc_s *plc, tag_def_s **tag, uint32_t *num + } + + if(!*tag) { +- log_info("Tag %.*s not found!", slice_len(tag_name_slice), (const char *)(tag_name_slice.data)); ++ if(has_members) { ++ log_info("Tag %s not found!", joined_name); ++ } else { ++ log_info("Tag %.*s not found!", slice_len(tag_name_slice), (const char *)(tag_name_slice.data)); ++ } + return false; + } + +diff --git a/src/tools/ab_server/main.c b/src/tools/ab_server/main.c +index f8f8593..29cd157 100644 +--- a/src/tools/ab_server/main.c ++++ b/src/tools/ab_server/main.c +@@ -603,9 +603,10 @@ void parse_cip_tag(const char *tag_str, plc_s *plc) { + + /* try to match the three parts of a tag definition string. */ + +- /* first match the name. */ ++ /* first match the name. OtOpcUa patch 0003: '.' is accepted so a flat tag can stand in for a ++ * UDT member path ("Seg.ID") — see parse_tag_path in cip.c. */ + start = 0; +- len = strspn(tag_str + start, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"); ++ len = strspn(tag_str + start, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_."); + if(!len) { + // NOLINTNEXTLINE + fprintf(stderr, "Unable to parse tag definition string, cannot find tag name in \"%s\"!\n", tag_str); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs new file mode 100644 index 00000000..2fd0df3b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs @@ -0,0 +1,259 @@ +using System.Runtime.CompilerServices; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Gitea #611 — writes to controller-discovered UDT members (item 2) and the whole-UDT refusal (item 3). +/// A discovered member becomes writable the way every v3 tag does: browse-commit authors it as a raw tag +/// (tagPath = the member path, dataType = the template's atomic type, AccessLevel Read — see +/// RawBrowseCommitMapperTests.A_discovered_AbCip_UDT_member_commits_read_only_with_its_member_path_and_type) +/// and the operator flips it to ReadWrite. These tests start from what discovery emits and assert what the +/// driver does with that tag. +/// +[Trait("Category", "Unit")] +public sealed class AbCipDiscoveredMemberWriteTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + /// + /// Discovery → commit → write, end to end at the driver seam: every atomic leaf of a browsed UDT + /// (top-level, nested, BIT) committed as a raw tag writes through a runtime named by its member path, + /// encoded as its browsed type. + /// + [Fact] + public async Task Every_browsed_member_leaf_committed_as_a_raw_tag_writes_to_its_own_member_path() + { + var leaves = await DiscoverLeavesAsync(); + leaves.Select(l => l.FullName).ShouldBe( + ["Seg.ID", "Seg.Enable", "Seg.RampTime", "Seg.Sub.Code"], ignoreOrder: true); + + var factory = new FakeAbCipTagFactory(); + var drv = await StartAsync(factory, [.. leaves.Select(l => Committed(l.FullName, l.NativeDataType!))]); + var values = new Dictionary + { + ["Seg.ID"] = 42, ["Seg.Enable"] = true, ["Seg.RampTime"] = 1.5f, ["Seg.Sub.Code"] = (short)7, + }; + + var results = await drv.WriteAsync( + [.. values.Select(kv => new WriteRequest(kv.Key, kv.Value))], CancellationToken.None); + + results.Select(r => r.StatusCode).ShouldAllBe(s => s == AbCipStatusMapper.Good); + foreach (var (path, value) in values) + { + var tag = factory.Tags[path]; + tag.CreationParams.TagName.ShouldBe(path); // the member path itself — never its UDT, never a hidden host + tag.Value.ShouldBe(value); + tag.WriteCount.ShouldBe(1); + } + factory.Tags["Seg.Enable"].CreationParams.AuthoredType.ShouldBe(AbCipDataType.Bool); + factory.Tags["Seg.Sub.Code"].CreationParams.AuthoredType.ShouldBe(AbCipDataType.Int); + } + + /// The operator's writable:false on a committed member is honoured — nothing is created or sent. + [Fact] + public async Task A_member_authored_not_writable_is_BadNotWritable() + { + var factory = new FakeAbCipTagFactory(); + var drv = await StartAsync(factory, Committed("Seg.ID", "DInt", writable: false)); + + (await drv.WriteAsync([new WriteRequest("Seg.ID", 1)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable); + factory.Tags.ShouldNotContainKey("Seg.ID"); + } + + /// + /// An INT authored over a member the controller holds as 4 bytes: libplctag would patch the low two bytes + /// of its read buffer and send all four. Refused BadTypeMismatch before anything is sent, handle kept, + /// health untouched. + /// + [Fact] + public async Task A_member_authored_narrower_than_the_controller_value_is_refused_before_sending() + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Size = 4, Value = 0 } }; + // The mistyped tag cannot prime health itself — its READ is refused for the same width mismatch. + var drv = await StartAsync(factory, Committed("Seg.ID", "Int"), Committed("Seg.RampTime", "Real")); + await PrimeHealthAsync(drv, "Seg.RampTime"); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg.ID", (short)5)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + factory.Tags["Seg.ID"].WriteCount.ShouldBe(0); + factory.Tags["Seg.ID"].Disposed.ShouldBeFalse(); + drv.GetHealth().ShouldBe(health); + } + + /// + /// A BOOL authored over a 4-byte value (a packed BOOL-array DWORD, or a mis-typed DINT member): writing it + /// would rewrite all 32 bits. Refused BadNotSupported — the native runtime's refusal — before sending. + /// + [Fact] + public async Task A_BOOL_over_a_wider_value_is_refused_NotSupported_before_sending() + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Size = 4, Value = 0 } }; + var drv = await StartAsync(factory, Committed("Seg.Flags", "Bool")); + await PrimeHealthAsync(drv, "Seg.Flags"); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg.Flags", true)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + + factory.Tags["Seg.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().ShouldBe(health); + } + + /// A BIT member reads back as one byte (type 0xC1), so a BOOL write to it goes out. + [Fact] + public async Task A_BOOL_member_held_as_one_byte_writes() + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Size = 1, Value = false } }; + var drv = await StartAsync(factory, Committed("Seg.Enable", "Bool")); + + (await drv.WriteAsync([new WriteRequest("Seg.Enable", true)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + factory.Tags["Seg.Enable"].WriteCount.ShouldBe(1); + } + + /// Gitea #611 item 3 — a whole-UDT write is refused by decision: no runtime, nothing sent, no health change. + [Fact] + public async Task A_whole_UDT_write_is_refused_NotSupported_without_a_runtime_or_a_health_change() + { + var factory = new FakeAbCipTagFactory(); + var drv = await StartAsync(factory, Committed("Seg", "Structure"), Committed("Seg.ID", "DInt")); + await PrimeHealthAsync(drv, "Seg.ID"); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg", new byte[8])], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + + factory.Tags.ShouldNotContainKey("Seg"); + drv.GetHealth().ShouldBe(health); + } + + /// + /// A runtime's NotSupportedException is a local refusal the device never saw — BadNotSupported with the + /// handle kept and health untouched (it used to latch Degraded). + /// + [Fact] + public async Task A_runtime_NotSupported_refusal_leaves_health_alone_and_keeps_the_handle() + { + var factory = new FakeAbCipTagFactory { Customise = p => new RefusingEncodeTag(p) { Value = 0 } }; + var drv = await StartAsync(factory, Committed("Seg.ID", "DInt")); + await PrimeHealthAsync(drv, "Seg.ID"); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg.ID", 1)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + + drv.GetHealth().ShouldBe(health); + factory.Tags["Seg.ID"].Disposed.ShouldBeFalse(); + factory.Tags["Seg.ID"].WriteCount.ShouldBe(0); + } + + // ---- helpers --------------------------------------------------------------------------------------- + + /// Browses a UDT shaped like the Z-35162 press's UDT_TemperatureProfile_Segment (+ a nested struct). + private static async Task> DiscoverLeavesAsync() + { + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + EnableControllerBrowse = true, + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-browse", enumeratorFactory: new OneTagEnumeratorFactory( + new AbCipDiscoveredTag("Seg", null, AbCipDataType.Structure, ReadOnly: false))); + await drv.InitializeAsync("{}", CancellationToken.None); + drv.SeedDiscoveredUdtShapeForTest(Device, "Seg", new AbCipUdtShape("UDT_Segment", 24, + [ + new AbCipUdtMember("ID", 0, AbCipDataType.DInt, ArrayLength: 1), + new AbCipUdtMember("ZZZZZZZZZZUDT_Segm1", 4, AbCipDataType.SInt, ArrayLength: 1), // hidden BIT host + new AbCipUdtMember("Enable", 4, AbCipDataType.Bool, ArrayLength: 1), + new AbCipUdtMember("RampTime", 8, AbCipDataType.Real, ArrayLength: 1), + new AbCipUdtMember("Sub", 12, AbCipDataType.Structure, ArrayLength: 1), + ])); + drv.SeedDiscoveredUdtShapeForTest(Device, "Sub", new AbCipUdtShape("UDT_Sub", 4, + [new AbCipUdtMember("Code", 0, AbCipDataType.Int, ArrayLength: 1)])); + + var builder = new LeafRecorder(); + await drv.DiscoverAsync(builder, CancellationToken.None); + await drv.ShutdownAsync(CancellationToken.None); + return builder.Leaves; + } + + /// A raw tag as RawBrowseCommitMapper commits a discovered leaf (plus an optional writable key). + private static RawTagEntry Committed(string tagPath, string dataType, bool? writable = null) => + new(tagPath, + writable is { } w + ? $$"""{"tagPath":"{{tagPath}}","dataType":"{{dataType}}","writable":{{(w ? "true" : "false")}}}""" + : $$"""{"tagPath":"{{tagPath}}","dataType":"{{dataType}}"}""", + WriteIdempotent: false, + DeviceName: Device); + + private static async Task StartAsync(FakeAbCipTagFactory factory, params RawTagEntry[] tags) + { + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = tags, + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return drv; + } + + /// One good read so health holds a concrete Healthy snapshot the refusal must leave untouched. + private static async Task PrimeHealthAsync(AbCipDriver drv, string reference) => + (await drv.ReadAsync([reference], CancellationToken.None)).Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + + private sealed class RefusingEncodeTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override void EncodeValue(AbCipDataType type, int? bitIndex, object? value) => + throw new NotSupportedException("refused locally"); + } + + private sealed class OneTagEnumeratorFactory(params AbCipDiscoveredTag[] tags) : IAbCipTagEnumeratorFactory + { + public IAbCipTagEnumerator Create() => new Enumerator(tags); + + private sealed class Enumerator(AbCipDiscoveredTag[] tags) : IAbCipTagEnumerator + { + public async IAsyncEnumerable EnumerateAsync( + AbCipTagCreateParams deviceParams, [EnumeratorCancellation] CancellationToken cancellationToken) + { + await Task.CompletedTask; + foreach (var t in tags) yield return t; + } + + public void Dispose() { } + } + } + + private sealed class LeafRecorder : IAddressSpaceBuilder + { + public List Leaves { get; } = []; + + public IAddressSpaceBuilder Folder(string browseName, string displayName) => this; + + public IVariableHandle Variable(string browseName, string displayName, DriverAttributeInfo info) + { + Leaves.Add(info); + return new Handle(info.FullName); + } + + public void AddProperty(string browseName, DriverDataType dataType, object? value) { } + + private sealed class Handle(string fullRef) : IVariableHandle + { + public string FullReference => fullRef; + public IAlarmConditionSink MarkAsAlarmCondition(AlarmConditionInfo info) => new NullSink(); + } + + private sealed class NullSink : IAlarmConditionSink + { + public void OnTransition(AlarmEventArgs args) { } + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 27a14895..91a1a428 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -196,6 +196,31 @@ public sealed class NativeAbCipTagRuntimeTests ch.Sent[^1].ShouldBe(Convert.FromHexString("4D02" + "91015400" + "C400" + "0100" + "2A000000")); } + /// + /// Gitea #611 — a discovered UDT member writes to the MEMBER: one symbolic segment per member (plus an + /// element segment), the member's own CIP type, and only its own bytes. For a BIT member (Seg.Enable, + /// a BOOL packed into a hidden SINT host) that is what keeps the neighbour bits safe: the request names + /// the member and carries one BOOL byte, never the host, so the controller applies the bit. + /// + [Theory] + [InlineData("Seg.Enable", AbCipDataType.Bool, "C100", "00", true, + "4D07" + "9103536567009106456E61626C65" + "C100" + "0100" + "01")] + [InlineData("Seg.Sub.Code", AbCipDataType.Int, "C300", "0000", (short)-2, + "4D09" + "910353656700" + "910353756200" + "9104436F6465" + "C300" + "0100" + "FEFF")] + [InlineData("Seg.Points[2]", AbCipDataType.DInt, "C400", "00000000", 42, + "4D08" + "910353656700" + "9106506F696E7473" + "2802" + "C400" + "0100" + "2A000000")] + public async Task A_member_path_write_names_the_member_and_sends_only_its_own_bytes( + string tag, AbCipDataType type, string typeHex, string readHex, object value, string expectedRequestHex) + { + var ch = Replying(typeHex, readHex); + using var rt = await ReadAsync(ch, Params(tag, type)); + ch.Respond = req => Task.FromResult(FakeCipChannel.OkReply(req)); + rt.EncodeValue(type, null, value); + await rt.WriteAsync(CancellationToken.None); + rt.GetStatus().ShouldBe((int)AbStatus.Ok); + ch.Sent[^1].ShouldBe(Convert.FromHexString(expectedRequestHex)); + } + [Fact] public async Task A_STRING_write_sends_Struct_and_the_handle_learned_from_the_prior_read() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs index bdc5af13..bb70c77c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs @@ -985,4 +985,41 @@ public sealed class RawBrowseCommitMapperTests refused.ShouldBe(expectRefused); blobRejected.ShouldBe(expectRefused); } + + // ---- AB CIP discovered UDT members (Gitea #611) ---------------------------------------------- + + /// + /// Gitea #611 — a controller-discovered UDT member leaf commits as an ordinary raw tag: its member path + /// is the tagPath, the template's atomic type is the dataType, and it lands READ-ONLY with + /// no writable key. That is the whole writability rule for discovered members: the driver writes a + /// member only once the operator authors the raw tag ReadWrite (the node-manager gate), and the + /// commit never pre-authors that — the browse's SecurityClass is not consulted. + /// + /// The discovered member's full name (its Logix member path). + /// The member's browse name. + /// The atomic type the template stated. + [Theory] + [InlineData("_EmptyTemperatureSegment.RampTime", "RampTime", "Real")] + [InlineData("_EmptyTemperatureSegment.Enable", "Enable", "Bool")] + [InlineData("Program:Main.Motor1.Status.Code", "Code", "Int")] + public void A_discovered_AbCip_UDT_member_commits_read_only_with_its_member_path_and_type( + string memberPath, string browseName, string nativeType) + { + var row = RawBrowseCommitMapper.MapLeaf( + driverType: DriverTypeNames.AbCip, + fullName: memberPath, + browseName: browseName, + driverDataType: null, + defaultDataType: "Int32", + groupPrefix: null, + folderPath: null, + createGroups: false, + addressFields: NativeTypeFields(nativeType)); + + row.Tag.AccessLevel.ShouldBe(TagAccessLevel.Read); + var o = JsonNode.Parse(row.Tag.TagConfig)!.AsObject(); + o["tagPath"]!.GetValue().ShouldBe(memberPath); + o["dataType"]!.GetValue().ShouldBe(nativeType); + o.ContainsKey("writable").ShouldBeFalse(); + } } From f85fc8fbeb35c591ec9e63dcc2c1c5ad7beeb856 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 9 Oct 2026 04:31:45 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(abcip):=20#611=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20native=20keeps=20BOOL=20type=20distinction,=20Write?= =?UTF-8?q?WidthRefusal=20tests,=20doc=20caveats?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - L2: WriteWidthRefusal leaves a BOOL over a wider value to a runtime that sees the CIP type (native): mistyped integer member -> BadTypeMismatch (as its read), 0xD3 BOOL-array DWORD -> BadNotSupported; libplctag keeps BadNotSupported for both (size only). - L4: direct theory over WriteWidthRefusal (1/2/4/8 widths incl LINT/ULINT/LREAL, String/Dt, unknown size) + array/bit-selector exemptions with a non-vacuous control. - L1: CLAUDE.md member/BIT-member writes are ab_server-proven only. - L3: patch 0003 blind spots documented (Docker README, AbServer-Test-Fixture.md); nits. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 2 +- docs/drivers/AbCip.md | 9 +- docs/drivers/AbServer-Test-Fixture.md | 4 +- .../AbCipDriver.cs | 13 +- .../Docker/README.md | 10 +- .../Docker/docker-compose.yml | 2 +- .../AbCipDiscoveredMemberWriteTests.cs | 115 ++++++++++++++++++ 7 files changed, 146 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9d1513cf..c8f1717e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1102,7 +1102,7 @@ once per tag — only genuine transport failures evict and degrade (a runtime `N it used to latch `Degraded`). AbCip width-checks every scalar **write** against the controller's bytes before sending, on both transports (#611; libplctag used to patch part of its buffer and send it all). Controller-discovered UDT members are writable once browse-committed and authored `ReadWrite` (BIT members included — the controller -applies the bit); whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the +applies the bit) — ab_server-proven only, member and BIT-member writes are unverified on real hardware; whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT factories' `Register(registry, ILoggerFactory?)` take one too, #602 CONV-3). The AbCip `browsePageDelayMs` knob has an AdminUI field (`AbCipDriverForm` "Browse page delay (ms)"). libplctag is confined to `Libplctag*.cs` adapters by `LibplctagConfinementGuardTests`; do not reference it elsewhere. See `docs/drivers/AbCip.md` §Transport, `docs/drivers/AbLegacy.md` §Transport + §Wire evidence, `docs/Configuration.md` §`Drivers:AbCip`, and diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 98cb235d..bb81a9f1 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -405,8 +405,10 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic with the bytes the controller holds at the path (the runtime's last read) and refuses a mismatch without writing: a narrower/wider atomic type is `BadTypeMismatch` (libplctag would otherwise patch part of its read buffer and send the whole buffer back — e.g. an `Int` over a DINT member wrote four bytes with a stale high - word); a `Bool` over a wider value (a packed BOOL-array DWORD, a mistyped integer) is `BadNotSupported`, - because writing it would rewrite every neighbouring bit. Native additionally refuses a same-width + word); a `Bool` over a wider value is never written, because writing it would rewrite every neighbouring + bit. On **native** the runtime judges it from the CIP type, exactly as its read does: a packed BOOL-array + DWORD (`0xD3`) is `BadNotSupported`, a BOOL authored over a mistyped integer member is `BadTypeMismatch`. + **libplctag** exposes only the size, cannot tell the two apart, and refuses both `BadNotSupported`. Native additionally refuses a same-width mismatch (REAL vs DINT) from the CIP type code — see [Native transport behaviour](#native-transport-behaviour). A refusal keeps the handle, warns once per tag and **does not touch driver health**. - **Not verified on a real controller.** Production presses are read-only, so member writes are proven on @@ -414,7 +416,8 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic Z-35162 controller tag `_EmptyTemperatureSegment` (`UDT_TemperatureProfile_Segment`), which no logic references — its members (`ID`, `Enable`, `RampTime`, …) are all zero and resolve as described above. -Pre-declared `Members` with `Writable: true` keep working exactly as before (same write path). +Pre-declared `Members` with `Writable: true` keep working as before — same write path, now width-checked +(a mistyped declared member is refused where libplctag used to write it). ### Whole-UDT writes — refused, by decision diff --git a/docs/drivers/AbServer-Test-Fixture.md b/docs/drivers/AbServer-Test-Fixture.md index e198c6c2..a56eba20 100644 --- a/docs/drivers/AbServer-Test-Fixture.md +++ b/docs/drivers/AbServer-Test-Fixture.md @@ -55,7 +55,9 @@ smoke test passes on both transports. Evidence: - ab_server serves no UDTs, so template parity is vacuous there. Docker patch 0003 serves a UDT **member path** from a flat tag named by that dotted path (`UdtSeg.ID`, `UdtSeg.Sub.Code`, `UdtSeg.Points[2]`) — the wire of a member read/write, not a structure — and accepts the pad byte libplctag adds to a 1-byte write, - which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Native writes remain + which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Two blind spots follow (see + the Docker README §0003): a +1-byte over-write of an odd-sized tag is no longer caught, and a malformed + single segment `"UdtSeg.ID"` resolves where a real Logix would refuse it. Native writes remain simulator-only. - **Golden-frame capture:** `scripts/ab-capture/capture.sh` (capture client in `scripts/ab-capture/client`); fixtures live in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/` with a README. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 64a22ca0..25bf13ba 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1197,7 +1197,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // BOOL-array DWORD wrote all 32 bits. Discovered UDT members are where a mis-authored type // is most likely (browse-commit fills dataType from the template, an operator may retype). size = runtime.GetSize(); - refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size); + refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size, runtimeTypesBool: runtime is NativeAbCipTagRuntime); if (refusal is null) { runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); @@ -1710,15 +1710,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// would clobber the neighbouring bits, the same refusal the native runtime raises; any other width /// mismatch is BadTypeMismatch. Arrays, bit selectors (Flags.3, served by the /// read-modify-write path) and an unknown size (<= 0) are not judged here. + /// + /// A BOOL over a wider value is left to the runtime when it can see the CIP type + /// ( — the native transport): it tells a packed BOOL-array DWORD + /// (0xD3, BadNotSupported) from a mistyped integer member (BadTypeMismatch, the + /// same verdict as its read), and refuses both before sending. libplctag exposes only the size, so the + /// two are indistinguishable there and both get the safe BadNotSupported. + /// /// /// The tag being written. /// The parsed bit selector, if any. /// The runtime's read-buffer size in bytes. + /// The runtime checks a BOOL write against the controller's CIP type itself. /// The refusal status, or null. - internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size) + internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size, bool runtimeTypesBool = false) { if (IsArrayTag(def) || bitIndex is not null || size <= 0) return null; if (ExpectedScalarWriteWidth(def.DataType) is not { } width || size == width) return null; + if (def.DataType == AbCipDataType.Bool && runtimeTypesBool) return null; return def.DataType == AbCipDataType.Bool ? AbCipStatusMapper.BadNotSupported : AbCipStatusMapper.BadTypeMismatch; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md index c0ebb5f0..189892c5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md @@ -90,6 +90,14 @@ things for the discovered-UDT-member write suite (`AbCipUdtMemberWriteTests`, Gi `0x03` (libplctag `ErrorBadParam`), so no libplctag BOOL/SINT write could pass here. The patch drops exactly one trailing pad byte when it alone would overrun an odd-sized target. +**Blind spots this patch introduces (accepted for a fixture):** (a) the pad trim also accepts a genuinely +wrong 2-byte payload aimed at a 1-byte tag, so ab_server can no longer catch a client over-writing an +odd-sized target by exactly one byte — the driver's own pre-write width check (`AbCipDriver.WriteWidthRefusal`) +covers our paths; (b) because `.` is legal in a `--tag` name, a malformed *single* symbolic segment +`"UdtSeg.ID"` (which a real Logix rejects) also resolves, via the original exact-match lookup — +`NativeAbCipTagRuntimeTests.A_member_path_write_names_the_member_and_sends_only_its_own_bytes` pins that our +encoder splits members into separate segments. + **Bumping `LIBPLCTAG_TAG` means re-checking every patch against the new tag.** It is a concrete tag rather than the `release` branch precisely so that this stays a deliberate act — the branch moves, and the Dockerfile previously called it @@ -159,7 +167,7 @@ place means updating both. | Family | Seeded tags | Notes | |---|---|---| -| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | +| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. `Pump_1_Speed`, `Line1_Count` and `Motor_Run` serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | | CompactLogix | `TestDINT` `TestREAL` `TestBOOL` | Narrow ConnectionSize cap enforced driver-side; ab_server accepts any size | | Micro800 | `TestDINT` `TestREAL` | ab_server has no `micro800` mode; falls back to `controllogix` emulation | | GuardLogix | `TestDINT` `SafetyDINT_S` | ab_server has no safety subsystem; `_S` suffix triggers driver-side classification only | diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml index 8e6e45ae..ce9dfaa0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml @@ -20,7 +20,7 @@ services: restart: "no" ports: - "44818:44818" - # The last three are here for the controller-browse gate (Gitea #468) and + # Pump_1_Speed, Line1_Count and Motor_Run are here for the controller-browse gate (Gitea #468) and # are additive — the read/write suites key off the Test* tags above. # The UdtSeg.* tags (Gitea #611) stand in for the members of a UDT tag `UdtSeg` # (modelled on the Z-35162 press's UDT_TemperatureProfile_Segment): ab_server has diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs index 2fd0df3b..74fdaae2 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs @@ -2,6 +2,7 @@ using System.Runtime.CompilerServices; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; @@ -153,6 +154,120 @@ public sealed class AbCipDiscoveredMemberWriteTests factory.Tags["Seg.ID"].WriteCount.ShouldBe(0); } + /// + /// Review L4 — directly, with a real size so the check runs: + /// every atomic width (1/2/4/8, incl. LINT/ULINT/LREAL) passes at its own size and is refused at another; + /// arrays, Tag.N bit selectors, STRING/DT and an unknown size (<= 0) are never judged. + /// + [Theory] + // matching widths write + [InlineData(AbCipDataType.Bool, 1, null)] + [InlineData(AbCipDataType.SInt, 1, null)] + [InlineData(AbCipDataType.USInt, 1, null)] + [InlineData(AbCipDataType.Int, 2, null)] + [InlineData(AbCipDataType.UInt, 2, null)] + [InlineData(AbCipDataType.DInt, 4, null)] + [InlineData(AbCipDataType.UDInt, 4, null)] + [InlineData(AbCipDataType.Real, 4, null)] + [InlineData(AbCipDataType.LInt, 8, null)] + [InlineData(AbCipDataType.ULInt, 8, null)] + [InlineData(AbCipDataType.LReal, 8, null)] + // mismatches are refused + [InlineData(AbCipDataType.LInt, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.LReal, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.ULInt, 2, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Real, 8, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Int, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.SInt, 2, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Bool, 4, AbCipStatusMapper.BadNotSupported)] + // types with no fixed encoded width are never judged + [InlineData(AbCipDataType.String, 88, null)] + [InlineData(AbCipDataType.String, 4, null)] + [InlineData(AbCipDataType.Dt, 8, null)] + // unknown size is never judged + [InlineData(AbCipDataType.DInt, 0, null)] + [InlineData(AbCipDataType.DInt, -1, null)] + public void WriteWidthRefusal_judges_scalar_widths_only(AbCipDataType type, int size, uint? expected) + { + var def = new AbCipTagDefinition("T", Device, "Seg.X", type); + AbCipDriver.WriteWidthRefusal(def, bitIndex: null, size).ShouldBe(expected); + } + + /// Review L4 — array tags and bit selectors are exempt even at a size that would otherwise refuse. + [Fact] + public void WriteWidthRefusal_exempts_arrays_and_bit_selectors() + { + // Control: the same scalar at the same size IS refused, so the exemptions below are not vacuous. + AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4) + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 4, IsArray: true), null, 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 2), null, 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Flags.3", AbCipDataType.Bool), bitIndex: 3, size: 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Word.3", AbCipDataType.Int), bitIndex: 3, size: 4) + .ShouldBeNull(); + } + + /// + /// Review L2 — a runtime that types BOOL writes itself (native) is left to judge a BOOL over a wider value; + /// only libplctag, which sees a size and nothing else, gets the pre-check's BadNotSupported. + /// + [Fact] + public void WriteWidthRefusal_leaves_a_wide_BOOL_to_a_runtime_that_sees_the_CIP_type() + { + var def = new AbCipTagDefinition("T", Device, "Seg.Flag", AbCipDataType.Bool); + AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: false).ShouldBe(AbCipStatusMapper.BadNotSupported); + AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: true).ShouldBeNull(); + // Non-BOOL mismatches are still refused up front on every runtime. + AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4, + runtimeTypesBool: true).ShouldBe(AbCipStatusMapper.BadTypeMismatch); + } + + /// + /// Review L2, end to end on the native runtime: a BOOL authored over a mistyped DINT member writes + /// BadTypeMismatch (the same verdict as its read); a BOOL over a packed BOOL-array DWORD writes + /// BadNotSupported. Neither sends a write; health is untouched. + /// + [Theory] + [InlineData("C400", AbCipStatusMapper.BadTypeMismatch)] // DINT member mistyped as Bool + [InlineData("D300", AbCipStatusMapper.BadNotSupported)] // packed BOOL-array DWORD + public async Task A_native_BOOL_over_a_wider_value_keeps_its_type_distinction(string typeHex, uint expected) + { + var channels = new List(); + var factory = new NativeFactory(p => + { + var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D2000000" + typeHex + "00000000")) }; + lock (channels) channels.Add(ch); + return NativeAbCipTagRuntime.ForTest(ch, p); + }); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = [Committed("Seg.Flag", "Bool")], + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-native", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg.Flag", true)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(expected); + + channels.ShouldHaveSingleItem().Sent.ShouldAllBe(req => req[0] != 0x4D); // no Write Tag request + drv.GetHealth().ShouldBe(health); + } + + private sealed class NativeFactory(Func create) : IAbCipTagFactory + { + public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); + } + // ---- helpers --------------------------------------------------------------------------------------- /// Browses a UDT shaped like the Z-35162 press's UDT_TemperatureProfile_Segment (+ a nested struct).