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

MIDI clock slave mode: fix timing and note-off behavior #1

Merged

Conversation

@unthingable
Copy link

@unthingable unthingable commented Sep 25, 2020

  • play() will reset both frame and pulse counts to 0, for alignment
  • stop() will silence
  • tap() will update immediately upon play(), not on the next frame
- play() will reset both frame and pulse counts to 0, for alingment
- stop() will silence
- tap() will update immediately upon play(), not on the next frame
@unthingable unthingable force-pushed the unthingable:midi-input-switching branch from 1637cd9 to e06cd58 Sep 28, 2020
@njanssen
Copy link
Owner

@njanssen njanssen commented Sep 28, 2020

@unthingable I notice the Clock.untap function (untouched in your PR) resets the count to 1 where I'd expect it to be reset to 0. This means the counter starts at 1 when MIDI clock signal stops. Don't think it matters much, but do you think this is on purpose?

@njanssen
Copy link
Owner

@njanssen njanssen commented Sep 28, 2020

@unthingable I looked up the MIDI specification about the MIDI clock, and the change seems to implement how start and stop was intended:

"The master needs to be able to start the slave precisely when the master starts. The master does this by sending a MIDI Start message. The MIDI Start message alerts the slave that, upon receipt of the very next MIDI Clock message, the slave should start the playback of its sequence." (midi.teragonaudio.com/tech/midispec/seq.htm)

I think the if (pulse.count == 0) { client.run() } in Clock.tap() implements this requirement, correct?

Also, the PR implements the following rule:

"If a slave receives MIDI Start or MIDI Continue messages while it’s in play, it should ignore those messages. Likewise, if it receives MIDI Stop messages while stopped, it ignores those. "

I ran a first test of this branch with TidalCycles and Orca seems to respond nicely. I'll play around with it a little bit more (mayble also with Ableton) to see if clock sync has become as predictable as I expect it to be now.

Thanks!

@njanssen
Copy link
Owner

@njanssen njanssen commented Sep 28, 2020

@unthingable When I use MIDI start and stop messages, beat syncing indeed seems to work nicely. However, when I use space to pause and resume Orca, the beat starts at the moment I pressed space. So, it forgets its alignment with the original MIDI start message that intentionally reset the counter to 0.

Since Clock.tap() keeps running while Orca is paused at the moment MIDI clock messages are coming in, we should probably treat manual pause/resume with space differently. For instance by leaving the counter running in the background while Orca is paused. For this, we should separate the handing of start and stop MIDI messages and manual pause/resume toggling.

What do you think?

@unthingable
Copy link
Author

@unthingable unthingable commented Sep 28, 2020

@njanssen thank you for the thorough read and for for digging up the standards.

I notice the Clock.untap function (untouched in your PR) resets the count to 1

Oops, I missed that one. Here play() resets the count so whatever untap() did with that wouldn't be noticeable. Also, it seems untap() is called only when clock stops arriving, in which case we're done slaving altogether.

do you think this is on purpose?

I don't fully understand the intent behind original pulse.count math: untap sets it to 1, tap increments it again and only then checks for 0 mod 6 and increments the frame. So if frames are incremented every 6th pulse starting at 0, and beats happen every X frames, and pulse count is never reset, and when play() is triggered the pulse count is most likely not a multiple of 6, then it's really hard to get a beat in sync with master — it's going to be off by some undetermined number of pulses.

It's only a guess but it looks like an attempt to sync based on the presence of the clock signal itself rather than START and STOP messages (notice how play and stop did nothing with the counts). First tap expected to start on some initial pulse count (presumably aligned with a beat), untap restored it for when the clock starts tapping again. Even with those assumptions the math was off.

The MIDI Start message alerts the slave that, upon receipt of the very next MIDI Clock message

That is an interesting detail!

🤔 Sounds like play() should be setting pulse.count to 5 instead of 0 (and definitely not 1). This could explain the 40ms delay I'm seeing. Will need to play around with it a little more.

Also, the PR implements the following rule: "If a slave receives MIDI Start or MIDI Continue messages while it’s in play, it should ignore those messages...

I believe that was already implemented, play and stop check isPaused before doing anything.

@unthingable
Copy link
Author

@unthingable unthingable commented Sep 28, 2020

@unthingable When I use MIDI start and stop messages, beat syncing indeed seems to work nicely.

👍

