Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr
- 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 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.
- 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. `device/state.py` does this structurally - an absent field is simply not in the entry's copy - so a property reads through it rather than checking by hand. The exception is a field the schema gives no presence at all, where absent and default are the same bytes: those are declared in the entry's `FieldPlan.no_presence` WITH the recorded evidence for what the default means, and `tests/test_state.py` holds the declaration against the schema. Never assume; declare.
- Never cache an incomplete reply AS IF IT WERE COMPLETE. Per field is the rule: the cache keeps what the unit actually sent and re-reads for what it did not, so a retry recovers the missing half without re-asking for the half already answered. What must never happen is a field the unit never sent being handed back as a value.
- Model state lives in `device/state.py`, and what is tracked is a `StateEntry` in `device/entries.py` - not an attribute a property fills in itself (ADR-0011). Pushes MERGE (an absent field means "not mentioned", never "reset to default"); a read REPLACES, because it is the unit's whole answer. A message that sets any field the entry does not keep - a schema field or a field number the bindings have never heard of - marks that entry for one re-read; there is no "harmless field" category, and adding one is a guess with a table around it. A message type no entry tracks is ignored outright, which is what makes the metronome's tempo stream free.
- A new entry decides for itself what `action` means on the types that feed it. The shared `SCAFFOLDING` skip covers it today because the two tracked types give it no meaning, and that is NOT true of `Grid`, where `action: DELETE` is what removes a block and an `UPDATE` with the same payload does nothing. Never widen `SCAFFOLDING` to make a new entry quiet.
- 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. `Device.close()` closes the state layer first, so a `Device` built by `from_client` stops listening on a connection it never owned.
- `import hid` appears exactly once, lazily, inside `session.open_device()`. Never import `hid` at module scope. A new module that needs it imports it inside the function that opens the device; `tests/test_import_cleanliness.py` walks the whole package and proves it.
- Never gitignore or delete `pyquadcortex/protocol/proto/*_pb2.py` - the generated bindings are committed on purpose (ADR-0001, written before the proto directory was moved). Regenerate only via `scripts/compile_protos.sh`, and bump the `protobuf` pin in `pyproject.toml` in the same commit as regenerated bindings. The `grpcio-tools` floor in the dev extra is part of that same commit: `grpcio-tools` carries its own protoc, so the installed version decides the gencode, and an older one emits older gencode that still imports and quietly walks the pin backwards (ADR-0008). Both directions are now guarded - the script refuses to write a downgrade, and `tests/test_packaging.py` proves the committed gencode equals the pin floor - so trust the failure and fix the cause rather than working around either. Never read the floor off `grpcio-tools` metadata; 1.82.1 declares `protobuf>=7.35.1` and emits 7.35.0. Run the compiler and read the stamp. CI's `build` job runs `scripts/check_artifacts.py`, which proves the bindings are inside the wheel and the sdist.
- New operations follow `docs/architecture.md` "How to add a new operation": register the type, add a thin client method (no HID, no bytes, no sleeps in `protocol/client.py`), add an offline test asserting the exact wire shape, then verify on hardware and update the coverage table in `docs/protocol.md`.
- Grid mutations use the row/column-keyed pattern (`set_param` / `set_bypass`) - never extend the wholesale `write_preset` path.
- Docstrings state their evidence: confirmed on hardware vs inferred from the schema. When you verify something on hardware, record it (docstring + coverage table) in the same change.
- Code in the RX path preserves "the RX thread never dies": wrap every decode, skip unknown types at debug level, reset the reassembly buffer on anything malformed.
- A `Transport.add_listener` listener runs ON the RX thread (ADR-0009). It applies what the push carries, notes what needs re-reading, and returns. It never reads from the device - `request`, `await_broadcast` and `collect` refuse to run on that thread, and that refusal is not to be relaxed for convenience. Anything registering a listener that must see the connect handshake's burst registers it through `protocol.connect(before_handshake=...)`; by the time `connect()` returns, the burst is still seconds away.
- A `Transport.add_listener` listener runs ON the RX thread (ADR-0009). It applies what the push carries, notes what needs re-reading, and returns. It never reads from the device - `request`, `await_broadcast` and `collect` refuse to run on that thread, and that refusal is not to be relaxed for convenience. That binds `device/state.py` as much as the transport: `apply_push` and everything it calls merge and mark, and the caller's thread does the reading. Anything registering a listener that must see the connect handshake's burst registers it through `protocol.connect(before_handshake=...)`; by the time `connect()` returns, the burst is still seconds away.
- Hardware sessions: quit Cortex Control first - it holds the HID interface exclusively.
- Describe the protocol work as documenting the device's protocol as-is (recovered schema, observed traffic). Do not call it "reverse engineering" in docs, comments, commit messages, or issues.
- Changed code under a path listed in `docs/STEERING.md` § Owned Paths? Diff and update STEERING/CLAUDE/ADR in the same PR.
Expand Down
35 changes: 35 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,41 @@ 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 keeps up with the unit on its own

