Sysex input feedback loop?

kylestetz commented 11 years ago

Hey- I'm working on a project that involves sending and receiving sysex messages, and for some reason I'm unable to receive long-format messages. Here's my implementation:

app.midi = {
  access: null,
  input: null,
  output: null,
  initialize: function(access) {
    this.access = access;
  },
  connectOutput: function(index) {
    if(this.access) {
      this.output = this.access.getOutput(index);
    }
  },
  connectInput: function(index) {
    if(this.access) {
      this.input = this.access.getInput(index);
      this.input.onmessage = this.listen;
    }
  },
  send: function(parameter, currentValue) {
    var ms = Math.floor(currentValue / 16);
    var ls = currentValue - ( ms * 16);
    this.output.send( [0xF0, 0x01, 0x20, 0x01, 0x01, parameter, ls, ms, 0xF7] );
  },
  listen: function(event) {
    console.log(event);
  },
  requestParameterDump: function() {
    this.output.send( [0xF0, 0x01, 0x20, 0x01, 0x06, 0xF7] );
  }
};

I can send message fine, but when I call app.midi.requestParameterDump, to which my synthesizer responds with ~220 bytes, my logs show me what looks like a feedback loop; the event.timestamp contains the same exact number & logs around 5,000 messages per second until Chrome gets fed up and stops responding.

I implemented the Jazz plugin directly and didn't get this issue:

app.midi = {
  Jazz: null,
  inputList: null,
  outputList: null,
  output: null,

  initialize: function() {
    this.Jazz = document.getElementById("Jazz1");
    if(!this.Jazz || !this.Jazz.isJazz) {
      this.Jazz = document.getElementById("Jazz2");
    }
    this.inputList = this.Jazz.MidiInList();
    this.outputList = this.Jazz.MidiOutList();
    console.log('inputs:');
    console.log(this.inputList);
    console.log('outputs:');
    console.log(this.outputList);
  },

  openInput: function(index) {
    this.Jazz.MidiInOpen(index, this.midiInCallback);
  },

  openOutput: function(index) {
    this.output = this.Jazz.MidiOutOpen(index);
  },

  midiInCallback: function(timestamp, byteArray) {
    console.log(byteArray);
  },

  send: function(parameter, currentValue) {
    var ms = Math.floor(currentValue / 16);
    var ls = currentValue - ( ms * 16);
    // evolver sysex
    this.Jazz.MidiOutLong( [0xF0, 0x01, 0x20, 0x01, 0x01, parameter, ls, ms, 0xF7] );
  },

  requestDump: function() {
    this.Jazz.MidiOutLong( [0xF0, 0x01, 0x20, 0x01, 0x06, 0xF7] );
  }
}

Any thoughts? I dug around this plugin and nothing immediately jumped out at me, though I'm a bit foggy as to how the event dispatching works.

Using Jazz v1.3 in Chrome 27.0.1453.116 on OS X 10.8.3

cwilso commented 11 years ago

Hmm, bizarre. Sorry, have been off-network for a bit. Will look at this late this week.

cwilso commented 11 years ago

