From b29623cf99f1508ea91e96beb0642f23616ae5db Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 5 Aug 2026 06:36:08 -0400 Subject: [PATCH] feat(mtconnect): IWritable via agent observation writes MTConnectDriver now implements IWritable over the Task-1 agent-client seam (WriteObservationAsync -> the Agent's AllowPut POST surface). A write is OBSERVATION INJECTION, not machine command: it sets what the Agent reports for a DataItem, is visible to every consumer of that Agent, and is overwritten by the next adapter/SHDR update. Off at two independent gates, both off by default - the Agent's own AllowPut and the driver's write.enabled (Task 0). Per-slot refusal order (never throws, strict N-in/N-out batch shape): write disabled -> BadNotWritable no live client -> BadNotConnected (ReadAsync's existing convention) reference unresolved -> BadNodeIdUnknown no resolvable device -> BadNodeIdUnknown CONDITION category -> BadNotWritable null / non-scalar value -> BadTypeMismatch agent refusal OR throw -> BadCommunicationError OperationCanceledException fills the remaining slots and returns. The Agent's refusal text is logged at Warning and never enters DriverHealth.LastError (type-only discipline); a refused write leaves health untouched. Device-name resolution: TagBinding gains WriteTargetsByDataItemId, built in BuildBinding from a probe-model walk that remembers which device each DataItem was declared under (name, or id when nameless) plus its category. Falls back to options.DeviceName; unresolvable -> BadNodeIdUnknown rather than a guess. Values render via Convert.ToString(..., InvariantCulture) - so a Boolean writes the invariant string "True"/"False", NOT "1"/"0". Arrays are refused rather than rendered: Convert.ToString would emit the literal "System.Double[]", which the Agent stores happily and which is invisible until read-back. FINDING - the CurrentWrite bit does NOT key on SecurityClassification, and the discovery stamping below is therefore cosmetic. Traced end to end: a browsed leaf's SecurityClass survives into the browse session's AttributeInfo (CapturedTreeBrowseSession.cs:53) and stops there - RawBrowseModal never reads it, RawBrowseCommitMapper.MapLeaf hardcodes TagAccessLevel.Read, and Tag has no SecurityClass column. AddressSpaceComposer:746/:809 sets Writable purely from Tag.AccessLevel == ReadWrite; AddressSpaceApplier ANDs !IsArray; OtOpcUaNodeManager.ComposeAccessLevel:2002 composes CurrentWrite from that alone. Driver capability is never consulted at compose time either - IWritable is checked only at runtime in DriverInstanceActor:755. Consequences: (a) raw tags already committed keep their authored AccessLevel, so nothing this commit does makes an existing node writable; (b) an operator must still set the tag to ReadWrite for an MTConnect write to be reachable over OPC UA; (c) conversely a ReadWrite tag on a non-IWritable driver has always advertised CurrentWrite and failed only at the actor - pre-existing, not touched here. Discovery stamps CanWrite(writeEnabled, dataItem) = write.enabled && !CONDITION -> Operate, else ViewOnly - kept honest so the browse never advertises a tier the write path would refuse (MQTT's CanWrite principle), with the caveat above recorded verbatim at the call site. Status-code constants are spelled WITHOUT MqttDriver's "Status" prefix, to match the two this file already declares (BadCommunicationError, BadNotConnected). StatusCodeParityTests strips either spelling and passes. TDD: WriteAsync landed first as a NotImplementedException scaffold - 19 of 20 new tests red on behaviour, 1 (the capability assertion) green on the scaffold - then implemented. MTConnectFactoryTests' "not_writable" characterization test is rewritten to pin the new truth, and is the guard for the concrete-return contract the factory remark explains. Tests: 532 passed / 0 failed (was ~509). Full solution build: 0 errors, 0 warnings; OTOPCUA0001 clean. --- .../MTConnectTagDefinition.cs | 9 +- .../MTConnectDriver.cs | 450 +++++++++++++++++- .../MTConnectDriverFactoryExtensions.cs | 14 +- .../MTConnectDiscoverTests.cs | 55 ++- .../MTConnectFactoryTests.cs | 16 +- .../MTConnectWriteTests.cs | 356 ++++++++++++++ 6 files changed, 868 insertions(+), 32 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectTagDefinition.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectTagDefinition.cs index c956f809..aaf040b8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectTagDefinition.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectTagDefinition.cs @@ -8,9 +8,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// current value from the Agent's /current snapshot / /sample stream by that id. /// /// -/// The MTConnect DataItem's id attribute (the Agent's /probe response). This is -/// the driver's reference for read/subscribe — MTConnect is a read-only source protocol, so -/// there is no corresponding write path. +/// The MTConnect DataItem's id attribute (the Agent's /probe response). This is the +/// driver's reference for read, subscribe and write — the write being an observation +/// injection through the Agent's AllowPut surface +/// (POST {agent}/{device}?{id}={value}), which sets what the Agent reports for the DataItem +/// rather than commanding the machine. It is off unless both the Agent's AllowPut and the +/// driver's write.enabled say otherwise, and CONDITION-category items are excluded. /// /// Logical data type the DataItem's value is coerced to. /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index 53e41b9d..d8324306 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -44,12 +44,23 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// /// /// Scope. This type implements , , -/// , , +/// , , , /// and — the last two /// built on state this class already caches (the Agent instanceId and the probe -/// model). There is deliberately no IWritable: MTConnect is a read-only protocol, -/// which is also why every discovered leaf is -/// . +/// model). +/// +/// +/// is observation injection, not machine command, and it is +/// off by default. A write goes to the Agent's AllowPut surface +/// (POST {agent}/{device}?{dataItemId}={value}) and sets what the Agent reports +/// for that DataItem — visible to every consumer of that Agent, and overwritten by the next +/// adapter/SHDR update. Nothing reaches the CNC. It is therefore gated twice, and off at both +/// gates by default: the Agent's own AllowPut (agent.cfg) and this driver's +/// . Either gate alone leaves the shipped read-only +/// behaviour byte-for-byte unchanged. CONDITION data items are excluded — they carry alarm +/// semantics and a different level|text wire format. MTConnect Interfaces, the +/// protocol's own request/response handshake, remains deferred and is a different mechanism +/// entirely (see docs/plans/2026-08-05-mtconnect-writeback-design.md §2.1). /// /// /// The read path never writes the shared observation index — see @@ -60,15 +71,43 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// (see ). /// /// -public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDiscovery, IHostConnectivityProbe, IRediscoverable +public sealed class MTConnectDriver + : IDriver, IReadable, IWritable, ISubscribable, ITagDiscovery, IHostConnectivityProbe, IRediscoverable { /// /// The Agent could not be reached, answered unusably, or blew the per-call deadline. Distinct /// from the index's BadNoCommunication, which means the Agent answered and said the - /// machine has no value — the two failures need different operator responses. + /// machine has no value — the two failures need different operator responses. On the write + /// path it also covers an Agent that answered and refused — see . /// private const uint BadCommunicationError = 0x80050000u; + // ---- Write status codes ---- + // + // NAMED constants, never inline literals: StatusCodeParityTests checks each against the pinned SDK + // by reflection and can only see a named const (Gitea #497/#519). The names must match + // Opc.Ua.StatusCodes members exactly. Spelled WITHOUT the "Status" prefix MqttDriver uses, to match + // the two this file already declares (the parity guard strips either spelling). + + /// + /// The reference names no DataItem this driver serves, or names one whose owning device cannot + /// be resolved — there is no Agent URL a write could be addressed to. + /// + private const uint BadNodeIdUnknown = 0x80340000u; + + /// + /// Writing is switched off (write.enabled), or the DataItem is CONDITION-category. The + /// honest answer to "can I write this?", and deliberately not a communication failure: nothing + /// was attempted and no Agent was involved. + /// + private const uint BadNotWritable = 0x803B0000u; + + /// + /// The supplied value has no invariant string form a PUT could carry — null, or a + /// non-scalar (see ). + /// + private const uint BadTypeMismatch = 0x80740000u; + /// /// There is no Agent client at all: the driver has not been initialized, its initialize /// faulted, or it has been shut down. Deliberately not one of the index's codes — none of @@ -694,6 +733,200 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis } } + // ---- IWritable: observation injection through the Agent's AllowPut surface ---- + + /// + /// Sets the Agent's reported observation for each requested reference, through its + /// AllowPut surface. Returns one status per request, in request order. + /// + /// The requested writes. + /// Cancellation for the batch. + /// One per request, in request order. + /// + /// + /// This writes the Agent, not the machine — see the class remarks. Good here means + /// "the Agent took the observation", which is a stronger statement than MQTT's optimistic + /// "published" (the Agent answers) and a weaker one than a device acknowledgement (no + /// MTConnect Agent speaks for the machine). The next adapter update for that DataItem + /// overwrites what was written, and the ordinary /sample ingest path is what + /// corrects the node. + /// + /// + /// Never throws, and one bad slot never fails the batch — matching + /// . Failing the whole call would let one stale reference reject + /// every other write in the same OPC UA request. Even cancellation returns a full-length + /// batch rather than propagating, so the node-manager can tell which slots were attempted. + /// + /// + /// Refusal order is fixed, and each step answers a different operator question: + /// writing switched off ⇒ BadNotWritable; no live client ⇒ BadNotConnected + /// (this driver is not running — 's convention); the reference + /// resolves to no DataItem, or to one whose owning device cannot be named ⇒ + /// BadNodeIdUnknown; CONDITION category ⇒ BadNotWritable; no invariant string + /// form ⇒ BadTypeMismatch; and only then the Agent, whose refusal or whose + /// unreachability ⇒ BadCommunicationError. + /// + /// + /// The Agent's refusal text is logged, never stored. It goes to a Warning line and + /// nowhere near , which is type-only by repo-wide + /// discipline — and a refused write is not evidence the read path degraded, so health is + /// left exactly as it is (unlike ). + /// + /// + /// is null — a caller bug. + public async Task> WriteAsync( + IReadOnlyList writes, CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(writes); + + if (writes.Count == 0) + { + return []; + } + + // Read once each, for the same reason ReadAsync does: every slot in this batch is judged + // against ONE options document and ONE tag table, never half against a table a concurrent + // re-initialize just replaced. + var options = _options; + var client = Volatile.Read(ref _session)?.Client; + var binding = Volatile.Read(ref _binding); + + var results = new WriteResult[writes.Count]; + + for (var i = 0; i < writes.Count; i++) + { + var request = writes[i]; + + if (!options.Write.Enabled) + { + results[i] = new WriteResult(BadNotWritable); + continue; + } + + if (client is null) + { + results[i] = new WriteResult(BadNotConnected); + continue; + } + + if (!binding.TryResolveWriteTarget(request.FullReference, out var dataItemId, out var target) + || target.DeviceName is null) + { + results[i] = new WriteResult(BadNodeIdUnknown); + continue; + } + + if (target.IsCondition) + { + results[i] = new WriteResult(BadNotWritable); + continue; + } + + if (!TryFormatValue(request.Value, out var text)) + { + results[i] = new WriteResult(BadTypeMismatch); + continue; + } + + try + { + var outcome = await client + .WriteObservationAsync(target.DeviceName, dataItemId, text, cancellationToken) + .ConfigureAwait(false); + + if (outcome.Accepted) + { + results[i] = new WriteResult(0u); + continue; + } + + _logger.LogWarning( + "MTConnect driver {DriverInstanceId}: {AgentUri} refused the write of '{Value}' to DataItem '{DataItemId}' on device '{DeviceName}' — {AgentError}. Check the Agent's AllowPut / AllowPutFrom settings and that the DataItem exists.", + _driverInstanceId, options.AgentUri, text, dataItemId, target.DeviceName, outcome.AgentError); + + results[i] = new WriteResult(BadCommunicationError); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // Caller-cancelled: fill the remainder rather than throwing, so the batch shape holds. + for (var j = i; j < writes.Count; j++) + { + results[j] = new WriteResult(BadCommunicationError); + } + + return results; + } + catch (Exception ex) + { + // A transport failure, a blown per-call deadline, or a disposed client. Logged with the + // exception (its TYPE is the diagnosis) and reported per slot; health is untouched. + _logger.LogWarning( + ex, + "MTConnect driver {DriverInstanceId} could not write DataItem '{DataItemId}' on device '{DeviceName}' to {AgentUri}.", + _driverInstanceId, dataItemId, target.DeviceName, options.AgentUri); + + results[i] = new WriteResult(BadCommunicationError); + } + } + + return results; + } + + /// + /// Renders a value to the invariant string a PUT query carries, or reports that it has no such + /// form. + /// + /// The value the OPC UA client supplied. + /// The rendered value; empty when this returns . + /// for a value that must be refused as a type mismatch. + /// + /// + /// is load-bearing. A comma decimal + /// separator produces a value the Agent accepts and every consumer reads back as a + /// different number. Booleans render as True/False — + /// is culture-independent, so that IS the invariant form, and + /// it is what an MTConnect consumer will see verbatim. + /// + /// + /// Arrays are refused rather than rendered. + /// falls back to + /// for a non- value, so an array + /// would be written as the literal text System.Double[] — which the Agent stores + /// happily, making the corruption invisible until someone reads the observation back. + /// TIME_SERIES / DATA_SET writes are out of scope for v1. + /// + /// + private static bool TryFormatValue(object? value, out string text) + { + text = string.Empty; + + // A PUT query has no null to send, and the empty string is a real observation rather than an + // absence — so "no value" is refused instead of being silently turned into one. + if (value is null or Array) + { + return false; + } + + string? rendered; + try + { + rendered = Convert.ToString(value, CultureInfo.InvariantCulture); + } + catch (Exception ex) when (ex is FormatException or InvalidCastException or OverflowException) + { + return false; + } + + if (rendered is null) + { + return false; + } + + text = rendered; + + return true; + } + // ---- ISubscribable: ONE shared /sample long poll behind every handle ---- /// @@ -940,7 +1173,11 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis // Read once: a concurrent re-initialize must not scope half this walk to one device and half // to another. - var deviceScope = _options.DeviceName?.Trim(); + // Both read once, together: a concurrent re-initialize must not scope half this walk to one + // device and half to another, nor stamp half its leaves writable and half not. + var options = _options; + var deviceScope = options.DeviceName?.Trim(); + var writeEnabled = options.Write.Enabled; var devices = 0; var variables = 0; @@ -966,7 +1203,7 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis } devices++; - variables += StreamContainer(device, builder.Folder(name, name)); + variables += StreamContainer(device, builder.Folder(name, name), writeEnabled); } // A configured scope that matches NO device is an authoring error — almost always a typo or @@ -1003,7 +1240,14 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis /// picker exists to show. Recursion depth is bounded by the parser, which refuses a probe /// document nested beyond its own limit. /// - private static int StreamContainer(IMTConnectComponentContainer container, IAddressSpaceBuilder scope) + /// The device or component whose data items are being streamed. + /// The builder folder they belong under. + /// + /// , read once by the caller so one walk cannot + /// stamp half its leaves under an old options document and half under a new one. + /// + private static int StreamContainer( + IMTConnectComponentContainer container, IAddressSpaceBuilder scope, bool writeEnabled) { var streamed = 0; @@ -1037,9 +1281,21 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis IsArray: inferred.IsArray, ArrayDim: inferred.ArrayDim, - // MTConnect is read-only and this driver implements no IWritable, so the tier - // that is read-only from OPC UA is the only honest one. - SecurityClass: SecurityClassification.ViewOnly, + // Operate exactly for the leaves WriteAsync would accept — the "a node cannot + // advertise writability the driver would refuse" principle. With write.enabled + // off (the default) every leaf stays ViewOnly, which is what this driver shipped. + // + // ⚠️ This does NOT make the committed tag writable over OPC UA, and must not be + // read as doing so. Nothing persists SecurityClass: RawBrowseCommitMapper.MapLeaf + // hardcodes TagAccessLevel.Read, Tag has no SecurityClass column, and the + // CurrentWrite bit is composed from Tag.AccessLevel alone + // (OtOpcUaNodeManager.ComposeAccessLevel). The stamp reaches the browse session's + // attribute side-channel and stops there, so an operator still has to author + // ReadWrite on the tag. It is kept honest anyway — a browse that advertised + // ViewOnly for a leaf this driver now writes would be a lie an operator reads. + SecurityClass: CanWrite(writeEnabled, dataItem) + ? SecurityClassification.Operate + : SecurityClassification.ViewOnly, // Historization is authored per tag, never inferred from a device model. IsHistorized: false, @@ -1067,12 +1323,26 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis continue; } - streamed += StreamContainer(component, scope.Folder(name, name)); + streamed += StreamContainer(component, scope.Folder(name, name), writeEnabled); } return streamed; } + /// + /// Whether would accept a write to — the + /// browse-time answer, asked of the same two facts the runtime asks of. + /// + /// Whether write.enabled is on. + /// The declared data item. + /// + /// The device-name leg of the runtime refusal is not asked here and does not need to be: a + /// leaf reached by this walk is inside a device the model declares, so its owning device + /// is known by construction. + /// + private static bool CanWrite(bool writeEnabled, MTConnectDataItem dataItem) => + writeEnabled && !IsCondition(dataItem.Category); + /// /// Whether a device is in this instance's configured scope. An unset scope is agent-wide. /// @@ -2933,10 +3203,13 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis if (options.RawTags.Count == 0) { + MTConnectTagDefinition[] only = [.. definitions.Values]; + return new TagBinding( FrozenDictionary.Empty, FrozenDictionary>.Empty, - [.. definitions.Values]); + only, + BuildWriteTargets(only, probeModel, options.DeviceName)); } var declared = IndexProbeDataItems(probeModel); @@ -2982,13 +3255,106 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis } } + MTConnectTagDefinition[] indexTags = [.. definitions.Values]; + return new TagBinding( idByRawPath.ToFrozenDictionary(StringComparer.Ordinal), rawPathsById.ToFrozenDictionary( pair => pair.Key, pair => (IReadOnlyList)pair.Value, StringComparer.Ordinal), - [.. definitions.Values]); + indexTags, + BuildWriteTargets(indexTags, probeModel, options.DeviceName)); + } + + /// + /// Derives the write-addressing table: for every DataItem this driver serves, the device the + /// Agent's AllowPut URL must name, and whether the item is CONDITION-category. + /// + /// Every DataItem the binding serves (the union of both authoring surfaces). + /// The Agent's device model when one has been fetched; else null. + /// + /// — the fallback for a DataItem the + /// /probe model does not declare. + /// + /// + /// + /// The device name is derived, not authored. The Agent's PUT surface is addressed + /// per device even on a single-device Agent, and the probe model already says which device + /// owns which DataItem — so a multi-device Agent is addressed correctly with no extra + /// config. Config is the fallback, for the dataItemId-keyed authoring surface (the driver + /// CLI) and for a DataItem the Agent has since stopped declaring. + /// + /// + /// A null is a refusal, not a default. Neither + /// the model nor config names a device, so there is no URL to POST to; guessing one yields + /// a 404 an operator reads as "the Agent is broken". reports it as + /// BadNodeIdUnknown. + /// + /// + /// The category comes from the probe first, the authored blob second. The Agent is + /// the authority on what a DataItem is; a blob's mtCategory is optional metadata + /// that may be absent (browse-commit writes only the address) or stale. + /// + /// + private static FrozenDictionary BuildWriteTargets( + IReadOnlyList definitions, + MTConnectProbeModel? probeModel, + string? configuredDeviceName) + { + if (definitions.Count == 0) + { + return FrozenDictionary.Empty; + } + + var owners = IndexProbeOwners(probeModel); + var fallback = string.IsNullOrWhiteSpace(configuredDeviceName) ? null : configuredDeviceName.Trim(); + var targets = new Dictionary(definitions.Count, StringComparer.Ordinal); + + foreach (var definition in definitions) + { + if (definition is null || string.IsNullOrWhiteSpace(definition.FullName)) continue; + + var owner = owners.TryGetValue(definition.FullName, out var declared) ? declared : null; + + targets[definition.FullName] = new WriteTarget( + owner?.DeviceName ?? fallback, + IsCondition(owner?.Category ?? definition.MtCategory)); + } + + return targets.ToFrozenDictionary(StringComparer.Ordinal); + } + + /// + /// Flattens a device model into a dataItemId → owning device lookup; null ⇒ empty. + /// Separate from because this one must remember which device + /// the walk was inside, which a flat DataItem lookup has thrown away. + /// + /// The Agent's device model, or null. + private static FrozenDictionary IndexProbeOwners(MTConnectProbeModel? probeModel) + { + if (probeModel is null) return FrozenDictionary.Empty; + + var map = new Dictionary(StringComparer.Ordinal); + foreach (var device in probeModel.Devices) + { + if (device is null) continue; + + // The same name/id fallback the browse tree uses: an Agent addresses a device by its + // name, and by its id when it declares no name. + var deviceName = FolderName(device.Name, device.Id); + var owner = new DataItemOwner( + string.IsNullOrWhiteSpace(deviceName) ? null : deviceName, Category: null); + + foreach (var dataItem in device.AllDataItems()) + { + if (dataItem is null || string.IsNullOrWhiteSpace(dataItem.Id)) continue; + + map[dataItem.Id] = owner with { Category = dataItem.Category }; + } + } + + return map.ToFrozenDictionary(StringComparer.Ordinal); } /// @@ -3174,11 +3540,45 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis /// . This is how a raw tag's declared (or /// probe-inferred) DriverDataType reaches the coercion. /// + /// + /// How each DataItem is addressed on the Agent's AllowPut surface, and whether it is + /// CONDITION-category — see . Keyed by DataItem id, so it + /// doubles as the write path's "is this a reference I serve at all?" test: a miss is + /// BadNodeIdUnknown, exactly as an index miss is on the read path. + /// private sealed record TagBinding( FrozenDictionary DataItemIdByRawPath, FrozenDictionary> RawPathsByDataItemId, - IReadOnlyList IndexTags) + IReadOnlyList IndexTags, + FrozenDictionary WriteTargetsByDataItemId) { + /// + /// Resolves a write reference to the DataItem id and the Agent address it writes through. + /// + /// The caller's reference (a RawPath under v3, or a DataItem id). + /// The resolved DataItem id; empty when this returns false. + /// Where and whether it can be written; default when false. + /// when no DataItem this driver serves answers to that reference. + /// + /// Stricter than , deliberately. A read may fall through to an + /// index miss and report itself; a write that fell through would POST an arbitrary + /// operator string at the Agent as if it were a DataItem id — the Agent's answer to which + /// is a refusal that reads like a comms fault. Unknown is answered here, before the wire. + /// + public bool TryResolveWriteTarget(string? reference, out string dataItemId, out WriteTarget target) + { + dataItemId = ToDataItemId(reference); + + if (dataItemId.Length != 0 && WriteTargetsByDataItemId.TryGetValue(dataItemId, out target)) + { + return true; + } + + target = default; + + return false; + } + /// /// Translates a subscriber/read reference to the DataItem id the index is keyed by. /// @@ -3218,6 +3618,24 @@ public sealed class MTConnectDriver : IDriver, IReadable, ISubscribable, ITagDis } } + /// + /// Everything needs to decide one slot before it touches the network. + /// + /// + /// The device path segment of the Agent PUT URL, or null when neither the probe model + /// nor config names one — which is a refusal, not a default (see ). + /// + /// + /// Whether the DataItem is CONDITION-category. Excluded from v1 writes: a condition PUT carries + /// alarm semantics and a different level|text wire format. + /// + private readonly record struct WriteTarget(string? DeviceName, bool IsCondition); + + /// The device a DataItem was declared under, and the category it was declared with. + /// The owning device's name (or id); null when it declares neither. + /// The DataItem's category attribute as the Agent spelled it. + private sealed record DataItemOwner(string? DeviceName, string? Category); + /// /// One live subscription: its handle and the references it wants. Both are effectively /// immutable once registered, so the pump reads without a lock — diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverFactoryExtensions.cs index 40faa2bd..cd33a5a0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverFactoryExtensions.cs @@ -31,12 +31,20 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// /// /// The instance is returned as its concrete type. The runtime resolves every optional -/// capability (, , +/// capability (, , , /// , , /// ) by pattern-matching the object /// hands back, so nothing here may narrow it — a -/// narrowed return is how a capability ends up implemented but never dispatched. There is -/// deliberately no IWritable: the MTConnect Agent surface is read-only. +/// narrowed return is how a capability ends up implemented but never dispatched. +/// is the standing example: it was added to +/// with no change whatsoever to this file, and would have been +/// silently undispatched had the return type ever been narrowed to . +/// +/// +/// Registering the factory does not enable writing. An MTConnect write is observation +/// injection through the Agent's AllowPut surface and is off at two independent gates +/// — the Agent's agent.cfg and the driver's own write.enabled (default +/// ). See 's remarks. /// /// public static class MTConnectDriverFactoryExtensions diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDiscoverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDiscoverTests.cs index ea6eef6a..0c6da662 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDiscoverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDiscoverTests.cs @@ -45,12 +45,16 @@ public sealed class MTConnectDiscoverTests "dev1_system_cond", ]; - private static MTConnectDriverOptions Opts(string? deviceName = null) => + private static MTConnectDriverOptions Opts(string? deviceName = null, bool writeEnabled = false) => new() { AgentUri = AgentUri, DeviceName = deviceName, + // Off unless a test says otherwise — the shipped default, and what every pre-write-back + // expectation in this file was written against. + Write = new MTConnectWriteOptions { Enabled = writeEnabled }, + // Discovery is a pure read of the /probe device model: it must enumerate what the Agent // declares, NOT what an operator has already authored. Left empty on purpose so a driver // that streamed its authored tag table instead could not pass. @@ -324,10 +328,10 @@ public sealed class MTConnectDiscoverTests } /// - /// MTConnect is a read-only protocol and this driver implements no IWritable, so every - /// browsed leaf is — the tier that is read-only - /// from OPC UA. Nothing is historized by discovery either: historization is authored per tag, - /// never inferred from a device model. + /// Write-back is off by default, so every browsed leaf is + /// — the tier that is read-only from OPC UA, and + /// exactly what this driver shipped. Nothing is historized by discovery either: historization + /// is authored per tag, never inferred from a device model. /// [Fact] public async Task Discover_streams_every_leaf_view_only_and_not_historized() @@ -341,6 +345,47 @@ public sealed class MTConnectDiscoverTests cap.Variables.ShouldAllBe(v => v.Attr.Source == NodeSourceKind.Driver); } + /// + /// With write.enabled on, the same probe model browses + /// — but only for the leaves WriteAsync would actually accept. A browse that advertised a + /// tier the write path refuses is the "silent no-op in a new costume" failure MQTT's + /// CanWrite pins against; the flag is the ONLY difference between this test and the one + /// above, so a driver that ignored it could not pass both. + /// + [Fact] + public async Task Discover_streams_writable_leaves_as_operate_when_write_is_enabled() + { + var (driver, _) = await InitializedDriverAsync(Opts(writeEnabled: true)); + + var cap = await DiscoverAsync(driver); + + var writable = cap.Variables.Where(v => !v.Attr.IsAlarm).ToList(); + + // Counted, not just ShouldAllBe: an empty sequence satisfies ShouldAllBe vacuously, so a + // discovery that streamed nothing at all would otherwise pass this test. + writable.Count.ShouldBe(FixtureDataItemIds.Length - 1); + writable.ShouldAllBe(v => v.Attr.SecurityClass == SecurityClassification.Operate); + } + + /// + /// A CONDITION data item stays in both + /// modes: a condition PUT carries alarm semantics and a different level|text wire + /// format, so the write path refuses it and the browse must say so. + /// + /// Whether the driver's write-back block is switched on. + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Discover_streams_a_condition_leaf_view_only_whether_or_not_write_is_enabled(bool writeEnabled) + { + var (driver, _) = await InitializedDriverAsync(Opts(writeEnabled: writeEnabled)); + + var cap = await DiscoverAsync(driver); + + cap.Variables.Single(v => v.Attr.FullName == "dev1_system_cond") + .Attr.SecurityClass.ShouldBe(SecurityClassification.ViewOnly); + } + // ---- browse names ---- /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs index 6184bc5f..0f7ee5c7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs @@ -207,19 +207,25 @@ public sealed class MTConnectFactoryTests } /// - /// The other half of the capability pin: MTConnect's Agent surface is read-only, so the - /// instance must NOT be . If it ever became writable, the write gate - /// would start offering an operation the protocol cannot honour. + /// The other half of the capability pin: the instance IS , so the + /// runtime's write gate dispatches to it. This is the seam the concrete return type exists for + /// — the capability was added to the driver with no change to the factory, and a narrowed + /// return would have left it implemented but never dispatched. /// + /// + /// Dispatchable is not enabled. The config below authors no write block, so this + /// instance refuses every write with BadNotWritable — and the Agent's own + /// AllowPut is a second, independent gate. See MTConnectWriteTests. + /// [Fact] - public void Registered_delegate_returns_an_instance_that_is_not_writable() + public void Registered_delegate_returns_an_instance_that_dispatches_writes() { var registry = new DriverFactoryRegistry(); MTConnectDriverFactoryExtensions.Register(registry); IDriver driver = registry.TryGet("MTConnect")!("mt1", "{\"agentUri\":\"http://a:5000\"}"); - (driver is IWritable).ShouldBeFalse("the MTConnect Agent surface is read-only"); + (driver is IWritable).ShouldBeTrue("MTConnect must dispatch writes (the Agent AllowPut surface)"); } /// Registering into a null registry is a wiring bug, not a silent no-op. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs new file mode 100644 index 00000000..89b30f14 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs @@ -0,0 +1,356 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests; + +/// +/// 's leg — the batch contract, the per-slot +/// refusals, and the one thing the driver has to get right before the Agent is even dialled: which +/// device a DataItem belongs to. +/// +/// +/// +/// The write itself is the live gate's job. Everything below stops at the +/// seam and asserts what the driver asked the Agent +/// for (), never that a fake was called for its own sake — +/// the same philosophy as MqttDriverWriteTests. What IS worth pinning offline is +/// everything that decides whether a write should reach an Agent at all, because that is where +/// a silent no-op would hide. +/// +/// +/// A write is observation injection, not machine command (design §2.2): it sets what the +/// Agent reports for a DataItem, and the next adapter update overwrites it. That is why the +/// feature is off at two independent gates — the Agent's AllowPut, and this driver's +/// write.enabled — and why the disabled leg below is the first test in the file. +/// +/// +public sealed class MTConnectWriteTests +{ + private const string AgentUri = "http://fixture-agent:5000"; + + /// The device the canned Fixtures/probe.xml declares — id="dev1" name="VMC-3Axis". + private const string FixtureDeviceName = "VMC-3Axis"; + + private const string PositionDataItemId = "dev1_pos"; + private const string ConditionDataItemId = "dev1_system_cond"; + + private const string PositionRawPath = "Plant/MTConnect/dev1/Position"; + private const string ConditionRawPath = "Plant/MTConnect/dev1/SystemCondition"; + private const string UnknownRawPath = "Plant/MTConnect/dev1/NotATag"; + + private const uint Good = 0x00000000u; + private const uint BadNodeIdUnknown = 0x80340000u; + private const uint BadNotWritable = 0x803B0000u; + private const uint BadTypeMismatch = 0x80740000u; + private const uint BadCommunicationError = 0x80050000u; + private const uint BadNotConnected = 0x808A0000u; + + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + /// One authored raw tag bound to a DataItem id, as the browse-commit flow writes it. + private static RawTagEntry Entry(string rawPath, string dataItemId) => + new(rawPath, $$"""{"fullName":"{{dataItemId}}"}""", WriteIdempotent: false, DeviceName: "dev1"); + + private static MTConnectDriverOptions Opts( + bool writeEnabled = true, string? deviceName = null, params RawTagEntry[] rawTags) => + new() + { + AgentUri = AgentUri, + DeviceName = deviceName, + Tags = [], + RawTags = rawTags, + Write = new MTConnectWriteOptions { Enabled = writeEnabled }, + }; + + private static (MTConnectDriver Driver, CannedAgentClient Client) NewDriver( + MTConnectDriverOptions options, RecordingDriverLogger? logger = null) + { + var client = CannedAgentClient.FromFixtures(); + + return (new MTConnectDriver(options, "mt1", _ => client, logger), client); + } + + private static async Task<(MTConnectDriver Driver, CannedAgentClient Client)> InitializedAsync( + MTConnectDriverOptions options, RecordingDriverLogger? logger = null) + { + var (driver, client) = NewDriver(options, logger); + await driver.InitializeAsync("{}", Ct); + + return (driver, client); + } + + /// The ordinary rig: write on, one writable position tag and the fixture's CONDITION item. + private static Task<(MTConnectDriver Driver, CannedAgentClient Client)> WritableDriverAsync( + RecordingDriverLogger? logger = null) => + InitializedAsync( + Opts( + writeEnabled: true, + deviceName: null, + Entry(PositionRawPath, PositionDataItemId), + Entry(ConditionRawPath, ConditionDataItemId)), + logger); + + // ---- the capability itself ---- + + [Fact] + public void MTConnectDriver_implements_IWritable() + // The interface's presence IS the feature: without it the runtime's capability dispatch never + // offers this driver a write, whatever a node advertises. + => NewDriver(Opts()).Driver.ShouldBeAssignableTo(); + + // ---- the two independent gates ---- + + [Fact] + public async Task A_driver_with_write_disabled_refuses_every_slot_without_dialling_the_agent() + { + // The default. An existing deployment that never authors the block keeps today's read-only + // behaviour EXACTLY — including making no request an Agent could log or accept. + var (driver, client) = await InitializedAsync( + Opts(writeEnabled: false, deviceName: null, Entry(PositionRawPath, PositionDataItemId))); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results.Count.ShouldBe(1); + results[0].StatusCode.ShouldBe(BadNotWritable); + client.Writes.ShouldBeEmpty(); + } + + [Fact] + public async Task A_write_before_initialize_reports_not_connected() + { + // Matches ReadAsync: "this driver is not running" is the only true statement available, and it + // is NOT the same operator problem as an Agent that answered and refused. + var (driver, _) = NewDriver(Opts(true, null, Entry(PositionRawPath, PositionDataItemId))); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results[0].StatusCode.ShouldBe(BadNotConnected); + } + + [Fact] + public async Task A_write_after_shutdown_reports_not_connected() + { + var (driver, _) = await WritableDriverAsync(); + await driver.ShutdownAsync(Ct); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results[0].StatusCode.ShouldBe(BadNotConnected); + } + + // ---- per-slot refusals ---- + + [Fact] + public async Task An_unknown_reference_is_refused_in_its_own_slot() + { + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(UnknownRawPath, 1.0)], Ct); + + results[0].StatusCode.ShouldBe(BadNodeIdUnknown); + client.Writes.ShouldBeEmpty(); + } + + [Fact] + public async Task A_condition_data_item_is_refused_as_not_writable() + { + // CONDITION PUTs carry alarm semantics and a different level|text wire format — excluded from + // v1. BadNotWritable, not BadTypeMismatch: the value was never the problem. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(ConditionRawPath, "NORMAL")], Ct); + + results[0].StatusCode.ShouldBe(BadNotWritable); + client.Writes.ShouldBeEmpty(); + } + + [Fact] + public async Task A_null_value_is_refused_as_a_type_mismatch() + { + // A write IS a URL query value; there is no null to send. Refusing beats writing the empty + // string, which the Agent would accept as a real observation. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, null)], Ct); + + results[0].StatusCode.ShouldBe(BadTypeMismatch); + client.Writes.ShouldBeEmpty(); + } + + [Fact] + public async Task An_array_value_is_refused_as_a_type_mismatch() + { + // Convert.ToString would happily render "System.Double[]" and the Agent would happily store it: + // a silent corruption visible only on read-back. Array/TIME_SERIES writes are out of scope. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, new[] { 1.0, 2.0 })], Ct); + + results[0].StatusCode.ShouldBe(BadTypeMismatch); + client.Writes.ShouldBeEmpty(); + } + + // ---- what actually reaches the seam ---- + + [Fact] + public async Task An_accepted_write_reports_good_and_names_the_owning_device_and_data_item() + { + // The device NAME is resolved from the /probe model, not from config: the Agent's PUT surface + // is addressed per device, and a multi-device Agent must be addressed correctly with no extra + // authoring. The RawPath is translated to the DataItem id, exactly as the read path does. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results[0].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem() + .ShouldBe((FixtureDeviceName, PositionDataItemId, "42.5")); + } + + [Fact] + public async Task A_value_is_rendered_with_the_invariant_culture() + { + // A comma decimal separator would be accepted by the Agent and read back as a different number. + var (driver, client) = await WritableDriverAsync(); + + await driver.WriteAsync([new WriteRequest(PositionRawPath, 1234.5678d)], Ct); + + client.Writes.ShouldHaveSingleItem().Value.ShouldBe("1234.5678"); + } + + [Fact] + public async Task A_boolean_is_rendered_as_its_invariant_true_false_string() + { + // Boolean.ToString is culture-independent, so the invariant rendering is "True"/"False" — NOT + // "1"/"0". Documented because it is the value an MTConnect consumer will see verbatim. + var (driver, client) = await WritableDriverAsync(); + + await driver.WriteAsync([new WriteRequest(PositionRawPath, true)], Ct); + + client.Writes.ShouldHaveSingleItem().Value.ShouldBe("True"); + } + + // ---- device-name resolution ---- + + [Fact] + public async Task A_data_item_the_probe_does_not_declare_falls_back_to_the_configured_device() + { + // The dataItemId-keyed authoring surface (the driver CLI) and any tag whose DataItem the Agent + // has since dropped: config is the only device name available, and it is a real one. + var (driver, client) = await InitializedAsync( + Opts(writeEnabled: true, deviceName: "ConfiguredDevice", Entry(PositionRawPath, "not_in_probe"))); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 7)], Ct); + + results[0].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem().ShouldBe(("ConfiguredDevice", "not_in_probe", "7")); + } + + [Fact] + public async Task A_data_item_with_no_resolvable_device_is_refused_as_an_unknown_node() + { + // Neither the probe model nor config names a device, so there is no URL to POST to. Refused + // rather than guessed: a wrong device path is a 404 the operator reads as "the Agent is broken". + var (driver, client) = await InitializedAsync( + Opts(writeEnabled: true, deviceName: null, Entry(PositionRawPath, "not_in_probe"))); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 7)], Ct); + + results[0].StatusCode.ShouldBe(BadNodeIdUnknown); + client.Writes.ShouldBeEmpty(); + } + + // ---- how the Agent's own answers map ---- + + [Fact] + public async Task An_agent_refusal_maps_to_a_communication_error_and_logs_the_agents_own_text() + { + var logger = new RecordingDriverLogger(); + var (driver, client) = await WritableDriverAsync(logger); + client.WriteOutcome = new MTConnectWriteOutcome(Accepted: false, AgentError: "UNSUPPORTED: no such device"); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results[0].StatusCode.ShouldBe(BadCommunicationError); + logger.WarningsSnapshot().ShouldContain(w => w.Contains("UNSUPPORTED: no such device", StringComparison.Ordinal)); + } + + [Fact] + public async Task An_agent_refusal_never_reaches_the_health_LastError() + { + // Type-only discipline: LastError carries an exception TYPE, never Agent-authored text, and a + // refused write is not evidence the driver's read path is degraded either. + var (driver, client) = await WritableDriverAsync(); + var before = driver.GetHealth(); + client.WriteOutcome = new MTConnectWriteOutcome(Accepted: false, AgentError: "UNSUPPORTED: no such device"); + + await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + var after = driver.GetHealth(); + after.LastError.ShouldBe(before.LastError); + after.State.ShouldBe(before.State); + } + + [Fact] + public async Task A_transport_failure_maps_to_a_communication_error_without_throwing() + { + var (driver, client) = await WritableDriverAsync(); + client.WriteFailure = new HttpRequestException("connection refused"); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, 42.5)], Ct); + + results[0].StatusCode.ShouldBe(BadCommunicationError); + } + + // ---- batch contract ---- + + [Fact] + public async Task Cancellation_fills_the_remaining_slots_and_returns_the_batch() + { + // Never throws, even cancelled: the caller gets one status per request or the node-manager + // cannot tell which writes were attempted. + var (driver, client) = await WritableDriverAsync(); + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + var results = await driver.WriteAsync( + [new WriteRequest(PositionRawPath, 1.0), new WriteRequest(PositionRawPath, 2.0)], cts.Token); + + results.Count.ShouldBe(2); + results.ShouldAllBe(r => r.StatusCode == BadCommunicationError); + client.Writes.ShouldBeEmpty(); + } + + [Fact] + public async Task One_bad_request_does_not_fail_the_others_in_the_batch() + { + // Mirrors ReadAsync: failing the whole call would let one stale reference reject every other + // write in the same OPC UA request. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync( + [ + new WriteRequest(UnknownRawPath, 1.0), + new WriteRequest(ConditionRawPath, "NORMAL"), + new WriteRequest(PositionRawPath, 42.5), + ], + Ct); + + // Order-preserving, one status per request, each naming its own distinct cause. + results.Count.ShouldBe(3); + results[0].StatusCode.ShouldBe(BadNodeIdUnknown); + results[1].StatusCode.ShouldBe(BadNotWritable); + results[2].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem().DataItemId.ShouldBe(PositionDataItemId); + } + + [Fact] + public async Task An_empty_batch_is_answered_with_an_empty_result() + => (await NewDriver(Opts()).Driver.WriteAsync([], Ct)).ShouldBeEmpty(); + + [Fact] + public async Task WriteAsync_rejects_a_null_batch() + => await Should.ThrowAsync( + async () => await NewDriver(Opts()).Driver.WriteAsync(null!, Ct)); +}