midi note-off variation

[okyeron]

Fix to midi.lua for some midi instruments that send a note-on, velocity zero instead of note-off

per #646

[okyeron]

I’m not actually sure what that byte is doing in the channel message. But I think you are correct and just changing msg.type would be fine.

…

On Nov 20, 2018, at 11:26 AM, Mark Wheeler ***@***.***> wrote: @markwheeler commented on this pull request. In lua/midi.lua: > + if data[3] > 0 then + --print("note") + msg = { + type = "note_on", + note = data[2], + vel = data[3], + ch = data[1] - 0x90 + 1 + } + -- if velocity is zero then send note off + elseif data[3] == 0 then + --print("noteoff") + msg = { + type = "note_off", + note = data[2], + vel = data[3], + ch = data[1] - 0x80 + 1 Should channel not be the same as above? Which makes me wonder if the only thing contained in the if statement should be the msg.type? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[catfact]

@okyeron sorry for the lag, im waiting for someone else to put eyes on this since i really don't/can't use the midi stuff.

but i remember something about this a while ago - it seems strange but IIRC there was actually controversy about mapping vel=0 to noteoff. maybe it should be a configuration option in the lua environment.

[okyeron]

I'm sure there's probably some situation where note-on plus vel=0 should NOT be a note-off, but if half the MIDI instrument manufacturers are doing it that way, what can you do? 🤷‍♂️

A configuration option would be nice, but I wouldn't know how to implement that.

[tehn]

accidentally approved, disregard.

I’m not actually sure what that byte is doing in the channel message. But I think you are correct and just changing msg.type would be fine.

just change the msg.type. the change to channel will result in the wrong channel being sent. did you test this with your hardware?

[artfwo]

there indeed was a controversy about this logic, but i think it was related to the low-level midi events and for the high-level api it will not harm.

[okyeron]

updated. tested with Qunexus and Livid Block.
(Having an problem with MK-225C controller, but that seems related to issue #610 - will investigate later)

is the following syntax OK?

  -- note on
  if data[1] & 0xf0 == 0x90 then
    msg = {
      note = data[2],
      vel = data[3],
      ch = data[1] - 0x90 + 1
    }
    if data[3] > 0 then 
      msg.type = "note_on"
    elseif data[3] == 0 then -- if velocity is zero then send note off
      msg.type = "note_off"
    end

[okyeron]

FWIW - my problem with the MK-225C controller is that it constantly sends an Active Sensing byte which barfs inside function Midi.to_msg(data)

my quick hack around this was to wrap most of Midi.to_msg in a check to be sure there is a second data byte

if data[2] ~= nil then     -- Ignore Active Sensing and others
~
end

Are clock/start/stop/continue (which also have a nil second byte) ignored somewhere?

[artfwo]

clock events don't have a second byte, their length is determined in matron (device_midi.c). what exactly is an active sensing byte?

[okyeron]

what exactly is an active sensing byte?

Not really sure myself, but I see that message when I plug that device into my mac with MIDI monitor running.

The MIDI spec shows this:

11111110= FE= 254 | Active Sensing | -- | --

a google search gives me this description:

A MIDI message sent by some MIDI devices all the time to confirm that they are still there and hasn’t gone off line. It carries no other information and hasn’t been widely implemented in the industry.

[artfwo]

a message with a status byte of 0xfe will be handled as other 1-byte messages, Midi.to_msg should just return { type = "other" } for these messages, what exactly is the problem with them?

[okyeron]

Apparently 0xfe is not being handled as a 1-byte message and it tries to evaluate it as note/cc and maiden spits out a stream of errors about attempting a bitwise operation on nil.

looking at device_midi.c 0xfe is not handled explicitly and ends up with the default msg_len = 2;

I can file a separate issue for this

[artfwo]

but it's handled, see https://github.com/monome/norns/blob/master/matron/src/device/device_midi.c#L55

so the problem is with the lua part, can you send the specific script code that you have a problem with?

[okyeron]

Ok so I have a very basic connection script

m = midi.connect(1)
m.event = function(data)
  local d = midi.to_msg(data)
  tab.print(d)
end

With the MK-225C as soon as this runs i get a stream of the following errors:

/home/we/norns/lua/midi.lua:188: attempt to perform bitwise operation on a nil value (field '?')
stack traceback:
	/home/we/norns/lua/midi.lua:188: in function 'midi.to_msg'
	/home/we/dust/scripts/okyeron/midi-example.lua:3: in field 'event'
	/home/we/norns/lua/midi.lua:120: in local 'event'
	/home/we/norns/lua/midi.lua:304: in function </home/we/norns/lua/midi.lua:292>

So here it appears to be trying to evaluate the 0xfe message as a note-on. and the second byte of data is nil and causes the error.

[tehn]

FYI doesn't seem like it's handled, and defaulting to length 2:

https://github.com/monome/norns/blob/master/matron/src/device/device_midi.c#L98

feel free to submit a PR that has a case for 0xFE and just throws it away

[tehn]

oh wait, it's handled here (missed the bitmask on line 66)

https://github.com/monome/norns/blob/master/matron/src/device/device_midi.c#L79

so there needs to be a nested case for 0x0E which just sets len to 0

@okyeron if you can test this with your device that'd be helpful (i don't have any sensing devices)

[tehn]

ie, just add

case 0x0E:

after line 89, recompile, and it should be good?

[okyeron]

debugging this active sensing thing further...

It seems norns.midi.event = function(id, data) is receiving 0xfe byte and then a nil byte.

I think what's happening is that there's 2 events being sent from device_midi.c. It looks like there might be a bracket out of place at

norns/matron/src/device/device_midi.c

Line 105 in 39afbf7

}

