# S7 Driver — Implementation Plan > Source of gap analysis: [featuregaps.md → S7](../featuregaps.md#s7-siemens-s7-3004001200--1500) > > Covers Build = Yes items only. Skip-rated rows are noted at the end for context. ## Summary The S7 driver (`src/ZB.MOM.WW.OtOpcUa.Driver.S7/`) ships a working scaffold over **S7netplus 0.20**: ISO-on-TCP / S7comm, single-connection-per-PLC (`SemaphoreSlim`), DB / M / I / Q / T / C address parsing, atomic scalar reads/writes for Bool / Byte / I16 / U16 / I32 / U32 / F32, polled `ISubscribable` overlay, `IHostConnectivityProbe` via `ReadStatusAsync`, and a Snap7-server-backed CI fixture on `localhost:1102`. The 16 Build = Yes gaps fall into six tractable phases. **The hard one is gap #1 (S7-1500 Optimized DB / Symbolic addressing)** — S7netplus speaks classic S7comm only and cannot reach optimized DBs at all. Phase 6 calls that out as an explicit architectural decision: ship the constraint as documentation and the rest as S7netplus-compatible features, *or* fork to a library that supports S7Plus (Sharp7-fork, Snap7 v2, custom S7Plus). Phases 1-5 do not depend on that decision and are landable on the current S7netplus base. Every PR ships unit-test coverage and — where wire semantics matter — extends the Snap7-server profile in `Docker/server.py` so the integration fixture exercises the new path. PRs that need real S7-1500 firmware features the simulator doesn't mimic (PUT/GET protection, password-tier auth, SZL diagnostic buffer) call that out and gate the live-firmware test on the dev-box S7-1500 lab rig. Architectural invariants we explicitly preserve: - Single connection per PLC; `_gate` (SemaphoreSlim) serializes every PDU. - Strict address-parse-at-init; bad config fails fast with `FormatException`. - PUT/GET-disabled mapped to sticky `BadDeviceFailure`, not Polly-retried. - 100 ms minimum publishing interval (matches CPU mailbox scan reality). - `WriteIdempotent` per-tag flag is the only retry-policy lever. ## Phased delivery | Phase | Theme | PRs | Gaps closed | |------:|-------|-----|-------------| | 1 | Data-type correctness | PR-S7-A1..A5 | #7, #8, #9, #19 | | 2 | Performance — multi-tag PDU packing | PR-S7-B1..B2 | #3, #22 | | 3 | Operability knobs | PR-S7-C1..C5 | #2, #4, #20, #21, #24 | | 4 | Workflow — symbol import + UDTs | PR-S7-D1..D3 | #5, #6, #10 | | 5 | Diagnostics & security | PR-S7-E1..E2 | #11, #14 | | 6 | S7-1500 Optimized DB / Symbolic | PR-S7-F (decision) | #1 | Phases 1-3 run sequentially because Phase 2 packing and Phase 3 deadbands are both keyed off the type-decode work in Phase 1. Phase 4 (UDT/symbol import) is parallelizable with Phase 5; Phase 6 is gated on the library-choice decision in Open Questions (a). --- ## Per-PR detail ### Phase 1 — Data-type correctness #### PR-S7-A1 — 64-bit scalar types (LInt / ULInt / LReal / LWord) Closes gap #9. `Float64`/`Int64`/`UInt64` cases in `S7Driver.ReadOneAsync`/ `WriteOneAsync` currently throw `NotSupportedException`. - **Files**: `S7Driver.cs` (read + write switch), `S7DriverOptions.cs` (extend `S7Size` with `LWord` for 8-byte access), `S7AddressParser.cs` (accept `DBL` / `LD` size suffix; S7netplus encodes 8-byte access via byte-array reads, so the parser converts `DB1.LD0` to a byte-range read internally). - **Tests**: unit decode tests for the byte-pattern → `long` / `ulong` / `double` conversion; Snap7-server profile gets `f64` and `i64` seed types. - **Risks**: S7netplus's `ReadAsync(string)` does not accept `LD` natively; fallback path is `Plc.ReadBytes(DataType.DataBlock, db, byteOffset, 8)` then `BitConverter` with explicit endian flip (S7 is big-endian on the wire, `BitConverter` is little-endian on x86/x64). - **Effort**: M (3-4 days incl. tests). - **Deps**: none. - **Docs / fixture / e2e**: extends the type-mapping table in `docs/v2/s7.md` with `LInt` / `ULInt` / `LReal` / `LWord` rows; adds the new sizes (`LInt`, `ULInt`, `LReal`) to the `read` / `write` cookbook in `docs/Driver.S7.Cli.md`; updates `docs/drivers/S7-Test-Fixture.md` §"What it actually covers" to list the new 64-bit types and removes them from §5 "Data types beyond the scalars"; extends the snap7 seed-type set in `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/server.py` with `i64`, `u64`, `f64` cases; adds seeds at known offsets (e.g. `DB1.DBL40` for i64, `DB1.DBL48` for f64) to `Docker/profiles/s7_1500.json`; adds `S7_1500Profile` constants for the new tags + a `Driver_reads_seeded_64bit_batch` smoke test in `S7_1500SmokeTests`; adds an LInt loopback assertion to `scripts/e2e/test-s7.ps1`. #### PR-S7-A2 — STRING / WSTRING / CHAR / WCHAR Closes gap #8 (string portion). S7 `STRING(n)` is `[max-len][actual-len][bytes...]` (2-byte header + ASCII). `WSTRING(n)` is 4-byte header + UTF-16BE bytes. `CHAR` is 1 byte; `WCHAR` is 2 bytes UTF-16BE. - **Files**: `S7Driver.cs` (new `ReadStringAsync` / `WriteStringAsync` private helpers using `Plc.ReadBytes` for raw byte-range fetch), `S7DriverOptions.cs` (already has `StringLength`; add `S7DataType.WString`, `Char`, `WChar`). - **Tests**: unit tests for header parsing including the "actual-len > max-len" PLC bug case (clamp on read, reject on write); Snap7 `ascii` seed type already exists, add `wstring` seed. - **Risks**: write must respect the configured `StringLength` to avoid overrunning the DB; mismatched max-len is a common field bug. - **Effort**: M. - **Deps**: PR-S7-A1 (byte-range read helper lands there). - **Docs / fixture / e2e**: extends the type-mapping section in `docs/v2/s7.md` with `STRING(n)` / `WSTRING(n)` / `CHAR` / `WCHAR` layouts (2-byte vs 4-byte header, UTF-16BE encoding, the "actual-len > max-len" PLC bug); extends the `read` / `write` cookbook in `docs/Driver.S7.Cli.md` with `--type WString` / `--type Char` / `--type WChar` examples and the `--string-length` flag for WString; updates `docs/drivers/S7-Test-Fixture.md` §"What it actually covers" to list ascii/wstring/char/wchar; adds `wstring`, `char`, `wchar` seed types to `Docker/server.py` (existing `ascii` covers STRING); seeds a `DB1.WSTRING[256]` and a `DB1.CHAR[300]` in `Docker/profiles/s7_1500.json`; adds `Driver_round_trips_string_types` smoke test exercising read + write of every variant; adds a string round-trip assertion to `scripts/e2e/test-s7.ps1`. #### PR-S7-A3 — DTL / DATE_AND_TIME / S5TIME / TIME / TOD / DATE Closes gap #8 (date/time portion). - DTL is 12 bytes: year(u16) / month / day / weekday / hour / minute / second / nanos(u32). - DATE_AND_TIME (DT) is 8 bytes BCD: yy mm dd hh mm ss msH msL+dow. - S5TIME is 16-bit BCD with a 2-bit time-base. - TIME is `Int32` ms since 1972 (S7-300/400) or signed-ms duration (S7-1200/1500). - TOD is `UInt32` ms since midnight; DATE is `UInt16` days since 1990-01-01. - **Files**: `S7Driver.cs` + new `S7DateTimeCodec.cs` static class encapsulating every encode/decode (keep the driver lean; codec is unit-testable in isolation). - **Tests**: round-trip tests per type with golden byte vectors taken from the Siemens "STEP 7 V18 — Programming Reference" document. Snap7-server seed profile gains `dtl`, `dt`, `s5time`, `time` types. - **Risks**: BCD parsing must reject invalid month/day combinations; PLC programs occasionally write 0x00 0x00 ... when uninitialized — surface as `BadOutOfRange` rather than parsing to year 0. - **Effort**: L (4-5 days incl. all six types and the golden-vector suite). - **Deps**: PR-S7-A1. - **Docs / fixture / e2e**: extends `docs/v2/s7.md` with a new "Date / time types" subsection documenting DTL / DT (BCD) / S5TIME / TIME / TOD / DATE byte layouts and the S7-300/400 vs S7-1200/1500 TIME-encoding split; adds `--type Dtl` / `--type DateAndTime` / `--type S5Time` / `--type Time` / `--type TimeOfDay` / `--type Date` to the `docs/Driver.S7.Cli.md` cookbook; updates `docs/drivers/S7-Test-Fixture.md` §"What it actually covers" with the new datetime types and removes "DTL / DATE_AND_TIME" from §5 "Data types beyond the scalars"; adds `dtl`, `dt`, `s5time`, `time`, `tod`, `date` seed types to `Docker/server.py` with golden-byte vectors documented in comments; seeds `DB1.DTL[260]`, `DB1.DT[272]`, `DB1.S5TIME[280]`, `DB1.TIME[284]`, `DB1.TOD[288]`, `DB1.DATE[292]` in `Docker/profiles/s7_1500.json`; adds `S7DateTimeCodecTests` (unit) + `Driver_round_trips_datetime_types` smoke test; no `scripts/e2e/test-s7.ps1` change required (CLI cookbook examples cover the manual surface). #### PR-S7-A4 — Array tags (ValueRank=1) Closes gap #7. `S7TagDefinition` currently has no array dimension; `MapDataType` hard-codes `IsArray: false`. - **Files**: `S7DriverOptions.cs` (extend `S7TagDefinition` with `ArrayDim` int? and `ElementCount` int?), `S7Driver.cs` (read path: detect array tag, issue one byte-range read covering N elements, slice client-side; write path: same in reverse), `DiscoverAsync` reports `IsArray: true, ArrayDim: [N]`. - **Tests**: unit tests for `Array[0..9] of Int` and `Array[0..9] of Real`; Snap7-server profile adds an array seed type. Round-trip array-write test proves slice ordering. - **Risks**: S7-1500 supports multi-dim arrays; declare ValueRank=1 only and document multi-dim as a follow-up. Array-of-UDT lands with PR-S7-D2. - **Effort**: M. - **Deps**: PR-S7-A1 (byte-range reads). - **Docs / fixture / e2e**: adds an "Array tags (ValueRank=1)" subsection to `docs/v2/s7.md` documenting `Array[0..N]` syntax + the multi-dim follow-up note; extends `docs/Driver.S7.Cli.md` with an `--array-count N` flag in the `read` / `write` cookbook and worked examples for `Array[0..9] of Int` and `Array[0..9] of Real`; updates `docs/drivers/S7-Test-Fixture.md` §"What it actually covers" to list array round-trips and removes "arrays of structs" from §5 (struct arrays land in PR-S7-D2); extends `Docker/server.py` with an `array` meta-seed-type that takes an inner-type + count and lays out N elements contiguously; seeds `DB1.ArrayInt[300]` (10×Int) and `DB1.ArrayReal[320]` (10×Real) in `Docker/profiles/s7_1500.json`; adds `Driver_round_trips_array_int10` + `Driver_round_trips_array_real10` smoke tests proving slice ordering; adds an array round-trip assertion to `scripts/e2e/test-s7.ps1`. #### PR-S7-A5 — LOGO! 8 + S7-200 V-memory area Closes gap #19. `S7AddressParser` currently rejects the `V` area letter. - **Files**: `S7AddressParser.cs` (add `V` case → maps to `S7Area.DataBlock` with `DbNumber=1` for S7-200 / DbNumber per LOGO! VM-mapping table; document the conversion), `S7DriverOptions.cs` (note CpuType-dependent meaning of V). - **Tests**: unit tests for `VW0` / `VD4` / `V0.0` parsing, both S7-200 and LOGO! conventions; document caller responsibility to set `CpuType.S7200` or `S7200Smart`. - **Risks**: LOGO! VM base address differs by firmware (V0=0 vs V0=1024 depending on block); document the offset table rather than auto-detecting. - **Effort**: S (1-2 days, mostly parser + tests; no wire changes). - **Deps**: none. - **Docs / fixture / e2e**: adds a "LOGO! 8 / S7-200 V-memory" subsection to `docs/v2/s7.md` covering the `V` area letter, the `S7200` / `S7200Smart` CpuType pre-requisite, the LOGO! VM-mapping table by firmware band, and the "V0 = DB1.DBX0.0" semantic; extends the address grammar cheat sheet in `docs/Driver.S7.Cli.md` with `VW0` / `VD4` / `V0.0` rows and a `-c S7200Smart` worked example; updates `docs/drivers/S7-Test-Fixture.md` §"What it does NOT cover" item 4 to note S7-200 / LOGO! parser coverage now exists at unit level; adds unit-only `S7AddressParserTests` cases — no Snap7 fixture change (server.py already exposes DB1, which is where V-memory aliases land); no `scripts/e2e/test-s7.ps1` change required (live-LOGO! testing is documented as field-only). ### Phase 2 — Performance (multi-tag PDU packing + block coalescing) #### PR-S7-B1 — Multi-variable PDU packing Closes gap #3. `ReadAsync(IReadOnlyList)` currently issues one `plc.ReadAsync` per tag inside the semaphore — N PDUs for N tags. - **Files**: `S7Driver.cs` (replace per-tag loop with a packer that builds a list of `S7.Net.Types.DataItem`, calls `plc.ReadMultipleVarsAsync`, then fans the results back to the per-tag decoder). Keep the existing per-tag decode switch — only the wire fetch becomes batched. - **Tests**: integration test that subscribes to 100 tags and asserts the packet count seen by the Snap7 server is 1 (or N / packing-budget) rather than 100. Unit-level test covers packer chunking when the negotiated PDU size won't fit all items. - **Risks**: `ReadMultipleVarsAsync` errors are per-item; we must surface per-tag StatusCodes correctly rather than failing the whole batch on one bad tag. Packing budget = `negotiatedPduSize - 18 (header) - per_item(12)`, conservatively cap at 19 items per PDU on a 240-byte PDU. - **Effort**: L (5-6 days incl. the per-item-error fan-out semantics). - **Deps**: Phase 1 PRs do not block this — but conflicts in `S7Driver.cs` are likely, so land Phase 1 first. - **Docs / fixture / e2e**: adds a "Performance — multi-variable PDU packing" subsection to `docs/v2/s7.md` describing `ReadMultipleVarsAsync`, the negotiated-PDU packing budget formula (`pdu - 18 - 12·N`), the 19-items-per-240-byte-PDU rule of thumb, and the per-item-error semantics; no `docs/Driver.S7.Cli.md` change (CLI is single-tag); no Snap7-server seed change required (existing seeds cover the wire path); adds `S7MultiVarPduPackingTests` to the unit suite (planner chunking when items don't fit) + a 100-tag perf integration test `Driver_packs_100_tags_into_minimum_pdus` that asserts request-count reduction; no `scripts/e2e/test-s7.ps1` change required. #### PR-S7-B2 — Block-read coalescing for contiguous DBs Closes gap #22. Reading `DB1.DBW0`, `DB1.DBW2`, `DB1.DBW4` should issue one 6-byte byte-range read against DB1 starting at offset 0, sliced client-side. - **Files**: `S7Driver.cs` adds a planner pass: group same-DB tags by contiguous byte ranges (gap-merge threshold = configurable, default 16 bytes; over-fetching 16 bytes is cheaper than one extra PDU). Merged ranges become a single `Plc.ReadBytes` call; the result is sliced per-tag. - **Tests**: unit tests for the merge planner (input list → expected ranges); integration test with 50 contiguous DB words proves wire-level reduction. - **Risks**: STRINGs / arrays should opt out of merging because the per-tag byte size is variable. Add an "opaque-size" flag so the planner skips them. - **Effort**: M. - **Deps**: PR-S7-B1 (the multi-var packer). The two interact: the planner emits sum-reads, then the packer puts multiple sum-reads on one PDU. - **Docs / fixture / e2e**: extends the §"Performance" section in `docs/v2/s7.md` with a "Block-read coalescing" subsection — the default 16-byte gap-merge threshold, the opaque-size opt-out for STRINGs / arrays, and operator guidance for tuning the threshold per DB; no CLI doc change; no Snap7-server seed change (existing contiguous DB1 seeds — DBW0 / DBW10 / DBD20 — already exercise contiguous-merge); adds `S7BlockCoalescingPlannerTests` (unit) covering the merge planner + opaque opt-out; adds a 50-contiguous-DBW integration test `Driver_coalesces_contiguous_DBWs_into_single_byte_range_read` that asserts wire-level reduction; no `scripts/e2e/test-s7.ps1` change. ### Phase 3 — Operability #### PR-S7-C1 — PDU size negotiation surfaced Closes gap #2. S7netplus's `Plc` instance exposes the negotiated PDU size after `OpenAsync` via `Plc.MaxPDUSize`. - **Files**: `S7Driver.cs` (read `Plc.MaxPDUSize` after open, store on `_health`; expose via `GetHealth().Diagnostics["NegotiatedPduSize"]` — this requires adding a `Diagnostics` dictionary to `DriverHealth`, which is a Core change). Operator-visible via the Admin UI driver-diagnostics panel that already renders Modbus diagnostic stats. - **Tests**: integration test asserts the value is non-zero after init. - **Risks**: `DriverHealth` extension must be backward-compatible — existing drivers should still compile against the unchanged record. Make the new property nullable with a default of `null`. - **Effort**: S. - **Deps**: Core `DriverHealth` shape change (single PR coordinated with the Modbus diagnostic surface). - **Docs / fixture / e2e**: adds a "Diagnostics surfacing" subsection to `docs/v2/s7.md` documenting the `Diagnostics["NegotiatedPduSize"]` surface + how it renders in the Admin UI driver-diagnostics panel; no CLI doc change (CLI doesn't expose diagnostics); updates `docs/drivers/S7-Test-Fixture.md` §"What it actually covers" with a "negotiated PDU size surfaces in driver health" line; no Snap7 seed-type change (snap7's PDU negotiation is fixed at 240 bytes — document the fixture's negotiated size in the README); adds `Driver_exposes_negotiated_pdu_size_post_init` smoke test asserting the value is non-zero; no `scripts/e2e/test-s7.ps1` change. #### PR-S7-C2 — TSAP / Connection Type selector Closes gap #4. S7netplus picks PG-class TSAPs by default; hardened CPUs may require OP / S7-Basic / Other. - **Files**: `S7DriverOptions.cs` (new `TsapMode` enum: `Auto` / `Pg` / `Op` / `S7Basic` / `Other`; `Auto` preserves current behavior. Optional `LocalTsap` / `RemoteTsap` `ushort?` for explicit override). `S7Driver.cs` branches on the mode to pick the S7netplus `Plc(CpuType, ...)` constructor vs the `Plc(string ip, byte rack, byte slot, ushort localTsap, ushort remoteTsap)` raw-TSAP overload. Document the raw-TSAP table in `docs/v2/s7.md`. - **Tests**: unit test on the mode → TSAP-byte mapping; live-firmware test documented but only runnable against the dev-box S7-1500 lab rig. - **Risks**: wrong TSAP causes connection refused at handshake — same failure shape as wrong slot. Document the mapping prominently. - **Effort**: M. - **Deps**: none. - **Docs / fixture / e2e**: adds a "TSAP / Connection Type" section to `docs/v2/s7.md` covering the `TsapMode` enum, the raw-TSAP table (PG = 0x0100/0x0102, OP = 0x0200/0x0202, S7-Basic = 0x0300/0x0302, Other = caller-supplied), and the hardened-CPU motivation; adds `--tsap-mode` and `--local-tsap` / `--remote-tsap` flags to `docs/Driver.S7.Cli.md`'s common-flags table with a worked example hitting an OP-class TSAP; no Snap7 seed change (snap7 accepts any TSAP from the CLI, so the unit-level mapping test is sufficient); no smoke test change (live-firmware-only); no `scripts/e2e/test-s7.ps1` change. #### PR-S7-C3 — Per-tag scan group / publish rate Closes gap #20. `SubscribeAsync` takes one publishing interval for the whole list; mixed 100 ms / 1 s / 10 s tags need three subscribe calls today. - **Files**: `S7DriverOptions.cs` (extend `S7TagDefinition` with optional `ScanGroup` string). `S7Driver.cs` (`SubscribeAsync` partitions the input list into one poll loop per distinct interval; `PollGroupEngine`-style internal group, but driver-local — same engine the TwinCAT driver uses). - **Tests**: unit test with three tags at three rates asserts three independent poll-tick streams; integration test asserts no group starves the others. - **Risks**: the `_gate` semaphore still serializes — three poll loops can contend. Document the contention as part of the "1 connection / 1 mailbox" invariant; if it bites, follow-up adds a fairness queue. - **Effort**: M. - **Deps**: none. - **Docs / fixture / e2e**: adds a "Per-tag scan groups" subsection to `docs/v2/s7.md` documenting `S7TagDefinition.ScanGroup`, the multi-rate partitioning semantics, and the `_gate` contention caveat; no CLI doc change (CLI is single-tag); no Snap7 seed change required (existing scalar seeds suffice); adds `S7ScanGroupPartitioningTests` (unit) + `Driver_three_scan_groups_publish_independently` smoke test that subscribes 3 tags at 100 ms / 1 s / 10 s rates and asserts independent tick streams; no `scripts/e2e/test-s7.ps1` change (subscribe assertion already covers the polling path). #### PR-S7-C4 — Deadband / on-change with thresholds Closes gap #21. `PollOnceAsync` currently does `!Equals(prev, current)` only — no analog deadband. - **Files**: `S7DriverOptions.cs` (extend `S7TagDefinition` with `DeadbandAbsolute double?` and `DeadbandPercent double?`). `S7Driver.cs` (`PollOnceAsync` evaluates per-tag deadband for numeric types; non-numeric types fall through to exact equality). - **Tests**: unit tests for absolute and percent deadbands at edge cases (NaN, ±Infinity, sign flip, near-zero percent). - **Risks**: percent deadband against a zero baseline diverges; document and fall back to absolute when |baseline| < 1e-6. - **Effort**: S. - **Deps**: PR-S7-C3 helpful but not required. - **Docs / fixture / e2e**: adds a "Deadband / on-change" subsection to `docs/v2/s7.md` documenting `DeadbandAbsolute` / `DeadbandPercent` per tag, NaN / ±Infinity / sign-flip / near-zero-percent edge cases, and the |baseline| < 1e-6 fallback; no CLI doc change (CLI's `subscribe` already polls on change); no Snap7 seed change; adds `S7DeadbandTests` (unit) covering all edge cases — no integration test required since deadband is pre-publish filtering inside the polling loop; no `scripts/e2e/test-s7.ps1` change. #### PR-S7-C5 — Pre-flight PUT/GET enablement test Closes gap #24. We currently surface `BadDeviceFailure` only at first read. Add a pre-flight check during `InitializeAsync` (after `OpenAsync`) that issues one trivial read (`MW0` or the configured `Probe.ProbeAddress`) and surfaces the dedicated diagnostic message before declaring `DriverState.Healthy`. - **Files**: `S7Driver.cs` (`InitializeAsync` adds the probe read; on `S7.Net.PlcException` with the PUT/GET-disabled error code, throw a typed `S7PutGetDisabledException` with a configuration-fix hint). - **Tests**: integration test toggles a Snap7 simulator quirk that mimics the PUT/GET-disabled response (Snap7 doesn't model this; gate the test on a `--with-real-plc` opt-in or document as live-firmware-only). - **Risks**: pre-flight against a real `Probe.ProbeAddress` requires the address to exist in the PLC; document that the default `MW0` is fine for most installs but allow `null` / "skip" for sites that haven't wired one. - **Effort**: S. - **Deps**: none. - **Docs / fixture / e2e**: extends the "PUT/GET must be enabled" section of `docs/Driver.S7.Cli.md` with the new typed `S7PutGetDisabledException` message + the "skip pre-flight" knob; adds the same content as a "Pre-flight PUT/GET enablement" subsection in `docs/v2/s7.md`; no Snap7 seed change (snap7 doesn't model PUT/GET-disabled — the test for the success path uses the existing MW0 seed); adds `Driver_preflight_passes_when_probe_address_seeded` smoke test; documents the live-firmware test as gated on a `--with-real-plc` opt-in flag in `docs/drivers/S7-Test-Fixture.md` §"Follow-up candidates"; no `scripts/e2e/test-s7.ps1` change (probe test already runs first). ### Phase 4 — Workflow (symbol import + UDTs + instance DBs) #### PR-S7-D1 — Symbol-table / TIA Portal export browse Closes gap #5. Operators currently hand-edit `S7TagDefinition` JSON. TIA Portal exports symbols as **`.s7p` archive → External tags → CSV / SDF**. The lighter target is the CSV format used by the "Generate source from blocks" exporter. - **Files**: new `src/ZB.MOM.WW.OtOpcUa.Driver.S7/SymbolImport/` directory: - `TiaCsvImporter.cs` — parses TIA Portal "Show all tags" CSV (`Name`, `Address`, `Data type`, `Comment`, `Visible in HMI`). Output: list of `S7TagDefinition`. - `AwlImporter.cs` — best-effort AWL `VAR_GLOBAL` / `DATA_BLOCK` parser for legacy STEP 7 Classic projects. - **Files (Admin UI)**: a "Import S7 symbols" button on the Driver Tags tab that POSTs the file to a new `POST /api/drivers/{id}/import-s7-symbols` endpoint and reports the diff. - **Tests**: unit tests with golden-input CSV / AWL fixtures; round-trip test that imports → produces tags → reads against simulator. - **Risks**: TIA Portal CSV is locale-dependent (decimal-comma in DE locale). Detect from the header row and accept both. UDT-typed symbols import as a placeholder until PR-S7-D2. - **Effort**: L (5-7 days incl. the Admin UI flow). - **Deps**: see Open Question (c) — confirm CSV+AWL is the right scope, or whether `.s7p` / `.zip` archive parsing is required. - **Docs / fixture / e2e**: adds new doc `docs/drivers/S7-TIA-Import.md` documenting the supported TIA Portal CSV format (column names, locale-comma detection, UDT-typed placeholders) and the AWL `VAR_GLOBAL` / `DATA_BLOCK` parser scope; cross-links it from `docs/v2/s7.md`'s new "Symbol import" section and from `docs/Driver.S7.Cli.md` with a future `import` subcommand hook; adds golden-input fixtures `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Fixtures/sample_tia_export.csv`, `sample_tia_export_de_locale.csv`, and `sample_step7_classic.awl`; no Snap7 seed change required (existing DB1 seeds support the import-then-read round-trip); adds `TiaCsvImporterTests` and `AwlImporterTests` (unit) + `Driver_imports_csv_then_reads_seeded_tags` integration test that imports the sample CSV → reads via Snap7; no `scripts/e2e/test-s7.ps1` change (Admin-UI flow has its own end-to-end coverage in the Admin UI test suite). #### PR-S7-D2 — UDT / STRUCT / nested-DB handling Closes gap #6. Today's tag map is flat scalar-only; UDT-typed DBs are unusable without hand-flattening every member. - **Files**: `S7DriverOptions.cs` (extend `S7TagDefinition` with `UdtName string?`; alongside, a new `IReadOnlyList Udts` on the options that declares the layout: name, ordered members `(Name, Offset, S7DataType, ArrayDim?)`). `S7Driver.cs` fans a UDT-typed tag into per-member sub-tags at `InitializeAsync`, so the read/write path stays scalar-only. - **Tests**: unit tests for fan-out with nested UDTs (UDT-of-UDT); integration test with a Snap7 DB seeded as a UDT-shape byte array proves the fan-out decodes correctly. - **Risks**: UDT-of-UDT arbitrary nesting depth — cap at 4 levels and reject deeper with a clear error. Optimized DBs would let TIA reorder members, re-introducing gap #1; document that user-defined UDTs require "Optimized block access" off, same as the general DB rule. - **Effort**: L (1-2 weeks). - **Deps**: PR-S7-D1 (symbol importer drops UDT-typed entries with a placeholder; D2 makes those usable). - **Docs / fixture / e2e**: adds a "UDT / STRUCT support" section to `docs/v2/s7.md` documenting `S7UdtDefinition`, the fan-out semantics, the 4-level nesting cap, and the "Optimized block access must be off" prerequisite; extends `docs/drivers/S7-TIA-Import.md` (created in PR-S7-D1) with a UDT-typed-entry section showing how the importer + `Udts` declaration cooperate; updates `docs/drivers/S7-Test-Fixture.md` §"What it does NOT cover" item 5 to remove "UDT fan-out"; extends `Docker/server.py` with a `udt_layout` meta-seed-type that lays out per-member offsets within a DB byte range; seeds a `DB1.MyUdt[400]` (e.g. Real + Int + Bool) in `Docker/profiles/s7_1500.json`; adds `S7UdtFanOutTests` (unit) + `Driver_fans_out_udt_into_member_tags` integration test covering a nested-UDT case; adds a UDT-member round-trip assertion to `scripts/e2e/test-s7.ps1`. #### PR-S7-D3 — Instance-DB / FB parameter access Closes gap #10. Multi-instance FBs are addressed symbolically (`MyFB_Instance.MyParam`) with no fixed absolute DB byte offset visible without a TIA project export. - **Files**: extends PR-S7-D1's importer to recognize "instance DB" entries (TIA export shows them with a different "DB type" column value); the importer translates `MyFB_Instance.MyParam` to the resolved `DBn.DBW_offset` based on the FB's interface declaration in the export. - **Tests**: golden-input test with an FB-instance DB export; resolved addresses match Siemens reference. - **Risks**: when the FB interface changes (TIA "online change"), instance-DB layouts shift. Document that re-import is required after any FB-interface edit. Eventually surface this as a startup warning when the symbol-table hash differs from the imported snapshot — out of scope for this PR. - **Effort**: M. - **Deps**: PR-S7-D1, PR-S7-D2. - **Docs / fixture / e2e**: extends `docs/drivers/S7-TIA-Import.md` with an "Instance DBs / FB parameters" section covering the importer's `MyFB_Instance.MyParam` → `DBn.DBW_offset` resolution, the "DB type" column convention, and the "re-import on FB-interface edit" caveat; adds the same caveat as a paragraph in `docs/v2/s7.md`'s "UDT / STRUCT" section; adds a golden-input fixture `Fixtures/sample_tia_export_with_fb_instance.csv` to the integration tests; no Snap7 seed change required (resolved addresses land in DB1 which the existing seeds back); adds `InstanceDbResolverTests` (unit) + `Driver_resolves_fb_instance_then_reads_seeded_member` integration test; no `scripts/e2e/test-s7.ps1` change (FB-instance lookup is an import-time concern). ### Phase 5 — Diagnostics & security #### PR-S7-E1 — CPU diagnostic buffer / SZL reads Closes gap #11. SZL (System Status List) IDs surface CPU type, firmware version, cycle-time min/avg/max, and the diagnostic-buffer entries. - **Files**: `S7Driver.cs` exposes a small set of "system tags" alongside `Tags` — virtual addresses prefixed `@System.` that the read path recognizes and dispatches to S7netplus's `ReadSzlAsync` (or, if not exposed, a raw `Plc.ReadBytes` against the SZL-via-S7comm sub-protocol): - `@System.CpuType`, `@System.Firmware`, `@System.OrderNo` — SZL 0x0011 - `@System.CycleMs.Min` / `.Max` / `.Avg` — SZL 0x0132 / 0x0432 - `@System.DiagBuffer[0..N]` — SZL 0x00A0 ring-buffer entries - **Files (discovery)**: `DiscoverAsync` adds a `Diagnostics/` subfolder with the system-tag set when `S7DriverOptions.ExposeSystemTags = true`. - **Tests**: unit tests for the SZL response parser (golden bytes); live- firmware test against the dev-box S7-1500. - **Risks**: S7netplus's SZL surface is incomplete; may need a raw `Plc.ReadBytes` against `0x84` register or a small SZL-PDU helper. - **Effort**: M-L. - **Deps**: PR-S7-C1 (`DriverHealth.Diagnostics` dictionary already there). - **Docs / fixture / e2e**: adds a "CPU diagnostics (SZL)" section to `docs/v2/s7.md` listing the exposed `@System.*` virtual addresses, the underlying SZL IDs, and the `ExposeSystemTags` opt-in; extends `docs/Driver.S7.Cli.md` with a worked `read -a @System.CpuType` example in the cookbook; updates `docs/drivers/S7-Test-Fixture.md` §"What it does NOT cover" with a note that snap7 does not implement SZL — golden- byte unit tests cover the parser, live SZL is gated on a real S7-1500; no Snap7 seed change (snap7 returns a fixed handshake banner that the test checks for "SZL not supported on simulator" branch); adds `S7SzlParserTests` (unit) with golden bytes; documents the live SZL test in `docs/drivers/S7-Test-Fixture.md` §"Follow-up candidates"; no `scripts/e2e/test-s7.ps1` change. #### PR-S7-E2 — PLC password / protection-level handling Closes gap #14. S7-300/400 protection levels 1-3 and S7-1200/1500 connection mechanisms can require a password on connect. - **Files**: `S7DriverOptions.cs` (new `Password string?` and `ProtectionLevel` enum). `S7Driver.cs` calls S7netplus's `SetPassword` (if the API surfaces it — newer S7netplus versions ship `Plc.SendPassword(string)`; if not, raw-PDU fallback per Siemens "Communication Function Manual" §5.2). - **Tests**: live-firmware-gated; password-tier failure modes don't reproduce in Snap7. Unit-level coverage for the options-binding shape only. - **Risks**: S7netplus may not expose password auth — fallback is to call into the lower-level `S7.Net.S7Protocol` types or to fork. Land the options surface unconditionally, gate the wire path on library support, document the limitation if the library doesn't oblige. - **Effort**: M (S if S7netplus ships it; L if we need a fallback path). - **Deps**: none. - **Docs / fixture / e2e**: adds a "PLC password / protection levels" section to `docs/v2/s7.md` documenting the `Password` / `ProtectionLevel` options + the S7-300/400 levels 1-3 vs S7-1200/1500 connection-mechanism semantics + the "limitation if S7netplus doesn't ship `SendPassword`" note; adds a `--password` flag to `docs/Driver.S7.Cli.md`'s common-flags table with a hardened-CPU worked example; updates `docs/drivers/S7-Test-Fixture.md` §"What it does NOT cover" with a "password / protection levels not modelled by snap7" note; no Snap7 seed change (snap7 doesn't enforce protection levels); adds options-binding unit tests only — no integration test (live-firmware-only); no `scripts/e2e/test-s7.ps1` change. ### Phase 6 — S7-1500 Optimized DB / Symbolic addressing (decision PR) #### PR-S7-F — Optimized DB / S7Plus Closes gap #1. **This is an architectural decision PR, not a code PR.** S7netplus speaks classic S7comm only. Optimized DBs on S7-1500 (default for new TIA projects) reorder fields and have no fixed byte offsets — absolute `DB1.DBW0` reads return `BadDeviceFailure`. Three tracks: 1. **Document the constraint and stay on S7netplus.** Operators must uncheck "Optimized block access" in TIA Portal for any DB the driver reads. This is what the test fixture already documents. Effort: S (docs only). 2. **Migrate to a library that supports S7Plus.** - **Snap7 v2 / `Snap7Net`** — C-library wrapper, supports classic S7comm only (same limitation as S7netplus). Not a fix. - **Sharp7 fork** — community fork of Snap7 with **partial** S7-1200/1500 PUT/GET semantics. Still classic S7comm. - **Custom S7Plus implementation** — Wireshark dissector exists; reverse engineering is substantial. Effort: ≥ 4 weeks; ongoing protocol-version maintenance. Risk: Siemens has not published S7Plus. 3. **Embed an OPC UA → OPC UA bridge to the S7-1500's onboard OPC UA server.** The S7-1500 V2.5+ exposes its own OPC UA server with full symbolic access. Our `OPC UA Client driver` (already shipping per memory) could read the target CPU's OPC UA server and re-publish — sidesteps S7Plus entirely. Effort: S; semantics: requires the customer to license Siemens OPC UA on the CPU. Most modern S7-1500 deployments already license it. **Recommendation**: ship Track 1 docs immediately (closes the operator expectation gap) and Track 3 as the Optimized-DB workflow path (re-uses existing OPC UA Client driver). Track 2 (S7Plus reverse-engineering) is out of scope unless a customer pays for it. - **Files**: `docs/v2/s7.md` (Optimized DB section + how to disable), `docs/featuregaps.md` row #1 updated to reflect the Track 1+3 decision. - **Tests**: live-firmware test against the dev-box S7-1500 with optimized block access toggled both ways, asserting `BadDeviceFailure` vs successful read. - **Risks**: Track 3's OPC-UA-Client-bridging needs Admin UI plumbing to configure; that's a larger workstream tracked separately. - **Effort**: S (docs + decision); L if Track 2 is taken. - **Deps**: Open Question (a) below. - **Docs / fixture / e2e**: rewrites `docs/v2/s7.md` to land a prominent "Optimized DB constraint" section at the top — explicitly documents the S7-1200 V4.0+ / S7-1500 default, the `BadDeviceFailure` shape on absolute `DB1.DBW0` reads against an optimized DB, the "Uncheck Optimized block access in TIA Portal" fix, and the recommended **bridge-via-OpcUaClient** pattern with a worked example (Siemens S7-1500 V2.5+ onboard OPC UA server → `OpcUaClient` driver → re-publish on the OtOpcUa server's address space); updates `docs/featuregaps.md` row #1 to reflect the Track 1+3 decision; updates the "Optimized-DB" line of `docs/drivers/S7-Test-Fixture.md` §"What it does NOT cover" item 4 to point at the new doc; no CLI doc change (CLI is a probe tool, not the bridging path); no Snap7 fixture change (snap7 has no Optimized- DB mode); the live-firmware test toggling Optimized block access on / off is recorded as a manual checklist in `docs/drivers/S7-Test-Fixture.md` §"Follow-up candidates" and gated behind `--with-real-plc`; if Track 2 is taken later, this PR's doc surface becomes the migration baseline; no `scripts/e2e/test-s7.ps1` change. --- ## Documentation, fixture, and e2e impact Consolidated view of every per-PR `Docs / fixture / e2e` line above, so a reviewer can see the cross-cutting churn at a glance and so the doc / fixture / e2e maintainers can sequence their work alongside the code PRs. ### User-facing documentation churn | PR | `docs/v2/s7.md` | `docs/Driver.S7.Cli.md` | `docs/drivers/S7-Test-Fixture.md` | New / cross-cut docs | |----|-----------------|-------------------------|------------------------------------|----------------------| | PR-S7-A1 (LInt/ULInt/LReal/LWord) | extend type-mapping table | new sizes in cookbook | remove "no 64-bit types" | — | | PR-S7-A2 (STRING/WSTRING/CHAR/WCHAR) | string layout subsection | `--type WString` / `--string-length` | list new types | — | | PR-S7-A3 (DTL/DT/S5TIME/TIME/TOD/DATE) | "Date / time types" subsection | datetime cookbook entries | list new types | — | | PR-S7-A4 (arrays) | "Array tags (ValueRank=1)" subsection | `--array-count` flag + examples | list array round-trips | — | | PR-S7-A5 (V-memory) | "LOGO! 8 / S7-200 V-memory" subsection | grammar table + S7200Smart example | parser coverage note | — | | PR-S7-B1 (PDU packing) | "Performance — multi-variable PDU packing" subsection | — | — | — | | PR-S7-B2 (block coalescing) | "Block-read coalescing" subsection | — | — | — | | PR-S7-C1 (negotiated PDU diag) | "Diagnostics surfacing" subsection | — | "negotiated PDU size" line | — | | PR-S7-C2 (TSAP) | "TSAP / Connection Type" section | `--tsap-mode` / `--local-tsap` / `--remote-tsap` flags | — | — | | PR-S7-C3 (scan groups) | "Per-tag scan groups" subsection | — | — | — | | PR-S7-C4 (deadband) | "Deadband / on-change" subsection | — | — | — | | PR-S7-C5 (PUT/GET pre-flight) | "Pre-flight PUT/GET enablement" subsection | extend "PUT/GET must be enabled" | mark live-firmware test | — | | PR-S7-D1 (TIA CSV / AWL import) | "Symbol import" cross-link | future `import` subcommand stub | — | **new `docs/drivers/S7-TIA-Import.md`** | | PR-S7-D2 (UDT / STRUCT) | "UDT / STRUCT support" section | — | remove "UDT fan-out" | extend `S7-TIA-Import.md` | | PR-S7-D3 (instance DB) | re-import-on-FB-edit caveat | — | — | extend `S7-TIA-Import.md` | | PR-S7-E1 (SZL diagnostics) | "CPU diagnostics (SZL)" section | `read -a @System.CpuType` example | "SZL not modelled by snap7" + Follow-up | — | | PR-S7-E2 (PLC password) | "PLC password / protection levels" section | `--password` flag | "password not modelled by snap7" | — | | PR-S7-F (Optimized DB / S7Plus) | top-level "Optimized DB constraint" + bridge-via-OpcUaClient worked example | — | point §"What it does NOT cover" at new doc | also updates `docs/featuregaps.md` row #1 | ### Snap7-server fixture seed-type additions per PR The snap7 simulator at `localhost:1102` (driven by `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/server.py` + `Docker/profiles/s7_1500.json`) has a `seed_buffer` pump with a fixed type set — `u8 / i8 / u16 / i16 / u32 / i32 / f32 / bool / ascii`. New PRs need new seed-type cases in `server.py`, new offsets in `s7_1500.json`, and matching constants in `S7_1500Profile.cs`. The table below names the delta for each Build-Yes PR: | PR | New `server.py` seed types | New `s7_1500.json` seed offsets | `S7_1500Profile.cs` additions | |----|----------------------------|----------------------------------|-------------------------------| | PR-S7-A1 | `i64`, `u64`, `f64` | `DB1.DBL40` (i64), `DB1.DBL48` (f64), `DB1.DBL56` (u64) | `SmokeI64Tag` / `SmokeU64Tag` / `SmokeF64Tag` | | PR-S7-A2 | `wstring`, `char`, `wchar` (existing `ascii` covers STRING) | `DB1.WSTRING[256]`, `DB1.CHAR[300]` | `SmokeWStringTag` / `SmokeCharTag` | | PR-S7-A3 | `dtl`, `dt`, `s5time`, `time`, `tod`, `date` (golden-byte vectors in comments) | `DB1.DTL[260]`, `DB1.DT[272]`, `DB1.S5TIME[280]`, `DB1.TIME[284]`, `DB1.TOD[288]`, `DB1.DATE[292]` | `SmokeDtl` / `SmokeDt` / `SmokeS5Time` / `SmokeTime` / `SmokeTod` / `SmokeDate` | | PR-S7-A4 | `array` meta-seed (inner-type + count) | `DB1.ArrayInt[300]` 10×Int, `DB1.ArrayReal[320]` 10×Real | `ArrayInt10Tag` / `ArrayReal10Tag` | | PR-S7-A5 | none (V-memory aliases land in DB1, which `server.py` already exposes) | none | unit-only — no profile change | | PR-S7-B1 | none | none (existing scalar seeds suffice for packing) | none — perf integration test reuses scalar tags | | PR-S7-B2 | none | none (existing contiguous DBW0 / DBW10 / DBD20 already test merge) | none | | PR-S7-C1 | none | none | none | | PR-S7-C2 | none (snap7 accepts any TSAP) | none | none | | PR-S7-C3 | none | none | none | | PR-S7-C4 | none | none | none | | PR-S7-C5 | none (existing `MK0` MW0 seed covers success path) | none | none | | PR-S7-D1 | none (CSV import lands tags pointing at existing seeds) | none | possibly add fixture-pointer constants | | PR-S7-D2 | `udt_layout` meta-seed (per-member offsets) | `DB1.MyUdt[400]` (Real + Int + Bool layout) | `MyUdtTag` + member tags | | PR-S7-D3 | none (resolved addresses land in DB1) | none | none | | PR-S7-E1 | none — snap7 doesn't model SZL; unit-level golden bytes cover the parser | none | none | | PR-S7-E2 | none — snap7 doesn't enforce protection levels; options-binding unit tests only | none | none | | PR-S7-F | none — snap7 has no Optimized-DB mode; live-firmware checklist instead | none | none | ### E2E `scripts/e2e/test-s7.ps1` impact `scripts/e2e/test-s7.ps1` runs the five-assertion CLI loopback (probe / driver-loopback / forward-bridge / reverse-bridge / subscribe-sees-change) against `DB1.DBW0` Int16. Build-Yes PRs that add CLI surface get a matching loopback assertion; PRs that touch only internals or admin-UI flows do not. | PR | E2E script change | |----|-------------------| | PR-S7-A1 | add LInt loopback assertion (write 0x7FFFFFFFFFFFFFFF, read back) | | PR-S7-A2 | add string round-trip assertion | | PR-S7-A3 | none (CLI cookbook covers manual surface) | | PR-S7-A4 | add array round-trip assertion | | PR-S7-A5 | none (live-LOGO! field-only) | | PR-S7-B1 | none | | PR-S7-B2 | none | | PR-S7-C1 | none | | PR-S7-C2 | none (live-firmware-only) | | PR-S7-C3 | none (subscribe assertion already covers polling) | | PR-S7-C4 | none | | PR-S7-C5 | none (probe runs first today) | | PR-S7-D1 | none (Admin UI has its own e2e) | | PR-S7-D2 | add UDT-member round-trip assertion | | PR-S7-D3 | none (import-time concern) | | PR-S7-E1 | none | | PR-S7-E2 | none (live-firmware-only) | | PR-S7-F | none (decision PR; live-firmware checklist instead) | --- ## Skip-rated items (for context) | # | Gap | Skip rationale | |---|-----|---------------| | 12 | AS-Alarms / Alarm_S / ProDiag | Alarms are a separate workstream; no `IAlarmSource` shipped on this driver yet, and the gap analysis flags it as a deferred topic. | | 13 | CPU Run / Stop control / block download | Security and safety risk. PG-class writes that change CPU state are explicitly out of scope. | | 15 | S7-1500 Secure Communication / TLS | Significant work; S7netplus has no TLS surface. Reconsider when S7Plus track is taken. | | 16 | S7-400H redundant H-system support | Rare in our deployment scope. Server-level redundancy (`docs/Redundancy.md`) covers the OPC UA layer; H-system driver-level failover is a separate axis. | | 17 | Multi-CPU rack parallel sessions | One session per CPU works for the deployments we target; multi-CPU racks are an S7-400 niche. | | 18 | MPI / Profibus / RFC1006-routed transports | Declining use; brownfield only. S7netplus is Ethernet-only. | | 23 | Connection-resource budget / parallel jobs | One connection works; premature optimization until a deployment hits the cap. | --- ## Open questions ### (a) Library choice for S7Plus PR-S7-F gates on this decision. Options: 1. **Stay on S7netplus + document Optimized-DB constraint** (preferred default). 2. **Fork to Sharp7 / Snap7 v2** — does *not* solve the S7Plus / Optimized-DB problem; both are classic S7comm only. Adopting them buys nothing for this gap. Reject unless we want it for unrelated reasons. 3. **Custom S7Plus client over Wireshark-dissected protocol** — large effort, ongoing maintenance risk. Only if a customer is paying. 4. **OPC UA → OPC UA bridge via existing OPC UA Client driver** — sidesteps S7Plus by re-using Siemens's onboard OPC UA server. Recommended secondary track. Decision needed before Phase 6 PR-S7-F kicks off. ### (b) `WriteIdempotent` semantics for new types The `WriteIdempotent` per-tag flag (decisions #44, #45, #143) governs replay- safe writes. New types from Phase 1: - **STRING / WSTRING** — typically idempotent (recipe / message text). Replay-safe by default? **Need confirmation.** Risk: PLC programs that treat a new string write as a "new message" event would double-fire. - **DTL / DT** — usually written from a clock master; replay-safe. - **Arrays of UDT** — depends on the UDT semantics (recipe = safe, command block = unsafe). Inherit `WriteIdempotent` from the parent tag, do not add a per-member flag. - **64-bit types** — same rule as 32-bit equivalents. Default: keep `WriteIdempotent = false` for everything. Operators flip per tag based on PLC program semantics. **No semantic extension needed**, but document the per-type guidance in `docs/v2/s7.md`. ### (c) Symbol-import file format(s) PR-S7-D1 ships an importer. Which formats? - **TIA Portal CSV** (Show all tags / Export) — preferred entry point; most common. **Confirm.** - **TIA Portal SDF / Excel** — same data; harder to parse. Skip unless customer demand emerges. - **STEP 7 Classic AWL / SCL `.AWL`** — secondary. Useful for legacy S7-300/400 sites still on Classic. **Include in D1?** - **`.s7p` / `.zap` project archive** — full TIA project. ZIP-shaped; symbol export would require unpacking and parsing internal XML. Large scope. **Defer.** - **`.udt` / `.SDF` external tag library** — niche; defer unless asked. Recommendation: PR-S7-D1 ships **TIA CSV** + **AWL** only. Anything else is a follow-up. Decision needed before Phase 4 work begins.