docs(phase4d): S7 wide types + Timer/Counter design (approved)
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,129 @@
|
||||
# S7 Wide Types + Timer/Counter — Design
|
||||
|
||||
> **Status:** Approved 2026-06-17. Branch `feat/stillpending-phase-4d-s7-wide-types` off master `12e114b3`.
|
||||
> Next step: writing-plans → subagent-driven-development → finish (merge to master + push).
|
||||
|
||||
## Goal
|
||||
|
||||
Close the `stillpending.md` §2 driver-layer lines:
|
||||
|
||||
- *"S7 — Int64 / UInt64 / Float64(LReal) / String / DateTime read+write not implemented"* (`S7Driver.cs:350` `UnimplementedDataTypes`).
|
||||
- *"S7 — Timer/Counter address areas unsupported"* (`S7Driver.cs:306` `RejectUnsupportedTagAddresses`).
|
||||
|
||||
Scope (confirmed via AskUserQuestion): **all five wide types + Timer/Counter read** in one phase.
|
||||
|
||||
## Key discovery — the author/compose surfaces are already wired
|
||||
|
||||
The slice is **almost entirely driver-internal**. Already in place:
|
||||
|
||||
- `S7DataType` (`S7DriverOptions.cs:124`) already enumerates all 12 members incl. Int64/UInt64/Float64/String/DateTime.
|
||||
- `S7TagConfigModel` (AdminUI typed editor) reads/writes `dataType` + `stringLength` against the full enum — no `UnimplementedDataTypes` gate in the UI.
|
||||
- `S7EquipmentTagParser` already parses `dataType` + `stringLength` (+ `arrayCount`) from the equipment-tag `TagConfig` blob.
|
||||
- `DriverDataType` already has `Int64`/`UInt64` members (added in stillpending Phase 4) — so S7's lossy `Int64/UInt64 → Int32` node mapping can be corrected.
|
||||
- `S7.Cli` Read/Write/Subscribe `--data-type` already accepts the full enum; only the help text says "not yet implemented".
|
||||
|
||||
What actually blocks these types today: two **init guards** that reject them, the **codec** (`ReinterpretRawValue`/`BoxValueForWrite`) that throws `NotSupportedException` for them, and one **lossy node mapping**.
|
||||
|
||||
## Library facts (verified against S7netplus 0.20.0)
|
||||
|
||||
- `Plc.ReadBytesAsync(DataType, db, startByteAdr, count, ct)` / `Plc.WriteBytesAsync(...)` exist (the 4c array path already uses `ReadBytesAsync`).
|
||||
- Pure static byte-array codecs exist in `S7.Net.Types`: `S7String`, `S7WString`, `DateTime` (DT, 8-byte BCD), `DateTimeLong` (DTL, 12-byte), `Timer`, `Counter`, `LReal` — each with `FromByteArray`/`ToByteArray`. These take/return `byte[]`, so decode stays **unit-testable without a live PLC**.
|
||||
- The classic S7 string address suffix (`Plc.ReadAsync("DB1.DBD0")`) decodes only 1/2/4-byte sizes (bool/byte/ushort/uint). There is **no 8-byte/string/datetime string-suffix path** — wide types must use the byte-buffer path.
|
||||
|
||||
## Approaches considered
|
||||
|
||||
- **A — Byte-buffer path + pure S7.Net.Types decoders (CHOSEN).** Narrow types (Bool…Float32) keep the proven string `ReadAsync`/`WriteAsync` path *unchanged*. Wide/structured/Timer/Counter route through `ReadBytesAsync`/`WriteBytesAsync` (the 4c mechanism) feeding a **pure** `DecodeScalarBlock`/`EncodeScalarBlock`: `BinaryPrimitives` for the 8-byte numerics, S7.Net's pure static `FromByteArray`/`ToByteArray` for String/DateTime/Timer/Counter. Mirrors 4c exactly; fully unit-testable; minimal churn to working code.
|
||||
- **B — Migrate everything to the buffer path.** One uniform codec, but rewrites the proven narrow path for zero functional gain. Rejected (needless regression surface).
|
||||
- **C — Lean on S7.Net's typed string `Read(VarType)`.** Re-introduces S7.Net's own address re-parsing (the seam 4c deliberately owns) and isn't unit-testable. Rejected in favor of A's pure functions.
|
||||
|
||||
## Architecture (Approach A)
|
||||
|
||||
### Codec dispatch
|
||||
|
||||
In `ReadOneAsync`/`WriteOneAsync`, branch on the tag's `DataType`:
|
||||
|
||||
- **Narrow** (Bool, Byte, Int16, UInt16, Int32, UInt32, Float32) → existing string `plc.ReadAsync(tag.Address)` / `plc.WriteAsync(tag.Address, boxed)` path, **unchanged**.
|
||||
- **Wide/structured/Timer/Counter** → new `ReadScalarBlockAsync` / `WriteScalarBlockAsync` (resolve `(area, db, byteOffset)` from the parsed address, call `ReadBytesAsync`/`WriteBytesAsync` for `width` bytes) → pure `DecodeScalarBlock(tag, addr, block)` / `EncodeScalarBlock(tag, value)`.
|
||||
|
||||
`width(DataType)`:
|
||||
|
||||
| DataType | Width | Decode / Encode |
|
||||
|---|---|---|
|
||||
| Int64 | 8 | `BinaryPrimitives.Read/WriteInt64BigEndian` |
|
||||
| UInt64 | 8 | `BinaryPrimitives.Read/WriteUInt64BigEndian` |
|
||||
| Float64 (LReal) | 8 | `BinaryPrimitives.Read/WriteDoubleBigEndian` |
|
||||
| String | `StringLength + 2` | `S7.Net.Types.S7String.FromByteArray`/`ToByteArray` (classic 1-byte-char `STRING`) |
|
||||
| DateTime | 8 | `S7.Net.Types.DateTime.FromByteArray`/`ToByteArray` (`DATE_AND_TIME`/DT, BCD) |
|
||||
| Timer (`T{n}`) | 2 | `S7.Net.Types.Timer.FromByteArray` → `double` seconds (read-only) |
|
||||
| Counter (`C{n}`) | 2 | `S7.Net.Types.Counter.FromByteArray` → `int` count (read-only) |
|
||||
|
||||
The pure `DecodeScalarBlock`/`EncodeScalarBlock` keep the same factoring rationale as the existing `ReinterpretRawValue`/`BoxValueForWrite`/`DecodeArrayBlock` — exercised in unit tests against known byte blocks. Verify exact S7.Net.Types method signatures at implementation (the implementer confirms against the 0.20.0 DLL).
|
||||
|
||||
### Addressing convention (wide/structured types)
|
||||
|
||||
**Byte-anchored.** Wide and structured types are addressed by their **start byte** via the byte suffix: `DB{n}.DBB{offset}`, `MB{offset}`, `IB{offset}`, `QB{offset}`. The `B` suffix names the start byte; the actual width comes from the `DataType` (there is no native 8-byte S7 address suffix, and `DBD` is reserved for 4-byte Real/DInt). Timer/Counter keep `T{n}` / `C{n}`.
|
||||
|
||||
A wide/structured type configured with a non-`B` suffix (e.g. `DBW`/`DBD`) is a **fail-fast init error** with a clear message.
|
||||
|
||||
### Timer / Counter semantics
|
||||
|
||||
- Decode is driven by the **area** (`S7Area.Timer`/`Counter`), read via `ReadBytesAsync(area, 0, number, 2)`.
|
||||
- **Timer** → `double` seconds → node `DriverDataType.Float64`; require the tag `DataType` be `Float64` (else config error).
|
||||
- **Counter** → `int` count → node `DriverDataType.Int32`; require the tag `DataType` be `Int32` (else config error).
|
||||
- **Read-only** in this phase — writing a live timer/counter preset is rare and higher-risk (named follow-up). A write to a Timer/Counter tag returns `BadNotWritable`.
|
||||
|
||||
### Init guards (preserve fail-fast)
|
||||
|
||||
- Empty `UnimplementedDataTypes` (all five now implemented).
|
||||
- Remove the Timer/Counter rejection from `RejectUnsupportedTagAddresses`.
|
||||
- **Add** a guard rejecting at init: (a) `isArray:true` + wide `DataType` (wide-type **arrays** stay out of scope), (b) wide/structured `DataType` + non-`B` address suffix, (c) Timer/Counter + incompatible `DataType`. Each with a clear remediation message — fail-fast-at-init promise holds (no `BadNotSupported`-on-every-read leakage).
|
||||
|
||||
### Node mapping fix
|
||||
|
||||
`MapDataType`: `Int64 → DriverDataType.Int64`, `UInt64 → DriverDataType.UInt64` (today both lossily map to `Int32` for values > 2³¹−1). `Float64`/`String`/`DateTime` already map correctly.
|
||||
|
||||
### CLI + docs
|
||||
|
||||
- `S7.Cli` `ReadCommand`/`WriteCommand`/`SubscribeCommand`: drop the "Int64, UInt64, Float64, String, DateTime are not yet implemented" help notes.
|
||||
- `docs/v2/driver-specs.md §5` + `docs/drivers/S7.md`: document the byte-anchored addressing for wide types, the supported type table, Timer/Counter read, and the named deferrals.
|
||||
- Clear the `stillpending.md` §2 lines **via the plan record only** (do not stage `stillpending.md`).
|
||||
|
||||
## Scope boundaries (named deferrals — not silent)
|
||||
|
||||
- **Wide-type arrays** (`Int64[]`, `String[]`, `DateTime[]`, …) — `DecodeArrayBlock` keeps its existing deferred-throw; the new init guard rejects them up-front.
|
||||
- **`S7WString`** (2-byte UTF-16 chars) — classic 1-byte `STRING` only this phase.
|
||||
- **`DTL`** (12-byte `DateTimeLong`) — `DATE_AND_TIME` (8-byte DT) only this phase.
|
||||
- **Timer/Counter writes** — read-only this phase.
|
||||
|
||||
## Testing
|
||||
|
||||
- xUnit + Shouldly, offline (no live PLC), in `ZB.MOM.WW.OtOpcUa.Driver.S7.Tests` mirroring the existing `DecodeArrayBlock`/`ReinterpretRawValue`/`BoxValueForWrite` tests:
|
||||
- `DecodeScalarBlock` round-trips for Int64/UInt64/LReal (known big-endian byte blocks), String (header + chars), DateTime (BCD block), Timer (seconds), Counter (count).
|
||||
- `EncodeScalarBlock` round-trips for Int64/UInt64/LReal/String/DateTime (and a decode∘encode identity).
|
||||
- `MapDataType` → Int64/UInt64 (not Int32).
|
||||
- Init guards: wide-type-array rejected, wide-type-non-Byte-address rejected, Timer/Counter incompatible-DataType rejected, narrow types + Timer/Counter accepted.
|
||||
- `S7.Cli.Tests` help-text assertions if any pin the removed notes.
|
||||
- **No bUnit** — the AdminUI editor already surfaces the enum (Razor unchanged); covered by the existing model tests.
|
||||
|
||||
## Live `/run`
|
||||
|
||||
Feasible against the rig's S7 sim (`10.100.0.35:1102`, `S7_SIM_ENDPOINT`) — best-effort read of an LReal/Int64 equipment tag over the wire (mirroring 4c's Modbus headline), and a round-trip write where the sim permits. If the sim lacks populated 8-byte DBs, fall back to a unit-proven gate + note (as 4c did for the non-Modbus drivers). Local docker-dev rig; login disabled; agent-driven.
|
||||
|
||||
## Task slicing (subagent-driven)
|
||||
|
||||
All edits are in the S7 driver + its tests/CLI/docs (disjoint from other drivers). Within `S7Driver.cs` the codec tasks serialize.
|
||||
|
||||
- **T1** — Init guards (empty `UnimplementedDataTypes`, drop Timer/Counter address reject, add wide-array / non-Byte-address / Timer-Counter-DataType guards) + `MapDataType` Int64/UInt64 fix. `standard`.
|
||||
- **T2** — 8-byte numerics (Int64/UInt64/LReal) scalar read+write: codec dispatch + `ReadScalarBlockAsync`/`WriteScalarBlockAsync` + pure `DecodeScalarBlock`/`EncodeScalarBlock` (numeric cases) + tests. `high-risk` (introduces the dispatch seam).
|
||||
- **T3** — String read+write (`S7String` + `StringLength`) + tests. `standard`.
|
||||
- **T4** — DateTime read+write (`DATE_AND_TIME`/DT) + tests. `standard`.
|
||||
- **T5** — Timer/Counter read (decode by area, read-only) + tests. `standard`.
|
||||
- **T6** — CLI help + `docs/v2/driver-specs.md` + `docs/drivers/S7.md` + plan-record §2 clear. `small`.
|
||||
- **T7** — Full build + S7 + S7.Cli test run + final integration review. `standard`.
|
||||
- **T8** — Live `/run` acceptance (S7 sim best-effort) + finish branch (merge to master + push). `standard`.
|
||||
|
||||
Dependency graph: `T1 → T2 → {T3, T4, T5}` (T3/T4/T5 all follow T2's dispatch seam and share `S7Driver.cs`, so serialize) `→ T6 → T7 → T8`.
|
||||
|
||||
## Hard rules (carried from prior phases)
|
||||
|
||||
Stage by explicit path, never `git add .`; never stage `sql_login.txt` / `src/Server/.../pki/` / `pending.md` / `current.md` / `docker-dev/docker-compose.yml` / `stillpending.md`; never echo/commit secrets; no force-push; no `--no-verify`; **NO EF migration**; **NO Commons wire/proto contract change** (the codec + `EnsureVariable` array params from 4c are unchanged; `S7DataType`/`DriverDataType` already carry every member needed); **NO bUnit**; `dangerouslyDisableSandbox` for all build/test/rig commands.
|
||||
Reference in New Issue
Block a user