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 stability and usability improvements #240

Merged
merged 14 commits into from Oct 1, 2020

Conversation

@njanssen
Copy link
Contributor

@njanssen njanssen commented Sep 24, 2020

Hi!

I ran into an issue in the Electron version of Orca where MIDI clock messages were never processed when Orca started with no initial input MIDI device selected. After switching to the next MIDI input device with CmdOrCtrl+, (in my case selecting IAC Driver Bus 1) the MIDI message handler (Midi.receive) was not attached resulting in Orca not being notified of the clock messages received from this device.

I resolved this problem by relying on the logic in MIDI.selectInput (which sets the new input device index and MIDI message handler) in MIDI.selectNextInput when switching between input devices. I also updated the wrapping logic so index -1 (no input device) is selected when the user reached the end of the array with available input devices (Midi.inputs).

njanssen added 6 commits Sep 24, 2020
…put to correctly refresh MIDI message handler when no MIDI input was selected at startup
… we've reached the end of the `Midi.inputs` array.

Now relying on `midi.selectInput` to do all the work, including setting `Midi.inputIndex` - this make sure an existing MIDI message handler is removed from the previous input device before we switch to the next one (or no input device in case of index -1)
@njanssen
Copy link
Contributor Author

@njanssen njanssen commented Sep 24, 2020

Also tested these changes in the browser (Chrome), seems to work fine too 👍

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

Nice! But, are you able to get the frames to advance once the clock is running and playing? I see Orca reporting received start and stop messages, but the frames are not increasing.