Anything a `Device` tells you is what the unit is doing now, including changes you
make on its touchscreen while your script is running. You do not have to re-read
anything, and nothing you read comes with a "this might be out of date" warning.

It works because the unit says when things change, and the model listens from the
moment it connects. Connecting is also when the unit volunteers most of what it
knows, in one burst, so the model usually has your answer before you ask for it.
Where the unit says nothing - its firmware version, for one - the model asks, once,
the first time you want it.

```python
import pyquadcortex

with pyquadcortex.connect() as device:
print(device.firmware) # asks the unit
print(device.firmware) # free
```

Two things it will not do. It will not hand you a value the unit never sent: a
field the unit left out raises rather than coming back as an empty string, and
asking again can still succeed. And it will not answer at all once you close the
`Device` - what it remembers stopped being true of the unit the moment the
connection went away.

If the unit mentions something the model does not yet understand, the model stops
trusting that part of what it remembers and asks the unit next time you read it.
Slower, and right. `device.state` shows you what it currently holds and what it is
about to re-read.

Presets, the grid and the Directory are not in the cache yet - they arrive with the
surfaces that read them. Nor is reconnecting after the unit sleeps or the cable
comes out; that is still your code's job for now.

### New: listen to everything the unit sends

The unit talks without being asked. Turn a knob on its touchscreen, recall a
Expand Down
21 changes: 21 additions & 0 deletions docs/ADR.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,24 @@ Records are append-only once `Decided` and built upon: a shipped decision is nev
- ADR-0007 keeps its status and its rule. It currently has no instance, which is the healthy state for it.
- Epic #8's dependency on the TEMPO MODE wire path is resolved. It never gated M1; it no longer gates M3.
- A negative result about device traffic now states which question the instrument answered. "The unit does not announce X" and "X is not on the wire" are separate claims and the second needs a READ.

## ADR-0011: A push merges, a read replaces, and anything the cache cannot place forces one read