and it needs to move down to line 119

[artfwo]

@tehn @okyeron there's no need to add additional cases for handling the 0xfe status byte, see https://github.com/monome/norns/blob/master/matron/src/device/device_midi.c#L55

the bracket at line 105 is in the right place, moving it to line 119 will cause nothing. adding continue operator to https://github.com/monome/norns/blob/master/matron/src/device/device_midi.c#L60 might actually help, but i also don't have any hardware to test that change. @okyeron can you try that instead?

[tehn]

@artfwo good find, i clearly didn't go back far enough!

@okyeron i'd suggest getting some debug prints in here to trace what's going on

[okyeron]

@artfwo the bracket change totally fixes the issue for me.

I added some debug in testing and the if (byte >= 0xf8) { events were executing event_post(ev); twice
The stuff at line 107 should not execute for byte >= 0xf8, right? currently it's outside the if (byte >= 0xf8) { structure so events happen twice.

[okyeron]

FWIW - this should be testable with any midi hardware sending clock or start/stop messages.

[okyeron]

Getting back to the original pull request... Are the changes above good?

And then... Would it be reasonable to add the following items to Midi.to_msg() instead of just being set to type "other" ?
(and then it could also ignore active sensing from "other" when printing out the whole event stream as well)

i think this might be useful for someone working outside of the beatclock library - or for making beakclock more readable.

  -- start
  elseif data[1] == 0xfa then
    msg.type = "start"
  -- stop
  elseif data[1] == 0xfc then
    msg.type = "stop"
  -- continue
  elseif data[1] == 0xfb then
    msg.type = "continue"
  -- clock
  elseif data[1] == 0xf8 then
    msg.type = "clock"
  -- ignore active sensing
  elseif data[1] == 0xfe then
    -- do nothing

[tehn]

this is now probably fine as-is. i can't think of anything that would send a note-on with vel 0 expecting an actual note-on.

we definitely can't add more msg.type categories without much more modification to midi.lua, look there for further ramifications

edit: sorry for the lack of clarity. would need to add a bunch of code to to_data and to_msg etc, which i take is what you were proposing!

[okyeron]

more modification to midi.lua, look there for further ramifications

(I hope I'm not being too thick here but...) I'm not sure I see those ramifications since it just sets the msg.type for those incoming bytes. Unless you mean that we would then need to support those same types being sent out? (which I could look at as well)

[okyeron]

@tehn just in case you did not notice since it got buried by the other conversation here... The change for just updating msg.type has already been made: f9aa361