diff --git a/docs/plans/2026-06-17-stillpending-bit-rmw-writes-design.md b/docs/plans/2026-06-17-stillpending-bit-rmw-writes-design.md new file mode 100644 index 00000000..ae2d5377 --- /dev/null +++ b/docs/plans/2026-06-17-stillpending-bit-rmw-writes-design.md @@ -0,0 +1,115 @@ +# Inbound bit-index RMW writes (AbLegacy B/I/O + TwinCAT BOOL-within-word) — Design + +> **Status:** Approved 2026-06-17. Branch `feat/stillpending-bit-rmw-writes` off master `67da6d4f`. +> Next step: writing-plans → subagent-driven-development → finish (merge to master + push). + +## Goal + +Close the two `stillpending.md` §2 "bit-within-word write" lines: + +- *"AbLegacy — B/I/O-file bit-within-word writes may fall to `BadNotSupported`"* (`AbLegacyDriver.cs:355-358` / `367-368`, the RMW-exclusion comment vs. the condition disagree). +- *"TwinCAT — BOOL-within-word (bit-index) writes not implemented"* (`AdsTwinCATClient.cs:166-168` / `189-191`, throws `NotSupportedException`, "tracked in task #181"). + +Both are the same shape — flip one bit inside a parent word via read-modify-write — but the two drivers start from very different places: **AbLegacy already has the full RMW machinery and merely excludes B/I/O**, while **TwinCAT has no write-side RMW at all** (it throws). + +Scope (confirmed via AskUserQuestion): **bit-index RMW writes** — these two §2 items, single-bit BOOL-within-word writes only. + +## Key discovery — AbLegacy's RMW path already works; B/I/O are just excluded + +`AbLegacyDriver.WriteBitInWordAsync` (`:603-649`) already implements the parent-word read → mask → write-back with a per-parent `GetRmwLock`, and it works today for N/L/S/A files. The dispatch guard (`:367-368`) is: + +```csharp +if (def.DataType == AbLegacyDataType.Bit && parsed?.BitIndex is int bit + && parsed.FileLetter is not "B" and not "I" and not "O") +``` + +The `is not "B" and not "I" and not "O"` clause routes B/I/O bit writes *past* `WriteBitInWordAsync` to the generic `runtime.EncodeValue(def.DataType, bitIndex, …)` path, which hits the guard throw in `LibplctagLegacyTagRuntime.EncodeValue` (`:138-144`: *"Bit-with-bitIndex writes must go through AbLegacyDriver.WriteBitInWordAsync."*) → caught at `:398-402` → `BadNotSupported`. The class-doc comment at `:363-366` even claims the path *"Applies to … + B-file bits (B3:0/0)"* — so the exclusion contradicts the stated intent. B-files are 16-bit word files (file-numbered); I/O image files are 16-bit (no file number, per `AbLegacyAddress`). All three are exactly the 16-bit-`Int`-parent case the existing code already handles. + +## Library / addressing facts (verified against the code) + +- `AbLegacyAddress.TryParse` already accepts `B3:0/0`, `I:0/0`, `O:1/2` (bit range-checked 0..15 for the 16-bit N/B/I/O/S/A files; `MaxBitIndexFor`). `parentAddress.ToLibplctagName()` (with `BitIndex = null`) yields `B3:0` / `I:0` / `O:1` — the libplctag name format the parent-word runtime already consumes. +- `WriteBitInWordAsync` derives the parent width from the file letter (`L` → 32-bit `Long`, everything else → 16-bit `Int`); B/I/O fall into the 16-bit branch with no new code. +- TwinCAT's BOOL-within-word **read** (`AdsTwinCATClient.cs:120-128`) already does the inverse: strip the `.N` suffix (`StripBitSuffix`), read the parent as `uint`, `ExtractBit`. The symbol table does not expose `WordVar.N` as its own entry (ADS returns `DeviceSymbolNotFound`), so RMW is the only path. The write must mirror this read symmetrically. +- The TwinCAT.Ads managed binding marshals a written `uint` to the symbol's native width (BYTE/WORD/DWORD); reading-first preserves the other bits, so flipping bits 0..31 in a `uint` is width-safe. + +## Approaches considered + +- **A — Extend each driver's own RMW path (CHOSEN).** AbLegacy: drop the B/I/O exclusion so B/I/O route through the existing `WriteBitInWordAsync`. TwinCAT: build a new RMW in `WriteValueAsync` that mirrors the existing bit-read (strip `.N` → read parent `uint` → set/clear → write `uint`) under a new per-parent-symbol lock. Minimal churn, each driver owns its own primitives, fully unit-testable against the existing fakes. +- **B — Shared cross-driver RMW helper.** One generic "bit-in-word RMW" helper both drivers call. Rejected: the two backends (libplctag tag-runtime vs ADS symbol client) are completely different I/O primitives; a shared helper is an awkward abstraction over incompatible backends and couples two disjoint driver projects for no real DRY win. YAGNI. +- **C — Native single-bit write (no RMW).** PCCC bit-write command / ADS raw IndexGroup bit-offset. Rejected: bypasses the symbolic seam both drivers deliberately chose, is far harder to unit-test offline, and isn't uniformly available across SLC/ML models. RMW is the portable, already-proven pattern. + +## Architecture (Approach A) + +### Part 1 — AbLegacy B/I/O-file bit writes + +Remove the `and not "B" and not "I" and not "O"` clause from the `WriteBitInWordAsync` dispatch guard (`AbLegacyDriver.cs:367-368`). B/I/O bit writes then route through the existing RMW: + +- Parent-word address = `parsed with { BitIndex = null }` → `ToLibplctagName()` → `B3:0` / `I:0` / `O:1`. +- 16-bit `Int` parent (the non-`L` branch), `widthMask = 0xFFFF`, per-parent `GetRmwLock` — all unchanged. +- Update the class-doc comment (`:363-366`) so the stated coverage matches the code (B/I/O now included). + +**Input-image (`I`) caveat.** The PLC drives the input image from physical inputs, so an `I`-file write may be rejected by the device. We do **not** pre-reject at the driver — we let the write reach the PLC and surface its real PCCC status faithfully (consistent with the driver's "pass the address through, surface the device status" philosophy). Documented in code + `docs/drivers`, not silently swallowed. (Output-image `O` and binary `B` writes are ordinary and fully supported.) + +### Part 2 — TwinCAT BOOL-within-word writes + +Replace the `NotSupportedException` throw in `AdsTwinCATClient.WriteValueAsync` (`:189-191`) with an RMW symmetric to the read path: + +``` +parent = StripBitSuffix(symbolPath) +acquire per-parent-symbol RMW lock + read parent as uint (mirrors the read; ADS DeviceSymbolNotFound on WordVar.N is why) + updated = set ? parent | (1u< BYTE/WORD/DWORD native width) +release lock +map write status +``` + +- New per-parent-symbol `SemaphoreSlim` RMW lock (a `ConcurrentDictionary` keyed by the stripped parent symbol), mirroring AbLegacy's `GetRmwLock`. Serializes our concurrent bit-writers to the same word. +- Width-safe: read preserves the other bits within the symbol's native width; we only flip bits 0..31; the write marshals back to the symbol's width. +- Update the stale `bitIndex … (not supported for writes)` doc comment on `WriteValueAsync`. + +### Shared RMW limitation (documented for both) + +The parent-word lock serializes *our* concurrent bit-writers but cannot protect against the PLC program itself writing the same word between our read and write — inherent to RMW, already accepted for AbLegacy N-files. Stated in both drivers' docs. + +## Testing + +xUnit + Shouldly, offline (no live PLC/PLC-runtime), against the existing fakes — **no bUnit** (no AdminUI surface; the author surfaces already accept bit addresses because reads work): + +- **AbLegacy** — extend `AbLegacyBitRmwTests` (the N-file RMW tests + `FakeAbLegacyTagFactory`/`FakeAbLegacyTag`, which track the parent-word value by key e.g. `factory.Tags["N7:0"]`): + - `B3:0/0`, `I:0/0`, `O:1/2` bit-**set** RMW round-trips (parent read → OR bit → write back; assert the parent word). + - bit-**clear** preserves the other bits in the word. + - concurrent bit-writers to the same B/O word compose correctly (per-parent lock). + - regression: the existing N/L path still passes unchanged. +- **TwinCAT** — extend `TwinCATReadWriteTests` + `FakeTwinCATClient`: + - bit set/clear RMW on a **WORD** parent and a **DWORD** parent; assert the written parent value (other bits preserved). + - write-back is `uint` (the fake records the written CLR type/value). + - per-parent serialization of concurrent bit-writers. + - regression: existing scalar/array writes unchanged; the old throw is gone. + +## Live `/run` + +Fixture-gated — there is no local PCCC (libplctag SLC/ML) or ADS (TwinCAT runtime is Windows-only) sim on this Mac. Primary acceptance gate is **unit-proven** (as 4c did for the non-Modbus drivers). Best-effort only: attempt a libplctag `ab_server` emulator for an AbLegacy B-file bit round-trip; if it can't host a writable B-file locally, record the honest unit-proven gate + the fixture/operator follow-up. TwinCAT live stays operator-gated (needs a TwinCAT/ADS target). + +## Scope boundaries (named deferrals — not silent) + +- Whole-word writes — already supported; unchanged. +- Array-of-bits writes (a `BOOL[]` / multi-bit array) — out of scope (array writes are a standing named deferral from 4c). +- Sub-element writes (`T4:0.DN`, etc.) — out of scope; not bit-index writes. +- AbLegacy `I`-file writes are *enabled but device-gated* — we surface the PLC's accept/reject rather than guaranteeing success. + +## Task slicing (subagent-driven) + +The two drivers are **disjoint projects** → their implementers run concurrently. + +- **T1** — AbLegacy: drop the B/I/O exclusion + fix the class-doc comment + extend `AbLegacyBitRmwTests` (B/I/O set/clear/concurrent/regression). `standard`. Parallelizable with T2. +- **T2** — TwinCAT: replace the BOOL-within-word throw with RMW + per-parent lock + doc-comment fix + extend `TwinCATReadWriteTests`/`FakeTwinCATClient`. `standard`. Parallelizable with T1. +- **T3** — Docs: `docs/drivers/AbLegacy.md` + `docs/drivers/TwinCAT.md` (B/I/O bit writes, BOOL-within-word RMW, the input-image caveat + the shared RMW-vs-PLC-program limitation); clear the `stillpending.md` §2 lines **via the plan record only** (do not stage `stillpending.md`). `small`. +- **T4** — Full build + AbLegacy + TwinCAT test run + final integration review. `standard`. +- **T5** — Live `/run` acceptance (best-effort AbLegacy `ab_server`; else unit-proven gate) + finish branch (merge to master + push). `standard`. + +Dependency graph: `{T1 ∥ T2} → T3 → T4 → T5`. + +## 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** (both edits are driver-internal — `AbLegacyDriver`/`LibplctagLegacyTagRuntime` and `AdsTwinCATClient`; no `IDriver`/`WriteRequest`/`WriteResult` change); **NO bUnit**; `dangerouslyDisableSandbox` for all build/test/rig commands.