diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 86015b4bf..6075719ab 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -728,8 +728,11 @@ def preset_change(self, index): logging.error("Bad Rest request: %s" % url) self.current.preset_index = index - # Update name on lcd + # Update name on lcd, and relight any footswitch mapped to a snapshot. self.lcd.draw_title() + for fs in self.hardware.footswitches: + if fs.preset_callback_arg is not None: + self.lcd.update_footswitch(fs) # Bypass/param changes from the snapshot arrive via the WS drain (source of truth). def preset_incr_and_change(self, *argv): diff --git a/pistomp/footswitch.py b/pistomp/footswitch.py index 9595ece83..f4fef88c7 100755 --- a/pistomp/footswitch.py +++ b/pistomp/footswitch.py @@ -128,7 +128,7 @@ def __init__(self, id, led_pin, pixel, midi_CC, midi_channel, midiout, refresh_c def get_display_label(self): if self.taptempo and self.taptempo.is_enabled(): return str(round(self.taptempo.get_bpm())) - elif self.midi_CC is None: + elif self.midi_CC is None and self.preset_callback_arg is None: return "" else: return self.display_label @@ -149,10 +149,10 @@ def drives_display(self) -> bool: def set_value(self, bypass_value: float): self.toggled = (bypass_value < 1) - self._set_led(self.toggled) + self.set_led(self.toggled) self.refresh_callback(footswitch=self) - def _set_led(self, enabled): + def set_led(self, enabled): if self.led is not None: if self.taptempo: tempo = self.taptempo.get_bpm() @@ -228,7 +228,7 @@ def pressed(self, state): r.enable() else: r.disable() - self._set_led(self.toggled) + self.set_led(self.toggled) self.refresh_callback(True) # True means this is a bypass change only else: # TODO consider case where relay and longpress are specified @@ -257,7 +257,7 @@ def pressed(self, state): logging.debug("Sending CC event: %d" % self.midi_CC) self.midiout.send_message(cc) if self.drives_display: - self._set_led(self.toggled) + self.set_led(self.toggled) if self.drives_display: self.refresh_callback(footswitch=self) @@ -282,4 +282,6 @@ def clear_pedalboard_info(self): self.display_label = None self.set_category(None) self.preset_callback = None + self.preset_callback_arg = None + self.parameter = None self.clear_relays() diff --git a/pistomp/hardware.py b/pistomp/hardware.py index fa385a18c..5f9d0b61c 100755 --- a/pistomp/hardware.py +++ b/pistomp/hardware.py @@ -347,6 +347,13 @@ def __init_footswitches_default(self): fs.clear_relays() self.__init_footswitches(self.cfg) + def __clear_footswitch_midi_cc(self, fs) -> None: + fs.set_midi_CC(None) + for k, v in self.controllers.items(): + if v == fs: + self.controllers.pop(k) + break + def __init_footswitches(self, cfg): if cfg is None or (Token.HARDWARE not in cfg) or (Token.FOOTSWITCHES not in cfg[Token.HARDWARE]): return @@ -380,11 +387,7 @@ def __init_footswitches(self, cfg): if Token.MIDI_CC in f: cc = f[Token.MIDI_CC] if cc == Token.NONE: - fs.set_midi_CC(None) - for k, v in self.controllers.items(): - if v == fs: - self.controllers.pop(k) - break + self.__clear_footswitch_midi_cc(fs) else: fs.set_midi_channel(self.midi_channel) fs.set_midi_CC(cc) @@ -393,6 +396,7 @@ def __init_footswitches(self, cfg): # Preset Control if Token.PRESET in f: + self.__clear_footswitch_midi_cc(fs) preset_value = f[Token.PRESET] if preset_value == Token.UP: fs.add_preset(callback=self.handler.preset_incr_and_change) diff --git a/pistomp/lcd320x240.py b/pistomp/lcd320x240.py index 77fa9612e..d779a5caa 100644 --- a/pistomp/lcd320x240.py +++ b/pistomp/lcd320x240.py @@ -478,7 +478,10 @@ def parameter_commit_enum(self, param_value_tuple): # Footswitches # def footswitch_label(self, footswitch): - """Label for a footswitch bound to a plugin param: the param name, or the plugin instance for a :bypass binding.""" + """Label for a footswitch bound to a snapshot index or a plugin param.""" + if footswitch.preset_callback_arg is not None: + name = self.current.presets.get(footswitch.preset_callback_arg) if self.current else None + return self.shorten_name(name, self.footswitch_width) if name else str(footswitch.preset_callback_arg) param = footswitch.parameter if param is None: return None @@ -489,6 +492,10 @@ def footswitch_label(self, footswitch): def draw_footswitch(self, plugin): for c in plugin.controllers: if isinstance(c, Footswitch): + if c.preset_callback_arg is not None: + # Preset-bound switches are drawn by draw_unbound_footswitches, + # regardless of any (stale) plugin binding. + continue fs_id = c.id #fss[fs_id] = None label = self.footswitch_label(c) @@ -509,12 +516,23 @@ def draw_unbound_footswitches(self): if fs.id in self.footswitch_slots: continue slot = fs.id - dl = fs.get_display_label() - label = "" if dl is None else dl + if fs.preset_callback_arg is not None: + label = self.footswitch_label(fs) + fs.set_display_label(label) + active = self.current is not None and self.current.preset_index == fs.preset_callback_arg + fs.toggled = active + fs.set_led(active) # a press never touches toggled for preset switches + color = self.foreground + is_bypassed = not fs.toggled + else: + dl = fs.get_display_label() + label = "" if dl is None else dl + color = None + is_bypassed = True y = 0 x = self.get_footswitch_pitch() * slot p = FootswitchWidget(Box.xywh(x, y, self.plugin_width, self.footswitch_height), self.small_font, - label, None, True, parent=self.footswitch_panel, object=fs) + label, color, is_bypassed, parent=self.footswitch_panel, object=fs) self.w_footswitches.append(p) self.footswitch_panel.add_widget(p) self.footswitch_panel.refresh() @@ -522,7 +540,12 @@ def draw_unbound_footswitches(self): def update_footswitch(self, footswitch): for wfs in self.w_footswitches: if wfs.object == footswitch: - if footswitch.parameter is not None: + if footswitch.preset_callback_arg is not None: + footswitch.set_display_label(self.footswitch_label(footswitch)) + active = self.current is not None and self.current.preset_index == footswitch.preset_callback_arg + footswitch.toggled = active + footswitch.set_led(active) + elif footswitch.parameter is not None: # Binding may be new (e.g. MIDI learn) — reflect label + color. footswitch.set_display_label(self.footswitch_label(footswitch)) wfs.color = self.get_category_color(footswitch.category) diff --git a/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/after_preset_change.png b/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/after_preset_change.png new file mode 100644 index 000000000..8476af267 Binary files /dev/null and b/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/after_preset_change.png differ diff --git a/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/before_preset_change.png b/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/before_preset_change.png new file mode 100644 index 000000000..1a33b7cf0 Binary files /dev/null and b/tests/snapshots/v3/test_footswitch_presets/test_preset_change_does_not_blank_the_label/before_preset_change.png differ diff --git a/tests/test_footswitch.py b/tests/test_footswitch.py index c504d5085..bf605b8ee 100644 --- a/tests/test_footswitch.py +++ b/tests/test_footswitch.py @@ -138,3 +138,21 @@ def test_clears_state(self): assert fs.toggled is False assert fs.display_label is None assert fs.preset_callback is None + + def test_clears_preset_callback_arg(self): + with _make_footswitch(midi_CC=None) as fs: + fs.add_preset(callback=MagicMock(), callback_arg=5) + fs.set_display_label("Lead") + + fs.clear_pedalboard_info() + + assert fs.preset_callback_arg is None + assert fs.get_display_label() == "" + + def test_clears_parameter(self): + with _make_footswitch(midi_CC=None) as fs: + fs.parameter = MagicMock() # pyright: ignore[reportAttributeAccessIssue] + + fs.clear_pedalboard_info() + + assert fs.parameter is None diff --git a/tests/test_lcd320x240.py b/tests/test_lcd320x240.py index 21542056b..05447bfdc 100644 --- a/tests/test_lcd320x240.py +++ b/tests/test_lcd320x240.py @@ -13,6 +13,8 @@ class MockObject: + preset_callback_arg = None # footswitch default; override via kwargs + def __init__(self, **kwargs): self.__dict__.update(kwargs) diff --git a/tests/v3/test_footswitch_presets.py b/tests/v3/test_footswitch_presets.py new file mode 100644 index 000000000..3efc0211a --- /dev/null +++ b/tests/v3/test_footswitch_presets.py @@ -0,0 +1,213 @@ +"""Footswitches mapped directly to a snapshot index (`preset: ` in config). + +Covers four behaviors: + 1. The label shows the snapshot name, not the raw index. + 2. `Hardware.__init_footswitches` clears any midi_CC (default-config or + override) when `preset:` is configured, since a preset footswitch's own + press dispatch always short-circuits on preset_callback before ever + reaching midi_CC emission (see Footswitch.pressed) -- an + inherited-but-unused CC otherwise sits in `hw.controllers` where + ControllerManager.bind() can match it against an unrelated plugin's + MIDI-learned binding and steal fs.parameter. + 3. The label survives even if `fs.parameter` still ends up set by some + other path -- defense in depth on top of (2), so + `draw_footswitch`/`draw_unbound_footswitches`/`update_footswitch` never + let a plugin/param name clobber a preset label. + 4. The footswitch's LED/indicator lights only when its mapped snapshot is + the currently active one. +""" + +import yaml +from unittest.mock import MagicMock + +from common.parameter import Parameter +from tests.types import SystemFixture + + +def _plugin_param() -> Parameter: + p = MagicMock(spec=Parameter) + p.symbol = ":bypass" + p.instance_id = "/Reverb" + p.value = 0 + p.minimum = 0 + p.maximum = 1 + return p + + +class TestPresetFootswitchLabel: + def test_shows_snapshot_name_not_index(self, v3_system: SystemFixture): + handler = v3_system.handler + fs = v3_system.hw.footswitches[0] + fs.add_preset(callback=handler.preset_set_and_change, callback_arg=1) + + # v3_system's snapshot/list mock returns {"0": "Clean", "1": "Lead"} + # (footswitch_label lowercases, same as it does for plugin names) + assert handler.lcd.footswitch_label(fs) == "lead" + + def test_falls_back_to_index_when_name_unknown(self, v3_system: SystemFixture): + handler = v3_system.handler + fs = v3_system.hw.footswitches[0] + fs.add_preset(callback=handler.preset_set_and_change, callback_arg=7) + + assert handler.lcd.footswitch_label(fs) == "7" + + +class TestPresetConfigClearsDefaultMidiCC: + """Root-cause coverage: a footswitch config'd with `preset:` must lose any + midi_CC (default-config or override) and drop out of hw.controllers, so it + can never be matched by ControllerManager.bind() against an unrelated + plugin's MIDI-learned parameter.""" + + def test_preset_override_clears_inherited_default_midi_cc(self, v3_system: SystemFixture, tmp_path): + handler = v3_system.handler + hw = v3_system.hw + # default_config_pistomptre.yml gives footswitch 1 midi_CC: 61 with no preset. + fs1 = hw.footswitches[1] + assert fs1.midi_CC == 61 + assert any(v is fs1 for v in hw.controllers.values()) + + bundle_dir = tmp_path / "preset_rig.pedalboard" + bundle_dir.mkdir() + (bundle_dir / "config.yml").write_text( + yaml.dump({"hardware": {"footswitches": [{"id": 1, "preset": 0}]}}) + ) + pb = handler.pedalboards["/path/to/new.pedalboard"] + pb.bundle = str(bundle_dir) + pb.plugins = [] + + handler.set_current_pedalboard(pb) + + assert fs1.midi_CC is None + assert all(v is not fs1 for v in hw.controllers.values()) + + +class TestPresetFootswitchLabelSurvivesParameterBinding: + def test_draw_unbound_footswitches_keeps_snapshot_label(self, v3_system: SystemFixture): + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + fs = hw.footswitches[0] + fs.add_preset(callback=handler.preset_set_and_change, callback_arg=0) + fs.parameter = _plugin_param() # defense-in-depth: simulates fs.parameter + # getting set through some other path despite (2) above + + lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + lcd.draw_main_panel() + + assert fs.get_display_label() == "clean" + + def test_update_footswitch_keeps_snapshot_label(self, v3_system: SystemFixture): + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + fs = hw.footswitches[0] + fs.add_preset(callback=handler.preset_set_and_change, callback_arg=0) + + lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + lcd.draw_main_panel() + + fs.parameter = _plugin_param() # bound after the fact (e.g. MIDI learn) + lcd.update_footswitch(fs) + + assert fs.get_display_label() == "clean" + + +class TestPresetFootswitchIndicator: + def test_active_snapshot_footswitch_drives_physical_led(self, v3_system: SystemFixture): + """A press never touches fs.toggled for preset footswitches + (Footswitch.pressed returns early on the preset_callback branch), so + the LCD redraw path is the only place that can also light the + physical LED/pixel.""" + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + fs0, fs1 = hw.footswitches[0], hw.footswitches[1] + fs0.pixel = MagicMock() + fs1.pixel = MagicMock() + fs0.add_preset(callback=handler.preset_set_and_change, callback_arg=0) + fs1.add_preset(callback=handler.preset_set_and_change, callback_arg=1) + handler.current.preset_index = 1 + + lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + lcd.draw_main_panel() + + fs0.pixel.set_enable.assert_called_once_with(False) + fs1.pixel.set_enable.assert_called_once_with(True) + assert fs0.toggled is False + assert fs1.toggled is True + + def test_active_snapshot_footswitch_is_lit(self, v3_system: SystemFixture): + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + fs0, fs1 = hw.footswitches[0], hw.footswitches[1] + fs0.add_preset(callback=handler.preset_set_and_change, callback_arg=0) + fs1.add_preset(callback=handler.preset_set_and_change, callback_arg=1) + handler.current.preset_index = 1 + + lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + lcd.draw_main_panel() + + w0 = next(w for w in lcd.w_footswitches if w.object is fs0) + w1 = next(w for w in lcd.w_footswitches if w.object is fs1) + assert w1.is_bypassed is False # active snapshot -> lit + assert w0.is_bypassed is True # inactive snapshot -> dark + + def test_preset_change_relights_indicator(self, v3_system: SystemFixture): + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + fs0, fs1 = hw.footswitches[0], hw.footswitches[1] + fs0.add_preset(callback=handler.preset_set_and_change, callback_arg=0) + fs1.add_preset(callback=handler.preset_set_and_change, callback_arg=1) + + lcd.link_data(handler.pedalboard_list, handler.current, hw.footswitches) + lcd.draw_main_panel() + + handler.preset_change(1) + + w0 = next(w for w in lcd.w_footswitches if w.object is fs0) + w1 = next(w for w in lcd.w_footswitches if w.object is fs1) + assert w1.is_bypassed is False + assert w0.is_bypassed is True + + +class TestPresetFootswitchLabelSurvivesRelight: + """Regression: Hardware.__init_footswitches clears midi_CC for preset + footswitches configured via config.yml (see TestPresetConfigClearsDefaultMidiCC), + but Footswitch.get_display_label() treats `midi_CC is None` as "unbound, + show nothing" -- a rule written before preset switches could legitimately + have no CC. Anywhere that reads get_display_label() instead of the + freshly computed label (update_footswitch's `wfs.label = ...` line) blanks + the snapshot-name label the instant it's touched again, e.g. on the very + next preset_change().""" + + def test_preset_change_does_not_blank_the_label(self, v3_system: SystemFixture, tmp_path, snapshot): + handler = v3_system.handler + hw = v3_system.hw + lcd = handler.lcd + + # Footswitch 1 has midi_CC: 61 in default_config_pistomptre.yml -- the + # config overlay path (not fs.add_preset() called directly) is what + # triggers Hardware.__clear_footswitch_midi_cc and reproduces the bug. + bundle_dir = tmp_path / "preset_rig.pedalboard" + bundle_dir.mkdir() + (bundle_dir / "config.yml").write_text( + yaml.dump({"hardware": {"footswitches": [{"id": 1, "preset": 0}]}}) + ) + pb = handler.pedalboards["/path/to/new.pedalboard"] + pb.bundle = str(bundle_dir) + pb.plugins = [] + handler.set_current_pedalboard(pb) + + fs1 = hw.footswitches[1] + assert fs1.midi_CC is None # cleared by the preset config, as expected + + w1 = next(w for w in lcd.w_footswitches if w.object is fs1) + assert w1.label == "clean" # correct immediately after the initial draw + snapshot("before_preset_change") + + handler.preset_change(1) # switches the active snapshot; fs1 stays mapped to 0 + + assert w1.label == "clean" # BUG: get_display_label() blanks it to "" + snapshot("after_preset_change")