Skip to content

feat: add MIDI tempo control and fix tap-tempo UI crashes and paramet…#214

Open
jpremill wants to merge 2 commits into
TreeFallSound:pistomp-v3from
jpremill:add-MIDI-tempo-to-system-menu
Open

feat: add MIDI tempo control and fix tap-tempo UI crashes and paramet…#214
jpremill wants to merge 2 commits into
TreeFallSound:pistomp-v3from
jpremill:add-MIDI-tempo-to-system-menu

Conversation

@jpremill

Copy link
Copy Markdown
  • Add 'Set MIDI Tempo' menu option to the System Menu.
  • display midi tempo in top area of screen; allow users to select and set tempofrom there
  • Defer BPM UI changes to the main thread's fast 10ms tick to prevent Pygame/SPI graphics crashes.
  • Isolate modal panels in PanelStack to prevent background menu bleed-through.
  • Fix a bug where outbound WebSocket traffic (like parameter adjustments) was permanently suppressed at boot.
  • minor ui param adjustment rendering perf improement

…er updates

- Add 'Set MIDI Tempo' menu option to the System Menu.
- Defer BPM UI changes to the main thread's fast 10ms tick to prevent Pygame/SPI graphics crashes.
- Isolate modal panels in PanelStack to prevent background menu bleed-through.
- Fix a bug where outbound WebSocket traffic (like parameter adjustments) was permanently suppressed at boot.
@jpremill

Copy link
Copy Markdown
Author

implements #213

@rreichenbach

Copy link
Copy Markdown
Collaborator

Thanks for putting this together — appreciate you taking the time to work on pi-stomp. This PR seems to bundle a few different fixes/features which makes it hard to review and merge safely (if one piece needs changes, it holds up the rest).

It makes sense IMO to add a system menu item for setting tap tempo (similar to having Tuner available there) for access via navigation instead of footswitch.

@sastraxi has made some changes regarding tap tempo, merged to main branch, which will soon appear in the 3.2 release. For example, he's displaying the tempo like this when set tap tempo is enabled:
tap-tempo

Would that not be sufficient? I guess not ideal if not using footswitch D longpress to get there, but the upper left message area shouldn't be used for any semi-permanent info, that's reserved for temporary status messages and now "tool tip" info when scrolling.

Anyway, let's get on the same page about this and try to keep PR's atomic addressing one thing at a time.

@rreichenbach
rreichenbach requested a review from sastraxi July 20, 2026 18:19
@sastraxi

sastraxi commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

@jpremill thanks for the contribution! As Randall mentioned, we actually have a new mainline branch of development. It's mostly been just me working on the software for the last few months, so the visibility issues are on me.

That being said, there are some challenges with the approach here:

  • <MagicMock ... is getting added to the snapshot tests
  • the top-left area we're now using as a "subtitle" to explain what's currently under the selection cursor; this BPM placement would interfere with that
  • the modal/ws stuff is not necessary anymore due to those having been refactored in recent weeks (of course, no way for you to know this!)
  • the system menu is already getting pretty slammed with features. I want to avoid this being a catch-all

An alternative approach

If I step back and look at your bug report (#213) I think I have a much simpler option: let's use MOD's native ability to bind e.g. a rotary encoder to MIDI:

image

This works out-of-the-box as tweak1/2 by default on v3 send MIDI CCs. Click the highlighted button there, select MIDI, and then move the encoder. I tried this myself, and the binding works.

Two defects:

  1. because of MOD's 20-280 range and MIDI's 128 values (0-127) you get ~2.04 bpm change per detent
  2. no visual feedback!

The second is the bigger issue, and I think it's probably a simple fix as tweak1/2 usually do show one when bound.

My ask

If you're game, first try out the MIDI mapping via MOD learn, then try implementing the above to trigger a "Parameter Dialog" when this value is moved, just like what happens if you bind tweak1/2 to a real effect parameter. This fits in with our design goals for the UI and makes maximum re-use of the existing code!

If the 2bpm-per-detent is a dealbreaker, we need something more bespoke. That would look like updating the config schema to allow taking over a tweak encoder, no longer sending MIDI CCs but instead using WS to send the float value (so we're no longer restricted to 7-bit). Of course, we'd still want to show the parameter dialog for user feedback. Bonus points if you also make it show it when the host changes the BPM too!

If you'd like, start from https://github.com/TreeFallSound/pi-stomp/tree/prerelease -- I'm not 100% sure if this will still deploy cleanly to the latest release image (v3.1.0 available via TreeFallSound/pi-gen-pistomp). If it doesn't, reach out and I'll give you a hand.

@jpremill

Copy link
Copy Markdown
Author

You know that is a good point - I should probably split this into at least two PRs.

Should I not include the test snapshots? That really does quite number on the amount of files changed.

As I read through I do get that these changes don't really fit with what you are trying to do. I definitely fiddled with the tests more that I probably should have. I see the magic mock is the real issue not the snapshots.

I don't think that tap fs4 bpm display works for the purpose I was thinking. If I need to make a quick adjustment to the bpm and the timing is not accurate i figured it would be useful to quickly select and update the bpm which does work pretty well.

The tap tempo on fs4 prevents toggling that effect it feels a little complicated to adjust bpm then toggle the effect on or off. I know this isn't a huge deal but it was enough to get started on the task.

I had no idea that mod UI could do that natively but the 2bpm aspect doesn't solve the problem well as one of the issues I was having was getting to to not be 1bpm offish which is admittedly not likely to be notifed but still concerning.

I will close out the pr, essentially restart on the general task from the pre release and go from there.

One thing I had considered in that upper left area was to only show the bpm if the area is just not in use.

It does still feel like there should be a way to adjust the tempo without committing the encoder mapping but I can't argue with the issue that this does way too much and some of the concerns have already been addressed.

Would you be open to moving the bpm and tuner to a utilities sub menu? It could eventually expand to include other functionality.

When are you targeting the v3.2.0 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants