From 68705628caf3ab3d5995ba130fc95080403e236c Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Thu, 23 Jul 2026 23:55:34 -0400 Subject: [PATCH 1/6] fix: correct transport BPM routing before refactor Defects in the WebSocket BPM path, isolated in one commit so it can be reverted cleanly when John's fixes land upstream. - The encoder emit guard keyed on "is transport", but only :bpm has a high-precision channel; the guard suppressed CC for :bpb and :rolling too, which have no other way to mod-ui. Key on the symbol so they keep emitting CC. - set_mod_tap_tempo ignored send_bpm's return, so every detent also fired a blocking POST on the 10ms loop. The POST is now the backpressure fallback its test always claimed it was. - _last_bpm_change_time was written and never read. - Restore the early return in parameter_value_commit's audio arm; without it an audio parameter that gains a binding would emit a stray CC. Records why timeInfo's `available` mask is deliberately discarded, and why param_set never carries the transport designations (mod-ui rejects them). Co-Authored-By: Claude Opus 4.8 --- modalapi/modhandler.py | 32 ++++++++++++---- modalapi/pedalboard.py | 7 +++- tests/integration/test_tap_tempo.py | 14 ++++++- tests/v3/test_transport_bindings.py | 59 +++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 10 deletions(-) diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 5e9b12d4d..cbf28c044 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -125,6 +125,14 @@ STARTUP_REST_BACKOFF_S = (0.25, 0.25, 0.5, 1.0, 2.0) +def _is_transport_bpm(param: Parameter | None) -> bool: + return ( + param is not None + and param.instance_id == Pedalboard.TRANSPORT_INSTANCE_ID + and param.symbol == BPM_SYMBOL + ) + + def _remove_binding_row(layer: ContextLayer, binding_id: str) -> None: # Drop any PEDALBOARD-layer row whose control.id matches a learned binding # that's being replaced. Scans all event_kind buckets since a re-learn could @@ -430,8 +438,9 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: # Unconditional, and must stay that way: an unbound encoder has no row, # and this emit is the only way mod-ui sees its CC to MIDI-learn it. # Emission is hardware-level, below the table (see input/README.md). - # Transport parameters bypass 7-bit MIDI CC emission for high-precision WebSocket transport. - if c.parameter is None or c.parameter.instance_id != Pedalboard.TRANSPORT_INSTANCE_ID: + # :bpm alone leaves by WebSocket — 20..280 does not survive 7 bits. The + # other transport ports still ride CC; they have no other way out. + if not _is_transport_bpm(c.parameter): self._emit_midi(c, emit_value) return True @@ -1446,6 +1455,7 @@ def parameter_value_commit(self, param, value): # Audio parameter (volume, EQ, etc.) - handled locally, no remote update needed if param.instance_id is None: self.audio_parameter_commit(param.symbol, value) + return # External MIDI parameters have no mod-host counterpart. The dialog's NAV # path owns sending the CC that _handle_encoder would have sent for a turn; @@ -1460,7 +1470,11 @@ def parameter_value_commit(self, param, value): self._emit_midi(controller, int(value)) return - if not self._is_pedalboard_loading and param.instance_id is not None and param.instance_id != Pedalboard.TRANSPORT_INSTANCE_ID: + # mod-ui rejects param_set on the /pedalboard transport designations + # (:bpm/:bpb/:rolling) — they travel by CC or the dedicated transport-* + # commands, never here. bpm's dialog commit reaches mod-ui reactively. + if not self._is_pedalboard_loading and param.instance_id is not None \ + and param.instance_id != Pedalboard.TRANSPORT_INSTANCE_ID: self.ws_bridge.send_parameter(param.instance_id, param.symbol, param.value) @property @@ -1817,11 +1831,13 @@ def get_callback(self, callback_name): return util.DICT_GET(self.callbacks, callback_name) def set_mod_tap_tempo(self, bpm: float | None) -> None: - if bpm is not None: - self._last_bpm_change_time = time.time() - if self.ws_bridge is not None: - self.ws_bridge.send_bpm(bpm) - self._rest_post(self.root_uri + "set_bpm", json={"value": bpm}) + # WebSocket first: _rest_post blocks the 10ms loop, and an encoder spin + # calls this once per detent. POST only when backpressure refused the send. + if bpm is None: + return + if self.ws_bridge is not None and self.ws_bridge.send_bpm(bpm): + return + self._rest_post(self.root_uri + "set_bpm", json={"value": bpm}) def set_sync_mode(self, mode: SyncMode) -> None: """Optimistically switch the clock source; mod-ui's transport echo diff --git a/modalapi/pedalboard.py b/modalapi/pedalboard.py index dc8127408..9b80885fd 100755 --- a/modalapi/pedalboard.py +++ b/modalapi/pedalboard.py @@ -238,7 +238,12 @@ def hydrate(self, plugin_dict) -> None: def _build_transport_plugin(self, time_info: dict | None) -> Plugin.Plugin: """The /pedalboard pseudo-instance carrying :bpm/:bpb/:rolling. Built - from mod-ui's timeInfo block (or default unbound parameters when absent).""" + from mod-ui's timeInfo block, or from mod-ui's own defaults when the + board carries none.""" + # timeInfo's `available` mask says which ports mod-ui has *addressed*, + # not which exist — transport is global and all three are always + # settable. Built unconditionally so the type stays non-Optional; a + # board that never addressed them just has unbound parameters. time_info = time_info or {} parameters: dict[Symbol, Parameter] = { diff --git a/tests/integration/test_tap_tempo.py b/tests/integration/test_tap_tempo.py index 2702864e7..f2a1f839b 100644 --- a/tests/integration/test_tap_tempo.py +++ b/tests/integration/test_tap_tempo.py @@ -6,12 +6,24 @@ def test_set_mod_tap_tempo(modhandler_system: SystemFixture): - """set_mod_tap_tempo() POSTs to /set_bpm with the BPM value.""" + """set_mod_tap_tempo() sends BPM over the WebSocket, not the blocking POST.""" handler = modhandler_system.handler mock_post = modhandler_system.mock_post handler.set_mod_tap_tempo(120) + assert "transport-bpm 120" in modhandler_system.ws_bridge.sent + mock_post.assert_not_called() + + +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.""" + handler = modhandler_system.handler + mock_post = modhandler_system.mock_post + modhandler_system.ws_bridge.send_bpm = MagicMock(return_value=False) + + 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] diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index 618365821..dda79a9c7 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -510,6 +510,65 @@ def test_encoder_bpm_turn_without_websocket_bridge_falls_back_to_rest_post( 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 backpressure fallback, not a companion to the send — 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 unittest.mock import MagicMock + from pistomp.input.event import EncoderEvent + + handler = v3_system.handler + hw = v3_system.hw + 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(":") + tp = _attach_transport_plugin(handler, bpb_cc={"channel": int(channel), "control": int(cc)}) + + handler._emit_midi = MagicMock() + handler._handle_encoder(EncoderEvent(controller=enc1, rotations=1, multiplier=1.0)) + + assert tp.parameters[BPB_SYMBOL].value == 5.0 + handler._emit_midi.assert_called_once() + + +def test_transport_dialog_commit_never_param_sets(v3_system: SystemFixture, make_plugin): + """mod-ui rejects param_set on the transport designations, so a :bpb dialog + commit must not emit one — it would draw a "specially designated port" error.""" + handler = v3_system.handler + assert handler.current is not None + + handler.current.pedalboard.plugins = [make_plugin("noise", bypassed=False)] + tp = _attach_transport_plugin(handler) + + v3_system.ws_bridge.sent.clear() + handler.parameter_value_commit(tp.parameters[BPB_SYMBOL], 6.0) + + assert not any("param_set" in m and ":bpb" in m for m in v3_system.ws_bridge.sent) + + def test_encoder_bpm_turn_parameter_dialog_snapshot(v3_system: SystemFixture, make_plugin, snapshot): """Turning a BPM-bound encoder 1 detent notch displays the parameter dialog on the LCD at 121 BPM.""" from pistomp.input.event import EncoderEvent From da8e61ff24a150055e334c30e5d1a15e1f9ec8d1 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 24 Jul 2026 00:36:19 -0400 Subject: [PATCH 2/6] refactor: give parameters a provenance and an upstream sink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR wired :bpm's tempo send through a Parameter observer, which made a plain `param.value = x` write emit a WebSocket packet. That coupling forced `_suppress_bpm_event` — a re-entrancy flag guarding the one call site (TransportMessage) where a *remote* write must not echo back at the sender. Every future write to :bpm was a trap someone had to remember to suppress. Split the two reasons a value moves into two verbs: - `param.value = x` / `set_param_value` — a remote reconcile. mod-ui is the single writer; adopt the value, notify observers to repaint, publish nothing. - `param.edit(x)` — a local command (knob turn, dialog commit). Set the value and publish it upstream through the parameter's `sink`. A reconcile now has no send to suppress: the distinction is which method the caller reaches for, not a flag on a shared setter. `_suppress_bpm_event`, the subscribe/unsub bookkeeping, and `_on_bpm_param_changed` are gone. `ParamSink` (common/param_source.py) is the outbound dual of the existing `ParamSource` protocol. :bpm's sink routes to set_mod_tap_tempo; it is attached at bind time. The encoder and dialog-commit paths for :bpm call edit(); the CC emit and param_set arms are unchanged for every other parameter. This is the transport slice. Extending sinks to CC / param_set / audio so parameter_value_commit's dispatch collapses entirely is a natural follow-up. Co-Authored-By: Claude Opus 4.8 --- common/param_source.py | 22 ++++++++++- common/parameter.py | 20 +++++++++- modalapi/modhandler.py | 58 +++++++++++++++++------------ tests/v3/test_transport_bindings.py | 26 +++++++------ 4 files changed, 90 insertions(+), 36 deletions(-) diff --git a/common/param_source.py b/common/param_source.py index 763564396..0df1f1e57 100644 --- a/common/param_source.py +++ b/common/param_source.py @@ -74,4 +74,24 @@ def is_bypassed(self) -> bool: ... def set_bypass(self, bypass: bool) -> None: ... - pedalboard_snapshot: "dict[Symbol, float]" \ No newline at end of file + pedalboard_snapshot: "dict[Symbol, float]" + + +@runtime_checkable +class ParamSink(Protocol): + """The outbound dual of `ParamSource` — where a *local edit* goes next. + + A parameter's value moves for one of two reasons, and they are not the same + event. A **local edit** (a knob turn, a dialog commit) is a command: set the + value and tell whoever owns it upstream. A **remote reconcile** (mod-ui + echoing its own state back at us) is the opposite: adopt the value, tell + nobody — mod-ui is already the single writer. + + `Parameter.value`'s setter carries the reconcile semantics (notify observers + to repaint, publish nothing); `Parameter.edit()` carries the command (set, + then publish through this sink). A reconcile has no send to suppress — the + distinction is which method the caller reaches for, not a flag guarding a + shared one. A parameter with no sink is display-only: reconciled, never sent. + """ + + def publish(self, param: "Parameter") -> None: ... \ No newline at end of file diff --git a/common/parameter.py b/common/parameter.py index 819ef8b3c..6246e3d1a 100644 --- a/common/parameter.py +++ b/common/parameter.py @@ -17,10 +17,13 @@ from collections.abc import Callable from enum import Enum -from typing import NewType, NotRequired, TypedDict +from typing import NewType, NotRequired, TypedDict, TYPE_CHECKING import json import common.util as util +if TYPE_CHECKING: + from common.param_source import ParamSink + # strings as they appear in TTL files TTL_ENUMERATION = 'enumeration' TTL_INTEGER = 'integer' @@ -120,6 +123,10 @@ def __init__(self, plugin_info: PortInfo, value: float, binding: str | None, ins self._observers: list[Callable[[Parameter], None]] = [] self._value: float = 0.0 self.value = float(value) + # Where local edits go upstream. None = display-only: reconciled from + # mod-ui, never sent back. Attached at bind time by whoever owns the + # remote channel. + self.sink: ParamSink | None = None self.binding: str | None = binding self.instance_id: str | None = instance_id.lstrip("/") if instance_id else instance_id self.type = Type.DEFAULT @@ -162,6 +169,17 @@ def value(self, v: float) -> None: for observe in self._observers: observe(self) + def edit(self, value: float) -> None: + """A local edit — a knob turn or dialog commit. Set the value (notifying + observers to repaint) and publish it upstream. Publishing is + unconditional, like the CC emit and param_set it stands in for: the LCD + paths pre-write `value` for display, so an edit cannot rely on the + setter's change detection. Contrast the plain setter, which reconciles a + remote echo and publishes nothing.""" + self.value = value + if self.sink is not None: + self.sink.publish(self) + def subscribe(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: """Register *cb* to fire on every changed-value write. Returns its own unsubscriber. An unchanged write (v == current) does not notify.""" diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index cbf28c044..f5f436d73 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -133,6 +133,19 @@ def _is_transport_bpm(param: Parameter | None) -> bool: ) +class _TransportBpmSink: + """:bpm's upstream channel. mod-ui's :bpm is a global designation, not a + plugin control port — param_set is rejected — so an edit rides the dedicated + transport-bpm command (POST fallback). That routing lives in + set_mod_tap_tempo; this just names :bpm's edits as its callers.""" + + def __init__(self, send: Callable[[float], None]): + self._send = send + + def publish(self, param: Parameter) -> None: + self._send(param.value) + + def _remove_binding_row(layer: ContextLayer, binding_id: str) -> None: # Drop any PEDALBOARD-layer row whose control.id matches a learned binding # that's being replaced. Scans all event_kind buckets since a re-learn could @@ -233,10 +246,6 @@ def __init__(self, audiocard: Audiocard, homedir, data_dir="/home/pistomp/data") # Suppress outbound WebSocket messages while a pedalboard change is in flight. self._is_pedalboard_loading = False - # Reactive BPM parameter observer state - self._bpm_unsub: Callable[[], None] | None = None - self._suppress_bpm_event: bool = False - # Tuner state self._tuner_source_factory: TunerSourceFactory | None = None self._tuner_source_spec: str = "jack" @@ -429,6 +438,13 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: ) if c.parameter is not None: new_value = ParameterSteps.for_parameter(c.parameter).move(delta) + # :bpm rides its own WebSocket sink — 20..280 does not survive 7 + # bits — and must not also emit CC. Every other bound param's CC is + # its transport to mod-host, so it falls through to the emit below. + if _is_transport_bpm(c.parameter): + c.parameter.edit(new_value) + self.lcd.display_parameter_value(c.parameter, new_value) + return True c.parameter.value = new_value self.lcd.display_parameter_value(c.parameter, new_value) emit_value = c.bar_midi_value() @@ -438,10 +454,7 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: # Unconditional, and must stay that way: an unbound encoder has no row, # and this emit is the only way mod-ui sees its CC to MIDI-learn it. # Emission is hardware-level, below the table (see input/README.md). - # :bpm alone leaves by WebSocket — 20..280 does not survive 7 bits. The - # other transport ports still ride CC; they have no other way out. - if not _is_transport_bpm(c.parameter): - self._emit_midi(c, emit_value) + self._emit_midi(c, emit_value) return True def encoder_fallback(self, controller: EncoderController) -> int: @@ -889,14 +902,13 @@ def _handle_ws_message(self, msg: WebSocketMessage): # labels track even when the change originates elsewhere (Link, # MIDI slave, another HMI). :rolling's enum flips Playing/Stopped. if self._current is not None: + # A remote reconcile: adopt mod-ui's values, publish nothing. + # set_param_value routes through the plain setter, not edit(), + # so :bpm's sink never fires back at the sender. tp = self.current.pedalboard.transport_plugin tp.set_param_value(ROLLING_SYMBOL, 1.0 if msg.rolling else 0.0) tp.set_param_value(BPB_SYMBOL, msg.beats_per_bar) - self._suppress_bpm_event = True - try: - tp.set_param_value(BPM_SYMBOL, msg.bpm) - finally: - self._suppress_bpm_event = False + tp.set_param_value(BPM_SYMBOL, msg.bpm) if self.hardware and self.hardware.taptempo: self.hardware.taptempo.set_bpm(msg.bpm) if self.hardware.taptempo.is_enabled(): @@ -1222,19 +1234,12 @@ def bind_current_pedalboard(self): # The pedalboard data has already been loaded, but this will overlay # any real time settings self._controller_manager.bind(self.current) - self._bind_transport_bpm_listener() + self._attach_transport_bpm_sink() - def _bind_transport_bpm_listener(self) -> None: - if self._bpm_unsub is not None: - self._bpm_unsub() - self._bpm_unsub = None + def _attach_transport_bpm_sink(self) -> None: if self._current is not None: bpm_param = self.current.pedalboard.transport_plugin.parameters[BPM_SYMBOL] - self._bpm_unsub = bpm_param.subscribe(self._on_bpm_param_changed) - - def _on_bpm_param_changed(self, param: Parameter) -> None: - if not self._suppress_bpm_event: - self.set_mod_tap_tempo(param.value) + bpm_param.sink = _TransportBpmSink(self.set_mod_tap_tempo) def _redraw_after_binding(self, controller: Controller, is_footswitch: bool) -> None: if is_footswitch: @@ -1439,6 +1444,13 @@ def effective_table(self) -> ContextStack: # Parameter Stuff # def parameter_value_commit(self, param, value): + # :bpm carries its own sink (transport-bpm WebSocket); edit() sets the + # value and publishes through it. It is neither a plugin control port + # nor an audio/external param, so it exits before those arms. + if _is_transport_bpm(param): + param.edit(value) + return + # Route plugin params through the plugin's mirror so a bound footswitch # reconciles now, not only on the mod-host echo — the same set_value the # ParamSetMessage arm runs. Audio/external params have no plugin mirror. diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index dda79a9c7..1998691ab 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -90,9 +90,10 @@ def test_transport_plugin_unconditional_fallback(v3_system: SystemFixture): assert ROLLING_SYMBOL in tp_zero.parameters -def test_reactive_bpm_parameter_change_triggers_set_mod_tap_tempo(v3_system: SystemFixture): - """Writing to transport_plugin.parameters[BPM_SYMBOL].value reactively notifies - subscribers and triggers set_mod_tap_tempo.""" +def test_bpm_edit_publishes_but_reconcile_does_not(v3_system: SystemFixture): + """The provenance split: `edit()` is a local command and publishes upstream; + the plain setter / `set_param_value` is a remote reconcile and publishes + nothing. mod-ui stays the single writer with no flag to suppress.""" from unittest.mock import MagicMock handler = v3_system.handler @@ -101,18 +102,21 @@ def test_reactive_bpm_parameter_change_triggers_set_mod_tap_tempo(v3_system: Sys _attach_transport_plugin(handler) ws_bridge.send_bpm = MagicMock(return_value=True) + bpm = handler.current.pedalboard.transport_plugin.parameters[BPM_SYMBOL] - # Change BPM parameter value directly (e.g. via encoder or set_param_value) - tp = handler.current.pedalboard.transport_plugin - tp.set_param_value(BPM_SYMBOL, 148.0) + # Reconcile (mod-ui echoing its own state): adopt the value, send nothing. + bpm.value = 148.0 + handler.current.pedalboard.transport_plugin.set_param_value(BPM_SYMBOL, 149.0) + ws_bridge.send_bpm.assert_not_called() - # Verify reactive subscriber triggered send_bpm - ws_bridge.send_bpm.assert_called_once_with(148.0) + # Local edit (a knob turn): set and publish through the sink. + bpm.edit(150.0) + ws_bridge.send_bpm.assert_called_once_with(150.0) -def test_transport_message_ws_suppresses_bpm_echo(v3_system: SystemFixture): - """An incoming WebSocket TransportMessage updates transport parameters without - echo-calling send_bpm back to mod-ui.""" +def test_transport_message_reconciles_without_echo(v3_system: SystemFixture): + """An incoming TransportMessage reconciles the transport parameters through the + plain setter, so :bpm's sink never echoes send_bpm back at mod-ui.""" from unittest.mock import MagicMock handler = v3_system.handler From 4ee294b87d67f2285defb9124060bb6f43c760b0 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 24 Jul 2026 01:18:37 -0400 Subject: [PATCH 3/6] refactor: name the three value motions; roll back unconfirmed edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Privatize Parameter's raw setter and route every write through a verb that names its provenance: reconcile(v) adopt the single writer's value — repaint, mark confirmed, send nothing. Also the optimistic-preview channel the dialog scrubs on. preview(v) a local move not yet committed — repaint, leave unconfirmed. commit(v) a finished edit — repaint, publish through the sink. _confirmed tracks the last value mod-ui echoed back; the gap from _value is `pending`. With that ledger, commit can fix a real bug: if the sink's send never leaves (WS backpressure and POST both fail) it rolls the value back to _confirmed, so the LCD can no longer show a BPM mod-ui never accepted. ParamSink.publish and set_mod_tap_tempo now return that success flag. The language no longer has a word for "just set it," so every call site declares which motion it is. preview and reconcile share mechanics today; the ledger is the seam where they'd diverge. Co-Authored-By: Claude Opus 4.8 --- common/param_source.py | 21 +++----- common/parameter.py | 57 ++++++++++++++-------- modalapi/modhandler.py | 40 +++++++-------- modalapi/plugin.py | 4 +- plugins/audio_midi/source.py | 2 +- tests/integration/test_tap_tempo.py | 17 ++++++- tests/test_parameter_steps.py | 2 +- tests/v3/test_encoder_value_resync.py | 4 +- tests/v3/test_footswitch_param_sync.py | 2 +- tests/v3/test_reactive_parameter.py | 67 +++++++++++++++++++++----- tests/v3/test_transport_bindings.py | 4 +- uilib/parameterdialog.py | 4 +- 12 files changed, 146 insertions(+), 78 deletions(-) diff --git a/common/param_source.py b/common/param_source.py index 0df1f1e57..3ef2c7271 100644 --- a/common/param_source.py +++ b/common/param_source.py @@ -79,19 +79,12 @@ def set_bypass(self, bypass: bool) -> None: ... @runtime_checkable class ParamSink(Protocol): - """The outbound dual of `ParamSource` — where a *local edit* goes next. - - A parameter's value moves for one of two reasons, and they are not the same - event. A **local edit** (a knob turn, a dialog commit) is a command: set the - value and tell whoever owns it upstream. A **remote reconcile** (mod-ui - echoing its own state back at us) is the opposite: adopt the value, tell - nobody — mod-ui is already the single writer. - - `Parameter.value`'s setter carries the reconcile semantics (notify observers - to repaint, publish nothing); `Parameter.edit()` carries the command (set, - then publish through this sink). A reconcile has no send to suppress — the - distinction is which method the caller reaches for, not a flag guarding a - shared one. A parameter with no sink is display-only: reconciled, never sent. + """The outbound dual of `ParamSource` — where a `Parameter.commit` goes next. + + `reconcile` adopts the single writer's value and sends nothing; `commit` + publishes a finished local edit through this sink. `publish` returns whether + the send left — False lets commit roll the value back. A param with no sink + is display-only. """ - def publish(self, param: "Parameter") -> None: ... \ No newline at end of file + def publish(self, param: "Parameter") -> bool: ... \ No newline at end of file diff --git a/common/parameter.py b/common/parameter.py index 6246e3d1a..e83627eea 100644 --- a/common/parameter.py +++ b/common/parameter.py @@ -117,12 +117,13 @@ def __init__(self, plugin_info: PortInfo, value: float, binding: str | None, ins # fallbacks only serve the params we synthesise (bypass, volume, VU). self.default: float = float(ranges.get("default", self.minimum)) - # Reactive value: a property setter that notifies observers. _observers - # must exist before the first assignment below, or the write fires into - # a missing list. + # 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; the gap from + # _value is `pending`. self._observers: list[Callable[[Parameter], None]] = [] - self._value: float = 0.0 - self.value = float(value) + self._value: float = float(value) + self._confirmed: float = float(value) # Where local edits go upstream. None = display-only: reconciled from # mod-ui, never sent back. Attached at bind time by whoever owns the # remote channel. @@ -161,25 +162,41 @@ def __init__(self, plugin_info: PortInfo, value: float, binding: str | None, ins def value(self) -> float: return self._value - @value.setter - def value(self, v: float) -> None: - if v == self._value: + @property + def pending(self) -> bool: + """A committed edit the single writer hasn't echoed back yet.""" + return self._value != self._confirmed + + def reconcile(self, value: float) -> None: + """Adopt the single writer's value: repaint and mark confirmed, publish + nothing. Also the optimistic-preview channel — the dialog scrubs by + reconciling its own value, which is why commit can't gate on change.""" + self._confirmed = value + self._set(value) + + def preview(self, value: float) -> None: + """An optimistic local move not yet committed — a knob mid-turn whose CC + emit is the real send. Repaint; leave the edit unconfirmed; publish + nothing through the sink.""" + self._set(value) + + def commit(self, value: float) -> None: + """A finished local edit: repaint, then publish through the sink. Roll + back to the last confirmed value if the send never leaves — otherwise the + LCD would show a number mod-ui never accepted. Publishing is + unconditional; preview and reconcile share mechanics, so there is nothing + to diff against here.""" + self._set(value) + if self.sink is not None and not self.sink.publish(self): + self._set(self._confirmed) + + def _set(self, value: float) -> None: + if value == self._value: return - self._value = v + self._value = value for observe in self._observers: observe(self) - def edit(self, value: float) -> None: - """A local edit — a knob turn or dialog commit. Set the value (notifying - observers to repaint) and publish it upstream. Publishing is - unconditional, like the CC emit and param_set it stands in for: the LCD - paths pre-write `value` for display, so an edit cannot rely on the - setter's change detection. Contrast the plain setter, which reconciles a - remote echo and publishes nothing.""" - self.value = value - if self.sink is not None: - self.sink.publish(self) - def subscribe(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: """Register *cb* to fire on every changed-value write. Returns its own unsubscriber. An unchanged write (v == current) does not notify.""" diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index f5f436d73..233675ed6 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -135,15 +135,15 @@ def _is_transport_bpm(param: Parameter | None) -> bool: class _TransportBpmSink: """:bpm's upstream channel. mod-ui's :bpm is a global designation, not a - plugin control port — param_set is rejected — so an edit rides the dedicated - transport-bpm command (POST fallback). That routing lives in - set_mod_tap_tempo; this just names :bpm's edits as its callers.""" + plugin control port — param_set is rejected — so a commit rides the dedicated + transport-bpm command (POST fallback). set_mod_tap_tempo returns whether the + send left; commit rolls back if it didn't.""" - def __init__(self, send: Callable[[float], None]): + def __init__(self, send: Callable[[float], bool]): self._send = send - def publish(self, param: Parameter) -> None: - self._send(param.value) + def publish(self, param: Parameter) -> bool: + return self._send(param.value) def _remove_binding_row(layer: ContextLayer, binding_id: str) -> None: @@ -419,7 +419,7 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: if c.type == Token.VOLUME and c.parameter is not None: new_value = ParameterSteps.for_parameter(c.parameter).move(delta) - c.parameter.value = new_value + c.parameter.preview(new_value) self.audiocard.set_volume_parameter(self.audiocard.MASTER, new_value) d = self.lcd.draw_audio_parameter_dialog(c.parameter, self.audio_parameter_commit) if d is not None: @@ -442,10 +442,10 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: # bits — and must not also emit CC. Every other bound param's CC is # its transport to mod-host, so it falls through to the emit below. if _is_transport_bpm(c.parameter): - c.parameter.edit(new_value) + c.parameter.commit(new_value) self.lcd.display_parameter_value(c.parameter, new_value) return True - c.parameter.value = new_value + c.parameter.preview(new_value) self.lcd.display_parameter_value(c.parameter, new_value) emit_value = c.bar_midi_value() else: @@ -551,7 +551,7 @@ def _fire_row(self, decl: BindingDecl, event: ControllerEvent) -> bool: controller.set_led(controller.toggled) self._emit_midi(controller, 127 if controller.toggled else 0) if controller.parameter is not None: - controller.parameter.value = controller.value_for(controller.toggled) + controller.parameter.preview(controller.value_for(controller.toggled)) self.update_lcd_fs(footswitch=controller) case ParamEffect(): # Footswitch PRESS with a bound plugin param. "on" polarity @@ -566,7 +566,7 @@ def _fire_row(self, decl: BindingDecl, event: ControllerEvent) -> bool: if fs.midi_CC is not None: self._emit_midi(fs, 127 if new_toggled else 0) if fs.parameter is not None: - fs.parameter.value = fs.value_for(new_toggled) + fs.parameter.preview(fs.value_for(new_toggled)) self.update_lcd_fs(footswitch=fs) case RelayEffect(): if fs is not None: @@ -903,8 +903,8 @@ def _handle_ws_message(self, msg: WebSocketMessage): # MIDI slave, another HMI). :rolling's enum flips Playing/Stopped. if self._current is not None: # A remote reconcile: adopt mod-ui's values, publish nothing. - # set_param_value routes through the plain setter, not edit(), - # so :bpm's sink never fires back at the sender. + # set_param_value routes through reconcile, not commit, so :bpm's + # sink never fires back at the sender. tp = self.current.pedalboard.transport_plugin tp.set_param_value(ROLLING_SYMBOL, 1.0 if msg.rolling else 0.0) tp.set_param_value(BPB_SYMBOL, msg.beats_per_bar) @@ -1448,7 +1448,7 @@ def parameter_value_commit(self, param, value): # value and publishes through it. It is neither a plugin control port # nor an audio/external param, so it exits before those arms. if _is_transport_bpm(param): - param.edit(value) + param.commit(value) return # Route plugin params through the plugin's mirror so a bound footswitch @@ -1462,7 +1462,7 @@ def parameter_value_commit(self, param, value): if plugin is not None: plugin.set_param_value(param.symbol, value) else: - param.value = value + param.preview(value) # Audio parameter (volume, EQ, etc.) - handled locally, no remote update needed if param.instance_id is None: @@ -1842,14 +1842,16 @@ def audio_parameter_commit(self, symbol, value): def get_callback(self, callback_name): return util.DICT_GET(self.callbacks, callback_name) - def set_mod_tap_tempo(self, bpm: float | None) -> None: + 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. + # Returns whether the value left, so a failed send rolls the LCD back. if bpm is None: - return + return False if self.ws_bridge is not None and self.ws_bridge.send_bpm(bpm): - return - self._rest_post(self.root_uri + "set_bpm", json={"value": bpm}) + return True + resp = self._rest_post(self.root_uri + "set_bpm", json={"value": bpm}) + return resp is not None and resp.ok def set_sync_mode(self, mode: SyncMode) -> None: """Optimistically switch the clock source; mod-ui's transport echo diff --git a/modalapi/plugin.py b/modalapi/plugin.py index 06c1ade70..80b625c64 100755 --- a/modalapi/plugin.py +++ b/modalapi/plugin.py @@ -129,7 +129,7 @@ def toggle_bypass(self) -> float: if param is None: return 0.0 new_value = 0.0 if param.value else 1.0 - param.value = new_value + param.preview(new_value) return new_value def set_param_value(self, symbol: Symbol, value: float) -> None: @@ -141,7 +141,7 @@ def set_param_value(self, symbol: Symbol, value: float) -> None: param = self.parameters.get(symbol) if param is None: return - param.value = value + param.reconcile(value) for c in self.controllers: # Only stateful controllers hold a presentation copy to sync (a # footswitch keycap, a pot's reading). Encoders own no copy. diff --git a/plugins/audio_midi/source.py b/plugins/audio_midi/source.py index ffe073efd..45994ae03 100644 --- a/plugins/audio_midi/source.py +++ b/plugins/audio_midi/source.py @@ -106,7 +106,7 @@ def set_param_value(self, symbol: Symbol, value: float) -> None: self._hardware.recalibrateVU_gain(value) p = self.parameters.get(symbol) if p is not None: - p.value = value + p.reconcile(value) def subscribe(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: unsubs = [p.subscribe(cb) for p in self.parameters.values()] diff --git a/tests/integration/test_tap_tempo.py b/tests/integration/test_tap_tempo.py index f2a1f839b..e9c99d4b7 100644 --- a/tests/integration/test_tap_tempo.py +++ b/tests/integration/test_tap_tempo.py @@ -6,16 +6,29 @@ def test_set_mod_tap_tempo(modhandler_system: SystemFixture): - """set_mod_tap_tempo() sends BPM over the WebSocket, not the blocking POST.""" + """set_mod_tap_tempo() sends BPM over the WebSocket, not the blocking POST, + and reports that the value left.""" handler = modhandler_system.handler mock_post = modhandler_system.mock_post - handler.set_mod_tap_tempo(120) + assert handler.set_mod_tap_tempo(120) is True assert "transport-bpm 120" in modhandler_system.ws_bridge.sent mock_post.assert_not_called() +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 + 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 + + 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.""" handler = modhandler_system.handler diff --git a/tests/test_parameter_steps.py b/tests/test_parameter_steps.py index fa910bfbc..eca64427a 100644 --- a/tests/test_parameter_steps.py +++ b/tests/test_parameter_steps.py @@ -59,7 +59,7 @@ def __init__(self, parameters: dict[Symbol, Parameter]) -> None: def set_param_value(self, symbol: Symbol, value: float) -> None: p = self.parameters.get(symbol) if p is not None: - p.value = value + p.reconcile(value) class _ConcretePluginPanel: diff --git a/tests/v3/test_encoder_value_resync.py b/tests/v3/test_encoder_value_resync.py index fa394212b..98e141f89 100644 --- a/tests/v3/test_encoder_value_resync.py +++ b/tests/v3/test_encoder_value_resync.py @@ -57,7 +57,7 @@ def test_value_continues_from_nav_change(v3_system: SystemFixture): assert param.value > 50.0 # Nav encoder (simulated via direct write, as parameter_value_change does). - param.value = 90.0 + param.preview(90.0) # One detent forward — continues from 90, not from the pre-nav position. enc.refresh(1) @@ -70,7 +70,7 @@ def test_value_stays_near_nav_change_on_backward_turn(v3_system: SystemFixture): enc = _bound_tweak(v3_system, param) enc.refresh(5) - param.value = 80.0 + param.preview(80.0) enc.refresh(-1) assert param.value > 60.0 diff --git a/tests/v3/test_footswitch_param_sync.py b/tests/v3/test_footswitch_param_sync.py index 66264c195..8a5e4ebb3 100644 --- a/tests/v3/test_footswitch_param_sync.py +++ b/tests/v3/test_footswitch_param_sync.py @@ -66,7 +66,7 @@ def test_press_on_writes_param_to_max(v3_system: SystemFixture, make_plugin, mak def test_press_off_writes_param_to_min(v3_system: SystemFixture, make_plugin, make_parameter): handler, fs0, solo = _bind_solo_footswitch(v3_system, make_plugin, make_parameter) fs0.toggled = True - solo.value = solo.maximum + solo.reconcile(solo.maximum) event = SwitchEvent(controller=fs0, kind=SwitchEventKind.PRESS, timestamp=1000.0) assert handler.handle(event) is True diff --git a/tests/v3/test_reactive_parameter.py b/tests/v3/test_reactive_parameter.py index 70bf73864..638da0fff 100644 --- a/tests/v3/test_reactive_parameter.py +++ b/tests/v3/test_reactive_parameter.py @@ -108,28 +108,71 @@ def _open_fullscreen(v3_system: SystemFixture, plugin: Plugin) -> _TrackedFullsc # --------------------------------------------------------------------------- -# 1. Parameter.value setter notifies on change, skips on no-change +# 1. reconcile notifies on change, skips on no-change # --------------------------------------------------------------------------- -def test_param_value_setter_notifies_on_change_not_on_noop(): - """Writing param.value from any site notifies; an unchanged write does not.""" +def test_reconcile_notifies_on_change_not_on_noop(): + """A value write notifies observers; an unchanged write does not.""" info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 1}} p = Parameter(info, 0.0, None, "inst") calls: list[Parameter] = [] p.subscribe(lambda param: calls.append(param)) - p.value = 1.0 + p.reconcile(1.0) assert len(calls) == 1 assert calls[0] is p - p.value = 1.0 # unchanged — no notification + p.reconcile(1.0) # unchanged — no notification assert len(calls) == 1 - p.value = 0.5 + p.reconcile(0.5) assert len(calls) == 2 +def test_commit_publishes_and_stays_pending_until_reconciled(): + """A committed edit publishes, holds its value, and reads pending until the + single writer echoes it back.""" + info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 200}} + p = Parameter(info, 120.0, None, "inst") + sent: list[float] = [] + + class OkSink: + def publish(self, param: Parameter) -> bool: + sent.append(param.value) + return True + + p.sink = OkSink() + p.commit(150.0) + + assert p.value == 150.0 + assert sent == [150.0] + assert p.pending + + p.reconcile(150.0) + assert not p.pending + + +def test_commit_rolls_back_when_publish_never_leaves(): + """A send that doesn't leave reverts the value to the last confirmed one — + the LCD must never show a number the single writer didn't accept.""" + info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 200}} + p = Parameter(info, 120.0, None, "inst") + seen: list[float] = [] + p.subscribe(lambda param: seen.append(param.value)) + + class DeadSink: + def publish(self, param: Parameter) -> bool: + return False + + p.sink = DeadSink() + p.commit(150.0) + + assert p.value == 120.0 + assert not p.pending + assert seen == [150.0, 120.0] # painted optimistically, then reverted + + def test_subscribe_returns_unsubscriber(): """The returned callable tears down the subscription.""" info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 1}} @@ -137,11 +180,11 @@ def test_subscribe_returns_unsubscriber(): calls: list[Parameter] = [] unsub = p.subscribe(lambda param: calls.append(param)) - p.value = 1.0 + p.reconcile(1.0) assert len(calls) == 1 unsub() - p.value = 0.0 + p.reconcile(0.0) assert len(calls) == 1 # no more notifications after unsubscribe @@ -151,12 +194,12 @@ def test_plugin_subscribe_fans_out_to_all_params(make_plugin): calls: list[Parameter] = [] unsub = plugin.subscribe(lambda param: calls.append(param)) - plugin.parameters[BYPASS_SYMBOL].value = 1.0 - plugin.parameters[Symbol("gain")].value = 0.9 + plugin.parameters[BYPASS_SYMBOL].reconcile(1.0) + plugin.parameters[Symbol("gain")].reconcile(0.9) assert len(calls) == 2 unsub() - plugin.parameters[BYPASS_SYMBOL].value = 0.0 + plugin.parameters[BYPASS_SYMBOL].reconcile(0.0) assert len(calls) == 2 # unsubscribed @@ -539,5 +582,5 @@ def test_dismissed_dialog_unsubscribes(v3_system: SystemFixture, make_plugin): dialog.pop() v3_system.handler.poll_lcd_updates() - gain.value = 0.75 + gain.reconcile(0.75) assert dialog.last_param_value == 0.5 # never redrew after dismissal diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index 1998691ab..7385121f2 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -105,12 +105,12 @@ def test_bpm_edit_publishes_but_reconcile_does_not(v3_system: SystemFixture): bpm = handler.current.pedalboard.transport_plugin.parameters[BPM_SYMBOL] # Reconcile (mod-ui echoing its own state): adopt the value, send nothing. - bpm.value = 148.0 + bpm.reconcile(148.0) handler.current.pedalboard.transport_plugin.set_param_value(BPM_SYMBOL, 149.0) ws_bridge.send_bpm.assert_not_called() # Local edit (a knob turn): set and publish through the sink. - bpm.edit(150.0) + bpm.commit(150.0) ws_bridge.send_bpm.assert_called_once_with(150.0) diff --git a/uilib/parameterdialog.py b/uilib/parameterdialog.py index 5393569fc..ba84783cf 100644 --- a/uilib/parameterdialog.py +++ b/uilib/parameterdialog.py @@ -296,7 +296,7 @@ def tick(self): def update_value(self, new_value: float) -> None: """Update display with new value (controller already calculated it).""" self.reset_timeout() - self.parameter.value = new_value + self.parameter.preview(new_value) def parameter_value_change(self, direction, count: int = 1, multiplier: float = 1.0): self.reset_timeout() @@ -311,7 +311,7 @@ def parameter_value_change(self, direction, count: int = 1, multiplier: float = if new_value == self.parameter.value: return - self.parameter.value = new_value + self.parameter.preview(new_value) if self.action is not None: self.action(self.object, new_value) From f81b0d7d620e7eba1b81471a3e90b4c3d099c5f2 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 24 Jul 2026 01:27:08 -0400 Subject: [PATCH 4/6] refactor: mirror footswitch keycaps by subscription, not a manual loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The keycap sync moves off Plugin.set_param_value's hand-rolled controller loop onto a reactive channel. A parameter now carries two observer lists: the live one (subscribe) that repaints panels on any change including a preview, and a settled one (subscribe_settled) fired only by reconcile and commit — never a bare preview — and fired unconditionally, even at an unchanged value. That split is the keycap's existing invariant made explicit: a local footswitch press previews the value and refreshes its own toggle/LED, waiting for the echo; a menu/dialog commit and the mod-ui echo both settle it and refresh the keycap now. StatefulController.bind_to_parameter subscribes to the settled channel through the _unsub_param lifecycle already scaffolded for it, so set_param_value collapses to a bare reconcile and neither write path reaches for controllers. Co-Authored-By: Claude Opus 4.8 --- common/parameter.py | 42 ++++++++++++++++++++++------- modalapi/plugin.py | 20 +++++--------- pistomp/controller.py | 5 ++++ tests/v3/test_reactive_parameter.py | 31 +++++++++++++++++++++ 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/common/parameter.py b/common/parameter.py index e83627eea..1ce2b4708 100644 --- a/common/parameter.py +++ b/common/parameter.py @@ -122,6 +122,7 @@ def __init__(self, plugin_info: PortInfo, value: float, binding: str | None, ins # is the last value the single writer (mod-ui) echoed back; the gap from # _value is `pending`. self._observers: list[Callable[[Parameter], None]] = [] + self._settled_observers: list[Callable[[Parameter], None]] = [] self._value: float = float(value) self._confirmed: float = float(value) # Where local edits go upstream. None = display-only: reconciled from @@ -168,27 +169,31 @@ def pending(self) -> bool: return self._value != self._confirmed def reconcile(self, value: float) -> None: - """Adopt the single writer's value: repaint and mark confirmed, publish - nothing. Also the optimistic-preview channel — the dialog scrubs by - reconciling its own value, which is why commit can't gate on change.""" + """Adopt the single writer's value: repaint, mark confirmed, settle, + publish nothing. The settle 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() def preview(self, value: float) -> None: """An optimistic local move not yet committed — a knob mid-turn whose CC - emit is the real send. Repaint; leave the edit unconfirmed; publish - nothing through the sink.""" + emit is the real send, a footswitch keycap already toggled by the press. + Repaints live observers; does not settle; publishes nothing.""" self._set(value) def commit(self, value: float) -> None: - """A finished local edit: repaint, then publish through the sink. Roll - back to the last confirmed value if the send never leaves — otherwise the - LCD would show a number mod-ui never accepted. Publishing is - unconditional; preview and reconcile share mechanics, so there is nothing - to diff against here.""" + """A finished local edit: repaint, publish through the 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. + Publishing is unconditional; preview and reconcile share mechanics, so + there is nothing to diff against here.""" self._set(value) if self.sink is not None and not self.sink.publish(self): self._set(self._confirmed) + return + self._notify_settled() def _set(self, value: float) -> None: if value == self._value: @@ -197,6 +202,10 @@ def _set(self, value: float) -> None: for observe in self._observers: observe(self) + def _notify_settled(self) -> None: + for observe in self._settled_observers: + observe(self) + def subscribe(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: """Register *cb* to fire on every changed-value write. Returns its own unsubscriber. An unchanged write (v == current) does not notify.""" @@ -208,6 +217,19 @@ def _unsub() -> None: pass return _unsub + def subscribe_settled(self, cb: Callable[[Parameter], None]) -> Callable[[], None]: + """Register *cb* for settled 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) + def _unsub() -> None: + try: + self._settled_observers.remove(cb) + except ValueError: + pass + return _unsub + def get_enum_value_list(self) -> list[tuple[str, float]]: return [(v["label"], v["value"]) for v in self.enum_values] diff --git a/modalapi/plugin.py b/modalapi/plugin.py index 80b625c64..f9bc69d90 100755 --- a/modalapi/plugin.py +++ b/modalapi/plugin.py @@ -24,7 +24,7 @@ from common.color import RectBorder from common.parameter import BYPASS_SYMBOL, Parameter, Symbol, json_default from modalapi.plugin_customization import PluginCustomization, PluginExtraData -from pistomp.controller import Controller, StatefulController +from pistomp.controller import Controller if TYPE_CHECKING: from plugins.base import PluginPanel @@ -133,20 +133,12 @@ def toggle_bypass(self) -> float: return new_value def set_param_value(self, symbol: Symbol, value: float) -> None: - """Cache a param's value from mod-ui and mirror it onto any bound - footswitch. The mirror is unconditional (outside the idempotent setter) - because a MIDI-originated echo arrives at the same value we already - wrote optimistically — the setter skips it, but the footswitch keycap - still needs to update. See plan: the mod-ui MIDI echo asymmetry.""" + """Reconcile a param to mod-ui's value. Any bound stateful controller + (a footswitch keycap) resyncs through its own subscription, so this + doesn't reach for controllers.""" param = self.parameters.get(symbol) - if param is None: - return - param.reconcile(value) - for c in self.controllers: - # Only stateful controllers hold a presentation copy to sync (a - # footswitch keycap, a pot's reading). Encoders own no copy. - if c.parameter is param and isinstance(c, StatefulController): - c.set_value(value) + if param is not None: + param.reconcile(value) def set_bypass(self, bypass: bool) -> None: self.set_param_value(BYPASS_SYMBOL, 1.0 if bypass else 0.0) diff --git a/pistomp/controller.py b/pistomp/controller.py index fa117d106..05c62bb69 100755 --- a/pistomp/controller.py +++ b/pistomp/controller.py @@ -113,3 +113,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 + # 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)) diff --git a/tests/v3/test_reactive_parameter.py b/tests/v3/test_reactive_parameter.py index 638da0fff..92e43ab50 100644 --- a/tests/v3/test_reactive_parameter.py +++ b/tests/v3/test_reactive_parameter.py @@ -173,6 +173,37 @@ def publish(self, param: Parameter) -> bool: assert seen == [150.0, 120.0] # painted optimistically, then reverted +def test_settled_fires_on_reconcile_and_commit_not_preview(): + """subscribe_settled 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)) + + p.preview(130.0) + assert settled == [] # a scrub does not settle + + p.reconcile(130.0) # echo confirming the previewed value — unchanged + assert settled == [130.0] # ...still settles, unconditionally + + class OkSink: + def publish(self, param: Parameter) -> bool: + return True + + p.sink = OkSink() + p.commit(140.0) + assert settled == [130.0, 140.0] + + class DeadSink: + def publish(self, param: Parameter) -> bool: + return False + + p.sink = DeadSink() + p.commit(150.0) + assert settled == [130.0, 140.0] # rolled back — did not settle + + def test_subscribe_returns_unsubscriber(): """The returned callable tears down the subscription.""" info: PortInfo = {"shortName": "x", "symbol": "x", "ranges": {"minimum": 0, "maximum": 1}} From 83624d17fa992a27d9eb046adb50bfd50367cb82 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 24 Jul 2026 01:51:01 -0400 Subject: [PATCH 5/6] refactor: route commits through provenance sinks, not a dispatch chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parameter_value_commit re-derived a param's upstream on every commit — a five-arm if-chain over transport/plugin/audio/external. Give each parameter its sink at bind time instead: _sink_for maps provenance to a channel (_PluginParamSink, _ExternalCcSink, _AudioParamSink, _TransportBpmSink), and the commit collapses to param.commit(value). A param edited before bind, or added to a live board, gets its sink on first commit. The plugin path no longer reconciles on a local commit: it stays pending until mod-ui echoes it back, matching the single-writer discipline the other sinks already follow. Co-Authored-By: Claude Opus 4.8 --- modalapi/modhandler.py | 140 ++++++++++++++++++++++++++--------------- 1 file changed, 90 insertions(+), 50 deletions(-) diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 233675ed6..3b8e4aa26 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -57,6 +57,7 @@ TapTempoEffect, ) from common.parameter import BYPASS_SYMBOL, Parameter, PortInfo, Symbol +from common.param_source import ParamSink from common.parameter_steps import ParameterSteps, effective_multiplier from modalapi.plugin import Plugin from blend.input_controller import InputController @@ -146,6 +147,40 @@ def publish(self, param: Parameter) -> bool: return self._send(param.value) +class _PluginParamSink: + """A plugin control port's upstream channel: param_set over the WebSocket. + Returns whether the send left so a failed commit rolls the LCD back.""" + + def __init__(self, send: Callable[[Parameter], bool]): + self._send = send + + def publish(self, param: Parameter) -> bool: + return self._send(param) + + +class _ExternalCcSink: + """An externally-routed param's upstream channel: a raw CC to outboard gear. + No mod-host counterpart, so nothing echoes it back.""" + + def __init__(self, emit: Callable[[Parameter], bool]): + self._emit = emit + + def publish(self, param: Parameter) -> bool: + return self._emit(param) + + +class _AudioParamSink: + """An audio-card level's upstream channel: a local ALSA write, no remote echo, + so the send always lands.""" + + def __init__(self, commit: Callable[[Symbol, float], None]): + self._commit = commit + + def publish(self, param: Parameter) -> bool: + self._commit(param.symbol, param.value) + return True + + def _remove_binding_row(layer: ContextLayer, binding_id: str) -> None: # Drop any PEDALBOARD-layer row whose control.id matches a learned binding # that's being replaced. Scans all event_kind buckets since a re-learn could @@ -1234,12 +1269,54 @@ def bind_current_pedalboard(self): # The pedalboard data has already been loaded, but this will overlay # any real time settings self._controller_manager.bind(self.current) - self._attach_transport_bpm_sink() + self._attach_sinks() + + def _attach_sinks(self) -> None: + """Give every editable parameter its upstream channel, keyed by + provenance (see `_sink_for`). Eager over the whole board so an encoder + turn — which commits with no dialog in between — finds a sink already + there; dialog commits fall back to a lazy attach for hand-built params.""" + if self._current is None: + return + # transport_plugin is kept out of .plugins (the effect graph must not + # paint it), so reach it explicitly for :bpm's sink. + boards = [*self.current.pedalboard.plugins, self.current.pedalboard.transport_plugin] + for plugin in boards: + for param in plugin.parameters.values(): + param.sink = self._sink_for(param) + for controller in self.hardware.controllers.values(): + if controller.parameter is not None: + controller.parameter.sink = self._sink_for(controller.parameter) + if self.volume_parameter is not None: + self.volume_parameter.sink = self._sink_for(self.volume_parameter) + + def _sink_for(self, param: Parameter) -> ParamSink | None: + """The upstream channel a param's commit rides, by provenance. None is + display-only: reconciled from mod-ui, never sent back — the transport's + :bpb/:rolling designations, which mod-ui rejects on param_set.""" + if _is_transport_bpm(param): + return _TransportBpmSink(self.set_mod_tap_tempo) + if param.instance_id == ExternalMidi.EXTERNAL_INSTANCE_ID: + return _ExternalCcSink(self._emit_external_cc) + if param.instance_id is None: + return _AudioParamSink(self.audio_parameter_commit) + if param.instance_id == Pedalboard.TRANSPORT_INSTANCE_ID: + return None + return _PluginParamSink(self._publish_plugin_param) - def _attach_transport_bpm_sink(self) -> None: - if self._current is not None: - bpm_param = self.current.pedalboard.transport_plugin.parameters[BPM_SYMBOL] - bpm_param.sink = _TransportBpmSink(self.set_mod_tap_tempo) + def _publish_plugin_param(self, param: Parameter) -> bool: + if self._is_pedalboard_loading or self.ws_bridge is None or param.instance_id is None: + return False + return self.ws_bridge.send_parameter(param.instance_id, param.symbol, param.value) + + def _emit_external_cc(self, param: Parameter) -> bool: + if param.binding is None: + return False + controller = self.hardware.controllers.get(param.binding) + if controller is None: + return False + self._emit_midi(controller, int(param.value)) + return True def _redraw_after_binding(self, controller: Controller, is_footswitch: bool) -> None: if is_footswitch: @@ -1443,51 +1520,14 @@ def effective_table(self) -> ContextStack: # # Parameter Stuff # - def parameter_value_commit(self, param, value): - # :bpm carries its own sink (transport-bpm WebSocket); edit() sets the - # value and publishes through it. It is neither a plugin control port - # nor an audio/external param, so it exits before those arms. - if _is_transport_bpm(param): - param.commit(value) - return - - # Route plugin params through the plugin's mirror so a bound footswitch - # reconciles now, not only on the mod-host echo — the same set_value the - # ParamSetMessage arm runs. Audio/external params have no plugin mirror. - plugin = ( - next((p for p in self.current.pedalboard.plugins if p.instance_id == param.instance_id), None) - if param.instance_id is not None and self._current is not None - else None - ) - if plugin is not None: - plugin.set_param_value(param.symbol, value) - else: - param.preview(value) - - # Audio parameter (volume, EQ, etc.) - handled locally, no remote update needed - if param.instance_id is None: - self.audio_parameter_commit(param.symbol, value) - return - - # External MIDI parameters have no mod-host counterpart. The dialog's NAV - # path owns sending the CC that _handle_encoder would have sent for a turn; - # the binding table identifies which controller carries it. - if param.binding is not None: - winner = self.effective_table.resolve( - ControlRef(cls=ControlClass.ANALOG, id=param.binding), EventKind.ROTATE - ) - if winner is not None and any(isinstance(e, MidiCcEffect) for e in winner.effects): - controller = self.hardware.controllers.get(param.binding) - if controller is not None: - self._emit_midi(controller, int(value)) - return - - # mod-ui rejects param_set on the /pedalboard transport designations - # (:bpm/:bpb/:rolling) — they travel by CC or the dedicated transport-* - # commands, never here. bpm's dialog commit reaches mod-ui reactively. - if not self._is_pedalboard_loading and param.instance_id is not None \ - and param.instance_id != Pedalboard.TRANSPORT_INSTANCE_ID: - self.ws_bridge.send_parameter(param.instance_id, param.symbol, param.value) + def parameter_value_commit(self, param: Parameter, value: float) -> None: + # The sink owns the route (WebSocket param_set, transport-bpm, external + # CC, local ALSA write); commit repaints, publishes through it, settles. + # Bind attaches sinks eagerly; a param edited before then (or added to a + # live board) gets one on first commit. + if param.sink is None: + param.sink = self._sink_for(param) + param.commit(value) @property def wifi_ip(self) -> str | None: From a5358877060e0bef9239566a38752da8af1a69b6 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 24 Jul 2026 02:21:54 -0400 Subject: [PATCH 6/6] refactor: unify the encoder turn on commit; one transport per bound param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _handle_encoder forked bpm (commit through its WebSocket sink) from every other bound param (preview + a separate CC emit). Drop the fork: a bound turn is c.parameter.commit(new_value), and the param's sink picks the transport — _MidiCcSink for a mapped encoder, _TransportBpmSink for :bpm (its range won't survive 7 bits). The unbound turn keeps its fallback CC for MIDI-learn. _MidiCcSink absorbs the old _ExternalCcSink: a mapped plugin param and an external route are the same send now. The param carries the value; the controller carries the MIDI mechanics — bar_midi_value splits into a pure to_midi(value) so the sink reads the value off the param and asks the controller only to convert and route. The parameter stays MIDI-agnostic. Co-Authored-By: Claude Opus 4.8 --- modalapi/modhandler.py | 78 ++++++++++++++--------------- pistomp/encoder_controller.py | 19 ++++--- tests/v3/test_transport_bindings.py | 5 +- 3 files changed, 53 insertions(+), 49 deletions(-) diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 3b8e4aa26..ad316583a 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -158,15 +158,21 @@ def publish(self, param: Parameter) -> bool: return self._send(param) -class _ExternalCcSink: - """An externally-routed param's upstream channel: a raw CC to outboard gear. - No mod-host counterpart, so nothing echoes it back.""" - - def __init__(self, emit: Callable[[Parameter], bool]): +class _MidiCcSink: + """A MIDI-mapped param's upstream channel: the CC its encoder emits. For a + pedalboard mapping mod-ui applies its map and echoes param_set (reconciling + us); for an external route the CC drives outboard gear. Either way the CC is + the whole send and it always lands — so an encoder turn and its dialog agree + on one transport. The param carries the value; the controller carries the + MIDI mechanics (7-bit conversion, channel, routing).""" + + def __init__(self, controller: EncoderController, emit: Callable[[Controller, int], None]): + self._controller = controller self._emit = emit def publish(self, param: Parameter) -> bool: - return self._emit(param) + self._emit(self._controller, self._controller.to_midi(param.value)) + return True class _AudioParamSink: @@ -463,33 +469,26 @@ def _handle_encoder(self, event: EncoderEvent) -> bool: # Resolve the binding row for badge shadow_state (side effect), even # though the effect type no longer branches the encoder-turn response. - # CC is the transport for a plugin-bound (MIDI-learned) encoder; mod-ui - # applies its mapping on receipt. The local param.value write drives - # reactive observers; the CC tail below is the sole transport to mod-host. if c.midi_CC is not None: self.effective_table.resolve( ControlRef(cls=ControlClass.ANALOG, id=f"{c.midi_channel}:{c.midi_CC}"), EventKind.ROTATE, ) if c.parameter is not None: + # One transport per bound turn: the param's sink (CC to mod-host for + # a mapped encoder, the WebSocket for :bpm) owns the send. bind + # attaches it; a hand-built encoder gets it on first turn. + if c.parameter.sink is None: + c.parameter.sink = self._sink_for(c.parameter, c) new_value = ParameterSteps.for_parameter(c.parameter).move(delta) - # :bpm rides its own WebSocket sink — 20..280 does not survive 7 - # bits — and must not also emit CC. Every other bound param's CC is - # its transport to mod-host, so it falls through to the emit below. - if _is_transport_bpm(c.parameter): - c.parameter.commit(new_value) - self.lcd.display_parameter_value(c.parameter, new_value) - return True - c.parameter.preview(new_value) + c.parameter.commit(new_value) self.lcd.display_parameter_value(c.parameter, new_value) - emit_value = c.bar_midi_value() - else: - emit_value = self._advance_encoder_fallback(c, delta) + return True - # Unconditional, and must stay that way: an unbound encoder has no row, - # and this emit is the only way mod-ui sees its CC to MIDI-learn it. - # Emission is hardware-level, below the table (see input/README.md). - self._emit_midi(c, emit_value) + # Unbound: no sink, no row. This fallback CC is the only way mod-ui sees + # the encoder to MIDI-learn it. Emission is hardware-level, below the + # table (see input/README.md). + self._emit_midi(c, self._advance_encoder_fallback(c, delta)) return True def encoder_fallback(self, controller: EncoderController) -> int: @@ -1286,21 +1285,29 @@ def _attach_sinks(self) -> None: param.sink = self._sink_for(param) for controller in self.hardware.controllers.values(): if controller.parameter is not None: - controller.parameter.sink = self._sink_for(controller.parameter) + controller.parameter.sink = self._sink_for(controller.parameter, controller) if self.volume_parameter is not None: self.volume_parameter.sink = self._sink_for(self.volume_parameter) - def _sink_for(self, param: Parameter) -> ParamSink | None: + def _sink_for(self, param: Parameter, controller: Controller | None = None) -> ParamSink | None: """The upstream channel a param's commit rides, by provenance. None is - display-only: reconciled from mod-ui, never sent back — the transport's - :bpb/:rolling designations, which mod-ui rejects on param_set.""" + display-only: reconciled from mod-ui, never sent back — bpb/rolling when + unmapped (mod-ui rejects param_set on them) and an external footswitch + (its press path owns the CC). A param mapped to an encoder rides that + encoder's CC; :bpm is the exception — its range won't fit 7 bits, so it + keeps the WebSocket. Pass *controller* when the caller holds it (an + encoder turn); otherwise it's recovered from the binding.""" if _is_transport_bpm(param): return _TransportBpmSink(self.set_mod_tap_tempo) - if param.instance_id == ExternalMidi.EXTERNAL_INSTANCE_ID: - return _ExternalCcSink(self._emit_external_cc) if param.instance_id is None: return _AudioParamSink(self.audio_parameter_commit) - if param.instance_id == Pedalboard.TRANSPORT_INSTANCE_ID: + enc = controller if isinstance(controller, EncoderController) else None + if enc is None and param.binding is not None: + enc = self.hardware.controllers.get(param.binding) + enc = enc if isinstance(enc, EncoderController) else None + if enc is not None and enc.midi_CC is not None: + return _MidiCcSink(enc, self._emit_midi) + if param.instance_id in (ExternalMidi.EXTERNAL_INSTANCE_ID, Pedalboard.TRANSPORT_INSTANCE_ID): return None return _PluginParamSink(self._publish_plugin_param) @@ -1309,15 +1316,6 @@ def _publish_plugin_param(self, param: Parameter) -> bool: return False return self.ws_bridge.send_parameter(param.instance_id, param.symbol, param.value) - def _emit_external_cc(self, param: Parameter) -> bool: - if param.binding is None: - return False - controller = self.hardware.controllers.get(param.binding) - if controller is None: - return False - self._emit_midi(controller, int(param.value)) - return True - def _redraw_after_binding(self, controller: Controller, is_footswitch: bool) -> None: if is_footswitch: # Footswitch: redraw just that one switch, not the whole board. diff --git a/pistomp/encoder_controller.py b/pistomp/encoder_controller.py index c339163c0..85755c021 100644 --- a/pistomp/encoder_controller.py +++ b/pistomp/encoder_controller.py @@ -135,19 +135,22 @@ 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" + midi_value = util.renormalize( + value, self.parameter.minimum, self.parameter.maximum, 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" - midi_value = util.renormalize( - self.parameter.value, - self.parameter.minimum, - self.parameter.maximum, - self.midi_min, - self.midi_max, - ) - return int(_clamp(midi_value, 0, 127)) + return self.to_midi(self.parameter.value) def _compute_multiplier(self, rotations: int) -> float: now = time.monotonic() diff --git a/tests/v3/test_transport_bindings.py b/tests/v3/test_transport_bindings.py index 7385121f2..e0ecc99a0 100644 --- a/tests/v3/test_transport_bindings.py +++ b/tests/v3/test_transport_bindings.py @@ -549,9 +549,12 @@ def test_encoder_bpb_turn_still_emits_midi_cc(v3_system: SystemFixture, make_plu 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(":") - tp = _attach_transport_plugin(handler, bpb_cc={"channel": int(channel), "control": int(cc)}) + # Mock before binding: the CC sink captures its emitter when the encoder binds. handler._emit_midi = MagicMock() + tp = _attach_transport_plugin(handler, bpb_cc={"channel": int(channel), "control": int(cc)}) + handler._emit_midi.reset_mock() + handler._handle_encoder(EncoderEvent(controller=enc1, rotations=1, multiplier=1.0)) assert tp.parameters[BPB_SYMBOL].value == 5.0