Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add accurate midi timestamps #477

Merged
merged 6 commits into from
Oct 16, 2023
Merged

Add accurate midi timestamps #477

merged 6 commits into from
Oct 16, 2023

Conversation

stephen322
Copy link
Contributor

Add midi timestamps via Mido callback.

I originally wrote this in July/Aug, (and re-wrote now due to the many upstream changes). But never created a pull request because it's all for naught if PixelStrip's render holds the GIL for the duration. On my Pi/ledstrip setup, it's held for a good 11ms, which means any two notes could be recorded off by up to 22ms, enough to be noticeable.

This time I decided to patch rpi-ws281x to prevent GIL locking during render:
rpi-ws281x/rpi-ws281x-python#102
This should make the project much more thread-friendly regardless of the timestamps.

There's also discussion on Mido to use the backend rtmidi's delta timestamps:
https://github.com/orgs/mido/discussions/485
But I'm not sure if those deltas would also be affected by PixelStrip's render lock.

The midi parsing in visualizer.py was also cleaned up a bit.

This allows tracking the accurate time a midi event came in, instead of waiting to timestamp in our potentially slow or laggy event loop.  Also needs a non-thread-blocking led render.
Needed for accurate midi file recording.
@stephen322
Copy link
Contributor Author

Updated to use time.perf_counter():
-Highest resolution time source
-Monotonic
-Typically lower loss of precision due to smaller number for float (although maybe perf_counter_ns() should be considered)

@@ -158,7 +158,7 @@ def log_unhandled_exception(exc_type, exc_value, exc_traceback):
midiports.last_activity = time.time()
hotspot.hotspot_script_time = time.time()

last_control_change = 0
last_sustain = 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being a bit nitpicky, but is there a specific reason for changing the name of this variable? It should be keeping track of every 'control_change,' not just sustain.

value = msg.value

# midi control 64 = sustain pedal
if control == 64:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now the reason for variable change, but why limit it only to a sustain pedal?

Copy link
Contributor Author

@stephen322 stephen322 Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misread intent?
The variable is only ever checked in fade processing, and specifically against "pedal_deadzone" so I assumed that meant sustain.
Hmm, if it's any control, then what about "modulation", "volume", "expression", etc., or any number of private-use CC's?

I was thinking about tracking CC changes in "midi_state" when designing ColorMode interface, but so far nothing is using it, so it remains unimplemented...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I thought you had something else in mind. If it is currently used in only one scenario, the new name is even more fitting.

@onlaj
Copy link
Owner

onlaj commented Oct 16, 2023

I manually tested light modes, recording, playing, and sequences. Everything seems to be working fine.
Thank you for your contribution, merging :)

@onlaj onlaj merged commit 7f1578a into onlaj:master Oct 16, 2023
@stephen322 stephen322 deleted the midi_reorg branch October 16, 2023 17:55
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.

2 participants