From fd0bec4ffe8e343cd3da006d37732308fd786128 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 15 Jul 2026 16:54:09 -0400 Subject: [PATCH] docs(driver-expansion): resolve all open design concerns from the review pass Every concern the 7-agent review parked is now decided and integrated: - universal browser: in-flight capture coalescing keyed (driverType, config-hash) + global cap of 4 concurrent captures; CanBrowse catches a throwing TryCreate and defensively shuts down the throwaway instance; cleanup ShutdownAsync bounded at 10s (R2-01); picker Refresh = close + re-capture. Coalesced sessions share an immutable tree, so DisposeAsync drops the session's reference, not the tree. - mtconnect: UNAVAILABLE pinned to BadNoCommunication (fleet's BadCommunicationError stays reserved for the driver's own transport failures); TIME_SERIES sampleCount flows into ArrayDim; Subscribe timeout bounds only the stream-start handshake; library version/TFM folded into the license checklist. - mqtt: browse rebirth is an explicit DriverOperator-gated "Request rebirth" button (RequestRebirthAsync(scope)) - never fired by open/root/expand; hand-rolled reconnect loop committed for P1 (MQTTnet v5 dropped ManagedMqttClient); Wave-2-start library re-verification checkbox; implement from the Sparkplug v3.0 spec text. - bacnet: P1 opens with a first-spike checklist (package TFM, BBMD/ segmented-RPM/COV API smoke, unicast-I-Am fixture behavior); AdminUI-node browse registers a second foreign device - BBMD FD-table sizing note. - sql-poll: split-node browse needs env parity for Sql__ConnectionStrings__ refs on admin nodes (actionable error + session-only pasted literal); one-row-per-key query contract (last-wins + rate-limited warning); absent key -> BadNoData; operationTimeout > commandTimeout authoring rule; Browser->Driver.Sql SqlClient transitive on AdminUI accepted on record. - omron: FINS framer gated on W227 + live golden vectors; CIP string layout is the first P1 live-gate item; AbCip harness static-init anti-pattern note; writable-defaults-true operator warning. - modbus-rtu: first P1 step confirms pymodbus exposes framer=rtu on a TCP server (custom-script fallback otherwise). - program doc: new cross-cutting rule - driver ctors must be connection-free (the universal browser's CanBrowse throwaway instances depend on it). --- .../2026-07-15-bacnet-ip-driver-design.md | 42 +++++++++++-- ...6-07-15-driver-expansion-program-design.md | 4 ++ .../2026-07-15-modbus-rtu-driver-design.md | 6 +- ...2026-07-15-mqtt-sparkplug-driver-design.md | 29 +++++---- .../2026-07-15-mtconnect-driver-design.md | 15 ++--- docs/plans/2026-07-15-omron-driver-design.md | 61 +++++++++++++++---- .../2026-07-15-sql-poll-driver-design.md | 55 ++++++++++++++--- ...7-15-universal-discovery-browser-design.md | 50 ++++++++++++--- 8 files changed, 210 insertions(+), 52 deletions(-) diff --git a/docs/plans/2026-07-15-bacnet-ip-driver-design.md b/docs/plans/2026-07-15-bacnet-ip-driver-design.md index 59cd232d..2efbd711 100644 --- a/docs/plans/2026-07-15-bacnet-ip-driver-design.md +++ b/docs/plans/2026-07-15-bacnet-ip-driver-design.md @@ -57,7 +57,7 @@ only (it owns the shared `TagConfigJson` field readers used by the parser). ```xml - + ``` @@ -238,6 +238,16 @@ open. That is the documented **P-later graduation** to a bespoke lazy browser: **v1 uses the universal browser. State this clearly in the PR: BACnet ships with `SupportsOnlineDiscovery = true` and zero browser code.** +**Deployment note — AdminUI node network posture.** The universal browser runs its +`InitializeAsync → DiscoverAsync` capture **in the AdminUI process**, so the **AdminUI node itself needs +UDP reachability to the BACnet network** for the picker to browse — this is the same in-process posture as +the existing OpcUaClient/Galaxy browsers, but BACnet is the **first UDP/broadcast** one, so firewall/VLAN +rules that only whitelist the runtime driver node will silently break browse. When the AdminUI node is +off-segment, its browse capture registers as a **second foreign device** with the site BBMD (in addition +to the runtime driver's standing registration) for the duration of the capture — some BBMDs cap their +foreign-device-table entries, so size the FD table (and any per-source firewall rules) for **both** +registrations, not one. + --- ## 5. TagConfig + driver-config JSON @@ -475,8 +485,10 @@ live-gate discipline: route** (and bridged docker networks don't forward subnet broadcasts into containers either). So the suite as run from the dev machine has exactly two working paths: (a) **directed unicast Who-Is** at the fixture's host IP:port (host-mode `bacserv` answers a unicast Who-Is with a unicast I-Am to the - request's source address — verify this against the pinned `bacserv` build early, since spec-strict - stacks may broadcast the I-Am, which would never arrive); and (b) the **foreign-device/BBMD leg** — + request's source address — **verifying this against the pinned `bacserv`/`BasicServer` builds is item 3 + of the P1 first-spike checklist (§10)**, since spec-strict stacks may broadcast the I-Am, which would + never arrive; if they broadcast, this leg collapses to the BBMD/foreign-device leg below); and (b) the + **foreign-device/BBMD leg** — register with a BBMD container on the docker host; BVLC `Forwarded-NPDU` relay is *unicast* and routes across subnets, so this leg exercises real broadcast distribution cross-VM and is exactly the field topology. A genuinely broadcast-only leg (plain Who-Is with no BBMD) can only run **on the docker host @@ -498,6 +510,26 @@ slightly above OpcUaClient, driven by the callback→async adapter + COV lifetim Each phase is independently shippable (mirror how OpcUaClient landed across PRs): - **P1 — Connect + Read + Discover (+ universal browse) + BBMD. (MVP, delivers real value.)** + + **P1 opens with a first-spike verification checklist.** The library and fixture facts below came from + research (`docs/research/drivers/bacnet-ip.md`) and were **unverifiable offline** — verify them on the + wire before building anything on top of them: + + 1. **NuGet package reality.** Confirm the exact current version of the `BACnet` package (ela-compil + fork) and that its TFM set actually restores + loads on **net10.0** (pin the confirmed version in + `Directory.Packages.props` — see the §2 placeholder). + 2. **API-surface smoke test.** Exercise the specific API surface this design depends on: + BBMD/foreign-device registration (`Register-Foreign-Device` + TTL), **segmented** + ReadPropertyMultiple (a large `object-list` fetch), and SubscribeCOV callback delivery + (`OnCOVNotification`). Any gap here reshapes §3/§4 before real work starts. + 3. **Fixture unicast I-Am behavior.** Verify the pinned `bacserv` and `BasicServer` fixture builds + answer a **unicast** Who-Is with a **unicast** I-Am to the request's source address — a spec-strict + stack may broadcast the I-Am, which would **silently break the hermetic cross-VM test path** §9 + relies on (the directed-Who-Is leg from the dev machine). If either fixture broadcasts, adjust the + cross-VM suite to the **BBMD/foreign-device leg only** and keep directed-unicast tests on the + docker host itself. + + Then the P1 build proper: `.Contracts` (`BacnetDriverOptions`, enums, `BacnetTagAddress`, `BacnetEquipmentTagParser`); `BacnetClientAdapter` (async wrap); `BacnetDriver : IDriver, ITagDiscovery, IReadable, IHostConnectivityProbe` with `SupportsOnlineDiscovery=true` + `RediscoverPolicy => @@ -518,7 +550,9 @@ Each phase is independently shippable (mirror how OpcUaClient landed across PRs) 1. **BBMD / foreign-device registration + UDP broadcast reachability (HIGH — the top risk).** The server is usually not on the controllers' L2 segment, so correct foreign-device registration + TTL renewal is - load-bearing and is exactly what's hardest to reproduce in docker/CI. Mitigation: build BBMD in from + load-bearing and is exactly what's hardest to reproduce in docker/CI. Note the AdminUI browse capture + adds a **second** foreign-device registration off-segment (§4.2 deployment note) — account for it in + BBMD FD-table sizing. Mitigation: build BBMD in from P1, test explicitly with the `bacnet-docker` multi-network compose, gate a live suite against a real site BBMD. 2. **COV lifetime/renewal + non-COV fallback (HIGH, P2).** Subscriptions silently expire if not renewed; diff --git a/docs/plans/2026-07-15-driver-expansion-program-design.md b/docs/plans/2026-07-15-driver-expansion-program-design.md index 59704bb3..52f2a1b8 100644 --- a/docs/plans/2026-07-15-driver-expansion-program-design.md +++ b/docs/plans/2026-07-15-driver-expansion-program-design.md @@ -100,6 +100,10 @@ driver-typed-editors design): fleet plumbing lands. - **Secrets** — connection strings / API keys / broker creds come from env/secret refs (mirror `ServerHistorian__ApiKey`), never committed or logged. +- **Ctor is connection-free** — driver constructors must not open connections or touch the + network; all connects happen in `InitializeAsync`. Every existing driver already obeys this, + and the universal browser's `CanBrowse` throwaway-instance pattern **depends** on it — a + ctor-connecting driver would leak connections on every capability probe. - **Live-verify discipline** — Razor binding + deploy-inertness bugs pass unit tests and review; every driver's picker/editor and deploy path gets a docker-dev `/run` live-verify before it's trusted. diff --git a/docs/plans/2026-07-15-modbus-rtu-driver-design.md b/docs/plans/2026-07-15-modbus-rtu-driver-design.md index a2ac4131..984e489c 100644 --- a/docs/plans/2026-07-15-modbus-rtu-driver-design.md +++ b/docs/plans/2026-07-15-modbus-rtu-driver-design.md @@ -296,7 +296,11 @@ is an address *builder* (grammar helper), not a live browser, and stays valid fo checked-in compose services carry no `labels:` entries today). Drive it via `lmxopcua-fix up modbus rtu_over_tcp` + `lmxopcua-fix sync modbus`. This exercises the real CRC + FC-aware framing end-to-end with **no serial anything** — the only - genuinely new logic. + genuinely new logic. **First P1 step: confirm the pymodbus simulator entrypoint this + fixture uses actually exposes the RTU framer on a TCP server** (pymodbus 3.x supports + `framer=rtu` on TCP servers generally, but the simulator CLI/json config may not + surface the knob); if it doesn't, fall back to a small custom server script alongside + the existing `exception_injector.py` pattern. 2. **`diagslave`** (TCP RTU mode) for manual/soak — optional, not unit CI. 3. ~~Virtual serial pair (socat pty ↔ pymodbus RTU serial slave; com0com on Windows)~~ — **descoped with the direct-serial transport**; would return with it. diff --git a/docs/plans/2026-07-15-mqtt-sparkplug-driver-design.md b/docs/plans/2026-07-15-mqtt-sparkplug-driver-design.md index 53312af4..4a63db24 100644 --- a/docs/plans/2026-07-15-mqtt-sparkplug-driver-design.md +++ b/docs/plans/2026-07-15-mqtt-sparkplug-driver-design.md @@ -73,7 +73,7 @@ tests/Drivers/.../Docker/docker-compose.yml # mosquitto + sparkplug-sim + | Concern | Choice | Package | |---|---|---| -| **MQTT transport (both modes)** | **MQTTnet v5** — MIT, **.NET Foundation** (`dotnet/MQTTnet`), the standard .NET MQTT client. v5 explicitly ships a `net10.0` TFM. Client + TLS + MQTT 3.1.1/5.0. | `MQTTnet` (add `PackageVersion Include="MQTTnet" Version="5.x"` to `Directory.Packages.props`) | +| **MQTT transport (both modes)** | **MQTTnet v5** — MIT, **.NET Foundation** (`dotnet/MQTTnet`), the standard .NET MQTT client. v5 explicitly ships a `net10.0` TFM. Client + TLS + MQTT 3.1.1/5.0. **Note:** v5 dropped v4's `ManagedMqttClient` — reconnect is hand-rolled in P1 (§8). | `MQTTnet` (add `PackageVersion Include="MQTTnet" Version="5.x"` to `Directory.Packages.props`) | | **Sparkplug decode** | **Hand-rolled Tahu protobuf** (recommended) — generate C# from Tahu's `sparkplug_b.proto` with `Google.Protobuf` (`Grpc.Tools`/`protoc`), decode over a single MQTTnet-5 client. Small, stable schema; we own the alias/seq/rebirth state machine exactly as we want it. | `Google.Protobuf` + `Grpc.Tools` (build-time) | | **Alternative Sparkplug decode** | **SparkplugNet** — MIT, provides `Application`/`Node`/`Device` base classes, saves the birth/alias plumbing. **Rejected as primary** for two repo-specific reasons (below). | `SparkplugNet` (only if the risk clears) | @@ -81,6 +81,8 @@ tests/Drivers/.../Docker/docker-compose.yml # mosquitto + sparkplug-sim + **Decision: hand-roll Tahu-proto decode over one MQTTnet-5 client.** It removes the diamond and the net10-TFM concern entirely, and the Sparkplug payload schema (`Payload { seq, timestamp, Metric[], body }`, `Metric { name, alias, timestamp, datatype, value, is_null, ... }`) is ~90 lines of `.proto` — a bounded, well-understood effort. Phase 1 (plain MQTT) references **only MQTTnet-5**, so we validate MQTTnet-v5-on-net10 before ever touching Sparkplug. (If a future need for SparkplugNet arises, the phase-2 codec seam `SparkplugCodec` is the single swap point.) +- [ ] **Re-verify the external-library claims at Wave-2 start (before P1 coding):** confirm the exact **MQTTnet v5 version + `net10.0` TFM** on NuGet, and re-check the **SparkplugNet MQTTnet-4.x transitive-pin** claim — it is directionally right from the research but was unverifiable offline. The hand-roll decision does **not** hinge on that pin being exact (it stands on the net10-TFM concern + owning the alias/seq/rebirth state machine regardless), so this is a confirm-the-record check, not a decision gate. + **Vendor the proto:** commit Tahu `sparkplug_b.proto` into `Driver.Mqtt.Contracts/Protos/` (pinned to a known Tahu commit, with a provenance comment) and generate at build via `Grpc.Tools`. Don't fetch at build time. **Existing protobuf tooling, honestly:** `Google.Protobuf` is **already centrally pinned** (`Directory.Packages.props:31`, 3.34.1) and consumed by `Driver.Galaxy` — but the Galaxy proto types ship **pre-generated** inside the `ZB.MOM.WW.MxGateway.*` NuGets; there is **no `.proto` file or `Grpc.Tools` protoc codegen anywhere in this repo today**. So `Grpc.Tools` is a new (build-time-only, `PrivateAssets=all`) package and this is the repo's first in-repo proto codegen step. If that step proves objectionable, the fallback is checking in the generated C# (the schema is small and stable) with the same provenance comment. @@ -103,6 +105,8 @@ tests/Drivers/.../Docker/docker-compose.yml # mosquitto + sparkplug-sim + - **Plain mode:** subscribe to each authored tag's `topic` at its `qos` (dedupe identical topics; coalesce shared wildcards where the operator authored one). On message: match topic → authored tag(s), extract value at `jsonPath` (or raw/scalar), raise `OnDataChange(handle, FullReference, snapshot)`. Seed initial value from the broker's **retained** message replayed on subscribe (OPC UA initial-data convention). - **Sparkplug mode:** subscribe `spBv1.0/{groupId}/#` (+ the host-state topic when acting as primary host — **`spBv1.0/STATE/{hostId}` with a JSON `{online, timestamp}` payload in spec v3.0**; pre-3.0 peers used top-level `STATE/{hostId}` with `ONLINE`/`OFFLINE` text — target the v3.0 form, tolerate the legacy one on receive). Maintain a **`BirthCache` + `AliasTable` per (edgeNode, device)** from NBIRTH/DBIRTH. On NDATA/DDATA: resolve each metric by **alias → (name, datatype)** via the alias table, map to the authored tag's `FullReference` **by stable metric name**, raise `OnDataChange`. On NDEATH/DDEATH: emit **STALE / Bad-quality** snapshots for that node's/device's metrics. On missed birth / unknown alias / seq gap: issue a **rebirth NCMD** (§3.6). +**Spec version:** implement from the **Sparkplug v3.0 spec text** — the research report cites v2.2, which is superseded on exactly the STATE topic/payload point above (this doc already carries the correct v3.0 form). + - `publishingInterval` is **advisory** (MQTT/Sparkplug are event-driven). Use it only for optional server-side coalescing/deadband, mirroring Modbus's `ShouldPublish`. `SubscribeAsync` returns an `ISubscriptionHandle` whose `DiagnosticId` names the mode+filter; `OnDataChange` fires from the MQTT receive handler for the whole driver lifetime. `SubscribeAsync` maps the requested `FullReference`s onto the live receive stream: it registers them in a `FullReference → MqttTagDefinition` resolver (parsed once, cached — reuse the shared `EquipmentTagRefResolver` from `Core.Abstractions`, exactly as Modbus does), so an inbound message/metric routes to the right node. @@ -156,7 +160,7 @@ The alias→metric map, seq-gap detection, late-join rebirth, and death→STALE 2. **Rebuild the alias table on every (re)birth.** NBIRTH/DBIRTH replaces the `(edgeNode,device)` `AliasTable` wholesale; never merge. 3. **Track `seq` (0–255, wraps) per edge-node.** A gap (or an unknown alias, or a data message before any birth) ⇒ request rebirth (`RebirthRequester` publishes `NCMD` writing `Node Control/Rebirth = true`), gated on `requestRebirthOnGap`. `bdSeq` ties an NDEATH to its NBIRTH. 4. **Death → STALE.** NDEATH/DDEATH (LWT-driven) emits Bad/uncertain-quality snapshots for all affected metrics; the next birth restores Good. -5. **Late join.** On connect the driver may have missed births — proactively request rebirth (or wait for the retained STATE + natural rebirth) so metadata is recovered. This is the same primitive the browser uses (§4). +5. **Late join.** On connect the driver may have missed births — proactively request rebirth (or wait for the retained STATE + natural rebirth) so metadata is recovered. (This runtime-driver late-join rebirth is automatic and correct — the driver *owns* its subscription; it is the same NCMD primitive the browser exposes only as the explicit operator "Request rebirth" action, §4.1.) Unit-test these explicitly against a controllable simulator: rebirth, missed-birth, seq-gap, alias-reuse-across-rebirth, death→stale→rebirth. @@ -177,7 +181,7 @@ v1 ships **read/subscribe/discover only** — a genuinely useful UNS-ingest driv The universal browser (`docs/plans/2026-07-15-universal-discovery-browser-design.md`) captures whatever `DiscoverAsync` streams and re-presents it, gated on `SupportsOnlineDiscovery`. **MQTT/Sparkplug is the one roadmap driver that needs a bespoke browser despite the universal one**, because: - Runtime `DiscoverAsync` is **authored-only** (§3.3). A universal Discover-backed browser would replay only tags the operator already authored — it would **not discover new topics/metrics**, which is the entire point of a picker. -- MQTT/Sparkplug discovery is **observational**: you learn what exists only by *listening* for a bounded window. That's a fundamentally different session shape than "connect → one-shot enumerate → disconnect" — the tree **fills in over time**, and Sparkplug can be *accelerated* by forcing a rebirth NCMD. +- MQTT/Sparkplug discovery is **observational**: you learn what exists only by *listening* for a bounded window. That's a fundamentally different session shape than "connect → one-shot enumerate → disconnect" — the tree **fills in over time**, and Sparkplug can be *accelerated on demand* by an operator-requested rebirth NCMD (§4.1 — explicitly not automatic). **Considered + rejected:** feeding the Sparkplug `BirthCache` into `DiscoverAsync` device-enumeration to plug into universal. Rejected because it would make runtime discovery auto-provision from births (the unbounded-address-space failure §3.3 explicitly avoids) and still wouldn't cover plain MQTT. The birth cache stays a browse-time + subscribe-time structure, not a discovery source. @@ -189,16 +193,19 @@ The universal browser (`docs/plans/2026-07-15-universal-discovery-browser-design 1. Parse the **same `MqttDriverOptions` JSON** the runtime driver consumes (shared `JsonSerializerOptions`), but connect with a **distinct client-id suffix** (`{clientId}-browse-{guid8}`) so the browse session never collides with the running driver's MQTT session / clean-session state. 2. Connect an MQTTnet client under a bounded connect budget (clamp like OpcUaClient's `PerEndpointConnectTimeout`, 5–30 s). 3. Subscribe to the discovery filter: `spBv1.0/{groupId}/#` (Sparkplug) or `#`/`{topicPrefix}#` (plain). -4. **Kick off the observation window immediately** and, in Sparkplug mode, optionally publish a **rebirth NCMD** to force every edge node to re-announce (turns the passive wait into a near-synchronous enumeration). +4. **Kick off the observation window immediately — passively.** The session only *listens*; opening it publishes **nothing**. This keeps browse read-only by default, matching the house browse pattern (driver-browsers design §6: "browse is read-only and doesn't mutate config or driver state"), and avoids rebirth-storming every edge node in the group whenever an operator merely opens a picker — on a large plant an auto-rebirth on session open would stampede the whole group's edge nodes into re-publishing full births. 5. Return an `MqttBrowseSession` whose internal tree **fills in over the window and keeps filling** while the session lives. Because `IBrowseSession` methods are async and the session lands in the AdminUI `BrowseSessionRegistry` (TTL-reaped, per-call 20 s timeout), the observation cost is amortised across the user's clicks in the picker. +**Explicit "Request rebirth" — the one operator-initiated write (Sparkplug mode only).** The picker carries a **"Request rebirth"** button, enabled in Sparkplug mode and **scoped to the currently selected group or edge node**. Clicking it invokes `MqttBrowseSession.RequestRebirthAsync(scope)` — an MQTT-specific session method *beyond* the base `IBrowseSession` surface, dispatched by the picker through `BrowserSessionService` for MQTT sessions — which publishes the rebirth **NCMD** (`Node Control/Rebirth = true`, via the same `RebirthRequester` encode path) to the selected edge node, or to each *observed* edge node under the selected group. That converts the passive wait into a near-synchronous enumeration precisely when the operator asks for it. This is the design's single deliberate deviation from browse-is-read-only: it is operator-initiated, scoped, gated by the same `DriverOperator` policy that gates the picker, and logged (Info, with the target scope) — it is **never** fired automatically by `OpenAsync`, `RootAsync`, or expansion. + ### 4.2 `MqttBrowseSession : IBrowseSession` Serves from an **accumulating observed tree** (thread-safe; the MQTT receive handler mutates it, browse calls read it). Refreshes `LastUsedUtc` on each call. -- **`RootAsync`** — Sparkplug: observed **groups** (or edge nodes under the configured group). Plain: top-level topic segments. `BrowseNode { Kind=Folder, HasChildrenHint=true }`. If the window hasn't yielded yet, return what's accumulated (possibly empty) and let the user re-expand; in Sparkplug mode trigger a rebirth on first `RootAsync` to populate fast. +- **`RootAsync`** — Sparkplug: observed **groups** (or edge nodes under the configured group). Plain: top-level topic segments. `BrowseNode { Kind=Folder, HasChildrenHint=true }`. If the window hasn't yielded yet, return what's accumulated (possibly empty) and let the user re-expand — or, in Sparkplug mode, click the explicit **"Request rebirth"** button (§4.1) to populate fast. `RootAsync` itself never publishes; browse calls are pure reads of the accumulated tree. - **`ExpandAsync(nodeId)`** — one level down the accumulated tree. Sparkplug: **Group → EdgeNode → Device → Metric**. Plain: next topic segment (split on `/`). Metrics / leaf-topics are `Kind=Leaf`. - **`AttributesAsync(nodeId)`** — Sparkplug: the metric's `AttributeInfo { Name, DriverDataType (from birth), IsArray, SecurityClass }` — **self-describing**, so the picker side-panel is populated (unlike the OPC UA browser which returns empty). Plain: a single synthetic attribute carrying the leaf topic's **inferred type + last-seen payload snippet**. +- **`RequestRebirthAsync(scope)`** — Sparkplug only, *beyond* the base `IBrowseSession` surface (§4.1): the single session member that publishes, and only ever on explicit operator click. - **`DisposeAsync`** — disconnect the MQTTnet client, best-effort (registry reaper may race), same pattern as `OpcUaClientBrowseSession`. ### 4.3 Picked node → TagConfig @@ -210,7 +217,7 @@ On commit the picker projects the selected `BrowseNode`/`AttributeInfo` into the ### 4.4 Addressing the passive/time-bounded wrinkle explicitly 1. Picker shows a live **"listening… N nodes/topics discovered"** affordance so the operator understands coverage is accumulating, not instantaneous. -2. Sparkplug mode **actively forces a rebirth** to convert the passive wait into a fast near-complete enumeration. +2. Sparkplug mode offers the explicit **"Request rebirth"** button (§4.1) — operator-initiated, scoped to the selected group/node — to convert the passive wait into a fast near-complete enumeration on demand. The window itself stays passive; if the operator doesn't click, coverage accumulates from natural traffic only. 3. The browse session stays alive (registry TTL) so re-expanding later reflects newly observed topics without reconnecting. 4. **Manual entry** of a topic/metric via the typed editor (§6) is always available as an escape hatch for a tag that's silent during the window. @@ -377,7 +384,7 @@ services.TryAddEnumerable(ServiceDescriptor.Singleton **License-pin caveat (from research §1.4):** older TrakHound releases carried mixed MIT / Apache-2.0 / "all rights reserved" strings. Before merge, confirm the embedded `LICENSE` of the *pinned* version is MIT (a legal-review checkbox, not a blocker). **Hand-roll fallback** if review fails: `HttpClient` + `System.Xml.Linq` for probe/current, a `multipart/x-mixed-replace` boundary reader for sample (~300–500 LoC behind the same `IMTConnectAgentClient` seam — the driver above the seam is unchanged, so the fallback is a drop-in of one class). +> **Library verification checklist (from research §1.4 — research-sourced facts, unverifiable offline).** Older TrakHound releases carried mixed MIT / Apache-2.0 / "all rights reserved" strings, and the version/TFM facts above came from research, not from a restore. **At implementation start, verify against the actual NuGet packages:** (1) the exact package version pin — confirm `6.9.0.2` (or the then-current pin) exists on nuget.org and restores; (2) its TFM set — confirm the packages ship `netstandard2.0` (or a net-10-compatible) target and load cleanly on .NET 10; (3) the license of the *pinned* version — confirm the embedded `LICENSE` is MIT (the before-merge legal-review checkbox, not a blocker). **Hand-roll fallback** if review fails: `HttpClient` + `System.Xml.Linq` for probe/current, a `multipart/x-mixed-replace` boundary reader for sample (~300–500 LoC behind the same `IMTConnectAgentClient` seam — the driver above the seam is unchanged, so the fallback is a drop-in of one class). ## 3. Capability mapping (concrete seam wiring) @@ -76,7 +76,8 @@ public async Task DiscoverAsync(IAddressSpaceBuilder builder, CancellationToken - each `Device` → `builder.Folder(device.Name, device.Name)` → child builder; - each nested `Component` → recursive `Folder(component.Name, component.DisplayName)` on the parent's child builder (component nesting becomes folder nesting — the builder-graph *is* the tree); - each `DataItem` → `child.Variable(browseName, displayName, attr)` where - `attr = new DriverAttributeInfo(FullName: dataItem.Id, DriverDataType: MTConnectDataTypeInference.Infer(category, type, units), IsArray: representation==TIME_SERIES, ArrayDim: null, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: category==CONDITION)`. + `attr = new DriverAttributeInfo(FullName: dataItem.Id, DriverDataType: MTConnectDataTypeInference.Infer(category, type, units), IsArray: representation==TIME_SERIES, ArrayDim: representation==TIME_SERIES && dataItem.SampleCount is int n and > 0 ? n : null, SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: category==CONDITION)`. + (For `TIME_SERIES` items the probe-declared `sampleCount` attribute flows into `ArrayDim` — the record's doc-comment defines `ArrayDim` as the declared array length when `IsArray` is true. `null` only when the device model doesn't declare `sampleCount`; many agents omit it for variable-length series.) (`ViewOnly` is the read-only-from-OPC-UA tier — `SecurityClassification` has no `ReadOnly` member.) `browseName` = `dataItem.Name ?? dataItem.Id` (dataItem `name` is optional in MTConnect; fall back to `id`, which is guaranteed unique). @@ -91,13 +92,13 @@ Pure function in `.Contracts` (shared by driver, browser-commit, and the typed e | MTConnect | `DriverDataType` | |---|---| | `SAMPLE` numeric (has `units`) | `Float64` | -| `SAMPLE` `representation=TIME_SERIES` | `Float64` array (`IsArray=true`) | +| `SAMPLE` `representation=TIME_SERIES` | `Float64` array (`IsArray=true`; `ArrayDim` = probe-declared `sampleCount` when present, else null — §3.2) | | `EVENT` numeric type (`PartCount`, `Line`, …) | `Int64` | | `EVENT` controlled-vocab (`Execution`, `ControllerMode`, `Availability`, …) | `String` | | `EVENT` free text (`Program`, `Block`, `Message`) | `String` | | `CONDITION` | `String` (state word; §3.6) | -**Quality mapping (`DataValueSnapshot.StatusCode`):** an observation value of `UNAVAILABLE` (MTConnect's explicit no-data sentinel) → OPC UA **`Bad`/`Uncertain`**, not the literal string. Missing dataItem / empty condition → `Bad`. `observation.timestamp` → `SourceTimestamp` (not a separate node). +**Quality mapping (`DataValueSnapshot.StatusCode`):** an observation value of `UNAVAILABLE` (MTConnect's explicit no-data sentinel) → **`BadNoCommunication` (`0x80310000u`)** with a null value, not the literal string — this is *the* UNAVAILABLE code, used everywhere in this design (read, subscribe, and the §8 fixtures). Rationale + fleet fit: semantically, `UNAVAILABLE` means the Agent is reachable but has no device-backed value (the adapter↔device link is down or the item has never reported) — exactly OPC UA's `BadNoCommunication` ("communication to the data source has failed"). The fleet grep shows no competing convention for this case: every existing driver's `BadCommunicationError` (`0x80050000u`, Modbus/FOCAS/TwinCAT/OpcUaClient/S7/AbCip/AbLegacy/Galaxy) marks the **driver's own transport failure** to its peer — a different failure (and the code this driver also uses when the *Agent* HTTP call fails, per §7) — while `BadNoData` (`0x800E0000u`) is historian-domain only (`Historian.Gateway/Mapping/SampleMapper.cs`). `BadNoCommunication` is already in the CLI's `SnapshotFormatter` name table (`src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs`), so it renders by name, not hex. Declare it as a `private const uint` in `MTConnectDriver` (the Modbus `StatusBadCommunicationError` pattern). Missing dataItem / empty condition → `Bad`. `observation.timestamp` → `SourceTimestamp` (not a separate node). ### 3.4 `IReadable` — `/current` @@ -161,14 +162,14 @@ Avoid the raw-JSON fallback by adding a typed editor (mirrors the Modbus templat Every agent HTTP request MUST carry a **per-call deadline** — never an unbounded wait (the R2-01 S7 finding: an async read that ignores its socket timeout wedges the poll loop against a frozen peer). Concretely: -- **`/probe` and `/current`:** wrap each call in a `CancellationTokenSource(RequestTimeoutMs)` linked to the caller's `ct`; a timeout surfaces as a `Bad`-coded snapshot (read) or a Faulted init (probe), never a hang. Set `HttpClient.Timeout` **and** a linked CTS (belt-and-suspenders — `HttpClient.Timeout` doesn't cover the response-body read of a streamed multipart). +- **`/probe` and `/current`:** wrap each call in a `CancellationTokenSource(RequestTimeoutMs)` linked to the caller's `ct`; a timeout surfaces as a `BadCommunicationError`-coded snapshot (read — the fleet-standard driver-transport-failure code, distinct from the §3.3 `BadNoCommunication` UNAVAILABLE mapping) or a Faulted init (probe), never a hang. Set `HttpClient.Timeout` **and** a linked CTS (belt-and-suspenders — `HttpClient.Timeout` doesn't cover the response-body read of a streamed multipart). - **`/sample` long-poll stream watchdog:** the stream is intentionally long-lived, so `HttpClient.Timeout` cannot bound it. Instead run a **watchdog**: if no chunk **and** no keep-alive heartbeat arrives within `HeartbeatMs × N` (e.g. 3× the Agent heartbeat), treat the stream as dead → cancel it, transition `HostState.Stopped`, and reconnect. The Agent's own `heartbeat` on the multipart boundary is the liveness signal; absence past the watchdog window is the frozen-peer detector. - **Reconnect / backoff:** geometric backoff (`MinBackoffMs`→`MaxBackoffMs`) on stream drop or connect failure, mirroring `ModbusReconnectOptions`. On reconnect, re-baseline via `/current` then resume `/sample` from the fresh `nextSequence`. -- All capability calls flow through the existing Phase-6.1 `IDriverCapabilityInvoker` resilience pipeline (Read/Discover/Subscribe/Probe auto-retry); the per-call deadlines above are the driver-internal floor beneath that pipeline. +- All capability calls flow through the existing Phase-6.1 `IDriverCapabilityInvoker` resilience pipeline (Read/Discover/Subscribe/Probe auto-retry); the per-call deadlines above are the driver-internal floor beneath that pipeline. **Subscribe-timeout interplay:** the pipeline's Subscribe timeout bounds only the `SubscribeAsync` call itself — the `/sample` stream-start handshake must complete within it, and `SubscribeAsync` returns once the stream is established; the long-lived pump then runs outside that timeout, guarded by the heartbeat watchdog above instead. ## 8. Test fixtures -1. **Canned XML unit fixtures (bulk of coverage, no network).** Capture one `MTConnectDevices` (probe) + a couple of `MTConnectStreams` (current + sample, incl. one with a forced `nextSequence` gap) from a demo agent into `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/Fixtures/`. Drive through the `IMTConnectAgentClient` fake to pin: `DiscoverAsync` tree shape + leaf `FullName==dataItemId`, `MTConnectDataTypeInference` table, observation indexing, `UNAVAILABLE→Bad`, CONDITION→state-word, multipart chunk framing, and ring-buffer re-baseline paging. +1. **Canned XML unit fixtures (bulk of coverage, no network).** Capture one `MTConnectDevices` (probe) + a couple of `MTConnectStreams` (current + sample, incl. one with a forced `nextSequence` gap) from a demo agent into `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/Fixtures/`. Drive through the `IMTConnectAgentClient` fake to pin: `DiscoverAsync` tree shape + leaf `FullName==dataItemId`, `MTConnectDataTypeInference` table, observation indexing, `UNAVAILABLE→BadNoCommunication` (§3.3), CONDITION→state-word, multipart chunk framing, and ring-buffer re-baseline paging. 2. **Dockerized `mtconnect/cppagent` (reproducible integration fixture).** Add `tests/.../Docker/docker-compose.yml` with the **`project: lmxopcua`** label on every service, seeded with a canned `Devices.xml` (+ an SHDR simulator or the built-in adapter), exposed on the shared docker host `10.100.0.35`; drive it via `lmxopcua-fix up mtconnect` + `sync`. Env-gated integration suite (`*.IntegrationTests`), skips cleanly when the fixture is down — the analog of the Modbus/S7 sims. 3. **Public demo agent (manual live smoke only).** `https://demo.mtconnect.org/` (or NIST `smstestbed` `Devices.xml`) for a real-shape browse/read/stream smoke. Internet-dependent, not in CI. Live-verify the browse picker on docker-dev per the universal-browser discipline: open the `/uns` TagModal picker for an MTConnect driver, confirm the Device→Component→DataItem tree renders and a picked leaf commits `TagConfig.FullName = ` (Razor binding bugs pass unit tests — always `/run` it). diff --git a/docs/plans/2026-07-15-omron-driver-design.md b/docs/plans/2026-07-15-omron-driver-design.md index 58bad039..0f88a788 100644 --- a/docs/plans/2026-07-15-omron-driver-design.md +++ b/docs/plans/2026-07-15-omron-driver-design.md @@ -74,7 +74,7 @@ src/Drivers/ | Path | Library | License | Notes | |---|---|---|---| | **CIP** | **`libplctag.NET`** (``), attribute string `plc=omron-njnx` | native core dual MPL-2.0 / LGPL-2.1; .NET wrapper **MPL-2.0** | **Zero new license review** — the identical dependency AbCip already ships. | -| **FINS** | **Hand-rolled** (no third-party dep) | our own code | FINS framing is ~200 lines, fully documented (W227 + Wireshark dissector); mirrors `ModbusTcpTransport`. | +| **FINS** | **Hand-rolled** (no third-party dep) | our own code | FINS framing is ~200 lines, documented (W227 + Wireshark dissector) but research-sourced — trusted only after the P2 validation gate (§10); mirrors `ModbusTcpTransport`. | **Avoid `HslCommunication`.** The maintained builds are **commercial-paid**; the "MIT" claim in casual sources refers to a stale, abandoned community fork @@ -187,6 +187,12 @@ programmer set its **Network Publish** attribute (`Publish Only`) in **Sysmac St it are invisible to any external client — including our reads. Document this loudly in `docs/drivers/Omron.md`: a CIP read of an unpublished variable fails at the wire, not a config bug. +**Operator documentation task (`docs/drivers/Omron.md`, P1):** alongside the Network-Publish note +above, the driver doc must carry an explicit operator-facing warning that **`writable` defaults to +`true` when omitted** from a tag's config (the convention inherited from AbCip's parser, §5.4). On +a write-capable PLC driver, forgetting the field does not fail safe — it silently authors a +writable node. Operators must set `"writable": false` explicitly on every read-only point. + --- ## 4. Browse story — reconciliation with the universal browser @@ -337,6 +343,11 @@ via `EquipmentTagRefResolver`. For an **equipment tag**, `FullName` **is the raw `AbCipDataTypeExtensions`) maps to `DriverDataType`; `SecurityClass` = `Operate` when `writable` else `ViewOnly`. +**`writable` defaults to `true` when absent** — the parser inherits AbCip's convention, on both +transport shapes (§5.2/§5.3). An Omron tag authored without an explicit `writable` field is +therefore a **writable** node (`SecurityClass = Operate`). On a write-capable PLC driver this +deserves an explicit operator-facing warning — see the operator-documentation item in §3.1. + --- ## 6. Typed editor + validator @@ -464,14 +475,26 @@ The honest gap: **there is no free NJ/NX CIP simulator.** - **Primary CI surface — hand-rolled `IOmronCipRuntime` fake** (the same seam pattern AbCip uses via `IAbCipTagRuntime`/factory injection). Drives read/write/decode/array/BOOL-in-word/status-mapping unit tests with **no hardware and no container**. This is where correctness of the driver *body* is - proven. + proven. When mirroring AbCip's test structure, do **not** copy its integration-harness + profile-class pattern that had the textual static-initializer-order bug (`AbServerProfile.cs`, + fixed in `326b22a7`) — initialize profile constants free of cross-field static init-order + dependence. - **Live gate — env-gated `Category=LiveIntegration`** against real NJ/NX hardware, skipping cleanly when the env var is absent (the pattern the HistorianGateway live suite uses). This is the **only** - way to validate the real `plc=omron-njnx` libplctag wire path, Network-Publish behavior, and Omron - string/array/UDT quirks. Env vars e.g. `OMRON_CIP_ENDPOINT` / `OMRON_CIP_TEST_TAG` / - `OMRON_CIP_WRITE_SANDBOX_TAG`. **Flag as an infra-gated known limitation** in the driver doc + this - design until an NJ/NX is on the bench. (Omron's own CX-Simulator is Windows-only + license-gated in - CX-One — not a CI fixture.) + way to validate the real `plc=omron-njnx` libplctag wire path. **P1 live-hardware gate checklist, + in order:** + 1. **CIP string layout — FIRST.** Omron NJ/NX's string layout differs from Logix, and whether + libplctag's `GetString` handles it correctly can only be proven on hardware. This is the + likeliest wire surprise, and the `IOmronCipRuntime` fake cannot cover it — run a `String` + read/write round-trip before anything else. + 2. Basic read/write round-trips per `OmronDataType` over the real `plc=omron-njnx` path. + 3. Network-Publish behavior (unpublished variable → wire failure, per §3.1). + 4. Array reads/writes + dotted UDT-member access (§3.1's live-gate item). + + Env vars e.g. `OMRON_CIP_ENDPOINT` / `OMRON_CIP_TEST_TAG` / + `OMRON_CIP_WRITE_SANDBOX_TAG` / `OMRON_CIP_STRING_SANDBOX_TAG`. **Flag as an infra-gated known + limitation** in the driver doc + this design until an NJ/NX is on the bench. (Omron's own + CX-Simulator is Windows-only + license-gated in CX-One — not a CI fixture.) ### FINS - **Primary — hand-rolled byte-level `IFinsTransport` fake** (mirror the Modbus `IModbusTransport` @@ -497,20 +520,34 @@ can't be proven in CI, so it should land when live NJ/NX hardware is available f | Phase | Scope | Effort | Gate | |---|---|---|---| -| **P1 — Omron CIP r/w (NJ/NX)** | `Driver.Omron` + `.Contracts`, `DriverType="Omron"`, `OmronCipTransport` on libplctag `plc=omron-njnx`; Connect/Read/Write/Subscribe(poll)/Probe; authored + equipment tags; typed editor + validator; factory + registration | Low–Medium (70–80% AbCip reuse) | hand-rolled runtime fake (CI) + env-gated live gate (hardware) | -| **P2 — Omron FINS r/w (CJ/CS/CP)** | `IFinsTransport` codec (TCP+UDP, `0x0101`/`0x0102`, area-code table, word-swap) behind the same `IDriver` shell; memory-area TagConfig + editor fields (no browse) | Medium (net-new codec, no dep/license risk) | byte-level fake (CI) + OSS FINS sim container | +| **P1 — Omron CIP r/w (NJ/NX)** | `Driver.Omron` + `.Contracts`, `DriverType="Omron"`, `OmronCipTransport` on libplctag `plc=omron-njnx`; Connect/Read/Write/Subscribe(poll)/Probe; authored + equipment tags; typed editor + validator; factory + registration | Low–Medium (70–80% AbCip reuse) | hand-rolled runtime fake (CI) + env-gated live gate (hardware; §9 checklist — string layout first) | +| **P2 — Omron FINS r/w (CJ/CS/CP)** | `IFinsTransport` codec (TCP+UDP, `0x0101`/`0x0102`, area-code table, word-swap) behind the same `IDriver` shell; memory-area TagConfig + editor fields (no browse) | Medium (net-new codec, no dep/license risk) | byte-level fake (CI) + OSS FINS sim container + **FINS validation gate** (below) | + +**P2 FINS validation gate (explicit):** every FINS wire specific in this design — the area-code +table, the `0x0101`/`0x0102` command framing, the FINS/TCP node-address handshake, port 9600, and +the bit-write behavior — is **research-sourced with no in-repo evidence**. The hand-rolled FINS +framer is therefore **gated on validation against the Omron W227 FINS reference manual plus golden +request/response vectors captured from live CJ/CS/CP hardware before it is trusted**; the captured +vectors become the byte-level fake's fixtures (§9). Wire specifics stay parameterised per CPU +family (the `FinsAreaCodeTable` keying already hedged in §5.3), so a manual/hardware discrepancy is +a table correction, not a codec rewrite. | **P3 — Sysmac tag-export importer (CIP)** | AdminUI offline importer: parse Sysmac Studio variable export → author `OmronTagDefinition`s. **Not** a live browser. | Medium | parser unit tests on real export samples | ### Top risks 1. **No free NJ/NX CIP simulator** (the headline). CIP wire correctness (string/array/UDT/ Network-Publish) is provable only on live hardware behind the env-gated `LiveIntegration` suite → **infra-gated known limitation** until an NJ/NX is on the bench. The hand-rolled fake proves the - driver *body*, not the *wire*. + driver *body*, not the *wire* — and the NJ/NX-vs-Logix **string layout** is the likeliest wire + surprise, so it is the *first* item on the P1 live-gate checklist (§9). 2. **CIP browse is not free reuse.** libplctag's `@tags` walker returns `ErrorUnsupported` on Omron NJ/NX; `SupportsOnlineDiscovery=false`, the universal browser doesn't apply, and browse ships as an offline Sysmac-export importer (P3), with online browse deferred to a later research phase. -3. **FINS per-family variance** in memory-area codes + multi-word ordering — contained by the - CPU-family-parameterised `FinsAreaCodeTable` + per-tag `wordSwap`. +3. **FINS wire specifics are research-sourced, not evidence-backed.** The area codes, + `0x0101`/`0x0102` commands, FINS/TCP handshake, port 9600, and bit-write behavior have no + in-repo precedent — the hand-rolled framer is gated on the P2 validation gate (W227 manual + + golden vectors from live hardware) before it is trusted. Per-family variance in memory-area + codes + multi-word ordering is contained by the CPU-family-parameterised `FinsAreaCodeTable` + + per-tag `wordSwap`. 4. **Enum-serialization trap** — every config JSON surface must attach `JsonStringEnumConverter`, or AdminUI-authored Omron configs fault the driver. Live-`/run` verify catches it; unit tests don't. diff --git a/docs/plans/2026-07-15-sql-poll-driver-design.md b/docs/plans/2026-07-15-sql-poll-driver-design.md index c55b5268..5e9df5ec 100644 --- a/docs/plans/2026-07-15-sql-poll-driver-design.md +++ b/docs/plans/2026-07-15-sql-poll-driver-design.md @@ -62,7 +62,7 @@ All `net10.0`, `Nullable`/`ImplicitUsings` enabled, `TreatWarningsAsErrors`, mat |---|---| | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/` | Options DTOs, `SqlProvider` enum, tag-model records, `SqlEquipmentTagParser`. **Zero transport deps** (refs only `Core.Abstractions`) so AdminUI + browser can reference it without dragging `Microsoft.Data.SqlClient`. | | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/` | `SqlDriver` (runtime capabilities), `ISqlDialect` + `SqlServerDialect`, `SqlPollReader`, factory extensions. **Owns the `Microsoft.Data.SqlClient` package ref.** | -| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/` | `SqlDriverBrowser : IDriverBrowser`, `SqlBrowseSession : IBrowseSession` (schema walk). Refs `Commons` + `Sql.Contracts` + **`Driver.Sql`** (for `ISqlDialect`/`SqlServerDialect` — the dialect's catalog SQL *is* the browse engine, so it must be shared, not duplicated; `Microsoft.Data.SqlClient` comes transitively). This runtime-project ref is a deliberate deviation from `OpcUaClient.Browser` (which refs only its Contracts and owns the OPC UA client packages itself): the browser still opens its own transient connection. | +| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/` | `SqlDriverBrowser : IDriverBrowser`, `SqlBrowseSession : IBrowseSession` (schema walk). Refs `Commons` + `Sql.Contracts` + **`Driver.Sql`** (for `ISqlDialect`/`SqlServerDialect` — the dialect's catalog SQL *is* the browse engine, so it must be shared, not duplicated; `Microsoft.Data.SqlClient` comes transitively). This runtime-project ref is a deliberate deviation from `OpcUaClient.Browser` (which refs only its Contracts and owns the OPC UA client packages itself): the browser still opens its own transient connection. The resulting **transitive `Microsoft.Data.SqlClient` reference on AdminUI is reviewed and accepted** — AdminUI already uses SqlClient for ConfigDb, so this adds no new package to its closure. Recorded here deliberately so nobody "fixes" the deviation later without this context. | | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/` | Unit suite (SQLite fixture, §9). | | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/` | Env-gated integration suite (central SQL Server, §9). | | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/` | Browser unit tests (SQLite `PRAGMA` dialect). | @@ -257,6 +257,16 @@ Three models; **(a) and (b) ship in P1**, (c) is P3. per poll; index rows by key, slice back. Values in the `IN` list are **bound parameters**, one per key — never interpolated. +**Query contract — one row per key (v1).** The `WHERE IN (…)` poll assumes the authored +source yields **exactly one row per key**. Point the tag at a current-values table, or — for +append-only staging/history tables that hold many rows per key — at a **view or query the operator +writes** that reduces to latest-per-key (`GROUP BY key` + `MAX(timestamp)`, or +`ROW_NUMBER() OVER (PARTITION BY ORDER BY DESC) = 1`); that +operator-authored latest-per-key view is the **supported workaround** for append-only sources. If a +poll nonetheless returns multiple rows for a key, the driver deterministically takes the **last** +occurrence in reader order and emits a **rate-limited warning** log naming the offending key — the +value stays usable, but the config is telling you the source violates the contract. + **(b) Wide row** *(cheap).* One "latest status" row, many columns → many tags. A tag names a `table`, a `columnName`, and a `rowSelector` (`{whereColumn, whereValue}` or `{topByTimestamp: }`). **GroupKey = `(table, rowSelector)`.** All wide-row tags on the same row fold into one @@ -297,7 +307,9 @@ the address-space layer maps to OPC UA built-ins. **override** the inferred type explicitly (`"type": "Int32"`) — the operator sometimes knows better than column metadata (e.g. a `varchar` column holding a number); the driver then coerces the read cell to the declared type. `NULL` → `DataValueSnapshot { Value = null }` with `Good`/`Uncertain` per -a `nullIsBad` option (default: treat as `Uncertain`, not `Bad`). +a `nullIsBad` option (default: treat as `Uncertain`, not `Bad`). A key **entirely absent** from the +result set (row deleted) is distinct from a NULL cell: that tag's snapshot goes **`BadNoData`** — +`nullIsBad` governs only a present row whose value cell is NULL. **Source timestamp:** if a tag declares `timestampColumn`, that column's value becomes `DataValueSnapshot.SourceTimestampUtc`; otherwise the poll-read wall-clock is used for both source and @@ -376,7 +388,7 @@ vs wide-row) + fills the key/row selector. `SecurityClass = ViewOnly` in read-on | Path | Purpose | |---|---| -| `Driver.Sql.Browser/SqlDriverBrowser.cs` | `IDriverBrowser`, `DriverType => "Sql"`; `OpenAsync(configJson, ct)` deserializes options, resolves the connection string from the form JSON (same trust boundary as `TestDriverConnect` + the OpcUaClient browser — transient, never cached), opens a `DbConnection`, returns a `SqlBrowseSession`. | +| `Driver.Sql.Browser/SqlDriverBrowser.cs` | `IDriverBrowser`, `DriverType => "Sql"`; `OpenAsync(configJson, ct)` deserializes options and resolves the connection string from the `connectionStringRef` in the form JSON via a **direct env-var read** (`Sql__ConnectionStrings__`) **in the AdminUI process** — the same resolution the driver performs at Initialize (§8.2). If the ref doesn't resolve, the open **fails with an actionable message naming the exact missing env var** (e.g. `Connection string ref 'MesStaging' not found — set env var Sql__ConnectionStrings__MesStaging on this admin node`). The picker form **additionally accepts a pasted literal connection string** for ad-hoc browse — used for that browse session only, **never persisted** into the driver config (same trust boundary as `TestDriverConnect` + the OpcUaClient browser — transient, never cached). Opens a `DbConnection`, returns a `SqlBrowseSession`. | | `Driver.Sql.Browser/SqlBrowseSession.cs` | `IBrowseSession`; `RootAsync`/`ExpandAsync`/`AttributesAsync` over the dialect catalog queries; a `SemaphoreSlim _gate` (one ADO.NET connection is not concurrent). Per-call work is bounded by the AdminUI's existing 20 s per-call timeout. | | AdminUI DI (`EndpointRouteBuilderExtensions.cs`, beside `:49-50`) | `services.AddSingleton();` | | `AdminUI/Uns/TagEditors/TagConfigEditorMap.cs` | `["Sql"] = typeof(Components.Shared.Uns.TagEditors.SqlTagConfigEditor)` (§6). | @@ -386,6 +398,15 @@ The browser reuses the AdminUI `BrowseSessionRegistry` + reaper + 2-min idle TTL unchanged. **The universal browser DI line is NOT added for `Sql`** — its `CanBrowse` is false anyway, and the bespoke registration wins. +**Deployment requirement — env parity across split nodes.** Because the browser resolves +`connectionStringRef` in the **AdminUI process**, on split-role deployments the **admin nodes must +carry the same `Sql__ConnectionStrings__` env vars as the driver nodes** for every ref the +operator wants browseable (the same split-role lesson as driver probes on admin nodes). A ref present +only on driver nodes still *polls* fine at runtime but cannot be *browsed*; when that happens the +browser-open failure names the exact missing env var so the operator knows precisely what to add to +the admin node's environment — or falls back to pasting a literal connection string into the picker +for a one-off session-only browse. + --- ## 5. TagConfig + driver-config JSON @@ -398,7 +419,7 @@ and the bespoke registration wins. // Connection string is resolved at Initialize from an env/secret ref — NOT stored here. "connectionStringRef": "MesStaging", // => env Sql__ConnectionStrings__MesStaging "defaultPollInterval": "00:00:05", - "operationTimeout": "00:00:15", // per-query wall-clock deadline (→ BadTimeout) + "operationTimeout": "00:00:15", // per-query wall-clock deadline (→ BadTimeout); MUST be > commandTimeout (§8.3) "commandTimeout": "00:00:10", // server-side CommandTimeout backstop (seconds granularity) "maxConcurrentGroups": 4, // cap on concurrent group queries (pool guard) "nullIsBad": false, // NULL cell → Uncertain (default) vs Bad @@ -436,7 +457,8 @@ and the bespoke registration wins. ``` GroupKey for batching = `("dbo.TagValues", "tag_name", "num_value", "sample_ts")`; all such tags fold -into one `… WHERE tag_name IN (@k0,@k1,…)` per poll. +into one `… WHERE tag_name IN (@k0,@k1,…)` per poll. The `table` must yield one row per `keyValue` — +for append-only sources, author a latest-per-key view and point `table` at it (§3.6 query contract). ### 5.3 Per-tag `TagConfig` — wide-row model @@ -485,7 +507,9 @@ its raw `TagConfig` JSON, parsed once and cached) — same as Modbus. ## 6. Typed editor + validator + the enum-serialization trap Add `["Sql"] = typeof(…SqlTagConfigEditor)` to `TagConfigEditorMap` and a matching entry to -`TagConfigValidator`. The editor is a thin Razor shell over a pure `SqlTagConfigModel` +`TagConfigValidator`. The validation layer also enforces the driver-level timeout-ordering rule +(§8.3): `operationTimeout` must be strictly greater than `commandTimeout`, rejected (or warned on) +at authoring time. The editor is a thin Razor shell over a pure `SqlTagConfigModel` (`FromJson`/`ToJson`/`Validate`, preserves unknown keys) — copy the Modbus template under `Components/Shared/Uns/TagEditors/` + `Uns/TagEditors/`. The model exposes: `Model` (KeyValue/WideRow/ Query), `Table`, `KeyColumn`/`KeyValue`/`ValueColumn`/`TimestampColumn` (key-value), `ColumnName` + @@ -588,9 +612,14 @@ comment) and the env-overridable ConfigDb connection string: - **Never log the resolved connection string.** Log only provider + server host + database name. - Prefer integrated/managed auth where the estate supports it (`Integrated Security=true` / Azure AD / Kerberos) so no password transits config at all. -- The `.Browser` path receives the secret in form JSON over the authenticated Blazor circuit (same - boundary as every other driver browser + `TestDriverConnect`); it builds one transient connection - and releases it — **no `_lastConfigJson` cached field anywhere.** +- The `.Browser` path resolves `connectionStringRef` via the **same direct env read, in the AdminUI + process** (§4.4) — which is why split-role deployments need **env parity**: admin nodes carry the + same `Sql__ConnectionStrings__` vars as driver nodes for any browseable ref, and an unresolved + ref fails the browse open with a message naming the exact missing env var. A pasted ad-hoc literal + connection string travels only over the authenticated Blazor circuit (same boundary as every other + driver browser + `TestDriverConnect`), is used for that browse session only, and is never persisted + into the driver config or logged. Either way the browser builds one transient connection and + releases it — **no `_lastConfigJson` cached field anywhere.** ### 8.3 Timeout / frozen-peer (high, risk #3) @@ -599,6 +628,14 @@ Per §3.2: `CommandTimeout` (server-side backstop) **and** `ExecuteReaderAsync(l R2-01 S7 blackhole finding. A frozen DB surfaces `BadTimeout` per group + degrades the driver + backs off the poll loop; it must never wedge a poll thread. Asserted by the blackhole live-gate (§9). +**Timeout-ordering rule (authoring validation).** `operationTimeout` **must be strictly greater than** +`commandTimeout` — the defaults (15 s > 10 s) encode this deliberately. The driver-page / +`TagConfigValidator`-level check **rejects** (or at minimum warns loudly on) +`operationTimeout <= commandTimeout`: inverting the order means the client-side abort always fires +first and **masks the server-side backstop** — every server-side timeout would surface as a generic +client cancellation instead of the `CommandTimeout` failure it actually is, hiding the diagnostic +that distinguishes "slow query/lock wait" from "driver gave up". + ### 8.4 Connection pooling / exhaustion (medium) Open-use-dispose per poll (`await using` on both `DbConnection` and `DbDataReader` — never leak a diff --git a/docs/plans/2026-07-15-universal-discovery-browser-design.md b/docs/plans/2026-07-15-universal-discovery-browser-design.md index 37a18e9b..a5b5f05d 100644 --- a/docs/plans/2026-07-15-universal-discovery-browser-design.md +++ b/docs/plans/2026-07-15-universal-discovery-browser-design.md @@ -184,7 +184,18 @@ line in the program doc's cross-cutting rules when P1 lands). Serves entirely from the in-memory captured tree (the shape the MTConnect research report proposed for its cached-`/probe` session — now subsumed by this generic component): `RootAsync` = top-level nodes; `ExpandAsync(nodeId)` = the captured children of that node; `AttributesAsync(nodeId)` = the leaf's `AttributeInfo` (+ any captured properties). -`Token`/`LastUsedUtc` per the interface; `DisposeAsync` drops the tree. No I/O after open. +`Token`/`LastUsedUtc` per the interface; `DisposeAsync` drops the **session's reference** to +the tree (the tree is immutable and may be shared by coalesced sessions, §4.2 — disposing one +session never affects another). No I/O after open. + +**Point-in-time snapshot + Refresh.** The captured tree is a snapshot taken at open time and +never updates for the session's whole TTL — a tag added on the controller after open does not +appear. The picker therefore exposes a **Refresh** action = close the session + re-open it +(`CloseAsync` then `OpenAsync` with the same driverType/configJson): a fresh capture that flows +through the same coalescing/cap path as any open. Because the coalescing map holds in-flight +captures only (no result caching), Refresh always yields a new capture — or joins one already +in flight, which is equally fresh. Cheap to build (the picker already has both calls); the UI +should label browse results as "snapshot taken at open/refresh time". ## 5. The online-discovery capability gate @@ -276,9 +287,10 @@ turns the FixedTree on; the settle waits for its cache to fill.) 1. **Eager, not lazy.** `DiscoverAsync` runs the whole enumeration at open. For huge trees (ControlLogix 10 k+ tags, OPC UA 100 k nodes) the node-cap truncates and the UX is worse than a bespoke per-click `ExpandAsync`. Graduate those to Tier 2. -2. **Runs the real driver in-process** (construct + connect + one-shot discover + shutdown) — - heavier than the lightweight ad-hoc bespoke session, and requires the driver assemblies in the - host (fused Host: yes). +2. **Runs the real driver in-process** (construct + connect + one-shot discover + bounded + shutdown) — heavier than the lightweight ad-hoc bespoke session, and requires the driver + assemblies in the host (fused Host: yes). The §4.2 coalescing + `MaxConcurrentCaptures` cap + bound the aggregate load, but each individual capture is still a full device walk. 3. **No incrementally-fetched attribute panel** beyond what `DriverAttributeInfo` + `AddProperty` carry (fine for the picker's needs; Galaxy's richer two-stage pick stays bespoke). @@ -288,10 +300,29 @@ turns the FixedTree on; the settle waits for its cache to fill.) known Folder/Variable/AddProperty/alarm graph; assert the `BrowseNode` tree, leaf `NodeId == FullName`, `AttributeInfo` mapping, node-cap truncation, and the no-op alarm sink. - **Unit — `DiscoveryDriverBrowser`:** fake `IDriverFactory` returning a fake driver; assert - Initialize→Discover→Shutdown ordering, open-timeout, `PatchForBrowse` merge, `CanBrowse` gate + Initialize→Discover→Shutdown ordering (shutdown always runs, even on discovery failure), + open-timeout, `PatchForBrowse` merge, `CanBrowse` gate (true for `SupportsOnlineDiscovery`, false otherwise / `TryCreate`→null), and the `UntilStable` settle (a fake `UntilStable` driver whose tree only fills on the second discovery pass must yield the full tree; a never-stable one must stop at the open-timeout). +- **Unit — capture coalescing:** two concurrent `OpenAsync` calls with the same + (driverType, configJson) run **one** capture (fake driver counts Initialize/Discover + invocations) and both callers receive **distinct session tokens** over the shared tree; + disposing one session leaves the other browsable; a failed capture fails both awaiters; a + subsequent open after completion (Refresh path) runs a **new** capture (in-flight-only map, + no result caching). +- **Unit — capture cap queueing:** with `MaxConcurrentCaptures = 4` and >4 concurrent opens on + **distinct** keys (fake drivers gated on a test signal), at most 4 captures are in flight at + once; queued opens complete after a slot frees; a queued open observes caller cancellation. +- **Unit — `CanBrowse` hardening:** a fake factory whose `TryCreate` **throws** on malformed + configJson yields `CanBrowse == false` (no exception escapes); when `TryCreate` returns a + non-discovery (or flag-false) instance, `CanBrowse` is false **and** the created instance + was shut down/disposed. +- **Unit — bounded shutdown:** a fake driver whose `ShutdownAsync` never completes — the open + still returns within open-timeout + the 10 s shutdown bound, on the **discovery** outcome, + for both legs: successful capture ⇒ a usable session is returned; failed discovery ⇒ the + discovery error (not a shutdown timeout) surfaces. The wedged instance is abandoned, not + awaited. - **Unit — `BrowserSessionService` resolution:** bespoke-first, universal-fallback, and manual-entry-when-neither; confirm no `ToDictionary` collision. - **Integration (fixture-gated):** point the universal browser at the existing driver fixtures on @@ -306,9 +337,12 @@ turns the FixedTree on; the settle waits for its cache to fill.) - **P1 — universal browser end-to-end:** `CapturingAddressSpaceBuilder` + `CapturedTreeBrowseSession` + `DiscoveryDriverBrowser` + `IUniversalDriverBrowser` + `BrowserSessionService` fallback + `ITagDiscovery.SupportsOnlineDiscovery` default member + - the AbCip/TwinCAT/FOCAS patches + the `UntilStable` settle loop (§4.2) + DI line + AdminUI - `CanBrowse`→Browse-button gate. Set `SupportsOnlineDiscovery=true` on - OpcUaClient(n/a—bespoke), AbCip, TwinCAT, FOCAS. **~3–5 days.** + the AbCip/TwinCAT/FOCAS patches + the `UntilStable` settle loop (§4.2) + capture coalescing + and the `MaxConcurrentCaptures` cap + the bounded (10 s) shutdown + hardened `CanBrowse` + (catch-throwing-`TryCreate`, defensive teardown) + DI line + AdminUI + `CanBrowse`→Browse-button gate + the picker **Refresh** action (§4.3). Set + `SupportsOnlineDiscovery=true` on OpcUaClient(n/a—bespoke), AbCip, TwinCAT, FOCAS. + **~3–5 days.** - **P2 — light up new drivers for free:** as MTConnect/BACnet land, each sets `SupportsOnlineDiscovery=true` (+ patch if config-gated) and gets a working picker with no browser code. (MQTT/Sparkplug and SQL poll ship bespoke browsers instead — program doc §4.)