Fix OPC UA address-space compliance in the device integration stack - #4117
Merged
Conversation
A live compliance audit of the PumpDeviceIntegrationServer against the OPC 40223 Pumps and OPC 10000-100 DI companion specifications found 13 address-space violations plus a serialization bug in the MCP tool. Every fix lands at its root cause in the shared stack rather than as a workaround in the sample. Source generator (tools/Opc.Ua.SourceGeneration.Core) - Preserve NodeSet2 AccessLevel bitmasks. The ModelDesign AccessLevel enumeration is not [Flags] and cannot represent combinations, so AccessLevel="5" (CurrentRead | HistoryRead) collapsed to Read. The importer now carries the verbatim bitmask on VariableDesign and code generation emits the named AccessLevels constants. UserAccessLevel mirrors AccessLevel, matching the runtime importer in UANodeSetHelpers. Server (src/Opc.Ua.Server) - Register fluent-created alarms with the owning node manager. AttachAlarm attached the alarm with AddChild but never indexed it, so it was invisible to Browse and NodeId lookup. The same gap existed in AddObject, CreateInstance and the state-machine creators. - Wire alarms as real event sources: set the HasCondition reference, initialise SourceNode/SourceName/ConditionName/InputNode, promote EventNotifier to include SubscribeToEvents up the ancestor chain, and publish HasNotifier from the Server object so clients subscribing on Server receive the events. ActivatesAlarm now genuinely raises and clears the condition and reports the event. - Stop advertising history that cannot be served. Startup now reconciles variables declaring Historizing or history access-level bits against the historian providers actually wired, clearing the advertisement and masking the attribute read callbacks when no provider resolves. Variables with a provider keep their NodeSet-declared history surface. - Publish NamespaceMetadata for every namespace. The new NamespaceMetadataPublisher walks the NamespaceArray and ensures each namespace has a NamespaceMetadataType object under Server/Namespaces, populating version and publication date from the ModelDependency attribute stamped on model assemblies. Companion-spec metadata objects never reached the address space because the NodeSet2 importer drops nodes parented to namespace 0, and neither route covered the server's own application namespace. Device integration (src/Opc.Ua.Di.Server) - Mint runtime-created instance NodeIds in the server's application namespace via DiNodeManager.InstanceNamespaceIndex instead of inheriting the parent's namespace. Companion specifications require that NodeIds of nodes they do not define must not use the standard namespaces. Vendor-defined functional groups and properties default their BrowseName namespace to the server namespace; spec-defined BrowseNames are unchanged. - Advertise DI 1.05 server facets. DiNodeManager implements IConformanceContributor and contributes facet URIs and conformance units computed from what is actually wired, merged into ServerProfileArray alongside StandardUA2017. - Fix TryAddToMachinesFolder, which used AddChild and therefore re-parented the device away from DeviceSet instead of adding the Organizes reference OPC 40001-1 requires. Pump sample (samples/PumpDeviceIntegrationServer) - Add --pumps N (CLI and environment, default 2) with every pump fully simulated using a per-pump phase offset. - Populate the mandatory ProductInstanceUri on every pump and the mandatory TrueState/FalseState on every supervision boolean. - Reference every pump from the Machinery Machines folder with Organizes. - Use BrowseNames without whitespace or '#', keeping the readable label in DisplayName. - Populate the Maintenance group and report BadWaitingForInitialData until the first simulation tick. - Wire UseHistorian so the history declared by the Pumps NodeSet works. MCP tool (tools/Opc.Ua.Mcp) - Fix VariantToObject serialising boolean false and numeric zero as JSON null. Variant.Null is default, so Variant.Equals adopted the non-null type and compared against the default value. It now dispatches on BuiltInType and uses typed TryGetValue accessors instead of AsBoxedObject, which the repository guidelines prohibit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses OPC UA address-space compliance gaps found in a live audit of samples/PumpDeviceIntegrationServer (DI + Pumps), by fixing the underlying shared stack behavior (node registration, event notifier wiring, namespace metadata publication, history advertisement reconciliation), and by correcting MCP Variant JSON serialization so default scalar values like false/0 are not misreported as null.
Changes:
- Improve stack-level address space correctness (namespace metadata publication, root notifier references, historical advertisement reconciliation, fluent node/alarm registration).
- Adjust DI hosting semantics (server-owned instance namespace, conformance facet contribution, Machines folder reference behavior).
- Fix MCP Variant JSON serialization (typed dispatch via
BuiltInType+TryGetValue) and add/extend regression tests and documentation.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Opc.Ua.SourceGeneration.Core/Schema/NodeSetToModelDesign.cs | Preserve raw AccessLevel bitmask from NodeSet2 import. |
| tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignExtensions.cs | Emit AccessLevels flags code from raw bitmask; mirror UserAccessLevel behavior. |
| tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesign.cs | Add VariableDesign.RawAccessLevel and include it in equality/hash. |
| tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs | Use variable-level access-level code mapping for generation. |
| tools/Opc.Ua.Mcp/Serialization/OpcUaJsonHelper.cs | Fix Variant JSON serialization for scalar/array/matrix values using typed TryGetValue. |
| tools/Opc.Ua.Mcp/README.md | Document Variant default handling (false/0 not treated as null). |
| tests/Opc.Ua.Tools.Tests/Mcp/OpcUaJsonHelperTests.cs | Add regression coverage for Variant serialization edge cases. |
| tests/Opc.Ua.Server.Tests/Historian/HistoricalAccessAdvertisementTests.cs | Add tests for startup reconciliation of historizing/access-level advertisement. |
| tests/Opc.Ua.Server.Tests/Fluent/FluentAlarmRegistrationIntegrationTests.cs | Add integration tests for fluent alarm registration + event notifier wiring. |
| tests/Opc.Ua.Di.Tests/TopologyElementBuilderTests.cs | Update tests for instance namespace/index and BrowseName changes. |
| tests/Opc.Ua.Di.Tests/PumpTypeInstanceTests.cs | Update pump instance expectations for new instance NodeIds/namespace. |
| tests/Opc.Ua.Di.Tests/PumpInstanceNodeIdRegressionTests.cs | Update NodeId minting expectations and model-surface assertions. |
| tests/Opc.Ua.Di.Tests/PumpHostedReferenceTests.cs | Align hosted reference tests to new pump BrowseNames and creation flow. |
| tests/Opc.Ua.Di.Tests/PumpAddressSpaceComplianceTests.cs | Add service-surface compliance regression tests for the pump server. |
| tests/Opc.Ua.Di.Tests/Opc.Ua.Di.Tests.csproj | Exclude new compliance test from net472 build. |
| tests/Opc.Ua.Di.Tests/DiConformanceFacetTests.cs | Add tests for DI conformance facet/profile advertisement. |
| tests/Opc.Ua.Di.Tests/DeviceBuilderTests.cs | Update expectations to use instance namespace index. |
| src/Opc.Ua.Server/Server/StandardServer.cs | Publish NamespaceMetadata for all namespaces during startup. |
| src/Opc.Ua.Server/Server/ServerInternalData.cs | Publish root notifier references after node managers start. |
| src/Opc.Ua.Server/Server/NamespaceMetadataPublisher.cs | New publisher to ensure Server/Namespaces is fully described. |
| src/Opc.Ua.Server/NodeManagerLog.cs | Add log message for cleared historical advertisement. |
| src/Opc.Ua.Server/NodeManager/MasterNodeManager.cs | Reconcile historical advertisement at startup. |
| src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs | Add historical advertisement reconciliation + root notifier reference publication. |
| src/Opc.Ua.Server/Fluent/SupervisionBuilderExtensions.cs | Improve alarm state transitions and ensure event reporting. |
| src/Opc.Ua.Server/Fluent/StateMachineBuilderExtensions.cs | Ensure created state machines use correct reference type and are registered. |
| src/Opc.Ua.Server/Fluent/ReferenceBuilderExtensions.cs | Register newly created objects and set ReferenceTypeId. |
| src/Opc.Ua.Server/Fluent/InstanceCreationBuilderExtensions.cs | Register created instances and set ReferenceTypeId. |
| src/Opc.Ua.Server/Fluent/FluentNodeRegistration.cs | Add helper to promote event sources and register root notifiers. |
| src/Opc.Ua.Server/Fluent/AlarmBuilderExtensions.cs | Register alarms, set HasCondition, initialize source fields, promote event source. |
| src/Opc.Ua.Server/EventIds.cs | Add event id range for NamespaceMetadataPublisher logs. |
| src/Opc.Ua.Di.Server/DiNodeManager.cs | Introduce InstanceNamespaceIndex, conformance contribution, and correct NodeId minting. |
| src/Opc.Ua.Di.Server/Builders/TopologyElementBuilderExtensions.cs | Use instance namespace for diagnostics functional group BrowseName. |
| src/Opc.Ua.Di.Server/Builders/SoftwareUpdateFacetWiring.cs | Remove sync-over-async; record software update conformance facet. |
| samples/PumpDeviceIntegrationServer/README.md | Update sample usage and naming (Pump_1, --pumps N, etc.). |
| samples/PumpDeviceIntegrationServer/PumpNodeManager.cs | Add options-driven pump count + expose created pump NodeIds. |
| samples/PumpDeviceIntegrationServer/PumpNodeManager.Configure.cs | Wire historian + per-pump simulation and status semantics. |
| samples/PumpDeviceIntegrationServer/Program.cs | Add --pumps configuration and configure services accordingly. |
| docs/SourceGeneratedNodeManagers.md | Document new registration behavior and access-level bitmask handling. |
| docs/ServerAddressSpaceMetadata.md | New doc for namespace metadata publication + history reconciliation. |
| docs/README.md | Link new server address-space metadata doc. |
| docs/MigrationGuide.md | Document DI instance NodeId namespace change + history reconciliation behavior. |
| docs/McpServer.md | Document corrected Variant serialization behavior. |
| docs/HistoricalAccess.md | Document startup advertisement reconciliation behavior. |
| docs/DeviceIntegration.md | Update guidance/examples for instance namespace and conformance facets. |
| docs/DeveloperGuide.md | Link server address-space metadata doc from developer guide. |
Upstream #4116 ("Add simulation to PumpDeviceIntegrationServer and Event modelling") independently reworked the same pump simulation and the same fluent alarm/supervision builders this branch changes, so the merge is a union of both intents rather than a fast-forward. Resolutions: - src/Opc.Ua.Server/Fluent/AlarmBuilderExtensions.cs Kept both sides. Upstream contributes the parent-must-be-an-Object guard, SetEnableState, and the HasEventSource references. This branch contributes the HasCondition reference, SourceNode/SourceName/ ConditionName/InputNode initialisation, registration of the alarm with the node manager so it is browsable, notifier promotion up the whole ancestor chain, and root-notifier registration. - src/Opc.Ua.Server/Fluent/SupervisionBuilderExtensions.cs Took upstream's SetAlarmActive. It is more spec-correct than this branch's version: it honours EnabledState, resets Acked/Confirmed on activation, and computes Retain per OPC 10000-9 so an unacknowledged Condition stays retained after going inactive. Kept this branch's null guard so a malformed alarm cannot dereference a missing EnabledState. - samples/PumpDeviceIntegrationServer/PumpNodeManager.Configure.cs Took upstream's push-based simulation (RegisterPumpSimulation, CreatePumpSimulation, IValueUpdater<T>, Initialize/Advance/Publish, deterministic per-pump phase offsets) as the structure, then folded in this branch's compliance work: the configurable pump count, the Maintenance group, TrueState/FalseState on the supervision booleans, historian wiring, per-pump mandatory Identification values, and BadWaitingForInitialData until the first tick publishes a value. - samples/PumpDeviceIntegrationServer/README.md Union of both, keeping upstream's address-space validation workflow section and this branch's naming and --pumps documentation. Test adjustments: - PumpHostedReferenceTests now resolves pump NodeIds from the browse result instead of hard-coding the DI namespace, and waits for the first published value, because this branch moved instance NodeIds to the server namespace and made values start as BadWaitingForInitialData. Added PumpCreatedAfterStartupJoinsTheLiveSimulationAsync to restore the upstream coverage that a pump created after startup through ConfigureDevicesFor joins the live simulation. - FluentAlarmRegistrationIntegrationTests no longer asserts that Retain clears when an alarm goes inactive. That assertion pinned this branch's weaker semantics; upstream's Part 9 behaviour keeps Retain set while the Condition is unacknowledged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
- AsyncCustomNodeManager: stop locking the shared ServerObjectState.
The synchronous root-notifier path wrote the forward HasNotifier
reference straight into Server.ServerObject under lock(serverObject).
Locking an externally reachable node risks lock-ordering problems, and
the write also bypassed the node manager that owns the Server Object.
The block is removed: PublishRootNotifierReferenceAsync already
publishes that edge through IMasterNodeManager.AddReferencesAsync, and
ServerInternalData runs it for every node manager during startup, so
behaviour is unchanged. The inverse edge on the event source is still
written directly, matching the legacy CustomNodeManager contract.
- Opc.Ua.Mcp: preserve element types when serialising arrays.
ArrayToList funnelled every element through item.ToString(), so numeric
and boolean arrays became arrays of JSON strings ("0" instead of 0)
even though scalar Variant serialisation now preserves types. Element
conversion is factored into ElementToObject, which mirrors the scalar
conventions exactly (numbers, booleans, base64 ByteString, round-trip
DateTime, LocalizedText text, symbolic StatusCode, nested Variant and
DataValue).
- PumpNodeManager: report the offending option in ArgumentOutOfRangeException.
Both guards passed nameof(options) as paramName, which does not tell the
caller which value was rejected. They now report
options.PumpCount / options.SimulationInterval.
Tests:
- OpcUaJsonHelperTests no longer expects stringified array elements, and
gains coverage for boolean, double and string arrays.
- FluentAlarmRegistrationIntegrationTests now asserts the forward
HasNotifier edge through the publication contract: the harness master
node manager applies references the way the real owner would, and the
test publishes before asserting. This pins the cross-manager contract
rather than a direct mutation of another manager's node.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
marcschier
commented
Jul 30, 2026
- docs: consolidate the four node-manager documents into NodeManagers.md. The new document opens with a table of contents, then an overview of what node managers are and why they matter, the built-in managers including MasterNodeManager and how it routes by namespace and merges cross-manager references, then core vs custom, registration, server address-space metadata, and finally source generation. CoreNodeManagerVsCustomNodeManager2.md, NodeManagerRegistration.md, ServerAddressSpaceMetadata.md and SourceGeneratedNodeManagers.md are removed and every inbound link across docs, samples and tools now points at the corresponding section anchor. - Opc.Ua.Server: add HistorianBuilder.UseInMemoryProvider so historian configuration reads as one fluent chain. UseInMemory deliberately returns the provider so callers can register more variables later, which means it cannot sit in the middle of a chain; the new overload keeps the provider owned by the builder and reachable through Provider. Constructing the provider at the call site instead would transfer ownership across the boundary and trips CA2000. - PumpDeviceIntegrationServer: use that chain for the historian setup. - Opc.Ua.Di.Server: move the conformance-unit names and server-profile URIs into ConformanceUnit.cs as internal static ConformanceUnits and ServerProfiles classes, so the advertised set can be reviewed against the OPC Foundation profile database in one place. DiNodeManager references them through using aliases because IConformanceContributor already defines ConformanceUnits and ServerProfiles members on the same type. The expected values in DiConformanceFacetTests are intentionally left as independent literals: asserting against the same constants the production code uses would let a typo pass unnoticed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
marcschier
commented
Jul 30, 2026
Three logging calls evaluated arguments that are not free before handing them to the logger, so the work happened even when the message would be dropped. - NodeCacheResolver.LoadDataTypesAsync computed the elapsed duration for an Information-level message. Guarded with ILogger.IsEnabled. - ClassicSubscriptionEngine validated the publish time of every incoming notification and converted it with ToLocalTime for two Trace-level messages. The whole diagnostics block is now guarded with ILogger.IsEnabled, which also skips the timestamp arithmetic that ran for every publish response in debug builds, not just the conversions the analyzer flagged. Both blocks are inside #if DEBUG, which does not satisfy the analyzer because debug builds are analysed too. ILogger.IsEnabled is the idiom already used elsewhere in this assembly, including a few lines above the subscription block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
docs/NodeManagers.md - Remove the numbering from the "core vs custom" headings and their table of contents entries, and rename them to sentence case for consistency with the rest of the document. - Fold the "Where NodeManagers come from" heading into the body of the "Registering node managers" section as a lead-in paragraph for the table it introduced. - Rename "What happens to Clients" to "Node manager lifecycle impact on clients". - Rename "Requirements on a reloadable NodeManager" to "How to make a node manager reloadable" and expand it from a description of the constraint into actual guidance: the INodeManagerReloadParticipant contract, what PrepareReloadAsync has to do with retained inbound references and with the counterparts the replacement can no longer satisfy, the shape of LocalReference, a worked implementation, and practical notes on tracking cross-manager references, rejecting an incompatible replacement, and relying on the transactional rollback. - Fix a table-of-contents anchor that dropped the generic parameter of the typed Publish<TEvent> heading and therefore did not resolve. docs/MigrationGuide.md - Remove the section on DI runtime instance NodeIds. Device integration hosting is new in 2.0, so there is nothing to migrate from 1.5.378; only unreleased 2.0 previews were affected, which this guide does not serve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
cristipogacean
approved these changes
Jul 30, 2026
Brings in #4114 (complete client subscriptions by identity instead of id) and #4115 (harden subscription retirement and refresh the client namespace table on model change). One conflict, of the modify/delete kind: #4115 added a paragraph to docs/NodeManagerRegistration.md, which this branch deleted when the four node-manager documents were consolidated into docs/NodeManagers.md. Resolved by keeping the deletion and carrying the upstream content over: - The new paragraph about a Client with model change tracking re-reading its namespace table is added to the Namespaces subsection of docs/NodeManagers.md, which is where that content now lives. - docs/ModelChangeTracking.md, also touched by #4115, linked to NodeManagerRegistration.md#namespaces. Repointed at NodeManagers.md#namespaces so the new cross-reference resolves. No code conflicts: the upstream changes are confined to the client subscription and model-change stack, which this branch does not touch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
Both blocks are now guarded by ILogger.IsEnabled, so the conditional compilation no longer buys anything: with the guard in place the work is already skipped unless the message would be emitted, and keeping the directive only meant release builds could never produce the diagnostics even when the caller had raised the log level. - ClassicSubscriptionEngine: the publish-time validation and its two Trace messages now compile into every configuration, still behind IsEnabled(LogLevel.Trace). - NodeCacheResolver: the LoadDataTypes duration message likewise, behind IsEnabled(LogLevel.Information). Its paired #if DEBUG around the startTimestamp declaration is removed as well, since the timestamp is now read unconditionally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
Resolves the pump sample against master's multi-pump rework (#4117): - ConfigureInstancesAsync keeps master's loop over PumpCount and adds the OpenUSD facility before it; the twin follows the first pump. - MaterialisePumpInstanceAsync keeps both the OpenUSD signal registration and master's machines-folder and pump-state bookkeeping. - The simulation tick is master's configurable SimulationInterval rather than a private constant, and the shaft integration derives its step from it, so the twin stays correct when the interval is changed. - The pump NodeId baseline takes the union of both sides' entries. Also routes the OpenUSD node id assignment through RequireNodeIdFactory, since ISystemContext.NodeIdFactory became nullable in the core stack change at the bottom of the stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485
marcschier
added a commit
to marcschier/UA-.NETStandard
that referenced
this pull request
Jul 31, 2026
Reconcile the PumpX-2000 datasheet work with the address-space compliance changes from OPCFoundation#4117, which reshaped the same sample. Conflict resolutions: - WithIdentification: keep the upstream per-pump signature (builder, pump, pumpNumber) and NodeId-based lookup, and apply the full 21-field datasheet nameplate through it. Unit-specific fields (serial, asset id, component name, location, fabrication number) are now derived from the pump number, so every instance materialised by --pumps N gets a consistent nameplate instead of only the first two. - Program.cs: take upstream. Identification is configured centrally in WithIdentification for every pump, so the per-pump nameplate block in the ConfigureDevicesFor delegate is superseded; the delegate keeps upstream's loop that adds the Diagnostics functional group. - PumpSimulationState.Publish: keep upstream's status code and source timestamp plumbing and publish the datasheet curve values through it. - Supervision booleans: keep upstream's WireBoolean helper (TrueState / FalseState text and history) and drop the alarm's MonitorVariable on the boolean, because the alarm now sources BearingTemperature, which is what its Kelvin trip points describe. - Node-surface baseline: union of both identification lists. - docs/SourceGeneratedNodeManagers.md was consolidated upstream into docs/NodeManagers.md; the datasheet-related snippet fixes were ported there and the old file deleted. Documentation and tests follow the upstream rename of the pump BrowseNames to Pump_1 / Pump_2 (DisplayName "Pump #1" / "Pump #2"). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe608993-f3c9-4779-a6b9-9a9eabfc24ba
This was referenced Jul 31, 2026
marcschier
added a commit
that referenced
this pull request
Aug 4, 2026
# Description The `PumpDeviceIntegrationServer` sample simulates OPC 40223 pumps but had no product documentation, a thin nameplate, and a duty point that was not physically self-consistent. This PR gives the sample an **official-style product datasheet** for its simulated asset and aligns the server to it, so the document and the running address space cannot drift apart. ### New: `samples/PumpDeviceIntegrationServer/DATASHEET.md` A vendor-style datasheet for the fictitious *SimPump Corp PumpX-2000* (clearly marked as a simulated device): - Nameplate table mapped to OPC UA browse paths **and namespaces** (DI / Machinery / Pumps) - Best-efficiency duty point, characteristic curves and a performance table - Operating limits, motor / materials / dimensions data - Instrument list (tag → browse path → unit → `EURange`) and a Mermaid process schematic - Alarm trip points and the deterministic simulation profile ### Server aligned to the datasheet - `PumpDatasheet.cs` — the datasheet expressed as compile-time constants. - `MaterialiseNameplate` — materialises the 19 optional identification properties through the generator-emitted `AddXxx(context)` helpers, so each carries the browse name, namespace and DataType declared by the model (21 nameplate fields in total per pump). - `WithIdentification` now assigns the full datasheet nameplate to **every** simulated unit. Product-level fields are shared; unit-level fields (`SerialNumber`, `AssetId`, `ComponentName`, `Location`, `FabricationNumber`) are derived from the pump number, so `--pumps N` yields a consistent nameplate for each instance. - **Simulation rebuilt around the datasheet curves.** Volumetric flow is the only independent variable; differential pressure, mass flow, efficiency and shaft power are derived from `H(Q) = 32 − 0.0104·Q²`, `η(Q) = 72·(1 − 0.6·((Q−25)/25)²)` and `P = ρ·g·Q·H/η`, so `P = Δp·Q/η` holds at every tick. Previously every measurement was an independent sine and the sample published 5 kW of shaft power for 0.05 kg/s at 2 bar Δp. - `EURange` values now come from the datasheet; `OverTempAlarm` reports the `BearingTemperature` chain (the correct `SourceNode` for an alarm whose limits are in Kelvin) rather than a boolean; `Cavitation` and `MotorOverheat` are derived from suction level and bearing temperature with hysteresis instead of `tick % n`. ### Diagrams The sample README gains a simulated-device summary, a link to the datasheet, and three Mermaid diagrams: **address space**, **startup / hosting sequence**, and **simulation + alarm dataflow**. ### Drift protection `PumpDatasheetConformanceTests` pins the nameplate, engineering ranges, trip points, published-value envelope and hydraulic consistency against the datasheet, and the exact pump node-surface baseline in `PumpInstanceNodeIdRegressionTests` covers the new identification properties. No changes to `src/` — this is sample, test and documentation only. ### Rebased on #4117 master has been merged into this branch. #4117 reshaped the same sample (loop-based pump creation via `--pumps N`, `Pump_1` / `Pump_2` BrowseNames with `Pump #n` DisplayNames, historian wiring, `WireBoolean`, per-pump `Diagnostics` group). Both intents are preserved: - the datasheet nameplate is applied through upstream's per-pump `WithIdentification(builder, pump, pumpNumber)` hook, so it scales to `--pumps N`; - `Program.cs` is left exactly as upstream has it — identification is configured centrally, so the old per-pump block there is gone; - the datasheet curve values are published through upstream's status-code / source-timestamp plumbing, and upstream's `WireBoolean` (TrueState/FalseState text + history) is kept; - the node-surface baseline is the union of both identification lists; - `docs/SourceGeneratedNodeManagers.md` was consolidated upstream into `docs/NodeManagers.md`, so the snippet fixes were ported there. ## Related Issues _No tracking issue; this is a documentation and sample-quality improvement to the pump reference sample._ ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Verification (post-merge) - `dotnet build samples/PumpDeviceIntegrationServer` (net10.0): 0 warnings, 0 errors - `dotnet build tests/Opc.Ua.Di.Tests` (net10.0): 0 warnings, 0 errors - `dotnet test tests/Opc.Ua.Di.Tests -f net10.0`: **331/331 passed** (includes upstream's new `PumpAddressSpaceComplianceTests` and `DiConformanceFacetTests`) - `dotnet test tests/Opc.Ua.Di.Tests -f net48 --filter Pump`: **44/44 passed** - Live client read against the merged sample: Δp 214 079 Pa, ṁ 8.654 kg/s, η 69.33 %, P 2 677.4 W — `Δp·(ṁ/ρ)/η` matches the published shaft power; `Pump_1` reads SN-001 / PMP-1001 / Feed Pump A / Bay 3 and `Pump_2` reads SN-002 / PMP-1002 / Feed Pump B / Bay 4, exactly as documented. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe608993-f3c9-4779-a6b9-9a9eabfc24ba
marcschier
added a commit
that referenced
this pull request
Aug 6, 2026
…sition Two conflicts, plus one silent regression the merge would otherwise have introduced. UA.slnx - both sides appended sample projects at the same point. Kept both: the WotCon trio from upstream and GeneratorServer / SiteCompositionServer from this branch. samples/PumpDeviceIntegrationServer/OpenUsdComposition.cs - upstream (#4138) deleted the ProductionLine's OpenUsdRepresentation outright, and this branch had edited a line inside that same block. Took upstream's deletion. The block existed at the merge base and upstream removed all ten of its usages deliberately, with the reason stated in the code: the aggregation is an address-space demo only, and rendering a line pump - "a static topology entry, not a machine anyone is driving" - put phantom pumps in the twin that no client could account for. This branch's only change there was HasComponent -> HasAddIn, which is moot once the block is gone. The regression: upstream's new MaterialisePlantAggregationAsync adds a plant-level representation, and it arrived mounted with HasComponent. Git merged it cleanly because neither side touched those lines, so the merge would have silently reinstated exactly the spec violation this branch had just fixed everywhere else. Applied HasAddIn to it. That also exposed a hole in the regression test this branch added: it checked the Pump #1 representation alone, so it would have waved the new plant representation straight through. Broadened it to check every discovered representation, matching the robot suite. Verified it now fails when plantRep is reverted to HasComponent, and that the old narrow form did not. Full UA.slnx build succeeds. 870 OpenUSD and 418 DI tests green on net10.0. The 2 remaining CA1861 warnings are pre-existing on master from #4117 and are untouched by this branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe608993-f3c9-4779-a6b9-9a9eabfc24ba
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A live OPC UA compliance audit of
samples/PumpDeviceIntegrationServeragainst the OPC 40223 Pumps and OPC 10000-100 (DI) companion specifications found 13 address-space violations plus a serialization bug in the MCP tool. Every fix lands at its root cause in the shared stack rather than as a workaround in the sample, so all servers benefit.The sample now exposes N fully simulated pumps (
--pumps N) and advertises the DI conformance facets it actually satisfies.Findings and fixes
OverTempAlarmcreated by the fluent API did not exist in the address spaceAttachAlarmattached the alarm withAddChildbut never indexed it into the node managerAddObject,CreateInstanceand the state-machine creatorsEventNotifier = 0everywhere, noHasNotifierfromServerHasCondition, initialiseSourceNode/SourceName/ConditionName/InputNode, promoteEventNotifierup the ancestor chain, publishHasNotifierfrom theServerobjectns=4;s=5001_Pump #1)DiNodeManager.Newinherited the parent's namespace indexDiNodeManager.InstanceNamespaceIndex; instances move to the server's application namespaceServer/Namespacesdescribed only 2 of 6 namespacesNamespaceMetadataPublisherwalks theNamespaceArrayand fills version/publication date fromModelDependencyAttributeDiagnostics,LastError, …) sat in the DI namespaceAccessLevel="5"from the Pumps NodeSet was reported as1AccessLevelenum is not[Flags]and cannot representCurrentRead | HistoryReadAccessLevelsconstantsHistorizing = truewhileHistoryReadreturnedBadHistoryOperationUnsupportedProductInstanceUriunsetTrueState/FalseStateempty on every supervision booleanMachinesfolder empty (OPC 40001-1 §9.2 says shall)TryAddToMachinesFolderusedAddChild, which re-parents the device away fromDeviceSetOrganizesreference instead#Pump_1with the readable label inDisplayNameMaintenancegroup materialised but emptyGoodBadWaitingForInitialDatauntil the first simulation tickServerProfileArrayadvertised onlyStandardUA2017DiNodeManagerdid not implement the existingIConformanceContributorStandardUA2017falseand numeric0as JSONnullVariant.Nullisdefault, soVariant.Equalsadopts the non-null type and compares against the default valueBuiltInTypeand use typedTryGetValueaccessors instead of the prohibitedAsBoxedObjectRelationship to #4116
#4116 landed on
masterwhile this branch was in flight and targeted overlapping ground. The merge is a union of both intents, not a fast-forward:AlarmBuilderExtensions— kept both sides. Add simulation to PumpDeviceIntegrationServer and Event modelling #4116 contributes the parent-must-be-an-Object guard,SetEnableState, and theHasEventSourcereferences; this branch contributes theHasConditionreference, condition source initialisation, registration of the alarm with the node manager so it is browsable, notifier promotion up the whole ancestor chain, and root-notifier registration.SupervisionBuilderExtensions— took Add simulation to PumpDeviceIntegrationServer and Event modelling #4116'sSetAlarmActive. It is more spec-correct than this branch's version: it honoursEnabledState, resets Acked/Confirmed on activation, and computesRetainper OPC 10000-9 so an unacknowledged Condition stays retained after going inactive. This branch's null guard was kept so a malformed alarm cannot dereference a missingEnabledState.PumpNodeManager.Configure.cs— took Add simulation to PumpDeviceIntegrationServer and Event modelling #4116's push-based simulation (RegisterPumpSimulation,CreatePumpSimulation,IValueUpdater<T>,Initialize/Advance/Publish, deterministic per-pump phase offsets) as the structure, then folded in this branch's compliance work: configurable pump count, Maintenance group,TrueState/FalseState, historian wiring, per-pump mandatory Identification values, andBadWaitingForInitialDatauntil the first tick.Two test adjustments were needed, both because the merge legitimately changed behaviour the tests pinned:
PumpHostedReferenceTestsnow resolves pump NodeIds from the browse result instead of hard-coding the DI namespace, and waits for the first published value.PumpCreatedAfterStartupJoinsTheLiveSimulationAsyncwas added to restore Add simulation to PumpDeviceIntegrationServer and Event modelling #4116's coverage that a pump created after startup throughConfigureDevicesForjoins the live simulation.FluentAlarmRegistrationIntegrationTestsno longer asserts thatRetainclears when an alarm goes inactive — that pinned this branch's weaker semantics, and Add simulation to PumpDeviceIntegrationServer and Event modelling #4116's Part 9 behaviour is correct.Breaking change
Runtime-created DI device instance NodeIds move from the companion-spec namespace to the server's application namespace, e.g.
ns=<DI>;s=5001_Pump #1becomesns=<application>;s=5001_Pump #1. This invalidates NodeIds cached by clients from earlier 2.0 previews. It is not a model change — standard DI/Machinery/Pumps type NodeIds and spec-defined BrowseNames are untouched.Device integration hosting is new in 2.0, so there is nothing to migrate from 1.5.378 and this is deliberately not in
docs/MigrationGuide.md. Anyone tracking 2.0 previews should rediscover devices by browsingDeviceSetrather than persisting NodeIds, and resolve namespace indexes fromNamespaceArrayper connection.Verification
Compliance is pinned by regression tests that assert through the server's service surface (
BrowseAsync/ReadAsync/HistoryReadAsync), not by inspectingNodeStateobjects in memory. That distinction mattered: three defects survived in-memory assertions and were only caught through the read path — most subtly,Historizingwas cleared whileAccessLevelstill advertisedHistoryRead, because attribute read callbacks re-applied the NodeSet bits.Post-merge on net10.0:
Opc.Ua.Di.TestsOpc.Ua.Server.TestsOpc.Ua.Tools.TestsOpc.Ua.History.TestsOpc.Ua.SourceGeneration.Core.TestsThe full solution was also built and tested on net48 before the merge.
Review feedback from
copilot-pull-request-reviewerhas been addressed in commitbadad31dc: the sharedServerObjectStatelock was removed (the forwardHasNotifieredge is published through the owning node manager instead), MCP array serialization now preserves element types, and the pump option guards report the specific rejected option. The only failure isConfigureApplicationBuildsSharedClientAndServerConfigurationAsync, a pre-existing certificate-store issue that passes when run in isolation and is unrelated to this change.Documentation
Updated
docs/DeviceIntegration.md,docs/MigrationGuide.md,docs/HistoricalAccess.md,docs/DeveloperGuide.md,docs/McpServer.md,docs/README.md,samples/PumpDeviceIntegrationServer/README.mdandtools/Opc.Ua.Mcp/README.md, and consolidated the four node-manager documents into a singledocs/NodeManagers.md— overview, built-in managers (includingMasterNodeManager), core vs custom, registration, server address-space metadata, and source generation — with a TOC and all inbound links repointed.Related Issues
No tracking issue exists for this work yet — it originated from an ad-hoc compliance audit of the pump sample. Given the size of the change and the runtime-breaking NodeId move, please open (or link) a tracking issue so the design is recorded as an ADR before merging.
Checklist
Put an
xin the boxes that apply. You can complete these step by step after opening the PR.