Add simulation to PumpDeviceIntegrationServer and Event modelling - #4116
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR strengthens the Pump Device Integration Server sample by making pump instances participate correctly in OPC UA eventing/alarm hierarchies and by unifying simulation wiring so dynamically created pumps are simulated and validated consistently.
Changes:
- Wire
EventNotifierflags +HasNotifier/HasEventSourcereferences for pump eventing and alarm sources, and emit alarm condition events viaReportEvent. - Refactor simulation to register each pump (including post-startup instances) into a shared tick loop with deterministic phase offsets.
- Expand tests to validate event hierarchy references, simulation value changes, and observable alarm events via subscriptions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Opc.Ua.Server.Tests/Fluent/SupervisionBuilderExtensionsTests.cs | Adds assertions for event reporting, retain/severity, and disabled-alarm behavior. |
| tests/Opc.Ua.Server.Tests/Fluent/AlarmBuilderExtensionsTests.cs | Verifies HasEventSource refs/notifier promotion and enforces parent type constraints. |
| tests/Opc.Ua.Di.Tests/PumpInstanceNodeIdRegressionTests.cs | Updates expectations for shared simulation surface and validates notifier/event-source hierarchy. |
| tests/Opc.Ua.Di.Tests/PumpHostedReferenceTests.cs | Expands integration scenario to verify browsing, simulation updates, and receiving events. |
| src/Opc.Ua.Server/Fluent/SupervisionBuilderExtensions.cs | Makes alarm transitions update event metadata and report condition events. |
| src/Opc.Ua.Server/Fluent/AlarmBuilderExtensions.cs | Restricts alarm parents to Objects; enables alarms by default; wires event-source references. |
| samples/PumpDeviceIntegrationServer/README.md | Documents unified simulation + event subscription behavior and address-space validation tooling. |
| samples/PumpDeviceIntegrationServer/PumpNodeManager.cs | Ensures newly created pumps are added as root notifiers and registered into simulation. |
| samples/PumpDeviceIntegrationServer/PumpNodeManager.Configure.cs | Refactors simulation into per-pump state with shared ticking and dynamic registration. |
koepalex
force-pushed
the
koepalex/pump-src-6
branch
from
July 29, 2026 21:58
490ab81 to
c43882b
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
koepalex
force-pushed
the
koepalex/pump-src-6
branch
from
July 29, 2026 22:33
c43882b to
ad5266c
Compare
Contributor
Author
|
Pipeline follow-up: Azure build 16067 failed an unrelated hosting coverage test (RegisterPostStartRegistriesWiresHistorianAndAliasStoresOnPlainServerAsync). The test waited for OnServerStarted, but RegisterPostStartRegistries() runs afterward, so it could observe the source-registry alias store before the direct alias-store registration completed. Commit ad5266c now waits for the historian and both alias stores to be visible before asserting. The failed test passed 10/10 repeated runs, the full hosted-service coverage fixture passed, and the pump-focused suites remain green. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4116 +/- ##
==========================================
- Coverage 80.18% 79.89% -0.30%
==========================================
Files 1514 1514
Lines 209245 209408 +163
Branches 36008 36035 +27
==========================================
- Hits 167789 167301 -488
- Misses 28878 29563 +685
+ Partials 12578 12544 -34
🚀 New features to boost your workflow:
|
marcschier
approved these changes
Jul 30, 2026
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
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
7 tasks
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
Resolves the PumpDeviceIntegrationServer simulation conflict. Both sides added a pump simulation: master (#4116) generalised it into a per-pump PumpSimulationState driven through IValueUpdater with phase profiles and multi-pump registration, while this branch turned the pump into an EN 733 twin that drives the OpenUSD viewport. Master's structure is kept because IValueUpdater.SetValue is the general mechanism for the defect this branch worked around with its own publish loop - a variable wired with only OnRead never raises a data change. The branch's twin specifics are re-expressed inside it: PumpSimulationState now integrates the shaft angle from the running flow and exposes ShaftAngleDegrees and AlarmActive, and the node manager publishes the OpenUSD signals that are created outside the fluent builder and so have no updater of their own. Also reconciles the pump NodeId baseline. The branch pinned six InputArguments/OutputArguments nodes on argument-less condition methods that the source generator emitted before the method declaration fix earlier on this branch; master's baseline never had them and the merged address space no longer produces them, so they are dropped. Every one of master's 80 entries and all of the branch's OpenUSD representation entries are preserved. JsonDecoder merged cleanly - the two sides changed different methods. Validated: PumpDeviceIntegrationServer builds with 0 warnings, Opc.Ua.Di.Tests 342 passed / 0 failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
…4117) # Description A live OPC UA compliance audit of `samples/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, so all servers benefit. The sample now exposes **N fully simulated pumps** (`--pumps N`) and advertises the DI conformance facets it actually satisfies. > This branch has been merged with `master` including **#4116** ("Add simulation to PumpDeviceIntegrationServer and Event modelling"), which independently reworked the same pump simulation and the same fluent alarm builders. See [Relationship to #4116](#relationship-to-4116) below for how the two were reconciled. ## Findings and fixes | # | Finding | Root cause | Fix | |---|---------|-----------|-----| | 1 | `OverTempAlarm` created by the fluent API did not exist in the address space | `AttachAlarm` attached the alarm with `AddChild` but never indexed it into the node manager | Register the node; the same gap existed in `AddObject`, `CreateInstance` and the state-machine creators | | 2 | Alarms were unsubscribable — `EventNotifier = 0` everywhere, no `HasNotifier` from `Server` | No event-source wiring at all | Set `HasCondition`, initialise `SourceNode`/`SourceName`/`ConditionName`/`InputNode`, promote `EventNotifier` up the ancestor chain, publish `HasNotifier` from the `Server` object | | 3 | Every instance NodeId was minted in the **DI standard namespace** (`ns=4;s=5001_Pump #1`) | `DiNodeManager.New` inherited the parent's namespace index | New `DiNodeManager.InstanceNamespaceIndex`; instances move to the server's application namespace | | 4 | `Server/Namespaces` described only 2 of 6 namespaces | The NodeSet2 importer drops nodes parented to namespace 0, so companion-spec metadata objects never reached the address space — and no route covered the server's own namespace | New `NamespaceMetadataPublisher` walks the `NamespaceArray` and fills version/publication date from `ModelDependencyAttribute` | | 5 | Vendor BrowseNames (`Diagnostics`, `LastError`, …) sat in the DI namespace | Builders defaulted to the parent's namespace | Default to the server namespace; **spec-defined BrowseNames are unchanged** | | 6 | `AccessLevel="5"` from the Pumps NodeSet was reported as `1` | The ModelDesign `AccessLevel` enum is not `[Flags]` and cannot represent `CurrentRead \| HistoryRead` | Carry the verbatim NodeSet2 bitmask and emit named `AccessLevels` constants | | 7 | `Historizing = true` while `HistoryRead` returned `BadHistoryOperationUnsupported` | Nothing reconciled the declared history surface with the wired historians | Startup clears the advertisement (including masking the attribute read callbacks) when no provider resolves; the sample wires a real historian | | 8 | Mandatory `ProductInstanceUri` unset | Sample omission | Populated on every pump | | 9 | Mandatory `TrueState`/`FalseState` empty on every supervision boolean | Absent in the official NodeSet; a server must still populate them | Populated on every pump | | 10 | Machinery `Machines` folder empty (OPC 40001-1 §9.2 says **shall**) | `TryAddToMachinesFolder` used `AddChild`, which *re-parents* the device away from `DeviceSet` | Add the `Organizes` reference instead | | 11 | BrowseName contained a space and `#` | Sample naming | `Pump_1` with the readable label in `DisplayName` | | 12 | `Maintenance` group materialised but empty | Sample omission | Populated | | 13 | Uninitialised values reported `Good` | No initial-value status | `BadWaitingForInitialData` until the first simulation tick | | — | `ServerProfileArray` advertised only `StandardUA2017` | `DiNodeManager` did not implement the existing `IConformanceContributor` | Contribute DI 1.05 server facets computed at runtime from what is actually wired, merged with `StandardUA2017` | | — | MCP tool serialised boolean `false` and numeric `0` as JSON `null` | `Variant.Null` is `default`, so `Variant.Equals` adopts the non-null type and compares against the default value | Dispatch on `BuiltInType` and use typed `TryGetValue` accessors instead of the prohibited `AsBoxedObject` | ## Relationship to #4116 #4116 landed on `master` while 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. #4116 contributes the parent-must-be-an-Object guard, `SetEnableState`, and the `HasEventSource` references; this branch contributes the `HasCondition` reference, 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 #4116'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. This branch's null guard was kept so a malformed alarm cannot dereference a missing `EnabledState`. - **`PumpNodeManager.Configure.cs`** — took #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, and `BadWaitingForInitialData` until the first tick. - **README** — union, keeping #4116's address-space validation workflow section. Two test adjustments were needed, both because the merge legitimately changed behaviour the tests pinned: - `PumpHostedReferenceTests` now resolves pump NodeIds from the browse result instead of hard-coding the DI namespace, and waits for the first published value. `PumpCreatedAfterStartupJoinsTheLiveSimulationAsync` was **added** to restore #4116's 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 pinned this branch's weaker semantics, and #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 #1` becomes `ns=<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 browsing `DeviceSet` rather than persisting NodeIds, and resolve namespace indexes from `NamespaceArray` per connection. ## Verification Compliance is pinned by regression tests that assert **through the server's service surface** (`BrowseAsync` / `ReadAsync` / `HistoryReadAsync`), not by inspecting `NodeState` objects in memory. That distinction mattered: three defects survived in-memory assertions and were only caught through the read path — most subtly, `Historizing` was cleared while `AccessLevel` still advertised `HistoryRead`, because attribute read callbacks re-applied the NodeSet bits. Post-merge on net10.0: | Suite | Result | |---|---| | `Opc.Ua.Di.Tests` | 326 passed | | `Opc.Ua.Server.Tests` | 3846 passed, 5 skipped | | `Opc.Ua.Tools.Tests` | 288 passed | | `Opc.Ua.History.Tests` | 506 passed, 27 skipped | | `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed, 8 skipped | The full solution was also built and tested on **net48** before the merge. Review feedback from `copilot-pull-request-reviewer` has been addressed in commit `badad31dc`: the shared `ServerObjectState` lock was removed (the forward `HasNotifier` edge 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 is `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, 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.md` and `tools/Opc.Ua.Mcp/README.md`, and consolidated the four node-manager documents into a single [`docs/NodeManagers.md`](docs/NodeManagers.md) — overview, built-in managers (including `MasterNodeManager`), 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. - Fixes # ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] 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. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
7 tasks
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.
This pull request introduces significant improvements to the Pump Device Integration Server sample, focusing on robust event/alarm integration, simulation enhancements, and comprehensive test coverage. The changes ensure that pump objects and their events are correctly wired for OPC UA eventing, improve alarm state management and reporting, and update documentation and tests to validate these behaviors.
Event and Alarm Integration Improvements:
EventNotifierflags and are linked viaHasNotifierreferences, ensuring they properly participate in the OPC UA event hierarchy. Each pump is also registered as a root notifier. [1] [2]HasEventSourcereferences andEventNotifierflags for both parent and alarm nodes. [1] [2]Alarm State and Reporting Enhancements:
Retainflag is managed according to OPC UA alarm state requirements.Simulation and API Changes:
CreatePumpAsyncnow registers new pump instances with the simulation, ensuring all pumps participate in the live simulation loop.Documentation Updates:
Test Coverage Expansion:
These changes ensure the sample is a robust reference for OPC UA eventing, simulation, and alarm integration, and that its correctness is validated both automatically and through enhanced documentation.# Description
Describe the changes here to communicate to the maintainers why they should accept this pull request. By default - this will become the Commit message after merging and thus define history.
Related Issues
Reference all GitHub issues this PR addresses. If there is no issue yet, open one and link it here.
If this is a relatively large or complex change, a design must have been discussed in the related tracking issue and signed off (which becomes the Architectural Decision Record (ADR)).
Checklist
Put an
xin the boxes that apply. You can complete these step by step after opening the PR.