However, when I use space to pause and resume Orca, the beat starts at the moment I pressed space. So, it forgets its alignment with the original MIDI start message that intentionally reset the counter to 0. ... we should probably treat manual pause/resume with space differently. For instance by leaving the counter running

Nice catch, I ignored that use case and focused on starting and stopping Orca from clock master, primarily because this is nontrivial to solve. But if we were to solve it...

The problem is not only we'd have to count pulses in the background, we also need to count phantom frames. Otherwise, for example, let's say Orca was stopped on pulse 4 and started on pulse 8, so we didn't advance the frame that would have happened on pulse 6. Next frame advance will happen on pulse 12 — in alignment with pulse count but we're now 1 frame behind the clock and our beats are out of alignment.

So, a possible solution: we count pulses and frames in the background, and then once Orca is unpaused, the next frame update sets the frame number to what it would have been as if Orca was playing all this time. It will be almost like we muted Orca, but with weirdness: clock-based things (D, C, etc.) will be in sync with the background clock, others (I, E/W/S/N, etc.) will not. I can try adding this, my guess you probably won't want to use space for pausing because your composition won't come back the same.

The other solution is to not pause Orca when slaved :) No harm in having both available.

@unthingable unthingable marked this pull request as draft Sep 28, 2020
@unthingable unthingable marked this pull request as ready for review Sep 28, 2020
@unthingable unthingable force-pushed the unthingable:midi-input-switching branch from f7a2a98 to 6cce494 Sep 28, 2020
@unthingable
Copy link
Author

@unthingable unthingable commented Sep 28, 2020

@njanssen I did it! And it's more useful than I pessimistically predicted :-D

In summary:

  • Stopped Orca stays in sync with MIDI master

  • Stopping master stops Orca, notes off

  • Starting master starts Orca (if stopped)

  • Playing Orca re-syncs on MIDI clock START

  • First beat in Orca after a cold start is still off, still don't know why but whatever

  • Timing is pretty solid, master still requires 40ms offset

I'm a bit unsure about that last one, it is going beyond the MIDI standard and further reinforces the assumption that master always sends START on the beat. Then again, I'm looking at my Novation Circuit, it always assumes that and it ignores START when already playing (but it does handle SPP, achieving the same goal and a little more).

I can't shake the feeling that this is a little too janky and controversial, but maybe that's ok. A more standard-friendly way would be to re-synchronize via SPP instead of clock START, I think I'd prefer to redo it that way or even not at all. I don't remember TidalCycles sending SPP, does it?

if (pulse.count % 6 === 0) {
client.run()
pulse.count = 0
} else {

This comment has been minimized.

@unthingable

unthingable Sep 29, 2020
Author

oops, this didn't need to be an else

@njanssen
Copy link
Owner

@njanssen njanssen commented Sep 30, 2020

I did it! And it's more useful than I pessimistically predicted :-D

Nice! I tried it out, and the only issue I ran into was timing offsets which I could solve by setting MIDI latency in SuperCollider or by using nudge in TidalCycles. As soon as things were lined up, I could get a steady beat sync on (1) manual pause/resume in the Orca UI with space and (2) starting/stopping MIDI clock.

I don't remember TidalCycles sending SPP, does it?

No, TidalCycles doesn't keep track of time in terms of seconds, has its own internal cycle count. The standard way of controlling midi clock is with start messages (i.e. send a start command at the beginning of a cycle), see also https://tidalcycles.org/index.php/MIDI_Clock

Thanks for working on this PR, I'm going to merge it in 👍

@njanssen njanssen merged commit bf36c79 into njanssen:midi-input-switching Sep 30, 2020
@unthingable
Copy link
Author

@unthingable unthingable commented Sep 30, 2020

Yay!

only issue I ran into was timing offsets which I could solve by setting MIDI latency in SuperCollider

Yeah, I played with different initial pulse count values to see if it would help line up with the beat without additional latency at the source and it didn't, it was always off by some other much larger amount. The current solution produces best results.

My naive assumption is that my pulse math is correct and there is some inherent latency somewhere in the chain (in my case, Bitwig sending clock -> IAC transmitting clock -> Electron reacting to MIDI -> Electron emitting MIDI -> IAC transmitting event -> Bitwig reacting to event), this wouldn't be surprising. The good news is the latency seems consistent, I'm getting repeatable solid sync with a 40ms offset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants