From 6b3da93151b414dacdfefe3d3c10d4f90df4bd43 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 15:20:07 -0400 Subject: [PATCH] fix(browse): commit the browsed data type, and fix the probe classification the fixture was hiding (#536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A browse-committed AB CIP / TwinCAT / FOCAS tag bound with its model's DEFAULT data type, not the one the device reported. A browsed REAL committed as `{"tagPath":"Pump_1_Speed","dataType":"DInt"}` — and those three drivers decode the wire bytes by that field, not by the raw row's OPC UA DataType — so the tag read four bytes of a float through an integer decoder and published a plausible wrong number with no error raised anywhere. FOCAS was missed when #536 was filed. The type is CARRIED, not derived. Reverse-mapping the leaf's driver-agnostic DriverDataType looks like the smaller fix and is wrong: SInt/Int/DInt/USInt/UInt all collapse into Int32 on the way out of the driver, so that version would bind a 16-bit INT as a 32-bit DINT while appearing to fix the REAL. Drivers now state their own type name on DriverAttributeInfo.NativeDataType, which reaches the picker as AttributeInfo.AddressFields["dataType"] and the commit mapper from there. A leaf that states nothing keeps the model default — the pre-#536 behaviour, kept deliberately as the fallback rather than a guess. Numeric strings are refused before parsing: Enum.TryParse accepts "9" for any enum and Enum.IsDefined then confirms it, so an unguarded parse would bind a tag to a type nobody named. A test caught this in the first version of the guard. --- and the other half, which the fixture was hiding --- Test Connect on a device with NO authored tags — every AB CIP device at the moment an operator first presses the button — reported "CIP handshake failed" against a healthy controller. That was written down as a fixture limitation ("ab_server doesn't implement @raw_cpu_type"). Both halves of that were wrong: * @raw_cpu_type is not a libplctag special tag at all (only @raw, @tags and @udt/ are), so it goes out as an ordinary symbolic name that no controller defines, and the controller's "tag does not exist" IS the successful answer. * ab_server answered an unresolvable tag path with CIP 0x03 where a Logix controller answers 0x04 — ErrorBadParam vs ErrorNotFound, which the probe classifies oppositely. Fixed in vendored patch 0002. * With the fixture then telling the truth, the real defect surfaced: AbCipDriverProbe.IsReachableException matched the native C status spellings ("STATUS_NOT_FOUND"), but libplctag.NET throws with the MANAGED enum name ("ErrorNotFound"). Those substrings can never match, so the reachable classification could not fire on the exception path — which is the path every no-authored-tags probe takes. This was failing on real hardware too, not just against the sim. It now recovers the Status and defers to IsReachableStatus, so there is one classification rather than two that can drift. Rejected: switching the fallback to @tags. libplctag builds a tag-listing tag with NO network I/O, so it reports OK against any TCP listener — measured green against a Mosquitto broker. Verification: full solution suite green. Falsification runs confirm exactly the intended tests go red — 9 for the type carry, 3 (the reachable statuses only) for the probe classification. Live on docker-dev: browse-committed TestString/ Motor_Run/TestSINT now store String/Bool/SInt, alongside last session's Pump_1_Speed still holding the wrong DInt. TestSINT is the falsifier — its row DataType is Int32, identical to TestDINT's, so no reverse map could have produced "SInt". Known consequence: pre-existing browse-committed tags keep their wrong dataType; the fix applies to new commits only. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- docs/Driver.AbCip.Cli.md | 7 +- ...versal-discovery-browser-implementation.md | 24 +++- .../Browsing/CapturedTreeBrowseSession.cs | 25 +++- .../DriverAttributeInfo.cs | 18 ++- .../Commands/ProbeCommand.cs | 6 +- .../AbCipDriverOptions.cs | 7 +- .../AbCipDriver.cs | 12 +- .../AbCipDriverProbe.cs | 40 ++++-- .../FocasDriver.cs | 31 ++++- .../TwinCATDriver.cs | 6 +- .../Uns/RawBrowseCommitMapper.cs | 75 ++++++++++- .../CapturedTreeBrowseSessionTests.cs | 37 ++++++ .../Docker/README.md | 32 ++++- ...r-answer-tag-not-found-with-cip-0x04.patch | 30 +++++ .../AbCipDataTypeExtensionsTests.cs | 44 +++++++ .../AbCipDriverDiscoveryTests.cs | 30 +++++ .../AbCipDriverProbeTests.cs | 42 ++++++ .../FocasCapabilityTests.cs | 7 + .../TwinCATCapabilityTests.cs | 6 + .../Uns/RawBrowseCommitMapperTests.cs | 121 ++++++++++++++++++ .../DriverProbeHandshakeE2eTests.cs | 36 +++++- 21 files changed, 585 insertions(+), 51 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDataTypeExtensionsTests.cs diff --git a/docs/Driver.AbCip.Cli.md b/docs/Driver.AbCip.Cli.md index d2a05819..3a82de36 100644 --- a/docs/Driver.AbCip.Cli.md +++ b/docs/Driver.AbCip.Cli.md @@ -33,7 +33,12 @@ Family ↔ CIP-path cheat sheet: ### `probe` — is the PLC up? ```powershell -# ControlLogix — read the canonical libplctag system tag +# ControlLogix — read a real tag +otopcua-abcip-cli probe -g ab://10.0.0.5/1,0 -t TestDINT --type DInt + +# …or any name at all: `@raw_cpu_type` is NOT a libplctag special tag (only `@raw`, `@tags` +# and `@udt/` are), so it goes out as an ordinary symbolic name that no controller defines. +# The "tag does not exist" answer is itself proof the CIP session came up. otopcua-abcip-cli probe -g ab://10.0.0.5/1,0 -t @raw_cpu_type --type DInt # Micro800 — point at a user-supplied global diff --git a/docs/plans/2026-07-15-universal-discovery-browser-implementation.md b/docs/plans/2026-07-15-universal-discovery-browser-implementation.md index a0fd089a..470e93bb 100644 --- a/docs/plans/2026-07-15-universal-discovery-browser-implementation.md +++ b/docs/plans/2026-07-15-universal-discovery-browser-implementation.md @@ -1132,12 +1132,24 @@ neither agreed with a controller. Fixed, with the layout now pinned by a capture ### Follow-ups discovered during the second gate -1. **Browse-commit discards the discovered data type** — a browsed REAL commits with - `"dataType":"DInt"` in its `TagConfig`, and the driver decodes off that field rather than the - row's `DataType`. Filed as **Gitea #536**; affects AbCip and (by inspection) TwinCAT. -2. **Test Connect fails against this fixture regardless of the device** — the probe falls back to - `@raw_cpu_type` when no tags are authored, which `ab_server` does not implement. Fixture - limitation, documented in the fixture README; browse is unaffected. +1. ~~**Browse-commit discards the discovered data type**~~ — **CLOSED 2026-08-01 (Gitea #536).** A + browsed REAL committed with `"dataType":"DInt"`, and AbCip/TwinCAT/**FOCAS** decode off that field + rather than the raw row's `DataType`, so the tag published a plausible wrong number with no error + raised. FOCAS was missed when the issue was filed. Fixed by carrying the driver's own type name on + `DriverAttributeInfo.NativeDataType` → `AttributeInfo.AddressFields["dataType"]` → + `RawBrowseCommitMapper`. **It could not be fixed by reverse-mapping the leaf's driver-agnostic + `DriverDataType`**: `SInt`/`Int`/`DInt`/`USInt`/`UInt` all collapse into `Int32`, so that "fix" + would still bind a 16-bit INT as a 32-bit DINT while looking correct on a REAL. +2. ~~**Test Connect fails against this fixture regardless of the device**~~ — **CLOSED 2026-08-01, and + the stated cause was wrong.** `@raw_cpu_type` is not a libplctag special tag at all (only `@raw`, + `@tags`, `@udt/` are), so it is sent as an ordinary symbolic name and the controller's "tag does not + exist" IS the successful answer. Two real defects hid behind the "fixture limitation" label: the sim + answered CIP **0x03** where a controller answers **0x04** (fixed in vendored patch 0002), and + `AbCipDriverProbe.IsReachableException` matched the native C status spellings (`STATUS_NOT_FOUND`) + that libplctag.NET never emits — it throws with the managed enum name — so the reachable + classification could not fire on the exception path, which is the path every no-authored-tags probe + takes. **Test Connect was therefore failing on real hardware too, not just against the sim.** + Pinned by `AbCip_WithNoAuthoredTags_ReportsTheControllerReachable` plus offline classification tests. ### Follow-ups discovered during the first gate diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/CapturedTreeBrowseSession.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/CapturedTreeBrowseSession.cs index 47ac6fdd..0ace043a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/CapturedTreeBrowseSession.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/CapturedTreeBrowseSession.cs @@ -50,7 +50,7 @@ public sealed class CapturedTreeBrowseSession(CapturedTree tree) : IBrowseSessio var attrs = new List(); if (n.Attribute is { } a) attrs.Add(new AttributeInfo(n.Name, a.DriverDataType.ToString(), a.IsArray, - a.SecurityClass.ToString(), a.IsAlarm || n.IsAlarmMarked)); + a.SecurityClass.ToString(), a.IsAlarm || n.IsAlarmMarked, NativeTypeFields(a))); attrs.AddRange(n.Properties.Select(p => new AttributeInfo(p.Name, p.Type.ToString(), false, "", false))); return Task.FromResult>(attrs); @@ -65,6 +65,29 @@ public sealed class CapturedTreeBrowseSession(CapturedTree tree) : IBrowseSessio private CapturedTree Tree() => _tree ?? throw new ObjectDisposedException(nameof(CapturedTreeBrowseSession)); + /// + /// Surfaces a leaf's driver-native type name as the dataType address field, so a + /// browse-commit can stamp the driver-typed TagConfig with the type the controller + /// actually reported (Gitea #536). + /// + /// The captured attribute. + /// The address fields, or null when the driver reported no native type. + /// + /// It rides rather than + /// because that field is the driver-AGNOSTIC + /// type and several consumers already read it as such. The commit mapper cannot recover the + /// native type from it: SInt/Int/DInt all arrive as Int32, so a + /// reverse map would bind a 16-bit INT as a 32-bit DINT and read four bytes for two. + /// + /// Null when the driver stamped nothing — the mapper then leaves the model's own default in + /// place, which is exactly the pre-#536 behaviour rather than a guess. + /// + /// + private static IReadOnlyDictionary? NativeTypeFields(DriverAttributeInfo a) + => string.IsNullOrWhiteSpace(a.NativeDataType) + ? null + : new Dictionary(StringComparer.Ordinal) { ["dataType"] = a.NativeDataType }; + private static BrowseNode Map(CapturedNode n) => n.IsLeaf ? new BrowseNode(n.Id, n.DisplayName, BrowseNodeKind.Leaf, false) : new BrowseNode(n.Id, n.DisplayName, BrowseNodeKind.Folder, n.Children.Count > 0); diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverAttributeInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverAttributeInfo.cs index c3b2085f..350a9037 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverAttributeInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverAttributeInfo.cs @@ -45,6 +45,21 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; /// Set when is — /// stable logical id the ScriptedAlarmEngine addresses by. Null otherwise. /// +/// +/// The driver's OWN type name for this attribute (e.g. "Real" for an +/// AbCipDataType, "Int16" for a TwinCATDataType) when the driver binds +/// by a driver-specific type enum in its TagConfig; null for a driver whose tag config +/// carries no type (OPC UA Client, Galaxy) or which takes the type from the wire (MQTT +/// Sparkplug births, MTConnect probe inference). +/// +/// It exists because is lossy in exactly the direction a +/// browse-commit needs (Gitea #536): AbCipDataType.SInt, .Int and .DInt +/// all map to , so a commit that reverse-mapped the +/// driver-agnostic type would bind a 16-bit INT as a 32-bit DINT. Nothing downstream of the +/// address-space build reads this — it is authoring metadata carried alongside the type the +/// node manager actually uses. +/// +/// public sealed record DriverAttributeInfo( string FullName, DriverDataType DriverDataType, @@ -56,7 +71,8 @@ public sealed record DriverAttributeInfo( bool WriteIdempotent = false, NodeSourceKind Source = NodeSourceKind.Driver, string? VirtualTagId = null, - string? ScriptedAlarmId = null); + string? ScriptedAlarmId = null, + string? NativeDataType = null); /// /// Discriminates which runtime subsystem owns this node's Read/Write/ diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index a0cf284f..804ff1bc 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -14,8 +14,10 @@ public sealed class ProbeCommand : AbCipCommandBase { /// Gets the tag path to probe. [CommandOption("tag", 't', Description = - "Tag path to probe. ControlLogix default is '@raw_cpu_type' (the canonical libplctag " + - "system tag); Micro800 takes a user-supplied global (e.g. '_SYSVA_CLOCK_HOUR').", + "Tag path to probe — a real tag on the controller. Any name works to prove reachability: " + + "libplctag knows only @raw, @tags and @udt/ as special names, so anything else (including " + + "'@raw_cpu_type', which no controller defines) is sent as an ordinary symbolic name and the " + + "controller's 'tag does not exist' answer already proves the CIP session is live.", IsRequired = true)] public string TagPath { get; init; } = default!; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs index 7c949862..24c9a74b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs @@ -213,8 +213,11 @@ public sealed class AbCipProbeOptions /// Tag path used for the probe. When is true but this is /// null/blank, the driver logs a warning and runs no probe loops; /// GetHostStatuses() will then report every device as Unknown. A family-default - /// system-tag fallback (e.g. @raw_cpu_type on ControlLogix) is a deferred follow-up; - /// today an operator opting into the probe must supply a tag path explicitly. + /// fallback is a deferred follow-up; today an operator opting into the probe must supply a tag + /// path explicitly. (This used to name @raw_cpu_type as the ControlLogix system tag such + /// a default would read. It is not one — libplctag's CIP special-tag dispatch knows only + /// @raw, @tags and @udt/ — so any default here would be a name the + /// controller rejects, which proves reachability but never returns a value.) /// public string? ProbeTagPath { get; init; } } 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 703579d6..2cd918ae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1068,7 +1068,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, : SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: member.WriteIdempotent)); + WriteIdempotent: member.WriteIdempotent, + NativeDataType: member.DataType.ToString())); } continue; } @@ -1145,7 +1146,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, : SecurityClassification.Operate, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: discovered.DataType.ToString())); } } } @@ -1248,7 +1250,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, : SecurityClassification.Operate, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: member.DataType.ToString())); anyEmitted = true; } @@ -1277,7 +1280,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, : SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: tag.WriteIdempotent); + WriteIdempotent: tag.WriteIdempotent, + NativeDataType: tag.DataType.ToString()); /// Count of registered devices — exposed for diagnostics + tests. internal int DeviceCount => _devices.Count; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs index 58e8a0ec..9ad6abae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs @@ -91,9 +91,16 @@ public sealed class AbCipDriverProbe : IDriverProbe // Prefer the first authored tag path as the probe tag name (it must exist on the PLC). v3: the // tags arrive as RawTagEntry blobs — map each through the pure factory to recover its TagPath + - // device routing key. Fall back to a benign system attribute present on all ControlLogix / - // CompactLogix controllers (@raw_cpu_type); still returns ErrorNotFound on Micro800 / MicroLogix, - // which is sufficient to confirm CIP reachability. + // device routing key. + // + // The fallback is NOT a system attribute the controller can read, despite its name and despite + // what this comment used to claim: libplctag's CIP special-tag dispatch knows only @raw, @tags + // and @udt/, so "@raw_cpu_type" is encoded and sent as an ORDINARY symbolic name. That is what + // makes it a usable probe — no controller defines it, so the controller answers CIP 0x04 ("tag + // does not exist") and the answer itself proves the session is live. IsReachableStatus is what + // turns that into a green tick, so a device with no authored tags reports reachable, not failed. + // Do not "fix" this to @tags: libplctag builds a tag-listing tag WITHOUT any network I/O, so it + // reports OK against any TCP listener — measured green against a Mosquitto broker. var mappedTags = opts.RawTags .Select(e => AbCipTagDefinitionFactory.FromTagConfig(e.TagConfig, e.RawPath, out var d) ? (Ok: true, Device: e.DeviceName, d.TagPath) @@ -167,22 +174,35 @@ public sealed class AbCipDriverProbe : IDriverProbe /// but the probe tag name was not found or did not match — sufficient to confirm CIP /// reachability. Returns true for (tag was found). /// - private static bool IsReachableStatus(Status s) => s is + internal static bool IsReachableStatus(Status s) => s is Status.Ok or Status.ErrorNotFound or Status.ErrorNoMatch or Status.ErrorBadDevice; /// - /// Inspects a message to decide whether the error is a - /// "controller answered CIP but tag not found" condition (reachable) versus a session / - /// transport failure (unreachable). Falls back to checking the exception message for - /// known tag-not-found phrases used by the libplctag native library. + /// Decides whether a is a "controller answered CIP but the + /// probe tag did not resolve" condition (reachable) or a session / transport failure + /// (unreachable), by recovering the the wrapper threw on and deferring + /// to — one classification, not two that can drift. /// - private static bool IsReachableException(LibPlcTagException ex) + /// The exception libplctag threw out of InitializeAsync. + /// true when the controller answered; false for a transport/session failure. + /// + /// The status arrives as the managed enum NAME. NativeTagWrapper.ThrowIfStatusNotOk + /// constructs the exception with Status.ToString(), so the message is exactly + /// "ErrorNotFound" — never the native C spelling "STATUS_NOT_FOUND" this method + /// used to look for. Those substrings could not match, so the reachable classification was + /// unreachable on this path — and this IS the path a probe takes whenever its tag does not + /// resolve, which is every Test Connect against a device with no authored tags. The native + /// spellings are kept as a secondary match in case a future wrapper reports them. + /// + internal static bool IsReachableException(LibPlcTagException ex) { - // libplctag wraps the Status in the message as "STATUS_NOT_FOUND", "STATUS_NO_MATCH", etc. var msg = ex.Message ?? string.Empty; + if (Enum.TryParse(msg.Trim(), ignoreCase: true, out var status) && Enum.IsDefined(status)) + return IsReachableStatus(status); + return msg.Contains("NOT_FOUND", StringComparison.OrdinalIgnoreCase) || msg.Contains("NO_MATCH", StringComparison.OrdinalIgnoreCase) || msg.Contains("BAD_DEVICE", StringComparison.OrdinalIgnoreCase); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index ebd68e38..aea048ff 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -613,7 +613,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: tag.WriteIdempotent)); + WriteIdempotent: tag.WriteIdempotent, + NativeDataType: tag.DataType.ToString())); } } return Task.CompletedTask; @@ -638,7 +639,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: NativeName(type))); } private static void EmitAxisVariable( @@ -653,9 +655,24 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: nameof(FocasDataType.Float64))); } + /// + /// The name a fixed-tree node binds by, for the browse-commit + /// path (Gitea #536). The fixed tree only ever emits these three, and each has an + /// identically-named member — the driver-agnostic + /// cannot be reversed (Byte/Int16/Int32 all collapse to + /// ), so the native name is carried rather than derived. + /// + private static string NativeName(FocasDriverDataType type) => type switch + { + FocasDriverDataType.Int32 => nameof(FocasDataType.Int32), + FocasDriverDataType.Float64 => nameof(FocasDataType.Float64), + _ => nameof(FocasDataType.String), + }; + /// /// Emit a variable under a named fixed-tree folder (Program, OperationMode, /// …). Full-reference shape is {deviceHost}/{folderPath}/{field}. @@ -673,7 +690,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: NativeName(type switch + { + DriverDataType.Int32 => FocasDriverDataType.Int32, + DriverDataType.Float64 => FocasDriverDataType.Float64, + _ => FocasDriverDataType.String, + }))); } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index e041066f..a2aaaf34 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -421,7 +421,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery : SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: tag.WriteIdempotent)); + WriteIdempotent: tag.WriteIdempotent, + NativeDataType: tag.DataType.ToString())); } // Controller-side symbol browse — opt-in. Falls back to pre-declared-only on any @@ -452,7 +453,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery : SecurityClassification.Operate, IsHistorized: false, IsAlarm: false, - WriteIdempotent: false)); + WriteIdempotent: false, + NativeDataType: dt.ToString())); } } catch (OperationCanceledException) { throw; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs index a76d6295..1a912093 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs @@ -3,7 +3,10 @@ using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; using ZB.MOM.WW.OtOpcUa.Commons.Types; using ZB.MOM.WW.OtOpcUa.Configuration.Enums; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; using ZB.MOM.WW.OtOpcUa.Driver.Mqtt; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns; @@ -192,11 +195,19 @@ public static class RawBrowseCommitMapper } /// - /// Builds the driver-typed TagConfig JSON for a browse-committed tag, setting ONLY the driver's - /// address field to and leaving every other field at its model default. Reuses - /// the <Driver>TagConfigModel for driver types that have a typed editor; the model-less Galaxy + /// Builds the driver-typed TagConfig JSON for a browse-committed tag: the driver's address field + /// from , plus — for a driver that binds by its OWN data-type enum — the + /// native type the browse leaf stated (see ). Every other field stays at + /// its model default, for the operator to author afterwards. Reuses the + /// <Driver>TagConfigModel for driver types that have a typed editor; the model-less Galaxy /// driver gets the canonical camelCase attributeRef key directly. /// + /// The data type is not optional decoration for AB CIP / TwinCAT / FOCAS — their tag-definition + /// factories decode the wire bytes by this field, not by the raw tag row's OPC UA DataType. A + /// committed leaf left at the model default therefore reads real data through the wrong decoder and + /// publishes a plausible wrong number rather than an error (Gitea #536). + /// + /// /// MQTT is the one driver whose address is not a single string — a Sparkplug tag binds by a /// (group, edgeNode, device?, metric) tuple — so it is built from the browse session's stated /// instead. See . @@ -218,13 +229,25 @@ public static class RawBrowseCommitMapper if (Is(driverType, DriverTypeNames.OpcUaClient)) return new OpcUaClientTagConfigModel { NodeId = address }.ToJson(); if (Is(driverType, DriverTypeNames.AbCip)) - return new AbCipTagConfigModel { TagPath = address }.ToJson(); + return new AbCipTagConfigModel + { + TagPath = address, + DataType = NativeType(addressFields, AbCipDataType.DInt), + }.ToJson(); if (Is(driverType, DriverTypeNames.AbLegacy)) return new AbLegacyTagConfigModel { Address = address }.ToJson(); if (Is(driverType, DriverTypeNames.TwinCAT)) - return new TwinCATTagConfigModel { SymbolPath = address }.ToJson(); + return new TwinCATTagConfigModel + { + SymbolPath = address, + DataType = NativeType(addressFields, TwinCATDataType.DInt), + }.ToJson(); if (Is(driverType, DriverTypeNames.FOCAS)) - return new FocasTagConfigModel { Address = address }.ToJson(); + return new FocasTagConfigModel + { + Address = address, + DataType = NativeType(addressFields, FocasDataType.Int32), + }.ToJson(); if (Is(driverType, DriverTypeNames.S7)) return new S7TagConfigModel { Address = address }.ToJson(); // MTConnect: the DataItem id. The driver also accepts "address"/"dataItemId", but the typed @@ -338,6 +361,46 @@ public static class RawBrowseCommitMapper + "the tag manually."; } + /// + /// Reads the browse leaf's driver-native data type out of the stated address fields, falling + /// back to — the driver model's own default — when the browser + /// stated none or stated one this driver's enum does not name. + /// + /// The driver's own data-type enum. + /// The leaf's stated address fields. + /// The model default to keep when no usable native type was stated. + /// The parsed native type, or . + /// + /// The type is READ, never derived (Gitea #536). A commit used to leave this at the + /// model default, so every browsed AB CIP leaf — a REAL, a BOOL, a STRING — was written as + /// DInt and the driver decoded four bytes of a float as an integer: a plausible + /// number, no error, wrong value. Deriving it from the leaf's driver-AGNOSTIC + /// DriverDataType instead would fix the REAL and still get the integers wrong — + /// SInt, Int and DInt all collapse into Int32 on the way out, so + /// a 16-bit INT would come back as a 32-bit DINT. The producing driver knows its own type + /// and states it; see CapturedTreeBrowseSession.NativeTypeFields. + /// + /// Case-insensitive because the key travels as an enum NAME, and a driver is free to spell + /// it however its own enum does. An unparseable value keeps the default rather than failing + /// the commit: the address — the part that cannot be re-derived later — is still correct, + /// and the operator can fix the type in the typed editor. + /// + /// + /// A numeric string is refused before parsing. Enum.TryParse accepts "9" + /// for any enum and hands back whatever member sits at ordinal 9 — which Enum.IsDefined + /// then happily confirms — so a driver that stamped a number would bind a tag to a type nobody + /// named. Same trap MTConnectTagConfigModel documents for its own dataType. + /// + /// + private static TEnum NativeType(IReadOnlyDictionary? addressFields, TEnum fallback) + where TEnum : struct, Enum + => TryGetField(addressFields, "dataType") is { Length: > 0 } raw + && !char.IsAsciiDigit(raw[0]) && raw[0] is not ('-' or '+') + && Enum.TryParse(raw, ignoreCase: true, out var parsed) + && Enum.IsDefined(parsed) + ? parsed + : fallback; + /// Reads one non-blank address field, or null when it is absent or blank. /// The stated address fields, or null. /// The field to read. diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Browsing/CapturedTreeBrowseSessionTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Browsing/CapturedTreeBrowseSessionTests.cs index 9aa3a433..76cbdfb2 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Browsing/CapturedTreeBrowseSessionTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/Browsing/CapturedTreeBrowseSessionTests.cs @@ -70,6 +70,43 @@ public class CapturedTreeBrowseSessionTests alarmAttrs.Single().IsAlarm.ShouldBeTrue(); } + /// + /// A driver's OWN type name reaches the picker as the dataType address field, so the + /// browse-commit can stamp the driver-typed TagConfig with the type the device reported rather + /// than the tag model's default (Gitea #536). It cannot ride + /// : that is the driver-agnostic type, and several + /// native types share one agnostic value. + /// + [Fact] + public async Task AttributesCarryTheDriverNativeTypeForTheCommitPath() + { + var b = new CapturingAddressSpaceBuilder(); + b.Variable("Speed", "Speed", new DriverAttributeInfo( + "Pump_1_Speed", DriverDataType.Float32, false, null, + SecurityClassification.Operate, false, NativeDataType: "Real")); + await using var s = new CapturedTreeBrowseSession(b.Build()); + + var a = (await s.AttributesAsync("Pump_1_Speed", CancellationToken.None)).Single(); + + a.AddressFields.ShouldNotBeNull()["dataType"].ShouldBe("Real"); + a.DriverDataType.ShouldBe(DriverDataType.Float32.ToString()); // the agnostic type is unchanged + } + + /// + /// A driver that states no native type states NOTHING — the commit then keeps its model default + /// instead of a guess. An empty-but-present dictionary would be indistinguishable from "the + /// driver stamped a blank type", so the absence has to be represented as absence. + /// + [Fact] + public async Task AttributesCarryNoAddressFieldsWhenTheDriverStatedNoNativeType() + { + await using var s = new CapturedTreeBrowseSession(SampleTree()); + + var a = (await s.AttributesAsync("Plc1.Speed", CancellationToken.None)).Single(); + + a.AddressFields.ShouldBeNull(); + } + [Fact] public async Task TruncatedTreeAppendsMarkerFolderAtRoot() { 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 a4478384..1c9b3844 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 @@ -12,10 +12,12 @@ clone needs Docker Desktop and nothing else. |---|---| | [`Dockerfile`](Dockerfile) | Multi-stage: build from libplctag at pinned tag → copy binary into `debian:bookworm-slim` runtime image | | [`docker-compose.yml`](docker-compose.yml) | One service per family (`controllogix` / `compactlogix` / `micro800` / `guardlogix`); all bind `:44818` | -| [`patches/`](patches/) | Applied to the libplctag checkout before the build — see [Local patches](#local-patches) | +| [`patches/`](patches/) | Applied to the libplctag checkout before the build, in filename order — see [Local patches](#local-patches) | ## Local patches +### 0001 — serve the Symbol Object listing + `patches/0001-ab_server-serve-cip-symbol-object-listing.patch` teaches `ab_server` to answer **CIP service 0x55 (Get Instance Attribute List) on class 0x6B**, the Symbol Object listing a client reads through the `@tags` pseudo-tag to enumerate @@ -49,7 +51,28 @@ Partial-transfer paging is exercised the same way by starting `ab_server` with a few hundred tags: all of them should come back, with no duplicates and no gaps at the page boundaries. -**Bumping `LIBPLCTAG_TAG` means re-checking the patch against the new tag.** It +### 0002 — answer "tag does not exist" the way a controller does + +`patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch` changes the reply +to an unresolvable tag path from CIP **0x03** (invalid parameter) to CIP **0x04** +(*IOI could not be deciphered or tag does not exist*), which is what a Logix +controller returns. libplctag translates the two very differently — `ErrorBadParam` +versus `ErrorNotFound` — and every client that classifies by status code reads the +first as "your request was malformed" and the second as "the controller answered, +that tag isn't there." + +The distinction is load-bearing here: the driver's Test Connect probe falls back to a +name no controller defines, so **"tag not found" is the successful answer** when a +device has no authored tags yet. Before this patch the fixture reported a perfectly +healthy sim as a failed handshake, and — worse — the discrepancy was written down as +a fixture limitation, which hid a real driver defect behind it for as long as it +stood (`AbCipDriverProbe.IsReachableException` matched the native C spellings, which +the .NET wrapper never produces, so the reachable classification could not fire at +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 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. @@ -117,11 +140,6 @@ place means updating both. unit tests with golden byte buffers. The Symbol Object patch above lists symbols, including their types; it does not add Template Object support, so a browsed UDT still cannot be expanded into members here. -- **No `@raw_cpu_type`** — the driver's Test Connect probe falls back to that - pseudo-tag when a device has no authored tags, and `ab_server` answers - `ErrorBadParam`. So **Test Connect reports failure against this fixture even - when the device is fine**, and browse works regardless. Author one tag and the - probe uses it instead. - **Controller-scope listing only** — a program-scope listing arrives with a leading symbolic segment naming the program, and `ab_server` has no notion of programs. `Program:`-scoped tags cannot be browsed here. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch new file mode 100644 index 00000000..3e7635e6 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch @@ -0,0 +1,30 @@ +diff --git a/src/tools/ab_server/cip.c b/src/tools/ab_server/cip.c +index 68a5794..f286504 100644 +--- a/src/tools/ab_server/cip.c ++++ b/src/tools/ab_server/cip.c +@@ -746,7 +746,11 @@ slice_s handle_read_request(uint8_t cip_service, slice_s cip_service_path, slice + if(!parse_tag_path(cip_service_path, plc, &tag, &num_indexes, &(indexes[0]))) { + log_info("Unable to parse tag path:"); + log_info_slice(cip_service_path); +- return make_cip_log_error(output, cip_service, CIP_ERR_INVALID_PARAM, false, 0); ++ /* A Logix controller answers an unresolvable tag path with 0x04 ("IOI could not be ++ * deciphered or tag does not exist"), which clients read as "tag not found" and ++ * distinguish from a malformed request. Answering 0x03 instead reported a live ++ * controller as a failed handshake to any client that classifies by status code. */ ++ return make_cip_log_error(output, cip_service, CIP_ERR_PATH_SEGMENT, false, 0); + } + + /* Record request arrival time for latency tracking */ +@@ -916,7 +920,11 @@ slice_s handle_write_request(uint8_t cip_service, slice_s cip_service_path, slic + if(!parse_tag_path(cip_service_path, plc, &tag, &num_indexes, &(indexes[0]))) { + log_info("Unable to parse tag path:"); + log_info_slice(cip_service_path); +- return make_cip_log_error(output, cip_service, CIP_ERR_INVALID_PARAM, false, 0); ++ /* A Logix controller answers an unresolvable tag path with 0x04 ("IOI could not be ++ * deciphered or tag does not exist"), which clients read as "tag not found" and ++ * distinguish from a malformed request. Answering 0x03 instead reported a live ++ * controller as a failed handshake to any client that classifies by status code. */ ++ return make_cip_log_error(output, cip_service, CIP_ERR_PATH_SEGMENT, false, 0); + } + + /* Record request arrival time for latency tracking */ diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDataTypeExtensionsTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDataTypeExtensionsTests.cs new file mode 100644 index 00000000..79777a1a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDataTypeExtensionsTests.cs @@ -0,0 +1,44 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Pins the one property of that another +/// subsystem depends on: it is LOSSY, so it cannot be run backwards. +/// +[Trait("Category", "Unit")] +public sealed class AbCipDataTypeExtensionsTests +{ + /// + /// Four distinct Logix integer widths leave the driver as one driver-agnostic type. This is why a + /// browse-commit carries the native type instead of deriving it from the leaf's + /// (Gitea #536) — a reverse map would have to pick one width for all + /// four, and picking wrong reads the wrong number of bytes with no error raised anywhere. + /// + [Fact] + public void The_integer_widths_collapse_which_is_why_the_native_type_is_carried() + { + var widths = new[] + { + AbCipDataType.SInt, AbCipDataType.Int, AbCipDataType.DInt, + AbCipDataType.USInt, AbCipDataType.UInt, + }; + + widths.Select(t => t.ToDriverDataType()).Distinct().ShouldHaveSingleItem() + .ShouldBe(DriverDataType.Int32); + } + + /// + /// The falsifying half: an unsigned 32-bit tag does NOT collapse with the rest, so a test that only + /// showed "several types share an output" would pass against a map that collapsed everything. + /// + [Fact] + public void The_wider_unsigned_types_stay_distinct() + { + AbCipDataType.UDInt.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + AbCipDataType.ULInt.ToDriverDataType().ShouldBe(DriverDataType.UInt64); + AbCipDataType.Real.ToDriverDataType().ShouldBe(DriverDataType.Float32); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs index 5be7525d..439a5bda 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs @@ -34,6 +34,36 @@ public sealed class AbCipDriverDiscoveryTests builder.Variables.Single(v => v.BrowseName == "Temperature").Info.SecurityClass.ShouldBe(SecurityClassification.ViewOnly); } + /// + /// Every emitted variable states its Logix type, so a browse-commit binds the tag with the type + /// the controller has rather than the tag model's DInt default (Gitea #536). The + /// driver-agnostic cannot serve — SInt, + /// Int and DInt all arrive there as . + /// + [Fact] + public async Task Emitted_variables_state_their_Logix_type_for_the_commit_path() + { + var builder = new RecordingBuilder(); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Speed", "ab://10.0.0.5/1,0", "Motor1.Speed", AbCipDataType.Real), + new AbCipTagDefinition("Count", "ab://10.0.0.5/1,0", "C", AbCipDataType.Int)), + }, "drv-1"); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.DiscoverAsync(builder, CancellationToken.None); + + builder.Variables.Single(v => v.BrowseName == "Speed").Info.NativeDataType.ShouldBe("Real"); + + // Int and DInt are indistinguishable downstream — this pair is the whole reason the native + // name is carried, so assert both halves rather than only the one that differs by kind. + var count = builder.Variables.Single(v => v.BrowseName == "Count").Info; + count.NativeDataType.ShouldBe("Int"); + count.DriverDataType.ShouldBe(DriverDataType.Int32); + } + /// Verifies that device folder display name falls back to host address when not provided. [Fact] public async Task Device_folder_displayname_falls_back_to_host_address() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverProbeTests.cs index c4dd326a..ba406b9f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverProbeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverProbeTests.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Sockets; +using libplctag; using Shouldly; using Xunit; @@ -144,4 +145,45 @@ public sealed class AbCipDriverProbeTests $"Expected 'timed out' or 'Connect failed' but got: {result.Message}"); result.Latency.ShouldBeNull(); } + + // ------------------------------------------------------------------------- + // 5. Reachable-vs-failed classification (offline — no controller needed) + // ------------------------------------------------------------------------- + + /// + /// The exception path classifies by the SAME rule as the status path. libplctag.NET builds + /// with Status.ToString(), so its message is + /// "ErrorNotFound" — this method used to look for the native C spelling "STATUS_NOT_FOUND", + /// which that message can never contain, so a controller that answered "tag does not exist" + /// was reported as a failed handshake. That is the answer every Test Connect gets on a device + /// with no authored tags, i.e. every device at the moment an operator first presses the button. + /// + /// The status libplctag threw on. + /// Whether the controller should be reported reachable. + [Theory] + [InlineData(Status.ErrorNotFound, true)] + [InlineData(Status.ErrorNoMatch, true)] + [InlineData(Status.ErrorBadDevice, true)] + [InlineData(Status.ErrorTimeout, false)] + [InlineData(Status.ErrorBadConnection, false)] + [InlineData(Status.ErrorBadGateway, false)] + [InlineData(Status.ErrorBadParam, false)] + public void ThrownStatus_IsClassifiedTheSameWayAsAReturnedStatus(Status status, bool expectedReachable) + { + // Exactly how NativeTagWrapper.ThrowIfStatusNotOk constructs it. + var ex = new LibPlcTagException(status.ToString()); + + AbCipDriverProbe.IsReachableException(ex).ShouldBe(expectedReachable); + AbCipDriverProbe.IsReachableStatus(status).ShouldBe(expectedReachable); + } + + /// + /// A message that names no status at all stays UNreachable rather than defaulting green — the + /// conservative half, and the one an over-eager "treat anything we cannot parse as reachable" + /// fix would break. + /// + [Fact] + public void AnUnrecognisedExceptionMessageIsNotTreatedAsReachable() + => AbCipDriverProbe.IsReachableException( + new LibPlcTagException("something the wrapper has never said")).ShouldBeFalse(); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasCapabilityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasCapabilityTests.cs index 54c6243d..3bbad4e8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasCapabilityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasCapabilityTests.cs @@ -36,6 +36,13 @@ public sealed class FocasCapabilityTests // Writable field, because WireFocasClient.WriteAsync always returns BadNotWritable. builder.Variables.Single(v => v.BrowseName == "Run").Info.SecurityClass.ShouldBe(SecurityClassification.ViewOnly); builder.Variables.Single(v => v.BrowseName == "Alarm").Info.SecurityClass.ShouldBe(SecurityClassification.ViewOnly); + + // Each variable states its FOCAS type for the browse-commit path (Gitea #536). Byte is the + // case that matters: it reaches Info.DriverDataType as Int32, exactly like Int16 and Int32, + // so a commit deriving the type from there would read four PMC bytes for a one-byte tag. + var run = builder.Variables.Single(v => v.BrowseName == "Run").Info; + run.NativeDataType.ShouldBe("Byte"); + run.DriverDataType.ShouldBe(DriverDataType.Int32); } // ---- ISubscribable ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs index a3fa2752..ff43a6a4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs @@ -34,6 +34,12 @@ public sealed class TwinCATCapabilityTests builder.Folders.ShouldContain(f => f.BrowseName == "ads://5.23.91.23.1.1:851" && f.DisplayName == "Mach1"); builder.Variables.Single(v => v.BrowseName == "Speed").Info.SecurityClass.ShouldBe(SecurityClassification.Operate); builder.Variables.Single(v => v.BrowseName == "Status").Info.SecurityClass.ShouldBe(SecurityClassification.ViewOnly); + + // Each variable states its IEC type, so a browse-commit binds the tag with the type the PLC + // declares rather than TwinCATTagConfigModel's DInt default (Gitea #536). It cannot be + // recovered from Info.DriverDataType — Byte/Int16/Int32 all land there as Int32. + builder.Variables.Single(v => v.BrowseName == "Status").Info.NativeDataType.ShouldBe("Bool"); + builder.Variables.Single(v => v.BrowseName == "Speed").Info.NativeDataType.ShouldBe("DInt"); } // ---- ISubscribable ---- 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 832496c1..d4cb90b9 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 @@ -6,6 +6,7 @@ using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; using ZB.MOM.WW.OtOpcUa.Commons.Types; using ZB.MOM.WW.OtOpcUa.Configuration.Enums; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; using ZB.MOM.WW.OtOpcUa.Driver.Mqtt; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; @@ -98,6 +99,126 @@ public sealed class RawBrowseCommitMapperTests JsonNode.Parse(json)!.AsObject()["address"]!.GetValue().ShouldBe("40001"); } + // ---- BuildTagConfig: the browsed data type (Gitea #536) -------------------------------------- + + /// The address fields a captured browse leaf states for its driver-native type. + /// The driver's own data-type enum name, as the driver stamped it. + /// The stated address fields. + private static Dictionary NativeTypeFields(string nativeType) => + new() { ["dataType"] = nativeType }; + + /// + /// The committed dataType is the type the CONTROLLER reported, not the model default. + /// AB CIP / TwinCAT / FOCAS decode the wire bytes by this field, so a browsed REAL left at the + /// DInt default reads four bytes of a float as an integer and publishes a plausible + /// wrong number with no error anywhere. + /// + /// The owning device's driver type. + /// The native type the browse leaf stated. + /// The leaf's driver-side reference. + [Theory] + [InlineData("AbCip", "Real", "Pump_1_Speed")] + [InlineData("AbCip", "Bool", "Motor_Run")] + [InlineData("AbCip", "String", "Recipe_Name")] + [InlineData("TwinCAT", "Bool", "MAIN.bStart")] + [InlineData("FOCAS", "Float64", "MACRO100")] + public void BuildTagConfig_stamps_the_browsed_data_type( + string driverType, string nativeType, string reference) + { + var json = RawBrowseCommitMapper.BuildTagConfig(driverType, reference, NativeTypeFields(nativeType)); + + JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue().ShouldBe(nativeType); + } + + /// + /// The narrow integer widths are the reason the native type is CARRIED rather than derived from + /// the leaf's driver-agnostic DriverDataType: SInt, Int and DInt all + /// leave the driver as Int32, so a reverse map would read a 16-bit INT through a 32-bit + /// decoder — two bytes of the next tag's memory included — and still look like a fix. + /// + /// The Logix integer width the controller reported. + [Theory] + [InlineData("SInt")] + [InlineData("Int")] + [InlineData("DInt")] + [InlineData("UDInt")] + public void BuildTagConfig_keeps_integer_widths_that_the_agnostic_type_collapses(string nativeType) + { + // That these widths really do collapse on the way out of the driver is pinned next to the map + // itself — AbCipDataTypeExtensionsTests.The_integer_widths_collapse_which_is_why_the_native_type_is_carried. + var json = RawBrowseCommitMapper.BuildTagConfig("AbCip", "Line1_Count", NativeTypeFields(nativeType)); + + JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue().ShouldBe(nativeType); + } + + /// + /// A leaf that states no type — a browser that reports none, or a session captured before the + /// driver stamped it — keeps the model default rather than guessing. That is the pre-#536 + /// behaviour, deliberately retained as the fallback. + /// + /// The owning device's driver type. + /// The model default the commit must fall back to. + [Theory] + [InlineData("AbCip", "DInt")] + [InlineData("TwinCAT", "DInt")] + [InlineData("FOCAS", "Int32")] + public void BuildTagConfig_falls_back_to_the_model_default_when_no_type_is_stated( + string driverType, string expectedDefault) + { + var noFields = RawBrowseCommitMapper.BuildTagConfig(driverType, "X"); + var unparseable = RawBrowseCommitMapper.BuildTagConfig( + driverType, "X", NativeTypeFields("NotATypeThisDriverKnows")); + + JsonNode.Parse(noFields)!.AsObject()["dataType"]!.GetValue().ShouldBe(expectedDefault); + JsonNode.Parse(unparseable)!.AsObject()["dataType"]!.GetValue().ShouldBe(expectedDefault); + } + + /// + /// A numeric string is NOT a type name. Enum.TryParse accepts "3" for any enum and yields + /// whatever member happens to sit at 3, so an unguarded parse would silently bind a tag to a type + /// nobody named — the same trap MTConnectTagConfigModel documents for its own dataType. + /// + [Fact] + public void BuildTagConfig_rejects_a_numeric_type_rather_than_indexing_the_enum() + { + // AbCipDataType member 9 is Real; a numeric parse would hand back exactly that. + ((AbCipDataType)9).ShouldBe(AbCipDataType.Real); + + var json = RawBrowseCommitMapper.BuildTagConfig("AbCip", "X", NativeTypeFields("9")); + + JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue().ShouldBe("DInt"); + } + + /// + /// The stated type never displaces the address — the two travel in different fields and a commit + /// that got the type right by losing the binding would be no better than the defect. + /// + [Fact] + public void BuildTagConfig_stamps_the_type_without_disturbing_the_address() + { + var json = RawBrowseCommitMapper.BuildTagConfig( + "AbCip", "Program:Main.Motor.Speed", NativeTypeFields("Real")); + + var o = JsonNode.Parse(json)!.AsObject(); + o["tagPath"]!.GetValue().ShouldBe("Program:Main.Motor.Speed"); + o["dataType"]!.GetValue().ShouldBe("Real"); + } + + /// + /// A driver whose tag config carries no data type is unaffected — the key must not appear, or a + /// typed editor would round-trip a field its model does not own. + /// + /// The owning device's driver type. + [Theory] + [InlineData("OpcUaClient")] + [InlineData("GalaxyMxGateway")] + public void BuildTagConfig_does_not_invent_a_data_type_for_a_driver_that_has_none(string driverType) + { + var json = RawBrowseCommitMapper.BuildTagConfig(driverType, "X", NativeTypeFields("Real")); + + JsonNode.Parse(json)!.AsObject().ContainsKey("dataType").ShouldBeFalse(); + } + // ---- BuildTagConfig: MQTT (the address is a DESCRIPTOR, not a single reference string) -------- /// The Sparkplug address a browse session states for a metric under a device. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs index 25de396a..850b6553 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/DriverProbeHandshakeE2eTests.cs @@ -173,11 +173,7 @@ public sealed class DriverProbeHandshakeE2eTests { SkipUnless(DockerHost, AbCipPort); // Probe an explicit tag that the ab_server ControlLogix sim actually defines - // (`TestDINT:DINT[1]`). The no-tags fallback (`@raw_cpu_type`) is NOT exercised here: - // ab_server answers an unknown/unsupported tag with libplctag ErrorBadParam (a REAL - // ControlLogix instead returns ErrorNotFound, which the probe classifies as - // reachable). Whether the `@raw_cpu_type` system-tag fallback is valid on a real - // ControlLogix is a hardware-gated follow-up (AbCipDriverOptions.cs flags it deferred). + // (`TestDINT:DINT[1]`). The no-tags fallback is exercised separately, below. // ⚠️ The config must use the v3 `RawTags` shape — `[{RawPath, TagConfig, WriteIdempotent, // DeviceName}]`, where the Logix tag path lives inside the TagConfig blob as `tagPath`. This // test passed the retired v2 `Tags: [{DeviceHostAddress, TagPath}]` shape until 2026-08-01, so @@ -200,4 +196,34 @@ public sealed class DriverProbeHandshakeE2eTests // ever resolved. Reading the authored tag is the entire point of this test. result.Message.ShouldBe("CIP session OK"); } + + /// + /// Test Connect on a device with NO authored tags — which is every AB CIP device at the moment + /// an operator first presses the button — must report the controller as reachable. + /// + /// + /// The probe falls back to a name no controller defines, so the green answer arrives as a CIP + /// "tag does not exist" (0x04 → libplctag ErrorNotFound), which + /// AbCipDriverProbe.IsReachableStatus classifies as reachable. Until 2026-08-01 that + /// classification had never been exercised against any backend: ab_server answered an + /// unresolvable tag path with 0x03 (ErrorBadParam) where a Logix controller answers 0x04, + /// so the fixture reported a healthy sim as a failed handshake and the discrepancy was written + /// down as a fixture limitation rather than fixed. It is now fixed in the vendored patch — + /// Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch — so this test + /// needs the fixture rebuilt from that patch, not merely running. + /// + [Fact] + public async Task AbCip_WithNoAuthoredTags_ReportsTheControllerReachable() + { + SkipUnless(DockerHost, AbCipPort); + var result = await new AbCipDriverProbe().ProbeAsync( + $"{{\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}],\"RawTags\":[]}}", + Timeout, Ct); + + result.Ok.ShouldBeTrue($"Probe message: {result.Message}"); + + // EXACT for the same reason as the test above, and here it carries the extra weight of proving + // WHICH green this is: the fallback's "probe tag not found", not a tag that was actually read. + result.Message.ShouldBe("CIP session OK (controller reachable; probe tag not found)"); + } }