docs(design): bit-index RMW writes (AbLegacy B/I/O + TwinCAT BOOL-within-word)
This commit is contained in:
Joseph Doherty
@@ -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<<bit) : parent & ~(1u<<bit)
|
||||
write parent back as uint (binding marshals uint -> BYTE/WORD/DWORD native width)
|
||||
release lock
|
||||
map write status
|
||||
```
|
||||
|
||||
- New per-parent-symbol `SemaphoreSlim` RMW lock (a `ConcurrentDictionary<string,SemaphoreSlim>` 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.
|
||||
Reference in New Issue
Block a user