Pre/Bypass switch color halo on LCD

[benh]

Hi !

Any opinion on the following patch ? Ie makes the halo of the Pre/Bypass switch on the LCD (leftmost by default) follow the corresponding LED if any and bypass state, which ... I prefer :-) IE, I actually get some color there !

I'll add it to a future pull request if you don't object.

Code: [Select]

diff --git a/pistomp/lcdbase.py b/pistomp/lcdbase.py
index cf56f31..7bb4d99 100755
--- a/pistomp/lcdbase.py
+++ b/pistomp/lcdbase.py
@@ -188,6 +188,8 @@ class Lcdbase(abstract_lcd.Lcd):
                 continue
             f = fss[fs_id]
             color = self.valid_color(f.lcd_color)
+            if self.color_plugin_bypassed is not None and not f.enabled:
+                color = self.color_plugin_bypassed
             label = "" if f.display_label is None else f.display_label
             x = self.footswitch_pitch[len(fss)] * fs_id
             self.draw_plugin(zone, x, 0, label, self.footswitch_width, False, None, True, color)
diff --git a/pistomp/lcdcolor.py b/pistomp/lcdcolor.py
index 1c23b06..9955bda 100755
--- a/pistomp/lcdcolor.py
+++ b/pistomp/lcdcolor.py
@@ -254,7 +254,7 @@ class Lcdcolor(lcdbase.Lcdbase):
         if is_footswitch:
             if plugin:
                 plugin.lcd_xyz = (xy1, xy2, zone)
-            c = self.color_plugin_bypassed if plugin is None or plugin.is_bypassed() else color
+            c = self.color_plugin_bypassed if plugin is not None and plugin.is_bypassed() else color
             self.draw_footswitch(xy1, xy2, zone, text, c)
         elif plugin:
             plugin.lcd_xyz = (xy1, xy2, zone)


[Randall (Admin)]

Maybe I'm not understanding the use case you're trying to address here.  I could maybe imagine the halo indicating relay bypass (even though we already have the toolbar indicator for that), but it looks like the code here isn't related to relay bypass.

Also, the default_config.yml color property for a footswitch is meant to be the enabled color, not a color used to identify the switch regardless of enable/disable.  Maybe you're wanting more of an identification color though?

Could you show the default_config.yml entry for the footswitch you're trying to change and explain what you expect the color to be and what its supposed to be indicating? 
 

[benh]

So unless I misunderstand something or have a weird behaviour... today, a switch that isn't associated with a plugin (Pre+/bypass or a MIDI only switch) will have its halo on the LCD always "grey" (color_plugin_bypassed) regardless of the state of the switch and regardless of the color configured in the .yml.

The physical LED halo (I think from reading the code as I haven't wired those to the GPIOs up yet) however will follow the switch state.

This patch makes those non-plugin switches halo on the "LCD" follow the state of the switch just like the GPIO halo (if any) instead of  just being "always grey".  For this to work, a color needs to be specified in the config, so I suppose I could improve the patch to test for that and stick to the existing behaviour when not.

In the case of the Pre+/bypass footswitch, yes, there is already an indication of the bypass state via the little power icon, but this is not nearly as visible (at least for me) when standing up near the pedal, and I feel having the switch halo reflect the switch state is a more "expected" behaviour from a pedal...

But maybe I'm missing something here.

[benh]

Digging more there are issues with my approach... when the switch isn't associated with presets, it starts toggling the halo on short stomps which doesn't reflect anything.

I feel the code in those areas has ... evolved a bit :-) I need a bit more time to untangle. I am not fan of how the LED is controlled by calls to set_led() without any relation to the "enabled" state of the switch. The disconnect between the LED and the displayed halo is what bugs me. I would think that in absence of physical LEDs the halos should behave like the physical LEDs do.

