OM-M1.4: read a preset, its grid and its scenes - #24
Conversation
…sibility translate.py had grown past the size where a reader holds it, and the grid readers this story needs have to live inside the boundary - the protocol layer's blocks(), splits() and friends hand back raw wire coordinates, so only the boundary may call them. Splitting first keeps that addition from landing in an 800-line file. The split is by responsibility: guards, coordinates, letters, addresses, units. Every public name is re-exported, so translate.row_to_wire() still resolves and no caller changed. The move is verbatim - the 1199 behavioural tests passed before the structural ones were touched. The structural guard needed care. Its exemption is now a DIRECTORY rather than a file, which is a bigger hole: anything in translate/ is skipped by the index-arithmetic scan. So BOUNDARY_MODULES names the package's modules and a new one has to come through that list. Verified by adding translate/sneaky.py containing '1 - 1': the arithmetic scan skipped it, and the new test caught it.
translate/grid.py turns a BinaryPreset into screen coordinates: occupied cells as row 1-4 and slot 1-8, branches with their splitter and mixer slots, each row's input and output port, per-scene bypass by scene LETTER, and scene names. It lives inside the boundary because the protocol-layer readers it needs - blocks(), splits(), bypass_state() - all hand back raw wire coordinates, so no other module may call them. bypass_state joins PROTOCOL_CONVERSIONS. The criterion is what a name hands over, and BypassState.scenes is an eight-tuple keyed by the wire's scene index: reading scenes[1] for scene B is the conversion, with no minus one in sight. The derived allowlist check caught something real and had to be weakened, which is recorded rather than glossed. Its premise was 'everything the boundary reaches for is a conversion, because converting is all it does'. True while the boundary converted scalars; false once it reads whole presets, because reading needs field_present and the port enums, and CLAUDE.md requires every model property to call field_present. So there is now a second list, PROTOCOL_NON_CONVERSIONS, with a reason per entry and a test that the two do not overlap. Splits had no offline coverage at all - both existing fixtures are serial on every row, so a reader that ignored 'mix' passed everything. Hence split_preset.bin, derived from the real fixture by setting the two branch shapes the protocol layer records from hardware: one that never rejoins (Strat Ambience 05B) and one that does at a different column (Darkglass AO900 1, 27H). The generator is committed beside it and a test asserts the fixture still has both shapes, so a regeneration cannot quietly stop testing splits. Seven mutations checked red, including two that survived the first pass: 'rejoins always set' and reading the wire column without validating it.
device.events carries two things: Changed, when a push moves a value the model holds, and Invalidated, when the model stops trusting part of its copy. The second is what makes the re-read-rather-than-merge decision liveable - a script following the unit closely hears that the grid moved and can fetch it straight away instead of waiting for someone to read a property. Delivery is on a thread the model owns, and that is the whole design rather than a detail. The unit's messages arrive on the transport's receiving thread, which ADR-0009 forbids from reading the unit. Handing an event over there would make the obvious reaction - go and re-read it - raise. So the receiving thread only queues, and this thread hands over, where reading is allowed. A test asserts the subscriber runs on a thread that is not the publisher's, because that property is the reason the class exists. Nothing is queued when nobody has subscribed. The unit pushes its tempo on every beat of every connection, so a stream that queued regardless would grow for the life of a script that never asked for events. Five mutations checked red, including delivering inline on the caller's thread and letting a raising subscriber kill the delivery loop. The wait helpers went in tests/waiting.py rather than conftest.py: there are two conftest files here, and 'from conftest import' picked up the hardware one on a full-tree run. It failed loudly this time and would have failed silently if the two files had ever shared a helper name.
Two entries, both read with one request and one reply. The preset comes
from RecallPreset{READ}, which reads the LIVE grid - unsaved edits
included, no side effects, active scene untouched. read_preset would have
recalled a stored slot and interrupted the audio, which is the opposite of
a read.
FieldPlan grows two flags.
invalidates: every message of this type makes the entry untrusted, whatever
it carries. This is the Grid entry making its own decision about 'action',
which root CLAUDE.md requires rather than inheriting the SCAFFOLDING skip:
action does not matter here, because an UPDATE and a DELETE both mean the
grid moved. It also closes a hole the per-field check cannot see. Renaming
scene A to a blank label sets NOTHING in ListFields() - SceneLabel gives
index and label no presence - so the old check would have watched that
message go by in silence while our copy of the preset went wrong.
new_subject: a different preset is loaded. Moves a counter that
preset.is_current reads, and an ordinary edit deliberately does not move
it: the preset is still the same preset, only our copy is behind.
Grid deltas are not merged, and what merging would take is written down
next to the decision rather than left as a shrug.
Three things the existing tests caught, all real:
- I first kept 'reason' on the RecallPreset plan to stop it marking the
entry. But read_current_preset returns only the preset, so 'reason' could
never be read back - and an entry that keeps a field it cannot re-read
loses it for good the first time it is marked. Dropped, with the three
cases costed in the docstring. Only one can cost a read, and only if the
connect burst's seed sets 'reason' at all, which is unmeasured. The
hardware test that counts reads after the handshake settles it.
- The cache would have held the preset BY REFERENCE - a container inside a
message the RX thread decoded and handed to every other listener. So a
submessage is now copied on the way in, and the structural test proves
the copy by mutating the source afterwards rather than trusting it.
- The presence-free-field check needed to learn about invalidates: a plan
that voids its copy has no blind spot because it does not look. That
exemption gets its own test, so a flag that stopped marking cannot leave
the exemption forgiving a real blind spot.
device.events now carries Changed and Invalidated. Forty Grid pushes from
one edit produce one Invalidated, because it fires on the change from
trusted to untrusted. A PresetDirty restating what we already knew produces
nothing.
Eight mutations checked red.
…invented
Hardware said three of this branch's assumptions were wrong. Measured
2026-08-15 on d14e.
1. The connect burst's seed RecallPreset DOES set 'reason'. Dropping that
field, which the last commit did, meant the burst marked the preset
stale and the first read of device.preset paid for a round trip the
unit had already made. So the protocol layer grows
read_current_preset_push, which hands back the whole reply instead of
the preset inside it. Pure refactor - read_current_preset now returns
.preset from it, same request, same match, same bytes.
2. A recall pushes Grid x8-13, then RecallPreset, then Scene, then
SetlistPosition - and NO PresetDirty. So a recall discards unsaved
edits and says nothing about it, which is why SetlistPosition must
invalidate the dirty entry. That was a guess before; it is measured
now. It also means SetlistPosition arrives about 90 ms AFTER the Grid
pushes that actually say the contents changed, so marking the preset on
it is redundant on a recall and actively wrong in the burst, where
there are no Grid pushes because nothing changed.
3. SetlistPosition{READ} really does answer - 3 ms, request id echoed,
first attempt. Section 9's table said so and nobody had checked. So the
invented subject counter is gone and the loaded slot is its own entry,
read from the unit like everything else. preset.is_current now compares
a fact the unit stated rather than the model's own bookkeeping.
Plus the rule those three needed: a push carrying every field an entry
keeps is the unit's whole answer, so it clears the mark the way a read
does. Without it the burst marks two entries and answers both in full a
millisecond later, and the marks would have stuck. It compares against the
ENTRY's field set rather than the plan's, because an invalidating plan
keeps nothing and would otherwise qualify vacuously - a Grid push would
clear the mark it had just set. There is a test for exactly that.
Verified on the unit: after the burst, reading the preset, the scene, the
dirty flag and the loaded slot sends the unit NOTHING. The counter that
proves it was itself checked by marking an entry stale and watching the
read appear.
preset.rows[1] to [4], row.slots[1] to [8], and a BlockGrid keyed by cell. Rows 1 and 3 come back as SplittableRow and rows 2 and 4 as a plain Row, so rows[2].create_split() is something an editor rejects rather than something that raises when it runs - on a literal index, which the docstring says rather than overclaiming. A BlockGrid is a BINDING to a scene, not a copy of the grid. preset.blocks follows whichever scene is active; scene.blocks is pinned to its own. Both read one payload, so which device sits where cannot disagree between them - only the scene-varying answers differ, which is the point. Two handles on one cell compare equal and 'is' is not the test, because two bindings of a cell cannot be one object if they are to answer differently about bypass. Handles are dropped whenever the payload underneath changes. The model re-reads the whole preset after every edit, so a handle memoized against the old payload would go on describing the block that used to be in that cell - which is the quiet kind of wrong this library exists to avoid. Iterating a grid yields occupied cells; iterating a row's slots yields all eight including the empty ones. The two answer different questions and the acceptance criteria name both. check_writable() is public on purpose. It is the precondition every write through a grid will run, and this release ships the guard before the writes it guards - editing is M2 - so a caller can ask, and a test can check, without reaching into a private. A test takes the step the error message names and confirms the refusal lifts, so the advice is not something nobody tried. Nine mutations checked red, including pinning a live binding at construction, keeping stale handles after a re-read, and reading bypass from the active scene instead of the binding's.
Preset, Scenes and Scene, wired to Device. device.preset always hands back the preset that is loaded now; hold one across a recall and it reports is_current False rather than quietly describing the preset that used to be there. is_current compares the loaded slot the unit last reported against the one the object was built at - both from the model's copy, so no round trip. has_unsaved_changes is the same. The double in test_preset.py counts every one of the three ways the model can ask the unit for something, and a test at the bottom of the file marks an entry stale and watches the read appear, because 'the model sent nothing' is otherwise a claim about the double. scene.activate() is the model's first write, and it goes through the cache rather than round the side: the copy updates immediately, the unit's echo confirms it in the background, and a write that never reached the unit marks the scene for re-reading and re-raises. That path was built and tested in story #11 and nothing had exercised it until now. Activating the scene that is already active still writes, because the unit is the authority on what is active and skipping would make the model the authority instead. The inactive-scene refusal now has the step it names: a test switches to the scene and confirms the refusal lifts. Eight mutations checked red. One survived the first pass and was a real test weakness, not a real behaviour: preset.blocks is a property, so asking again after the scene changed built a fresh grid and a binding pinned at construction passed. The test now holds the grid across the change.
The acceptance criterion this story exists for. Two accounts of one payload - the wire's numbering and the screen's - asserted against each other cell by cell: every block's row and slot, every row's input and output port, every branch's splitter slot and mixer slot or absence, every scene label, and bypass in all eight scenes. The offline suite checks the same conversions against a fixture. What it cannot check is whether the fixture still looks like what the unit sends, which is the gap this closes. Two tests exist to stop the others passing on nothing. One says out loud whether the loaded preset actually branches, because every split assertion here is vacuous on a serial preset. The other proves the read counter can see a read, by marking an entry stale and watching it appear - without it, 'the model asked the unit for nothing' is a claim about the counter. Ran against the unit: 33 passed, 4 skipped across the whole hardware suite. One skip is new and honest - the loaded preset routes no row into another row, so the missing-lane-output rule had nothing to check here. It is covered offline against a recorded payload that does route that way. The scene-activate test is the only thing here that writes. It restores the scene that was active through the suite's own restore fixture.
ADR-0012 for the decision the hardware session forced: a Grid or SceneLabel push voids the entry's copy and the next read fetches the whole live preset, rather than merging a sparse delta into a nested structure. The alternatives are written down with the reasons they lost, including what merging would actually take, so deferring it is a decision rather than an omission. domain-model section 9 gains three rules and the event surface. Sections 2 and 3 say what is built and, more usefully, name the four things left out and why: the User/Factory preset split (nothing separates them until save() exists in M2), preset.instrument and preset.address (both Directory facts), and which row an output feeds (obvious is not confirmed, and a wrong row is the silent kind). Section 9's table is corrected against the unit rather than against what the table used to claim. The section 2 note about 'exactly one Block object' needed spelling out now that it is code: one object means one CELL. Two bindings hand back two handles, because a single object could not answer bypassed differently for preset.blocks and sceneB.blocks, which is the whole point of a binding. CLAUDE.md records the boundary being a package - its exemption now covers a directory, so the module list is what keeps that from being a hole - the second allowlist for protocol names that are not conversions, Grid's own decision about action, and that submessages are cached by copy. Also removes a guard that guarded nothing. covers_the_whole_entry() read as a careful check and every branch of it was unreachable: an invalidating plan never reaches that code, and a plan keeping nothing cannot match a non-empty field set. The mutation sweep found it by surviving. The check that does the work compares what the MESSAGE carried, which is the stronger question anyway. All 28 mutations across the branch now turn a test red.
architecture.md listed device/translate.py by name, which the earlier STEERING entry had claimed needed no update. It did. Its module map now carries preset.py, grid.py, blocks.py, events.py and errors.py, and translate/ as a package with the note about why the test names the modules inside it. The 'Built:' claims in domain-model.md pointed at the old path too. The change-log entries that mention it are left alone: they record what was true on the day they were written, which is what a change log is for.
Three merge-blockers, all verified by hand before anything changed. CI was red: scripts/check_artifacts.py still named pyquadcortex/device/translate.py, which this branch deleted, and the build job runs that script on every pull request. It now names the package's __init__ and two real converter modules, because a packaging rule that took the directory but dropped its contents would ship a boundary that re-exports names it no longer has. Checked by building a wheel and an sdist and running the gate against them. A hardware test could never pass on the case it existed for. Row defines no equality and preset.rows rebuilds its objects on every access, so 'row not in routed' compared identity across two Rows instances and was always True - the second loop asserted the exact opposite of the first, and it only ever passed by taking the skip. A stale Preset reported a DIFFERENT preset's contents. is_current went False while name returned the new preset's name, and Scene.activate() would switch the new preset's scene, audibly. The docstring promised it would not quietly describe the preset that used to be loaded; it quietly described another one instead. Every read through a non-current Preset now refuses, and so does activate(). is_current and __repr__ still answer, because asking whether an object is good must not raise and a debugger must not either. Guards this branch introduced that were weaker than they read: a wire enum converted to a letter outside the boundary, where neither scan could see it; the boundary module list keyed on the filename, so translate/legacy/ grid.py passed as 'grid' while the arithmetic scan skipped it; and a test that built an empty message, so its assertion held for a plan keeping two fields. Correctness bugs in what this branch added: the delivery thread caught Exception where the transport's own precedent catches BaseException, so a subscriber calling sys.exit() killed it silently; closing from inside a subscriber tried to join its own thread and aborted the shutdown before the USB interface was released; and declaring the recall reset as a plan on dirty and scene meant the model could not tell its own READ reply from a recall, publishing two events saying the unit had changed when only the library had asked. That rule is now declared once, on the entry that knows the loaded slot, and fires only on a real change from a known value. Block equality keyed on the payload's identity, which the model replaces on every re-read, so a block put in a set vanished after any edit and hash() could issue a device read. Keyed on the preset now. The two letter types compared and hashed equal, contradicting their own module header. They refuse each other on == while still hashing as the plain letter, so preset.stomps['E'] keeps working. Not taken: copying the preset payload out of Preset.wire. It defeats the grid's handle memo and makes every block property re-derive the whole grid, so the property says instead that it is the cache's own object and mutating it corrupts the model - the same framing device.client uses. ADR-0013 records the boundary weakening, which the PR body pointed at and which was nowhere. ADR-0012 rewritten in the file's declared shape. 31 mutations red. Offline 1436 passed. Hardware 34 passed, 4 skipped.
Review responseTriage ran read-only and posted nothing, so there are no threads to resolve - this is the The three merge-blockers: all real, all fixed1. The packaging gate named the file this PR deleted. Confirmed: 2. A hardware test could never pass on the case it existed for. Confirmed: 3. A stale Guards this PR introduced that were weaker than they read4. A conversion outside the boundary. Confirmed - taking 5. The module list was keyed on the filename. Confirmed - 6. A test that could not fail. Confirmed: it built an empty message, so any Correctness bugs in what I added9 and 10. Both real. The delivery thread now catches 11. Real, and the fix changed the design rather than patching it. Declaring 7 and 8. Real. Block equality keyed on the payload's identity, which the model replaces 19. Real, and it contradicted the module's own header. 21. Real. Tests that asserted the wrong thing20. Real. It replayed a recall backwards and its name asserted what the measured order 18. Real. Documentation15, 16, 17, 22, 30 all real. Three new places restated a ADR-0012 was rewritten in the file's declared shape with a declared status, and ADR-0013 13, 14, 25, 26, 32 all real and all fixed: the silent retraction now logs, the event What I did not changeNits 23, 24, 27, 28, 29, 31 are fair and I am leaving them for the owner rather than
32's naming collision - |
1 participant
Part of #12. The Directory half is a follow-on story - see the last section.
What this does
This adds the first read-only view of what is loaded on the Quad Cortex.
Before this change, the library could talk to the unit but had no model of the preset
sitting on the grid. Now you can open a connection and ask. You get the preset's name, its
four rows, the eight slots in each row, which amp or pedal is in each slot, where a row
splits into a parallel path and where that path rejoins, what feeds each row and where it
goes out, and all eight scenes with their labels and their bypass states.
Everything reads in the numbers the touchscreen shows. The unit counts rows from 0 and
scenes from 0; a player counts rows from 1 and scenes A to H. Getting that wrong is
dangerous here because it fails silently: an edit aimed at row 2 that lands on row 1 works
perfectly and reads back perfectly, and nothing tells you. So all of that counting lives
behind one wall, and a test reads the source of every other file in the package to prove
none of them do it themselves. That wall used to be a single file. It grew past what one
person can hold in their head, so this PR splits it into a package of six modules by
responsibility.
There is also a new way to be told when something happened. The library keeps a copy of
what the unit is doing, and it normally only goes back and asks for a fresh value when you
read a property. A script that follows the unit closely wants to know sooner. So you can
now subscribe: the library tells you when it noticed a value move, and when it stopped
trusting its own copy of something, and you can go fetch the fresh value yourself.
Three things hardware changed
A session with the unit contradicted assumptions this branch was built on:
explaining why the preset changed. The library was throwing that field away, and its
own rules say an unrecognised field means "re-read everything". So connecting made the
library distrust the very thing it had just been handed.
message to wait for, so the library has to go and ask after every recall.
claimed existed but nobody had tried. It works, and it answers in about three
milliseconds. So a counter this branch had invented to guess at the same thing is gone,
replaced by the unit's own answer.
What is not here
Writing is not in this PR, with one exception: switching scenes. That one is here because
it is audible and needed proving on real hardware. Placing, moving, removing and saving
blocks are all later work, as is the Directory (setlists, addressing a preset as "28C",
recents and favourites).
The Directory was split out for a specific reason, not just size. Asking the unit what is
in a setlist produces a flood of several hundred messages over about fifteen seconds. The
caching layer here is built for questions that get exactly one answer back, and it says so
in as many words. Teaching it to handle a flood is real work, and it should not sit next
to objects that three other pieces of work are waiting on.
Review
A triage pass raised 32 findings, three of them merge-blockers. All three were real and
are fixed: CI was red because the packaging gate still named a file this PR deleted; a
hardware test could never pass on the case it existed for; and a preset held across a
recall reported the new preset's contents while correctly saying it was stale. The
finding-by-finding reply is in the comments, including the two nits left open on purpose
and the one suggestion not taken, with reasoning.
Testing
pytest tests/hardware --hardware): 34 passed, 4 skipped. Three skips arepre-existing; the fourth says out loud that the loaded preset exercises no
row-feeding-a-row routing, which is covered offline against a recorded payload that does.