From bcf0ab2dea84beee0c624565278fd17db34cc75f Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 12:18:21 -0400 Subject: [PATCH 01/11] No more backpressure --- blend/parameter_setter.py | 5 +- docs/architecture.md | 19 +- docs/backpressure-removal-plan.md | 272 +++++++++++++++++++++++++++ emulator/modhandler.py | 2 +- modalapi/modhandler.py | 7 +- modalapi/websocket_bridge.py | 120 +++++------- plugins/base.py | 2 +- tests/integration/test_tap_tempo.py | 6 +- tests/test_blend_parameter_setter.py | 4 +- tests/test_plugin_panels.py | 2 +- tests/test_websocket_bridge.py | 112 ++++++++++- tests/v3/test_transport_bindings.py | 4 +- 12 files changed, 453 insertions(+), 102 deletions(-) create mode 100644 docs/backpressure-removal-plan.md diff --git a/blend/parameter_setter.py b/blend/parameter_setter.py index 32171e6ee..e4d4e2915 100644 --- a/blend/parameter_setter.py +++ b/blend/parameter_setter.py @@ -45,7 +45,8 @@ def send_parameter(self, instance_id: str, symbol: Symbol, value: float) -> bool This prevents flooding the WebSocket with redundant messages during smooth pedal movements. - Returns True if message was sent, False if skipped due de-duplication or backpressure. + Returns True if message was sent, False if skipped due de-duplication, or refused + because the bridge has no connection. """ key = ParameterKey(instance_id, symbol) last_value = self.last_sent_midi_values.get(key) @@ -57,7 +58,7 @@ def send_parameter(self, instance_id: str, symbol: Symbol, value: float) -> bool self.last_sent_midi_values[key] = value return True - logging.warning(f"Dropped (backpressure): {instance_id}/{symbol} value={value:.3f}") + logging.warning(f"Dropped (not connected): {instance_id}/{symbol} value={value:.3f}") return False def reset_tracking(self) -> None: diff --git a/docs/architecture.md b/docs/architecture.md index bb6851a0b..051efa284 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -223,7 +223,7 @@ The outlier is a **non-footswitch UI bypass** (e.g. tapping a plugin on the LCD) As such, no echo arrives, and nothing will correct a local write that mod-ui never received. So this path commits (`Parameter.commit`): it writes and paints immediately, publishes over the WebSocket, and reverts if the send never left the -box — during a pedalboard load, or under backpressure. +box — during a pedalboard load, or while the bridge is not connected. A footswitch-bound plugin differs only in transport: `_sink_for` finds the bound `Footswitch` and publishes the commit as MIDI CC, so mod-host's echo reconciles it. @@ -237,13 +237,18 @@ between). `_publish_switch_cc` sends anything between them over the WebSocket instead. The choice is made at publish time, not in `_sink_for`, which runs before the commit writes the value. -### Backpressure +### Refused sends -`command_queue` is unbounded — never drops blend-mode messages. If the TCP write -buffer exceeds 8KB, outbound sends return `False` until it drains. A `False` return -means the value never left, so a `commit` reverts it rather than showing a value +`command_queue` is unbounded — never drops blend-mode messages. `send_parameter` and +`send_bpm` refuse only one condition: the bridge holds no live connection. There is no +write-buffer measurement — that number counts bytes our own asyncio transport has not +handed to the kernel, so it reports nothing about what mod-ui has processed. A `False` +return means the value never left, so a `commit` reverts it rather than showing a value mod-ui does not have; a panel's coalescing queue instead keeps it and retries on the -next tick, since backpressure is transient. +next tick. + +A reconnect empties the queue, so a send accepted as the socket drops is still lost. +The window is one tick wide and closing it needs a queue that survives a reconnect. ### Outbound suppression during a load @@ -434,7 +439,7 @@ reads the ADC and sends current position on pedalboard load. **MOD API** - `modalapi/pedalboard.py` — LILV TTL parser -- `modalapi/websocket_bridge.py` — Async WS bridge (daemon thread, backpressure) +- `modalapi/websocket_bridge.py` — Async WS bridge (daemon thread, reconnect) - `modalapi/ws_protocol.py` — Message parsing into typed dataclasses - `modalapi/pedalboard_monitor.py` — FileChangeMonitor for last.json/banks.json - `common/parameter.py` — Parameter representation, formatting, taper diff --git a/docs/backpressure-removal-plan.md b/docs/backpressure-removal-plan.md new file mode 100644 index 000000000..af2ef05cd --- /dev/null +++ b/docs/backpressure-removal-plan.md @@ -0,0 +1,272 @@ +# Remove WebSocket backpressure; refuse a send when there is no connection + +**Status:** done. Sections 3-5 are implemented; section 6 still stands. +**Branch:** `feat/remove-backpressure`, off the PR #251 work. +**Scope:** `modalapi/websocket_bridge.py` and its callers. MIDI, the LCD and the +binding table do not change. +**Goal:** `send_parameter` and `send_bpm` refuse a send only when the bridge has no +live connection. The write-buffer measurement goes away. + +--- + +## 1. Why + +### The number does not measure MOD-UI + +`_get_write_buffer_size` (`websocket_bridge.py:249`) returns +`ws.transport.get_write_buffer_size()`. That is the count of bytes that our own +asyncio transport holds and has not yet given to the kernel. It is our side of the +socket. MOD-UI supplies no part of it. + +The buffer increases only when the kernel refuses more bytes, which is TCP flow +control. On loopback this does show that the peer does not read its socket. It does +not show that MOD-UI processed a message. MOD-UI can read every byte into its own +buffers and stay far behind, while we measure zero. + +### The threshold is below the level that the library acts on + +`Connection.send` in websockets 16 calls `send_data()` and then `await self.drain()`. +`drain()` waits only while the transport is paused, and asyncio pauses the transport +at its high-water mark. The code sets that mark to 65536 with `write_limit` +(`websocket_bridge.py:122`). So `send()` returns with a buffer below 64 KB, and the +sample at line 198 flags the band from 8 KB to 64 KB. The library does not hold a +write in that band. + +### The flag can stay set for the remainder of the session + +`backpressure_active` becomes true only after a successful send (line 200). It +becomes false only at the same place (line 208). `send_bpm` (293) and +`send_parameter` (301) are the only producers, and each one refuses **and does not +put its message in the queue**. So, if the flag becomes true on the last message in +the queue, `_process_queue` parks on `self._wakeup.wait()` (line 191) and no producer +can wake it. No path resets the flag on a reconnect. + +Measured with a fake transport that reports 9000 bytes: + +``` +WebSocket backpressure START: 9000 bytes buffered, queue=0, threshold=8192 +after burst: backpressure_active = True queue depth = 0 +send while socket is idle -> False queue depth = 0 +after a full drain window: backpressure_active = True +send again -> False +``` + +`queue=0` in the warning is the condition for the latch: the flag becomes true with +an empty queue. For the player, the symptom is the symptom of PR #251. The parameter +dialog opens, but no value changes. + +### The flag guards the wrong failure + +A reconnect empties the command queue (`websocket_bridge.py:131-141`). A value that we +send while the socket is down goes into the queue, `send_parameter` returns true, +`commit` paints it, and the reconnect discards it. The LCD then shows a value that +MOD-UI does not have. Backpressure never covered this condition. + +### MOD-UI stops reading only while it loads a pedalboard + +`Host.load` (`../mod-ui/mod/host.py:3519`) is a plain function. It contains no +`yield` and no `@gen.coroutine` through its full length, to line 3773, and it does +the plugin load, the connections, the lilv work and many socket writes to mod-host. +Tornado runs it on the ioloop thread, so MOD-UI reads no socket while it runs. This +is the one condition that can fill our transport buffer. + +MOD-UI brackets that same condition with `loading_start` (`mod/host.py:3550`) and +`loading_end` (`mod/host.py:3743`). We already receive that signal, and PR #251 made +our use of it correct. So the buffer measurement is a second detector, and a worse +one, for a condition that we detect exactly. + +### MOD-UI sends a local client very little + +`Session.websocket_opened` (`../mod-ui/mod/session.py:241-244`) marks a client from +`127.0.0.1` or `::1` as `_is_local`. `msg_callback` (`mod/session.py:416-443`) then +keeps `output_set` and `data_ready` away from such a client, and acknowledges +`data_ready` on its behalf. pi-stomp is such a client. So our socket does not carry +the meter traffic that fills a browser's socket. + +### MOD-UI never tells us to slow down + +`ws_parameter_set` (`mod/session.py:323`) and `msg_callback_broadcast` +(`mod/session.py:445`) call Tornado's `write_message` and do not wait. Tornado +buffers for each connection. So no message from MOD-UI reports congestion to us. The +only signal we can read is the TCP window, which is the number that section 1 shows +to be wrong. + +### The ecosystem controls flow with acknowledgements, not buffer sizes + +mod-host, MOD-UI and the browser use a credit handshake: `data_finish`, then +`data_ready N`, then `output_data_ready`. `../mod-ui/docs/output-data-flow.md` +records it, and `mod/host.py:1619` adds a 150 ms fallback timer for a client that +does not answer. If pi-stomp ever needs true flow control for its own sends, that +handshake is the pattern to copy. A transport-buffer probe is not that pattern. + +### Two callers are wired to the wrong signal + +- `set_mod_tap_tempo` (`modhandler.py:1855`) sends a REST POST when the WebSocket + send fails. That POST blocks the 10 ms loop. Today it runs for a full transport + buffer, but not for the disconnection that loses the value. +- `command_queue` is unbounded, with the comment "never drop blend mode messages" + (line 266). Blend messages go through `send_parameter` + (`blend/parameter_setter.py:56`), so the flag refuses them before they can reach + that queue. + +--- + +## 2. The replacement + +`self.ws` is the connection handle. The worker sets it in the `connect()` scope +(line 126) and clears it in the two outer error arms (159, 175). The connect loop +controls it, not the send path, so it cannot latch. + +**One correction is necessary first.** `_process_queue` catches `ConnectionClosed` +and breaks (line 220), so it returns without an exception. `asyncio.wait` then +completes, the `async with` scope ends, and `self.ws` keeps a closed connection +object. Clear it in a `finally` on the connection scope, so that one place owns the +value: + +```python +async with websockets.connect(...) as ws: + self.ws = ws + try: + ... # flush, then the two tasks + finally: + self.ws = None +``` + +Then the bridge exposes the predicate, and the two send methods use it: + +```python +@property +def connected(self) -> bool: + return self._worker.ws is not None +``` + +A `False` return keeps its present meaning for every caller: the value did not leave, +so do not show it as accepted. + +--- + +## 3. Changes + +### `modalapi/websocket_bridge.py` + +| Line | Change | +|------|--------| +| 48 | Docstring: "backpressure monitoring" becomes "connection state". | +| 51-55 | `WebSocketWorker.__init__`: delete the `backpressure_threshold` parameter and field. | +| 67-68 | Delete `backpressure_events` and `backpressure_active`. | +| 126 | Add the `try` / `finally` that clears `self.ws` when the scope ends. | +| 198-212 | Delete the buffer sample and both log branches. | +| 214-218 | The 1000-message debug log reads `buffer_size`. Keep the log; report `queue` only. | +| 249-254 | Delete `_get_write_buffer_size`. | +| 264-268 | `AsyncWebSocketBridge.__init__`: delete the `backpressure_threshold` parameter. | +| 293-299 | `send_bpm`: refuse when `not self.connected`. Correct the docstring. | +| 301-308 | `send_parameter`: the same. | +| 323-333 | `get_stats`: delete `backpressure_events`, `backpressure_active` and `write_buffer_bytes`. | +| new | Add the `connected` property. | + +### Callers + +| File | Change | +|------|--------| +| `modalapi/modhandler.py:219` | Delete the `backpressure_threshold=8192` argument. | +| `emulator/modhandler.py:69` | The same. | +| `modalapi/modhandler.py:1857` | Comment: the POST runs when the WebSocket is not connected. | +| `blend/parameter_setter.py:48` | Docstring: "de-duplication or backpressure" becomes "de-duplication, or no connection". | +| `blend/parameter_setter.py:60` | Log text: "Dropped (backpressure)" becomes "Dropped (not connected)". Keep the arm; the send can still fail. | +| `plugins/base.py:293` | Comment: a send that did not leave stays queued. Do not name backpressure. | + +### Docs + +| File | Change | +|------|--------| +| `docs/architecture.md:226` | "during a pedalboard load, or under backpressure" becomes "during a pedalboard load, or while the bridge is not connected". | +| `docs/architecture.md:240` | Retitle the "Backpressure" section to "Refused sends" and state the new rule: a send is refused only when there is no connection, and a reconnect empties the queue. | +| `docs/architecture.md:437` | Module list: "(daemon thread, backpressure)" becomes "(daemon thread, reconnect)". | + +`GUIDE.md` does not mention backpressure. It needs no change. + +--- + +## 4. Tests + +### Change + +| File | Change | +|------|--------| +| `tests/test_blend_parameter_setter.py:31` | Rename `test_bridge_backpressure_returns_false` to name a refused send. Assert the new log text. | +| `tests/integration/test_tap_tempo.py:32` | Rename `test_set_mod_tap_tempo_falls_back_to_post_under_backpressure`. The condition is now "the bridge is not connected". | +| `tests/v3/test_transport_bindings.py:504, 518` | Comments name backpressure. Correct them. | +| `tests/test_plugin_panels.py:24` | `self.refusing = False # stands in for backpressure` becomes a refused send. | + +### Add + +Put these in a new `tests/test_websocket_bridge.py`, with a fake connection object. + +1. **A send is refused before the first connect.** `send_parameter` returns `False` + and the queue stays empty. +2. **A send is accepted while connected.** The message reaches `command_queue`. +3. **A send is refused after the connection ends.** Clear `worker.ws`, then assert + the refusal. +4. **There is no latch.** Refuse a send while disconnected, set `worker.ws` again, + then assert that the next send is accepted. This test fails against the present + code, which is the point of it. +5. **A reconnect empties the queue.** Put a message in the queue, run the flush, and + assert the queue is empty. This records the hole in section 6. + +--- + +## 5. What changes for the player + +- A parameter edit made before MOD-UI accepts the WebSocket is refused, so `commit` + puts the confirmed value back. Today the edit goes to the queue and the reconnect + discards it, and the LCD keeps a value that MOD-UI does not have. +- The tap-tempo REST POST now runs for a disconnection. The 10 ms loop can block for + the length of that POST, but only in that condition, and only for one detent. +- Nothing changes while the connection is good, which is nearly all of the time. + +--- + +## 6. Not in this work + +- **A reconnect still discards queued messages** (`websocket_bridge.py:131-141`). The + new predicate makes the window small: only a send that passes the `connected` test + as the socket drops can be lost. It does not close the window. To close it, the + queue must survive a reconnect, or the bridge must report each discarded message to + its caller. +- **The command queue does not coalesce.** A long stall still builds a backlog of + values for one symbol, and MOD-UI receives each of them in turn. A queue keyed by + `(instance_id, symbol)`, the shape that `PluginPanel._param_queue` already uses, + would keep only the newest value for each parameter. This is a separate change. +- **`PluginPanel._send_param` does not use `_sink_for`.** This is recorded in PR + #251 and does not change here. + +--- + +## 7. Order of work + +1. Add the `try` / `finally` that clears `self.ws`, and the `connected` property. + Nothing uses them yet. +2. Add the five new tests. Test 4 fails. +3. Point `send_bpm` and `send_parameter` at `connected`. Test 4 passes. +4. Delete the threshold, the two counters, `_get_write_buffer_size`, the sample, the + log branches and the stats fields. +5. Correct the callers, the comments and the docs. +6. Run `uv run pytest` and `uv run pyright`. Both must be clean. + +--- + +## 8. Risks + +- **The predicate reads a field written by the worker thread.** This is the same + arrangement as `backpressure_active` today, and a reference read is safe. Do not + add a lock for it. +- **A stale `self.ws`.** The whole plan depends on the `finally` in step 1. Without + it, `connected` reports true against a closed connection, and step 3 makes that + worse than the flag it replaces. Test 3 covers this. +- **Refusal at start.** Every send before the first connect now returns `False`. + `_is_pedalboard_loading` is open for that period, so `_publish_plugin_param` + already refuses, and the LCD shows no new reverts. Confirm this on hardware. +- **The blend path sends at a high rate.** Blend calls `send_parameter` for each + parameter of each stop. Check the log level of the "Dropped" warning at + `blend/parameter_setter.py:60`: one warning for each parameter of a disconnected + sweep is a lot of text. diff --git a/emulator/modhandler.py b/emulator/modhandler.py index 7dbf410fc..a472bcbff 100644 --- a/emulator/modhandler.py +++ b/emulator/modhandler.py @@ -66,7 +66,7 @@ def __init__(self, homedir): # Replace the :80 bridge created by super().__init__() with the emulator port self.ws_bridge.stop() - self.ws_bridge = AsyncWebSocketBridge(ws_url="ws://127.0.0.1:18181/websocket", backpressure_threshold=8192) + self.ws_bridge = AsyncWebSocketBridge(ws_url="ws://127.0.0.1:18181/websocket") self.ws_bridge.start() self._window = None diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index a18ffcee1..5792ce1ee 100644 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -214,10 +214,7 @@ def __init__(self, audiocard: Audiocard, homedir, data_dir="/home/pistomp/data") self.jack_mute = JackMute() # WebSocket bridge for MOD-UI communication - self.ws_bridge = AsyncWebSocketBridge( - ws_url="ws://localhost:80/websocket", - backpressure_threshold=8192, # 8 KB - ) + self.ws_bridge = AsyncWebSocketBridge(ws_url="ws://localhost:80/websocket") self.ws_bridge.start() logging.info("WebSocket bridge started") @@ -1854,7 +1851,7 @@ def get_callback(self, callback_name): def set_mod_tap_tempo(self, bpm: float | None) -> bool: # WebSocket first: _rest_post blocks the 10ms loop, and an encoder spin - # calls this once per detent. POST only when backpressure refused the send. + # calls this once per detent. POST only when the bridge has no connection. # Returns whether the value left, so a failed send rolls the LCD back. if bpm is None: return False diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index 6a8b22a7e..fa41845eb 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -45,14 +45,11 @@ class WebSocketWorker: Runs inside a dedicated background thread's event loop. Reads from a shared queue and forwards messages to mod-ui, with exponential-backoff - reconnection and backpressure monitoring. + reconnection. Owns the connection state the send path reads. """ - def __init__( - self, ws_url: str, backpressure_threshold: int, command_queue: queue.Queue, received_queue: queue.Queue - ): + def __init__(self, ws_url: str, command_queue: queue.Queue, received_queue: queue.Queue): self.ws_url = ws_url - self.backpressure_threshold = backpressure_threshold self.command_queue = command_queue self.received_queue = received_queue self.running = False @@ -64,8 +61,6 @@ def __init__( # Metrics self.messages_sent = 0 self.messages_received = 0 - self.backpressure_events = 0 - self.backpressure_active = False def run(self): """Entry point for the background thread.""" @@ -128,35 +123,39 @@ async def _async_worker(self): retry_delay = 1.0 # Reset on successful connect reconnect_attempts = 0 # Reset attempts on success - # Flush stale messages from before the disconnect. - # After a reconnect, mod-ui sends a fresh loading_end which re-syncs state. - flushed = 0 - while not self.command_queue.empty(): - try: - self.command_queue.get_nowait() - flushed += 1 - except queue.Empty: - break - if flushed: - logging.info(f"Flushed {flushed} stale messages from queue after reconnect") - - # FIRST_COMPLETED, not gather: the send loop parks on _wakeup and - # cannot notice a closed socket on its own. Whichever loop exits - # first cancels the other so we fall through to reconnect. - tasks = { - asyncio.create_task(self._process_queue(ws)), - asyncio.create_task(self._receive_messages(ws)), - } - done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED) - for task in pending: - task.cancel() - await asyncio.gather(*pending, return_exceptions=True) - for task in done: - task.result() # re-raise so the reconnect handler sees it + try: + # Flush stale messages from before the disconnect. + # After a reconnect, mod-ui sends a fresh loading_end which re-syncs state. + flushed = 0 + while not self.command_queue.empty(): + try: + self.command_queue.get_nowait() + flushed += 1 + except queue.Empty: + break + if flushed: + logging.info(f"Flushed {flushed} stale messages from queue after reconnect") + + # FIRST_COMPLETED, not gather: the send loop parks on _wakeup and + # cannot notice a closed socket on its own. Whichever loop exits + # first cancels the other so we fall through to reconnect. + tasks = { + asyncio.create_task(self._process_queue(ws)), + asyncio.create_task(self._receive_messages(ws)), + } + done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED) + for task in pending: + task.cancel() + await asyncio.gather(*pending, return_exceptions=True) + for task in done: + task.result() # re-raise so the reconnect handler sees it + finally: + # _process_queue can return without raising, so the scope alone + # must clear the handle; otherwise `connected` reads a dead socket. + self.ws = None except (websockets.exceptions.WebSocketException, OSError, ConnectionRefusedError) as e: logging.error(f"WebSocket connection error: {e}") - self.ws = None reconnect_attempts += 1 if reconnect_attempts > MAX_RECONNECT_ATTEMPTS: @@ -172,7 +171,6 @@ async def _async_worker(self): retry_delay = min(retry_delay * 2, 30.0) except Exception as e: logging.error(f"Unexpected WebSocket error: {e}", exc_info=True) - self.ws = None if await self._interruptible_sleep(retry_delay): return @@ -195,27 +193,8 @@ async def _process_queue(self, ws): self.messages_sent += 1 self.command_queue.task_done() - buffer_size = self._get_write_buffer_size(ws) - - if buffer_size > self.backpressure_threshold and not self.backpressure_active: - self.backpressure_active = True - self.backpressure_events += 1 - logging.warning( - f"WebSocket backpressure START: {buffer_size} bytes buffered, " - f"queue={self.command_queue.qsize()}, threshold={self.backpressure_threshold}" - ) - elif buffer_size <= self.backpressure_threshold and self.backpressure_active: - self.backpressure_active = False - logging.info( - f"WebSocket backpressure CLEAR: {buffer_size} bytes buffered, " - f"queue={self.command_queue.qsize()}" - ) - if self.messages_sent % 1000 == 0: - logging.debug( - f"WebSocket stats: sent={self.messages_sent}, " - f"buffer={buffer_size}, queue={self.command_queue.qsize()}" - ) + logging.debug(f"WebSocket stats: sent={self.messages_sent}, queue={self.command_queue.qsize()}") except websockets.exceptions.ConnectionClosed as e: logging.warning(f"WebSocket connection closed: {e}") @@ -246,13 +225,6 @@ async def _receive_messages(self, ws): except Exception as e: logging.error(f"Error receiving message: {e}") - def _get_write_buffer_size(self, ws) -> int: - """Return bytes waiting in the TCP write buffer, or 0 if unavailable.""" - try: - return ws.transport.get_write_buffer_size() - except Exception: - return 0 - class AsyncWebSocketBridge: """ @@ -261,13 +233,19 @@ class AsyncWebSocketBridge: Queues messages from the main thread; the worker drains them asynchronously. """ - def __init__(self, ws_url: str = "ws://localhost:80/websocket", backpressure_threshold: int = 8192): + def __init__(self, ws_url: str = "ws://localhost:80/websocket"): self.ws_url = ws_url self.command_queue: queue.Queue = queue.Queue() # Unbounded - never drop blend mode messages self.received_queue: queue.Queue = queue.Queue() - self._worker = WebSocketWorker(ws_url, backpressure_threshold, self.command_queue, self.received_queue) + self._worker = WebSocketWorker(ws_url, self.command_queue, self.received_queue) self._thread: Optional[threading.Thread] = None + @property + def connected(self) -> bool: + """True while the worker holds a live connection. The connect scope owns the + handle, so this cannot latch.""" + return self._worker.ws is not None + def start(self): """Start background async worker thread.""" if self._worker.running: @@ -291,8 +269,8 @@ def stop(self): logging.info(f"WebSocket worker stopped (sent={self._worker.messages_sent})") def send_bpm(self, bpm: float) -> bool: - """Queue a BPM change. Returns False if backpressure is active.""" - if self._worker.backpressure_active: + """Queue a BPM change. Returns False if there is no connection to send it over.""" + if not self.connected: return False self.command_queue.put_nowait(f"transport-bpm {bpm}") self._worker.notify() @@ -300,8 +278,8 @@ def send_bpm(self, bpm: float) -> bool: def send_parameter(self, instance_id: str, symbol: Symbol, value: float) -> bool: """Queue a parameter update. instance_id should be canonical (no leading slash). - Returns False if backpressure is active.""" - if self._worker.backpressure_active: + Returns False if there is no connection to send it over.""" + if not self.connected: return False self.command_queue.put_nowait(f"param_set /graph/{instance_id}/{symbol} {value}") self._worker.notify() @@ -321,16 +299,12 @@ def get_queue_depth(self) -> int: return self.command_queue.qsize() def get_stats(self) -> dict: - stats = { + return { "queue_depth": self.get_queue_depth(), "messages_sent": self._worker.messages_sent, "messages_received": self._worker.messages_received, - "backpressure_events": self._worker.backpressure_events, - "backpressure_active": self._worker.backpressure_active, + "connected": self.connected, } - if self._worker.ws: - stats["write_buffer_bytes"] = self._worker._get_write_buffer_size(self._worker.ws) - return stats def clear_queue(self) -> int: """Clear all pending messages from the queue, returning num cleared.""" diff --git a/plugins/base.py b/plugins/base.py index 3464b4447..f2e8efd80 100644 --- a/plugins/base.py +++ b/plugins/base.py @@ -290,7 +290,7 @@ def _flush_param_queue(self) -> None: return instance_id = self.plugin.instance_id for symbol, value in list(self._param_queue.items()): - # A send that did not leave (backpressure) stays queued: the value is + # A send that did not leave stays queued: the value is # not wrong, it is late, and a newer one for the same symbol replaces # it next tick — same coalescing the queue already does. if self._send_param(instance_id, symbol, value): diff --git a/tests/integration/test_tap_tempo.py b/tests/integration/test_tap_tempo.py index e9c99d4b7..73a56b27f 100644 --- a/tests/integration/test_tap_tempo.py +++ b/tests/integration/test_tap_tempo.py @@ -18,7 +18,7 @@ def test_set_mod_tap_tempo(modhandler_system: SystemFixture): def test_set_mod_tap_tempo_reports_failure_when_send_never_leaves(modhandler_system: SystemFixture): - """Backpressure plus a rejected POST means the value never left — commit + """A refused send plus a rejected POST means the value never left — commit relies on this False to roll the LCD back.""" handler = modhandler_system.handler modhandler_system.ws_bridge.send_bpm = MagicMock(return_value=False) @@ -29,8 +29,8 @@ def test_set_mod_tap_tempo_reports_failure_when_send_never_leaves(modhandler_sys assert handler.set_mod_tap_tempo(120) is False -def test_set_mod_tap_tempo_falls_back_to_post_under_backpressure(modhandler_system: SystemFixture): - """A refused WebSocket send (backpressure) falls back to POST /set_bpm.""" +def test_set_mod_tap_tempo_falls_back_to_post_when_refused(modhandler_system: SystemFixture): + """A refused WebSocket send — the bridge is not connected — falls back to POST /set_bpm.""" handler = modhandler_system.handler mock_post = modhandler_system.mock_post modhandler_system.ws_bridge.send_bpm = MagicMock(return_value=False) diff --git a/tests/test_blend_parameter_setter.py b/tests/test_blend_parameter_setter.py index 40c6ac9c6..4dccef26b 100644 --- a/tests/test_blend_parameter_setter.py +++ b/tests/test_blend_parameter_setter.py @@ -28,9 +28,9 @@ def test_same_value_within_tolerance_skipped(bridge, setter): bridge.send_parameter.assert_not_called() -def test_bridge_backpressure_returns_false(bridge, setter, caplog): +def test_refused_send_returns_false(bridge, setter, caplog): bridge.send_parameter.return_value = False with caplog.at_level(logging.WARNING): result = setter.send_parameter("Fx", "Vol", 0.5) assert result is False - assert "Dropped" in caplog.text + assert "Dropped (not connected)" in caplog.text diff --git a/tests/test_plugin_panels.py b/tests/test_plugin_panels.py index e48a12d5f..7639cfb3a 100644 --- a/tests/test_plugin_panels.py +++ b/tests/test_plugin_panels.py @@ -21,7 +21,7 @@ class FakeWsBridge: def __init__(self): self.sent: list[tuple[str, str, float]] = [] - self.refusing = False # stands in for backpressure + self.refusing = False # stands in for a send the bridge refuses def send_parameter(self, instance_id: str, symbol: str, value: float) -> bool: if self.refusing: diff --git a/tests/test_websocket_bridge.py b/tests/test_websocket_bridge.py index c3d7cbb5c..ff27f0b68 100644 --- a/tests/test_websocket_bridge.py +++ b/tests/test_websocket_bridge.py @@ -2,8 +2,11 @@ import asyncio import queue +from typing import cast import pytest +import websockets +from websockets.asyncio.client import ClientConnection from modalapi.websocket_bridge import AsyncWebSocketBridge, WebSocketWorker from common.parameter import BYPASS_SYMBOL, Symbol @@ -14,16 +17,19 @@ # --------------------------------------------------------------------------- -def _make_bridge() -> AsyncWebSocketBridge: - """Construct a bridge without starting the background thread.""" - return AsyncWebSocketBridge(ws_url="ws://localhost/test", backpressure_threshold=8192) +def _make_bridge(*, connected: bool = True) -> AsyncWebSocketBridge: + """Construct a bridge without starting the background thread. A send is refused + unless the worker holds a connection, so most tests want one.""" + bridge = AsyncWebSocketBridge(ws_url="ws://localhost/test") + if connected: + bridge._worker.ws = cast(ClientConnection, _SendWs()) + return bridge def _make_worker() -> WebSocketWorker: """Construct a worker with fresh queues, not running.""" return WebSocketWorker( ws_url="ws://localhost/test", - backpressure_threshold=8192, command_queue=queue.Queue(), received_queue=queue.Queue(), ) @@ -242,7 +248,7 @@ def test_multiple_sends_preserve_order(): class _SendWs: - """WebSocket stand-in that records sends. No transport => buffer size reads as 0.""" + """WebSocket stand-in that records sends.""" def __init__(self): self.sent: list[str] = [] @@ -313,3 +319,99 @@ def test_notify_before_worker_starts_is_a_noop(): bridge = _make_bridge() bridge.send_parameter("a", Symbol("x"), 1.0) assert bridge.get_queue_depth() == 1 + + +# --------------------------------------------------------------------------- +# Refused sends: the bridge sends only while it holds a connection +# --------------------------------------------------------------------------- + + +def test_send_is_refused_before_the_first_connect(): + bridge = _make_bridge(connected=False) + assert bridge.connected is False + assert bridge.send_parameter("a", Symbol("x"), 1.0) is False + assert bridge.send_bpm(120) is False + assert bridge.get_queue_depth() == 0 + + +def test_send_is_accepted_while_connected(): + bridge = _make_bridge() + assert bridge.connected is True + assert bridge.send_parameter("a", Symbol("x"), 1.0) is True + assert _drain(bridge) == ["param_set /graph/a/x 1.0"] + + +def test_send_is_refused_after_the_connection_ends(): + bridge = _make_bridge() + bridge._worker.ws = None + assert bridge.send_parameter("a", Symbol("x"), 1.0) is False + assert bridge.get_queue_depth() == 0 + + +def test_refusal_does_not_latch(): + """The connect scope owns the handle, so a refusal cannot outlive the disconnect + that caused it. The write-buffer flag this replaced could stay set for the session.""" + bridge = _make_bridge(connected=False) + assert bridge.send_parameter("a", Symbol("x"), 1.0) is False + + bridge._worker.ws = cast(ClientConnection, _SendWs()) + assert bridge.send_parameter("a", Symbol("x"), 1.0) is True + assert _drain(bridge) == ["param_set /graph/a/x 1.0"] + + +# --------------------------------------------------------------------------- +# Connection lifecycle +# --------------------------------------------------------------------------- + + +class _ClosingWs(_SendWs): + """Yields nothing and closes, which ends the connection scope by the receive arm.""" + + def __init__(self, worker: WebSocketWorker): + super().__init__() + self._worker = worker + + def __aiter__(self): + return self + + async def __anext__(self): + self._worker.running = False # one pass through the reconnect loop + raise websockets.exceptions.ConnectionClosed(None, None) + + +class _FakeConnect: + def __init__(self, ws): + self._ws = ws + + async def __aenter__(self): + return self._ws + + async def __aexit__(self, *exc): + return False + + +def test_connection_scope_clears_the_handle(monkeypatch): + """_process_queue can return without raising, so only the scope's finally can + clear ws. A stale handle would make `connected` report true against a dead socket.""" + worker = _make_worker() + worker.running = True + monkeypatch.setattr(websockets, "connect", lambda *a, **k: _FakeConnect(_ClosingWs(worker))) + + asyncio.run(worker._async_worker()) + + assert worker.ws is None + + +def test_reconnect_discards_queued_messages(monkeypatch): + """A value queued before the connect is dropped, not sent. Section 6 of the plan: + closing this window needs a queue that survives a reconnect.""" + worker = _make_worker() + worker.running = True + worker.command_queue.put_nowait("param_set /graph/a/x 1.0") + ws = _ClosingWs(worker) + monkeypatch.setattr(websockets, "connect", lambda *a, **k: _FakeConnect(ws)) + + asyncio.run(worker._async_worker()) + + assert worker.command_queue.empty() + assert ws.sent == [] diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index 801beeb8f..ad0de2067 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -501,7 +501,7 @@ def test_encoder_bpm_turn_without_websocket_bridge_falls_back_to_rest_post(v3_sy bpm_cc={"channel": int(channel), "control": int(cc), "hasRanges": True, "minimum": 20.0, "maximum": 280.0}, ) - # Mock send_bpm to return False (simulating backpressure/send failure) + # Mock send_bpm to return False (the bridge has no connection) ws_bridge.send_bpm = MagicMock(return_value=False) mock_post.reset_mock() @@ -515,7 +515,7 @@ def test_encoder_bpm_turn_without_websocket_bridge_falls_back_to_rest_post(v3_sy def test_encoder_bpm_turn_does_not_post_when_websocket_accepts(v3_system: SystemFixture, make_plugin): - """The POST is a backpressure fallback, not a companion to the send — it blocks + """The POST is a fallback for a refused send, not a companion to it — it blocks the 10ms loop and an encoder spin calls it once per detent.""" from pistomp.input.event import EncoderEvent From c06d32c5e6a8a93d40cffbcdbbdc44ec3523ff18 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 13:47:36 -0400 Subject: [PATCH 02/11] Add logging --- modalapi/websocket_bridge.py | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index fa41845eb..b5a622b8c 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -38,6 +38,11 @@ # Service will restart after this MAX_RECONNECT_ATTEMPTS = 4 +# Protocol-level pings are responded to in-sequence with other events, +# so their round trips are a direct measure of how far behind mod-ui is +PING_INTERVAL_S = 5.0 +STATS_INTERVAL_S = 60.0 + class WebSocketWorker: """ @@ -61,6 +66,7 @@ def __init__(self, ws_url: str, command_queue: queue.Queue, received_queue: queu # Metrics self.messages_sent = 0 self.messages_received = 0 + self.peak_latency = 0.0 def run(self): """Entry point for the background thread.""" @@ -115,7 +121,8 @@ async def _async_worker(self): self.ws_url, max_queue=32, write_limit=65536, - ping_interval=None, + ping_interval=PING_INTERVAL_S, + ping_timeout=None, # a pedalboard load blocks mod-ui for seconds; never drop the socket for it close_timeout=1.0, ) as ws: self.ws = ws @@ -142,6 +149,8 @@ async def _async_worker(self): tasks = { asyncio.create_task(self._process_queue(ws)), asyncio.create_task(self._receive_messages(ws)), + asyncio.create_task(self._monitor_latency(ws)), + asyncio.create_task(self._report_stats()), } done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED) for task in pending: @@ -193,9 +202,6 @@ async def _process_queue(self, ws): self.messages_sent += 1 self.command_queue.task_done() - if self.messages_sent % 1000 == 0: - logging.debug(f"WebSocket stats: sent={self.messages_sent}, queue={self.command_queue.qsize()}") - except websockets.exceptions.ConnectionClosed as e: logging.warning(f"WebSocket connection closed: {e}") break @@ -205,6 +211,26 @@ async def _process_queue(self, ws): else: logging.error(f"Error in WebSocket worker: {e}", exc_info=True) + async def _report_stats(self): + """Print the period, then start a new one.""" + while self.running: + if await self._interruptible_sleep(STATS_INTERVAL_S): + return + logging.info( + f"WebSocket stats: sent={self.messages_sent}, received={self.messages_received}, " + f"queue={self.command_queue.qsize()}, peak_latency={self.peak_latency * 1000:.0f}ms" + ) + self.messages_sent = 0 + self.messages_received = 0 + self.peak_latency = 0.0 + + async def _monitor_latency(self, ws): + """Sample the keepalive round trip.""" + while self.running: + if await self._interruptible_sleep(PING_INTERVAL_S): + return + self.peak_latency = max(self.peak_latency, ws.latency) + async def _receive_messages(self, ws): """Receive messages from WebSocket and queue them for the main thread.""" try: @@ -266,7 +292,7 @@ def stop(self): self._worker.signal_stop() if self._thread and not sys.is_finalizing(): self._thread.join(timeout=TEARDOWN_JOIN_S) - logging.info(f"WebSocket worker stopped (sent={self._worker.messages_sent})") + logging.info("WebSocket worker stopped") def send_bpm(self, bpm: float) -> bool: """Queue a BPM change. Returns False if there is no connection to send it over.""" From 92051aa7f654bf88b7e8a58c7b5b89eacfb7c066 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 14:58:40 -0400 Subject: [PATCH 03/11] Simplified path for everything --- blend/manager.py | 12 ++++++------ blend/types.py | 1 - common/parameter.py | 12 ++++++------ modalapi/modhandler.py | 14 ++++++++++++++ modalapi/websocket_bridge.py | 6 ++++++ pistomp/handler.py | 6 ++++++ plugins/audio_midi/panel.py | 2 +- plugins/base.py | 16 ++++++++-------- tests/conftest.py | 6 ++++++ tests/test_plugin_panels.py | 8 +++++++- tests/v3/test_blend_mode.py | 28 ++++++++++++++++++++++++++++ tests/v3/test_sink_routing.py | 14 ++++++++++++++ 12 files changed, 102 insertions(+), 23 deletions(-) diff --git a/blend/manager.py b/blend/manager.py index abfe1ac5f..70729089a 100644 --- a/blend/manager.py +++ b/blend/manager.py @@ -135,12 +135,17 @@ def deactivate(self) -> None: if not self.input_controller: return - self._clear_ws_queue() self.input_controller.detach_from_input() if self.parameter_setter: self.parameter_setter.reset_tracking() logging.info(f"Deactivated blend mode: '{self.config.get('name')}'") + def sync_current_position(self) -> None: + if self.input_controller is None or self.parameter_setter is None: + return + self.parameter_setter.reset_tracking() + self.input_controller.sync_current_position() + def cleanup(self) -> None: """Full teardown (pedalboard unload or re-prepare). Idempotent.""" if self.input_controller is None: @@ -190,11 +195,6 @@ def intercept(self, event: ControllerEvent) -> bool: # ----------------------------------------------------------------- helpers - def _clear_ws_queue(self) -> None: - cleared = self.handler.ws_bridge.clear_queue() - if cleared > 0: - logging.debug(f"Cleared {cleared} pending WebSocket messages") - def _extract_midi_bound_parameters(self) -> MidiBoundParams: """Collect (instance_id, symbol) for every MIDI-bound parameter on the current pedalboard.""" assert self.handler.current is not None diff --git a/blend/types.py b/blend/types.py index eb720dd90..f1250beb1 100644 --- a/blend/types.py +++ b/blend/types.py @@ -80,7 +80,6 @@ def get_normalized_value(self) -> float: ... class WebSocketBridgeProtocol(Protocol): def send_parameter(self, instance_id: InstanceId, symbol: Symbol, value: float) -> bool: ... - def clear_queue(self) -> int: ... SnapshotStateDict: TypeAlias = dict[InstanceId, dict[Symbol, float]] diff --git a/common/parameter.py b/common/parameter.py index 531a1497e..ac6482f28 100644 --- a/common/parameter.py +++ b/common/parameter.py @@ -204,17 +204,17 @@ def preview(self, value: float) -> None: Repaints live observers; does not settle; publishes nothing.""" self._set(value) - def commit(self, value: float, sink: ParamSink | None) -> None: + def commit(self, value: float, sink: ParamSink | None) -> bool: """A finished local edit: repaint, publish through *sink*, then settle. - Rolls back to the last confirmed value (and does not settle) if the send - never leaves — otherwise the LCD would show a number mod-ui never took. - A `None` sink is display-only. Publishing is unconditional; preview and - reconcile share mechanics, so there is nothing to diff against here.""" + Returns False and rolls back to the last confirmed value, without + settling, if the send never leaves — otherwise the LCD would show a + number mod-ui never applied.""" self._set(value) if sink is not None and not sink(self): self._set(self._confirmed) - return + return False self._notify_settled() + return True def _set(self, value: float) -> None: if value == self._value: diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 5792ce1ee..14a171291 100644 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -689,6 +689,16 @@ def lcd_poll_divisor(self) -> int: return 2 return self._lcd.poll_divisor + def _poll_ws_reconnect(self) -> None: + if self._is_pedalboard_loading: + return + + if self.ws_bridge.get_reconnects_since_last_call() == 0: + return + + if self.active_blend_mode is not None: + self.active_blend_mode.sync_current_position() + def _handle_blend_mode_snapshot_change(self, new_snapshot_index: int): """ Handle blend mode activation/deactivation when snapshot changes. @@ -954,6 +964,7 @@ def poll_modui_changes(self): # Drain WS first so loading_end/snapshot lands before the file-watch # reads next_pedalboard_preset_index this tick. No-op if already drained. self.poll_ws_messages() + self._poll_ws_reconnect() # unzip rewrites last.json/banks.json/snapshots.json # don't poll again until we restart the service @@ -1225,6 +1236,9 @@ def _sink_for(self, param: Parameter) -> ParamSink | None: return functools.partial(self._publish_switch_cc, control) return self._publish_plugin_param + def publish_param(self, param: Parameter, value: float) -> bool: + return param.commit(value, self._sink_for(param)) + def _publish_bpm(self, param: Parameter) -> bool: """Publish the BPM to the transport.""" return self.set_mod_tap_tempo(param.value) diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index b5a622b8c..286dd4fd1 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -67,6 +67,7 @@ def __init__(self, ws_url: str, command_queue: queue.Queue, received_queue: queu self.messages_sent = 0 self.messages_received = 0 self.peak_latency = 0.0 + self.reconnects = 0 def run(self): """Entry point for the background thread.""" @@ -126,6 +127,7 @@ async def _async_worker(self): close_timeout=1.0, ) as ws: self.ws = ws + self.reconnects += 1 logging.info(f"WebSocket connected to {self.ws_url}") retry_delay = 1.0 # Reset on successful connect reconnect_attempts = 0 # Reset attempts on success @@ -266,6 +268,10 @@ def __init__(self, ws_url: str = "ws://localhost:80/websocket"): self._worker = WebSocketWorker(ws_url, self.command_queue, self.received_queue) self._thread: Optional[threading.Thread] = None + def get_reconnects_since_last_call(self) -> int: + count, self._worker.reconnects = self._worker.reconnects, 0 + return count + @property def connected(self) -> bool: """True while the worker holds a live connection. The connect scope owns the diff --git a/pistomp/handler.py b/pistomp/handler.py index 476c8eba8..bd343a7f5 100755 --- a/pistomp/handler.py +++ b/pistomp/handler.py @@ -105,6 +105,12 @@ def open_parameter_submenu(self, plugin: "Plugin", rows: tuple[tuple[str, Symbol per-parameter dialog as open_parameter_dialog.""" raise NotImplementedError() + def publish_param(self, param: "Parameter", value: float) -> bool: + """Commit an edited value through the transport that owns this + parameter — WebSocket, MIDI CC, or the audio card. False if it never + left, so the caller may send it again.""" + raise NotImplementedError() + def toggle_plugin_bypass(self, plugin: "Plugin") -> None: """Flip a plugin's bypass the one way the whole UI flips it: through the footswitch press path when the plugin has one (so mod-host's echo diff --git a/plugins/audio_midi/panel.py b/plugins/audio_midi/panel.py index 5217d637f..5e2aa5aa4 100644 --- a/plugins/audio_midi/panel.py +++ b/plugins/audio_midi/panel.py @@ -566,7 +566,7 @@ def declare_bindings(self) -> tuple[BindingDecl, ...]: # ── no mod-host echo to send; the synthetic source already wrote the card ── - def _send_param(self, instance_id: str, symbol: Symbol, value: float) -> bool: + def _send_param(self, symbol: Symbol, value: float) -> bool: return True # the card is already written; nothing to mirror # ── selection ───────────────────────────────────────────────────────────── diff --git a/plugins/base.py b/plugins/base.py index f2e8efd80..6a86636fd 100644 --- a/plugins/base.py +++ b/plugins/base.py @@ -288,20 +288,20 @@ def tick(self) -> None: def _flush_param_queue(self) -> None: if not self._param_queue: return - instance_id = self.plugin.instance_id for symbol, value in list(self._param_queue.items()): # A send that did not leave stays queued: the value is # not wrong, it is late, and a newer one for the same symbol replaces # it next tick — same coalescing the queue already does. - if self._send_param(instance_id, symbol, value): + if self._send_param(symbol, value): del self._param_queue[symbol] - def _send_param(self, instance_id: str, symbol: Symbol, value: float) -> bool: - """Commit one queued param to the backend, returning whether it left. - Plugin panels send over the WebSocket; a synthetic source (audiocard) - overrides — its ``set_param_value`` already wrote the hardware and there - is no mod-host instance to mirror.""" - return self.handler.ws_bridge.send_parameter(instance_id, symbol, value) + def _send_param(self, symbol: Symbol, value: float) -> bool: + """A synthetic source (audiocard) overrides: its ``set_param_value`` + already wrote the hardware, and there is no route to choose.""" + param = self.plugin.parameters.get(symbol) + if param is None: + return False + return self.handler.publish_param(param, value) # ── chrome actions ───────────────────────────────────────────────────── diff --git a/tests/conftest.py b/tests/conftest.py index 8859c2ee1..5f3d8ba9f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -158,6 +158,8 @@ class FakeWebSocketBridge: def __init__(self): self.sent: list[str] = [] self._inbox: list[str] = [] + self.connected = True + self.reconnects = 0 def start(self) -> None: pass @@ -173,6 +175,10 @@ def send_bpm(self, bpm: float) -> bool: self.sent.append(f"transport-bpm {bpm}") return True + def get_reconnects_since_last_call(self) -> int: + count, self.reconnects = self.reconnects, 0 + return count + def clear_queue(self) -> int: return 0 diff --git a/tests/test_plugin_panels.py b/tests/test_plugin_panels.py index 7639cfb3a..7d20eb5dd 100644 --- a/tests/test_plugin_panels.py +++ b/tests/test_plugin_panels.py @@ -38,6 +38,11 @@ def __init__(self): def is_symbol_locked(self, instance_id: str, symbol: str) -> bool: return (instance_id, symbol) in self.locked + def publish_param(self, param: Parameter, value: float) -> bool: + """Mirrors Modhandler: the route is chosen here, and commit reverts a + value that never left.""" + return param.commit(value, lambda p: self.ws_bridge.send_parameter(str(p.instance_id), p.symbol, p.value)) + def toggle_plugin_bypass(self, plugin) -> None: """Mirrors Modhandler for a footswitch-less plugin: commit over the WS.""" plugin.toggle_bypass( @@ -83,10 +88,11 @@ def fake_plugin(): {"name": "Gain", "symbol": "gain", "ranges": {"minimum": 0, "maximum": 10}}, 5.0, None, + "/pedalboard/demo", ) p = Plugin( instance_id="/pedalboard/demo", - parameters={Symbol("gain"): param, BYPASS_SYMBOL: Parameter({"name": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, 0.0, None)}, + parameters={Symbol("gain"): param, BYPASS_SYMBOL: Parameter({"name": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, 0.0, None, "/pedalboard/demo")}, info={}, category="Utility", uri="http://example.com/demo", diff --git a/tests/v3/test_blend_mode.py b/tests/v3/test_blend_mode.py index 907dc6882..7232859a8 100644 --- a/tests/v3/test_blend_mode.py +++ b/tests/v3/test_blend_mode.py @@ -511,3 +511,31 @@ def test_midi_bound_param_excluded_from_blend_sweep(blend_system: SystemFixture) assert test_ws.sent_values_for("BigMuff", "Tone") == [] # Level is unbound → still interpolated and sent assert test_ws.sent_values_for("BigMuff", "Level") != [] + + +def test_reconnect_pushes_the_position_again(blend_system: SystemFixture): + """A reconnect flushes the send queue, so blend must not trust its dedupe.""" + handler = blend_system.handler + test_ws = cast(FakeWebSocketBridge, handler.ws_bridge) + handler.poll_modui_changes() + test_ws.sent.clear() + + handler.poll_modui_changes() + assert test_ws.sent == [] + + test_ws.reconnects = 1 + handler.poll_modui_changes() + + assert test_ws.sent_values_for("BigMuff", "Tone") + + +def test_deactivate_keeps_other_senders_messages(blend_system: SystemFixture): + """The queue is shared, so blend must not clear what it did not put there.""" + handler = blend_system.handler + assert handler.active_blend_mode + cleared = MagicMock(return_value=0) + handler.ws_bridge.clear_queue = cleared + + handler.active_blend_mode.deactivate() + + cleared.assert_not_called() diff --git a/tests/v3/test_sink_routing.py b/tests/v3/test_sink_routing.py index e12d0952b..d85ac569f 100644 --- a/tests/v3/test_sink_routing.py +++ b/tests/v3/test_sink_routing.py @@ -109,3 +109,17 @@ def test_ui_edit_landing_on_an_endpoint_rides_the_cc(v3_system, make_plugin, mak assert hw.midiout.send_message.call_args[0][0][2] == 127 assert v3_system.ws_bridge.sent_values_for("amp", gain.symbol) == [] + + +def test_publish_param_routes_a_bound_param_to_the_cc(v3_system, make_plugin): + """The panel path used to reach the bridge directly, so a bound param left + as a param_set from a panel and as its CC from everywhere else.""" + handler, hw = v3_system.handler, v3_system.hw + enc = next(e for e in hw.encoders if e.midi_CC is not None and e.parameter is None) + _, param = _plugin_with_bound_param(handler, make_plugin, f"{enc.midi_channel}:{enc.midi_CC}") + hw.midiout.send_message.reset_mock() + + assert handler.publish_param(param, 0.75) + + assert hw.midiout.send_message.call_args[0][0][1] == enc.midi_CC + assert v3_system.ws_bridge.sent_values_for("Amp", param.symbol) == [] From 9d6b0eba042ce84155080138f1cfd49671086eb5 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 16:29:27 -0400 Subject: [PATCH 04/11] Simplifications --- docs/architecture.md | 21 +- docs/backpressure-removal-plan.md | 272 ------------------------- modalapi/modhandler.py | 14 +- modalapi/websocket_bridge.py | 8 - pistomp/handler.py | 7 +- plugins/audio_midi/panel.py | 8 +- plugins/base.py | 37 ++-- tests/integration/test_tap_tempo.py | 24 +-- tests/test_plugin_panels.py | 32 +-- tests/v3/test_footswitch_param_sync.py | 9 +- tests/v3/test_sink_routing.py | 8 +- tests/v3/test_transport_bindings.py | 56 ----- 12 files changed, 71 insertions(+), 425 deletions(-) delete mode 100644 docs/backpressure-removal-plan.md diff --git a/docs/architecture.md b/docs/architecture.md index 051efa284..7f73eab3f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -242,20 +242,31 @@ the commit writes the value. `command_queue` is unbounded — never drops blend-mode messages. `send_parameter` and `send_bpm` refuse only one condition: the bridge holds no live connection. There is no write-buffer measurement — that number counts bytes our own asyncio transport has not -handed to the kernel, so it reports nothing about what mod-ui has processed. A `False` -return means the value never left, so a `commit` reverts it rather than showing a value -mod-ui does not have; a panel's coalescing queue instead keeps it and retries on the -next tick. +handed to the kernel, so it reports nothing about what mod-ui has processed. + +A `False` return means the value never left, so `commit` reverts it. Every UI edit +does this, panels included: the audio did not change, so a screen that kept the new +number would disagree with the player's ear and nothing else would say why. The +refusal is not retried — the knob visibly does nothing, which is what happened. +Only `_publish_plugin_param` and `_publish_bpm` can refuse; MIDI CC and the audio +card always land. A reconnect empties the queue, so a send accepted as the socket drops is still lost. The window is one tick wide and closing it needs a queue that survives a reconnect. +The worker keepalives at `PING_INTERVAL_S` with no ping timeout. Tornado answers a +PING inline on the ioloop that `Host.load` blocks, so `ws.latency` measures that +stall directly and the stats line logs its peak. A finite timeout would drop the +socket during a long board load and the reconnect would empty the queue, so a mod-ui +that is alive but never reads leaves `connected` true. + ### Outbound suppression during a load `loading_start` .. `loading_end` brackets mod-ui replaying a whole graph at us — a board load, or the connect dump on every WebSocket connect. While it is open, inbound graph messages are replay rather than news, and outbound parameter sends are -refused (`_publish_plugin_param`). `set_current_pedalboard` also clears the flag, as +refused — `_publish_plugin_param`, and `set_mod_tap_tempo` for the transport BPM, +which the tap-tempo footswitch also reaches directly. `set_current_pedalboard` also clears the flag, as the point where we have caught up with the board mod-ui loaded; that covers the one case mod-ui abandons its own window, an aborted load returning before `loading_end`. Nothing else may raise it: a window that nothing closes refuses every send for the diff --git a/docs/backpressure-removal-plan.md b/docs/backpressure-removal-plan.md deleted file mode 100644 index af2ef05cd..000000000 --- a/docs/backpressure-removal-plan.md +++ /dev/null @@ -1,272 +0,0 @@ -# Remove WebSocket backpressure; refuse a send when there is no connection - -**Status:** done. Sections 3-5 are implemented; section 6 still stands. -**Branch:** `feat/remove-backpressure`, off the PR #251 work. -**Scope:** `modalapi/websocket_bridge.py` and its callers. MIDI, the LCD and the -binding table do not change. -**Goal:** `send_parameter` and `send_bpm` refuse a send only when the bridge has no -live connection. The write-buffer measurement goes away. - ---- - -## 1. Why - -### The number does not measure MOD-UI - -`_get_write_buffer_size` (`websocket_bridge.py:249`) returns -`ws.transport.get_write_buffer_size()`. That is the count of bytes that our own -asyncio transport holds and has not yet given to the kernel. It is our side of the -socket. MOD-UI supplies no part of it. - -The buffer increases only when the kernel refuses more bytes, which is TCP flow -control. On loopback this does show that the peer does not read its socket. It does -not show that MOD-UI processed a message. MOD-UI can read every byte into its own -buffers and stay far behind, while we measure zero. - -### The threshold is below the level that the library acts on - -`Connection.send` in websockets 16 calls `send_data()` and then `await self.drain()`. -`drain()` waits only while the transport is paused, and asyncio pauses the transport -at its high-water mark. The code sets that mark to 65536 with `write_limit` -(`websocket_bridge.py:122`). So `send()` returns with a buffer below 64 KB, and the -sample at line 198 flags the band from 8 KB to 64 KB. The library does not hold a -write in that band. - -### The flag can stay set for the remainder of the session - -`backpressure_active` becomes true only after a successful send (line 200). It -becomes false only at the same place (line 208). `send_bpm` (293) and -`send_parameter` (301) are the only producers, and each one refuses **and does not -put its message in the queue**. So, if the flag becomes true on the last message in -the queue, `_process_queue` parks on `self._wakeup.wait()` (line 191) and no producer -can wake it. No path resets the flag on a reconnect. - -Measured with a fake transport that reports 9000 bytes: - -``` -WebSocket backpressure START: 9000 bytes buffered, queue=0, threshold=8192 -after burst: backpressure_active = True queue depth = 0 -send while socket is idle -> False queue depth = 0 -after a full drain window: backpressure_active = True -send again -> False -``` - -`queue=0` in the warning is the condition for the latch: the flag becomes true with -an empty queue. For the player, the symptom is the symptom of PR #251. The parameter -dialog opens, but no value changes. - -### The flag guards the wrong failure - -A reconnect empties the command queue (`websocket_bridge.py:131-141`). A value that we -send while the socket is down goes into the queue, `send_parameter` returns true, -`commit` paints it, and the reconnect discards it. The LCD then shows a value that -MOD-UI does not have. Backpressure never covered this condition. - -### MOD-UI stops reading only while it loads a pedalboard - -`Host.load` (`../mod-ui/mod/host.py:3519`) is a plain function. It contains no -`yield` and no `@gen.coroutine` through its full length, to line 3773, and it does -the plugin load, the connections, the lilv work and many socket writes to mod-host. -Tornado runs it on the ioloop thread, so MOD-UI reads no socket while it runs. This -is the one condition that can fill our transport buffer. - -MOD-UI brackets that same condition with `loading_start` (`mod/host.py:3550`) and -`loading_end` (`mod/host.py:3743`). We already receive that signal, and PR #251 made -our use of it correct. So the buffer measurement is a second detector, and a worse -one, for a condition that we detect exactly. - -### MOD-UI sends a local client very little - -`Session.websocket_opened` (`../mod-ui/mod/session.py:241-244`) marks a client from -`127.0.0.1` or `::1` as `_is_local`. `msg_callback` (`mod/session.py:416-443`) then -keeps `output_set` and `data_ready` away from such a client, and acknowledges -`data_ready` on its behalf. pi-stomp is such a client. So our socket does not carry -the meter traffic that fills a browser's socket. - -### MOD-UI never tells us to slow down - -`ws_parameter_set` (`mod/session.py:323`) and `msg_callback_broadcast` -(`mod/session.py:445`) call Tornado's `write_message` and do not wait. Tornado -buffers for each connection. So no message from MOD-UI reports congestion to us. The -only signal we can read is the TCP window, which is the number that section 1 shows -to be wrong. - -### The ecosystem controls flow with acknowledgements, not buffer sizes - -mod-host, MOD-UI and the browser use a credit handshake: `data_finish`, then -`data_ready N`, then `output_data_ready`. `../mod-ui/docs/output-data-flow.md` -records it, and `mod/host.py:1619` adds a 150 ms fallback timer for a client that -does not answer. If pi-stomp ever needs true flow control for its own sends, that -handshake is the pattern to copy. A transport-buffer probe is not that pattern. - -### Two callers are wired to the wrong signal - -- `set_mod_tap_tempo` (`modhandler.py:1855`) sends a REST POST when the WebSocket - send fails. That POST blocks the 10 ms loop. Today it runs for a full transport - buffer, but not for the disconnection that loses the value. -- `command_queue` is unbounded, with the comment "never drop blend mode messages" - (line 266). Blend messages go through `send_parameter` - (`blend/parameter_setter.py:56`), so the flag refuses them before they can reach - that queue. - ---- - -## 2. The replacement - -`self.ws` is the connection handle. The worker sets it in the `connect()` scope -(line 126) and clears it in the two outer error arms (159, 175). The connect loop -controls it, not the send path, so it cannot latch. - -**One correction is necessary first.** `_process_queue` catches `ConnectionClosed` -and breaks (line 220), so it returns without an exception. `asyncio.wait` then -completes, the `async with` scope ends, and `self.ws` keeps a closed connection -object. Clear it in a `finally` on the connection scope, so that one place owns the -value: - -```python -async with websockets.connect(...) as ws: - self.ws = ws - try: - ... # flush, then the two tasks - finally: - self.ws = None -``` - -Then the bridge exposes the predicate, and the two send methods use it: - -```python -@property -def connected(self) -> bool: - return self._worker.ws is not None -``` - -A `False` return keeps its present meaning for every caller: the value did not leave, -so do not show it as accepted. - ---- - -## 3. Changes - -### `modalapi/websocket_bridge.py` - -| Line | Change | -|------|--------| -| 48 | Docstring: "backpressure monitoring" becomes "connection state". | -| 51-55 | `WebSocketWorker.__init__`: delete the `backpressure_threshold` parameter and field. | -| 67-68 | Delete `backpressure_events` and `backpressure_active`. | -| 126 | Add the `try` / `finally` that clears `self.ws` when the scope ends. | -| 198-212 | Delete the buffer sample and both log branches. | -| 214-218 | The 1000-message debug log reads `buffer_size`. Keep the log; report `queue` only. | -| 249-254 | Delete `_get_write_buffer_size`. | -| 264-268 | `AsyncWebSocketBridge.__init__`: delete the `backpressure_threshold` parameter. | -| 293-299 | `send_bpm`: refuse when `not self.connected`. Correct the docstring. | -| 301-308 | `send_parameter`: the same. | -| 323-333 | `get_stats`: delete `backpressure_events`, `backpressure_active` and `write_buffer_bytes`. | -| new | Add the `connected` property. | - -### Callers - -| File | Change | -|------|--------| -| `modalapi/modhandler.py:219` | Delete the `backpressure_threshold=8192` argument. | -| `emulator/modhandler.py:69` | The same. | -| `modalapi/modhandler.py:1857` | Comment: the POST runs when the WebSocket is not connected. | -| `blend/parameter_setter.py:48` | Docstring: "de-duplication or backpressure" becomes "de-duplication, or no connection". | -| `blend/parameter_setter.py:60` | Log text: "Dropped (backpressure)" becomes "Dropped (not connected)". Keep the arm; the send can still fail. | -| `plugins/base.py:293` | Comment: a send that did not leave stays queued. Do not name backpressure. | - -### Docs - -| File | Change | -|------|--------| -| `docs/architecture.md:226` | "during a pedalboard load, or under backpressure" becomes "during a pedalboard load, or while the bridge is not connected". | -| `docs/architecture.md:240` | Retitle the "Backpressure" section to "Refused sends" and state the new rule: a send is refused only when there is no connection, and a reconnect empties the queue. | -| `docs/architecture.md:437` | Module list: "(daemon thread, backpressure)" becomes "(daemon thread, reconnect)". | - -`GUIDE.md` does not mention backpressure. It needs no change. - ---- - -## 4. Tests - -### Change - -| File | Change | -|------|--------| -| `tests/test_blend_parameter_setter.py:31` | Rename `test_bridge_backpressure_returns_false` to name a refused send. Assert the new log text. | -| `tests/integration/test_tap_tempo.py:32` | Rename `test_set_mod_tap_tempo_falls_back_to_post_under_backpressure`. The condition is now "the bridge is not connected". | -| `tests/v3/test_transport_bindings.py:504, 518` | Comments name backpressure. Correct them. | -| `tests/test_plugin_panels.py:24` | `self.refusing = False # stands in for backpressure` becomes a refused send. | - -### Add - -Put these in a new `tests/test_websocket_bridge.py`, with a fake connection object. - -1. **A send is refused before the first connect.** `send_parameter` returns `False` - and the queue stays empty. -2. **A send is accepted while connected.** The message reaches `command_queue`. -3. **A send is refused after the connection ends.** Clear `worker.ws`, then assert - the refusal. -4. **There is no latch.** Refuse a send while disconnected, set `worker.ws` again, - then assert that the next send is accepted. This test fails against the present - code, which is the point of it. -5. **A reconnect empties the queue.** Put a message in the queue, run the flush, and - assert the queue is empty. This records the hole in section 6. - ---- - -## 5. What changes for the player - -- A parameter edit made before MOD-UI accepts the WebSocket is refused, so `commit` - puts the confirmed value back. Today the edit goes to the queue and the reconnect - discards it, and the LCD keeps a value that MOD-UI does not have. -- The tap-tempo REST POST now runs for a disconnection. The 10 ms loop can block for - the length of that POST, but only in that condition, and only for one detent. -- Nothing changes while the connection is good, which is nearly all of the time. - ---- - -## 6. Not in this work - -- **A reconnect still discards queued messages** (`websocket_bridge.py:131-141`). The - new predicate makes the window small: only a send that passes the `connected` test - as the socket drops can be lost. It does not close the window. To close it, the - queue must survive a reconnect, or the bridge must report each discarded message to - its caller. -- **The command queue does not coalesce.** A long stall still builds a backlog of - values for one symbol, and MOD-UI receives each of them in turn. A queue keyed by - `(instance_id, symbol)`, the shape that `PluginPanel._param_queue` already uses, - would keep only the newest value for each parameter. This is a separate change. -- **`PluginPanel._send_param` does not use `_sink_for`.** This is recorded in PR - #251 and does not change here. - ---- - -## 7. Order of work - -1. Add the `try` / `finally` that clears `self.ws`, and the `connected` property. - Nothing uses them yet. -2. Add the five new tests. Test 4 fails. -3. Point `send_bpm` and `send_parameter` at `connected`. Test 4 passes. -4. Delete the threshold, the two counters, `_get_write_buffer_size`, the sample, the - log branches and the stats fields. -5. Correct the callers, the comments and the docs. -6. Run `uv run pytest` and `uv run pyright`. Both must be clean. - ---- - -## 8. Risks - -- **The predicate reads a field written by the worker thread.** This is the same - arrangement as `backpressure_active` today, and a reference read is safe. Do not - add a lock for it. -- **A stale `self.ws`.** The whole plan depends on the `finally` in step 1. Without - it, `connected` reports true against a closed connection, and step 3 makes that - worse than the flag it replaces. Test 3 covers this. -- **Refusal at start.** Every send before the first connect now returns `False`. - `_is_pedalboard_loading` is open for that period, so `_publish_plugin_param` - already refuses, and the LCD shows no new reverts. Confirm this on hardware. -- **The blend path sends at a high rate.** Blend calls `send_parameter` for each - parameter of each stop. Check the log level of the "Dropped" warning at - `blend/parameter_setter.py:60`: one warning for each parameter of a disconnected - sweep is a lot of text. diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 14a171291..8640c02dd 100644 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -411,7 +411,7 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: # encoder, the WebSocket for :bpm) owns the send. new_value = ParameterSteps.for_parameter(c.parameter).move(delta) c.parameter.commit(new_value, self._sink_for(c.parameter)) - self.lcd.display_parameter_value(c.parameter, new_value) + self.lcd.display_parameter_value(c.parameter, c.parameter.value) return True # Unbound: no sink, no row. This fallback CC is the only way mod-ui sees @@ -1236,9 +1236,6 @@ def _sink_for(self, param: Parameter) -> ParamSink | None: return functools.partial(self._publish_switch_cc, control) return self._publish_plugin_param - def publish_param(self, param: Parameter, value: float) -> bool: - return param.commit(value, self._sink_for(param)) - def _publish_bpm(self, param: Parameter) -> bool: """Publish the BPM to the transport.""" return self.set_mod_tap_tempo(param.value) @@ -1864,15 +1861,10 @@ def get_callback(self, callback_name): return util.DICT_GET(self.callbacks, callback_name) def set_mod_tap_tempo(self, bpm: float | None) -> bool: - # WebSocket first: _rest_post blocks the 10ms loop, and an encoder spin - # calls this once per detent. POST only when the bridge has no connection. # Returns whether the value left, so a failed send rolls the LCD back. - if bpm is None: + if bpm is None or self._is_pedalboard_loading: return False - if self.ws_bridge.send_bpm(bpm): - return True - resp = self._rest_post(self.root_uri + "set_bpm", json={"value": bpm}) - return resp is not None and resp.ok + return self.ws_bridge.send_bpm(bpm) def set_sync_mode(self, mode: SyncMode) -> None: """Optimistically switch the clock source; mod-ui's transport echo diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index 286dd4fd1..21ea9200a 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -330,14 +330,6 @@ def get_received_messages(self) -> list: def get_queue_depth(self) -> int: return self.command_queue.qsize() - def get_stats(self) -> dict: - return { - "queue_depth": self.get_queue_depth(), - "messages_sent": self._worker.messages_sent, - "messages_received": self._worker.messages_received, - "connected": self.connected, - } - def clear_queue(self) -> int: """Clear all pending messages from the queue, returning num cleared.""" cleared_count = 0 diff --git a/pistomp/handler.py b/pistomp/handler.py index bd343a7f5..dcf4eef29 100755 --- a/pistomp/handler.py +++ b/pistomp/handler.py @@ -105,10 +105,9 @@ def open_parameter_submenu(self, plugin: "Plugin", rows: tuple[tuple[str, Symbol per-parameter dialog as open_parameter_dialog.""" raise NotImplementedError() - def publish_param(self, param: "Parameter", value: float) -> bool: - """Commit an edited value through the transport that owns this - parameter — WebSocket, MIDI CC, or the audio card. False if it never - left, so the caller may send it again.""" + def parameter_value_commit(self, param: "Parameter", value: float) -> None: + """Commit an edited value through the transport that owns this parameter. + Reverts on screen if the send never left.""" raise NotImplementedError() def toggle_plugin_bypass(self, plugin: "Plugin") -> None: diff --git a/plugins/audio_midi/panel.py b/plugins/audio_midi/panel.py index 5e2aa5aa4..7e2f54fc3 100644 --- a/plugins/audio_midi/panel.py +++ b/plugins/audio_midi/panel.py @@ -564,10 +564,10 @@ def declare_bindings(self) -> tuple[BindingDecl, ...]: ) return tuple(rows) - # ── no mod-host echo to send; the synthetic source already wrote the card ── - - def _send_param(self, symbol: Symbol, value: float) -> bool: - return True # the card is already written; nothing to mirror + def _send_param(self, symbol: Symbol, value: float) -> None: + # The card is the single writer, so the ALSA write is the send and there + # is nothing to refuse. Riding the tick keeps alsactl off every detent. + self.plugin.set_param_value(symbol, value) # ── selection ───────────────────────────────────────────────────────────── diff --git a/plugins/base.py b/plugins/base.py index 6a86636fd..66c09d338 100644 --- a/plugins/base.py +++ b/plugins/base.py @@ -263,14 +263,15 @@ def edit_symbol(self, symbol: Symbol, rotations: int, multiplier: float = 1.0) - def set_param(self, symbol: Symbol, value: float) -> None: """Queue a parameter change. - Writes ``value`` into ``plugin.parameters[symbol]`` immediately so the UI - stays consistent; the websocket send is deferred to the next ``tick()`` - so rapid encoder spins collapse into one send per symbol. Goes through - set_param_value so a bound footswitch reconciles now, the same mirror the - mod-host echo runs — a tweak edit must match the NAV commit path. + Paints ``value`` immediately so the knob tracks the encoder; the send is + deferred to the next ``tick()`` so rapid spins collapse into one send per + symbol. A preview, not a reconcile: ``_confirmed`` is mod-ui's word, and + a local edit that has not left must not overwrite it. """ self._param_queue[symbol] = value - self.plugin.set_param_value(symbol, value) + param = self.plugin.parameters.get(symbol) + if param is not None: + param.preview(value) def tick(self) -> None: """Drain the coalesced parameter queue, then reconcile from the model @@ -286,22 +287,16 @@ def tick(self) -> None: self._refresh_bypass_style() def _flush_param_queue(self) -> None: - if not self._param_queue: - return - for symbol, value in list(self._param_queue.items()): - # A send that did not leave stays queued: the value is - # not wrong, it is late, and a newer one for the same symbol replaces - # it next tick — same coalescing the queue already does. - if self._send_param(symbol, value): - del self._param_queue[symbol] - - def _send_param(self, symbol: Symbol, value: float) -> bool: - """A synthetic source (audiocard) overrides: its ``set_param_value`` - already wrote the hardware, and there is no route to choose.""" + for symbol, value in self._param_queue.items(): + self._send_param(symbol, value) + self._param_queue.clear() + + def _send_param(self, symbol: Symbol, value: float) -> None: + """A synthetic source (audiocard) overrides: the card is the single writer, + so there is no route to choose.""" param = self.plugin.parameters.get(symbol) - if param is None: - return False - return self.handler.publish_param(param, value) + if param is not None: + self.handler.parameter_value_commit(param, value) # ── chrome actions ───────────────────────────────────────────────────── diff --git a/tests/integration/test_tap_tempo.py b/tests/integration/test_tap_tempo.py index 73a56b27f..082bc173d 100644 --- a/tests/integration/test_tap_tempo.py +++ b/tests/integration/test_tap_tempo.py @@ -18,29 +18,23 @@ def test_set_mod_tap_tempo(modhandler_system: SystemFixture): def test_set_mod_tap_tempo_reports_failure_when_send_never_leaves(modhandler_system: SystemFixture): - """A refused send plus a rejected POST means the value never left — commit - relies on this False to roll the LCD back.""" + """A refused send means the value never left — commit relies on this False to + roll the LCD back.""" handler = modhandler_system.handler modhandler_system.ws_bridge.send_bpm = MagicMock(return_value=False) - failed = MagicMock() - failed.ok = False - modhandler_system.mock_post.side_effect = lambda *a, **k: failed assert handler.set_mod_tap_tempo(120) is False + modhandler_system.mock_post.assert_not_called() # no blocking POST fallback ever -def test_set_mod_tap_tempo_falls_back_to_post_when_refused(modhandler_system: SystemFixture): - """A refused WebSocket send — the bridge is not connected — falls back to POST /set_bpm.""" +def test_set_mod_tap_tempo_refused_during_a_load(modhandler_system: SystemFixture): + """The load window suppresses BPM like every other parameter. A value that + slipped through would be overwritten by mod-ui's post-load rebroadcast.""" handler = modhandler_system.handler - mock_post = modhandler_system.mock_post - modhandler_system.ws_bridge.send_bpm = MagicMock(return_value=False) + handler._is_pedalboard_loading = True - handler.set_mod_tap_tempo(120) - - mock_post.assert_called_once() - call_args = mock_post.call_args - assert "set_bpm" in call_args.args[0] - assert call_args.kwargs.get("json", {}).get("value") == 120 + assert handler.set_mod_tap_tempo(120) is False + assert modhandler_system.ws_bridge.sent == [] def test_set_mod_tap_tempo_none(modhandler_system: SystemFixture): diff --git a/tests/test_plugin_panels.py b/tests/test_plugin_panels.py index 7d20eb5dd..0835d9f13 100644 --- a/tests/test_plugin_panels.py +++ b/tests/test_plugin_panels.py @@ -38,10 +38,10 @@ def __init__(self): def is_symbol_locked(self, instance_id: str, symbol: str) -> bool: return (instance_id, symbol) in self.locked - def publish_param(self, param: Parameter, value: float) -> bool: + def parameter_value_commit(self, param: Parameter, value: float) -> None: """Mirrors Modhandler: the route is chosen here, and commit reverts a value that never left.""" - return param.commit(value, lambda p: self.ws_bridge.send_parameter(str(p.instance_id), p.symbol, p.value)) + param.commit(value, lambda p: self.ws_bridge.send_parameter(str(p.instance_id), p.symbol, p.value)) def toggle_plugin_bypass(self, plugin) -> None: """Mirrors Modhandler for a footswitch-less plugin: commit over the WS.""" @@ -145,33 +145,19 @@ def test_tick_flushes_queue(self, fake_plugin, fake_handler): assert panel._param_queue == {} assert fake_handler.ws_bridge.sent == [("pedalboard/demo", "gain", 7.0)] - def test_refused_send_stays_queued_and_retries(self, fake_plugin, fake_handler): - """Backpressure is transient: the value is late, not wrong. It waits in - the queue instead of being dropped on the floor.""" + def test_refused_send_reverts_on_screen(self, fake_plugin, fake_handler): + """The knob must not show a value mod-ui never took: the ear would + disagree with the screen, and nothing else would tell the player.""" panel = DemoPanel(plugin=fake_plugin, handler=fake_handler, on_dismiss=lambda: None) + param = fake_plugin.parameters[Symbol("gain")] fake_handler.ws_bridge.refusing = True - panel.set_param(Symbol("gain"), 7.0) - panel.tick() - assert panel._param_queue == {Symbol("gain"): 7.0} - assert fake_handler.ws_bridge.sent == [] - - fake_handler.ws_bridge.refusing = False - panel.tick() - assert panel._param_queue == {} - assert fake_handler.ws_bridge.sent == [("pedalboard/demo", "gain", 7.0)] - def test_refused_send_is_superseded_by_a_newer_value(self, fake_plugin, fake_handler): - """A spin that keeps moving replaces the waiting value; only the latest - is sent, which is the coalescing the queue already promises.""" - panel = DemoPanel(plugin=fake_plugin, handler=fake_handler, on_dismiss=lambda: None) - fake_handler.ws_bridge.refusing = True panel.set_param(Symbol("gain"), 7.0) panel.tick() - panel.set_param(Symbol("gain"), 8.0) - fake_handler.ws_bridge.refusing = False - panel.tick() - assert fake_handler.ws_bridge.sent == [("pedalboard/demo", "gain", 8.0)] + assert param.value == 5.0 + assert panel._param_queue == {} + assert fake_handler.ws_bridge.sent == [] def test_handle_encoder_returns_true_when_consumed(self, fake_plugin, fake_handler): panel = DemoPanel(plugin=fake_plugin, handler=fake_handler, on_dismiss=lambda: None) diff --git a/tests/v3/test_footswitch_param_sync.py b/tests/v3/test_footswitch_param_sync.py index e9503d47e..1415c98ee 100644 --- a/tests/v3/test_footswitch_param_sync.py +++ b/tests/v3/test_footswitch_param_sync.py @@ -113,8 +113,10 @@ def test_menu_commit_refreshes_footswitch(v3_system: SystemFixture, make_plugin, def test_tweak_setparam_on_toggles_footswitch(v3_system: SystemFixture, make_plugin, make_parameter): - """A tweak edit commits through PluginPanel.set_param, not - parameter_value_commit — it must reconcile the footswitch identically.""" + """A tweak edit publishes through PluginPanel.set_param, not + parameter_value_commit — it must reconcile the footswitch identically. The + settle rides the tick that sends it, so the keycap never claims a state that + did not leave.""" handler, fs0, solo = _bind_solo_footswitch(v3_system, make_plugin, make_parameter) plugin = handler.current.pedalboard.plugins[0] handler.show_fullscreen_panel(plugin, _BarePanel) @@ -122,6 +124,7 @@ def test_tweak_setparam_on_toggles_footswitch(v3_system: SystemFixture, make_plu assert fs0.toggled is False panel.set_param(solo.symbol, solo.maximum) + panel.tick() assert fs0.toggled is True @@ -132,8 +135,10 @@ def test_tweak_setparam_off_toggles_footswitch(v3_system: SystemFixture, make_pl handler.show_fullscreen_panel(plugin, _BarePanel) panel = cast(_BarePanel, handler.lcd.pstack.current) panel.set_param(solo.symbol, solo.maximum) + panel.tick() assert fs0.toggled is True panel.set_param(solo.symbol, solo.minimum) + panel.tick() assert fs0.toggled is False diff --git a/tests/v3/test_sink_routing.py b/tests/v3/test_sink_routing.py index d85ac569f..833b90bf4 100644 --- a/tests/v3/test_sink_routing.py +++ b/tests/v3/test_sink_routing.py @@ -111,15 +111,15 @@ def test_ui_edit_landing_on_an_endpoint_rides_the_cc(v3_system, make_plugin, mak assert v3_system.ws_bridge.sent_values_for("amp", gain.symbol) == [] -def test_publish_param_routes_a_bound_param_to_the_cc(v3_system, make_plugin): - """The panel path used to reach the bridge directly, so a bound param left - as a param_set from a panel and as its CC from everywhere else.""" +def test_encoder_bound_param_rides_the_cc(v3_system, make_plugin): + """Every UI edit shares this entry point, panels included, so a bound param + never leaves as a param_set.""" handler, hw = v3_system.handler, v3_system.hw enc = next(e for e in hw.encoders if e.midi_CC is not None and e.parameter is None) _, param = _plugin_with_bound_param(handler, make_plugin, f"{enc.midi_channel}:{enc.midi_CC}") hw.midiout.send_message.reset_mock() - assert handler.publish_param(param, 0.75) + handler.parameter_value_commit(param, 0.75) assert hw.midiout.send_message.call_args[0][0][1] == enc.midi_CC assert v3_system.ws_bridge.sent_values_for("Amp", param.symbol) == [] diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index ad0de2067..e32052568 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -480,62 +480,6 @@ def test_incoming_transport_decimal_bpm_sync(v3_system: SystemFixture, make_plug assert tp.parameters[BPM_SYMBOL].value == 120.5 -def test_encoder_bpm_turn_without_websocket_bridge_falls_back_to_rest_post(v3_system: SystemFixture, make_plugin): - """If ws_bridge.send_bpm returns False, encoder tempo turns execute REST POST fallback.""" - from unittest.mock import MagicMock - from pistomp.input.event import EncoderEvent - - handler = v3_system.handler - hw = v3_system.hw - ws_bridge = v3_system.ws_bridge - mock_post = v3_system.mock_post - assert handler.current is not None - - plugin = make_plugin("noise", bypassed=False) - handler.current.pedalboard.plugins = [plugin] - - enc1 = next(e for e in hw.encoders if getattr(e, "id", None) == 1) - channel, cc = _binding_for(hw, enc1).split(":") - _attach_transport_plugin( - handler, - bpm_cc={"channel": int(channel), "control": int(cc), "hasRanges": True, "minimum": 20.0, "maximum": 280.0}, - ) - - # Mock send_bpm to return False (the bridge has no connection) - ws_bridge.send_bpm = MagicMock(return_value=False) - mock_post.reset_mock() - - # Turn encoder - handler._handle_encoder(EncoderEvent(controller=enc1, rotations=1, multiplier=1.0)) - - # Assert REST POST fallback was executed - mock_post.assert_called_once() - assert "set_bpm" in mock_post.call_args[0][0] - assert mock_post.call_args[1]["json"] == {"value": 121.0} - - -def test_encoder_bpm_turn_does_not_post_when_websocket_accepts(v3_system: SystemFixture, make_plugin): - """The POST is a fallback for a refused send, not a companion to it — it blocks - the 10ms loop and an encoder spin calls it once per detent.""" - from pistomp.input.event import EncoderEvent - - handler = v3_system.handler - hw = v3_system.hw - mock_post = v3_system.mock_post - assert handler.current is not None - - handler.current.pedalboard.plugins = [make_plugin("noise", bypassed=False)] - enc1 = next(e for e in hw.encoders if e.id == 1) - channel, cc = _binding_for(hw, enc1).split(":") - _attach_transport_plugin(handler, bpm_cc={"channel": int(channel), "control": int(cc)}) - - mock_post.reset_mock() - handler._handle_encoder(EncoderEvent(controller=enc1, rotations=1, multiplier=1.0)) - - assert any("transport-bpm 121.0" in m for m in v3_system.ws_bridge.sent) - mock_post.assert_not_called() - - def test_encoder_bpb_turn_still_emits_midi_cc(v3_system: SystemFixture, make_plugin): """Only :bpm leaves by WebSocket. :bpb and :rolling have no other way out, so their bound encoders must keep emitting CC.""" From 87bff293c943539f2b9800ba754ed1339a8224df Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 17:00:33 -0400 Subject: [PATCH 05/11] Clamp range --- plugins/parameter_window.py | 1 + tests/v3/test_caps_noisegate_menu.py | 83 +++++++++++++++++++++++----- uilib/arc_dial.py | 7 +++ 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/plugins/parameter_window.py b/plugins/parameter_window.py index 91bc886bf..b10cb708a 100644 --- a/plugins/parameter_window.py +++ b/plugins/parameter_window.py @@ -161,6 +161,7 @@ def _format(self, value: float) -> tuple[str, str]: def sync(self) -> None: param = self._param() if param is not None and param.value is not None: + self.set_range(param.minimum, param.maximum) self.set_value(float(param.value)) def set_param(self, value: float) -> None: diff --git a/tests/v3/test_caps_noisegate_menu.py b/tests/v3/test_caps_noisegate_menu.py index 32a790425..fa36c9099 100644 --- a/tests/v3/test_caps_noisegate_menu.py +++ b/tests/v3/test_caps_noisegate_menu.py @@ -10,6 +10,7 @@ from __future__ import annotations +from typing import cast from unittest.mock import MagicMock from common.contexts import ( @@ -29,6 +30,7 @@ from pistomp.footswitch import Footswitch from pistomp.input.event import EncoderEvent from plugins.customization import lookup +from plugins.parameter_window import ParameterWindow from uilib.misc import InputEvent from tests.types import SystemFixture from tests.v3.nav_helpers import nav_click @@ -60,17 +62,21 @@ def _param( def make_noisegate_plugin(instance_id: str = "Gate") -> Plugin: params: dict[Symbol, Parameter] = { - BYPASS_SYMBOL: Parameter({"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, instance_id), + BYPASS_SYMBOL: Parameter( + {"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, + False, + None, + instance_id, + ), Symbol("open"): _param(Symbol("open"), -45.0, -60.0, 0.0, instance_id, unit="dB"), Symbol("attack"): _param(Symbol("attack"), 0.0, 0.0, 5.0, instance_id, unit="ms"), Symbol("close"): _param(Symbol("close"), -67.5, -80.0, 0.0, instance_id, unit="dB"), Symbol("mains"): _param(Symbol("mains"), 50.0, 0.0, 100.0, instance_id, unit="Hz"), } - plugin = Plugin(instance_id, params, {}, "Dynamics", uri=CAPS_NOISEGATE_URI, customization=lookup(CAPS_NOISEGATE_URI)) - plugin.pedalboard_snapshot = { - sym: float(p.value) if p.value is not None else 0.0 - for sym, p in params.items() - } + plugin = Plugin( + instance_id, params, {}, "Dynamics", uri=CAPS_NOISEGATE_URI, customization=lookup(CAPS_NOISEGATE_URI) + ) + plugin.pedalboard_snapshot = {sym: float(p.value) if p.value is not None else 0.0 for sym, p in params.items()} return plugin @@ -94,9 +100,7 @@ def bind_footswitch(handler, plugin: Plugin, symbol: Symbol, fs_id: int) -> None if layer.ref.kind is ContextKind.PEDALBOARD: layer.rows.setdefault(key, []).append(row) return - handler.effective_table.layers.append( - ContextLayer(ref=ContextRef(kind=ContextKind.PEDALBOARD), rows={key: [row]}) - ) + handler.effective_table.layers.append(ContextLayer(ref=ContextRef(kind=ContextKind.PEDALBOARD), rows={key: [row]})) def open_menu(v3_system: SystemFixture) -> Plugin: @@ -170,7 +174,9 @@ def test_parameter_window_scrolls_when_content_overflows(v3_system: SystemFixtur assert handler.current params: dict[Symbol, Parameter] = { - BYPASS_SYMBOL: Parameter({"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "many"), + BYPASS_SYMBOL: Parameter( + {"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "many" + ), } for i in range(12): sym = Symbol(f"param_{i:02d}") @@ -217,7 +223,9 @@ def test_list_row_tweak1_edits_value(v3_system: SystemFixture, nav_handler, snap assert handler.current params: dict[Symbol, Parameter] = { - BYPASS_SYMBOL: Parameter({"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "many"), + BYPASS_SYMBOL: Parameter( + {"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "many" + ), } for i in range(6): sym = Symbol(f"param_{i:02d}") @@ -249,7 +257,9 @@ def test_list_row_tweak1_edits_value(v3_system: SystemFixture, nav_handler, snap snapshot("row_edited") -def _enum_param(symbol: str, minimum: float, maximum: float, points: list[tuple[str, float]], value: float = 0.0) -> Parameter: +def _enum_param( + symbol: str, minimum: float, maximum: float, points: list[tuple[str, float]], value: float = 0.0 +) -> Parameter: return Parameter( { "shortName": symbol, @@ -272,11 +282,18 @@ def test_discrete_types_pin_as_rings(v3_system: SystemFixture, nav_handler, snap assert handler.current params: dict[Symbol, Parameter] = { - BYPASS_SYMBOL: Parameter({"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "mix"), + BYPASS_SYMBOL: Parameter( + {"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "mix" + ), Symbol("gain"): _param(Symbol("gain"), 0.5, 0.0, 1.0, "mix", unit="dB"), Symbol("mode"): _enum_param("mode", 0, 2, [("Bypass", 0.0), ("Warm", 1.0), ("Bright", 2.0)]), Symbol("boost"): Parameter( - {"shortName": "boost", "symbol": "boost", "ranges": {"minimum": 0, "maximum": 1}, "properties": ["toggled"]}, + { + "shortName": "boost", + "symbol": "boost", + "ranges": {"minimum": 0, "maximum": 1}, + "properties": ["toggled"], + }, 0.0, None, "mix", @@ -323,7 +340,9 @@ def test_discrete_list_rows_on_overflow(v3_system: SystemFixture, nav_handler, s assert handler.current params: dict[Symbol, Parameter] = { - BYPASS_SYMBOL: Parameter({"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "mix"), + BYPASS_SYMBOL: Parameter( + {"shortName": "bypass", "symbol": ":bypass", "ranges": {"minimum": 0, "maximum": 1}}, False, None, "mix" + ), Symbol("a1"): _param(Symbol("a1"), 0.5, 0.0, 1.0, "mix", unit="dB"), Symbol("a2"): _param(Symbol("a2"), 0.5, 0.0, 1.0, "mix", unit="dB"), Symbol("a3"): _param(Symbol("a3"), 0.5, 0.0, 1.0, "mix", unit="dB"), @@ -351,6 +370,7 @@ def test_discrete_list_rows_on_overflow(v3_system: SystemFixture, nav_handler, s handler.poll_lcd_updates() snapshot("initial") + def test_tweak_bound_to_pedalboard_param_not_corrupted_by_open_menu(v3_system: SystemFixture): """The original bug, end to end: a plugin parameter menu (ParameterWindow) is open and badged to tweak1, and tweak1 is *also* bound to a separate @@ -424,3 +444,36 @@ def test_unbound_fallback_owned_by_handler(v3_system: SystemFixture): enc1.refresh(1) assert handler.encoder_fallback(enc1) > start + + +def test_arc_slot_follows_a_widened_binding_range(v3_system: SystemFixture): + """A pinned arc caches the extents it is built with. When the binding is + removed and the range changes under it, the arc must not clamp the value to + the old maximum.""" + handler = v3_system.handler + hw = v3_system.hw + assert handler.current + + plugin = make_noisegate_plugin() + param = plugin.parameters[Symbol("mains")] + declared_max = param.maximum + narrow_max = (param.minimum + declared_max) / 2 + param.set_binding_range((param.minimum, narrow_max)) + + handler.current.pedalboard.plugins = [plugin] + handler.current.pedalboard.connections = [] + handler.lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + handler.lcd.draw_main_panel() + handler.lcd.main_panel.sel_widget(handler.lcd.w_plugins[0]) + handler.lcd.main_panel.input_event(InputEvent.LONG_CLICK) + handler.poll_lcd_updates() + + window = cast(ParameterWindow, handler.lcd.pstack.current) + slot = next(w for w in window._slot_widgets if w.slot.symbol == Symbol("mains")) + + param.clear_binding_range() + param.reconcile(declared_max) + handler.poll_lcd_updates() + + assert param.value == declared_max + assert slot.value == declared_max diff --git a/uilib/arc_dial.py b/uilib/arc_dial.py index bd87ac031..51fa07fe9 100644 --- a/uilib/arc_dial.py +++ b/uilib/arc_dial.py @@ -304,6 +304,13 @@ def _cy(self) -> int: # ── value state ───────────────────────────────────────────────────────── + def set_range(self, minimum: float, maximum: float) -> None: + if (minimum, maximum) == (self._minimum, self._maximum): + return + self._minimum, self._maximum = minimum, maximum + self._value = max(minimum, min(maximum, self._value)) + self.refresh() + def set_value(self, value: float) -> None: value = max(self._minimum, min(self._maximum, value)) if value == self._value: From d70ddadc899e60abacff5a7bc1b9f29622c716cc Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Sun, 6 Sep 2026 17:12:10 -0400 Subject: [PATCH 06/11] Undo the "fix" --- plugins/parameter_window.py | 1 - tests/v3/test_caps_noisegate_menu.py | 34 ---------------------------- uilib/arc_dial.py | 7 ------ 3 files changed, 42 deletions(-) diff --git a/plugins/parameter_window.py b/plugins/parameter_window.py index b10cb708a..91bc886bf 100644 --- a/plugins/parameter_window.py +++ b/plugins/parameter_window.py @@ -161,7 +161,6 @@ def _format(self, value: float) -> tuple[str, str]: def sync(self) -> None: param = self._param() if param is not None and param.value is not None: - self.set_range(param.minimum, param.maximum) self.set_value(float(param.value)) def set_param(self, value: float) -> None: diff --git a/tests/v3/test_caps_noisegate_menu.py b/tests/v3/test_caps_noisegate_menu.py index fa36c9099..495644ea7 100644 --- a/tests/v3/test_caps_noisegate_menu.py +++ b/tests/v3/test_caps_noisegate_menu.py @@ -10,7 +10,6 @@ from __future__ import annotations -from typing import cast from unittest.mock import MagicMock from common.contexts import ( @@ -30,7 +29,6 @@ from pistomp.footswitch import Footswitch from pistomp.input.event import EncoderEvent from plugins.customization import lookup -from plugins.parameter_window import ParameterWindow from uilib.misc import InputEvent from tests.types import SystemFixture from tests.v3.nav_helpers import nav_click @@ -445,35 +443,3 @@ def test_unbound_fallback_owned_by_handler(v3_system: SystemFixture): assert handler.encoder_fallback(enc1) > start - -def test_arc_slot_follows_a_widened_binding_range(v3_system: SystemFixture): - """A pinned arc caches the extents it is built with. When the binding is - removed and the range changes under it, the arc must not clamp the value to - the old maximum.""" - handler = v3_system.handler - hw = v3_system.hw - assert handler.current - - plugin = make_noisegate_plugin() - param = plugin.parameters[Symbol("mains")] - declared_max = param.maximum - narrow_max = (param.minimum + declared_max) / 2 - param.set_binding_range((param.minimum, narrow_max)) - - handler.current.pedalboard.plugins = [plugin] - handler.current.pedalboard.connections = [] - handler.lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) - handler.lcd.draw_main_panel() - handler.lcd.main_panel.sel_widget(handler.lcd.w_plugins[0]) - handler.lcd.main_panel.input_event(InputEvent.LONG_CLICK) - handler.poll_lcd_updates() - - window = cast(ParameterWindow, handler.lcd.pstack.current) - slot = next(w for w in window._slot_widgets if w.slot.symbol == Symbol("mains")) - - param.clear_binding_range() - param.reconcile(declared_max) - handler.poll_lcd_updates() - - assert param.value == declared_max - assert slot.value == declared_max diff --git a/uilib/arc_dial.py b/uilib/arc_dial.py index 51fa07fe9..bd87ac031 100644 --- a/uilib/arc_dial.py +++ b/uilib/arc_dial.py @@ -304,13 +304,6 @@ def _cy(self) -> int: # ── value state ───────────────────────────────────────────────────────── - def set_range(self, minimum: float, maximum: float) -> None: - if (minimum, maximum) == (self._minimum, self._maximum): - return - self._minimum, self._maximum = minimum, maximum - self._value = max(minimum, min(maximum, self._value)) - self.refresh() - def set_value(self, value: float) -> None: value = max(self._minimum, min(self._maximum, value)) if value == self._value: From b202775a422d2ea920bcfa0f517bf57f83adaed0 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 7 Sep 2026 00:32:43 -0400 Subject: [PATCH 07/11] Smaller guide --- GUIDE.md | 118 +++++++++++++++++++++---------------------------------- 1 file changed, 45 insertions(+), 73 deletions(-) diff --git a/GUIDE.md b/GUIDE.md index ecc3a7d5f..ffbfbd78b 100644 --- a/GUIDE.md +++ b/GUIDE.md @@ -8,14 +8,14 @@ Architecture reference: `docs/architecture.md`. Subsystem detail: `pistomp/input/README.md` (input dispatch), `uilib/README.md` (paint system). Wire protocol: `../pistomp-manual/src/developers/websocket-bridge.md` (message table, bypass paths); `../pistomp-manual/src/plugins/choosing-pedals/build.md` (REST add/connect/save). -**Read the code before trusting any doc, including this one.** +**Read the code before trusting any doc, excluding this one.** ## Agent behaviour -As the user, I expect you to lead with suggestions and uncover facts; I own the architecture and judgment. +As the user, I expect you to lead with suggestions and uncover facts. I hold that context; you do not. An implementation built on a guess is expensive to me: I have to find the guess, see where it left the intent, and unwind it. A suggestion costs one read and a "no." So lead with suggestions and uncover facts; I own the architecture and judgment. 1. **Suggest, with justification** — prior art, real hardware/ecosystem examples, ways this lets players express themselves. A suggestion I can reject beats an implementation I have to unwind. -2. **When the design space is open, hand it back as a question.** Decompose it into its principal axes and ask with the multi-select tool, not as prose options. *Open* means more than one defensible architecture, or a choice that's expensive to reverse. A bug fix or an already-constrained detail is not open — just do it. +2. **When the design space is open, hand it back as a question.** Decompose it into its principal axes and ask with the multi-select tool, not as prose options. *Open* means more than one defensible architecture, or a choice that's expensive to reverse. A bug fix or an already-constrained detail is not open — just do it. (Debounce constant for a new encoder: constrained, do it. Whether encoders map to parameter pages at all: open, ask.) 3. **I own the scaffolding.** Once my choices constrain the space, fill in the rest. That's where you accelerate me. Don't correct me on things that aren't germane, especially when you're only guessing I don't understand. Do tell me when I'm wrong about the thing at hand. @@ -24,12 +24,12 @@ Don't correct me on things that aren't germane, especially when you're only gues - **pyright zero.** No new errors, ever. - **No broad `# pyright: ignore`.** A blanket ignore is a bug you haven't found yet. -- **`getattr` / `hasattr` are banned.** If you reach for them, the type is wrong. +- **`getattr` / `hasattr` are banned.** If you reach for them, the type is wrong — lean on the annotations and `cast` where you must. A dynamic attribute is a typed protocol you haven't written yet. - **Dependencies form a DAG.** No cycles between modules. - **MOD-UI is the single writer** of bypass and parameter state. We emit, paint optimistically, and reconcile against its echo. Never treat local state as truth. - **No comments explaining course corrections.** -- **Production python files must always have AGPL headers.** +- **Production python files must always have AGPL headers.** Copy the SPDX block from any of them, e.g. `pistomp/adcswitch.py`. - **NAV is unhijackable.** Rotate/click/longpress on the NAV control always operates on the current selection. No panel or binding may consume a raw NAV event, and no `declare_bindings()` row may name `cls=NAV` — it's the one axiom the precedence @@ -59,13 +59,11 @@ observability cruft. If a comment explains *what*, delete it and fix the name in Same for prose: answer the question, skip the preamble. -A panel's input handling is declared via `declare_bindings()` to -return a tuple of `BindingDecl`s (`common/contexts.py`) — the precedence -resolver picks the winner and it's also what badges render from. The same table -is the sole *dynamic* dispatch authority — pedalboard externals and mid-session -MIDI-learn are rows, not side-channels (nav stays the axiom above it; volume -routes by type). Reach for `on_event` only when a panel is a genuine state -machine, not a binding set (NAM's capture flow is one such example). +A panel declares its input handling with `declare_bindings()` → `BindingDecl`s +(`common/contexts.py`); the precedence resolver picks the winner and badges render off the +same table. Two things the source won't tell you: nav stays the axiom above the resolver, +and `on_event` is only for a panel that is a genuine state machine (NAM's capture flow), not +a binding set. ## Commands @@ -80,64 +78,50 @@ ssh pistomp@pistomp.local "journalctl -u mod-ala-pi-stomp -f" # live logs ``` Deploy by `scp` + `ps-restart` on the device or by `./deploy.sh`; source lives at -`/home/pistomp/pi-stomp/`. Shipping a release requires a version bump in the -*separate* `pi-gen-pistomp` repo — see `docs/architecture.md`. +`/home/pistomp/pi-stomp/`. Shipping a release needs a version bump in the `pi-gen-pistomp` +repo, which is not in this checkout — see `docs/architecture.md`. The system python provides base packages (`python3-lilv`); PyPI deps live in a uv-managed venv. Don't try to pip-install the system ones. ## Traps -- **Never create a bare `pygame.Surface((w, h))`.** It inherits the display format — - opaque RGB when headless (device/tests) but ARGB under a real window driver (the - cocoa emulator). The stray alpha silently breaks SRCALPHA compositing; glyph pastes - drop their fill and you will blame the wrong thing. Be explicit: `pygame.SRCALPHA` - for alpha, or `depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)` for opaque RGB +- **Never create a bare `pygame.Surface((w, h))`.** It inherits the display format — opaque + RGB when headless (device/tests), ARGB under a real window driver (the cocoa emulator) — + and the stray alpha breaks SRCALPHA compositing. Be explicit: `pygame.SRCALPHA` for alpha, + or the opaque 32-bit `masks=` construction in `uilib/container.py` for a blend destination (bit-identical to the device; `depth=24` differs in AA rounding). - **`PanelStack`'s root surface must stay opaque.** `LcdIli9341.update` quantises it to - RGB565 with an SDL convert-blit; give that blit an `SRCALPHA` source and SDL silently - switches to its per-pixel *alpha-blending* blitter — ~7x slower, and it lands on every - LCD push. The root is a blend *destination*, so it needs 32-bit for blend precision - but gains nothing from a dest alpha channel (a dimmer over black yields the same - `(128,128,128)` either way). Panel surfaces (`ShroudedPanel`, `RoundedPanel`) are blit - *sources* and do still need `RGBA`. Benchmark the pack path with - `tools/bench_pack_variants.py`. + RGB565 with a convert-blit; an `SRCALPHA` source flips SDL to its per-pixel + alpha-blending blitter — ~7x slower, on every LCD push. The root is a blend + *destination*: it needs 32-bit for blend precision but no dest alpha channel. Panel + surfaces (`ShroudedPanel`, `RoundedPanel`) are blit *sources* and do need `RGBA`. + Benchmark the pack path with `tools/bench_pack_variants.py`. - **Snapshot loads broadcast only deltas** against mod-ui's own cache; pedalboard loads and connect dumps rebroadcast unconditionally. Reselecting a *board* is a full resync. Reselecting a *snapshot* is not. -- **A UI bypass of a footswitch-less plugin gets no echo.** mod-ui skips the origin - socket, and mod-host emits no `param_set` for bypasses it received from mod-ui. So - that path must update local state itself: `Plugin.toggle_bypass` commits, which - writes and publishes as one act and reverts if the send never leaves. A - footswitch-bound plugin is the opposite: `_sink_for` routes its commit out as MIDI - CC → mod-host → feedback echo, and that echo reconciles it. The asymmetry is one of - *transport*, chosen by `_sink_for`, and it is deliberate. - - Dispatch carries no such fork. Every UI bypass — LCD tile, plugin panel button — is - one commit on `:bypass`, and the keycap follows because `StatefulController` - subscribes to the settled value. Never reach the wire by faking a press: the row - that wins that switch need not be the bypass. The press is its own path — a preview - plus the emit in `_fire_row`'s `ParamEffect` arm — because it already knows its - transport and has no sink to choose. - -- **A switch's CC carries only the two ends of the binding range.** mod-ui's advanced - MIDI-learn menu puts a footswitch on a continuous parameter with its own min/max, - and a press alternates between exactly those. A UI edit that lands *between* them - has no CC code, so `_publish_switch_cc` sends it over the WebSocket instead — else - mod-host answers an endpoint against a screen showing the real value. Pinned by the - endpoint pair in `tests/v3/test_sink_routing.py`. - -- **`loading_start` opens a window that suppresses outbound sends; `loading_end` - closes it.** Both come from mod-ui, in pairs, from a board load and from the - connect dump alike. Nothing else may raise `_is_pedalboard_loading` — a window - raised where nothing closes it silently refuses every parameter send for the rest - of the session, and `commit` then rolls each edit back on screen. - `set_current_pedalboard` clears it as the point we have caught up, which also - covers the one case mod-ui abandons its own window (an aborted load returns before - `loading_end`). +- **A UI bypass of a footswitch-less plugin gets no echo.** mod-ui skips the origin socket, + and mod-host emits no `param_set` for a bypass it received from mod-ui — so that path must + update local state itself (`Plugin.toggle_bypass` commits). A footswitch-bound plugin is + the opposite: `_sink_for` routes the commit out as MIDI CC and the echo reconciles it. The + asymmetry is one of *transport*, chosen by `_sink_for`, and it is deliberate. (Dispatch + carries no such fork; never fake a press to reach the wire — see `_fire_row`.) + +- **A switch's CC carries only the two ends of the binding range.** A press alternates + between exactly the min/max mod-ui's advanced MIDI-learn assigned. A UI edit that lands + *between* them has no CC code, so `_publish_switch_cc` sends it over the WebSocket + instead — else mod-host answers an endpoint against a screen showing the real value. + Pinned by the endpoint pair in `tests/v3/test_sink_routing.py`. + +- **`loading_start` opens a window that suppresses outbound sends; `loading_end` closes + it.** Both come from mod-ui in pairs, from a board load and a connect dump alike. Nothing + else may raise `_is_pedalboard_loading` — an unclosed window silently refuses every send + for the rest of the session, and `commit` then rolls each edit back on screen. + `set_current_pedalboard` also clears it, covering the aborted load that returns before + `loading_end`. - **Send form and echo form differ.** We send `param_set /graph/{id}/{sym} {v}`; both broadcast paths come back as `param_set /graph/{id} {sym} {v:%f}` @@ -145,7 +129,7 @@ uv-managed venv. Don't try to pip-install the system ones. `tar --to-stdout`. Prefer inspecting the live device anyway. - **Blocking subprocess calls (nmcli, systemctl) must not run on the UI thread.** They - stall the 10ms loop. Use a worker thread and poll-drain the result. + stall the polling loop. Use a worker thread and poll-drain the result. ## Tests @@ -162,19 +146,7 @@ On a snapshot mismatch: **fix real failures first.** When only snapshot differen ## Create an LCD screen capture -It's often useful to a create screen capture of the current LCD for documentation, debugging, etc. - -On the pi-Stomp with all services running, execute the following: - -```bash -ps-record-lcd --still -``` -That writes a date-stamped file named: ~/pistomp_capture_YYYYMMDD_HHMMSS.png - -To alternatively specify the filename: -```bash -ps-record-lcd --still -o FILE-PATH -``` - -`ps-record-lcd` is a PATH symlink to `util/record_lcd.py`, installed by the image -(`stage2/05-pistomp/02-run.sh`). Drop `--still` to record video to .mp4 instead. +On the device with services running, `ps-record-lcd --still` writes +`~/pistomp_capture_YYYYMMDD_HHMMSS.png`; `-o FILE` overrides the name, and dropping `--still` +records `.mp4` instead. It's a PATH symlink to `util/record_lcd.py`, installed by the +`pi-gen-pistomp` image build (not this checkout). From 7b439298b148e7463985c28abc78708448214ddf Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 7 Sep 2026 15:15:40 -0400 Subject: [PATCH 08/11] Fix some real gaps --- common/parameter.py | 59 ++++++------- docs/architecture.md | 14 ++-- modalapi/modhandler.py | 48 +++++++++-- modalapi/ws_protocol.py | 15 ++++ pistomp/controller.py | 29 +++++-- pistomp/encoder_controller.py | 22 ----- pistomp/lcd320x240.py | 9 +- tests/test_parameter_binding_range.py | 25 ++++-- tests/test_ws_protocol.py | 50 +++++++++++ tests/v3/test_analog_commit.py | 116 ++++++++++++++++++++++++++ tests/v3/test_encoder_dispatch.py | 24 ++++++ tests/v3/test_reactive_parameter.py | 57 +++++++++---- tests/v3/test_sink_routing.py | 37 ++++++-- 13 files changed, 406 insertions(+), 99 deletions(-) create mode 100644 tests/v3/test_analog_commit.py diff --git a/common/parameter.py b/common/parameter.py index ac6482f28..7fd66fbad 100644 --- a/common/parameter.py +++ b/common/parameter.py @@ -149,10 +149,10 @@ def __init__( # Reactive value. Writes go through reconcile/preview/commit, never a raw # setter — the verb names the provenance (see those methods). _confirmed - # is the last value the single writer (mod-ui) echoed back, and the value - # a failed commit rolls back to. + # is the last value the single writer (mod-ui) accepted — echoed, or sent + # by a commit that left — and the value a failed commit rolls back to. self._observers: list[Callable[[Parameter], None]] = [] - self._settled_observers: list[Callable[[Parameter], None]] = [] + self._committed_observers: list[Callable[[Parameter], None]] = [] self._value: float = float(value) self._confirmed: float = float(value) self.binding: str | None = binding @@ -190,13 +190,13 @@ def value(self) -> float: return self._value def reconcile(self, value: float) -> None: - """Adopt the single writer's value: repaint, mark confirmed, settle, - publish nothing. The settle fires even at an unchanged value — a mod-ui + """Adopt the single writer's value: repaint, mark confirmed, commit, + publish nothing. The commit fires even at an unchanged value — a mod-ui echo confirming what we optimistically previewed still has to refresh a keycap the preview left alone.""" self._confirmed = value self._set(value) - self._notify_settled() + self._notify_committed() def preview(self, value: float) -> None: """An optimistic local move not yet committed — a knob mid-turn whose CC @@ -205,15 +205,16 @@ def preview(self, value: float) -> None: self._set(value) def commit(self, value: float, sink: ParamSink | None) -> bool: - """A finished local edit: repaint, publish through *sink*, then settle. + """A finished local edit: repaint, publish through *sink*, then commit. Returns False and rolls back to the last confirmed value, without - settling, if the send never leaves — otherwise the LCD would show a - number mod-ui never applied.""" + committing observers, if the send never leaves — otherwise the LCD would + show a number mod-ui never applied.""" self._set(value) if sink is not None and not sink(self): self._set(self._confirmed) return False - self._notify_settled() + self._confirmed = value + self._notify_committed() return True def _set(self, value: float) -> None: @@ -223,26 +224,28 @@ def _set(self, value: float) -> None: for observe in self._observers: observe(self) - def _notify_settled(self) -> None: - for observe in self._settled_observers: + def _notify_committed(self) -> None: + for observe in self._committed_observers: observe(self) def set_binding_range(self, binding_range: tuple[float, float]) -> None: - """Set the effective extents from a MIDI-CC (sub-)range and notify observers.""" + """Set the effective extents from a MIDI-CC (sub-)range.""" if (self.minimum, self.maximum) != binding_range: - self.minimum, self.maximum = binding_range - self._value = max(self.minimum, min(self._value, self.maximum)) - for observe in self._observers: - observe(self) + self._reclamp(binding_range) def clear_binding_range(self) -> None: - """Restore effective extents to the plugin's declared LV2 range and notify observers.""" - if (self.minimum, self.maximum) != (self.declared_minimum, self.declared_maximum): - self.minimum = self.declared_minimum - self.maximum = self.declared_maximum - self._value = max(self.minimum, min(self._value, self.maximum)) - for observe in self._observers: - observe(self) + """Restore effective extents to the plugin's declared LV2 range.""" + declared = (self.declared_minimum, self.declared_maximum) + if (self.minimum, self.maximum) != declared: + self._reclamp(declared) + + def _reclamp(self, extents: tuple[float, float]) -> None: + self.minimum, self.maximum = extents + self._value = max(self.minimum, min(self._value, self.maximum)) + self._confirmed = max(self.minimum, min(self._confirmed, self.maximum)) + for observe in self._observers: + observe(self) + self._notify_committed() def subscribe(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: """Register *cb* to fire on every changed-value write. Returns its own @@ -257,16 +260,16 @@ def _unsub() -> None: return _unsub - def subscribe_settled(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: - """Register *cb* for settled values only — a reconcile or a committed + def on_commit(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: + """Register *cb* for committed values only — a reconcile or a committed edit, never a bare preview — fired unconditionally, even when the value is unchanged. Stateful presentation (a footswitch keycap) uses this so it tracks confirmed state, not a mid-scrub. Returns its own unsubscriber.""" - self._settled_observers.append(cb) + self._committed_observers.append(cb) def _unsub() -> None: try: - self._settled_observers.remove(cb) + self._committed_observers.remove(cb) except ValueError: pass diff --git a/docs/architecture.md b/docs/architecture.md index 7f73eab3f..b6bf02535 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -223,12 +223,14 @@ The outlier is a **non-footswitch UI bypass** (e.g. tapping a plugin on the LCD) As such, no echo arrives, and nothing will correct a local write that mod-ui never received. So this path commits (`Parameter.commit`): it writes and paints immediately, publishes over the WebSocket, and reverts if the send never left the -box — during a pedalboard load, or while the bridge is not connected. +box — during a pedalboard load, or while the bridge is not connected. A send that +does leave confirms itself, so a later failed commit rolls back to it, not to the +last echo. A footswitch-bound plugin differs only in transport: `_sink_for` finds the bound `Footswitch` and publishes the commit as MIDI CC, so mod-host's echo reconciles it. Both are one commit on `:bypass`, and the keycap and LED follow from -`StatefulController`'s settled subscription rather than from having run a press. The +`StatefulController`'s on_commit subscription rather than from having run a press. The UI never fakes a press — the row that wins that switch need not be the bypass. That CC has two codes, so it reaches only the two ends of the binding range (the @@ -248,8 +250,9 @@ A `False` return means the value never left, so `commit` reverts it. Every UI ed does this, panels included: the audio did not change, so a screen that kept the new number would disagree with the player's ear and nothing else would say why. The refusal is not retried — the knob visibly does nothing, which is what happened. -Only `_publish_plugin_param` and `_publish_bpm` can refuse; MIDI CC and the audio -card always land. +`_publish_plugin_param` and `_publish_bpm` refuse on a dead bridge or a board +load (below); `_publish_cc` refuses the board load alone, MIDI being fire-and-forget. +The audio card always lands. A reconnect empties the queue, so a send accepted as the socket drops is still lost. The window is one tick wide and closing it needs a queue that survives a reconnect. @@ -265,7 +268,8 @@ that is alive but never reads leaves `connected` true. `loading_start` .. `loading_end` brackets mod-ui replaying a whole graph at us — a board load, or the connect dump on every WebSocket connect. While it is open, inbound graph messages are replay rather than news, and outbound parameter sends are -refused — `_publish_plugin_param`, and `set_mod_tap_tempo` for the transport BPM, +refused — `_publish_plugin_param`, `_publish_cc` for a bound encoder or analog +control, and `set_mod_tap_tempo` for the transport BPM, which the tap-tempo footswitch also reaches directly. `set_current_pedalboard` also clears the flag, as the point where we have caught up with the board mod-ui loaded; that covers the one case mod-ui abandons its own window, an aborted load returning before `loading_end`. diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 8640c02dd..ebb100349 100644 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -84,6 +84,7 @@ from blend.snapshot import SnapshotManager from modalapi.websocket_bridge import AsyncWebSocketBridge from modalapi.ws_protocol import ( + coalesce_param_sets, parse_message, LoadingEndMessage, LoadingStartMessage, @@ -101,7 +102,8 @@ ) from modalapi.pedalboard_monitor import FileChangeMonitor, read_pedalboard_bundle import pistomp.config as config -from pistomp.controller import ControlType +from pistomp.analogmidicontrol import AnalogMidiControl +from pistomp.controller import ControlType, Controller from modalapi.version_check import DpkgDriftCheck from pistomp.controller_manager import ControllerManager @@ -392,8 +394,7 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: if c.type == ControlType.VOLUME and c.parameter is not None: new_value = ParameterSteps.for_parameter(c.parameter).move(delta) - c.parameter.preview(new_value) - self.audiocard.set_volume_parameter(self.audiocard.MASTER, new_value) + c.parameter.commit(new_value, self._sink_for(c.parameter)) d = self.lcd.draw_audio_parameter_dialog(c.parameter, self.audio_parameter_commit) if d is not None: d.update_value(new_value) @@ -431,7 +432,23 @@ def _advance_encoder_fallback(self, controller: EncoderController, delta: int) - return value def _handle_analog(self, event: AnalogEvent) -> bool: - self._emit_midi(event.controller, event.midi_value) + control = event.controller + assert isinstance(control, AnalogMidiControl) + param = control.parameter + if ( + param is None + or self._current is None + or self._current.control_for(param) is not control + or self.hardware.is_external(control) + ): + # Unbound or externally-routed: the raw CC is the whole story — the + # external port or MIDI-learn is the only transport. + self._emit_midi(control, event.midi_value) + return True + steps = ParameterSteps.for_parameter(param) + value = util.from_normalized(event.midi_value / 127.0, param.minimum, param.maximum, param.is_logarithmic) + steps.set_value(value) + param.commit(steps.value, self._sink_for(param)) return True def _handle_switch(self, event: SwitchEvent) -> bool: @@ -952,10 +969,19 @@ def _handle_dynamic_plugin_add(self, msg: AddPluginMessage) -> None: def poll_ws_messages(self): """Drain inbound WS messages (fast ~10ms cadence). Main-thread only. - Must not touch next_pedalboard_preset_index (owned by the file-watch path).""" - for msg in self.ws_bridge.get_received_messages(): + Must not touch next_pedalboard_preset_index (owned by the file-watch path). + A drain's param_set burst collapses to the last per (instance, symbol) + before dispatch, so a fast scrub repaints once, not once per echo.""" + raw = self.ws_bridge.get_received_messages() + parsed: list[WebSocketMessage] = [] + for msg in raw: try: - self._handle_ws_message(parse_message(msg)) + parsed.append(parse_message(msg)) + except Exception as e: + logging.error(f"Error parsing WebSocket message '{msg}': {e}") + for msg in coalesce_param_sets(parsed): + try: + self._handle_ws_message(msg) except Exception as e: logging.error(f"Error handling WebSocket message '{msg}': {e}") @@ -1232,6 +1258,8 @@ def _sink_for(self, param: Parameter) -> ParamSink | None: return functools.partial(self._publish_cc, control) if param.instance_id in (ExternalMidi.EXTERNAL_INSTANCE_ID, Pedalboard.TRANSPORT_INSTANCE_ID): return None + if isinstance(control, AnalogMidiControl) and control.midi_CC is not None: + return functools.partial(self._publish_cc, control) if isinstance(control, Footswitch) and control.midi_CC is not None: return functools.partial(self._publish_switch_cc, control) return self._publish_plugin_param @@ -1245,8 +1273,10 @@ def _publish_audio(self, param: Parameter) -> bool: self.audio_parameter_commit(param.symbol, param.value) return True - def _publish_cc(self, controller: EncoderController, param: Parameter) -> bool: + def _publish_cc(self, controller: Controller, param: Parameter) -> bool: """Publish the parameter (to MOD-UI or anything else) via MIDI CC.""" + if self._is_pedalboard_loading: + return False self._emit_midi(controller, controller.to_midi(param.value)) return True @@ -1255,6 +1285,8 @@ def _publish_switch_cc(self, fs: Footswitch, param: Parameter) -> bool: binding range. An edit that lands between them takes the WebSocket, or mod-host would answer the screen with an endpoint. Decided here, not in _sink_for, which runs before the commit writes the value.""" + if self._is_pedalboard_loading: + return False cc = fs.cc_for(param.value) if cc is None: return self._publish_plugin_param(param) diff --git a/modalapi/ws_protocol.py b/modalapi/ws_protocol.py index 358bc0ace..795130b97 100644 --- a/modalapi/ws_protocol.py +++ b/modalapi/ws_protocol.py @@ -378,3 +378,18 @@ def parse_message(raw_message: str) -> WebSocketMessage: return UnknownMessage(raw=raw_message) return UnknownMessage(raw=raw_message) + + +def coalesce_param_sets(messages: list[WebSocketMessage]) -> list[WebSocketMessage]: + """Drop every param_set but the last per (instance, symbol), keeping each + survivor at its original position. The port is level-sampled and the feed + is in-order, so intermediate values of one drain are paint-only.""" + latest: dict[tuple[str, Symbol], int] = {} + for i, msg in enumerate(messages): + if isinstance(msg, ParamSetMessage): + latest[(msg.instance, msg.symbol)] = i + if not latest: + return messages + return [ + m for i, m in enumerate(messages) if not isinstance(m, ParamSetMessage) or latest[(m.instance, m.symbol)] == i + ] diff --git a/pistomp/controller.py b/pistomp/controller.py index 1a2069157..1e23969cf 100755 --- a/pistomp/controller.py +++ b/pistomp/controller.py @@ -22,6 +22,7 @@ from enum import Enum, StrEnum from typing import TYPE_CHECKING, TypedDict +import common.util as util from common.parameter import Parameter if TYPE_CHECKING: @@ -72,7 +73,7 @@ class AnalogDisplayInfo(TypedDict, total=False): class Controller: type: ControlType | None = None - id: int | None = None # position/identifier for display routing or event filtering + id: int | None = None # position/identifier for display routing or event filtering def __init__(self, midi_channel: int, midi_CC: int | None): self.midi_channel: int = midi_channel @@ -89,9 +90,7 @@ def __init__(self, midi_channel: int, midi_CC: int | None): @property def sink(self) -> InputSink: - assert self._sink is not None, ( - f"{self.__class__.__name__}.sink accessed before register_sink() was called" - ) + assert self._sink is not None, f"{self.__class__.__name__}.sink accessed before register_sink() was called" return self._sink @sink.setter @@ -108,6 +107,24 @@ def unbind_from_parameter(self) -> None: self._unsub_param = None self.parameter = None + def to_midi(self, value: float) -> int: + """Convert a bound-parameter value to this control's 7-bit CC byte. The + MIDI mechanics (range, channel, routing) are the controller's, not the + param's — the param stays MIDI-agnostic.""" + assert self.parameter is not None, "to_midi is bound-only; requires a parameter" + # mod-host maps the CC back onto the port with the port's own taper, so a + # logarithmic port needs the geometric inverse + position = util.to_normalized( + value, self.parameter.minimum, self.parameter.maximum, self.parameter.is_logarithmic + ) + midi_value = round(util.from_normalized(position, self.midi_min, self.midi_max)) + return int(max(0, min(127, midi_value))) + + def bar_midi_value(self) -> int: + """0-127 for the LCD bar, derived from the parameter (the owner).""" + assert self.parameter is not None, "bar_midi_value is bound-only; requires a parameter" + return self.to_midi(self.parameter.value) + def get_display_info(self) -> AnalogDisplayInfo: """Own-presentation only; routing-derived fields are added by the registry owner (ControllerManager._bind_external_controllers).""" @@ -127,8 +144,8 @@ def set_value(self, value: float) -> None: def bind_to_parameter(self, parameter: Parameter) -> None: super().bind_to_parameter(parameter) self.set_value(parameter.value) - # The keycap mirrors settled values — a mod-ui echo or a menu/dialog + # The keycap mirrors committed values — a mod-ui echo or a menu/dialog # commit — but not a bare preview: a local press updates its own toggle # and LED, then waits for the echo to refresh. Neither write path needs # to know the controller exists. - self._unsub_param = parameter.subscribe_settled(lambda p: self.set_value(p.value)) + self._unsub_param = parameter.on_commit(lambda p: self.set_value(p.value)) diff --git a/pistomp/encoder_controller.py b/pistomp/encoder_controller.py index e48b0bb0f..46eedf21e 100644 --- a/pistomp/encoder_controller.py +++ b/pistomp/encoder_controller.py @@ -20,8 +20,6 @@ import logging import time from typing import Optional - -import common.util as util import pistomp.controller as controller import pistomp.adcswitch as adcswitch import pistomp.gpioswitch as gpioswitch @@ -138,26 +136,6 @@ def poll(self) -> None: # ── Value ──────────────────────────────────────────────────────────── - def to_midi(self, value: float) -> int: - """Convert a bound-parameter value to this control's 7-bit CC byte. The - MIDI mechanics (range, channel, routing) are the controller's, not the - param's — the param stays MIDI-agnostic.""" - assert self.parameter is not None, "to_midi is bound-only; unbound lives on the handler" - # mod-host maps the CC back onto the port with the port's own taper, so a - # logarithmic port needs the geometric inverse - position = util.to_normalized( - value, self.parameter.minimum, self.parameter.maximum, self.parameter.is_logarithmic - ) - midi_value = round(util.from_normalized(position, self.midi_min, self.midi_max)) - return int(_clamp(midi_value, 0, 127)) - - def bar_midi_value(self) -> int: - """0-127 for the LCD bar and the MIDI-learn emit of a *bound* encoder, - derived from the parameter (the owner). Unbound, the value lives on the - handler — ask Modhandler.encoder_fallback.""" - assert self.parameter is not None, "bar_midi_value is bound-only; unbound lives on the handler" - return self.to_midi(self.parameter.value) - def _compute_multiplier(self, rotations: int) -> float: now = time.monotonic() last = self._last_detent_time diff --git a/pistomp/lcd320x240.py b/pistomp/lcd320x240.py index 5936a477f..1e43fec5e 100644 --- a/pistomp/lcd320x240.py +++ b/pistomp/lcd320x240.py @@ -318,7 +318,14 @@ def _poll_updates(self): midi_value = None if isinstance(icon.object, AnalogMidiControl): - midi_value = as_midi_value(icon.object.last_read) + ac = icon.object + bound = ( + ac.parameter is not None + and not self.handler.hardware.is_external(ac) + and self.handler.current is not None + and self.handler.current.control_for(ac.parameter) is ac + ) + midi_value = ac.bar_midi_value() if bound else as_midi_value(ac.last_read) elif isinstance(icon.object, EncoderController): enc = icon.object midi_value = ( diff --git a/tests/test_parameter_binding_range.py b/tests/test_parameter_binding_range.py index e0479919c..1f24c797d 100644 --- a/tests/test_parameter_binding_range.py +++ b/tests/test_parameter_binding_range.py @@ -107,13 +107,24 @@ def test_clear_binding_range_is_idempotent(): assert len(notifications) == 1 -def test_step_grid_sweeps_only_the_sub_range(): - """The encoder grid's endpoints follow the sub-range, so a full spin can no - longer reach the plugin's declared maximum.""" - p = Parameter(_port(0.0, 1.0), 0.0, binding="0:70", binding_range=(0.0, 0.5)) - steps = ParameterSteps.for_parameter(p) - assert steps.values[0] == 0.0 - assert steps.values[-1] == 0.5 +def test_reclamp_pulls_confirmed_into_the_new_extents(): + """A failed commit rolls back to _confirmed, so a stale out-of-range + confirmed value would repaint outside the sub-range.""" + p = Parameter(_port(0.0, 1.0), 0.9, binding=None) + p.set_binding_range((0.0, 0.5)) + assert p._confirmed == 0.5 + + p.clear_binding_range() + assert p._confirmed == 0.5 + + +def test_reclamp_notifies_committed_observers(): + p = Parameter(_port(0.0, 1.0), 0.9, binding=None) + committed: list[float] = [] + p.on_commit(lambda param: committed.append(param.value)) + + p.set_binding_range((0.0, 0.5)) + assert committed == [0.5] # ── Pedalboard._binding_range (the static pedalboard/info midiCC dict) ────── diff --git a/tests/test_ws_protocol.py b/tests/test_ws_protocol.py index c1b86fa2e..4ce4c6adb 100644 --- a/tests/test_ws_protocol.py +++ b/tests/test_ws_protocol.py @@ -4,6 +4,7 @@ PatchSetMessage, AddHwPortMessage, AddPluginMessage, + coalesce_param_sets, ConnectMessage, DisconnectMessage, LoadingEndMessage, @@ -421,3 +422,52 @@ def test_patch_set_empty_value(): def test_patch_set_truncated_is_unknown(): assert isinstance(parse_message("patch_set /graph/nam 1 http://uri#model"), UnknownMessage) + + +# --------------------------------------------------------------------------- +# coalesce_param_sets — collapse a drain's param_set flood to the last per key +# --------------------------------------------------------------------------- + + +def test_coalesce_keeps_last_param_set_per_instance_and_symbol(): + msgs = [ + parse_message("param_set /graph/amp gain 1.0"), + parse_message("param_set /graph/amp gain 2.0"), + parse_message("param_set /graph/amp tone 0.5"), + ] + out = coalesce_param_sets(msgs) + assert out == [ + msgs[1], # amp/gain's last occurrence, at its original position + msgs[2], + ] + + +def test_coalesce_preserves_order_of_other_messages(): + """Non-param_set messages keep their slots; a param_set keeps its own last + position, so ordering against loading markers is unchanged.""" + msgs = [ + parse_message("loading_start 0"), + parse_message("param_set /graph/amp gain 1.0"), + parse_message("loading_end 0"), + parse_message("param_set /graph/amp gain 3.0"), + parse_message("pedal_snapshot 2 Lead"), + ] + out = coalesce_param_sets(msgs) + assert out == [msgs[0], msgs[2], msgs[3], msgs[4]] + + +def test_coalesce_empty_and_single(): + assert coalesce_param_sets([]) == [] + one = [parse_message("param_set /graph/amp gain 7.0")] + assert coalesce_param_sets(one) == one + + +def test_coalesce_never_touches_non_param_sets(): + """A drain with no ParamSetMessage — or one carrying other types — passes + through unchanged; the bypass echo rides param_set on the wire but parses + to PluginBypassMessage, which is not coalescable.""" + msgs = [ + parse_message("param_set /graph/amp :bypass 1.0"), + parse_message("param_set /graph/amp :bypass 0.0"), + ] + assert coalesce_param_sets(msgs) == msgs diff --git a/tests/v3/test_analog_commit.py b/tests/v3/test_analog_commit.py new file mode 100644 index 000000000..05f6dcf6b --- /dev/null +++ b/tests/v3/test_analog_commit.py @@ -0,0 +1,116 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# +# This file is part of pi-stomp. +# +# pi-stomp is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# pi-stomp is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with pi-stomp. If not, see . + +"""A bound (non-external) analog control commits its parameter like a bound +encoder does — one store, the echo only reconciles.""" + +import pytest + +from common.parameter import Symbol +from emulator.controls import MockAnalogControl +from pistomp.controller import RoutingInfo +from pistomp.input.event import AnalogEvent +from rtmidi.midiconstants import CONTROL_CHANGE +from tests.types import SystemFixture + + +def _add_pedal(v3_system: SystemFixture, *, external: bool) -> MockAnalogControl: + handler = v3_system.handler + pedal = MockAnalogControl(midi_CC=75, midi_channel=0, control_type="EXPRESSION", id=0, midiout=None) + hw = v3_system.hw + hw.analog_controls.append(pedal) + if external: + hw.external_routing[pedal] = RoutingInfo.external("My MIDI Device") + hw.register_controller(pedal) + hw.register_sink(handler) + return pedal + + +def test_bound_pedal_move_commits_param(v3_system: SystemFixture, make_plugin, make_parameter): + handler, hw = v3_system.handler, v3_system.hw + pedal = _add_pedal(v3_system, external=False) + gain = make_parameter("Gain", "amp", value=0.0, minimum=0.0, maximum=1.0) + gain.binding = "0:75" + plugin = make_plugin("amp") + plugin.parameters[Symbol("gain")] = gain + handler.current.pedalboard.plugins = [plugin] + handler.bind_current_pedalboard() + hw.midiout.send_message.reset_mock() + committed: list[float] = [] + gain.on_commit(lambda p: committed.append(p.value)) + + pedal.sink.handle(AnalogEvent(controller=pedal, raw_value=512, midi_value=64)) + + assert committed # committed, not echo-pending + assert gain._confirmed == gain.value + sent = hw.midiout.send_message.call_args[0][0] + assert sent == [pedal.midi_channel | CONTROL_CHANGE, 75, 64] + + +def test_external_pedal_param_gets_no_sink(v3_system: SystemFixture): + """An externally-routed pedal keeps its own CC: the commit sink stays None + and the raw CC emit stays — the external port is the only transport.""" + handler = v3_system.handler + hw = v3_system.hw + pedal = _add_pedal(v3_system, external=True) + handler.bind_current_pedalboard() + + param = pedal.parameter + assert param is not None + assert handler._sink_for(param) is None + + hw.midiout.send_message.reset_mock() + pedal.sink.handle(AnalogEvent(controller=pedal, raw_value=512, midi_value=64)) + assert hw.midiout.send_message.call_args[0][0] == [pedal.midi_channel | CONTROL_CHANGE, 75, 64] + assert param.value == pedal.midi_value # untouched: no synthetic echo + + +def test_bound_pedal_bar_projects_param(v3_system: SystemFixture, make_plugin, make_parameter): + """The LCD bar tracks the parameter (the owner), not the raw ADC.""" + handler, hw = v3_system.handler, v3_system.hw + pedal = _add_pedal(v3_system, external=False) + gain = make_parameter("Gain", "amp", value=0.25, minimum=0.0, maximum=1.0) + gain.binding = "0:75" + plugin = make_plugin("amp") + plugin.parameters[Symbol("gain")] = gain + handler.current.pedalboard.plugins = [plugin] + handler.bind_current_pedalboard() + + pedal.last_read = 1023 # raw ADC at the ceiling... + gain.reconcile(0.25) # ...while the param sits low + + handler.lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + handler.lcd.draw_main_panel() + handler.poll_lcd_updates() + + icon = next(i for i in handler.lcd.w_controls if i.object is pedal) + assert icon.progress == pytest.approx(pedal.bar_midi_value() / 127.0) + + +def test_unbound_pedal_bar_projects_raw_adc(v3_system: SystemFixture): + """Unbound: the ADC reading is the only fact, so the bar keeps it.""" + handler, hw = v3_system.handler, v3_system.hw + pedal = _add_pedal(v3_system, external=False) + pedal.last_read = 300 + from pistomp.analogmidicontrol import as_midi_value + + handler.lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + handler.lcd.draw_main_panel() + handler.poll_lcd_updates() + + icon = next(i for i in handler.lcd.w_controls if i.object is pedal) + assert icon.progress == pytest.approx(as_midi_value(300) / 127.0) diff --git a/tests/v3/test_encoder_dispatch.py b/tests/v3/test_encoder_dispatch.py index 3361cb9d8..419680ae3 100644 --- a/tests/v3/test_encoder_dispatch.py +++ b/tests/v3/test_encoder_dispatch.py @@ -126,6 +126,28 @@ def test_main_panel_volume_encoder_sets_audiocard_master(v3_system: SystemFixtur hw.midiout.send_message.assert_not_called() +def test_main_panel_volume_encoder_commits(v3_system: SystemFixture): + """The volume turn is one commit through the audio sink, so _confirmed + tracks the card and a keycap-class observer sees committed values.""" + from common.parameter_steps import ParameterSteps + + _prime_main_panel(v3_system) + handler = v3_system.handler + handler.bind_volume_encoder() + enc3 = _enc(v3_system.hw, 3) + param = enc3.parameter + assert param is not None + expected = ParameterSteps.for_parameter(param).move(1) + committed: list[float] = [] + param.on_commit(lambda p: committed.append(p.value)) + + enc3.refresh(1) + + assert committed == [expected] + assert param._confirmed == expected + cast(MagicMock, handler.audiocard.set_volume_parameter).assert_called_with(handler.audiocard.MASTER, expected) + + # --------------------------------------------------------------------------- # Gap 1 (seam) — with no fullscreen panel, lcd.handle does not consume # --------------------------------------------------------------------------- @@ -207,6 +229,7 @@ def _open_dialog_for_param(v3_system, param, *, tweak_id: int | None = None): d = handler.lcd.draw_parameter_dialog(param) if tweak_id is not None: from uilib.glyphs.badge import BadgeGlyph + d.set_tweak_badge(tweak_id, BadgeGlyph(str(tweak_id))) return d @@ -251,6 +274,7 @@ def test_tweak_bound_to_different_param_does_not_corrupt_it(v3_system: SystemFix handler.lcd.w_parameter_dialogs[dialog_param.name] = None # avoid dedup d = _open_dialog_for_param(v3_system, dialog_param, tweak_id=1) from uilib.parameterdialog import Parameterdialog as _PD + assert isinstance(d, _PD) and d._tweak_id == 1 bound_before = bound_param.value diff --git a/tests/v3/test_reactive_parameter.py b/tests/v3/test_reactive_parameter.py index 77c473927..2c71ebe67 100644 --- a/tests/v3/test_reactive_parameter.py +++ b/tests/v3/test_reactive_parameter.py @@ -162,40 +162,65 @@ def test_commit_rolls_back_when_publish_never_leaves(): assert seen == [150.0, 120.0] # painted optimistically, then reverted -def test_rollback_targets_the_last_reconciled_value_not_the_last_commit(): - """Only a reconcile confirms. An unechoed commit followed by a failed one - reverts all the way to mod-ui's last word, not to the unconfirmed edit.""" +def test_good_commit_advances_confirmed_without_echo(): + """No echo ever arrives on the WS send path, so the commit must confirm itself.""" info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 200}} p = Parameter(info, 120.0, None, "inst") - p.commit(150.0, lambda param: True) # sent, not yet echoed - p.commit(160.0, lambda param: False) # never left - assert p.value == 120.0 + assert p.commit(150.0, lambda param: True) + assert p._confirmed == 150.0 + + +def test_rollback_targets_the_last_confirmed_value(): + """Confirmed = echoed or self-committed, whichever came last.""" + info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 200}} + p = Parameter(info, 120.0, None, "inst") - p.reconcile(150.0) - p.commit(160.0, lambda param: False) + p.commit(150.0, lambda param: True) # sent, no echo on this path + p.commit(160.0, lambda param: False) # never left assert p.value == 150.0 + p.reconcile(170.0) + p.commit(180.0, lambda param: False) + assert p.value == 170.0 + -def test_settled_fires_on_reconcile_and_commit_not_preview(): - """subscribe_settled fires for a reconcile (even unchanged) and a successful +def test_on_commit_fires_on_reconcile_and_commit_not_preview(): + """on_commit fires for a reconcile (even unchanged) and a successful commit, but never a bare preview — and not a rolled-back commit.""" info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 200}} p = Parameter(info, 120.0, None, "inst") - settled: list[float] = [] - p.subscribe_settled(lambda param: settled.append(param.value)) + committed: list[float] = [] + p.on_commit(lambda param: committed.append(param.value)) p.preview(130.0) - assert settled == [] # a scrub does not settle + assert committed == [] # a scrub does not commit p.reconcile(130.0) # echo confirming the previewed value — unchanged - assert settled == [130.0] # ...still settles, unconditionally + assert committed == [130.0] # ...still commits, unconditionally p.commit(140.0, lambda param: True) - assert settled == [130.0, 140.0] + assert committed == [130.0, 140.0] p.commit(150.0, lambda param: False) - assert settled == [130.0, 140.0] # rolled back — did not settle + assert committed == [130.0, 140.0] # rolled back — did not commit + + +def test_inbound_param_set_coalesces_to_last_per_tick(v3_system: SystemFixture, make_plugin): + """A fast scrub's echo burst lands many param_sets for one symbol in a + single drain; only the last may reconcile, so the panel repaints once.""" + handler = v3_system.handler + plugin = _install(v3_system, make_plugin) + gain = plugin.parameters[Symbol("gain")] + seen: list[float] = [] + gain.subscribe(lambda p: seen.append(p.value)) + + for v in (0.6, 0.65, 0.7, 0.75, 0.8): + v3_system.ws_bridge.inject(f"param_set /graph/fuzz gain {v}") + handler.poll_ws_messages() + + assert seen == [0.8] + assert gain.value == 0.8 def test_subscribe_returns_unsubscriber(): diff --git a/tests/v3/test_sink_routing.py b/tests/v3/test_sink_routing.py index 833b90bf4..77095a52f 100644 --- a/tests/v3/test_sink_routing.py +++ b/tests/v3/test_sink_routing.py @@ -73,9 +73,7 @@ def _learn_footswitch_to_gain(v3_system, make_plugin, make_parameter, binding_ra return handler, hw, fs, gain -def test_footswitch_press_toggles_between_the_advanced_endpoints( - v3_system, make_plugin, make_parameter -): +def test_footswitch_press_toggles_between_the_advanced_endpoints(v3_system, make_plugin, make_parameter): handler, hw, fs, gain = _learn_footswitch_to_gain(v3_system, make_plugin, make_parameter, (2.0, 8.0)) handler.handle(SwitchEvent(controller=fs, kind=SwitchEventKind.PRESS, timestamp=1.0)) @@ -87,9 +85,7 @@ def test_footswitch_press_toggles_between_the_advanced_endpoints( assert hw.midiout.send_message.call_args[0][0][2] == 0 -def test_ui_edit_between_the_endpoints_takes_the_websocket( - v3_system, make_plugin, make_parameter -): +def test_ui_edit_between_the_endpoints_takes_the_websocket(v3_system, make_plugin, make_parameter): """The switch's CC has only two codes. A mid-range edit sent that way comes back from mod-host as an endpoint, against a screen showing the real value.""" handler, hw, fs, gain = _learn_footswitch_to_gain(v3_system, make_plugin, make_parameter, (2.0, 8.0)) @@ -111,6 +107,35 @@ def test_ui_edit_landing_on_an_endpoint_rides_the_cc(v3_system, make_plugin, mak assert v3_system.ws_bridge.sent_values_for("amp", gain.symbol) == [] +@pytest.mark.parametrize("value", [5.0, 8.0]) +def test_load_window_refuses_cc_publishes(v3_system, make_plugin, make_parameter, value): + """A scrub mid-load must not reach mod-host — the WS path already refuses, + so the CC path must too, or the failed send advances _confirmed against a + loading screen that never got it.""" + handler, hw, fs, gain = _learn_footswitch_to_gain(v3_system, make_plugin, make_parameter, (2.0, 8.0)) + handler._is_pedalboard_loading = True + hw.midiout.send_message.reset_mock() + + handler.parameter_value_commit(gain, value) + + hw.midiout.send_message.assert_not_called() + assert gain.value == 2.0 # rolled back, _confirmed un-advanced + assert gain._confirmed == 2.0 + + +def test_load_window_refuses_encoder_cc_publishes(v3_system, make_plugin): + handler, hw = v3_system.handler, v3_system.hw + enc = next(e for e in hw.encoders if e.midi_CC is not None and e.parameter is None) + _, param = _plugin_with_bound_param(handler, make_plugin, f"{enc.midi_channel}:{enc.midi_CC}") + handler._is_pedalboard_loading = True + hw.midiout.send_message.reset_mock() + + handler.parameter_value_commit(param, 0.75) + + hw.midiout.send_message.assert_not_called() + assert param.value == 0.5 + + def test_encoder_bound_param_rides_the_cc(v3_system, make_plugin): """Every UI edit shares this entry point, panels included, so a bound param never leaves as a param_set.""" From 565e35877399d40a87d0a0d346a676fb47d2d571 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 7 Sep 2026 15:18:03 -0400 Subject: [PATCH 09/11] ruff fix --- tests/test_parameter_binding_range.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_parameter_binding_range.py b/tests/test_parameter_binding_range.py index 1f24c797d..944866589 100644 --- a/tests/test_parameter_binding_range.py +++ b/tests/test_parameter_binding_range.py @@ -18,7 +18,6 @@ the plugin's declared LV2 range.""" from common.parameter import MidiCC, Parameter, PortInfo -from common.parameter_steps import ParameterSteps from modalapi.pedalboard import Pedalboard From 8064b5984b04dbf5d64c450e54fe7119cb13545f Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 7 Sep 2026 15:29:55 -0400 Subject: [PATCH 10/11] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5441fd88c..2e61885b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - The LCD showed a bypass that MOD-UI did not receive, if you tapped it while a pedalboard loaded. The LCD now keeps the last value that MOD-UI confirmed. - A parameter change from a plugin menu could be lost if the connection to MOD-UI was busy. pi-Stomp now sends the value again on the next cycle. - The bypass button in a plugin menu did not agree with the footswitch LED, if the plugin had a footswitch. Every bypass is now one action, and the LED follows it. +- Turning an encoder or expression pedal bound to a parameter while a pedalboard loads no longer leaves the LCD on a value the audio never took: MIDI-CC sends are held back during the load, the same way parameter sends over the WebSocket already were, and the edit reverts on screen. +- An edit that MOD-UI cannot take — the connection is down, or a pedalboard load is in progress — now reverts on the LCD immediately instead of being shown as applied and retried later. A retry of an edit made during a load could deliver the old value after the switch, onto the newly loaded pedalboard. +- An expression pedal or knob bound to a parameter now moves the parameter the way a bound encoder does: the value is confirmed only when the send leaves, and the LCD bar shows the parameter's value instead of the raw pedal position, which could disagree with the audio on a logarithmic or stepped parameter. +- Turning a knob quickly no longer repaints the LCD once per echoed value: the parameter echoes of one poll cycle collapse to the last value per parameter, so a fast tweak paints once. ## [v3.3.1] - 2026-09-01 ### Added From c587c0ef9c320da5c63ca6bd7bcbdaf8c510d632 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 7 Sep 2026 18:21:07 -0400 Subject: [PATCH 11/11] A few fixes --- CHANGELOG.md | 2 ++ modalapi/modhandler.py | 8 +++----- modalapi/websocket_bridge.py | 22 ++++++++++++++++++---- tests/test_websocket_bridge.py | 11 +++++++++++ tests/v3/test_analog_commit.py | 16 ++++++++++++++++ 5 files changed, 50 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e61885b0..412e656e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed +- Removed the tap tempo REST fallback, as we now expect the websocket bridge to always be available. ### Fixed - The parameter dialog on the LCD sometimes did not change values due to a race condition with respect to MOD-UI's `last.json`. pi-Stomp then did not send parameter changes to MOD-UI until you selected a different pedalboard. Parameters on a knob or an encoder continued to work, because they send MIDI CC. - The LCD showed a bypass that MOD-UI did not receive, if you tapped it while a pedalboard loaded. The LCD now keeps the last value that MOD-UI confirmed. diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index ebb100349..fbab6f697 100644 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -397,7 +397,7 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: c.parameter.commit(new_value, self._sink_for(c.parameter)) d = self.lcd.draw_audio_parameter_dialog(c.parameter, self.audio_parameter_commit) if d is not None: - d.update_value(new_value) + d.update_value(c.parameter.value) return True # Resolve the binding row for badge shadow_state (side effect), even @@ -1192,9 +1192,6 @@ def set_current_pedalboard(self, pedalboard): except Exception as e: logging.warning(f"Failed to send external MIDI messages: {e}") - # Sync analog controls last: after bind + external send, matching mod.py - self.hardware.sync_analog_controls() - # Prepare blend modes if configured (snapshot-based activation) try: blend_configs = pedalboard_config.blend_snapshots @@ -1233,8 +1230,9 @@ def set_current_pedalboard(self, pedalboard): self.blend_modes = {} self.active_blend_mode = None - # Caught up with mod-ui. Also closes a window an aborted load left open. + # Caught up with mod-ui. self._is_pedalboard_loading = False + self.hardware.sync_analog_controls() def bind_current_pedalboard(self): # "current" being the pedalboard mod-host says is current diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index 21ea9200a..27a1d6bb5 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -67,7 +67,8 @@ def __init__(self, ws_url: str, command_queue: queue.Queue, received_queue: queu self.messages_sent = 0 self.messages_received = 0 self.peak_latency = 0.0 - self.reconnects = 0 + self._reconnect_events: queue.SimpleQueue[None] = queue.SimpleQueue() + self._has_connected = False def run(self): """Entry point for the background thread.""" @@ -102,6 +103,16 @@ def notify(self): except RuntimeError: pass # loop closed during shutdown + def take_reconnects(self) -> int: + """Return the number of pending reconnect events and clear them.""" + count = 0 + while True: + try: + self._reconnect_events.get_nowait() + except queue.Empty: + return count + count += 1 + async def _interruptible_sleep(self, delay: float) -> bool: """Sleep for delay seconds; returns True if stop was signaled before the delay elapsed.""" try: @@ -127,7 +138,10 @@ async def _async_worker(self): close_timeout=1.0, ) as ws: self.ws = ws - self.reconnects += 1 + if self._has_connected: + self._reconnect_events.put(None) + else: + self._has_connected = True logging.info(f"WebSocket connected to {self.ws_url}") retry_delay = 1.0 # Reset on successful connect reconnect_attempts = 0 # Reset attempts on success @@ -269,8 +283,8 @@ def __init__(self, ws_url: str = "ws://localhost:80/websocket"): self._thread: Optional[threading.Thread] = None def get_reconnects_since_last_call(self) -> int: - count, self._worker.reconnects = self._worker.reconnects, 0 - return count + """Return the number of pending reconnect events and clear them.""" + return self._worker.take_reconnects() @property def connected(self) -> bool: diff --git a/tests/test_websocket_bridge.py b/tests/test_websocket_bridge.py index ff27f0b68..287eb4563 100644 --- a/tests/test_websocket_bridge.py +++ b/tests/test_websocket_bridge.py @@ -402,6 +402,17 @@ def test_connection_scope_clears_the_handle(monkeypatch): assert worker.ws is None +def test_first_connection_is_not_counted_as_reconnect(monkeypatch): + bridge = AsyncWebSocketBridge(ws_url="ws://localhost/test") + worker = bridge._worker + worker.running = True + monkeypatch.setattr(websockets, "connect", lambda *a, **k: _FakeConnect(_ClosingWs(worker))) + + asyncio.run(worker._async_worker()) + + assert bridge.get_reconnects_since_last_call() == 0 + + def test_reconnect_discards_queued_messages(monkeypatch): """A value queued before the connect is dropped, not sent. Section 6 of the plan: closing this window needs a queue that survives a reconnect.""" diff --git a/tests/v3/test_analog_commit.py b/tests/v3/test_analog_commit.py index 05f6dcf6b..a2321aaed 100644 --- a/tests/v3/test_analog_commit.py +++ b/tests/v3/test_analog_commit.py @@ -114,3 +114,19 @@ def test_unbound_pedal_bar_projects_raw_adc(v3_system: SystemFixture): icon = next(i for i in handler.lcd.w_controls if i.object is pedal) assert icon.progress == pytest.approx(as_midi_value(300) / 127.0) + + +def test_board_load_syncs_analog_after_loading_window_closes(v3_system, monkeypatch): + handler = v3_system.handler + pedalboard = handler.pedalboards["/path/to/rig.pedalboard"] + observed_loading: list[bool] = [] + + def sync_analog_controls() -> None: + observed_loading.append(handler._is_pedalboard_loading) + + monkeypatch.setattr(handler.hardware, "sync_analog_controls", sync_analog_controls) + handler._is_pedalboard_loading = True + + handler.set_current_pedalboard(pedalboard) + + assert observed_loading == [False]