So, I don't have any sysex-dumping hardware unpacked yet (I've been moving). Could you set a breakpoint around line 290 of the library, in this code:

        case 0xF0:
        switch (data[i]) {
            case 0xf0:  // variable-length sysex.
              // count the length;
              length = -1;
              for (j=i+1; (j<data.length) && (data[j] != 0xF7); j++)
                ;
            length = j-i+1;
            break;

and step through to see what's happening? It's clearly not counting the data length properly; it should count to the end of the sysex data (marked by an 0xF7 byte).

kylestetz commented 11 years ago

Alright, I'm seeing the issue.

The sysex message I'm expecting is 220 bytes long. I'm not sure if it's my synth (a DSI Evolver) or the Jazz plugin, but this message is coming in broken up into small chunks. The assumption this bit of code makes is that it will see an array in the format [ 0xF0, ... , 0xF7 ], but since the message is broken up into smaller arrays it's looking for meaning where there isn't any. This is what happens:

The first 3 bytes come in: [ 0xF0, 0x01, 0x20 ]
It finds 0xF0 in the switch on line 280 and hits the case 0xf0 on line 294
the case 0xf0 on line 296 causes it to try and count the length (which it does incorrectly in this instance: j already evaluates to the right length in the for loop and line 301 adds 1 to it)
it breaks & line 320 dispatches the event data (the incorrect length doesn't matter in this case)

here's where we run into problems...

The next bundle of bytes comes in: [ 0x01, 0x03, 0x00, 0x26, 0x32 ]
the switch on line 280 is evaluating data[i] & 0xF0, in this case 0x01 & 0xF0, which = 0x00, so there is no matching case
An event is dispatched on line 320 containing an empty array
length is never iterated, since we never hit any of the cases in the switch on line 280, so now we're in an infinite loop. this explains why there are thousands of events dispatched with the same timecode.

If the API is expected to dispatch the entire sysex message as a single event, we'll have to toss the messages into a buffer and wait until we see 0xF7. I'm getting around this issue right now in my own implementation (using Jazz directly) because I already know the length of the message I'll be receiving, but that isn't going to be the case here.

cwilso commented 11 years ago

Sigh. Yeah, I was afraid of that; sysex messages aren't guaranteed to come in in one packet in OSX.

I'll toss in some buffering code this week.

On Sat, Jun 29, 2013 at 8:11 AM, Kyle Stetz notifications@github.comwrote:

Alright, I'm seeing the issue.

The sysex message I'm expecting is 220 bytes long. I'm not sure if it's my synth (a DSI Evolver) or the Jazz plugin, but this message is coming in broken up into small chunks. The assumption this bit of code makes is that it will see an array in the format [ 0xF0, ... , 0xF7 ], but since the message is broken up into smaller arrays it's looking for meaning where there isn't any. This is what happens:

The first 3 bytes come in: [ 0xF0, 0x01, 0x20 ]

It finds 0xF0 in the switch on line 280 and hits the case 0xf0 on line 294

the case 0xf0 on line 296 causes it to try and count the length (which it does incorrectly in this instance: j already evaluates to the right length in the for loop and line 301 adds 1 to it)

it breaks & line 320 dispatches the event data (the incorrect length doesn't matter in this case)

here's where we run into problems...

The next bundle of bytes comes in: [ 0x01, 0x03, 0x00, 0x26, 0x32 ]

the switch on line 280 is evaluating data[i] & 0xF0, in this case 0x01 & 0xF0, which = 0x00, so there is no matching case

An event is dispatched on line 320 containing an empty array

length is never iterated, since we never hit any of the cases in the switch on line 280, so now we're in an infinite loop. this explains why there are thousands of events dispatched with the same timecode.

If the API is expected to dispatch the entire sysex message as a single event, we'll have to toss the messages into a buffer and wait until we see 0xF7. I'm getting around this issue right now in my own implementation (using Jazz directly) because I already know the length of the message I'll be receiving, but that isn't going to be the case here.

— Reply to this email directly or view it on GitHubhttps://github.com/cwilso/WebMIDIAPIShim/issues/29#issuecomment-20231462 .

cwilso commented 11 years ago

OK, try again now. I added some buffering code that should work across calls. I tested it by hand, so I'm pretty sure it works, but a real world test would be best.

kylestetz commented 11 years ago

Almost! I'm curious as to how you did your test.

Everything's good for the first packet of data; it's added to the buffer and, since data[i-1] != 0xF7 on line 281, we return on line 283.

Second time around this._inLongSysexMessage is true so we set i = this.bufferLongSysex(data,i) on line 260. However there's still the rest of the current loop of the for statement to get through, so on line 304 an event is made and on line 307 we decide we're done with the sysex thing so we end up dispatching the event, quite prematurely.

Every loop after that makes the same mistake of looking for 0xF0, so we get that infinite loop again. I don't have a lot of time today or I would dig in a little further and suggest a fix.

Thanks for taking a look at this though! I see you've taken the opportunity to throw some Promises in there as well. Very nice.

cwilso commented 11 years ago

Oops. One more try.

I only tested breaking across two packets, not three. Should be fixed now.

On Mon, Jul 1, 2013 at 4:50 PM, Kyle Stetz notifications@github.com wrote:

Almost! I'm curious as to how you did your test.

Everything's good for the first packet of data; it's added to the buffer and, since data[i-1] != 0xF7 on line 281, we return on line 283.

Second time around this._inLongSysexMessage is true so we set i = this.bufferLongSysex(data,i) on line 260. However there's still the rest of the current loop of the for statement to get through, so on line 304 an event is made and on line 307 we decide we're done with the sysex thing so we end up dispatching the event, quite prematurely.

Every loop after that makes the same mistake of looking for 0xF0, so we get that infinite loop again. I don't have a lot of time today or I would dig in a little further and suggest a fix.

Thanks for taking a look at this though! I see you've taken the opportunity to throw some Promises in there as well. Very nice.

— Reply to this email directly or view it on GitHubhttps://github.com/cwilso/WebMIDIAPIShim/issues/29#issuecomment-20318427 .

kylestetz commented 11 years ago

Perfect! Thanks again.

cwilso / WebMIDIAPIShim

Sysex input feedback loop? #29