- **Status:** Decided (2026-08-14)
- **Decision:** The model's cache treats an inbound message and a read answer differently, on purpose. A **push merges**: the fields the model keeps are applied, and everything else it holds is left alone. A **read replaces**: the answer is the unit's whole account of that entry, so a field it does not carry is dropped rather than kept from before. The per-field check that decides whether our copy is still trustworthy is **conservative and has no exemptions**: a message that sets any field the entry does not keep - including a field number the recovered schema has never heard of - marks that entry, and the next read goes to the unit. The only fields skipped are `action` and `request_id`, which belong to the transport and describe no state. A mark is consumed by **exactly one** read, and what makes that safe is a count rather than a hope: a read clears the mark only if nothing arrived for that entry beyond the read's own answer.
- **Context:** `docs/domain-model.md` section 9 gives the rules and the reason for them - "applying the half of a message we understand and silently dropping the rest is the one failure mode that leaves the cache confidently wrong" - and issue #11 restates the check as per FIELD, not per message type. What section 9 does not settle is what happens when the read that resolves a mark carries the same unkept fields that caused it. Answered naively, the entry re-arms its own mark from its own answer and never caches anything again, so every access becomes a round trip and the cache is decorative. The other half of the problem is the opposite failure: clearing the mark unconditionally throws away any push that landed while the read was in flight, and that push is the only record of a change the replacement has just overwritten.
- **Options:**
- **(a) Push merges, read replaces, one read consumes the mark, guarded by a count of what arrived - chosen.** Every message for an entry is counted as the listener handles it. The read path notes the count before it asks and, when the answer comes back, clears the mark only if the count moved by no more than the answer itself. Because the transport notifies listeners before waking the thread that asked (ADR-0009), the answer is already counted by then, and a push that arrives in the microseconds afterwards is counted under the same lock - so there is no window, rather than a small one.
- **(b) Declare, field by field, which changes cannot affect what we hold.** A `Version` reply carries a bootloader version and a MAC address; neither can make the firmware string wrong, so an entry could say so and skip the read. It is also a judgement call per field with nothing to check it against, made by whoever adds the field, and wrong quietly. Section 9 exists because the model does not guess, and this is a guess with a table around it.
- **(c) Tell a read answer from a push by correlating it.** The transport already refuses to promise this: READ replies carry no `request_id` echo, which is recorded in `Transport.request`. Correlating by type and arrival order would be the same count as (a) with more machinery and a worse failure mode.
- **(d) Suppress the listener for an entry while it is being read.** Simple, and it silently drops any genuine push that lands in the read window - which is the exact change the cache most needs to hear about, because a replacement is about to overwrite it.
- **Open Questions:** Whether the conservative rule costs enough reads to be worth refining once an entry is fed by whole preset dumps rather than small keyed pushes. It is measurable rather than arguable: the log names the entry and the field on every forced re-read. Nothing should be refined before that measurement exists.
- **Rationale:** The distinction between merging and replacing is not an implementation detail, it is the difference between the two things the unit says. A push is a delta about what changed; a read is an answer about what is. Merging a delta needs us to understand every field it names, which is why an unrecognised one costs trust; replacing needs no such understanding, which is why the same fields in an answer cost nothing. That is also why one read is enough, and why the alternative that looked simplest - suppressing or ignoring the listener during a read - is the one that loses information. On the conservative rule: the cost of being wrong in the safe direction is one read, logged with the field that caused it. The cost of being wrong in the other direction is a value the caller believes and the unit disagrees with, and the device gives no error for either.
- **Consequences:**
- Unknown field numbers count. The schema here is recovered rather than published (ADR-0010), so a field the unit really sends and the bindings have never heard of is ordinary rather than hypothetical. It decodes into nothing at all, which makes it the quietest way to drop half a message, and the cache notices it by weighing the message rather than by reading it. That costs one message copy per push per entry - cheap for the small keyed pushes an edit produces, and worth measuring before an entry is fed by full preset dumps.
- A field with no wire presence needs recorded evidence before it is kept, because absent and default are the same bytes and there is nothing to check. `PresetDirty.is_dirty` is the first and only one; the evidence is the protocol layer's, watched flipping across a save on hardware. `tests/test_state.py` holds every such declaration against the schema, so a field that does have presence cannot be declared this way.
- An entry with no reader is not an entry. Section 9's table is longer than the registry, and each remaining row arrives with the surface that reads it - otherwise every push mentioning a field it did not keep would mark it for a read nobody had asked for.
- The mark is per entry, not per field. "Exactly that part of the cache" is the entry, which is what a single read replaces.
- A write the unit CONTRADICTS marks the entry too, which section 10 does not ask for. Section 10 asks for a log line, on the reasoning that a disagreement is a bug in our code rather than a stale cache. That is true of the field the unit named, whose value the echo has just put right. It is not true of the other fields in the same write: those went into the cache on our say-so, the echo did not carry them, and the write they belonged to is one the unit has just demonstrated it disagreed with. Leaving them there makes the one path that means "we have a bug" the one that cleans up after itself least, so it marks - which costs one read on a path that should never run.
- `action` and `request_id` are skipped on every entry, and only one of them is really the transport's. `request_id` always is. `action` is not: on `Grid` it is load-bearing state, because an `UPDATE` carrying `hash: 0` is transmitted and ignored while the same payload with `action: DELETE` removes the block. It is skipped today because the two tracked message types give it no meaning, and a `Grid` entry (issue #12) therefore cannot inherit the skip - two pushes with identical payloads and opposite meanings would apply identically and mark nothing. That entry gives `action` its own decision rather than widening the shared set.
- A field the wire gives no presence and the entry does not KEEP is undetectable rather than merely unkept: proto3 writes such a field only when it differs from its default, so a message leaving one at its default carries no bytes for it and no implementation of this check could see it. `tests/test_state.py` asserts no feeding type has one, which turns a limit of the wire into a question about our own code, checkable and checked.
Loading
Loading