OM-M1.2: one translation boundary between screen values and wire values - #21
Conversation
"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.
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.
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.
…ion-boundary # Conflicts: # docs/STEERING.md
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.
Three change logs conflicted, all in the same place: main's Unreleased section and this branch's both grew an entry after the `Device` one. Kept both, in the order they landed. - changelog.md: the broadcast listener (#20) then the translation groundwork - docs/STEERING.md: the boundary entry sits above TEMPO MODE (#22) and the listener (#20), which is where the newest entry goes in that file - docs/domain-model.md: TEMPO MODE closing then principle 5 being built, which is the order that file reads in
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.
…e 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.
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
jonathanstokes
left a comment
jonathanstokes
left a comment
There was a problem hiding this comment.
Re-review findings, posted for the record. GitHub will not let the author request changes on their own pull request, so this is a comment review; treat finding 1 as a merge blocker anyway.
Re-review of the branch after the merge with main and the tempo commit.
What this PR does
The Quad Cortex screen counts rows 1 to 4 and labels footswitches A to H. The wire underneath counts from zero. If library code subtracts one in the wrong place, the write still succeeds, the device still accepts it, and reading it back returns exactly what was written. You only find out when you plug in a guitar and the wrong pedal is switched off.
This PR puts every one of those conversions in a single file, pyquadcortex/device/translate.py, and adds two unusual tests that read the package's source code to prove no other file does the arithmetic or goes around the boundary. It also renames the model directory from model/ to device/, because model already means "amp or pedal" elsewhere in the codebase.
The design is right and the discipline is high. The merge itself is clean: 165 insertions against 5 deletions across the four docs, and all 5 deletions are model/ to device/ rewrites. Nothing from #19, #20 or #22 was clobbered, duplicated or reordered, and the rename sweep leaves no surviving pyquadcortex.model anywhere outside a correct historical changelog entry. Every factual claim translate.py makes about the protocol layer checks out. Suite green: 1062 passed, 1 skipped.
Three things stop it short, and they are in the threads below.
Blocking
1 is an actual bug: the front-door function that turns a preset address like "28C" into a number still accepts Arabic-Indic digits, so "\u0662\u0668C" becomes preset 218. A comment and a test both say that hole is closed, because they only check the other function that does the same job.
Substantive
2, 3 and 4 are one problem in three places: the source-reading guards are narrower than they read. They watch a hand-written list of twelve protocol names that leaves out the very function handing out raw footswitch numbers, and the arithmetic detector misses several ordinary ways to write the same conversion, including one that uses a table the boundary itself publishes. A plausible future file can reintroduce the original bug and pass both checks clean. 7 is one line and bites at story #12.
5, 6, 8 and 9 are fair as non-blocking.
Nits, not worth a thread
10 scripts/check_artifacts.py REQUIRED gained device/device.py and device/translate.py but not pyquadcortex/device/__init__.py, the file that defines the re-exports, nor pyquadcortex/__init__.py.
11 PR body test counts are stale after the merge: it says 993 passed and 477 for this story; the tree gives 1062 and 503.
12 Date hygiene. The STEERING and domain-model "built" entries are dated 2026-08-12 but were edited on 08-13 to describe the tempo half, and STEERING now has four consecutive 08-12 entries with the date as the only ordering signal. STEERING's "Last reviewed" still says 2026-08-03 while this PR edits sections 4 and 5. There is no "Also in this branch:" block for the merge-up, which is the convention in the entry directly below it.
13 test_the_address_conversion_says_the_naming_depends_on_the_mode asserts that a docstring contains three words. Any sentence containing them satisfies it, and it raises rather than skips under python -OO.
14 The "How each layer is faked" table in architecture.md gained no row for the boundary, though it already carries a non-layer row for namespaces.
Pre-existing, flagging only
15 architecture.md says scripts/compile_protos.sh "is the only script in the repo". That is false, and this PR edits scripts/check_artifacts.py.
16 roadmap.md's illustrative snippet shows preset.rows[0], which contradicts domain-model.md's number: int # 1..4, as on screen. It is now the only doc showing a zero-based model row, in the change that declares the boundary built.
|
|
||
| #: 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 = { |
There was a problem hiding this comment.
2. This list is the whole of the reach-past-the-boundary rule, and it omits most of what it should cover.
Twelve names. The protocol layer also publishes blocks, stomp_assignments, free_rows, row_status, input_chain_rows and splits, all of which carry coordinates, which is this set's own stated criterion.
protocol.stomp_assignments returns StompAssignment(row, column, footswitch) (client.py:3898). All three are raw wire indexes, and the footswitch one is literally the bug FootswitchLetter's docstring cites: a block at column 3 assigned to footswitch E comes back keyed 4. A model module that calls this and keys a mapping by that number reintroduces the bug without writing a single - 1.
test_every_name_on_the_allowlist_is_a_real_protocol_name guards typos, which is the direction that cannot cause the silent bug. The direction that just happened is unguarded: #22 added two conversions, and the list was updated by hand in a later commit. A derivable version exists - parse translate.py and assert every protocol name it reaches for is on the list.
There was a problem hiding this comment.
Fixed in 4eb997e, with one scoping judgement I want visible rather than buried,
so I have left this thread open.
Added to PROTOCOL_CONVERSIONS: blocks, Block, stomp_assignments,
StompAssignment, free_rows, row_status, RowStatus, input_chain_rows,
splits, Split, plus option_at and option_value, which convert between a
normalized 0..1 wire value and an option number. The set's comment now states
the criterion as what a name HANDS OVER rather than whether it reads like a
conversion, and cites stomp_assignments as the case that makes the point.
Also added the derivable check you described:
test_the_allowlist_covers_everything_the_boundary_delegates_to parses
translate.py and asserts every protocol name it reaches for is on the list.
That is the direction the list actually rots, and it is the one that missed #22.
It takes the leaf of each attribute chain, so protocol.QuadCortex.HOLD_TIMING_MS
yields HOLD_TIMING_MS and not QuadCortex.
Three of the names you listed are deliberately not on it, and the reason is
written next to the set: beats is already keyed by the 1-based BEAT the screen
shows, so a model module calling it has nothing left to convert; param_options
returns option NAMES, no coordinate and no scale; describe_mode names a mode
for a log line. Listing those makes the check fire on model code that has no
conversion to do, and a check that fires on correct code teaches people to work
around it. Say the word if you would rather have them in.
Verified: a file doing {a.footswitch: (a.row, a.column) for a in protocol.stomp_assignments(p)} is now named by
test_only_the_boundary_reaches_for_a_protocol_conversion.
| OTHER_MODEL_SOURCES = [p for p in MODEL_SOURCES if p != BOUNDARY] | ||
|
|
||
|
|
||
| def _index_arithmetic(tree: ast.AST) -> list[str]: |
There was a problem hiding this comment.
3. This check is materially narrower than the sample table implies.
Verified as MISSED on the branch:
- a letter table as a tuple,
("A", "B", ... "H"), or as a list, or as dict keys string.ascii_uppercase[index]- a table that does not start at "A", such as
"BCDEFGH" - a named offset,
OFF = 1thenrow - OFF letter.encode()[0] - 65translate.ROWS.index(row)andSLOTS.index(slot), which convert a coordinate with no arithmetic in them at all, using names the boundary itself puts in__all__
Only the string-literal spelling of a letter table is caught, while STEERING.md claims "a letter table" flatly.
The deeper problem is the guard test. All sixteen "should catch" samples are caught and all five "should not" are missed, so test_the_arithmetic_check_sees_what_it_claims_to is entirely self-confirming. Not one entry documents a known blind spot, so the table reads as a completeness proof for a check that is not complete.
There was a problem hiding this comment.
Confirmed on every spelling you listed. Fixed in 4eb997e, with one left open on
purpose - leaving this thread open for that.
Now caught: a letter table as a tuple, a list, a set or dict keys; a run that
does not start at "A"; string.ascii_uppercase and ascii_letters; the literal
65; and .index() on ROWS or SLOTS. That last one was the sharpest of them,
because it converts a coordinate using names the boundary itself exports and
contains no arithmetic at all.
OFFSET = 1 then row - OFFSET is still not caught, and I do not think it
can be by an AST pass - it needs the value followed across statements. Same for
a table built at run time under a name the check does not know, and for
arithmetic inside a helper in another module.
Which brings me to the part of your finding that mattered most. You are right
that a table where all sixteen "should catch" cases are caught and all five
"should not" are missed reads like a completeness proof. There is now a second
table, ARITHMETIC_BLIND_SPOTS, holding the four spellings known to get through
and asserting they get through. If one of them ever starts being caught, that
test fails and the entry moves up into ARITHMETIC_SAMPLES in the same edit
that corrects the prose. The check's docstring and STEERING now say where it
stops instead of implying it stops nowhere.
Writing that table was useful in itself: two of the four "blind spots" I first
wrote turned out to be caught already, which is exactly the kind of thing you
cannot know by reading.
|
|
||
|
|
||
| @pytest.mark.parametrize("index", range(6)) | ||
| def test_every_hold_timing_index_reads_as_the_screens_milliseconds(index): |
There was a problem hiding this comment.
5. This test cannot fail.
translate.ms_to_hold_timing(reference[index]) == index reduces to tuple.index(tuple[i]) == i, which is true for any six-tuple of distinct values. Replace HOLD_TIMING_MS with (100, 200, 300, 800, 900, 1000) and it stays green.
The six literals are genuinely pinned in tests/test_client.py, so the coverage exists. The problem is that this test's name claims it and it does not.
There was a problem hiding this comment.
The arithmetic is right and I am not taking the fix you imply, so this one stays
open for you.
tuple.index(tuple[i]) == i being trivially true is correct. But the fix would
be restating 500, 600, 700, 800, 900, 1000 in this file, and this file's own
rule - the paragraph above the display-unit tests - is that a second copy of a
measured constant drifts and both copies go on returning a plausible number.
HOLD_TIMING_MS is the shared constant on purpose, the same way the level
scales delegate rather than restate.
So what changed in 4eb997e is the claim, not the test. It is now
test_every_hold_timing_index_reads_the_same_way_the_protocol_layer_does, and
its docstring says outright that it does not check the six numbers, that
replacing the constant with nonsense keeps it green, and that the literals are
pinned 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 it does pin is the pair being
inverses over that constant, whatever it holds.
If you would rather have the six numbers duplicated here, that is your call to
make and I will make the change - but it would be the first place in this module
where a measured constant exists twice.
| MODEL_PACKAGE = device.__name__ | ||
|
|
||
|
|
||
| def _is_the_model(dotted: str) -> bool: |
There was a problem hiding this comment.
8. This PR widened a hole in the layering check it depends on.
pyquadcortex/__init__.py now re-exports three model types at top level. _is_the_model cannot see from pyquadcortex import PresetAddress - verified, it yields pyquadcortex.PresetAddress, which matches nothing. Nor from pyquadcortex import *, nor import pyquadcortex followed by pyquadcortex.device.translate.row_to_wire(...).
A lazy in-function import of that form inside a protocol module works at runtime and is invisible to this check. IMPORT_SPELLINGS gained a False case for a name import but no True case for the new re-exports.
There was a problem hiding this comment.
Confirmed, and it is this story's own hole - verified that
from pyquadcortex import PresetAddress yields pyquadcortex.PresetAddress,
which _is_the_model did not match. Fixed in 4eb997e. Leaving open for the second
half.
RE_EXPORTED is built from device.__all__, so it follows the code rather than
being a second list to keep current. IMPORT_SPELLINGS gained three True
cases: the plain re-export, one renamed with as, and two names in one
statement. Proved by patching a protocol module to do that import and watching
test_the_protocol_layer_never_imports_the_model name it:
AssertionError: client.py imports ['pyquadcortex.PresetAddress'] - the protocol
layer must not depend on the model
The other two spellings are still invisible and I have not closed them.
from pyquadcortex import * and import pyquadcortex then
pyquadcortex.device.translate.row_to_wire(...) both need more than reading
import statements - the second needs attribute chains resolved. They are pinned
in IMPORTS_THE_CHECK_CANNOT_SEE, asserted as unseen, so the gap is written
down and the sample table stops reading as a proof. Widening to attribute chains
is a real option if you want it; I did not want to guess at that scope.
The findings that did not get their own threadFindings 1 to 6, 8 and 9 are answered in their threads. These are the rest, all 7. 10. 11. Stale test counts in the PR body. Fixed. The description now says 12. Date hygiene. Fixed. Both "built" entries are dated 2026-08-13, which is 13. The docstring-word test. Half fixed, and worth saying which half. It 14. No row for the boundary in architecture.md's fakes table. Fixed. It says 15. "the only script in the repo". Fixed, since this PR touches 16. Where this leaves the PRFour threads are resolved: the blocker, the backstop anchoring, the tuner
Suite: |
1 participant
Closes #10.
First, a rename you will see in the diff
The model's package directory is now
pyquadcortex/device/instead ofpyquadcortex/model/. It is the first commit on the branch, on its own, so therest of the diff stays readable.
The reason is that model already means something else here. In this codebase the
identifier
modelis an amp or a pedal block:protocol/models.py,catalog.Model,ModelCatalog,set_block(model=...). The design doc renamedthat concept to virtual device in the model's own vocabulary, because that is
what the screen calls it, but the protocol layer still spells it
modelin codeand will keep doing so. So a directory called
model/collides with real code areader is looking at.
Nothing published points at the old path. The package exports
protocoland neverexported
model, and the model namespace has not been released at all, so there isno shim to write and nothing for a user to change.
What this adds, and the bug it prevents
The unit shows you rows 1 to 4 and slots 1 to 8. The wire counts from zero. The
same is true for scenes and footswitches, which the screen labels A to H and the
wire numbers 0 to 7, and for levels, which the screen shows in dB and the wire
stores as a raw 0 to 1 value.
So something has to convert. The question is where.
Right now the answer is one file,
pyquadcortex/device/translate.py, and nothingelse in the model is allowed to do it. That sounds like tidiness. It is not. It is
the whole point of the story, because of how this particular mistake fails.
Write
row - 1where you meantrowand the library edits row 2 instead of row 3.The device accepts it. The write succeeds. Reading it back returns exactly what you
wrote. Nothing anywhere reports a problem. You find out when you plug in a guitar
and the wrong block is bypassed. The protocol layer's own source says this in as
many words. A rule spread across twenty files is a rule someone breaks in the
twenty-first, and nothing catches it.
So two of the tests read the package's source code instead of calling it:
+ 1: alsoord/chr, a letter table like"ABCDEFGH",divmodon a presetposition, a one-based
enumerate, and the three ways of writing one that arenot the token
1(-1,1.0, andTrue, which equals 1)converts, whatever spelling it uses to get there - an aliased package, an
imported submodule, or two attributes at once
Both scan every file in the package that is not the protocol layer, not just the
model directory, because a rule scoped to a directory is satisfiable by moving the
code one directory up. Both have guard tests that feed them samples of what they
should and should not catch, and the samples that earn their place are the
spellings the first version of each check could not see. There is also a test
proving the boundary itself still does arithmetic, so the "nowhere else" test
cannot pass by everything having stopped converting.
Three new names a caller can import
PresetAddressrefuses a bad address when it is parsed, not when something triesto write it. That matters because a bad address that survives parsing still turns
into a number, and the device happily recalls whatever preset is at that number.
FootswitchLetteris the model's only footswitch key, and passing it the number 4raises. That is not style. 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, and every earlier sample
had the two numbers equal. Where a plain number can reach a model API, someone
eventually passes a column to it and gets a write that quietly does nothing.
Where the numbers come from
The two level scales and the tempo do not restate any arithmetic. They call the
protocol-layer helper that already carries the measurement and the evidence for it,
so there is only ever one copy of a measured scale. Two copies drift apart, and both
keep returning a number that looks plausible. The measured points themselves stay
pinned where the measurement lives, in
test_client.py.The other two mappings have no helper to call - the tuner has a documented rule and
hold timing has a shared constant - so their tests pin them against the protocol
method that sends the value, through a fake transport.
The tuner conversion says out loud how thin its evidence is. One observed pair, 442
Hz on screen against 2.0 on the wire, fixes the zero point and the direction and
nothing else. So it converts and does not invent a range the unit might not have.
What the first review changed
The review found that both source-reading guards were narrower than they read, and
that is worth saying plainly, because a guard that misses the natural spellings is
not a smaller version of the guard - it is the appearance of one. The arithmetic
check saw
row - 1and missed letter arithmetic, letter tables,divmod, and- True. The other check only recognised the literal nameprotocol. Both scannedone directory. All of that is fixed, with the missed spellings now in the sample
tables, and the whole-package scan verified by dropping a deliberately bad
pyquadcortex/coords.pyin and watching both checks name it.The review also found a cluster of the same mistake in the conversions themselves:
a wrong value that looks right. The protocol layer's coordinate enums are integers,
so a scene index converted to a row without complaint.
SceneLetterandFootswitchLetterare both strings of one letter, so each was accepted where theother belonged.
PresetAddress.from_wire(218.9)quietly meant preset 218. Allrefused now.
What the merge with main brought in
Main moved three PRs ahead while this branch sat: #19 (the grpcio-tools floor),
#20 (the broadcast listener) and #22 (TEMPO MODE). Three change logs conflicted in
the same way - main's entry and this branch's entry both wanted the same slot - and
both are kept, in the order they landed, in
changelog.md,docs/STEERING.mdanddocs/domain-model.md.The tempo now converts here too. #22 added
tempo_bpm()andbpm_to_tempo()next to the level helpers in
protocol/client.py, and a bpm on screen against a0 to 1 value on the wire is exactly what this boundary is for.
They could not be moved, only wrapped.
set_tempo_param(real=)callsbpm_to_tempofrom insideprotocol/client.py, so relocating the helper wouldmake the protocol layer import the model - the one direction the layering forbids.
That was checked rather than assumed: patching that call site to import from
pyquadcortex.device.translatemakestest_the_protocol_layer_never_imports_the_modelfail and name it.So the pair delegates, the way the level scales already do, with the measured span
attributed to the protocol layer rather than restated here, and with the type guard
this seam adds because the protocol helpers are arithmetic and will happily
multiply a bool.
Truereachingprotocol.tempo_bpmunguarded returns 240 bpm.The half that does the work is the allowlist. Both names are now in
PROTOCOL_CONVERSIONS, so a model module reaching forprotocol.tempo_bpmdirectly is caught the way the other conversions are. Verified by dropping a file
into the package that does both a
row - 1and aprotocol.tempo_bpm(...), andwatching both structural checks name it. There is also a new guard that every name
on that allowlist resolves in the protocol layer - a typo there reads like a rule
and protects nothing.
What the re-review changed
One real bug and one theme, and the theme is the same one the first review found.
The bug.
translate.slot_to_position("28C")written with Arabic-Indic digitsreturned preset 218. Only
PresetAddress.parsecarried the ASCII-digit pattern,while a comment and a test both read as though the module was covered. Two public
doors onto one conversion, refusing different things. They share one pattern now,
and one list of malformed names runs through both. Delegation could not have fixed
it:
protocol.slot_to_positionchecks the bank withstr.isdigit(), which is truefor those digits, so the check has to be at the boundary. There is a test pinning
that too, so the day the protocol layer tightens, the comment that says the
boundary holds the line stops being taken on faith.
The theme: both source-reading guards were narrower than they read.
string.ascii_uppercase, missed the literal 65, and missedtranslate.ROWS.index(row)- which converts a coordinate using the boundary'sown exported table and contains no arithmetic at all
protocol.stomp_assignments, which returns a raw footswitch index. That is thebug
FootswitchLetterexists to prevent, available to any model module, with no- 1written anywheresatisfied by an error-message formatter. All four converters could have stopped
converting with nothing failing
A file doing all three at once passed both checks. It fails both now, and each
check has a derived guard: the boundary cannot delegate to a protocol name that is
not on the allowlist, and the backstop names the four converters rather than the
file.
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. Two extra
tables assert the spellings that get through - a
1behind a name, a table builtat run time, a star import - and fail if one is ever closed, which is the edit
where the prose gets corrected too. Writing them was worth it on its own: two of
the four gaps first written down turned out to be caught already.
Scope
The boundary only. No Directory, no presets, no grid, no cache - those are the next
two stories. Conversions for values this release does not read yet will land with
the surface that needs them, in this same file.
Testing
Fully offline, no hardware, nothing imports
hid.535 of those are this story's, and the count is high on purpose. Exhaustive is the
mitigation here, not overkill: every row, every slot, all eight letters both ways,
all 256 preset addresses in a setlist round-tripped against the protocol layer, and
every entry point checked for the wrong type as well as the wrong value.
scripts/check_artifacts.pywas run against a real build after it gained the two__init__.pyfiles that decide whatimport pyquadcortexhands back.The hardware modules #22 added are import-safe offline, which is the failure a
clean merge hides:
tests/hardware/state_snapshot.py,test_tempo_mode.pyandtest_broadcast_listener.pyall import with no unit attached and nohidinsys.modules, and they still collect under--hardware.