Looking at the code it never gets out of isPaused == true:

  this.tap = function () {
    pulse.last = performance.now()
    if (!this.isPuppet) {
      console.log('Clock', 'Puppeteering starts..')
      this.isPuppet = true
...
    }
    if (this.isPaused) { return }
... // on every 6th pulse
      client.run()

But

  this.play = function (msg = false) {
    console.log('Clock', 'Play', msg)
    if (this.isPaused === false) { return }
    if (this.isPuppet === true) { console.warn('Clock', 'External Midi control'); return }
    this.isPaused = false
@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

BTW thank you for leaving fddfba6 in the history, I was looking for an example :)

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

Yep, when I do this it works:

--- a/desktop/sources/scripts/clock.js
+++ b/desktop/sources/scripts/clock.js
@@ -59,8 +59,8 @@ function Clock (client) {
   this.play = function (msg = false) {
     console.log('Clock', 'Play', msg)
     if (this.isPaused === false) { return }
-    if (this.isPuppet === true) { console.warn('Clock', 'External Midi control'); return }
     this.isPaused = false
+    if (this.isPuppet === true) { console.warn('Clock', 'External Midi control'); return }
     if (msg === true) { client.io.midi.sendClockStart() }
     this.setSpeed(this.speed.target, this.speed.target, true)
   }
@@ -68,8 +68,8 @@ function Clock (client) {
   this.stop = function (msg = false) {
     console.log('Clock', 'Stop')
     if (this.isPaused === true) { return }
-    if (this.isPuppet === true) { console.warn('Clock', 'External Midi control'); return }
     this.isPaused = true
+    if (this.isPuppet === true) { console.warn('Clock', 'External Midi control'); return }
     if (msg === true || client.io.midi.isClock) { client.io.midi.sendClockStop() }
     client.io.midi.allNotesOff()
     this.clearTimer()
@njanssen
Copy link
Contributor Author

@njanssen njanssen commented Sep 25, 2020

Hi @unthingable, for me the frames started advancing as soon as I sent midiClock commands. However, I now do notice that frames don't start advancing when Orca's clock is paused before starting the MIDI clock. As soon as the MIDI clock messages are received (and "Clock Puppeteering starts..") it's no longer possible to use space or MIDI start commands to unpause Orca and make the frames start advancing as you describe.

Your changes in Midi.play and Midi.stop indeed resolve this issue, because it makes it possible to resume (or pause) the clock by pressing space as you're used to when using Orca's own BPM clock. Something I did notice wasn't possible before when MIDI clock was running (thought it was by design). Besides using the keyboard, sending over MIDI start and stop messages to pause/resume Orca also work nicely after this change. I've added your change to the branch.

Thanks!

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

Thanks! I think the behavior you were seeing initially could be because Orca starts up already running, and I was pausing mine.

This PR is just in time for this discussion: #219

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

One minor annoyance is "clock start -> play" picks up the frame count where it left off and that is 6 times more likely to be out of sync with clock source. I think it makes more sense to reset the frame count. Giving it a try...

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

Ok, it's a little wonky but it works:

  this.tap = function () {
    pulse.last = performance.now()
    if (!this.isPuppet) {
      console.log('Clock', 'Puppeteering starts..')
      this.isPuppet = true
      this.clearTimer()
      pulse.timer = setInterval(() => {
        if (performance.now() - pulse.last < 2000) { return }
        this.untap()
      }, 2000)
    }
    if (this.isPaused) { return }
    if (pulse.count === 0) {
      client.run()
    }
    pulse.count = (pulse.count + 1) % 6
  }

  this.play = function (msg = false) {
    console.log('Clock', 'Play', msg)
    if (this.isPaused === false) { return }
    this.isPaused = false
    if (this.isPuppet === true) {
      pulse.count = 0  // works in conjunction with `tap` advancing on 0
      this.setFrame(0)  // make sure frame aligns with pulse count for an accurate beat
      console.warn('Clock', 'External Midi control')
      return 
    }
    if (msg === true) { client.io.midi.sendClockStart() }
    this.setSpeed(this.speed.target, this.speed.target, true)
  }

This assumes clock start/continue is always quantized at the source (starting on a beat), if not then there is also SPP to get an offset from. Interesting note about Bitwig's SPP implementation: when looping with SPP enabled it will send "stop" then SPP then "start" again. Without SPP it sends no stop/start, the clock just keeps going. Don't know if that's standard, haven't checked other hosts yet.

Once the clock is going steady the timing sounds internally consistent and solid. What's wonky is "start -> play" can be off by a random amount. Adjusting clock out offset at the source will bring it in sync and it will stay there until the next stop+start, then it will be off again. I wonder if this has something to do with onmidimessage scheduling, if so it could get worse when there is more MIDI data to process than just clock.

Although... playing with it a bit more, Bitwig driving Orca at 110bpm I'm getting pretty consistent results with no SPP enabled and clock out offset at 35-40ms, even after stopping and restarting Bitwig playback — the first beat in Orca is off but the rest are fine. With SPP same is true with the same but negative offset, -35 - -40ms, and the first beat will be off every time it loops. With SPP off it's pretty usable!

image

image

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

Happy to PR the above into this branch or master.

@njanssen
Copy link
Contributor Author

@njanssen njanssen commented Sep 25, 2020

Happy to PR the above into this branch or master.

@unthingable Sounds good! It would be great to have predictable MIDI clock syncing in Orca. I'm personally using TidalCycles to send the MIDI clock messages, and I also noticed the drifting of the beat alignment. I guess creating a PR on this branch instead of master would make testing easier.

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 25, 2020

@njanssen done: njanssen#1 (with an additional improvement: stopping notes while in clock slave mode)

- 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
Copy link
Contributor

@unthingable unthingable commented Sep 28, 2020

@njanssen 👋 :)

I've been playing with my version for a couple of days now, no drifting beats.

@njanssen
Copy link
Contributor Author

@njanssen njanssen commented Sep 28, 2020

@njanssen 👋 :)

I've been playing with my version for a couple of days now, no drifting beats.

Looks good! I found an issue when the user pauses/resumes Orca using space since that action will also reset the counter. I think we should fix that before we can merge in that PR.

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 28, 2020

@njanssen thanks! I see your comments in my PR, will respond there.

@unthingable
Copy link
Contributor

@unthingable unthingable commented Sep 30, 2020

@njanssen my fixes are ready to merge, @neauoire will be testing and merging this this weekend.

Summary of incoming changes:

  • Fixed: beat drift when stopping and resuming Orca
  • Fixed: beat drift when (re)starting master
  • Fixed: hung notes when stopping Orca while synced to external clock
  • Fixed: Orca unpausing itself after external clock stops
  • Changed: Orca will always sync to clock START messages by resetting frame count, even if already playing

This collaborative PR may need an updated title.

MIDI clock slave mode: fix timing and note-off behavior
@njanssen njanssen changed the title Set MIDI message handler to new input device when switching between MIDI inputs MIDI clock stability and usability improvements Sep 30, 2020
@neauoire
Copy link
Member

@neauoire neauoire commented Sep 30, 2020

I can't wait to give this a try, you're amazing u know <3

@unthingable
Copy link
Contributor

@unthingable unthingable commented Oct 1, 2020

For posterity, a fun mod for those wanting 16th notes without doubling the source bpm:

diff --git a/desktop/sources/scripts/clock.js b/desktop/sources/scripts/clock.js
index 95086cf..3a29be5 100644
--- a/desktop/sources/scripts/clock.js
+++ b/desktop/sources/scripts/clock.js
@@ -5,6 +5,7 @@
 function Clock (client) {
   const workerScript = 'onmessage = (e) => { setInterval(() => { postMessage(true) }, e.data)}'
   const worker = window.URL.createObjectURL(new Blob([workerScript], { type: 'text/javascript' }))
+  const pulsePerFrame = 3

   this.isPaused = true
   this.timer = null
@@ -65,7 +66,7 @@ function Clock (client) {
       if (!pulse.frame || midiStart) {  // no frames counted while paused (starting from no clock, unlikely) or triggered by MIDI clock START
         this.setFrame(0)  // make sure frame aligns with pulse count for an accurate beat
         pulse.frame = 0
-        pulse.count = 5   // by MIDI standard next pulse is the beat
+        pulse.count = pulsePerFrame - 1   // by MIDI standard next pulse is the beat
       }
     } else {
       if (msg === true) { client.io.midi.sendClockStart() }
@@ -97,7 +98,7 @@ function Clock (client) {
   }

   this.tap = function () {
-    pulse.count = (pulse.count + 1) % 6
+    pulse.count = (pulse.count + 1) % pulsePerFrame
     pulse.last = performance.now()
     if (!this.isPuppet) {
       console.log('Clock', 'Puppeteering starts..')
@neauoire neauoire merged commit 0050b53 into hundredrabbits:master Oct 1, 2020
@neauoire
Copy link
Member

@neauoire neauoire commented Oct 1, 2020

This works wonders!

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

3 participants