fix(alarms): populate ConditionClassId/ConditionClassName on condition events (#475) #476

2026-07-17T00:50:20-04:00

dohertj2 commented

2026-07-17 00:50:20 -04:00

Fixes #475.

Stacked PR — base is fix/alarm-condition-source-fields (#474), not master. Must merge after #474.
#474 touches the same function; this branch builds on it and extends the wire-level test file it added.

The bug

MaterialiseAlarmCondition never assigned the mandatory Part 9 ConditionType classification fields, so every condition event — native and scripted — shipped them unset. Same mechanism as #473: Create() builds the mandatory children from the type's embedded definition but leaves .Value unset, and nothing downstream synthesises them (ReportEvent / InstanceStateSnapshot copy children verbatim).

Observed on the wire against the pre-fix server (probe on a live condition event via a real client subscription):

ConditionClassId   = i=0                        (NodeId.Null)
ConditionClassName = LocalizedText{ Text: null }  (renders empty)

An HMI bucketing alarms by condition class — the standard OPC UA categorization — dropped every OtOpcUa alarm into an unclassified bin.

The fix (2 lines)

alarm.ConditionClassId.Value   = ObjectTypeIds.BaseConditionClassType;
alarm.ConditionClassName.Value = new LocalizedText("BaseConditionClass");

Why `BaseConditionClassType`, deliberately not `ProcessConditionClassType`

BaseConditionClassType is Part 9's "this server does not model condition classes" value, and it is the honest report: we hold no classification at the materialise seam. It fixes the real defect — a null that breaks conformant clients — by giving them a valid, resolvable class NodeId, without asserting a classification we cannot back.

ProcessConditionClassType (the SDK AlarmConditionServer sample's choice) was rejected on purpose: it would be actively wrong for a Galaxy alarm whose upstream category is Safety or Diagnostics. That trades a detectable null for an undetectable lie, which is strictly worse.

Real per-alarm classification is a separate future feature, not built here. It needs the driver's AlarmCategory, which today lives only on the runtime AlarmEventArgs transition — not on the authored composition this deploy-time seam sees — and only GalaxyDriver populates it. Wiring it needs a category → ConditionClass mapping plus a way to carry the category to materialise. Until then the IAlarmSource.cs:79 doc comment claiming the category "maps to ConditionClassName downstream" describes an intent, not the implementation; the docs now say so explicitly.

Tests — TDD, both observed RED first

Every test was written and watched fail against the pre-fix server before the production change existed (no stash needed — the fix genuinely was not in the file yet).

Wire-level (NativeAlarmEventIdentityFieldDeliveryTests) — RED:

Shouldly.ShouldAssertException : fields[ConditionClassIdIndex].Value
    should be i=11163   but was i=0
    Additional Info: ConditionClassId must carry a resolvable condition class, not null

The event did arrive and the field was delivered — it just carried NodeId.Null. So this is a genuine value bug, not a select-clause artifact.

Node-level (NodeManagerAlarmSourceFieldsTests) — RED on both realms, native (Raw) and scripted (Uns), same should be i=11163 but was i=0.

Two deliberate test-design notes:

The #473 guard's select clause is untouched. It mirrors ScadaBridge's exact 6-field clause and its indices are load-bearing consts, so appending would silently shift them. The new test carries its own clause. The only shared-code change is making EventCollector's message index a ctor param, since the two clauses place Message differently.
ConditionClassId / ConditionClassName are declared on ConditionType, not BaseEventType, so they are selected against that type — selecting them off BaseEventType returns no value regardless of the fix. Documented for client authors.

Verification

dotnet build ZB.MOM.WW.OtOpcUa.slnx    -> 0 Errors
OpcUaServer.Tests                      -> Passed: 381, Skipped: 4, Total: 385   (379 + 2 new)
OpcUaServer.IntegrationTests           -> Passed:  12, Total: 12                (11 + 1 new)

Docs

docs/AlarmTracking.md — the "Condition event identity fields" section gains both fields in the table, the BaseConditionClassType-vs-Process rationale, and the ConditionType-not-BaseEventType select-clause gotcha.

Incidental finding: `Quality` — NOT the same bug, do not fold in

The #474 review flagged that Quality may be similarly unset. Investigated empirically; reporting only, deliberately not fixed here.

It is never assigned anywhere in the tree (confirmed by grep — the sole .Quality hit in src/ is an unrelated variable status code in VirtualTagHostActor). But it does not arrive null on the wire:

QUALITY value=[0x00000000] type=[StatusCode] isGood=[True]   <- default(StatusCode) == 0 == Good

default(StatusCode) is 0, which is StatusCodes.Good — so it is accidentally-valid rather than broken. That makes it a different and lesser defect than #473/#475: conformant clients handle it fine, nothing null-derefs.

The real (smaller) issue: it hardcodes Good for every condition regardless of the backing tag's actual quality — a condition whose source tag is Bad/Uncertain still reports Good. That is a semantic correctness gap needing the driver's quality plumbed to the materialise/write seam, not a 2-line default, so it does not belong in this PR. Worth a separate issue at lower priority.

(Also observed while probing, no action implied: ClientUserId arrives null pre-ack, which is defensible per Part 9 — it is set when a condition method is invoked.)

Fixes #475. > **Stacked PR — base is `fix/alarm-condition-source-fields` (#474), not master. Must merge after #474.** > #474 touches the same function; this branch builds on it and extends the wire-level test file it added. ## The bug `MaterialiseAlarmCondition` never assigned the mandatory Part 9 `ConditionType` classification fields, so **every** condition event — native and scripted — shipped them unset. Same mechanism as #473: `Create()` builds the mandatory children from the type's embedded definition but leaves `.Value` unset, and nothing downstream synthesises them (`ReportEvent` / `InstanceStateSnapshot` copy children verbatim). Observed **on the wire** against the pre-fix server (probe on a live condition event via a real client subscription): ``` ConditionClassId = i=0 (NodeId.Null) ConditionClassName = LocalizedText{ Text: null } (renders empty) ``` An HMI bucketing alarms by condition class — the standard OPC UA categorization — dropped every OtOpcUa alarm into an unclassified bin. ## The fix (2 lines) ```csharp alarm.ConditionClassId.Value = ObjectTypeIds.BaseConditionClassType; alarm.ConditionClassName.Value = new LocalizedText("BaseConditionClass"); ``` ### Why `BaseConditionClassType`, deliberately not `ProcessConditionClassType` `BaseConditionClassType` is Part 9's **"this server does not model condition classes"** value, and it is the honest report: we hold no classification at the materialise seam. It fixes the real defect — a null that breaks conformant clients — by giving them a valid, resolvable class NodeId, **without asserting a classification we cannot back**. `ProcessConditionClassType` (the SDK `AlarmConditionServer` sample's choice) was rejected on purpose: it would be *actively wrong* for a Galaxy alarm whose upstream category is Safety or Diagnostics. That trades a detectable null for an **undetectable lie**, which is strictly worse. **Real per-alarm classification is a separate future feature, not built here.** It needs the driver's `AlarmCategory`, which today lives only on the runtime `AlarmEventArgs` transition — not on the authored composition this *deploy-time* seam sees — and only `GalaxyDriver` populates it. Wiring it needs a category → ConditionClass mapping plus a way to carry the category to materialise. Until then the `IAlarmSource.cs:79` doc comment claiming the category "maps to `ConditionClassName` downstream" describes an intent, not the implementation; the docs now say so explicitly. ## Tests — TDD, both observed RED first Every test was written and **watched fail against the pre-fix server** before the production change existed (no stash needed — the fix genuinely was not in the file yet). **Wire-level** (`NativeAlarmEventIdentityFieldDeliveryTests`) — RED: ``` Shouldly.ShouldAssertException : fields[ConditionClassIdIndex].Value should be i=11163 but was i=0 Additional Info: ConditionClassId must carry a resolvable condition class, not null ``` The event *did* arrive and the field *was* delivered — it just carried `NodeId.Null`. So this is a genuine value bug, not a select-clause artifact. **Node-level** (`NodeManagerAlarmSourceFieldsTests`) — RED on both realms, native (Raw) and scripted (Uns), same `should be i=11163 but was i=0`. Two deliberate test-design notes: - **The #473 guard's select clause is untouched.** It mirrors ScadaBridge's exact 6-field clause and its indices are load-bearing consts, so appending would silently shift them. The new test carries **its own** clause. The only shared-code change is making `EventCollector`'s message index a ctor param, since the two clauses place `Message` differently. - `ConditionClassId` / `ConditionClassName` are declared on **`ConditionType`, not `BaseEventType`**, so they are selected against that type — selecting them off `BaseEventType` returns no value regardless of the fix. Documented for client authors. ## Verification ``` dotnet build ZB.MOM.WW.OtOpcUa.slnx -> 0 Errors OpcUaServer.Tests -> Passed: 381, Skipped: 4, Total: 385 (379 + 2 new) OpcUaServer.IntegrationTests -> Passed: 12, Total: 12 (11 + 1 new) ``` ## Docs `docs/AlarmTracking.md` — the "Condition event identity fields" section gains both fields in the table, the BaseConditionClassType-vs-Process rationale, and the `ConditionType`-not-`BaseEventType` select-clause gotcha. --- ## Incidental finding: `Quality` — NOT the same bug, do not fold in The #474 review flagged that `Quality` may be similarly unset. **Investigated empirically; reporting only, deliberately not fixed here.** It is *never assigned anywhere in the tree* (confirmed by grep — the sole `.Quality` hit in `src/` is an unrelated variable status code in `VirtualTagHostActor`). But it does **not** arrive null on the wire: ``` QUALITY value=[0x00000000] type=[StatusCode] isGood=[True] <- default(StatusCode) == 0 == Good ``` `default(StatusCode)` is `0`, which *is* `StatusCodes.Good` — so it is accidentally-valid rather than broken. That makes it a **different and lesser** defect than #473/#475: conformant clients handle it fine, nothing null-derefs. The real (smaller) issue: it hardcodes **Good for every condition regardless of the backing tag's actual quality** — a condition whose source tag is Bad/Uncertain still reports Good. That is a semantic correctness gap needing the driver's quality plumbed to the materialise/write seam, **not** a 2-line default, so it does not belong in this PR. Worth a separate issue at lower priority. (Also observed while probing, no action implied: `ClientUserId` arrives null pre-ack, which is defensible per Part 9 — it is set when a condition method is invoked.)

dohertj2 changed target branch from fix/alarm-condition-source-fields to master

2026-07-17 00:50:55 -04:00

dohertj2 added 1 commit 2026-07-17 00:50:55 -04:00

fix(alarms): populate ConditionClassId/ConditionClassName on conditions (#475 ) e08b6b0e69

MaterialiseAlarmCondition never assigned the mandatory Part 9 ConditionType
classification fields, so every condition event — native and scripted — shipped
ConditionClassId = NodeId.Null (i=0) and ConditionClassName = empty text. Same
mechanism as #473: Create() builds the mandatory children from the type's
embedded definition but leaves them unset, and nothing downstream synthesises
them (ReportEvent / InstanceStateSnapshot copy children verbatim). An HMI
bucketing alarms by condition class dropped every OtOpcUa alarm as unclassified.

Report BaseConditionClassType — Part 9's "no condition class modelled" value.
This is the honest report: we hold no classification at the materialise seam.
Deliberately NOT ProcessConditionClassType (the SDK sample's pick), which would
assert a classification we cannot back and would be actively wrong for a Galaxy
alarm whose upstream category is Safety/Diagnostics — trading a detectable null
for an undetectable lie. Real per-alarm classification needs the driver's
AlarmCategory carried to this deploy-time seam (it lives only on the runtime
AlarmEventArgs transition today) and is a separate feature.

Guards, both observed RED against the pre-fix server:
- NativeAlarmEventIdentityFieldDeliveryTests: wire-level, its own select clause
  (the #473 test's clause mirrors ScadaBridge's exactly and its indices are
  load-bearing, so it is left untouched). The class fields are declared on
  ConditionType, not BaseEventType, so they are selected against that type.
- NodeManagerAlarmSourceFieldsTests: node-level, native (Raw) + scripted (Uns).

Stacked on #473 (PR #474) — merge after it.

dohertj2 referenced this pull request

2026-07-17 00:54:11 -04:00

bug(alarms): condition Quality is hardcoded Good regardless of the source tag's quality #477

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/alarm-condition-class-fields:fix/alarm-condition-class-fields

git checkout fix/alarm-condition-class-fields

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#476