So I wonder whether I could try (if you don't object) to rework things in that area a bit such that:

 - The LCD halo and the LED are always in the same state and reflect footswitch.enabled

 - Some logic is added so that footswitch.enabled doesn't "toggle" for a non-plugin switch which has no MIDI message and no preset, such that the leftmost switch "enabled" will effectively only toggle when doing long-presses for bypass control.

I will need more time to get my head around the various code path involved and make sure I can produce the above without breaking stuff, but I would appreciate your opinion as to whether this is an approach you would accept.

I think fundamentally this will turn into anything calling set_led from outside footswitch.py today turns into a new set_state which actually changes the enabled state. set_led becomes private. Then I'll add logic to figure out when a switch should toggle, ie prevent toggles when not associated with a midi control or a plugin.

[benh]

So the end result is in https://github.com/ozbenh/pi-stomp/commits/benh-hackery

(along with the rest of my experimental stuff)

footswitch.set_value() already existed, so the change wasn't super invasive. The behaviour of non-relay midi toggles shouldn't change. The changes should (hopefully only) be
that for the relay switch, halo and LED (if any) are in sync and match footswitch.enabled.

[Randall (Admin)]

Thanks for your clarification and code change.  I think I agree with your assertions.  Yes, the code did grow a bit ugly in that regard over time.

Your change looks good via visual inspection.  My main concern was that I didn't want the halos toggling when a footswitch is defined to change the preset, and it looks like you realized that too.

I'll need to test it.  On v1 hardware too since footswitch.py and mod.py are common to both. 

[benh]

Thanks. Yes of course, it does need testing :-) It's in the same branch as my "test" backend, so feel free to look at both when you have a chance. I rebase that branch regularly (it's a "work in progress" pile), I've added quite a few things to the test backend since last I mentioned it.

The main difference with v1 is what ? LCD ? My change to lcdcolor.py might need a matching change there then... a quick look indicates lcdgfx.py probably needs some work, maybe more.

[Randall (Admin)]

Wow, that test mode is super slick!  Very excited to get that in.

Things are looking good so far.  Still want to try some things tomorrow with audio and complete v1 testing.  But I'll likely accept the current PR as is, then you could queue up your next change.  It would be nice to have separate github PR's/Issues for each change if possible.

Yes the main difference with v1 is the LCD (monochrome).  It also has a second encoder and the behavior and state machines (in mod.py) are different.  The encoders are handling fast rotations better with your interrupt change included.

The v1 footswitch behavior is probably fine as is.  The v1 LCD includes markers instead of a "halo" for indicating if a plugin is enabled.  It looks somewhat odd adding that marker to a footswitch placeholder that is not bound to a plugin.  Perhaps for consistency with v2 it's worth having lcdgfx.draw_bound_plugins() call draw_plugin(), but not a big deal either way.  An unbound footswitch only has practical use if it's being used to control external gear via MIDI out - a valid but fairly rare usecase at this point.

[benh]

The stuff in the hackery branch will be split into separate PRs yes. As for the existing PR with the encoder interrupts & setup script fix, do you want me to split that one into two separate PRs ?

[Randall (Admin)]

Nah, that's fine.  I'll be finishing work in the next few hours and then plan to finish up testing the encoder change.

[benh]

BTW. You mentioned you had problems using interrupts before. I don't know whether you are aware or not, but I believe RPi GPIO uses a separate thread to handle GPIO interrupts, and the callbacks are called from the context of that thread. This means that unless you are careful about thread synchronization, bad things can happen. This is why I implemented it the way I did where the callback only handles an atomic counter which is read from the normal polling loop.

In fact I think we have a potential problem with the footswitch for that reason. "toggle" should probably just set a flag that we read from the main poller.

If you want faster "response" than the 10ms loop, we could replace the sleep there with something we can wake up from those callbacks. Either that or use a global lock.

[benh]

Also not sure you noticed, but the test mode does more interesting things if jackd isn't running... ie, I'm currently using Alsa directly so it conflicts