From 39981e2d7abbd64a6ffd8e5b657da8ee3fc0f8ea Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Wed, 12 Aug 2026 17:46:07 -0500 Subject: [PATCH 1/8] rename: the model package directory is pyquadcortex/device/, not model/ "Model" already means an amp or pedal block in this codebase - protocol/models.py, catalog.Model, ModelCatalog, set_block(model=...) - and docs/domain-model.md section 5 settled that collision once by giving the word to the virtual device list. The package directory had taken it back. Nothing published points at the old path. pyquadcortex.__all__ lists `protocol` and not `model`, and the model namespace has never been released - 0.40.0 is the last release and it predates the flip - so there is no deprecation shim to write and no user-visible change. --- CLAUDE.md | 2 +- docs/STEERING.md | 2 +- docs/architecture.md | 6 ++--- pyquadcortex/__init__.py | 2 +- pyquadcortex/{model => device}/__init__.py | 8 +++++- pyquadcortex/{model => device}/device.py | 0 scripts/check_artifacts.py | 2 +- tests/test_import_cleanliness.py | 6 ++--- tests/test_namespace.py | 30 ++++++++++++---------- 9 files changed, 33 insertions(+), 25 deletions(-) rename pyquadcortex/{model => device}/__init__.py (56%) rename pyquadcortex/{model => device}/device.py (100%) diff --git a/CLAUDE.md b/CLAUDE.md index 0091bd9..5d5a8ee 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr ## Conventions - Dev setup: `uv venv && uv pip install -e ".[dev]"` (or plain venv + pip, see contributing.md). Run tests with `.venv/bin/python -m pytest`. The suite passes offline - no hardware, no `hid` import, no `DYLD_LIBRARY_PATH`. -- Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/model/`. +- Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because *model* is the device's own word for an amp or pedal block (`protocol/models.py`, `catalog.Model`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`. - The model represents what the unit shows, in the unit's own words, and never guesses. A control we understand but cannot yet drive is modelled and REFUSES the operation (ADR-0007); a control we do not understand is omitted, with the reason recorded in `docs/domain-model.md`'s appendix. Nothing ships with a "this might be stale or wrong" caveat. - A model property that reads a device field checks the field is PRESENT (`protocol.field_present`) before reporting it. Most of this schema sits in synthetic `oneof`s, so protobuf returns `""` or `0` for a field the unit never sent, and reporting that as the answer is the guess the rule above forbids. Never cache a reply that came back incomplete - a retry has to be able to recover. - Anything the model caches is valid only while its connection is. A closed `Device` refuses reads rather than answering from cache, because a model that reports the unit's state through an object with no unit behind it is the failure the whole layer exists to avoid. diff --git a/docs/STEERING.md b/docs/STEERING.md index ca4d799..9625328 100644 --- a/docs/STEERING.md +++ b/docs/STEERING.md @@ -41,7 +41,7 @@ The protocol layer is stateless between calls: every read is a live exchange, an ## 4. Owned Paths -- `pyquadcortex/` - the package. Two namespaces: `pyquadcortex/model/` (the model of the unit) and `pyquadcortex/protocol/` (the message-level API, including the committed generated bindings in `pyquadcortex/protocol/proto/`) +- `pyquadcortex/` - the package. Two namespaces: `pyquadcortex/device/` (the model of the unit) and `pyquadcortex/protocol/` (the message-level API, including the committed generated bindings in `pyquadcortex/protocol/proto/`) - `protocol/` - the recovered `.proto` schema and its tooling - `tests/` - the fully offline suite and its fixtures - `examples/` - runnable scripts, also used as hardware-verification shapes diff --git a/docs/architecture.md b/docs/architecture.md index fe1bbc0..62fab9f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -36,7 +36,7 @@ only about the layer directly below it. ``` pyquadcortex/ THE MODEL - what import pyquadcortex hands back - model/device.py connect(): opens the unit, returns a Device. + device/device.py connect(): opens the unit, returns a Device. | Speaks the unit's vocabulary, never the wire. | (Directory, cache and grid land in later stories.) | @@ -75,7 +75,7 @@ only about the layer directly below it. ``` The model calls the protocol layer and never the other way round: nothing under -`pyquadcortex/protocol/` may import from `pyquadcortex/model/`. A caller can use +`pyquadcortex/protocol/` may import from `pyquadcortex/device/`. A caller can use either namespace, or both - `Device.from_client(qc)` puts a model on a protocol connection that is already open. @@ -166,7 +166,7 @@ gate, and the handshake's own version announce would race that READ's reply. `pyproject.toml` declares the console script as `pyquadcortex.protocol.cli:main`; `qcctl` itself is unchanged. -### model/device.py +### device/device.py `pyquadcortex.connect()` opens the unit through `protocol.connect()` and returns a `Device`, which carries the unit's identity and owns the connection. diff --git a/pyquadcortex/__init__.py b/pyquadcortex/__init__.py index 383e468..45d9b61 100644 --- a/pyquadcortex/__init__.py +++ b/pyquadcortex/__init__.py @@ -33,7 +33,7 @@ logging.getLogger(__name__).addHandler(logging.NullHandler()) from pyquadcortex import protocol # noqa: E402 -from pyquadcortex.model import Device, connect # noqa: E402 +from pyquadcortex.device import Device, connect # noqa: E402 from pyquadcortex.protocol import (DeviceLostError, # noqa: E402 DeviceNotFoundError) diff --git a/pyquadcortex/model/__init__.py b/pyquadcortex/device/__init__.py similarity index 56% rename from pyquadcortex/model/__init__.py rename to pyquadcortex/device/__init__.py index bb20e52..05a4480 100644 --- a/pyquadcortex/model/__init__.py +++ b/pyquadcortex/device/__init__.py @@ -4,10 +4,16 @@ the wire; it sits on :mod:`pyquadcortex.protocol` and turns the messages into the unit's own vocabulary - presets, scenes, rows, slots, blocks. +The directory is named ``device`` rather than ``model`` because *model* is +already taken twice over: the protocol layer's ``models.py``, ``Model`` and +``ModelCatalog`` are the device's own word for an amp or a pedal block, and +``docs/domain-model.md`` section 5 gave that word to the virtual device list for +exactly that reason. + Its public names are re-exported from :mod:`pyquadcortex`, which is where callers should import them from. The design is in ``docs/domain-model.md``. """ -from pyquadcortex.model.device import Device, connect +from pyquadcortex.device.device import Device, connect __all__ = ["Device", "connect"] diff --git a/pyquadcortex/model/device.py b/pyquadcortex/device/device.py similarity index 100% rename from pyquadcortex/model/device.py rename to pyquadcortex/device/device.py diff --git a/scripts/check_artifacts.py b/scripts/check_artifacts.py index 61600ff..c00f0b5 100755 --- a/scripts/check_artifacts.py +++ b/scripts/check_artifacts.py @@ -25,7 +25,7 @@ "pyquadcortex/protocol/proto/ProductionAutomation_pb2.py", "pyquadcortex/protocol/cli.py", "pyquadcortex/protocol/client.py", - "pyquadcortex/model/device.py", + "pyquadcortex/device/device.py", "pyquadcortex/_version.py", ) diff --git a/tests/test_import_cleanliness.py b/tests/test_import_cleanliness.py index 5c764a2..0cef51a 100644 --- a/tests/test_import_cleanliness.py +++ b/tests/test_import_cleanliness.py @@ -41,13 +41,13 @@ def test_the_walk_found_both_namespaces(): """Guards the parametrisation: an empty walk would pass vacuously. Both sentinels take the trailing dot, so each one needs a real module - INSIDE its namespace. Without it, `pyquadcortex.model` - the package entry + INSIDE its namespace. Without it, `pyquadcortex.device` - the package entry itself - satisfies the check, and a walk that quietly stopped descending would still look complete. The dot is also what keeps a future top-level - `pyquadcortex/models.py` from standing in for the model namespace. + `pyquadcortex/devices.py` from standing in for the model namespace. """ assert any(m.startswith("pyquadcortex.protocol.") for m in MODULES) - assert any(m.startswith("pyquadcortex.model.") for m in MODULES) + assert any(m.startswith("pyquadcortex.device.") for m in MODULES) @pytest.mark.parametrize("module", ["pyquadcortex"] + MODULES) diff --git a/tests/test_namespace.py b/tests/test_namespace.py index 97b4b0d..c295623 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -146,15 +146,15 @@ def test_the_protocol_sources_were_actually_found(): assert len(PROTOCOL_SOURCES) > 5 -MODEL_PACKAGE = "pyquadcortex.model" +MODEL_PACKAGE = "pyquadcortex.device" def _is_the_model(dotted: str) -> bool: """True for the model package and anything inside it, and nothing else. - The dot boundary matters: a top-level `pyquadcortex/models.py` is a - different module, and a prefix test with no boundary would report importing - it as a layering violation. + The dot boundary matters: a hypothetical top-level `pyquadcortex/devices.py` + is a different module, and a prefix test with no boundary would report + importing it as a layering violation. """ return dotted == MODEL_PACKAGE or dotted.startswith(MODEL_PACKAGE + ".") @@ -181,9 +181,9 @@ def _imported_modules(tree: ast.AST, package: str) -> list[str]: relative imports are resolved against. Covering all the spellings matters because the house style here is the - package-attribute form - `pyquadcortex/model/device.py` opens with - `from pyquadcortex import protocol` - so `from pyquadcortex import model` is - the spelling a future author is most likely to reach for, and it names no + package-attribute form - `pyquadcortex/device/device.py` opens with + `from pyquadcortex import protocol` - so `from pyquadcortex import device` + is the spelling a future author is most likely to reach for, and it names no module at all in the AST. The relative forms need `node.level` resolved for the same reason. """ @@ -222,14 +222,16 @@ def test_the_protocol_layer_never_imports_the_model(source): IMPORT_SPELLINGS = [ - ("absolute from", "from pyquadcortex.model import Device", True), - ("absolute plain", "import pyquadcortex.model", True), - ("package attribute", "from pyquadcortex import model", True), - ("relative from", "from ..model import Device", True), - ("relative attribute", "from .. import model", True), + ("absolute from", "from pyquadcortex.device import Device", True), + ("absolute plain", "import pyquadcortex.device", True), + ("package attribute", "from pyquadcortex import device", True), + ("relative from", "from ..device import Device", True), + ("relative attribute", "from .. import device", True), + ("the module inside it", "from pyquadcortex.device import device", True), ("a sibling module", "from pyquadcortex.protocol import client", False), - ("a top-level models.py", "from pyquadcortex import models", False), - ("the model's own package", "from pyquadcortex.models import X", False), + ("a hypothetical devices.py", "from pyquadcortex import devices", False), + ("a name, not a module", "from pyquadcortex.protocol import open_device", + False), ] From 6904d18852c83c9946d8a6a0b2664f29c0241788 Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Wed, 12 Aug 2026 17:59:57 -0500 Subject: [PATCH 2/8] feat: one translation boundary between screen values and wire values pyquadcortex/device/translate.py is the only place in the model where a screen value becomes a wire value or the other way round: rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, and the four display-unit mappings the protocol layer has measured (input gain dB, lane and mixer dB, tuner reference Hz, hold timing ms). It is one module rather than a convention because the bug it prevents is silent. The protocol layer's own header says it: an edit to the wrong row lands on a real row and reads back perfectly, so nothing tells the caller. Two of the tests therefore read the model package's source rather than calling it - one proves no +1/-1 arithmetic lives outside the boundary, the other proves no model module reaches past it for a protocol conversion helper. Both have guard tests feeding them samples, because a check with blind spots enforces the rule only for the spellings somebody thought of. Three public value types come with it, exported from pyquadcortex: - PresetAddress renders "28C" and parses it back, refusing a malformed address when it is parsed rather than when it is written. It converts through the protocol layer's own slot_to_position pair so the two layers cannot drift. - FootswitchLetter is the model's only footswitch key. A bare integer raises, with a message naming the trap: a footswitch index and a block's column are different numbers that agree often enough to look alike, which is how a block at column 3 assigned to footswitch E came back keyed 4. - SceneLetter is the same type for scenes. Conversions with a measured scale behind them delegate to the protocol-layer helper that carries the measurement and its evidence, and the tests check the boundary against that helper rather than against a number retyped in the test, which would agree with itself forever. Fully offline. Closes the story's acceptance criteria; no hardware needed. --- CLAUDE.md | 1 + changelog.md | 26 ++ docs/STEERING.md | 54 ++++ docs/architecture.md | 31 ++ docs/domain-model.md | 21 ++ pyquadcortex/__init__.py | 6 +- pyquadcortex/device/__init__.py | 5 +- pyquadcortex/device/translate.py | 450 ++++++++++++++++++++++++++ tests/test_namespace.py | 1 + tests/test_translation.py | 534 +++++++++++++++++++++++++++++++ 10 files changed, 1127 insertions(+), 2 deletions(-) create mode 100644 pyquadcortex/device/translate.py create mode 100644 tests/test_translation.py diff --git a/CLAUDE.md b/CLAUDE.md index 5d5a8ee..730677f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,6 +9,7 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr - Dev setup: `uv venv && uv pip install -e ".[dev]"` (or plain venv + pip, see contributing.md). Run tests with `.venv/bin/python -m pytest`. The suite passes offline - no hardware, no `hid` import, no `DYLD_LIBRARY_PATH`. - Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because *model* is the device's own word for an amp or pedal block (`protocol/models.py`, `catalog.Model`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`. +- Every conversion between a screen value and a wire value lives in `pyquadcortex/device/translate.py` and nowhere else in the model: rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, display units. No `+1`/`-1` on a coordinate outside it, and no model module reaching past it for a protocol conversion helper - `tests/test_translation.py` reads the source and proves both. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is. - The model represents what the unit shows, in the unit's own words, and never guesses. A control we understand but cannot yet drive is modelled and REFUSES the operation (ADR-0007); a control we do not understand is omitted, with the reason recorded in `docs/domain-model.md`'s appendix. Nothing ships with a "this might be stale or wrong" caveat. - A model property that reads a device field checks the field is PRESENT (`protocol.field_present`) before reporting it. Most of this schema sits in synthetic `oneof`s, so protobuf returns `""` or `0` for a field the unit never sent, and reporting that as the answer is the guess the rule above forbids. Never cache a reply that came back incomplete - a retry has to be able to recover. - Anything the model caches is valid only while its connection is. A closed `Device` refuses reads rather than answering from cache, because a model that reports the unit's state through an object with no unit behind it is the failure the whole layer exists to avoid. diff --git a/changelog.md b/changelog.md index 1d894d6..c78742c 100644 --- a/changelog.md +++ b/changelog.md @@ -85,6 +85,32 @@ To use both layers in one script, wrap a connection you already have with `Device.from_client(qc)`. It does not take ownership: closing the `Device` leaves your connection open. +### The model talks in the numbers on your screen + +Rows are 1 to 4, slots are 1 to 8, scenes and footswitches are letters, and levels +are the dB the unit displays. The wire counts from zero and stores raw scales, and +the model converts in exactly one place so nothing else has to remember to. + +Three value types come with it, exported from `pyquadcortex`: + +```python +from pyquadcortex import PresetAddress, FootswitchLetter, SceneLetter + +PresetAddress.parse("28C") # bank 28, position C +PresetAddress.parse("28X") # ValueError, here rather than at write time +FootswitchLetter.E # a footswitch is a letter, never a number +``` + +The footswitch rule is worth the sentence it costs. A footswitch index and a +block's column are different numbers that agree most of the time, which is how a +bug hid for months: a block in column 3 assigned to footswitch E is stored under +key 4. No model API takes a bare footswitch number, so a column cannot be passed +where a footswitch belongs. + +There is nothing handing out preset addresses yet - the Directory is still being +built - so today these are useful mostly for validating an address before you use +it with the protocol layer. + ### Withdrawn: the Tempo menu's MODE is "not on the wire" The 0.23.0 entry below records, under **Settled**, that the Tempo menu's MODE diff --git a/docs/STEERING.md b/docs/STEERING.md index 9625328..4ecdf5e 100644 --- a/docs/STEERING.md +++ b/docs/STEERING.md @@ -57,6 +57,7 @@ The protocol layer is stateless between calls: every read is a live exchange, an | Fake-per-layer offline tests | Each layer has a purpose-built double: golden captured frames for `framing`, `FakeHid` for `transport`, `FakeTransport` for `client` | see ADR-0002 | `FakeTransport` in `tests/test_client.py` | Hardware verification happens manually via `examples/`, outside the suite | | Evidence-bearing docstrings | Each operation's docstring states what is confirmed on hardware vs inferred from the schema | The device gives no errors for wrong writes, so recorded evidence is the only trail | `QuadCortex.read_preset` in `pyquadcortex/protocol/client.py` | Non-protocol helpers (pure functions) carry ordinary docstrings | | Keyed grid edits | Mutations are row/column-keyed `Grid` UPDATEs | The device applies grid updates by key; wholesale preset writes are silently ignored (see [`architecture.md`](architecture.md), "write_preset is a trap") | `QuadCortex.set_bypass` in `pyquadcortex/protocol/client.py` | Read paths, and non-grid operations | +| One translation boundary | Screen values become wire values in exactly one module, and a source-reading test proves no other model module does it | An off-by-one row is silent - the write lands on a real row and reads back perfectly - so a convention cannot be trusted to hold (design principle 5 in [`domain-model.md`](domain-model.md)) | `pyquadcortex/device/translate.py` | The protocol layer, which keeps its zero-based indexes and raw scales | ## 6. Constraints @@ -119,6 +120,59 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) ## Change Log +### 2026-08-12 - One translation boundary, and the model package is `device/` + +**What changed:** +- `pyquadcortex/device/translate.py`: the one module where a screen value becomes a wire + value and back - rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, + and the four display-unit mappings the protocol layer has measured (input gain dB, lane + and mixer dB, tuner reference Hz, hold timing ms). `PresetAddress`, `FootswitchLetter` + and `SceneLetter` are its public value types, re-exported from `pyquadcortex` +- Section 5 gained the pattern row; section 4's owned-paths line and CLAUDE.md name the + new rule. `architecture.md` carries the module in its layer map and a section on it; + `domain-model.md` marks principle 5, `PresetAddress` and `FootswitchLetter` as built +- **The model package directory is `pyquadcortex/device/`, renamed from `model/`.** Done + as its own commit so the story's diff stays readable + +**Why:** +- M1 Epic (stokes-audio/pyquadcortex#8), Story #10. It ships before the surfaces that use + it so no later story invents its own conversion. The Intent Brief names off-by-one as a + silent failure mode, and the protocol layer's own header agrees: an edit to the wrong + row still succeeds and still reads back correctly, so nothing tells you. A centralized, + exhaustively tested boundary is the whole mitigation, which is why two of its tests read + the model package's source instead of calling it +- The rename is an owner decision. *Model* already means an amp or pedal block here + (`protocol/models.py`, `catalog.Model`, `ModelCatalog`, `set_block(model=...)`), and + `domain-model.md` §5 settled that collision once by giving the word to the virtual + device list. The package directory had taken it back + +**Scope of impact:** +- **Updated:** STEERING.md, CLAUDE.md, architecture.md, domain-model.md, changelog.md, + `pyquadcortex/device/`, `pyquadcortex/__init__.py`, `scripts/check_artifacts.py`, + `tests/test_translation.py` (new), `tests/test_namespace.py`, + `tests/test_import_cleanliness.py` +- **Not updated (intentionally):** ADR.md - neither change reverses or refines a recorded + decision. The boundary IS design principle 5, already written and reviewed in + `domain-model.md`; the rename is a naming correction that the same document's §5 had + already decided in the other direction. README.md and api.md - the new value types have + no surface handing them out yet (the Directory is story #12), and the readme tour should + show what a caller can do, not what exists. The protocol layer - it keeps its zero-based + indexes, its `Footswitch` enum and its measured scales, and nothing below the seam + changed +- **No deprecation shim for the rename.** `pyquadcortex.__all__` lists `protocol` and never + listed `model`, and the model namespace has not been released - 0.40.0 predates the flip + - so nothing published points at the old path + +**Downstream to consider:** +- Stories #11 through #16 convert through this module rather than doing their own + arithmetic, and the source-reading tests will fail them if they do not +- The conversions M1 does not need yet land here too, with the surface that needs them. + A parameter whose display mapping is unverified stays out of the model entirely + (principle 3), so no mapping is ever invented in this module +- The `+1`/`-1` check is deliberately blunt: any literal one added to or subtracted from + anything in the model package outside the boundary fails it. If a future module has a + genuine counter, widening the check is a deliberate edit with a reason, not a quiet one + ### 2026-08-11 - The namespace flip lands, and ADR-0007 **What changed:** diff --git a/docs/architecture.md b/docs/architecture.md index 62fab9f..0262282 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -40,6 +40,10 @@ only about the layer directly below it. | Speaks the unit's vocabulary, never the wire. | (Directory, cache and grid land in later stories.) | + device/translate.py Screen values <-> wire values, and the ONLY place + | either becomes the other: rows, slots, scene and + | footswitch letters, preset addresses, display units + | | -- the model/protocol seam -- | pyquadcortex/protocol/ THE PROTOCOL LAYER - one call per protocol message @@ -178,6 +182,33 @@ The rest of the model - the Directory, the write-through cache, the loaded prese and the grid - is designed in [domain-model.md](domain-model.md) and is being built story by story. Nothing is stubbed out to look finished. +### device/translate.py + +The model speaks what the touchscreen shows - rows 1 to 4, slots 1 to 8, scenes +and footswitches as letters, dB, Hz, ms - and the wire speaks zero-based indexes +and raw scales. Every conversion between the two lives here and nowhere else in +`pyquadcortex/device/` (design principle 5 in +[domain-model.md](domain-model.md)). + +One module rather than a convention, because the mistake it prevents is silent. +This document's own layer map sits above a protocol layer whose header says it: a +write to the wrong row lands on a real row and reads back perfectly, so nothing +tells the caller. Collecting the arithmetic in one place makes it reviewable in +one place, and `tests/test_translation.py` proves the rest of the model package +does none of it by reading the source, rather than by trusting anyone to +remember. + +Conversions with a measured scale behind them - input gain dB, lane and mixer dB, +the slot-name/position pair - delegate to the protocol-layer helper that carries +the measurement and its evidence, instead of restating the arithmetic. Two copies +of a measured scale drift apart, and both copies go on returning a plausible +number. + +Public value types: `PresetAddress`, `FootswitchLetter`, `SceneLetter`, +re-exported from `pyquadcortex`. The conversion functions are not published: a +caller never needs them, and the model reaches them as +`translate.row_to_wire(...)`. + ## What flows through the layers A host command, top to bottom: diff --git a/docs/domain-model.md b/docs/domain-model.md index 6604074..2e18bf2 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -39,6 +39,8 @@ 5. **One translation boundary.** The model speaks touchscreen coordinates and display units everywhere. Conversion to protocol values (0-based indexes, raw scales) happens in exactly one module at the model-to-protocol seam. No `-1`/`+1` anywhere else. + **Built:** `pyquadcortex/device/translate.py`, with the rule enforced by a test that + reads the model package's source rather than trusting a convention. ## Namespaces: the model becomes the front door @@ -150,6 +152,14 @@ class PresetAddress: > with the mode - linear position 5 reads "1F" normally and "2B" under the hybrid - so an > address is only unambiguous alongside the mode it was read in. +> **`PresetAddress` is built**, in `pyquadcortex/device/translate.py` and exported from +> `pyquadcortex`. It speaks the non-hybrid naming, "A".."H". `PresetAddress.parse("28C")` +> refuses a malformed address there and then, rather than at write time, and `.to_wire()` +> / `.from_wire()` convert through the protocol layer's own `slot_to_position` pair so the +> two layers cannot drift on what "28C" means. The mode caveat above is on the converting +> function's docstring, where someone converting will read it. The Directory that hands +> addresses out is still ahead of the code. + ### Directory items are a type family Everything a Directory list can hold shares an `Item` base - the Directory's own word, @@ -670,6 +680,11 @@ class Stomps: # preset.stomps > eventually passes a column to it and gets a write that silently does nothing, which is > precisely the bug that cost a hardware session to find. > +> **`FootswitchLetter` is built**, in `pyquadcortex/device/translate.py` and exported from +> `pyquadcortex`. It is a `StrEnum`, so `stomps["E"]` and `stomps[FootswitchLetter.E]` are +> the same key and it prints as the screen labels it. Passing the number 4 raises, with a +> message naming the column trap. `SceneLetter` is the same type for scenes. +> > **A device-level footswitch object is deferred, deliberately.** There are now two > footswitch-keyed collections at different scopes - `preset.stomps` per preset and > `settings.looper_actions` global - plus the mode that decides which is live, so nothing @@ -1311,3 +1326,9 @@ the n/a rows below where they intersect the API at all. and refused rather than omitted or guessed (ADR-0007); the appendix row and §13 say the same. Nothing here ships at M1 - tempo is an M3 surface - so this is a design change, not a behaviour change. +- **2026-08-12** - Design principle 5 is built (M1 story #10): `pyquadcortex/device/translate.py` + owns every conversion between a screen value and a wire value, with `PresetAddress`, + `FootswitchLetter` and `SceneLetter` landing as part of it. The model package directory + is `device/` rather than `model/`, because §5 gave the word *model* to the virtual + device list and the directory had taken it back. No design changed here; this records + what is now code. diff --git a/pyquadcortex/__init__.py b/pyquadcortex/__init__.py index 45d9b61..967fbc9 100644 --- a/pyquadcortex/__init__.py +++ b/pyquadcortex/__init__.py @@ -33,7 +33,8 @@ logging.getLogger(__name__).addHandler(logging.NullHandler()) from pyquadcortex import protocol # noqa: E402 -from pyquadcortex.device import Device, connect # noqa: E402 +from pyquadcortex.device import (Device, FootswitchLetter, # noqa: E402 + PresetAddress, SceneLetter, connect) from pyquadcortex.protocol import (DeviceLostError, # noqa: E402 DeviceNotFoundError) @@ -41,6 +42,9 @@ "__version__", "connect", "Device", + "FootswitchLetter", + "SceneLetter", + "PresetAddress", "protocol", "DeviceNotFoundError", "DeviceLostError", diff --git a/pyquadcortex/device/__init__.py b/pyquadcortex/device/__init__.py index 05a4480..1065220 100644 --- a/pyquadcortex/device/__init__.py +++ b/pyquadcortex/device/__init__.py @@ -15,5 +15,8 @@ """ from pyquadcortex.device.device import Device, connect +from pyquadcortex.device.translate import (FootswitchLetter, PresetAddress, + SceneLetter) -__all__ = ["Device", "connect"] +__all__ = ["Device", "connect", "FootswitchLetter", "SceneLetter", + "PresetAddress"] diff --git a/pyquadcortex/device/translate.py b/pyquadcortex/device/translate.py new file mode 100644 index 0000000..00465ce --- /dev/null +++ b/pyquadcortex/device/translate.py @@ -0,0 +1,450 @@ +"""The one place a screen value becomes a wire value, and back. + +The model speaks what the touchscreen shows: rows 1 to 4, slots 1 to 8, scenes +and footswitches as letters, levels in dB, the tuner in Hz. The wire speaks +zero-based indexes and raw scales. Every conversion between the two lives here, +and nowhere else in :mod:`pyquadcortex.device` - design principle 5 in +``docs/domain-model.md``. + +**Why one module rather than a convention.** The protocol layer's own header says +it plainly: rows are zero-based, "getting this wrong is quiet rather than loud - +an edit lands on a real row, just not the one intended, and it reads back +perfectly". There is no error, no wrong-looking value, and no complaint from the +unit. A ``- 1`` written in the wrong place is therefore invisible until someone +plays the preset. Collecting the arithmetic in one module makes it reviewable in +one place, and ``tests/test_translation.py`` proves the rest of the model package +contains none of it. + +Nothing here talks to a device. These are pure functions and value types, so they +are cheap to test exhaustively, which is the point. + +Two words carry two meanings in this file, both of them the unit's own: + +* a **slot** is one of the eight cells in a grid row (``row.slots[3]``), and it + is also a preset's place in a setlist ("28C"). The design doc uses both. The + grid sense converts with :func:`slot_to_wire`; the setlist sense with + :func:`slot_to_position` and :class:`PresetAddress`. +* a **position** is the letter part of a preset address ("C") to the model, and + the linear index of that address (218) to the wire. +""" + +import enum +import re +from dataclasses import dataclass + +from pyquadcortex import protocol + +#: Rows on the touchscreen, top to bottom. The wire numbers the same four 0 to 3. +ROWS = (1, 2, 3, 4) + +#: The eight cells in a row, as the manual counts them ("four rows, each +#: containing eight device block slots"). The wire calls a cell a ``column`` and +#: numbers them 0 to 7. +SLOTS = (1, 2, 3, 4, 5, 6, 7, 8) + +#: The tuner's reference pitch when the wire offset is zero. The wire stores an +#: OFFSET from this, not the pitch itself - see :func:`tuner_reference_hz`. +CONCERT_A_HZ = 440.0 + +# What the wire may carry for each of the above, derived from them so the two +# accounts cannot disagree about how many there are. Scenes and footswitches +# share the eight-index range with slots. +_WIRE_ROWS = tuple(range(len(ROWS))) +_WIRE_COLUMNS = tuple(range(len(SLOTS))) +_LETTERS = "ABCDEFGH" + +#: Only the three value types are re-exported from :mod:`pyquadcortex`; a caller +#: holds those. The conversions are the seam's own business and are reached as +#: ``translate.row_to_wire(...)`` from inside the model. +__all__ = [ + "ROWS", "SLOTS", "CONCERT_A_HZ", + "FootswitchLetter", "SceneLetter", "PresetAddress", + "row_to_wire", "row_from_wire", "slot_to_wire", "slot_from_wire", + "footswitch_to_wire", "footswitch_from_wire", + "scene_to_wire", "scene_from_wire", + "slot_to_position", "position_to_slot", + "input_level_db", "db_to_input_level", + "lane_level_db", "db_to_lane_level", + "tuner_reference_hz", "hz_to_tuner_reference", + "hold_timing_ms", "ms_to_hold_timing", +] + + +def _screen_number(value, what: str, allowed: tuple) -> int: + """Check one screen coordinate before it is converted. + + ``bool`` is refused explicitly because it is a subclass of ``int`` and + ``True == 1``: an unguarded check converts ``True`` to the first row or slot + and edits it, which is exactly the silent wrong answer this module exists to + prevent. A float is refused for the same reason in slower motion - ``1.0`` + means the caller is computing coordinates in a type that rounds. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise TypeError( + f"{what} must be an int, not {type(value).__name__} ({value!r})" + ) + if value not in allowed: + raise ValueError( + f"{what} must be {allowed[0]} to {allowed[-1]} - the unit shows " + f"{len(allowed)} of them; got {value}" + ) + return value + + +def _a_number(value, what: str) -> float: + """A real number, and not a bool wearing one's clothes.""" + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise TypeError( + f"{what} must be a number, not {type(value).__name__} ({value!r})") + return float(value) + + +# -- coordinates: rows and slots -------------------------------------------- + + +def row_to_wire(row: int) -> int: + """The screen's row number (1-4) as the wire's row index (0-3).""" + return _screen_number(row, "a row", ROWS) - 1 + + +def row_from_wire(index: int) -> int: + """The wire's row index (0-3) as the row number the screen shows (1-4).""" + return _screen_number(index, "a wire row index", _WIRE_ROWS) + 1 + + +def slot_to_wire(slot: int) -> int: + """A row's slot number (1-8) as the wire's column index (0-7). + + The manual calls the eight cells in a row slots; the wire calls the same + thing a column. Same cell, two vocabularies, and this is the seam. + """ + return _screen_number(slot, "a slot", SLOTS) - 1 + + +def slot_from_wire(column: int) -> int: + """The wire's column index (0-7) as the slot number the screen shows (1-8).""" + return _screen_number(column, "a wire column index", _WIRE_COLUMNS) + 1 + + +# -- letters: scenes and footswitches --------------------------------------- + + +class FootswitchLetter(enum.StrEnum): + """A footswitch, as the unit labels it: A to H. + + **The model's only public footswitch key.** A footswitch index is not a + column, and the two are equal often enough to look like the same number: + ``stomp_is_momentary`` is keyed by footswitch index, and that stayed hidden + for months because every sample happened to have the two agree - until a + block at column 3 assigned to footswitch E came back keyed 4 + (``docs/domain-model.md`` section 7). Documenting the difference was not + enough, so the model takes a letter and the zero-based index stays inside the + protocol layer, where :class:`~pyquadcortex.protocol.enums.Footswitch` + already lives. + + It is a ``str``, so it prints as the screen shows it and keys an ordinary + mapping:: + + preset.stomps[FootswitchLetter.E] + preset.stomps["E"] # the same key + """ + + A = "A" + B = "B" + C = "C" + D = "D" + E = "E" + F = "F" + G = "G" + H = "H" + + +class SceneLetter(enum.StrEnum): + """A scene, as the unit labels it: A to H. A ``str``, like + :class:`FootswitchLetter`.""" + + A = "A" + B = "B" + C = "C" + D = "D" + E = "E" + F = "F" + G = "G" + H = "H" + + +def _letter(value, kind: type, what: str, trap: str): + """Coerce a caller's letter into `kind`, refusing a number outright. + + A number is refused rather than converted even though the wire is numeric. + ``trap`` names what that number would more likely have been - the mistake the + letter types exist to make impossible. + """ + if isinstance(value, kind): + return value + if isinstance(value, bool) or isinstance(value, (int, float)): + raise TypeError( + f"{what} is a letter A to H, not the number {value!r} - the model " + f"never takes a bare index here, because {trap}" + ) + if not isinstance(value, str): + raise TypeError( + f"{what} is a letter A to H, not {type(value).__name__} ({value!r})") + try: + return kind(value.strip().upper()) + except ValueError: + raise ValueError( + f"{what} is a letter A to H - the unit shows eight; got {value!r}" + ) from None + + +def footswitch_to_wire(footswitch) -> protocol.Footswitch: + """A footswitch letter as the zero-based index the wire carries. + + Takes a :class:`FootswitchLetter` or the plain letter. An ``int`` is refused: + see :class:`FootswitchLetter` for the block-at-column-3 case that makes a + number here a write that silently lands on the wrong switch. + """ + letter = _letter(footswitch, FootswitchLetter, "a footswitch", + "a footswitch index and a block's column are different " + "numbers that are equal often enough to look alike") + return protocol.Footswitch[letter.value] + + +def footswitch_from_wire(index) -> FootswitchLetter: + """The wire's footswitch index (0-7) as the letter the unit labels it with.""" + return FootswitchLetter( + _LETTERS[_screen_number(index, "a wire footswitch index", _WIRE_COLUMNS)]) + + +def scene_to_wire(scene) -> protocol.Scene: + """A scene letter as the zero-based index the wire carries. + + Takes a :class:`SceneLetter` or the plain letter, as ``scenes["B"]`` does. + """ + letter = _letter(scene, SceneLetter, "a scene", + "scene B is wire index 1, and a number here reads as " + "either one") + return protocol.Scene[letter.value] + + +def scene_from_wire(index) -> SceneLetter: + """The wire's scene index (0-7) as the letter the unit labels it with.""" + return SceneLetter( + _LETTERS[_screen_number(index, "a wire scene index", _WIRE_COLUMNS)]) + + +# -- preset addresses: "28C" on screen, a linear position on the wire ------- + + +def slot_to_position(name: str) -> int: + """A preset's slot name ("28C") as the linear position the wire carries (218). + + The letters run **A to H, eight to a bank** - the non-hybrid naming, which is + what the unit shows in every mode except one. A PRESET-containing HYBRID mode + halves the bank to four, so the SAME preset is named differently: linear + position 5 reads "1F" normally and "2B" under that hybrid. So a slot name - + and therefore a :class:`PresetAddress` - is only unambiguous alongside the + mode it was read in. The linear position is not: it means one preset whatever + the footswitches are doing, which is why it is what goes on the wire and why + two addresses are best compared as positions. + + Delegates to :func:`pyquadcortex.protocol.slot_to_position`, so the model and + the protocol layer cannot drift apart on what "28C" means. A zero-padded bank + ("01A") is accepted; :func:`position_to_slot` renders unpadded by default, + because that is what the unit displays. + """ + return protocol.slot_to_position(name) + + +def position_to_slot(position: int, pad: bool = False) -> str: + """The wire's linear position (218) as the slot name the unit shows ("28C"). + + The inverse of :func:`slot_to_position`, and it carries the same caveat: the + name it returns is the non-hybrid one, so it is only unambiguous alongside + the mode the address was read in. + + Unpadded by default ("1A"), which is what the unit displays; ``pad=True`` + gives "01A". + """ + return protocol.position_to_slot(position, pad=pad) + + +@dataclass(frozen=True) +class PresetAddress: + """Where a preset lives, as the Directory shows it: a bank and a position. + + ``PresetAddress(28, "C")`` renders as ``"28C"`` and + :meth:`parse` reads the same form back. Malformed input is refused here, + when the address is built, rather than later when something writes it: a bad + address that survives parsing turns into a wire position anyway, and the + device recalls whatever preset is at that position without complaint. + + ``position`` is the letter, "A" to "H" - the non-hybrid naming. See + :func:`slot_to_position` for why an address needs the mode beside it to be + unambiguous, and why comparing positions beats comparing names. + """ + + bank: int + position: str + + def __post_init__(self): + if isinstance(self.bank, bool) or not isinstance(self.bank, int): + raise TypeError( + f"a bank is a number, not {type(self.bank).__name__} " + f"({self.bank!r})") + if not isinstance(self.position, str): + raise TypeError( + f"a position is a letter A to H, not " + f"{type(self.position).__name__} ({self.position!r})") + object.__setattr__(self, "position", self.position.strip().upper()) + # Validation is the protocol helper's, so there is one account of how big + # a setlist is and what a slot name may look like. + slot_to_position(f"{self.bank}{self.position}") + + def __str__(self) -> str: + return f"{self.bank}{self.position}" + + def __repr__(self) -> str: + return f"PresetAddress({str(self)!r})" + + @classmethod + def parse(cls, text: str) -> "PresetAddress": + """Read an address a person wrote: ``"28C"``, ``"01a"``, ``" 32H "``. + + Raises ``ValueError`` for anything that is not a bank number followed by + a letter A to H, and ``TypeError`` for anything that is not text. + """ + if not isinstance(text, str): + raise TypeError( + f"a preset address is text like '28C', not " + f"{type(text).__name__} ({text!r})") + match = re.fullmatch(r"\s*(\d+)\s*([A-Za-z])\s*", text) + if not match: + raise ValueError( + f"a preset address is a bank number and a letter A to H, like " + f"'28C': {text!r}") + return cls(int(match.group(1)), match.group(2)) + + @classmethod + def from_wire(cls, position: int) -> "PresetAddress": + """The address at a linear wire position: ``218`` gives ``"28C"``.""" + return cls.parse(position_to_slot(position)) + + def to_wire(self) -> int: + """This address as the linear position the wire carries.""" + return slot_to_position(str(self)) + + +# -- display units ---------------------------------------------------------- +# +# Each mapping below was measured on hardware and is documented at the protocol +# layer, on the helper that performs it. The model delegates rather than +# restating the arithmetic: two copies of a measured scale drift, and the drift +# is invisible because both copies still return a plausible number. + + +def input_level_db(level: float) -> float: + """An input port's wire level (0..1) as the dB the unit displays. + + Input gain spans -12 to +60 dB. Delegates to + :func:`pyquadcortex.protocol.input_level_db`, which carries the measurement. + + An input port and a lane are both a 0..1 wire value and they are NOT the + same scale - see :func:`lane_level_db`. + """ + return protocol.input_level_db(level) + + +def db_to_input_level(db: float) -> float: + """Displayed input-gain dB as the wire level an input port takes. + + Refuses anything outside -12..+60 dB rather than clamping, because a clamped + write lands and reads back as a value the caller never asked for. + """ + return protocol.db_to_input_level(db) + + +def lane_level_db(value: float) -> float: + """A lane, mixer or splitter LEVEL wire value (0..1) as displayed dB. + + These span -40 to +12 dB, with 0 dB at :data:`pyquadcortex.protocol.UNITY_LEVEL` + (10/13). Delegates to :func:`pyquadcortex.protocol.lane_level_db`. + + The bottom of the knob is a detent, not a dB value: wire 0.0 reads "Off" on + screen and -39.5 dB (wire 0.01) is the lowest numeric step. This converts the + scale; it does not model the Off position. + """ + return protocol.lane_level_db(value) + + +def db_to_lane_level(db: float) -> float: + """Displayed dB as the wire value a lane, mixer or splitter LEVEL takes. + + Refuses anything outside -40..+12 dB. For silence write the wire's 0.0 + directly - the Off position - rather than converting a dB value. + """ + return protocol.db_to_lane_level(db) + + +def tuner_reference_hz(offset: float) -> float: + """The tuner's wire ``frequency`` as the absolute reference pitch on screen. + + The wire stores an OFFSET from 440 Hz, not the pitch: setting FREQ to 442 on + the unit broadcast ``frequency: 1.99999809``. The screen shows 442, so the + model does too. + + **Evidence:** that single observed pair (442 -> 2.0) is the whole of it. It + fixes the zero point and the direction; that the unit is one Hz per unit + rather than something that merely agrees at 2.0 has not been checked against + a second value on screen, and this function says so rather than implying more + (see :meth:`pyquadcortex.protocol.QuadCortex.set_tuner_reference`). No range + is enforced for the same reason: the unit's FREQ limits have not been read, + and a limit invented here would refuse a setting the unit allows. + """ + return CONCERT_A_HZ + _a_number(offset, "a tuner reference offset") + + +def hz_to_tuner_reference(hz: float) -> float: + """A reference pitch in Hz as the offset from 440 the wire carries. + + Inverse of :func:`tuner_reference_hz`; see it for the evidence and for why + no range is enforced. + """ + return _a_number(hz, "a tuner reference pitch") - CONCERT_A_HZ + + +def hold_timing_ms(index: int) -> int: + """The wire's ``hold_timing`` index as the milliseconds the screen shows. + + Six settings, 500 to 1000 ms in 100 ms steps. The device accepts and stores + any integer in that field without validating it, so an index outside the six + means something wrote a value no screen can show - reported rather than + rounded to the nearest real setting. + """ + choices = protocol.QuadCortex.HOLD_TIMING_MS + if isinstance(index, bool) or not isinstance(index, int) \ + or not 0 <= index < len(choices): + raise ValueError( + f"hold timing reads {index!r}, which is outside the " + f"{len(choices)} values the unit offers - something wrote an " + f"unvalidated value into it" + ) + return choices[index] + + +def ms_to_hold_timing(milliseconds: int) -> int: + """Milliseconds as the ``hold_timing`` index the wire carries. + + Only the six values the unit offers convert. Anything else is refused rather + than rounded, because the device would store it and no gesture would match + it. + """ + choices = protocol.QuadCortex.HOLD_TIMING_MS + try: + return choices.index(int(milliseconds)) + except (ValueError, TypeError): + raise ValueError( + f"hold timing must be one of {list(choices)} ms, " + f"not {milliseconds!r}" + ) from None diff --git a/tests/test_namespace.py b/tests/test_namespace.py index c295623..998bf4d 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -130,6 +130,7 @@ def test_the_model_is_what_the_top_level_offers(): assert pyquadcortex.connect is not protocol.connect assert set(pyquadcortex.__all__) == { "__version__", "connect", "Device", "protocol", + "FootswitchLetter", "SceneLetter", "PresetAddress", "DeviceNotFoundError", "DeviceLostError", } for name in pyquadcortex.__all__: diff --git a/tests/test_translation.py b/tests/test_translation.py new file mode 100644 index 0000000..65fbdd8 --- /dev/null +++ b/tests/test_translation.py @@ -0,0 +1,534 @@ +"""The one place screen coordinates and display units become wire values. + +Design principle 5 in ``docs/domain-model.md``: the model speaks touchscreen +coordinates and display units everywhere, and the conversion happens in exactly +one module. These tests are deliberately exhaustive, because the bug they exist +to stop is silent - the protocol layer's own header says an edit to the wrong row +"lands on a real row, just not the one intended, and it reads back perfectly". +Nothing on the unit and nothing in the reply tells you. So a test is the only +thing that can. + +Two of the checks below are structural rather than behavioural: they read the +model package's source and prove no other module does the arithmetic or reaches +past the boundary for a protocol helper that does. +""" +import ast +import pathlib + +import pytest + +from pyquadcortex import device, protocol +from pyquadcortex.device import translate + + +# -- rows: 1-4 on screen, 0-3 on the wire ------------------------------------ + + +@pytest.mark.parametrize("row,index", [(1, 0), (2, 1), (3, 2), (4, 3)]) +def test_every_row_converts_both_ways(row, index): + assert translate.row_to_wire(row) == index + assert translate.row_from_wire(index) == row + + +@pytest.mark.parametrize("row", [0, 5, -1, 100]) +def test_a_row_the_screen_does_not_show_is_refused(row): + with pytest.raises(ValueError, match="1 to 4"): + translate.row_to_wire(row) + + +@pytest.mark.parametrize("index", [-1, 4, 99]) +def test_a_wire_row_outside_the_grid_is_refused(index): + with pytest.raises(ValueError, match="0 to 3"): + translate.row_from_wire(index) + + +def test_a_bool_is_not_a_row(): + # True == 1, so an unguarded check converts True to wire row 0 and edits the + # top row. bool is a subclass of int, which is why this needs saying. + with pytest.raises(TypeError): + translate.row_to_wire(True) + + +def test_a_float_is_not_a_row(): + with pytest.raises(TypeError): + translate.row_to_wire(1.0) + + +# -- slots: 1-8 on screen, columns 0-7 on the wire --------------------------- + + +@pytest.mark.parametrize("slot,column", list(zip(range(1, 9), range(0, 8)))) +def test_every_slot_converts_both_ways(slot, column): + assert translate.slot_to_wire(slot) == column + assert translate.slot_from_wire(column) == slot + + +@pytest.mark.parametrize("slot", [0, 9, -1]) +def test_a_slot_the_screen_does_not_show_is_refused(slot): + with pytest.raises(ValueError, match="1 to 8"): + translate.slot_to_wire(slot) + + +@pytest.mark.parametrize("column", [-1, 8]) +def test_a_wire_column_outside_the_row_is_refused(column): + with pytest.raises(ValueError, match="0 to 7"): + translate.slot_from_wire(column) + + +def test_a_bool_is_not_a_slot(): + with pytest.raises(TypeError): + translate.slot_to_wire(True) + + +# -- footswitches: letters in the model, indexes on the wire ----------------- +# +# The protocol layer's `Footswitch` enum is the reference. It is also the reason +# this type exists: `stomp_is_momentary` is keyed by footswitch index, and a +# block at column 3 assigned to footswitch E comes back keyed 4 (domain-model.md +# section 7). Where a bare int can reach a model API, someone eventually passes +# a column to it. + +LETTERS = ("A", "B", "C", "D", "E", "F", "G", "H") + + +@pytest.mark.parametrize("letter", LETTERS) +def test_every_footswitch_converts_both_ways(letter): + reference = getattr(protocol.Footswitch, letter) + switch = translate.FootswitchLetter(letter) + assert translate.footswitch_to_wire(switch) == reference + assert translate.footswitch_from_wire(reference) is switch + + +def test_a_footswitch_letter_is_a_string(): + """So it prints as the screen shows it and keys an ordinary dict.""" + assert str(translate.FootswitchLetter.E) == "E" + assert translate.FootswitchLetter.E == "E" + assert {translate.FootswitchLetter.E: "vibe"}["E"] == "vibe" + + +def test_a_plain_letter_is_accepted_where_a_footswitch_is_wanted(): + assert translate.footswitch_to_wire("E") == protocol.Footswitch.E + assert translate.footswitch_to_wire("e") == protocol.Footswitch.E + + +def test_a_bare_integer_is_never_a_footswitch(): + """The whole reason FootswitchLetter exists. 4 is E, and it is also column 5.""" + with pytest.raises(TypeError, match="column"): + translate.footswitch_to_wire(4) + + +def test_a_letter_no_footswitch_carries_is_refused(): + with pytest.raises(ValueError, match="A to H"): + translate.footswitch_to_wire("J") + + +@pytest.mark.parametrize("index", [-1, 8]) +def test_a_wire_footswitch_index_off_the_pedalboard_is_refused(index): + with pytest.raises(ValueError): + translate.footswitch_from_wire(index) + + +# -- scenes: letters in the model, indexes on the wire ----------------------- + + +@pytest.mark.parametrize("letter", LETTERS) +def test_every_scene_converts_both_ways(letter): + reference = getattr(protocol.Scene, letter) + scene = translate.SceneLetter(letter) + assert translate.scene_to_wire(scene) == reference + assert translate.scene_from_wire(reference) is scene + + +def test_a_plain_letter_is_accepted_where_a_scene_is_wanted(): + assert translate.scene_to_wire("b") == protocol.Scene.B + + +def test_a_bare_integer_is_never_a_scene(): + with pytest.raises(TypeError): + translate.scene_to_wire(1) + + +def test_a_letter_no_scene_carries_is_refused(): + with pytest.raises(ValueError, match="A to H"): + translate.scene_to_wire("I") + + +# -- preset addresses: "28C" on screen, 218 on the wire ---------------------- + + +def test_an_address_renders_the_way_the_directory_shows_it(): + assert str(translate.PresetAddress(28, "C")) == "28C" + assert str(translate.PresetAddress(1, "A")) == "1A" + + +def test_an_address_parses_the_same_form_it_renders(): + assert translate.PresetAddress.parse("28C") == translate.PresetAddress(28, "C") + + +def test_a_padded_bank_parses_and_renders_unpadded(): + """The unit displays "1A". `slot_to_position` takes "01A" too, so parsing + accepts it and rendering does not produce it.""" + assert translate.PresetAddress.parse("01A") == translate.PresetAddress(1, "A") + assert str(translate.PresetAddress.parse("01A")) == "1A" + + +@pytest.mark.parametrize("written,bank,position", [ + (" 28c ", 28, "C"), + ("32H", 32, "H"), + ("1a", 1, "A"), +]) +def test_parsing_normalises_what_a_person_types(written, bank, position): + address = translate.PresetAddress.parse(written) + assert (address.bank, address.position) == (bank, position) + + +@pytest.mark.parametrize("malformed", [ + "", " ", "C", "28", "28I", "0A", "33A", "28CC", "-1A", "2.5C", "A28", +]) +def test_a_malformed_address_is_refused_when_it_is_parsed(malformed): + """Not when it is written. A bad address that survives parsing becomes a + wire position, and a wrong position is a preset that recalls fine and is the + wrong preset.""" + with pytest.raises(ValueError): + translate.PresetAddress.parse(malformed) + + +@pytest.mark.parametrize("wrong_type", [None, 218, ["28C"]]) +def test_an_address_that_is_not_text_is_refused(wrong_type): + with pytest.raises(TypeError): + translate.PresetAddress.parse(wrong_type) + + +@pytest.mark.parametrize("bank,position", [(0, "A"), (33, "A"), (28, "I"), + (28, ""), (28, "CC")]) +def test_an_impossible_address_is_refused_at_construction(bank, position): + with pytest.raises(ValueError): + translate.PresetAddress(bank, position) + + +def test_an_address_is_hashable_and_compares_by_value(): + a = translate.PresetAddress(28, "C") + assert a == translate.PresetAddress.parse("28C") + assert len({a, translate.PresetAddress(28, "C")}) == 1 + + +ALL_POSITIONS = list(range(32 * 8)) + + +@pytest.mark.parametrize("position", ALL_POSITIONS) +def test_every_address_in_a_setlist_round_trips_against_the_protocol_layer(position): + """All 256 of them, against the protocol layer's own pair as the reference.""" + name = protocol.position_to_slot(position) + address = translate.PresetAddress.from_wire(position) + assert str(address) == name + assert address.to_wire() == position + assert translate.slot_to_position(name) == protocol.slot_to_position(name) + assert translate.position_to_slot(position) == name + + +def test_a_padded_render_is_available_for_the_wire_facing_form(): + assert translate.position_to_slot(0, pad=True) == "01A" + assert protocol.position_to_slot(0, pad=True) == "01A" + + +@pytest.mark.parametrize("position", [-1, 256, 1000]) +def test_a_wire_position_outside_a_setlist_is_refused(position): + with pytest.raises(ValueError): + translate.position_to_slot(position) + with pytest.raises(ValueError): + translate.PresetAddress.from_wire(position) + + +def test_the_address_conversion_says_the_naming_depends_on_the_mode(): + """An address is only unambiguous alongside the mode it was read in: linear + position 5 reads "1F" normally and "2B" under a PRESET-containing HYBRID. + A caller who does not know that will mis-address a preset, so the function + that converts has to say it.""" + doc = translate.slot_to_position.__doc__ + assert "mode" in doc and "unambiguous" in doc + assert "HYBRID" in doc or "hybrid" in doc + + +# -- display units ----------------------------------------------------------- +# +# Each of these has a protocol-layer helper that already carries the measurement +# and its evidence. The model must not restate the arithmetic, so these tests +# check the boundary against that helper rather than against a number retyped +# here - a retyped constant agrees with itself forever. + + +class Recorder: + """The smallest transport a `QuadCortex` needs to record what it would send.""" + + def __init__(self): + self.sent = [] + + def send(self, message): + self.sent.append(message) + + +WIRE_LEVELS = [0.0, 0.01, 0.16667, 0.4, 0.5, protocol.UNITY_LEVEL, 0.9, 1.0] + + +@pytest.mark.parametrize("level", WIRE_LEVELS) +def test_input_level_matches_the_protocol_layers_own_conversion(level): + assert translate.input_level_db(level) == protocol.input_level_db(level) + + +@pytest.mark.parametrize("db", [-12.0, -6.0, 0.0, 17.2, 24.0, 60.0]) +def test_input_level_round_trips_through_the_wire_scale(db): + assert translate.input_level_db(translate.db_to_input_level(db)) == \ + pytest.approx(db) + assert translate.db_to_input_level(db) == protocol.db_to_input_level(db) + + +@pytest.mark.parametrize("db", [-12.1, 60.1, -100.0]) +def test_an_input_gain_the_unit_has_no_setting_for_is_refused(db): + with pytest.raises(ValueError, match="-12"): + translate.db_to_input_level(db) + + +@pytest.mark.parametrize("value", WIRE_LEVELS) +def test_lane_level_matches_the_protocol_layers_own_conversion(value): + assert translate.lane_level_db(value) == protocol.lane_level_db(value) + + +@pytest.mark.parametrize("db", [-40.0, -39.5, -3.1, 0.0, 6.0, 12.0]) +def test_lane_level_round_trips_through_the_wire_scale(db): + assert translate.lane_level_db(translate.db_to_lane_level(db)) == \ + pytest.approx(db) + assert translate.db_to_lane_level(db) == protocol.db_to_lane_level(db) + + +def test_unity_on_a_lane_level_is_zero_db(): + """10/13, measured on every row carrying one across 17 factory presets. + + The tolerance is absolute because the target is zero, and loose because + `UNITY_LEVEL` is 10/13 rounded to eight decimals - well inside what the + screen can show. + """ + assert translate.lane_level_db(protocol.UNITY_LEVEL) == \ + pytest.approx(0.0, abs=1e-6) + + +@pytest.mark.parametrize("db", [-40.1, 12.1, 60.0]) +def test_a_lane_level_the_unit_has_no_setting_for_is_refused(db): + with pytest.raises(ValueError, match="-40"): + translate.db_to_lane_level(db) + + +def test_the_two_level_scales_are_not_interchangeable(): + """Both are a 0..1 wire value and they mean different dB. Reading a lane + level with the input mapping is a wrong answer that looks plausible.""" + assert translate.input_level_db(0.5) != translate.lane_level_db(0.5) + + +# -- the tuner's reference pitch: absolute Hz on screen, an offset on the wire + + +@pytest.mark.parametrize("hz,offset", [(440.0, 0.0), (442.0, 2.0), (445.0, 5.0), + (436.5, -3.5)]) +def test_the_tuner_reference_converts_both_ways(hz, offset): + assert translate.hz_to_tuner_reference(hz) == pytest.approx(offset) + assert translate.tuner_reference_hz(offset) == pytest.approx(hz) + + +def test_the_tuner_reference_is_what_the_protocol_write_expects(): + """The reference is the protocol method itself: 442 Hz on screen broadcast + `frequency: 1.99999809`, so the wire carries the offset from 440.""" + recorder = Recorder() + qc = protocol.QuadCortex(recorder) + qc.set_tuner_reference(translate.hz_to_tuner_reference(442.0)) + assert recorder.sent[-1].frequency == pytest.approx(2.0) + + +def test_the_tuner_reference_refuses_something_that_is_not_a_pitch(): + with pytest.raises(TypeError): + translate.hz_to_tuner_reference("442") + + +# -- hold timing: milliseconds on screen, one of six indexes on the wire ------ + + +@pytest.mark.parametrize("index", range(6)) +def test_every_hold_timing_index_reads_as_the_screens_milliseconds(index): + reference = protocol.QuadCortex.HOLD_TIMING_MS + assert translate.hold_timing_ms(index) == reference[index] + assert translate.ms_to_hold_timing(reference[index]) == index + + +def test_hold_timing_is_what_the_protocol_write_expects(): + recorder = Recorder() + qc = protocol.QuadCortex(recorder) + qc.set_hold_timing(800) + assert recorder.sent[-1].hold_timing == translate.ms_to_hold_timing(800) + + +@pytest.mark.parametrize("ms", [499, 550, 1100, 0]) +def test_a_hold_timing_the_unit_does_not_offer_is_refused(ms): + """The device stores 0 or 5000 as happily as a real index and validates + nothing, so a value that is not one of the six is a setting no screen can + show and no gesture will match.""" + with pytest.raises(ValueError): + translate.ms_to_hold_timing(ms) + + +@pytest.mark.parametrize("index", [-1, 6, 5000]) +def test_a_hold_timing_index_the_unit_cannot_have_written_is_refused(index): + with pytest.raises(ValueError): + translate.hold_timing_ms(index) + + +# -- the boundary is the ONLY place ----------------------------------------- +# +# Everything above proves the conversions are right. These two prove they are +# the only ones, which is the half a reviewer cannot check by reading a diff: +# a stray `- 1` in a future module is one character, looks deliberate, and +# produces an edit that lands on a real row and reads back perfectly. + +BOUNDARY = pathlib.Path(translate.__file__).resolve() +MODEL_SOURCES = sorted( + p for p in pathlib.Path(device.__file__).resolve().parent.rglob("*.py")) +OTHER_MODEL_SOURCES = [p for p in MODEL_SOURCES if p != BOUNDARY] + + +def _off_by_one_arithmetic(tree: ast.AST) -> list[int]: + """Line numbers of every `x + 1` / `x - 1` / `x += 1` in `tree`. + + Blunt on purpose. A rule that only fired on operands spelled `row` or `slot` + would miss `n - 1`, and `n` is what the arithmetic is called by the time + someone has extracted a helper for it. + """ + def is_one(node) -> bool: + return (isinstance(node, ast.Constant) + and type(node.value) is int and node.value == 1) + + hits = [] + for node in ast.walk(tree): + if isinstance(node, ast.BinOp) and isinstance(node.op, (ast.Add, ast.Sub)): + if is_one(node.left) or is_one(node.right): + hits.append(node.lineno) + elif isinstance(node, ast.AugAssign) \ + and isinstance(node.op, (ast.Add, ast.Sub)) and is_one(node.value): + hits.append(node.lineno) + return sorted(set(hits)) + + +#: Protocol-layer names that carry a coordinate or a raw scale. Reaching for one +#: of these outside the boundary is how a second conversion gets written. +PROTOCOL_CONVERSIONS = { + "Footswitch", "Scene", "slot_to_position", "position_to_slot", + "input_level_db", "db_to_input_level", "lane_level_db", "db_to_lane_level", + "UNITY_LEVEL", "HOLD_TIMING_MS", +} + + +def _protocol_conversions_used(tree: ast.AST) -> list[str]: + """Every protocol-layer conversion name `tree` reaches for. + + Only through the protocol layer: `translate.slot_to_position(...)` is the + boundary doing its job and must not be reported, so a bare attribute name is + not enough to accuse on. + """ + def is_the_protocol_layer(node) -> bool: + return ((isinstance(node, ast.Name) and node.id == "protocol") + or (isinstance(node, ast.Attribute) and node.attr == "protocol")) + + found = [] + for node in ast.walk(tree): + if isinstance(node, ast.Attribute) and node.attr in PROTOCOL_CONVERSIONS \ + and is_the_protocol_layer(node.value): + found.append(node.attr) + elif isinstance(node, ast.ImportFrom) \ + and (node.module or "").startswith("pyquadcortex.protocol"): + found += [a.name for a in node.names if a.name in PROTOCOL_CONVERSIONS] + return sorted(set(found)) + + +def test_the_source_walk_found_the_model_package(): + """Guards both checks below: an empty list passes them vacuously.""" + assert BOUNDARY in MODEL_SOURCES + assert len(OTHER_MODEL_SOURCES) >= 2 + + +def test_the_boundary_itself_does_the_arithmetic(): + """The exclusion below has to be load-bearing. If the boundary stopped + converting, the check would pass because nothing anywhere converts - which + is the one failure a "no arithmetic elsewhere" test cannot see.""" + hits = _off_by_one_arithmetic(ast.parse(BOUNDARY.read_text())) + assert hits, f"{BOUNDARY.name} does no +1/-1 arithmetic at all" + + +@pytest.mark.parametrize("source", OTHER_MODEL_SOURCES, ids=lambda p: p.name) +def test_no_index_arithmetic_outside_the_boundary(source): + hits = _off_by_one_arithmetic(ast.parse(source.read_text())) + assert not hits, ( + f"{source.name} does +1/-1 arithmetic at line(s) {hits}. If that is a " + f"screen coordinate becoming a wire index, it belongs in " + f"{BOUNDARY.name} with a test - an off-by-one here edits a real row, " + f"just not the one intended, and reads back perfectly" + ) + + +@pytest.mark.parametrize("source", OTHER_MODEL_SOURCES, ids=lambda p: p.name) +def test_only_the_boundary_reaches_for_a_protocol_conversion(source): + found = _protocol_conversions_used(ast.parse(source.read_text())) + assert not found, ( + f"{source.name} uses the protocol layer's {found} directly. Convert " + f"through {BOUNDARY.name} instead, so there is one account of what a " + f"row, a slot, a letter or a dB means" + ) + + +ARITHMETIC_SAMPLES = [ + ("a bare decrement", "wire_row = row - 1", True), + ("a bare increment", "row = wire_row + 1", True), + ("an augmented one", "slot += 1", True), + ("one on the left", "column = 1 - offset", True), + ("hidden in a call", "qc.set_param(row=row - 1, column=slot - 1)", True), + ("a comprehension", "[s - 1 for s in slots]", True), + ("the boundary doing it", "wire_row = translate.row_to_wire(row)", False), + ("arithmetic that is not off-by-one", "total = a + 2", False), + ("a true that is not a one", "flag = other + True", False), +] + + +@pytest.mark.parametrize("label,source,detected", ARITHMETIC_SAMPLES, + ids=[s[0] for s in ARITHMETIC_SAMPLES]) +def test_the_arithmetic_check_sees_what_it_claims_to(label, source, detected): + """A check with blind spots enforces the rule only for the spellings + somebody happened to think of.""" + assert bool(_off_by_one_arithmetic(ast.parse(source))) is detected + + +CONVERSION_SAMPLES = [ + ("an attribute", "x = protocol.Footswitch.A", True), + ("through the package", "x = pyquadcortex.protocol.lane_level_db(v)", True), + ("an import", "from pyquadcortex.protocol import slot_to_position", True), + ("a module import", "from pyquadcortex.protocol.client import UNITY_LEVEL", True), + ("the boundary's own name", "x = translate.slot_to_position(name)", False), + ("a protocol name that is not a conversion", + "x = protocol.field_present(reply, 'serial')", False), +] + + +@pytest.mark.parametrize("label,source,detected", CONVERSION_SAMPLES, + ids=[s[0] for s in CONVERSION_SAMPLES]) +def test_the_reach_past_the_boundary_check_sees_what_it_claims_to(label, source, + detected): + assert bool(_protocol_conversions_used(ast.parse(source))) is detected + + +# -- what a caller can import ------------------------------------------------ + + +def test_the_letter_types_and_the_address_are_public(): + """A caller holds these: `preset.stomps[FootswitchLetter.E]` and + `device.recall(PresetAddress.parse("28C"))`. The conversion functions are + not published - they are the seam's own business.""" + import pyquadcortex + + assert pyquadcortex.FootswitchLetter is translate.FootswitchLetter + assert pyquadcortex.SceneLetter is translate.SceneLetter + assert pyquadcortex.PresetAddress is translate.PresetAddress + assert not hasattr(pyquadcortex, "row_to_wire") From b3cda52bce8763c63df0eb2acc0f32d8d686785a Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Wed, 12 Aug 2026 18:25:54 -0500 Subject: [PATCH 3/8] review: close the findings from the PR #21 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guards were weaker than they read, which is the failure this story is about. The arithmetic check saw `row - 1` and nothing else. It missed `ord(letter) - ord("A")`, `chr(ord("A") + i)`, a letter table, `divmod(position, 8)`, and the three ways of writing one that are not the token `1`: `-1`, `1.0` and `True`. Those are not exotic spellings - they are how a person writes the letter and address conversions this module owns. Worse, a sample asserted that `row - True` was correctly ignored, when `True == 1` makes it a real off-by-one and the same bool-is-an-int trap the module's own type guard exists to catch. The reach-past-the-boundary check only recognised an attribute whose parent was literally named `protocol`, so `protocol.QuadCortex.HOLD_TIMING_MS`, an aliased package, and an imported submodule all walked past it. It now resolves which local names mean the protocol layer before accusing anything. Both sample tables were self-confirming: every positive was a spelling already handled. The blind spots above are now positives in them. The scan was scoped to pyquadcortex/device/, so the whole rule was satisfiable by putting the arithmetic in pyquadcortex/coords.py, one directory up - which is where a failure message naming a directory sends you. It now covers every source file in the package that is not the protocol layer, and a guard checks that set against the import machinery's own walk so a module added tomorrow is covered. Also closed, all of them the same shape - a wrong value that looks right: - The protocol layer's coordinate enums are IntEnums, so Scene.B is an int equal to 1 and converted to a row without complaint. Any enum is refused now, and the two wire-index converters unwrap the RIGHT enum themselves. - SceneLetter and FootswitchLetter are both StrEnums over A to H, so each was accepted where the other belonged. Refused. - position_to_slot and PresetAddress.from_wire took int(position), so 218.9 was preset 218 and True was preset 1 - on the one path where a wrong answer recalls a real preset. - The level converters passed bools and strings through to the protocol layer: lane_level_db(True) returned +12 dB, and a string came back as a TypeError about multiplying a sequence. - ms_to_hold_timing rounded 500.9 to a valid setting and read "500" as a number, while its docstring said it refused rather than rounded. It refuses now. - hold_timing_ms raised ValueError for a wrong type where the rest of the module raises TypeError. - PresetAddress.parse accepted "28 C" and, because Python's \d spans every Unicode digit, read "٢٨C" as bank 28. - test_namespace's MODEL_PACKAGE was a hardcoded string, which would have gone vacuous the next time the package moved. Read from the package now. - translate.py added to check_artifacts.py's REQUIRED list; it is an import-time dependency of the package. Two claims corrected rather than defended. The rename rationale said section 5 gave the word "model" to the virtual device list, which is backwards - it took that word away and gave the concept the screen's name. The real reason stands without it: the protocol layer spells an amp or pedal block `model` in code and will keep doing so. And the display-unit comment claimed all four mappings delegate to a protocol helper; two do, and the tuner and hold timing have no helper to call. The test file now says what its equality assertions actually prove and names the tests in test_client.py where the measured numbers are pinned. --- CLAUDE.md | 2 +- changelog.md | 17 +- docs/STEERING.md | 31 ++-- docs/architecture.md | 17 +- docs/domain-model.md | 7 +- pyquadcortex/device/__init__.py | 12 +- pyquadcortex/device/translate.py | 166 ++++++++++++----- scripts/check_artifacts.py | 1 + tests/test_namespace.py | 8 +- tests/test_translation.py | 298 +++++++++++++++++++++++++++---- 10 files changed, 440 insertions(+), 119 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 730677f..17e38b4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr ## Conventions - Dev setup: `uv venv && uv pip install -e ".[dev]"` (or plain venv + pip, see contributing.md). Run tests with `.venv/bin/python -m pytest`. The suite passes offline - no hardware, no `hid` import, no `DYLD_LIBRARY_PATH`. -- Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because *model* is the device's own word for an amp or pedal block (`protocol/models.py`, `catalog.Model`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`. +- Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because in this codebase the identifier `model` means an amp or pedal block (`protocol/models.py`, `catalog.Model`, `ModelCatalog`, `set_block(model=...)`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`. - Every conversion between a screen value and a wire value lives in `pyquadcortex/device/translate.py` and nowhere else in the model: rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, display units. No `+1`/`-1` on a coordinate outside it, and no model module reaching past it for a protocol conversion helper - `tests/test_translation.py` reads the source and proves both. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is. - The model represents what the unit shows, in the unit's own words, and never guesses. A control we understand but cannot yet drive is modelled and REFUSES the operation (ADR-0007); a control we do not understand is omitted, with the reason recorded in `docs/domain-model.md`'s appendix. Nothing ships with a "this might be stale or wrong" caveat. - A model property that reads a device field checks the field is PRESENT (`protocol.field_present`) before reporting it. Most of this schema sits in synthetic `oneof`s, so protobuf returns `""` or `0` for a field the unit never sent, and reporting that as the answer is the guess the rule above forbids. Never cache a reply that came back incomplete - a retry has to be able to recover. diff --git a/changelog.md b/changelog.md index c78742c..74e96a6 100644 --- a/changelog.md +++ b/changelog.md @@ -85,13 +85,15 @@ To use both layers in one script, wrap a connection you already have with `Device.from_client(qc)`. It does not take ownership: closing the `Device` leaves your connection open. -### The model talks in the numbers on your screen +### Groundwork: the model will talk in the numbers on your screen -Rows are 1 to 4, slots are 1 to 8, scenes and footswitches are letters, and levels -are the dB the unit displays. The wire counts from zero and stores raw scales, and -the model converts in exactly one place so nothing else has to remember to. +Rows will be 1 to 4, slots 1 to 8, scenes and footswitches letters, and levels the +dB the unit displays. The wire counts from zero and stores raw scales, and the +model now converts in exactly one place so nothing else has to remember to. -Three value types come with it, exported from `pyquadcortex`: +**Nothing that reads a row or a level exists yet** - the preset and grid surfaces +are still being built - so what you can use today is the three value types this +groundwork brought with it, exported from `pyquadcortex`: ```python from pyquadcortex import PresetAddress, FootswitchLetter, SceneLetter @@ -107,9 +109,8 @@ bug hid for months: a block in column 3 assigned to footswitch E is stored under key 4. No model API takes a bare footswitch number, so a column cannot be passed where a footswitch belongs. -There is nothing handing out preset addresses yet - the Directory is still being -built - so today these are useful mostly for validating an address before you use -it with the protocol layer. +Until the Directory arrives, `PresetAddress` is most useful for checking an +address before you hand it to the protocol layer. ### Withdrawn: the Tempo menu's MODE is "not on the wire" diff --git a/docs/STEERING.md b/docs/STEERING.md index 4ecdf5e..80e6e68 100644 --- a/docs/STEERING.md +++ b/docs/STEERING.md @@ -125,9 +125,12 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) **What changed:** - `pyquadcortex/device/translate.py`: the one module where a screen value becomes a wire value and back - rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, - and the four display-unit mappings the protocol layer has measured (input gain dB, lane - and mixer dB, tuner reference Hz, hold timing ms). `PresetAddress`, `FootswitchLetter` - and `SceneLetter` are its public value types, re-exported from `pyquadcortex` + and four display-unit mappings (input gain dB, lane and mixer dB, tuner reference Hz, + hold timing ms). The two level scales call the protocol helper that carries the + measurement; the other two have no helper to call, so they are pinned against what the + protocol write method expects, and the tuner's docstring says how thin its evidence is - + one observed pair. `PresetAddress`, `FootswitchLetter` and `SceneLetter` are its public + value types, re-exported from `pyquadcortex` - Section 5 gained the pattern row; section 4's owned-paths line and CLAUDE.md name the new rule. `architecture.md` carries the module in its layer map and a section on it; `domain-model.md` marks principle 5, `PresetAddress` and `FootswitchLetter` as built @@ -141,10 +144,13 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) row still succeeds and still reads back correctly, so nothing tells you. A centralized, exhaustively tested boundary is the whole mitigation, which is why two of its tests read the model package's source instead of calling it -- The rename is an owner decision. *Model* already means an amp or pedal block here - (`protocol/models.py`, `catalog.Model`, `ModelCatalog`, `set_block(model=...)`), and - `domain-model.md` §5 settled that collision once by giving the word to the virtual - device list. The package directory had taken it back +- The rename is an owner decision. In this codebase the identifier `model` means an amp or + pedal block - `protocol/models.py`, `catalog.Model`, `ModelCatalog`, + `set_block(model=...)`. `domain-model.md` §5 renamed that concept to *virtual device* in + the model's vocabulary, because that is what the screen calls it, but the protocol layer + still spells it `model` and will keep doing so. A directory named `model/` therefore + collides with real code a reader is looking at, whatever the design doc calls the + concept **Scope of impact:** - **Updated:** STEERING.md, CLAUDE.md, architecture.md, domain-model.md, changelog.md, @@ -169,9 +175,14 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) - The conversions M1 does not need yet land here too, with the surface that needs them. A parameter whose display mapping is unverified stays out of the model entirely (principle 3), so no mapping is ever invented in this module -- The `+1`/`-1` check is deliberately blunt: any literal one added to or subtracted from - anything in the model package outside the boundary fails it. If a future module has a - genuine counter, widening the check is a deliberate edit with a reason, not a quiet one +- The arithmetic check is deliberately blunt and deliberately wide: a literal one in any + spelling (`1`, `1.0`, `True`, `-1`), `ord`/`chr`, a letter table, `divmod`, and a + one-based `enumerate` all fail it, anywhere in the package outside the boundary and the + protocol layer. If a future module has a genuine counter, widening the check is a + deliberate edit with a reason, not a quiet one +- The scan is scoped to the whole package rather than to `pyquadcortex/device/`, because + a rule scoped to a directory is satisfiable by moving the code one directory up - which + is precisely what a failure message naming a directory invites ### 2026-08-11 - The namespace flip lands, and ADR-0007 diff --git a/docs/architecture.md b/docs/architecture.md index 0262282..7a17bdf 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -198,16 +198,17 @@ one place, and `tests/test_translation.py` proves the rest of the model package does none of it by reading the source, rather than by trusting anyone to remember. -Conversions with a measured scale behind them - input gain dB, lane and mixer dB, -the slot-name/position pair - delegate to the protocol-layer helper that carries -the measurement and its evidence, instead of restating the arithmetic. Two copies -of a measured scale drift apart, and both copies go on returning a plausible -number. +Where a protocol-layer helper already performs the conversion - input gain dB, +lane and mixer dB, the slot-name/position pair - this module calls it instead of +restating the arithmetic. Two copies of a measured scale drift apart, and both +copies go on returning a plausible number. The tuner and hold-timing mappings +have no helper to call, only a documented rule and a shared constant, so their +tests pin them against what the protocol write method expects. Public value types: `PresetAddress`, `FootswitchLetter`, `SceneLetter`, -re-exported from `pyquadcortex`. The conversion functions are not published: a -caller never needs them, and the model reaches them as -`translate.row_to_wire(...)`. +re-exported from `pyquadcortex`. The conversion functions are the module's own +surface and are not re-exported: a caller never needs them, and the model reaches +them as `translate.row_to_wire(...)`. ## What flows through the layers diff --git a/docs/domain-model.md b/docs/domain-model.md index 2e18bf2..8b2e694 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -1329,6 +1329,7 @@ the n/a rows below where they intersect the API at all. - **2026-08-12** - Design principle 5 is built (M1 story #10): `pyquadcortex/device/translate.py` owns every conversion between a screen value and a wire value, with `PresetAddress`, `FootswitchLetter` and `SceneLetter` landing as part of it. The model package directory - is `device/` rather than `model/`, because §5 gave the word *model* to the virtual - device list and the directory had taken it back. No design changed here; this records - what is now code. + is `device/` rather than `model/`, because the protocol layer spells an amp or pedal + block `model` in code (`models.py`, `Model`, `ModelCatalog`) and will keep doing so, + whatever §5 renamed the concept to in this document. No design changed here; this + records what is now code. diff --git a/pyquadcortex/device/__init__.py b/pyquadcortex/device/__init__.py index 1065220..9843273 100644 --- a/pyquadcortex/device/__init__.py +++ b/pyquadcortex/device/__init__.py @@ -4,11 +4,13 @@ the wire; it sits on :mod:`pyquadcortex.protocol` and turns the messages into the unit's own vocabulary - presets, scenes, rows, slots, blocks. -The directory is named ``device`` rather than ``model`` because *model* is -already taken twice over: the protocol layer's ``models.py``, ``Model`` and -``ModelCatalog`` are the device's own word for an amp or a pedal block, and -``docs/domain-model.md`` section 5 gave that word to the virtual device list for -exactly that reason. +The directory is named ``device`` rather than ``model`` because in this codebase +the identifier ``model`` already means an amp or a pedal block: the protocol +layer's ``models.py``, ``Model``, ``ModelCatalog`` and ``set_block(model=...)`` +are all that sense of the word. ``docs/domain-model.md`` section 5 renamed that +concept to *virtual device* in the model's own vocabulary, which is what the +screen calls it - but the protocol layer still spells it ``model``, so a +directory named ``model/`` collides with real code a reader is looking at. Its public names are re-exported from :mod:`pyquadcortex`, which is where callers should import them from. The design is in ``docs/domain-model.md``. diff --git a/pyquadcortex/device/translate.py b/pyquadcortex/device/translate.py index 00465ce..0434b26 100644 --- a/pyquadcortex/device/translate.py +++ b/pyquadcortex/device/translate.py @@ -46,12 +46,13 @@ #: OFFSET from this, not the pitch itself - see :func:`tuner_reference_hz`. CONCERT_A_HZ = 440.0 -# What the wire may carry for each of the above, derived from them so the two -# accounts cannot disagree about how many there are. Scenes and footswitches -# share the eight-index range with slots. +# What the wire may carry for a row and a slot, derived from the screen values +# above so the two accounts cannot disagree about how many there are. Scene and +# footswitch indexes are NOT validated against these: they have their own enums +# at the protocol layer, and borrowing a range named for grid columns to check a +# footswitch is the confusion this module exists to end. _WIRE_ROWS = tuple(range(len(ROWS))) _WIRE_COLUMNS = tuple(range(len(SLOTS))) -_LETTERS = "ABCDEFGH" #: Only the three value types are re-exported from :mod:`pyquadcortex`; a caller #: holds those. The conversions are the seam's own business and are reached as @@ -70,24 +71,44 @@ ] -def _screen_number(value, what: str, allowed: tuple) -> int: - """Check one screen coordinate before it is converted. +def _a_whole_number(value, what: str) -> int: + """A plain ``int``, and nothing that is merely spelled like one. + + Three things are refused here, and each one is a silent wrong answer rather + than a crash if it gets through: - ``bool`` is refused explicitly because it is a subclass of ``int`` and - ``True == 1``: an unguarded check converts ``True`` to the first row or slot - and edits it, which is exactly the silent wrong answer this module exists to - prevent. A float is refused for the same reason in slower motion - ``1.0`` - means the caller is computing coordinates in a type that rounds. + * ``bool``, because it subclasses ``int`` and ``True == 1``, so an unguarded + check converts ``True`` to the first row or slot and edits it. + * a ``float``, because ``1.0`` means the caller is computing coordinates in + a type that rounds, and 218.9 becoming preset 218 recalls a real preset. + * any :class:`enum.Enum`, because the protocol layer's coordinate enums are + ``IntEnum``: :class:`~pyquadcortex.protocol.enums.Scene` ``B`` is 1, and + handing it to a row converter otherwise produces row 2 without complaint. + That is the footswitch-versus-column confusion in another costume. The two + wire-index converters that legitimately take one of those enums unwrap it + themselves, so only the RIGHT enum gets through. """ + if isinstance(value, enum.Enum): + raise TypeError( + f"{what} must be a plain int; {value!r} is a {type(value).__name__}, " + f"which numbers something else" + ) if isinstance(value, bool) or not isinstance(value, int): raise TypeError( f"{what} must be an int, not {type(value).__name__} ({value!r})" ) + return value + + +def _screen_number(value, what: str, allowed: tuple) -> int: + """One screen coordinate, checked against the values the unit shows.""" + _a_whole_number(value, what) if value not in allowed: - raise ValueError( - f"{what} must be {allowed[0]} to {allowed[-1]} - the unit shows " - f"{len(allowed)} of them; got {value}" - ) + contiguous = allowed[-1] - allowed[0] + 1 == len(allowed) + span = (f"{allowed[0]} to {allowed[-1]}" if contiguous + else f"one of {list(allowed)}") + raise ValueError(f"{what} must be {span} - the unit has " + f"{len(allowed)} of them; got {value}") return value @@ -179,9 +200,19 @@ def _letter(value, kind: type, what: str, trap: str): A number is refused rather than converted even though the wire is numeric. ``trap`` names what that number would more likely have been - the mistake the letter types exist to make impossible. + + The OTHER letter type is refused too. :class:`SceneLetter` and + :class:`FootswitchLetter` are both ``StrEnum`` over A to H, so each is a + plain string as far as any check goes, and scene E reaching a footswitch API + is the same wrong-thing-right-shape mistake as passing the number 4. """ if isinstance(value, kind): return value + if isinstance(value, enum.Enum): + raise TypeError( + f"{what} is a {kind.__name__}; {value!r} is a " + f"{type(value).__name__}, which labels something else" + ) if isinstance(value, bool) or isinstance(value, (int, float)): raise TypeError( f"{what} is a letter A to H, not the number {value!r} - the model " @@ -212,9 +243,20 @@ def footswitch_to_wire(footswitch) -> protocol.Footswitch: def footswitch_from_wire(index) -> FootswitchLetter: - """The wire's footswitch index (0-7) as the letter the unit labels it with.""" - return FootswitchLetter( - _LETTERS[_screen_number(index, "a wire footswitch index", _WIRE_COLUMNS)]) + """The wire's footswitch index (0-7) as the letter the unit labels it with. + + Takes a plain int or a :class:`~pyquadcortex.protocol.enums.Footswitch`. A + :class:`~pyquadcortex.protocol.enums.Scene` is refused even though it is an + ``IntEnum`` over the same eight numbers, because a scene index arriving here + means something upstream mixed up two things the unit keeps apart. + + The letter comes from the protocol enum's own member name rather than a + second copy of the alphabet, so the two layers cannot disagree about which + index is which switch. + """ + if not isinstance(index, protocol.Footswitch): + _a_whole_number(index, "a wire footswitch index") + return FootswitchLetter(protocol.Footswitch(index).name) def scene_to_wire(scene) -> protocol.Scene: @@ -229,9 +271,14 @@ def scene_to_wire(scene) -> protocol.Scene: def scene_from_wire(index) -> SceneLetter: - """The wire's scene index (0-7) as the letter the unit labels it with.""" - return SceneLetter( - _LETTERS[_screen_number(index, "a wire scene index", _WIRE_COLUMNS)]) + """The wire's scene index (0-7) as the letter the unit labels it with. + + Takes a plain int or a :class:`~pyquadcortex.protocol.enums.Scene`, and + refuses a footswitch index for the reason in :func:`footswitch_from_wire`. + """ + if not isinstance(index, protocol.Scene): + _a_whole_number(index, "a wire scene index") + return SceneLetter(protocol.Scene(index).name) # -- preset addresses: "28C" on screen, a linear position on the wire ------- @@ -254,6 +301,10 @@ def slot_to_position(name: str) -> int: ("01A") is accepted; :func:`position_to_slot` renders unpadded by default, because that is what the unit displays. """ + if not isinstance(name, str): + raise TypeError( + f"a slot name is text like '28C', not {type(name).__name__} " + f"({name!r})") return protocol.slot_to_position(name) @@ -266,8 +317,13 @@ def position_to_slot(position: int, pad: bool = False) -> str: Unpadded by default ("1A"), which is what the unit displays; ``pad=True`` gives "01A". + + A whole number only. The protocol helper takes ``int(position)``, so 218.9 + would quietly become 218 and ``True`` would become 1 - and unlike a bad row, + a bad position names a real preset that recalls without complaint. """ - return protocol.position_to_slot(position, pad=pad) + return protocol.position_to_slot( + _a_whole_number(position, "a wire preset position"), pad=pad) @dataclass(frozen=True) @@ -289,10 +345,7 @@ class PresetAddress: position: str def __post_init__(self): - if isinstance(self.bank, bool) or not isinstance(self.bank, int): - raise TypeError( - f"a bank is a number, not {type(self.bank).__name__} " - f"({self.bank!r})") + _a_whole_number(self.bank, "a bank") if not isinstance(self.position, str): raise TypeError( f"a position is a letter A to H, not " @@ -319,7 +372,10 @@ def parse(cls, text: str) -> "PresetAddress": raise TypeError( f"a preset address is text like '28C', not " f"{type(text).__name__} ({text!r})") - match = re.fullmatch(r"\s*(\d+)\s*([A-Za-z])\s*", text) + # ASCII digits only, and nothing between the bank and the letter. + # Python's `\d` spans every Unicode digit, so an unrestricted pattern + # read "٢٨C" as bank 28. + match = re.fullmatch(r"\s*([0-9]+)([A-Za-z])\s*", text) if not match: raise ValueError( f"a preset address is a bank number and a letter A to H, like " @@ -338,10 +394,17 @@ def to_wire(self) -> int: # -- display units ---------------------------------------------------------- # -# Each mapping below was measured on hardware and is documented at the protocol -# layer, on the helper that performs it. The model delegates rather than -# restating the arithmetic: two copies of a measured scale drift, and the drift -# is invisible because both copies still return a plausible number. +# Every mapping below was measured on hardware and is written up at the protocol +# layer. Two of the four - the level scales - have a protocol helper that +# performs the conversion, and this module calls it rather than restating the +# arithmetic: two copies of a measured scale drift, and both copies go on +# returning a plausible number. The other two have no helper to call. The tuner +# has only a documented rule, and hold timing has the protocol layer's constant +# tuple, which is the part worth sharing. Both are pinned in +# tests/test_translation.py against what the protocol WRITE method expects. +# +# What this module adds either way is the type guard, because the protocol +# helpers are arithmetic and will happily multiply a bool. def input_level_db(level: float) -> float: @@ -352,8 +415,14 @@ def input_level_db(level: float) -> float: An input port and a lane are both a 0..1 wire value and they are NOT the same scale - see :func:`lane_level_db`. + + A level outside 0..1 is converted rather than refused, unlike + :func:`hold_timing_ms`, which refuses an index outside its six. The + difference is that an out-of-span level still has a meaning under a linear + scale - it is off the end of the knob - while an index outside its list + names nothing at all. Neither has been seen from a unit. """ - return protocol.input_level_db(level) + return protocol.input_level_db(_a_number(level, "an input level")) def db_to_input_level(db: float) -> float: @@ -362,7 +431,7 @@ def db_to_input_level(db: float) -> float: Refuses anything outside -12..+60 dB rather than clamping, because a clamped write lands and reads back as a value the caller never asked for. """ - return protocol.db_to_input_level(db) + return protocol.db_to_input_level(_a_number(db, "an input gain in dB")) def lane_level_db(value: float) -> float: @@ -375,16 +444,21 @@ def lane_level_db(value: float) -> float: screen and -39.5 dB (wire 0.01) is the lowest numeric step. This converts the scale; it does not model the Off position. """ - return protocol.lane_level_db(value) + return protocol.lane_level_db(_a_number(value, "a lane level")) def db_to_lane_level(db: float) -> float: """Displayed dB as the wire value a lane, mixer or splitter LEVEL takes. - Refuses anything outside -40..+12 dB. For silence write the wire's 0.0 - directly - the Off position - rather than converting a dB value. + Refuses anything outside -40..+12 dB. + + **-40.0 dB is silence, not the bottom of the knob.** It converts to wire + 0.0, which is the Off detent: the lowest NUMERIC step on the unit is -39.5 + dB, and the screen reads "Off" below it. So asking for -40 dB mutes the + lane, and anything between -40.0 and -39.5 is a reading the screen has no + way to show. For silence, write the wire's 0.0 directly and mean it. """ - return protocol.db_to_lane_level(db) + return protocol.db_to_lane_level(_a_number(db, "a lane level in dB")) def tuner_reference_hz(offset: float) -> float: @@ -423,8 +497,8 @@ def hold_timing_ms(index: int) -> int: rounded to the nearest real setting. """ choices = protocol.QuadCortex.HOLD_TIMING_MS - if isinstance(index, bool) or not isinstance(index, int) \ - or not 0 <= index < len(choices): + _a_whole_number(index, "a wire hold-timing index") + if not 0 <= index < len(choices): raise ValueError( f"hold timing reads {index!r}, which is outside the " f"{len(choices)} values the unit offers - something wrote an " @@ -437,14 +511,16 @@ def ms_to_hold_timing(milliseconds: int) -> int: """Milliseconds as the ``hold_timing`` index the wire carries. Only the six values the unit offers convert. Anything else is refused rather - than rounded, because the device would store it and no gesture would match - it. + than rounded, and that is meant literally: 500.9 ms is not 500 ms, and + ``"500"`` is not a number. The protocol layer's setter takes + ``int(milliseconds)`` and so accepts both, which is the behaviour this + docstring would otherwise be describing wrongly. """ choices = protocol.QuadCortex.HOLD_TIMING_MS - try: - return choices.index(int(milliseconds)) - except (ValueError, TypeError): + _a_whole_number(milliseconds, "hold timing in ms") + if milliseconds not in choices: raise ValueError( f"hold timing must be one of {list(choices)} ms, " f"not {milliseconds!r}" - ) from None + ) + return choices.index(milliseconds) diff --git a/scripts/check_artifacts.py b/scripts/check_artifacts.py index c00f0b5..f2e25fd 100755 --- a/scripts/check_artifacts.py +++ b/scripts/check_artifacts.py @@ -26,6 +26,7 @@ "pyquadcortex/protocol/cli.py", "pyquadcortex/protocol/client.py", "pyquadcortex/device/device.py", + "pyquadcortex/device/translate.py", "pyquadcortex/_version.py", ) diff --git a/tests/test_namespace.py b/tests/test_namespace.py index 998bf4d..10ddbba 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -29,7 +29,7 @@ import pytest import pyquadcortex -from pyquadcortex import protocol +from pyquadcortex import device, protocol PRE_FLIP_INIT = (pathlib.Path(__file__).resolve().parent / "fixtures" / "surface" / "pre_flip_init.py.txt") @@ -147,7 +147,11 @@ def test_the_protocol_sources_were_actually_found(): assert len(PROTOCOL_SOURCES) > 5 -MODEL_PACKAGE = "pyquadcortex.device" +#: Read from the package rather than typed here. A hardcoded string survives the +#: package being renamed or moved, and the check below then looks for imports of +#: a package that no longer exists - passing for every source file, for ever, +#: with its own guard test still green. +MODEL_PACKAGE = device.__name__ def _is_the_model(dotted: str) -> bool: diff --git a/tests/test_translation.py b/tests/test_translation.py index 65fbdd8..90f1a38 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -13,11 +13,14 @@ past the boundary for a protocol helper that does. """ import ast +import importlib import pathlib +import pkgutil import pytest -from pyquadcortex import device, protocol +import pyquadcortex +from pyquadcortex import protocol from pyquadcortex.device import translate @@ -80,6 +83,48 @@ def test_a_bool_is_not_a_slot(): translate.slot_to_wire(True) +# -- one index is never another index ---------------------------------------- +# +# The protocol layer's coordinate enums are IntEnums, so each one is an int and +# passes any `isinstance(x, int)` check. Scene B is 1 and row 2 is wire 1, which +# means a Scene handed to a row converter produces a real row rather than a +# complaint. That is the same class of mistake as the footswitch-versus-column +# confusion this module exists to prevent, so it is refused the same way. + + +@pytest.mark.parametrize("converter,wrong", [ + ("row_to_wire", protocol.Scene.B), + ("slot_to_wire", protocol.Footswitch.C), + ("row_from_wire", protocol.Footswitch.A), + ("slot_from_wire", protocol.Scene.H), +]) +def test_a_coordinate_from_somewhere_else_is_refused(converter, wrong): + with pytest.raises(TypeError): + getattr(translate, converter)(wrong) + + +def test_a_scene_index_is_not_a_footswitch_index(): + with pytest.raises(TypeError): + translate.footswitch_from_wire(protocol.Scene.E) + + +def test_a_footswitch_index_is_not_a_scene_index(): + with pytest.raises(TypeError): + translate.scene_from_wire(protocol.Footswitch.E) + + +def test_a_scene_letter_is_not_a_footswitch(): + """Both are letters A to H and both are strings, so nothing but the type + itself keeps `scenes["E"]`'s key out of a footswitch API.""" + with pytest.raises(TypeError): + translate.footswitch_to_wire(translate.SceneLetter.E) + + +def test_a_footswitch_letter_is_not_a_scene(): + with pytest.raises(TypeError): + translate.scene_to_wire(translate.FootswitchLetter.B) + + # -- footswitches: letters in the model, indexes on the wire ----------------- # # The protocol layer's `Footswitch` enum is the reference. It is also the reason @@ -239,6 +284,31 @@ def test_a_wire_position_outside_a_setlist_is_refused(position): translate.PresetAddress.from_wire(position) +@pytest.mark.parametrize("position", [218.9, True, "218", None]) +def test_a_wire_position_that_is_not_a_whole_number_is_refused(position): + """The protocol helper takes `int(position)`, so 218.9 quietly becomes 218 + and True becomes 1. Every other coordinate path here refuses a float and a + bool; this is the path where the wrong answer recalls a real preset.""" + with pytest.raises(TypeError): + translate.position_to_slot(position) + with pytest.raises(TypeError): + translate.PresetAddress.from_wire(position) + + +@pytest.mark.parametrize("name", [218, None, ["28C"]]) +def test_a_slot_name_that_is_not_text_is_refused(name): + with pytest.raises(TypeError): + translate.slot_to_position(name) + + +@pytest.mark.parametrize("malformed", ["28 C", "٢٨C", "2 8C"]) +def test_an_address_with_stray_characters_is_refused(malformed): + """Internal whitespace and non-ASCII digits both parsed before: Python's + `\\d` spans every Unicode digit, so "٢٨C" read as bank 28.""" + with pytest.raises(ValueError): + translate.PresetAddress.parse(malformed) + + def test_the_address_conversion_says_the_naming_depends_on_the_mode(): """An address is only unambiguous alongside the mode it was read in: linear position 5 reads "1F" normally and "2B" under a PRESET-containing HYBRID. @@ -251,10 +321,26 @@ def test_the_address_conversion_says_the_naming_depends_on_the_mode(): # -- display units ----------------------------------------------------------- # -# Each of these has a protocol-layer helper that already carries the measurement -# and its evidence. The model must not restate the arithmetic, so these tests -# check the boundary against that helper rather than against a number retyped -# here - a retyped constant agrees with itself forever. +# **What the equality assertions below do and do not prove.** Two of these four +# mappings delegate to a protocol-layer helper, so `translate.input_level_db(v) +# == protocol.input_level_db(v)` cannot fail today - it is one function calling +# the other. It is not a check on the arithmetic, and it is not a substitute for +# one. What it pins is that the boundary goes on DELEGATING: the day someone +# copies the formula in here to add a clamp or a rounding rule, this is what +# fails. +# +# The measured numbers themselves are pinned where the measurement lives, in +# tests/test_client.py: `test_input_level_db_matches_the_four_measured_points` +# and `test_lane_level_db_matches_the_three_measured_points` check the screen +# readings taken against simultaneous wire reads. Nothing here restates them, +# because a second copy of a measured constant drifts and both copies keep +# returning a plausible number. +# +# The other two mappings - the tuner and hold timing - have no protocol helper +# to call, only a documented rule and a shared constant, so they are pinned +# below against the protocol WRITE path through a fake transport. That is a real +# check: it fails if the model's idea of 442 Hz stops matching what the method +# that sends it expects. class Recorder: @@ -317,6 +403,17 @@ def test_a_lane_level_the_unit_has_no_setting_for_is_refused(db): translate.db_to_lane_level(db) +@pytest.mark.parametrize("converter", ["input_level_db", "db_to_input_level", + "lane_level_db", "db_to_lane_level"]) +@pytest.mark.parametrize("wrong", [True, "0.5", None]) +def test_a_level_that_is_not_a_number_is_refused(converter, wrong): + """`True` is an int, so an unguarded lane level read it as full scale and + returned +12 dB. A string reached the protocol layer and came back as a + `TypeError` about multiplying a sequence.""" + with pytest.raises(TypeError): + getattr(translate, converter)(wrong) + + def test_the_two_level_scales_are_not_interchangeable(): """Both are a 0..1 wire value and they mean different dB. Reading a lane level with the input mapping is a wrong answer that looks plausible.""" @@ -379,39 +476,96 @@ def test_a_hold_timing_index_the_unit_cannot_have_written_is_refused(index): translate.hold_timing_ms(index) +@pytest.mark.parametrize("ms", [500.9, 800.0, "500", True]) +def test_a_hold_timing_that_is_not_a_whole_number_of_ms_is_refused(ms): + """`int(milliseconds)` rounded 500.9 down to a valid setting and read "500" + as a number, which is not what "refused rather than rounded" means.""" + with pytest.raises(TypeError): + translate.ms_to_hold_timing(ms) + + +@pytest.mark.parametrize("index", ["3", 3.0, True]) +def test_a_hold_timing_index_that_is_not_a_whole_number_is_refused(index): + """A wrong TYPE raises TypeError here, as it does everywhere else in this + module; a wrong VALUE raises ValueError. This one used to raise ValueError + for both.""" + with pytest.raises(TypeError): + translate.hold_timing_ms(index) + + # -- the boundary is the ONLY place ----------------------------------------- # # Everything above proves the conversions are right. These two prove they are # the only ones, which is the half a reviewer cannot check by reading a diff: # a stray `- 1` in a future module is one character, looks deliberate, and # produces an edit that lands on a real row and reads back perfectly. +# +# The scan covers EVERY source file in the package that is not the protocol +# layer, not just the model directory. Scoping it to `device/` would leave the +# rule satisfiable by putting the arithmetic in `pyquadcortex/coords.py`, one +# directory up - which is where somebody would put it after reading a failure +# message that named a directory. BOUNDARY = pathlib.Path(translate.__file__).resolve() -MODEL_SOURCES = sorted( - p for p in pathlib.Path(device.__file__).resolve().parent.rglob("*.py")) +PACKAGE_ROOT = pathlib.Path(pyquadcortex.__file__).resolve().parent +PROTOCOL_ROOT = pathlib.Path(protocol.__file__).resolve().parent +MODEL_SOURCES = sorted(p for p in PACKAGE_ROOT.rglob("*.py") + if not p.is_relative_to(PROTOCOL_ROOT)) OTHER_MODEL_SOURCES = [p for p in MODEL_SOURCES if p != BOUNDARY] -def _off_by_one_arithmetic(tree: ast.AST) -> list[int]: - """Line numbers of every `x + 1` / `x - 1` / `x += 1` in `tree`. +def _index_arithmetic(tree: ast.AST) -> list[str]: + """Every spelling of index arithmetic in `tree`, as "line N: what". - Blunt on purpose. A rule that only fired on operands spelled `row` or `slot` - would miss `n - 1`, and `n` is what the arithmetic is called by the time - someone has extracted a helper for it. + Blunt on purpose, and wider than `+ 1`. A rule that only fired on operands + spelled `row` or `slot` would miss `n - 1`, and `n` is what the arithmetic + is called by the time somebody has extracted a helper for it. The calls + listed here are how a person actually writes the conversions this module + owns: `ord`/`chr` or a letter table for a scene or footswitch letter, + `divmod` for a preset address. """ - def is_one(node) -> bool: + def one(node) -> bool: + """A literal one, however spelled: 1, 1.0, or True - which equals 1.""" return (isinstance(node, ast.Constant) - and type(node.value) is int and node.value == 1) + and isinstance(node.value, (int, float)) + and not isinstance(node.value, str) + and node.value == 1) + + def offset(node) -> bool: + return one(node) or (isinstance(node, ast.UnaryOp) + and isinstance(node.op, ast.USub) + and one(node.operand)) + + def letter_table(node) -> bool: + """A string literal that is a run of the letters the unit labels with.""" + return (isinstance(node, ast.Constant) and isinstance(node.value, str) + and len(node.value) >= 3 and "ABCDEFGH".startswith(node.value)) - hits = [] + def called(node): + if isinstance(node.func, ast.Name): + return node.func.id + return node.func.attr if isinstance(node.func, ast.Attribute) else None + + found = [] for node in ast.walk(tree): + where = f"line {getattr(node, 'lineno', 0)}" if isinstance(node, ast.BinOp) and isinstance(node.op, (ast.Add, ast.Sub)): - if is_one(node.left) or is_one(node.right): - hits.append(node.lineno) + if offset(node.left) or offset(node.right): + found.append(f"{where}: adds or subtracts one") elif isinstance(node, ast.AugAssign) \ - and isinstance(node.op, (ast.Add, ast.Sub)) and is_one(node.value): - hits.append(node.lineno) - return sorted(set(hits)) + and isinstance(node.op, (ast.Add, ast.Sub)) and offset(node.value): + found.append(f"{where}: adds or subtracts one") + elif isinstance(node, ast.Call): + name = called(node) + if name in ("ord", "chr"): + found.append(f"{where}: {name}() - letter arithmetic") + elif name == "divmod": + found.append(f"{where}: divmod() - splitting a linear position") + elif name == "enumerate" and len(node.args) > 1: + found.append(f"{where}: enumerate() with a start offset") + elif letter_table(node): + found.append(f"{where}: a letter table, {node.value!r}") + return sorted(set(found)) #: Protocol-layer names that carry a coordinate or a raw scale. Reaching for one @@ -429,15 +583,40 @@ def _protocol_conversions_used(tree: ast.AST) -> list[str]: Only through the protocol layer: `translate.slot_to_position(...)` is the boundary doing its job and must not be reported, so a bare attribute name is not enough to accuse on. + + Which local names MEAN the protocol layer is worked out first, because the + spellings that reach it are not all `protocol.`: a module can alias the + package, import a submodule of it, or reach through two attributes at once + (`protocol.QuadCortex.HOLD_TIMING_MS`). Each of those was a hole in the + first version of this check, and each is one a person would write without + any idea they were evading anything. """ - def is_the_protocol_layer(node) -> bool: - return ((isinstance(node, ast.Name) and node.id == "protocol") - or (isinstance(node, ast.Attribute) and node.attr == "protocol")) + # `protocol` is seeded because the house style is + # `from pyquadcortex import protocol`, and a module using that name without + # the import in the same snippet is the ordinary case in a sample below. + aliases = {"pyquadcortex", "protocol"} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name.startswith("pyquadcortex"): + aliases.add(alias.asname or alias.name.split(".")[0]) + elif isinstance(node, ast.ImportFrom): + module = node.module or "" + if module == "pyquadcortex" or module.startswith("pyquadcortex.protocol"): + for alias in node.names: + if module != "pyquadcortex" or alias.name == "protocol": + aliases.add(alias.asname or alias.name) + + def root_of(node): + """The leftmost name in an attribute chain, or None.""" + while isinstance(node, ast.Attribute): + node = node.value + return node.id if isinstance(node, ast.Name) else None found = [] for node in ast.walk(tree): if isinstance(node, ast.Attribute) and node.attr in PROTOCOL_CONVERSIONS \ - and is_the_protocol_layer(node.value): + and root_of(node.value) in aliases: found.append(node.attr) elif isinstance(node, ast.ImportFrom) \ and (node.module or "").startswith("pyquadcortex.protocol"): @@ -445,28 +624,41 @@ def is_the_protocol_layer(node) -> bool: return sorted(set(found)) -def test_the_source_walk_found_the_model_package(): - """Guards both checks below: an empty list passes them vacuously.""" +def test_the_scan_covers_every_module_that_is_not_the_protocol_layer(): + """Guards both checks below. An empty list passes them vacuously, and a + scan that skipped a module enforces nothing in it. + + Checked against the import machinery's own walk, so a module added tomorrow + is covered the day it is created rather than the day somebody remembers to + add it here. + """ assert BOUNDARY in MODEL_SOURCES assert len(OTHER_MODEL_SOURCES) >= 2 + walked = { + pathlib.Path(importlib.import_module(info.name).__file__).resolve() + for info in pkgutil.walk_packages(pyquadcortex.__path__, "pyquadcortex.") + if not info.name.startswith("pyquadcortex.protocol") + } + missed = sorted(str(p) for p in walked - set(MODEL_SOURCES)) + assert not missed, f"the scan does not cover {missed}" def test_the_boundary_itself_does_the_arithmetic(): """The exclusion below has to be load-bearing. If the boundary stopped converting, the check would pass because nothing anywhere converts - which is the one failure a "no arithmetic elsewhere" test cannot see.""" - hits = _off_by_one_arithmetic(ast.parse(BOUNDARY.read_text())) - assert hits, f"{BOUNDARY.name} does no +1/-1 arithmetic at all" + found = _index_arithmetic(ast.parse(BOUNDARY.read_text())) + assert found, f"{BOUNDARY.name} does no index arithmetic at all" @pytest.mark.parametrize("source", OTHER_MODEL_SOURCES, ids=lambda p: p.name) def test_no_index_arithmetic_outside_the_boundary(source): - hits = _off_by_one_arithmetic(ast.parse(source.read_text())) - assert not hits, ( - f"{source.name} does +1/-1 arithmetic at line(s) {hits}. If that is a " - f"screen coordinate becoming a wire index, it belongs in " - f"{BOUNDARY.name} with a test - an off-by-one here edits a real row, " - f"just not the one intended, and reads back perfectly" + found = _index_arithmetic(ast.parse(source.read_text())) + assert not found, ( + f"{source.name} does index arithmetic - {found}. If that is a screen " + f"value becoming a wire value it belongs in {BOUNDARY.name} with a " + f"test, wherever in the package the file sits - an off-by-one here " + f"edits a real row, just not the one intended, and reads back perfectly" ) @@ -487,9 +679,21 @@ def test_only_the_boundary_reaches_for_a_protocol_conversion(source): ("one on the left", "column = 1 - offset", True), ("hidden in a call", "qc.set_param(row=row - 1, column=slot - 1)", True), ("a comprehension", "[s - 1 for s in slots]", True), + ("a negated one", "wire_row = row + -1", True), + ("a float one", "wire_row = row - 1.0", True), + ("a bool one", "wire_row = row - True", True), + ("a letter from an index", "letter = chr(ord('A') + index)", True), + ("an index from a letter", "index = ord(letter) - ord('A')", True), + ("a letter table lookup", "letter = 'ABCDEFGH'[index]", True), + ("a letter table search", "index = 'ABCDEFGH'.index(letter)", True), + ("a letter table under any name", "LETTERS = 'ABCDEFGH'", True), + ("splitting a linear position", "bank, letter = divmod(position, 8)", True), + ("a one-based enumerate", "[(n, r) for n, r in enumerate(rows, 1)]", True), ("the boundary doing it", "wire_row = translate.row_to_wire(row)", False), ("arithmetic that is not off-by-one", "total = a + 2", False), - ("a true that is not a one", "flag = other + True", False), + ("a plain enumerate", "[(i, r) for i, r in enumerate(rows)]", False), + ("a string that is not a letter table", "name = 'ABY Splitter'", False), + ("an ordinary attribute", "name = block.device.name", False), ] @@ -497,8 +701,16 @@ def test_only_the_boundary_reaches_for_a_protocol_conversion(source): ids=[s[0] for s in ARITHMETIC_SAMPLES]) def test_the_arithmetic_check_sees_what_it_claims_to(label, source, detected): """A check with blind spots enforces the rule only for the spellings - somebody happened to think of.""" - assert bool(_off_by_one_arithmetic(ast.parse(source))) is detected + somebody happened to think of, while reading as though it enforced all of + them. + + The samples that earn their place are the ones the first version of this + check missed: letter arithmetic, a letter table, `divmod` on a preset + position, and the three ways of writing one that are not the token `1`. + None of those is exotic. They are how the conversions this module owns get + written by somebody writing them somewhere else. + """ + assert bool(_index_arithmetic(ast.parse(source))) is detected CONVERSION_SAMPLES = [ @@ -506,9 +718,19 @@ def test_the_arithmetic_check_sees_what_it_claims_to(label, source, detected): ("through the package", "x = pyquadcortex.protocol.lane_level_db(v)", True), ("an import", "from pyquadcortex.protocol import slot_to_position", True), ("a module import", "from pyquadcortex.protocol.client import UNITY_LEVEL", True), + ("two attributes deep", "x = protocol.QuadCortex.HOLD_TIMING_MS", True), + ("an aliased package", + "from pyquadcortex import protocol as p\nx = p.Footswitch.A", True), + ("an imported submodule", + "from pyquadcortex.protocol import client\nx = client.lane_level_db(v)", True), + ("an aliased submodule", + "from pyquadcortex.protocol import client as c\nx = c.UNITY_LEVEL", True), + ("a plain import", + "import pyquadcortex.protocol\nx = pyquadcortex.protocol.Scene.A", True), ("the boundary's own name", "x = translate.slot_to_position(name)", False), ("a protocol name that is not a conversion", "x = protocol.field_present(reply, 'serial')", False), + ("something else entirely, named the same", "x = self.grid.Scene.A", False), ] @@ -516,6 +738,8 @@ def test_the_arithmetic_check_sees_what_it_claims_to(label, source, detected): ids=[s[0] for s in CONVERSION_SAMPLES]) def test_the_reach_past_the_boundary_check_sees_what_it_claims_to(label, source, detected): + """Same standard as the arithmetic samples: the entries worth having are + the spellings the first version of this check could not see.""" assert bool(_protocol_conversions_used(ast.parse(source))) is detected From 68bc34edd2af83c721d86f81bf03249f7f25821d Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Wed, 12 Aug 2026 18:31:12 -0500 Subject: [PATCH 4/8] review: say that the tuner reference is not rounded, and prove it The wire value the unit sent for a screen reading of 442 Hz was 1.99999809, so the conversion returns 441.99999809 rather than 442. That is deliberate - rounding it would mean knowing how many digits the unit's FREQ field shows, and nobody has read that off the unit - but the docstring promised a display value and left the caller to discover the difference. --- pyquadcortex/device/translate.py | 5 +++++ tests/test_translation.py | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/pyquadcortex/device/translate.py b/pyquadcortex/device/translate.py index 0434b26..f9d0aa9 100644 --- a/pyquadcortex/device/translate.py +++ b/pyquadcortex/device/translate.py @@ -475,6 +475,11 @@ def tuner_reference_hz(offset: float) -> float: (see :meth:`pyquadcortex.protocol.QuadCortex.set_tuner_reference`). No range is enforced for the same reason: the unit's FREQ limits have not been read, and a limit invented here would refuse a setting the unit allows. + + Nothing is rounded either, so the wire's 1.99999809 reads back as + 441.99999809 rather than the 442 on the screen. How many digits the unit's + FREQ field shows has not been read off it, and rounding to a precision + nobody has checked would be the same guess in the other direction. """ return CONCERT_A_HZ + _a_number(offset, "a tuner reference offset") diff --git a/tests/test_translation.py b/tests/test_translation.py index 90f1a38..14d4a27 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -444,6 +444,14 @@ def test_the_tuner_reference_refuses_something_that_is_not_a_pitch(): translate.hz_to_tuner_reference("442") +def test_the_tuner_reference_is_not_rounded_to_a_precision_nobody_has_read(): + """The wire value the unit sent for a screen reading of 442 was + 1.99999809, so this returns 441.99999809. Rounding it to 442 would mean + knowing how many digits the FREQ field shows, and nobody has read that off + the unit.""" + assert translate.tuner_reference_hz(1.99999809) == pytest.approx(441.99999809) + + # -- hold timing: milliseconds on screen, one of six indexes on the wire ------ From 50feedb1159805f4f1880b75baf3ed939f7f9acf Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Wed, 12 Aug 2026 18:32:03 -0500 Subject: [PATCH 5/8] docs: the structural checks read the whole package, not just the model directory --- tests/test_translation.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_translation.py b/tests/test_translation.py index 14d4a27..2048023 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -9,8 +9,10 @@ thing that can. Two of the checks below are structural rather than behavioural: they read the -model package's source and prove no other module does the arithmetic or reaches -past the boundary for a protocol helper that does. +source of every file in the package that is not the protocol layer, and prove no +other module does the arithmetic or reaches past the boundary for a protocol +helper that does. Not just the model directory - a rule scoped to a directory is +satisfiable by moving the code to a different one. """ import ast import importlib From 766123247993beb1ca7d915ceab2f7477ebcff56 Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Thu, 13 Aug 2026 13:18:55 -0500 Subject: [PATCH 6/8] feat: the tempo's bpm converts at the boundary too PR #22 added `tempo_bpm()` / `bpm_to_tempo()` next to the level helpers in protocol/client.py. They belong at the model boundary the same way the level scales do, so `device/translate.py` wraps them. They cannot MOVE. `set_tempo_param(real=)` calls `bpm_to_tempo` from inside protocol/client.py, so relocating the helper would make the protocol layer import the model. Checked by patching that call site to import from `pyquadcortex.device.translate` and watching `test_the_protocol_layer_never_imports_the_model` name it. So the pair delegates, exactly as `input_level_db` and `lane_level_db` do: one copy of the measured span, with the measurement attributed to the protocol layer rather than restated here, plus the type guard this seam adds because the protocol helpers are arithmetic and will happily multiply a bool. `tempo_bpm` and `bpm_to_tempo` also join the protocol-conversion allowlist in tests/test_translation.py, which is the half that does the work: without it, a model module reaching for `protocol.tempo_bpm` passes the check. Verified by dropping a file into the package that does both a `row - 1` and a `protocol.tempo_bpm(...)`, watching both structural checks name it, and removing it again. New in the tests: a guard that every name on that allowlist resolves in the protocol layer. A typo there reads like a rule and protects nothing. --- changelog.md | 7 ++- docs/STEERING.md | 17 ++++-- docs/architecture.md | 18 ++++-- docs/domain-model.md | 3 +- pyquadcortex/device/translate.py | 53 ++++++++++++++---- tests/test_translation.py | 96 ++++++++++++++++++++++++++++---- 6 files changed, 158 insertions(+), 36 deletions(-) diff --git a/changelog.md b/changelog.md index c3f62bb..33c90e6 100644 --- a/changelog.md +++ b/changelog.md @@ -133,9 +133,10 @@ model keeping itself current without asking twice. ### Groundwork: the model will talk in the numbers on your screen -Rows will be 1 to 4, slots 1 to 8, scenes and footswitches letters, and levels the -dB the unit displays. The wire counts from zero and stores raw scales, and the -model now converts in exactly one place so nothing else has to remember to. +Rows will be 1 to 4, slots 1 to 8, scenes and footswitches letters, levels the dB +the unit displays, and the tempo the bpm it displays. The wire counts from zero +and stores raw scales, and the model now converts in exactly one place so nothing +else has to remember to. **Nothing that reads a row or a level exists yet** - the preset and grid surfaces are still being built - so what you can use today is the three value types this diff --git a/docs/STEERING.md b/docs/STEERING.md index 864c991..0f57575 100644 --- a/docs/STEERING.md +++ b/docs/STEERING.md @@ -128,12 +128,12 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) **What changed:** - `pyquadcortex/device/translate.py`: the one module where a screen value becomes a wire value and back - rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, - and four display-unit mappings (input gain dB, lane and mixer dB, tuner reference Hz, - hold timing ms). The two level scales call the protocol helper that carries the - measurement; the other two have no helper to call, so they are pinned against what the - protocol write method expects, and the tuner's docstring says how thin its evidence is - - one observed pair. `PresetAddress`, `FootswitchLetter` and `SceneLetter` are its public - value types, re-exported from `pyquadcortex` + and five display-unit mappings (input gain dB, lane and mixer dB, tempo bpm, tuner + reference Hz, hold timing ms). The two level scales and the tempo call the protocol + helper that carries the measurement; the other two have no helper to call, so they are + pinned against what the protocol write method expects, and the tuner's docstring says + how thin its evidence is - one observed pair. `PresetAddress`, `FootswitchLetter` and + `SceneLetter` are its public value types, re-exported from `pyquadcortex` - Section 5 gained the pattern row; section 4's owned-paths line and CLAUDE.md name the new rule. `architecture.md` carries the module in its layer map and a section on it; `domain-model.md` marks principle 5, `PresetAddress` and `FootswitchLetter` as built @@ -186,6 +186,11 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) - The scan is scoped to the whole package rather than to `pyquadcortex/device/`, because a rule scoped to a directory is satisfiable by moving the code one directory up - which is precisely what a failure message naming a directory invites +- A protocol conversion can be delegated to and still not be movable. `bpm_to_tempo` + (PR #22) is called by `QuadCortex.set_tempo_param` from inside the protocol layer, so + the helper stays there and the boundary wraps it, the same way it wraps the level + scales. Adding the name to the boundary's allowlist is the half that matters: without + it, a model module reaching for `protocol.tempo_bpm` passes the check ### 2026-08-12 - TEMPO MODE closes, and ADR-0010 diff --git a/docs/architecture.md b/docs/architecture.md index f241ff9..df636af 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -207,7 +207,7 @@ built story by story. Nothing is stubbed out to look finished. ### device/translate.py The model speaks what the touchscreen shows - rows 1 to 4, slots 1 to 8, scenes -and footswitches as letters, dB, Hz, ms - and the wire speaks zero-based indexes +and footswitches as letters, dB, Hz, bpm, ms - and the wire speaks zero-based indexes and raw scales. Every conversion between the two lives here and nowhere else in `pyquadcortex/device/` (design principle 5 in [domain-model.md](domain-model.md)). @@ -221,11 +221,17 @@ does none of it by reading the source, rather than by trusting anyone to remember. Where a protocol-layer helper already performs the conversion - input gain dB, -lane and mixer dB, the slot-name/position pair - this module calls it instead of -restating the arithmetic. Two copies of a measured scale drift apart, and both -copies go on returning a plausible number. The tuner and hold-timing mappings -have no helper to call, only a documented rule and a shared constant, so their -tests pin them against what the protocol write method expects. +lane and mixer dB, tempo bpm, the slot-name/position pair - this module calls it +instead of restating the arithmetic. Two copies of a measured scale drift apart, +and both copies go on returning a plausible number. The tuner and hold-timing +mappings have no helper to call, only a documented rule and a shared constant, so +their tests pin them against what the protocol write method expects. + +Delegating is not always a choice between two homes. `bpm_to_tempo` is called by +`QuadCortex.set_tempo_param` from inside the protocol layer, so it has to stay +there: moving it to the boundary would make the protocol layer import the model, +which `tests/test_namespace.py` refuses. The wrapper here is what gives the model +one way in without a second copy of the span. Public value types: `PresetAddress`, `FootswitchLetter`, `SceneLetter`, re-exported from `pyquadcortex`. The conversion functions are the module's own diff --git a/docs/domain-model.md b/docs/domain-model.md index e21e6c8..8f6db03 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -1324,7 +1324,8 @@ the n/a rows below where they intersect the API at all. diff, rather than looking for a field you expect. Still an M3 surface, so still not a behaviour change at M1. - **2026-08-12** - Design principle 5 is built (M1 story #10): `pyquadcortex/device/translate.py` - owns every conversion between a screen value and a wire value, with `PresetAddress`, + owns every conversion between a screen value and a wire value - including the tempo's + bpm, which arrived from the TEMPO MODE work above - with `PresetAddress`, `FootswitchLetter` and `SceneLetter` landing as part of it. The model package directory is `device/` rather than `model/`, because the protocol layer spells an amp or pedal block `model` in code (`models.py`, `Model`, `ModelCatalog`) and will keep doing so, diff --git a/pyquadcortex/device/translate.py b/pyquadcortex/device/translate.py index f9d0aa9..73a986b 100644 --- a/pyquadcortex/device/translate.py +++ b/pyquadcortex/device/translate.py @@ -1,10 +1,10 @@ """The one place a screen value becomes a wire value, and back. The model speaks what the touchscreen shows: rows 1 to 4, slots 1 to 8, scenes -and footswitches as letters, levels in dB, the tuner in Hz. The wire speaks -zero-based indexes and raw scales. Every conversion between the two lives here, -and nowhere else in :mod:`pyquadcortex.device` - design principle 5 in -``docs/domain-model.md``. +and footswitches as letters, levels in dB, the tuner in Hz, the tempo in bpm. The +wire speaks zero-based indexes and raw scales. Every conversion between the two +lives here, and nowhere else in :mod:`pyquadcortex.device` - design principle 5 +in ``docs/domain-model.md``. **Why one module rather than a convention.** The protocol layer's own header says it plainly: rows are zero-based, "getting this wrong is quiet rather than loud - @@ -66,6 +66,7 @@ "slot_to_position", "position_to_slot", "input_level_db", "db_to_input_level", "lane_level_db", "db_to_lane_level", + "tempo_bpm", "bpm_to_tempo", "tuner_reference_hz", "hz_to_tuner_reference", "hold_timing_ms", "ms_to_hold_timing", ] @@ -395,12 +396,12 @@ def to_wire(self) -> int: # -- display units ---------------------------------------------------------- # # Every mapping below was measured on hardware and is written up at the protocol -# layer. Two of the four - the level scales - have a protocol helper that -# performs the conversion, and this module calls it rather than restating the -# arithmetic: two copies of a measured scale drift, and both copies go on -# returning a plausible number. The other two have no helper to call. The tuner -# has only a documented rule, and hold timing has the protocol layer's constant -# tuple, which is the part worth sharing. Both are pinned in +# layer. Three of the five - the two level scales and the tempo - have a protocol +# helper that performs the conversion, and this module calls it rather than +# restating the arithmetic: two copies of a measured scale drift, and both copies +# go on returning a plausible number. The other two have no helper to call. The +# tuner has only a documented rule, and hold timing has the protocol layer's +# constant tuple, which is the part worth sharing. Both are pinned in # tests/test_translation.py against what the protocol WRITE method expects. # # What this module adds either way is the type guard, because the protocol @@ -461,6 +462,38 @@ def db_to_lane_level(db: float) -> float: return protocol.db_to_lane_level(_a_number(db, "a lane level in dB")) +def tempo_bpm(value: float) -> float: + """A ``TEMPO`` wire value (0..1) as the bpm the unit displays. + + Tempo spans 40 to 240 bpm. Delegates to + :func:`pyquadcortex.protocol.tempo_bpm`, which carries the measurement and its + limits: three screen-vs-wire points, with the two endpoints coming from the + fit rather than from a driven extreme. + + A wire value outside 0..1 is REFUSED, where :func:`input_level_db` and + :func:`lane_level_db` convert one. That difference is the protocol helpers' + rather than a rule added at this seam - the tempo one refuses because the bpm + a caller would read back does not exist on the unit - and this wrapper + neither widens it nor narrows it. + """ + return protocol.tempo_bpm(_a_number(value, "a tempo wire value")) + + +def bpm_to_tempo(bpm: float) -> float: + """A displayed tempo in bpm as the wire value ``TEMPO`` takes. + + Refuses anything outside 40..240 bpm rather than clamping, because a clamped + write lands and reads back as a tempo the caller never asked for. + + This pair is a wrapper and not a home. The protocol layer calls + :func:`pyquadcortex.protocol.bpm_to_tempo` itself, inside + :meth:`~pyquadcortex.protocol.QuadCortex.set_tempo_param`, so the helper has + to stay down there: moving it up here would make the protocol layer import + the model, which is the one direction the layering forbids. + """ + return protocol.bpm_to_tempo(_a_number(bpm, "a tempo in bpm")) + + def tuner_reference_hz(offset: float) -> float: """The tuner's wire ``frequency`` as the absolute reference pitch on screen. diff --git a/tests/test_translation.py b/tests/test_translation.py index 2048023..879da4f 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -323,7 +323,7 @@ def test_the_address_conversion_says_the_naming_depends_on_the_mode(): # -- display units ----------------------------------------------------------- # -# **What the equality assertions below do and do not prove.** Two of these four +# **What the equality assertions below do and do not prove.** Three of these five # mappings delegate to a protocol-layer helper, so `translate.input_level_db(v) # == protocol.input_level_db(v)` cannot fail today - it is one function calling # the other. It is not a check on the arithmetic, and it is not a substitute for @@ -332,11 +332,12 @@ def test_the_address_conversion_says_the_naming_depends_on_the_mode(): # fails. # # The measured numbers themselves are pinned where the measurement lives, in -# tests/test_client.py: `test_input_level_db_matches_the_four_measured_points` -# and `test_lane_level_db_matches_the_three_measured_points` check the screen -# readings taken against simultaneous wire reads. Nothing here restates them, -# because a second copy of a measured constant drifts and both copies keep -# returning a plausible number. +# tests/test_client.py: `test_input_level_db_matches_the_four_measured_points`, +# `test_lane_level_db_matches_the_three_measured_points` and +# `test_tempo_bpm_matches_every_measured_point` check the screen readings taken +# against simultaneous wire reads. Nothing here restates them, because a second +# copy of a measured constant drifts and both copies keep returning a plausible +# number. # # The other two mappings - the tuner and hold timing - have no protocol helper # to call, only a documented rule and a shared constant, so they are pinned @@ -406,12 +407,16 @@ def test_a_lane_level_the_unit_has_no_setting_for_is_refused(db): @pytest.mark.parametrize("converter", ["input_level_db", "db_to_input_level", - "lane_level_db", "db_to_lane_level"]) + "lane_level_db", "db_to_lane_level", + "tempo_bpm", "bpm_to_tempo"]) @pytest.mark.parametrize("wrong", [True, "0.5", None]) -def test_a_level_that_is_not_a_number_is_refused(converter, wrong): +def test_a_display_value_that_is_not_a_number_is_refused(converter, wrong): """`True` is an int, so an unguarded lane level read it as full scale and - returned +12 dB. A string reached the protocol layer and came back as a - `TypeError` about multiplying a sequence.""" + returned +12 dB. The tempo has the same shape - `protocol.tempo_bpm(True)` + is 240.0, the top of the span - and `bpm_to_tempo(True)` would be a bpm of + 1, refused for being off the bottom rather than for being a bool. A string + reached the protocol layer and came back as a `TypeError` about multiplying + a sequence.""" with pytest.raises(TypeError): getattr(translate, converter)(wrong) @@ -422,6 +427,57 @@ def test_the_two_level_scales_are_not_interchangeable(): assert translate.input_level_db(0.5) != translate.lane_level_db(0.5) +# -- the tempo: bpm on screen, a 0..1 value on the wire ----------------------- +# +# The wrapper is here rather than the helper itself. `set_tempo_param(real=)` +# calls `protocol.bpm_to_tempo` from inside the protocol layer, so moving the +# helper up to the boundary would make the protocol layer import the model - +# which `tests/test_namespace.py` refuses. Delegating gets one copy of the +# measured span either way. + + +TEMPO_WIRE_VALUES = [0.0, 0.095, 0.355, 0.4, 0.5, 1.0] + + +@pytest.mark.parametrize("value", TEMPO_WIRE_VALUES) +def test_the_tempo_matches_the_protocol_layers_own_conversion(value): + assert translate.tempo_bpm(value) == protocol.tempo_bpm(value) + + +@pytest.mark.parametrize("bpm", [40.0, 59.0, 111.0, 120.0, 240.0]) +def test_the_tempo_round_trips_through_the_wire_scale(bpm): + assert translate.tempo_bpm(translate.bpm_to_tempo(bpm)) == pytest.approx(bpm) + assert translate.bpm_to_tempo(bpm) == protocol.bpm_to_tempo(bpm) + + +@pytest.mark.parametrize("bpm", [39.9, 240.1, 0.0, 1000.0]) +def test_a_tempo_the_unit_has_no_setting_for_is_refused(bpm): + with pytest.raises(ValueError, match="40"): + translate.bpm_to_tempo(bpm) + + +@pytest.mark.parametrize("value", [-0.01, 1.01, 2.0]) +def test_a_tempo_wire_value_off_the_scale_is_refused(value): + """Unlike a level, which converts off the end of its knob. An out-of-span + tempo names a bpm the unit has no setting for, so the protocol helper + refuses it and this one inherits that.""" + with pytest.raises(ValueError): + translate.tempo_bpm(value) + + +def test_the_tempo_is_what_the_protocol_write_expects(): + """The same shape as the tuner and hold-timing checks: the model's idea of + 111 bpm has to be the number `set_tempo_param(real=)` puts on the wire. + Unlike those two this one CAN pass by delegation, so it is a check that the + two paths agree rather than a check on the arithmetic.""" + recorder = Recorder() + qc = protocol.QuadCortex(recorder) + qc.set_tempo_param("TEMPO", real=111.0) + sent = recorder.sent[-1].preset.tempoProgramData[0].params[0] + assert sent.param_values[0].float_value == \ + pytest.approx(translate.bpm_to_tempo(111.0)) + + # -- the tuner's reference pitch: absolute Hz on screen, an offset on the wire @@ -583,6 +639,7 @@ def called(node): PROTOCOL_CONVERSIONS = { "Footswitch", "Scene", "slot_to_position", "position_to_slot", "input_level_db", "db_to_input_level", "lane_level_db", "db_to_lane_level", + "tempo_bpm", "bpm_to_tempo", "UNITY_LEVEL", "HOLD_TIMING_MS", } @@ -653,6 +710,25 @@ def test_the_scan_covers_every_module_that_is_not_the_protocol_layer(): assert not missed, f"the scan does not cover {missed}" +def test_every_name_on_the_allowlist_is_a_real_protocol_name(): + """A misspelled entry protects nothing and says nothing about it. + + The check below only ever compares an attribute name against this set, so + `tempo_bmp` in it would sit there looking like a rule while `protocol.tempo_bpm` + went unwatched. Two lookups because `HOLD_TIMING_MS` hangs off `QuadCortex` + and the rest are module-level. + """ + missing = "not found" + unresolved = sorted( + name for name in PROTOCOL_CONVERSIONS + if getattr(protocol, name, missing) is missing + and getattr(protocol.QuadCortex, name, missing) is missing + ) + assert not unresolved, ( + f"{unresolved} is on the allowlist but is not a name the protocol layer " + f"publishes, so nothing is being kept out of the model by it") + + def test_the_boundary_itself_does_the_arithmetic(): """The exclusion below has to be load-bearing. If the boundary stopped converting, the check would pass because nothing anywhere converts - which From 668bf4fc316d9c67fad1449fd688cbc62ec0ab23 Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Thu, 13 Aug 2026 13:24:04 -0500 Subject: [PATCH 7/8] test: pin the tempo refusal to its actual span, and say what the write check does not prove `match="40"` would have passed on almost any message. The write-path check runs both sides through `protocol.bpm_to_tempo`, so it does not check the arithmetic - what it fails on is `set_tempo_param` routing `real=` through the catalog instead. Said so rather than letting the name imply more. --- tests/test_translation.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_translation.py b/tests/test_translation.py index 879da4f..ac0788a 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -452,7 +452,7 @@ def test_the_tempo_round_trips_through_the_wire_scale(bpm): @pytest.mark.parametrize("bpm", [39.9, 240.1, 0.0, 1000.0]) def test_a_tempo_the_unit_has_no_setting_for_is_refused(bpm): - with pytest.raises(ValueError, match="40"): + with pytest.raises(ValueError, match=r"40\.\.240"): translate.bpm_to_tempo(bpm) @@ -468,8 +468,12 @@ def test_a_tempo_wire_value_off_the_scale_is_refused(value): def test_the_tempo_is_what_the_protocol_write_expects(): """The same shape as the tuner and hold-timing checks: the model's idea of 111 bpm has to be the number `set_tempo_param(real=)` puts on the wire. - Unlike those two this one CAN pass by delegation, so it is a check that the - two paths agree rather than a check on the arithmetic.""" + + Weaker than those two, and worth saying so. Both sides of this equality run + through `protocol.bpm_to_tempo`, so it does not check the arithmetic. What it + fails on is `set_tempo_param` routing `real=` somewhere else - the catalog, + whose published range for TEMPO is a placeholder and would send a different + number.""" recorder = Recorder() qc = protocol.QuadCortex(recorder) qc.set_tempo_param("TEMPO", real=111.0) @@ -718,7 +722,7 @@ def test_every_name_on_the_allowlist_is_a_real_protocol_name(): went unwatched. Two lookups because `HOLD_TIMING_MS` hangs off `QuadCortex` and the rest are module-level. """ - missing = "not found" + missing = object() unresolved = sorted( name for name in PROTOCOL_CONVERSIONS if getattr(protocol, name, missing) is missing From 4eb997e45225f9d67f8b7372b93da00b3ac2eb5a Mon Sep 17 00:00:00 2001 From: Jonathan Stokes Date: Thu, 13 Aug 2026 14:00:03 -0500 Subject: [PATCH 8/8] review: close the findings from the PR #21 re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocker first. `translate.slot_to_position("٢٨C")` returned 218 - a real preset, from a name no screen shows, through a public function of the module whose whole job is to stop that. Only `PresetAddress.parse` carried the ASCII-digit pattern, while a comment and a test both read as though the module was covered. Both doors share one pattern now, and one list of malformed names runs through both. The protocol helper still accepts those digits by design (`str.isdigit()` is true for them), so there is a test pinning that too - if it ever tightens, the comment saying the boundary holds the line stops being true. Then the guard cluster, which was the same finding as the last two reviews, one layer down: the checks were narrower than they read. - the arithmetic check now sees a letter table as a tuple, list or dict, not only as a string; `string.ascii_uppercase`; the literal 65; and `ROWS.index(row)`, which converts a coordinate using the boundary's own exported table with no arithmetic in it anywhere - the allowlist gained the protocol readers that hand back raw wire coordinates, `stomp_assignments` among them - it returns the footswitch index whose confusion with a column is why `FootswitchLetter` exists. Verified: a file doing all of that at once passed both checks before and fails both now - `test_the_boundary_itself_does_the_arithmetic` was anchored to the file, and was satisfied by an error-message formatter in `_screen_number`. It names the four converters now, so they cannot quietly stop converting - a new derived check: everything the boundary delegates to must be on the allowlist. That is the direction the list actually rots, and it is what missed #22's tempo helpers Both checks now pin their KNOWN blind spots as blind spots. A sample table where every "should be caught" case is caught reads like a completeness proof; these fail if a listed gap closes, which is the edit where the prose gets fixed too. Also: - the layering check could not see `from pyquadcortex import PresetAddress`, a hole this story opened by re-exporting the value types. It reads `device.__all__` now, so it follows the code - `tests/test_docs.py` still exempted the dead `model` path and flagged the live `device` one - the rename missed it, and it would have fired at story #12 - the hold-timing and tuner tests said more than they prove. Both are delegation checks; they say so now, and say where the numbers are actually pinned - `check_artifacts.py` did not require the two `__init__.py` files that decide what `import pyquadcortex` hands back - the prose in CLAUDE.md, STEERING, architecture.md and domain-model.md scoped the rule to the model directory while the test scans the whole package - roadmap.md's illustrative snippet still showed `preset.rows[0]` - architecture.md said compile_protos.sh is the only script in the repo --- CLAUDE.md | 2 +- docs/STEERING.md | 55 ++++- docs/architecture.md | 18 +- docs/domain-model.md | 5 +- docs/roadmap.md | 4 +- pyquadcortex/device/translate.py | 31 ++- scripts/check_artifacts.py | 5 + tests/test_docs.py | 8 +- tests/test_namespace.py | 48 ++++- tests/test_translation.py | 337 ++++++++++++++++++++++++++++--- 10 files changed, 447 insertions(+), 66 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 81db1c7..8437adf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,7 +9,7 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr - Dev setup: `uv venv && uv pip install -e ".[dev]"` (or plain venv + pip, see contributing.md). Run tests with `.venv/bin/python -m pytest`. The suite passes offline - no hardware, no `hid` import, no `DYLD_LIBRARY_PATH`. - Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because in this codebase the identifier `model` means an amp or pedal block (`protocol/models.py`, `catalog.Model`, `ModelCatalog`, `set_block(model=...)`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`. -- Every conversion between a screen value and a wire value lives in `pyquadcortex/device/translate.py` and nowhere else in the model: rows 1-4, slots 1-8, scene and footswitch letters, preset addresses, display units. No `+1`/`-1` on a coordinate outside it, and no model module reaching past it for a protocol conversion helper - `tests/test_translation.py` reads the source and proves both. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is. +- Every conversion between a screen value and a wire value lives in `pyquadcortex/device/translate.py` and nowhere else in the package outside `pyquadcortex/protocol/` - the whole package, not just `device/`, because a rule scoped to a directory is satisfied by moving the code one directory up. It covers rows 1-4, slots 1-8, scene and footswitch letters, preset addresses and display units. Outside the boundary that means no `+1`/`-1` on a coordinate AND none of the other spellings of the same conversion (`ord`/`chr`, a letter table in any container, `divmod` on a position, a one-based `enumerate`, `ROWS.index(...)`), and no module reaching past the boundary for a protocol-layer name that carries a coordinate or a raw scale - the converters and also the readers that hand back wire indexes, such as `protocol.stomp_assignments`. `tests/test_translation.py` reads the source and proves both, and pins where each check stops seeing rather than implying it sees everything. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is, and its protocol-layer name joins that file's allowlist in the same commit. - The model represents what the unit shows, in the unit's own words, and never guesses. A control we understand but cannot yet drive is modelled and REFUSES the operation (ADR-0007); a control we do not understand is omitted, with the reason recorded in `docs/domain-model.md`'s appendix. Nothing ships with a "this might be stale or wrong" caveat. - A model property that reads a device field checks the field is PRESENT (`protocol.field_present`) before reporting it. Most of this schema sits in synthetic `oneof`s, so protobuf returns `""` or `0` for a field the unit never sent, and reporting that as the answer is the guess the rule above forbids. Never cache a reply that came back incomplete - a retry has to be able to recover. - Anything the model caches is valid only while its connection is. A closed `Device` refuses reads rather than answering from cache, because a model that reports the unit's state through an object with no unit behind it is the failure the whole layer exists to avoid. diff --git a/docs/STEERING.md b/docs/STEERING.md index 0f57575..93460b8 100644 --- a/docs/STEERING.md +++ b/docs/STEERING.md @@ -2,7 +2,7 @@ > **What this is:** durable technical context for the pyquadcortex library - what the system is and why it is shaped this way. > **What this is not:** coding rules (see the repo-root `CLAUDE.md`) or decision rationale (see [`ADR.md`](ADR.md)). -> **Last reviewed:** 2026-08-03 by Stokes +> **Last reviewed:** 2026-08-13 by Stokes > **Owners:** Stokes ## 1. Purpose @@ -57,7 +57,7 @@ The protocol layer is stateless between calls: every read is a live exchange, an | Fake-per-layer offline tests | Each layer has a purpose-built double: golden captured frames for `framing`, `FakeHid` for `transport`, `FakeTransport` for `client` | see ADR-0002 | `FakeTransport` in `tests/test_client.py` | Hardware verification happens manually via `examples/`, outside the suite | | Evidence-bearing docstrings | Each operation's docstring states what is confirmed on hardware vs inferred from the schema | The device gives no errors for wrong writes, so recorded evidence is the only trail | `QuadCortex.read_preset` in `pyquadcortex/protocol/client.py` | Non-protocol helpers (pure functions) carry ordinary docstrings | | Keyed grid edits | Mutations are row/column-keyed `Grid` UPDATEs | The device applies grid updates by key; wholesale preset writes are silently ignored (see [`architecture.md`](architecture.md), "write_preset is a trap") | `QuadCortex.set_bypass` in `pyquadcortex/protocol/client.py` | Read paths, and non-grid operations | -| One translation boundary | Screen values become wire values in exactly one module, and a source-reading test proves no other model module does it | An off-by-one row is silent - the write lands on a real row and reads back perfectly - so a convention cannot be trusted to hold (design principle 5 in [`domain-model.md`](domain-model.md)) | `pyquadcortex/device/translate.py` | The protocol layer, which keeps its zero-based indexes and raw scales | +| One translation boundary | Screen values become wire values in exactly one module, and a source-reading test proves no other module in the package does it - the whole package outside `protocol/`, not just `device/` | An off-by-one row is silent - the write lands on a real row and reads back perfectly - so a convention cannot be trusted to hold (design principle 5 in [`domain-model.md`](domain-model.md)) | `pyquadcortex/device/translate.py` | The protocol layer, which keeps its zero-based indexes and raw scales | ## 6. Constraints @@ -123,7 +123,7 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) ## Change Log -### 2026-08-12 - One translation boundary, and the model package is `device/` +### 2026-08-13 - One translation boundary, and the model package is `device/` **What changed:** - `pyquadcortex/device/translate.py`: the one module where a screen value becomes a wire @@ -156,10 +156,10 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) concept **Scope of impact:** -- **Updated:** STEERING.md, CLAUDE.md, architecture.md, domain-model.md, changelog.md, - `pyquadcortex/device/`, `pyquadcortex/__init__.py`, `scripts/check_artifacts.py`, - `tests/test_translation.py` (new), `tests/test_namespace.py`, - `tests/test_import_cleanliness.py` +- **Updated:** STEERING.md, CLAUDE.md, architecture.md, domain-model.md, roadmap.md, + changelog.md, `pyquadcortex/device/`, `pyquadcortex/__init__.py`, + `scripts/check_artifacts.py`, `tests/test_translation.py` (new), + `tests/test_namespace.py`, `tests/test_import_cleanliness.py`, `tests/test_docs.py` - **Not updated (intentionally):** ADR.md - neither change reverses or refines a recorded decision. The boundary IS design principle 5, already written and reviewed in `domain-model.md`; the rename is a directory name, chosen to stop colliding with an @@ -179,10 +179,14 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) A parameter whose display mapping is unverified stays out of the model entirely (principle 3), so no mapping is ever invented in this module - The arithmetic check is deliberately blunt and deliberately wide: a literal one in any - spelling (`1`, `1.0`, `True`, `-1`), `ord`/`chr`, a letter table, `divmod`, and a - one-based `enumerate` all fail it, anywhere in the package outside the boundary and the - protocol layer. If a future module has a genuine counter, widening the check is a - deliberate edit with a reason, not a quiet one + spelling (`1`, `1.0`, `True`, `-1`), `ord`/`chr`, the literal 65, a letter table as a + string, tuple, list or dict, `string.ascii_uppercase`, `divmod`, a one-based + `enumerate`, and `.index()` on `ROWS` or `SLOTS` all fail it, anywhere in the package + outside the boundary and the protocol layer. If a future module has a genuine counter, + narrowing the check is a deliberate edit with a reason, not a quiet one. What it cannot + see - a one behind a name, a table built at run time, arithmetic inside somebody else's + helper - is pinned as a failing-if-it-changes list in the same file, because a sample + table where every case passes reads like a completeness proof and is not one - The scan is scoped to the whole package rather than to `pyquadcortex/device/`, because a rule scoped to a directory is satisfiable by moving the code one directory up - which is precisely what a failure message naming a directory invites @@ -191,6 +195,35 @@ Single-device, single-connection USB HID at interactive rates (129-byte reports) the helper stays there and the boundary wraps it, the same way it wraps the level scales. Adding the name to the boundary's allowlist is the half that matters: without it, a model module reaching for `protocol.tempo_bpm` passes the check +- The allowlist is judgement, and its criterion is what a name HANDS OVER rather than + whether it reads like a conversion. `protocol.stomp_assignments` returns three raw wire + indexes including the footswitch one, so a model module could key a mapping by it and + reintroduce the exact bug `FootswitchLetter` exists to prevent, without writing a `- 1` + anywhere. The readers are therefore listed next to the converters + +**Also in this branch:** +- Merged main (PRs #19, #20, #22) up. The three change logs conflicted in the same place + and both sides were kept in the order they landed +- The review found the boundary's own front door open: `translate.slot_to_position` + accepted non-ASCII digits, so `"٢٨C"` returned preset 218. Only `PresetAddress.parse` + carried the ASCII pattern, while a comment and a test both read as though the module + was covered. The two doors now share one pattern, and one list of malformed names is + run through both +- Both source-reading checks were narrower than they read, again. The arithmetic check + now sees a letter table in a tuple, list or dict, `string.ascii_uppercase`, the literal + 65, and `ROWS.index(row)` - a coordinate conversion written with the boundary's own + exported table and no arithmetic in it at all. The allowlist gained the protocol + readers that hand back wire coordinates. A file doing all of that at once passed both + checks before and fails both now +- The backstop that proves the boundary still converts is anchored to the four converters + by name. It had been satisfied by an error-message formatter elsewhere in the file, so + the converters could have gone arithmetic-free with nothing failing +- Each check now pins its KNOWN blind spots as blind spots. A sample table where every + "should be caught" case is caught reads like a completeness proof; these fail if a + listed gap ever closes, which is the edit where the prose gets corrected too +- The layering check could not see `from pyquadcortex import PresetAddress`, a hole this + story opened by re-exporting the value types at top level. It reads `device.__all__` + now, so it follows the code ### 2026-08-12 - TEMPO MODE closes, and ADR-0010 diff --git a/docs/architecture.md b/docs/architecture.md index df636af..7ca8e9e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -209,16 +209,19 @@ built story by story. Nothing is stubbed out to look finished. The model speaks what the touchscreen shows - rows 1 to 4, slots 1 to 8, scenes and footswitches as letters, dB, Hz, bpm, ms - and the wire speaks zero-based indexes and raw scales. Every conversion between the two lives here and nowhere else in -`pyquadcortex/device/` (design principle 5 in -[domain-model.md](domain-model.md)). +`pyquadcortex/` outside `protocol/` - the whole package, not just the model +directory (design principle 5 in [domain-model.md](domain-model.md)). One module rather than a convention, because the mistake it prevents is silent. This document's own layer map sits above a protocol layer whose header says it: a write to the wrong row lands on a real row and reads back perfectly, so nothing tells the caller. Collecting the arithmetic in one place makes it reviewable in -one place, and `tests/test_translation.py` proves the rest of the model package -does none of it by reading the source, rather than by trusting anyone to -remember. +one place, and `tests/test_translation.py` proves the rest of the package does +none of it by reading the source, rather than by trusting anyone to remember. +Two things it also proves, because neither is obvious: the four converters +themselves still do the arithmetic (otherwise "nowhere else" passes because +nowhere converts), and each check's known blind spots are pinned as blind spots, +so the sample tables cannot read as completeness proofs. Where a protocol-layer helper already performs the conversion - input gain dB, lane and mixer dB, tempo bpm, the slot-name/position pair - this module calls it @@ -364,8 +367,8 @@ record the result: update your docstring and the coverage table in An operation whose shape comes only from the schema should say so. Useful shapes for hardware work live in `examples/` (`switch_scenes.py`, -`list_presets.py`, `reroute_and_save.py`). `scripts/compile_protos.sh` is the -only script in the repo. +`list_presets.py`, `reroute_and_save.py`). `scripts/` holds +`compile_protos.sh`, `check_artifacts.py` and `generate_models.py`. ## Capturing the device's traffic @@ -472,6 +475,7 @@ How each layer is faked: | `model` | `FakeClient`: answers the calls the model makes on a `QuadCortex`, plus the same monkeypatched device+transport as `session` | `tests/test_device.py` | | schema | asserts the enum integers the code relies on and that core messages instantiate | `tests/test_schema_compiles.py` | | namespaces | the pre-flip `__all__`, read verbatim from git, must all resolve under `pyquadcortex.protocol` | `tests/test_namespace.py` | +| the translation boundary | none: it is pure functions, so it is called directly. Two of its tests take the package's SOURCE as their input instead, and read it with `ast` | `tests/test_translation.py` | ### The import-safety contract diff --git a/docs/domain-model.md b/docs/domain-model.md index 8f6db03..391d2a6 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -40,7 +40,8 @@ units everywhere. Conversion to protocol values (0-based indexes, raw scales) happens in exactly one module at the model-to-protocol seam. No `-1`/`+1` anywhere else. **Built:** `pyquadcortex/device/translate.py`, with the rule enforced by a test that - reads the model package's source rather than trusting a convention. + reads the source of the whole package outside `protocol/` - not just the model + directory - rather than trusting a convention. ## Namespaces: the model becomes the front door @@ -1323,7 +1324,7 @@ the n/a rows below where they intersect the API at all. capture every field of every message the device answers in each switch position and diff, rather than looking for a field you expect. Still an M3 surface, so still not a behaviour change at M1. -- **2026-08-12** - Design principle 5 is built (M1 story #10): `pyquadcortex/device/translate.py` +- **2026-08-13** - Design principle 5 is built (M1 story #10): `pyquadcortex/device/translate.py` owns every conversion between a screen value and a wire value - including the tempo's bpm, which arrived from the TEMPO MODE work above - with `PresetAddress`, `FootswitchLetter` and `SceneLetter` landing as part of it. The model package directory diff --git a/docs/roadmap.md b/docs/roadmap.md index a8be7d4..c6ab4e3 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -63,8 +63,8 @@ with pyquadcortex.connect() as qc: for block in scene.blocks: print(block.position, block.enabled) # effective state, sceneMode absorbed - scene.blocks[1, 3].enabled = False # grid coordinates - preset.rows[0].input = Input.RETURN_1 + scene.blocks[1, 3].enabled = False # grid coordinates, as on screen + preset.rows[1].input = Input.RETURN_1 # rows are 1 to 4, like the unit preset.save() # the recall/edit/save dance is internal preset.scenes["D"].copy_from(scene, keep_label=True) diff --git a/pyquadcortex/device/translate.py b/pyquadcortex/device/translate.py index 73a986b..670c313 100644 --- a/pyquadcortex/device/translate.py +++ b/pyquadcortex/device/translate.py @@ -284,6 +284,18 @@ def scene_from_wire(index) -> SceneLetter: # -- preset addresses: "28C" on screen, a linear position on the wire ------- +#: What a slot name may look like to the model: ASCII digits, then one letter, +#: with nothing between them. +#: +#: The protocol helper is looser. It checks the bank with ``str.isdigit()``, +#: which is true for every Unicode digit, and ``int()`` reads those too - so +#: ``protocol.slot_to_position("٢٨C")`` returns 218. That is a real position for +#: a name no screen ever shows, which is the shape of mistake this module exists +#: to stop, so the model checks the name before handing it down. Both of the +#: boundary's doors use this one pattern; they disagreed when only +#: :meth:`PresetAddress.parse` had it. +_SLOT_NAME = re.compile(r"\s*([0-9]+)([A-Za-z])\s*") + def slot_to_position(name: str) -> int: """A preset's slot name ("28C") as the linear position the wire carries (218). @@ -297,8 +309,11 @@ def slot_to_position(name: str) -> int: the footswitches are doing, which is why it is what goes on the wire and why two addresses are best compared as positions. - Delegates to :func:`pyquadcortex.protocol.slot_to_position`, so the model and - the protocol layer cannot drift apart on what "28C" means. A zero-padded bank + How big a setlist is, and the arithmetic, are + :func:`pyquadcortex.protocol.slot_to_position`'s, so the model and the + protocol layer cannot drift apart on what "28C" means. The SHAPE of the name + is checked here first, against :data:`_SLOT_NAME`, because the protocol + helper accepts non-ASCII digits and the model should not. A zero-padded bank ("01A") is accepted; :func:`position_to_slot` renders unpadded by default, because that is what the unit displays. """ @@ -306,6 +321,10 @@ def slot_to_position(name: str) -> int: raise TypeError( f"a slot name is text like '28C', not {type(name).__name__} " f"({name!r})") + if not _SLOT_NAME.fullmatch(name): + raise ValueError( + f"a slot name is a bank number and a letter A to H, like '28C': " + f"{name!r}") return protocol.slot_to_position(name) @@ -373,10 +392,10 @@ def parse(cls, text: str) -> "PresetAddress": raise TypeError( f"a preset address is text like '28C', not " f"{type(text).__name__} ({text!r})") - # ASCII digits only, and nothing between the bank and the letter. - # Python's `\d` spans every Unicode digit, so an unrestricted pattern - # read "٢٨C" as bank 28. - match = re.fullmatch(r"\s*([0-9]+)([A-Za-z])\s*", text) + # The same pattern :func:`slot_to_position` uses. Python's `\d` spans + # every Unicode digit, and so does `str.isdigit()`, so an unrestricted + # check reads "٢٨C" as bank 28 - see :data:`_SLOT_NAME`. + match = _SLOT_NAME.fullmatch(text) if not match: raise ValueError( f"a preset address is a bank number and a letter A to H, like " diff --git a/scripts/check_artifacts.py b/scripts/check_artifacts.py index f2e25fd..41cecd7 100755 --- a/scripts/check_artifacts.py +++ b/scripts/check_artifacts.py @@ -27,6 +27,11 @@ "pyquadcortex/protocol/client.py", "pyquadcortex/device/device.py", "pyquadcortex/device/translate.py", + # The two files that DECIDE what `import pyquadcortex` hands back. Ship a + # wheel without either and every module above is still present and correct, + # while the package exports nothing. + "pyquadcortex/device/__init__.py", + "pyquadcortex/__init__.py", "pyquadcortex/_version.py", ) diff --git a/tests/test_docs.py b/tests/test_docs.py index f53ebe2..a279d49 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -62,8 +62,14 @@ def test_the_api_table_carries_no_pre_flip_import_paths(): it wants a `(`. Before the move that row was the case being checked. A rule that silently passes over exactly the rows this change could leave behind is worth nothing here, so stale spellings are named instead. + + The two exempt prefixes are the two namespaces that exist: `protocol` and + `device`. The exemption said `model` until the model package was renamed, + which had it backwards - waving through the dead path and flagging the live + one. Nothing failed, because api.md documents the protocol layer and carries + no model rows yet; the first one would have tripped it. """ - stale = re.findall(r"`pyquadcortex\.(?!protocol\b|model\b)[a-z_][a-z0-9_.]*\(", + stale = re.findall(r"`pyquadcortex\.(?!protocol\b|device\b)[a-z_][a-z0-9_.]*\(", _api_table_rows()) assert not stale, ( f"docs/api.md still shows {sorted(set(stale))} - the message-level API " diff --git a/tests/test_namespace.py b/tests/test_namespace.py index 10ddbba..e6c7323 100644 --- a/tests/test_namespace.py +++ b/tests/test_namespace.py @@ -153,15 +153,26 @@ def test_the_protocol_sources_were_actually_found(): #: with its own guard test still green. MODEL_PACKAGE = device.__name__ +#: The model's names as `pyquadcortex` re-exports them, taken from the model +#: package itself so the set follows the code. +#: +#: `from pyquadcortex import PresetAddress` reaches the model without naming the +#: model package at all - the AST sees `pyquadcortex.PresetAddress`, which is not +#: a module and does not start with `pyquadcortex.device`. That spelling became +#: reachable when the boundary re-exported its value types at top level, so the +#: check has to know which top-level names ARE the model. +RE_EXPORTED = {f"pyquadcortex.{name}" for name in device.__all__} + def _is_the_model(dotted: str) -> bool: - """True for the model package and anything inside it, and nothing else. + """True for the model package, anything inside it, and its re-exported names. The dot boundary matters: a hypothetical top-level `pyquadcortex/devices.py` is a different module, and a prefix test with no boundary would report importing it as a layering violation. """ - return dotted == MODEL_PACKAGE or dotted.startswith(MODEL_PACKAGE + ".") + return (dotted == MODEL_PACKAGE or dotted.startswith(MODEL_PACKAGE + ".") + or dotted in RE_EXPORTED) def _package_of(source: pathlib.Path) -> str: @@ -233,10 +244,16 @@ def test_the_protocol_layer_never_imports_the_model(source): ("relative from", "from ..device import Device", True), ("relative attribute", "from .. import device", True), ("the module inside it", "from pyquadcortex.device import device", True), + ("a re-exported value type", "from pyquadcortex import PresetAddress", True), + ("a re-exported type, renamed", + "from pyquadcortex import FootswitchLetter as FS", True), + ("two of them at once", + "from pyquadcortex import SceneLetter, PresetAddress", True), ("a sibling module", "from pyquadcortex.protocol import client", False), ("a hypothetical devices.py", "from pyquadcortex import devices", False), ("a name, not a module", "from pyquadcortex.protocol import open_device", False), + ("the package's own version", "from pyquadcortex import __version__", False), ] @@ -247,7 +264,32 @@ def test_the_layering_check_reads_every_import_spelling(label, source, """Guards the check above against the imports it cannot see. A layering rule enforced by a check with blind spots is enforced only for - the spellings someone happened to think of. + the spellings someone happened to think of. The re-export cases are the ones + this story added: `pyquadcortex` now hands out three model types at top + level, and `from pyquadcortex import PresetAddress` names no module. """ found = _imported_modules(ast.parse(source), "pyquadcortex.protocol") assert any(_is_the_model(m) for m in found) is is_a_violation + + +IMPORTS_THE_CHECK_CANNOT_SEE = [ + ("a star import", "from pyquadcortex import *"), + ("the package, then an attribute reach", + "import pyquadcortex\nx = pyquadcortex.device.translate.row_to_wire(row)"), +] + + +@pytest.mark.parametrize("label,source", IMPORTS_THE_CHECK_CANNOT_SEE, + ids=[s[0] for s in IMPORTS_THE_CHECK_CANNOT_SEE]) +def test_the_layering_check_says_where_it_stops(label, source): + """Written down rather than left to be discovered. + + This reads only import statements, so a name that arrives through `*`, or a + module reached by attribute after importing the package, is invisible to it. + Neither is house style and neither appears in the tree, but a sample table + where every case passes reads like a completeness proof. If one of these + starts being caught, move it up into `IMPORT_SPELLINGS`. + """ + found = _imported_modules(ast.parse(source), "pyquadcortex.protocol") + assert not any(_is_the_model(m) for m in found), ( + f"the check now sees {label!r} - move it into IMPORT_SPELLINGS") diff --git a/tests/test_translation.py b/tests/test_translation.py index ac0788a..3c1391d 100644 --- a/tests/test_translation.py +++ b/tests/test_translation.py @@ -229,9 +229,18 @@ def test_parsing_normalises_what_a_person_types(written, bank, position): assert (address.bank, address.position) == (bank, position) -@pytest.mark.parametrize("malformed", [ - "", " ", "C", "28", "28I", "0A", "33A", "28CC", "-1A", "2.5C", "A28", -]) +#: Names the unit never shows. One list, because the boundary has TWO public +#: doors onto this conversion and a name that only one of them refuses is a hole +#: with a test in front of it - see the two tests below. +#: +#: "٢٨C" and "28²C" are the ones that were getting through. Python's `\d` spans +#: every Unicode digit and so does `str.isdigit()`, which is what the protocol +#: helper checks the bank with, and `int()` reads those digits too. +MALFORMED_NAMES = ["", " ", "C", "28", "28I", "0A", "33A", "28CC", "-1A", + "2.5C", "A28", "28 C", "2 8C", "٢٨C", "28²C"] + + +@pytest.mark.parametrize("malformed", MALFORMED_NAMES) def test_a_malformed_address_is_refused_when_it_is_parsed(malformed): """Not when it is written. A bad address that survives parsing becomes a wire position, and a wrong position is a preset that recalls fine and is the @@ -240,6 +249,21 @@ def test_a_malformed_address_is_refused_when_it_is_parsed(malformed): translate.PresetAddress.parse(malformed) +@pytest.mark.parametrize("malformed", MALFORMED_NAMES) +def test_the_other_door_onto_a_slot_name_refuses_them_too(malformed): + """`slot_to_position` is public, converts the same names, and was refusing a + different set. + + Only `PresetAddress.parse` had the ASCII-digit pattern, so + `translate.slot_to_position("٢٨C")` returned 218 - a real preset, from a name + no screen shows, through the front door of the module whose whole job is to + stop exactly that. Delegation could not fix it: the protocol helper accepts + those digits by design, so the shape check has to be at the boundary. + """ + with pytest.raises(ValueError): + translate.slot_to_position(malformed) + + @pytest.mark.parametrize("wrong_type", [None, 218, ["28C"]]) def test_an_address_that_is_not_text_is_refused(wrong_type): with pytest.raises(TypeError): @@ -303,12 +327,16 @@ def test_a_slot_name_that_is_not_text_is_refused(name): translate.slot_to_position(name) -@pytest.mark.parametrize("malformed", ["28 C", "٢٨C", "2 8C"]) -def test_an_address_with_stray_characters_is_refused(malformed): - """Internal whitespace and non-ASCII digits both parsed before: Python's - `\\d` spans every Unicode digit, so "٢٨C" read as bank 28.""" +def test_the_protocol_helper_really_is_looser_about_digits(): + """The reason the shape check is at the boundary and not left to delegation. + + If this ever starts raising, the protocol layer has tightened and the + boundary's own pattern is no longer the thing holding the line - which is + worth knowing, because the comment on `_SLOT_NAME` says it is. + """ + assert protocol.slot_to_position("٢٨C") == 218 with pytest.raises(ValueError): - translate.PresetAddress.parse(malformed) + translate.slot_to_position("٢٨C") def test_the_address_conversion_says_the_naming_depends_on_the_mode(): @@ -317,6 +345,8 @@ def test_the_address_conversion_says_the_naming_depends_on_the_mode(): A caller who does not know that will mis-address a preset, so the function that converts has to say it.""" doc = translate.slot_to_position.__doc__ + if doc is None: # python -OO strips docstrings + pytest.skip("docstrings are stripped under -OO") assert "mode" in doc and "unambiguous" in doc assert "HYBRID" in doc or "hybrid" in doc @@ -493,8 +523,16 @@ def test_the_tuner_reference_converts_both_ways(hz, offset): def test_the_tuner_reference_is_what_the_protocol_write_expects(): - """The reference is the protocol method itself: 442 Hz on screen broadcast - `frequency: 1.99999809`, so the wire carries the offset from 440.""" + """442 Hz on screen broadcast `frequency: 1.99999809`, so the wire carries + the offset from 440 and the model has to hand the method that offset. + + `set_tuner_reference` does no arithmetic - it passes its argument through as + `frequency=` - so the protocol METHOD is not an independent reference for + the number, and this does not check the 440. What it catches is the `+ 440` + moving across the seam: if the model started sending absolute Hz, or the + method started adding the offset itself, one of the two would double up and + this fails. + """ recorder = Recorder() qc = protocol.QuadCortex(recorder) qc.set_tuner_reference(translate.hz_to_tuner_reference(442.0)) @@ -518,7 +556,17 @@ def test_the_tuner_reference_is_not_rounded_to_a_precision_nobody_has_read(): @pytest.mark.parametrize("index", range(6)) -def test_every_hold_timing_index_reads_as_the_screens_milliseconds(index): +def test_every_hold_timing_index_reads_the_same_way_the_protocol_layer_does(index): + """The shared constant is the reference, so this does NOT check the six + numbers - `ms_to_hold_timing(reference[index]) == index` reduces to + `tuple.index(tuple[i]) == i`, true for any six distinct values. Replace + `HOLD_TIMING_MS` with nonsense and this stays green. + + The numbers are pinned as literals where they were read off the unit, in + `tests/test_client.py::test_set_hold_timing_writes_the_index_not_the_milliseconds` + (500 ms is index 0, 800 is 3, 1000 is 5). What this pins is the pair being + inverses of each other over that constant, whatever it holds. + """ reference = protocol.QuadCortex.HOLD_TIMING_MS assert translate.hold_timing_ms(index) == reference[index] assert translate.ms_to_hold_timing(reference[index]) == index @@ -584,6 +632,17 @@ def test_a_hold_timing_index_that_is_not_a_whole_number_is_refused(index): OTHER_MODEL_SOURCES = [p for p in MODEL_SOURCES if p != BOUNDARY] +#: The boundary's own coordinate tables. It publishes them, so a model module +#: can convert a coordinate with `translate.ROWS.index(row)` and never write a +#: `- 1` at all. That spelling used the boundary's own names to get around the +#: boundary, which is why looking one up is treated as arithmetic. +COORDINATE_TABLES = ("ROWS", "SLOTS") + +#: Letters the unit labels a scene or a footswitch with. A table of these, in +#: any container, is a conversion whether or not any number appears near it. +UNIT_LETTERS = "ABCDEFGH" + + def _index_arithmetic(tree: ast.AST) -> list[str]: """Every spelling of index arithmetic in `tree`, as "line N: what". @@ -592,7 +651,16 @@ def _index_arithmetic(tree: ast.AST) -> list[str]: is called by the time somebody has extracted a helper for it. The calls listed here are how a person actually writes the conversions this module owns: `ord`/`chr` or a letter table for a scene or footswitch letter, - `divmod` for a preset address. + `divmod` for a preset address, and `ROWS.index(...)` for a coordinate, + which does the job with no arithmetic in it anywhere. + + **What it cannot see**, so that nobody reads this as a proof. A constant + with a name (`OFFSET = 1` then `row - OFFSET`) needs the value followed + across statements, and a table built at run time (`tuple(range(1, 5))`, + a comprehension over `ascii_uppercase`) needs it evaluated. Both are past + what an AST pass does, and a check that claimed otherwise would be worse + than one that says where it stops. `ARITHMETIC_BLIND_SPOTS` below pins the + known ones so the gap is written down rather than discovered. """ def one(node) -> bool: """A literal one, however spelled: 1, 1.0, or True - which equals 1.""" @@ -606,22 +674,54 @@ def offset(node) -> bool: and isinstance(node.op, ast.USub) and one(node.operand)) + def letters(values) -> bool: + """A run of the unit's letters, as a sequence of one-character strings.""" + if len(values) < 3 or not all(isinstance(v, str) and len(v) == 1 + for v in values): + return False + return "".join(values) in UNIT_LETTERS + def letter_table(node) -> bool: - """A string literal that is a run of the letters the unit labels with.""" - return (isinstance(node, ast.Constant) and isinstance(node.value, str) - and len(node.value) >= 3 and "ABCDEFGH".startswith(node.value)) + """A literal table of the letters the unit labels with. + + A string ("ABCDEFGH"), a tuple or list of them, or the keys of a dict - + all four are one lookup away from an index, and only the string spelling + was caught before. A run that does not start at "A" counts: "BCDEFGH" + with an offset is the same conversion with a bug in it. + """ + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return len(node.value) >= 3 and node.value in UNIT_LETTERS + if isinstance(node, (ast.Tuple, ast.List, ast.Set)): + return letters([e.value for e in node.elts + if isinstance(e, ast.Constant)] + if all(isinstance(e, ast.Constant) for e in node.elts) + else []) + if isinstance(node, ast.Dict): + return letters([k.value for k in node.keys + if isinstance(k, ast.Constant)] + if all(isinstance(k, ast.Constant) + for k in node.keys) else []) + return False def called(node): if isinstance(node.func, ast.Name): return node.func.id return node.func.attr if isinstance(node.func, ast.Attribute) else None + def table_lookup(node) -> bool: + """`.index()` on one of the boundary's coordinate tables.""" + return (isinstance(node.func, ast.Attribute) and node.func.attr == "index" + and isinstance(node.func.value, (ast.Name, ast.Attribute)) + and _rightmost_name(node.func.value) in COORDINATE_TABLES) + found = [] for node in ast.walk(tree): where = f"line {getattr(node, 'lineno', 0)}" if isinstance(node, ast.BinOp) and isinstance(node.op, (ast.Add, ast.Sub)): if offset(node.left) or offset(node.right): found.append(f"{where}: adds or subtracts one") + elif _ord_of_a(node.left) or _ord_of_a(node.right): + found.append(f"{where}: 65, which is ord('A') - letter arithmetic") elif isinstance(node, ast.AugAssign) \ and isinstance(node.op, (ast.Add, ast.Sub)) and offset(node.value): found.append(f"{where}: adds or subtracts one") @@ -633,31 +733,70 @@ def called(node): found.append(f"{where}: divmod() - splitting a linear position") elif name == "enumerate" and len(node.args) > 1: found.append(f"{where}: enumerate() with a start offset") + elif table_lookup(node): + found.append(f"{where}: .index() on a coordinate table - a " + f"conversion with no arithmetic in it") + elif isinstance(node, ast.Attribute) \ + and node.attr in ("ascii_uppercase", "ascii_letters"): + found.append(f"{where}: string.{node.attr} - a letter table") elif letter_table(node): - found.append(f"{where}: a letter table, {node.value!r}") + found.append(f"{where}: a letter table") return sorted(set(found)) +def _rightmost_name(node) -> str | None: + """The last name in an attribute chain: `translate.ROWS` gives "ROWS".""" + if isinstance(node, ast.Attribute): + return node.attr + return node.id if isinstance(node, ast.Name) else None + + +def _ord_of_a(node) -> bool: + """The literal 65, which is `ord("A")` written without saying so.""" + return (isinstance(node, ast.Constant) and isinstance(node.value, int) + and not isinstance(node.value, bool) and node.value == 65) + + #: Protocol-layer names that carry a coordinate or a raw scale. Reaching for one #: of these outside the boundary is how a second conversion gets written. +#: +#: **The criterion is what a name HANDS OVER, not whether it looks like a +#: conversion.** `protocol.stomp_assignments` reads as a plain query and returns +#: `StompAssignment(row, column, footswitch)` - three raw wire indexes, one of +#: them the footswitch index whose confusion with a column is the reason +#: `FootswitchLetter` exists at all. A model module that calls it and keys a +#: mapping by that footswitch reintroduces the original bug without writing a +#: single `- 1`, so the readers below are on the list next to the converters. +#: +#: Two tests keep this honest, in the two directions it can rot: every name here +#: must resolve in the protocol layer, and everything the boundary itself +#: delegates to must be here. Neither can tell whether a name the boundary does +#: NOT use is missing - that half is judgement, applied when a protocol-layer +#: name starts handing out a coordinate or a raw scale. PROTOCOL_CONVERSIONS = { + # conversions "Footswitch", "Scene", "slot_to_position", "position_to_slot", "input_level_db", "db_to_input_level", "lane_level_db", "db_to_lane_level", - "tempo_bpm", "bpm_to_tempo", + "tempo_bpm", "bpm_to_tempo", "option_at", "option_value", "UNITY_LEVEL", "HOLD_TIMING_MS", + # readers that hand back raw wire coordinates + "blocks", "Block", "stomp_assignments", "StompAssignment", + "free_rows", "row_status", "RowStatus", "input_chain_rows", + "splits", "Split", } +#: Deliberately NOT here, having been considered: `beats` (already keyed by the +#: 1-based BEAT the screen shows, so nothing is left to convert), `param_options` +#: (option NAMES, no coordinate and no scale) and `describe_mode` (names a mode +#: for a log line). Listing those would make the check fire on model code that +#: has no conversion to do, which teaches people to work around it. -def _protocol_conversions_used(tree: ast.AST) -> list[str]: - """Every protocol-layer conversion name `tree` reaches for. +def _protocol_aliases(tree: ast.AST) -> set: + """The local names that MEAN the protocol layer in `tree`. - Only through the protocol layer: `translate.slot_to_position(...)` is the - boundary doing its job and must not be reported, so a bare attribute name is - not enough to accuse on. - - Which local names MEAN the protocol layer is worked out first, because the - spellings that reach it are not all `protocol.`: a module can alias the - package, import a submodule of it, or reach through two attributes at once + Worked out first, because the spellings that reach it are not all + `protocol.`: a module can alias the package, import a submodule of it, or + reach through two attributes at once (`protocol.QuadCortex.HOLD_TIMING_MS`). Each of those was a hole in the first version of this check, and each is one a person would write without any idea they were evading anything. @@ -677,6 +816,20 @@ def _protocol_conversions_used(tree: ast.AST) -> list[str]: for alias in node.names: if module != "pyquadcortex" or alias.name == "protocol": aliases.add(alias.asname or alias.name) + return aliases + + +def _protocol_names_reached(tree: ast.AST) -> set: + """Every protocol-layer name `tree` reaches for, conversion or not. + + The END of each attribute chain, not the pieces of it: + `protocol.QuadCortex.HOLD_TIMING_MS` reaches for `HOLD_TIMING_MS`, and + counting `QuadCortex` as well would make the allowlist test below demand a + class be listed as a conversion. + """ + aliases = _protocol_aliases(tree) + stepped_through = {id(node.value) for node in ast.walk(tree) + if isinstance(node, ast.Attribute)} def root_of(node): """The leftmost name in an attribute chain, or None.""" @@ -684,6 +837,31 @@ def root_of(node): node = node.value return node.id if isinstance(node, ast.Name) else None + found = set() + for node in ast.walk(tree): + if isinstance(node, ast.Attribute) and id(node) not in stepped_through \ + and root_of(node.value) in aliases: + found.add(node.attr) + elif isinstance(node, ast.ImportFrom) \ + and (node.module or "").startswith("pyquadcortex.protocol"): + found |= {a.name for a in node.names} + return found + + +def _protocol_conversions_used(tree: ast.AST) -> list[str]: + """Every protocol-layer CONVERSION name `tree` reaches for. + + Only through the protocol layer: `translate.slot_to_position(...)` is the + boundary doing its job and must not be reported, so a bare attribute name is + not enough to accuse on. + """ + aliases = _protocol_aliases(tree) + + def root_of(node): + while isinstance(node, ast.Attribute): + node = node.value + return node.id if isinstance(node, ast.Name) else None + found = [] for node in ast.walk(tree): if isinstance(node, ast.Attribute) and node.attr in PROTOCOL_CONVERSIONS \ @@ -733,12 +911,58 @@ def test_every_name_on_the_allowlist_is_a_real_protocol_name(): f"publishes, so nothing is being kept out of the model by it") -def test_the_boundary_itself_does_the_arithmetic(): +def test_the_allowlist_covers_everything_the_boundary_delegates_to(): + """The direction the list actually rots in. + + A conversion arrives at the protocol layer, the boundary starts calling it, + and the allowlist is not updated - so every OTHER module may call it too and + nothing says a word. That is not hypothetical: PR #22 added `tempo_bpm` and + `bpm_to_tempo`, and they sat unlisted until this branch put them in by hand. + + Derived rather than listed, so it cannot rot the same way: whatever + `translate.py` reaches into the protocol layer for is a conversion by + definition, because converting is all that module does. + """ + reached = _protocol_names_reached(ast.parse(BOUNDARY.read_text())) + assert reached, "the boundary reaches for nothing - this check is vacuous" + unlisted = sorted(reached - PROTOCOL_CONVERSIONS) + assert not unlisted, ( + f"{BOUNDARY.name} delegates to the protocol layer's {unlisted}, which " + f"is not in PROTOCOL_CONVERSIONS - so every other module in the package " + f"may call it directly and this suite will not notice") + + +#: The four functions the exclusion exists for. Named, because "somewhere in +#: translate.py" is not the thing being protected. +COORDINATE_CONVERTERS = ("row_to_wire", "row_from_wire", + "slot_to_wire", "slot_from_wire") + + +def test_the_boundary_itself_still_does_the_arithmetic(): """The exclusion below has to be load-bearing. If the boundary stopped converting, the check would pass because nothing anywhere converts - which - is the one failure a "no arithmetic elsewhere" test cannot see.""" - found = _index_arithmetic(ast.parse(BOUNDARY.read_text())) - assert found, f"{BOUNDARY.name} does no index arithmetic at all" + is the one failure a "no arithmetic elsewhere" test cannot see. + + Anchored to the four converters BY NAME rather than to the file. The + file-wide version was satisfied by `_screen_number`'s error-message + formatter (`allowed[-1] - allowed[0] + 1 == len(allowed)`), which converts + nothing - so all four converters could have been rewritten + `ROWS.index(row)`, arithmetic-free, and this backstop would have stayed + green on that one line while the "nowhere else" check stayed silent too. + """ + functions = {node.name: node + for node in ast.walk(ast.parse(BOUNDARY.read_text())) + if isinstance(node, ast.FunctionDef)} + gone = [name for name in COORDINATE_CONVERTERS if name not in functions] + assert not gone, ( + f"{BOUNDARY.name} no longer defines {gone} - this test names the " + f"converters it is protecting, so a rename has to come through here") + silent = [name for name in COORDINATE_CONVERTERS + if not _index_arithmetic(functions[name])] + assert not silent, ( + f"{silent} in {BOUNDARY.name} do no index arithmetic. If the conversion " + f"moved somewhere else, the 'nowhere else' check below is now passing " + f"because nothing anywhere converts") @pytest.mark.parametrize("source", OTHER_MODEL_SOURCES, ids=lambda p: p.name) @@ -777,13 +1001,28 @@ def test_only_the_boundary_reaches_for_a_protocol_conversion(source): ("a letter table lookup", "letter = 'ABCDEFGH'[index]", True), ("a letter table search", "index = 'ABCDEFGH'.index(letter)", True), ("a letter table under any name", "LETTERS = 'ABCDEFGH'", True), + ("a letter table as a tuple", + "LETTERS = ('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H')", True), + ("a letter table as a list", "LETTERS = ['A', 'B', 'C']", True), + ("a letter table as dict keys", "INDEX = {'A': 0, 'B': 1, 'C': 2}", True), + ("a letter table that starts late", "letter = 'BCDEFGH'[index]", True), + ("the standard library's letter table", + "import string\nletter = string.ascii_uppercase[index]", True), + ("ord('A') without ord", "index = letter.encode()[0] - 65", True), ("splitting a linear position", "bank, letter = divmod(position, 8)", True), ("a one-based enumerate", "[(n, r) for n, r in enumerate(rows, 1)]", True), + ("the boundary's own table, looked up", + "from pyquadcortex.device import translate\n" + "wire_row = translate.ROWS.index(row)", True), + ("the same, imported bare", "wire_column = SLOTS.index(slot)", True), ("the boundary doing it", "wire_row = translate.row_to_wire(row)", False), ("arithmetic that is not off-by-one", "total = a + 2", False), ("a plain enumerate", "[(i, r) for i, r in enumerate(rows)]", False), ("a string that is not a letter table", "name = 'ABY Splitter'", False), ("an ordinary attribute", "name = block.device.name", False), + ("a list of names, not letters", "MODELS = ['Amp', 'Bass', 'Cab']", False), + ("index() on something that is not a coordinate table", + "position = names.index(name)", False), ] @@ -794,15 +1033,47 @@ def test_the_arithmetic_check_sees_what_it_claims_to(label, source, detected): somebody happened to think of, while reading as though it enforced all of them. - The samples that earn their place are the ones the first version of this - check missed: letter arithmetic, a letter table, `divmod` on a preset - position, and the three ways of writing one that are not the token `1`. + The samples that earn their place are the ones a version of this check + missed: letter arithmetic, a letter table in any of four containers, + `divmod` on a preset position, the three ways of writing one that are not + the token `1`, and `ROWS.index(row)` - which converts a coordinate using + the boundary's own published table and contains no arithmetic at all. None of those is exotic. They are how the conversions this module owns get written by somebody writing them somewhere else. """ assert bool(_index_arithmetic(ast.parse(source))) is detected +#: Spellings this check CANNOT see, asserted as unseen. +#: +#: A table of samples where every "should be caught" case is caught reads like +#: proof of a complete check. It is not one, and the honest way to say so is to +#: pin the gap rather than describe it: each of these needs a value followed +#: across statements or a table evaluated, which is past what an AST pass does. +#: +#: This test fails if one of them starts being caught. That is the point - the +#: entry moves up into `ARITHMETIC_SAMPLES` and the docs saying where the check +#: stops get corrected in the same edit. +ARITHMETIC_BLIND_SPOTS = [ + ("a one with a name", "OFFSET = 1\nwire_row = row - OFFSET"), + ("a coordinate table under another name, built at run time", + "GRID_ROWS = tuple(range(1, 5))\nwire_row = GRID_ROWS.index(row)"), + ("a letter table under another name, sliced out of a longer one", + "TAGS = 'ABCDEFGHIJ'[:8]\nletter = TAGS[index]"), + ("arithmetic behind a helper somebody else wrote", + "from elsewhere import to_wire\nwire_row = to_wire(row)"), +] + + +@pytest.mark.parametrize("label,source", ARITHMETIC_BLIND_SPOTS, + ids=[s[0] for s in ARITHMETIC_BLIND_SPOTS]) +def test_the_arithmetic_check_says_where_it_stops(label, source): + """See `ARITHMETIC_BLIND_SPOTS`. These are the ones known to get through.""" + assert not _index_arithmetic(ast.parse(source)), ( + f"the check now catches {label!r} - move it into ARITHMETIC_SAMPLES and " + f"correct the 'what it cannot see' paragraph in _index_arithmetic") + + CONVERSION_SAMPLES = [ ("an attribute", "x = protocol.Footswitch.A", True), ("through the package", "x = pyquadcortex.protocol.lane_level_db(v)", True),