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 subscribe/unsubscribe #476

Merged
merged 23 commits into from Aug 28, 2018
Merged

midi subscribe/unsubscribe #476

merged 23 commits into from Aug 28, 2018

Conversation

@tehn
Copy link
Member

@tehn tehn commented Jul 25, 2018

howto:

local o = midi.connect() -- defaults to port 1 (which is set in SYSTEM > DEVICES)
-- process incoming midi: convert to message table and print
o.event = function(data) tab.print(midi.to_msg(data)) end
-- helper send functions:
o.note_on(80,100)
o.note_off(80) -- optional off vel
-- raw bytes:
o.send{144,80,100}
-- or message table:
o.send{type="note_on", note=80, vel=100}

-- select different port
local second_midi = midi.connect(2)
second_midi.cc(50,100)

notes

  • changing ports in the system menu is dynamic. currently running scripts will not be interrupted. data redirected appropriately!

grids!

gr = grid.connect(1) -- get grid port 1 (defined in menu)
gr.all(0)
gr.led(2,2,5)
gr.refresh()
gr.event = function(x,y,z) print("hello!!!") end

follows same patterns as midi re: all/mirroring and named assignments

@tehn tehn requested a review from artfwo Jul 25, 2018
@tehn
Copy link
Member Author

@tehn tehn commented Jul 25, 2018

fixes #454

(once finished)

Copy link
Collaborator

@pq pq left a comment

a few nits. really loving the direction of this. curious to hear what @artfwo thinks.


--- send midi event to all devices
function Midi.send_all(data)
for id,device in pairs(Midi.devices) do

This comment has been minimized.

@pq

pq Jul 25, 2018
Collaborator

maybe for _, device in ... (since id is ignored?)

This comment has been minimized.

@pq

pq Jul 25, 2018
Collaborator

aside: i'm guessing arbitrary order is ok (and hence the use of pairs) but I wonder if the ordering of ipairs might ever help avoid surprises in debugging?


--- unsubscribe
function Midi.unsubscribe(callback)
for i,v in pairs(Midi.broadcast) do

This comment has been minimized.

@pq

pq Jul 25, 2018
Collaborator

again on pairs ... will an arbitrary unsubscription order ever be surprising? (in general i'd expect unsubscription to happen in a deterministic order the reverse of subscription but maybe in practice this doesn't matter?)

@@ -78,6 +146,14 @@ norns.midi.event = function(id, data)
if d.event ~= nil then
d.event(data)
end
-- do any individual subscribed callbacks
for name,callback in pairs(Midi.devices[id].callbacks) do

This comment has been minimized.

@pq

pq Jul 25, 2018
Collaborator

again maybe: for _, callback in pairs ... ? (also note that name here shadows name in the function scope.)

callback(data)
end
-- do broadcast callbacks
for n,callback in pairs(Midi.broadcast) do

This comment has been minimized.

@pq

pq Jul 25, 2018
Collaborator

one more chance for _ if it floats your boat.

@artfwo
Copy link
Member

@artfwo artfwo commented Jul 26, 2018

Looks good, but the part with returning a send function from subscribe is confusing, since the name subscribe doesn't precisely describe functionality here.

To resolve this, we could either have explicit methods for sending data to midi, i.e. keep send and send_all and use them in scripts (with device identifier constants if necessary). Or rename subscribe to something more specific like connect.

@tehn
Copy link
Member Author

@tehn tehn commented Jul 26, 2018

potential renames:

  • connect / disconnect
  • attach / detach
  • assign / dismiss
  • set / unset

but also, the return of a send function is a convenience. midi.send_all() still works, so does midi.send(dev) and midi.send_named(device_string)


furthermore, it's been confirmed that multiple similar devices enumerate with the same device string, so differentiation won't work for that use case.

i can't decide if that tradeoff merits change. my matching device names we can reattach midi devices with already-defined callbacks for specific device strings. this seems like the more important feature IMO.

luckily with custom controllers like the teensy you can rename the midi device string (right?) and that fixes the problem. so the use case left out is someone wanting to have two commercial midi controllers do different callbacks. but they can do this with the old method, attaching handlers to each device from midi.add (as is happening now with earthsea etc).

but my hope was to have a unified solution. not a "low level" and "convenience" way of handling midi/grids/etc.

side note: with grids we can attach handlers to the serial number, so this approach works perfectly there.

thoughts?

@markwheeler
Copy link
Collaborator

@markwheeler markwheeler commented Jul 26, 2018

Just throwing this out there as an observer, apologies if it's off track! :)

I feel like I would expect a subscribe function to return if it was successful - perhaps device_id if successful. And then convenience functions for different message types would help too, perhaps something like:

local dev_id = midi.get_device_id("controller name")
-- working with ids allows us to solve for multiple controllers with the same name by
-- letting you also browse the complete list of midi.devices

-- a generic subscribe to get everything
-- midi.subscribe(handler, dev_id, [channel])
local dev_connected = midi.subscribe(all_midi_func, dev_id)

-- there could also be midi.subscribe_by_name() for convenience

-- convenience subscribes for message types
-- midi.subscribe_cc(handler, dev_id, [channel])
midi.subscribe_cc(my_cc_func, dev_id, 1)

-- for sending any data there's midi.send(dev_id, {data})

-- also convenience functions for sending various data types
-- midi.send_note_on(dev_id, channel, note, velocity)
midi.send_note_on(dev_id, 1, 40, 64)
@tehn
Copy link
Member Author

@tehn tehn commented Jul 26, 2018

@markwheeler new proposed syntax below, as discussed with @pq and @artfwo

local op1 = midi.connect("OP-1 Midi Device")
op1.event = process_op1
op1.send{144,80,100}
function process_op1(data) print(data[1]) end

local all = midi.connect()
all.event = process_all
all.send{144,80,100}
function process_all(data) print(data[1]) end
@markwheeler
Copy link
Collaborator

@markwheeler markwheeler commented Jul 26, 2018

cool! presumably i could also connect op1 using an id if I had two plugged in?

and then in future it'll be nice to add some util functions or maybe just consts for message types etc for handling the midi data in a more readable way.

@tehn
Copy link
Member Author

@tehn tehn commented Jul 26, 2018

@markwheeler for two of the same-named device you'd need to redefine midi.add, as happens now in the scripts which use midi.

we're trying to come up with a simplified syntax that covers most use cases-- multi-same-named devices is the current downside of this approach, but it solves very many other issues (like re-plug working correctly).


i'm up for proposals regarding handing midi messages. ie

  • e = midi.parse(data) where return e is {.type = NOTE, .note = 80, .vel = 100} etc
  • midi.send_msg(e) where e has .type etc...

design ideas welcomed...

@tehn
Copy link
Member Author

@tehn tehn commented Jul 27, 2018

updated top post with newest syntax.

do we like the name event or handler for the callback? (i'm leaning towards event). also is it nice to define the function inline? (thinking tutorials) ie

op1.handler = function(data) print(data[1]) end

also added the beginnings of some util.

all.send{type="note", note=80, vel=100}

see the current code for how it works, haven't added types beyond note and cc. also it seems it would be nice to define some helper functions ie all.send_note(80,100) but i'm not quite sure how to do that gracefully with the current code (please suggest?)

also considering a "filter" model for redirecting traffic of incoming events. ie

op1.filter["note"] = process_op1_notes
op1.filter["cc"] = function(msg) print(msg.cc) end

where the filter table is initialized as everything pointing at op1.handler by default. so each type could be redefined. or set to nil if you want them to just die completely. thoughts?

todo:

  • data to msg
  • handler filtering by type
@TheTechnobear
Copy link

@TheTechnobear TheTechnobear commented Jul 31, 2018

is this approach assuming users are going to need to edit a script to reflect their midi devices?
how will they know easily what the names are, without doing some test code?
this doesn't seem very non-programmer friendly.

how about using a virtual /midi mapping approach?

basically scripts would refer to virtual devices (mididevice1..N)
and in the norns setup, a user could assign their particular devices to mididevice1..N, this could all be menu driven (in practice mididevice1= primary, mididevice2= secondary)

this means musicians would not have to deal with code etc, and also scripts would be more 'generic'

this approach would also work for multiple devices with the same device name (as they have difference id) e.g. two launchpad grids.

@Dewb
Copy link
Contributor

@Dewb Dewb commented Aug 2, 2018

This is looking great! My main feedback is that I still feel pretty strongly about the handlers needing to be lists you add and remove things from, otherwise it's going to be hard for people to combine multiple musical ideas from several scripts into a new script. (per discussion in #454)

The connect to get a device object makes sense, but rather than device.handler = function()... I would lean towards something like .add_handler() or .on():

local op1 = midi.connect("OP-1 Midi Device")
op1.add_handler(process_op1)
op1.send{144,80,100}
function process_op1(data) print(data[1]) end

If different parts of the script add separate handlers, they'll both be called.

This pattern can extend fairly naturally to allowing the user to offload some of the filtering to the system, either with arguments:

local op1 = midi.connect("OP-1 Midi Device")
op1.on(function (data) print(data[1]) end)
op1.on('cc', function (controller, value) ... end)
op1.on('cc', 17, function (value) ... end)
op1.send{144,80,100}

or with method names like .onNote(), .onCC(), .onChannelPressure() etc.

I agree with @TheTechnobear's comment above about virtual devices, but I think that could be deferred until later and implemented on top of .connect().

The main counterargument I can think of is that the system MIDI code could end up doing a lot of branching and having to check several different lists on every MIDI event. I'm still pretty new to Lua but I think that can be made efficient, and it'd be nice to do it once rather than making every script author think about it.

@Dewb
Copy link
Contributor

@Dewb Dewb commented Aug 6, 2018

After thinking about it some more, I'm no longer convinced it's critical that specific-device midi callbacks be stackable. It might be nice to use a method rather than the assignment operator so callbacks could become stackable later without breaking the API, but I withdraw my concerns from the previous comment.

@tehn
Copy link
Member Author

@tehn tehn commented Aug 15, 2018

@Dewb

After thinking about it some more, I'm no longer convinced it's critical that specific-device midi callbacks be stackable.

actually they're already stackable:

local first_op1 = midi.connect("OP-1 Midi Device")
local second_op1 = midi.connect("OP-1 Midi Device")

this works with the current code. it's important because menu.lua or a lib will want to receive midi data in addition to the user script (this was largely the impetus for this current change). currently there is nothing breaking about this API, it's just a layer on top.

@TheTechnobear @Dewb

how about using a virtual /midi mapping approach?

agreed, i should've mentioned earlier. i'm thinking an extra SYSTEM menu list for virtual devices with a selector:

  • list four device slots
  • on selection, the current MIDI connections (and ALL) will be listed, confirm to select, or cancel.
  • this device string gets stored in the system state.

these strings can get stored in a table so it'd be natural for a user script to say:

local first_op1 = midi.connect(1)

and then midi.connect would look up the first entry. of course if it's nil, then the default is ALL like before.

by default all the virtual devices would point at ALL

@tehn
Copy link
Member Author

@tehn tehn commented Aug 15, 2018

@Dewb

i could see adding the syntax you suggested:

op1.on(function(data) ... end)
op1.on_cc(function(data) ... end)

where basically

function midi.on(f)
   self.handler = f
end

function midi.on_cc(f)
  self.filter["cc"] = f

i'm still feeling a little up in the air about filtering/directing messages by type.

@tehn
Copy link
Member Author

@tehn tehn commented Aug 15, 2018

how do we want to handle note on vs. note off?

ie 0x80 data messages vs. velocity 0? do we need to have separate note_on and note_off types?

@TheTechnobear
Copy link

@TheTechnobear TheTechnobear commented Aug 15, 2018

note_off type, since that still allows for release velocity.
imo, if a controller sends note_on , velocity = 0, this should ('internally') also be converted to note_off, velocity = 100

@tehn
Copy link
Member Author

@tehn tehn commented Aug 15, 2018

@TheTechnobear so norns should also send note_off with vel 100 if the user script tries note_on with vel 0, yes?

@TheTechnobear
Copy link

@TheTechnobear TheTechnobear commented Aug 15, 2018

I was thinking more when receiving... so that user can handle note_off consistently, for both newer and older devices, with one bit of code, rather than having to handle both use cases

sending, hmm, i guess thats consistent.
i guess the only trouble with that is old gear, might not implement note off... not sure how much of a problem that is.

or sending, we leave it up to the script, if the script is not doing anything with release velocity, then they can choose either to send note_off, or note_on/v=0...

@tehn
Copy link
Member Author

@tehn tehn commented Aug 15, 2018

many improvements. see top post (and code) for updates!

@simonvanderveldt
Copy link
Member

@simonvanderveldt simonvanderveldt commented Aug 17, 2018

i'd appreciate any testing and feedback so we can get this merged. (and then i'll finally get the midi/grid study done!)

I can probably do some testing this weekend if that's not too late

@antonhornquist
Copy link
Collaborator

@antonhornquist antonhornquist commented Aug 19, 2018

tried this out now. found a bug.

to recreate:

  1. select awake script
  2. plug in a grid
  3. go into system > devices > grid menu and change 1 to the grid
  4. press key1 to switch to script mode.
  5. press key1 to get back to menu.

expected result: system > devices > grid menu is shown
actual result: screen freezes and the menu is not shown. key2 will navigate back to system menu but menu navigation appears broken. after that it's not possible to navigate into system > devices again using key3.

@antonhornquist
Copy link
Collaborator

@antonhornquist antonhornquist commented Aug 19, 2018

also one question - is midi subscribe / unsubscribe additions supposed to be backwards compatible (i think i read something about this).

i tried my current step script on a clean boot and it doesn't work well after the additions (no leds show up upon starting script with grid connected, if grid is reconnected leds show up ok but key presses do not have any effect). let me now if i should ignore these issues and just convert my script to the new subscribe / unsubscribe pattern

@antonhornquist
Copy link
Collaborator

@antonhornquist antonhornquist commented Aug 19, 2018

also, a comment on the API. when is all devices really applicable? given virtual devices 1...n, and properly auto-routing newly connected devices to virtual devices (without having to restart script), i personally don't see the need for all. at least not for midi/led output.

to clarify: for one-grid scenarios local grid = grid.connect(1) would do.

for multi-device scenarios, this would:

local grid1 = grid.connect(1)
local grid2 = grid.connect(2)

likewise for midi...

rather than to have all as default, it would be none until a device is connected, at which point the device would take the next "open" slot (tagged with none). (tho i'm aware auto-connection is not implemented)

@tehn
Copy link
Member Author

@tehn tehn commented Aug 20, 2018

@antonhornquist thanks for the bug report. i'll fix it.

i tried not to break any backwards compatibility with midi/grid regarding .add etc. the manager syntax is just a layer on top. i can check this again. the g grid management went away, however.

the "all" method is necessary as auto-population wouldn't really work given the current implementation. the virtual device list is a set of strings. it doesn't execute callbacks when these strings are changed... this would add a substantial layer of complication and likely require the whole thing to be rewritten (which could be a future project when someone is up for it.)

i don't see dynamic virtual-device switching mid-script as a needed feature. scripts should just be re-initialized.

agreed that "none" is a good idea for a default, but the first-time use case isn't very nice. ie. someone loads awake and inserts a grid. the grid would be registered as device 1, but then wouldn't be initialized unless the script is restarted.

i definitely see an advantage to having "all" for midi, and "all" for grids is less useful certainly. (though mirrored grids is novel certainly... it does reveal some libmonome issue)

@tehn
Copy link
Member Author

@tehn tehn commented Aug 20, 2018

@antonhornquist menu bug fixed

@tehn
Copy link
Member Author

@tehn tehn commented Aug 24, 2018

anyone spent some time with this? i'll do more testing today ahead of a merge tonight.

i'll make an accompanying update to dust fixing all of the scripts (in terms of grid interaction)

@tehn
Copy link
Member Author

@tehn tehn commented Aug 24, 2018

considering naming the callbacks something nicer than "handler"

midi: input
grid: key

or perhaps just universally using input? keep in mind arc and hid are coming, so some future-conscious pattern setting is at hand.

@tehn
Copy link
Member Author

@tehn tehn commented Aug 24, 2018

@Dewb i'll have a go at updating awake etc to set up the beatclock in the new midi syntax style

@tehn
Copy link
Member Author

@tehn tehn commented Aug 27, 2018

big rework of the virtual port system for midi. much improved! see top post.

all connect addressing is to ports now, defined via system menu. live changing of the menu works.

handler renamed to event

todo

  • convert grid similarly
  • add logic for duplicate name modification, ie, two op-1's: second-plugged op-1 will get 2 appended (or +, or similar)
  • have device menu indicate "attached" state
@simonvanderveldt
Copy link
Member

@simonvanderveldt simonvanderveldt commented Aug 27, 2018

I get this when using this the first time after going to SYSTEM > DEVICES > MIDI with nothing plugged in:

all
none
all
none

It's double for some reason

@tehn
Copy link
Member Author

@tehn tehn commented Aug 27, 2018

@simonvanderveldt thanks for the report. i'll test with clean system settings. it should look like this:

all
none
none
none

edit, oh you mean in the selector. aha, i'll investigate

@simonvanderveldt
Copy link
Member

@simonvanderveldt simonvanderveldt commented Aug 27, 2018

Just ran the two testscripts from maiden, all MIDI events are correctly logged.
CCs seem a bit slow, for example when quickly rotating a knob, but that might also be the maiden/console output side of things?

Also checked the grid with a minor change (gr.event = function(x,y,z) print("hello!!!", x, y, z) end) and that's working as expected as well :)

I guess this change means all scripts that use MIDI or a grid need to be updated? Will they use device 1 by default? Or how will that work exactly?

@tehn
Copy link
Member Author

@tehn tehn commented Aug 27, 2018

@simonvanderveldt great, thanks.

i believe any slowness in the cc's is just print lag. within the system, cc's mapped to filter, for example, is incredibly responsive. i tested with a touch controller, and it works for percussive playing.

dust scripts are already updated: monome/dust#184

default port is 1, yes.

@artfwo
artfwo approved these changes Aug 28, 2018
@antonhornquist
Copy link
Collaborator

@antonhornquist antonhornquist commented Aug 28, 2018

this may the same issue as reported by @simonvanderveldt above.

with a 128 plugged and changing device for 1. all entry these options up:

monome 128 m1000604
all
none
all

it's also possible to move cursor down until nothing is selected.

@antonhornquist
Copy link
Collaborator

@antonhornquist antonhornquist commented Aug 28, 2018

also, after plugging out a midi device (OP-1 Midi Device) that midi device still shows up in the list of possible devices to choose even though there's a dev_delete(): removing device 2 print output in matron

edit: this appears to be intermittent. anyhow a selected "OP-1 Midi Device" remains in the DEVICE > MIDI screen after it has been removed. and the possible devices to choose from seems a bit buggy (as @simonvanderveldt has pointed out). when i removed my OP-1 now it ended up in "all", "none", "all", "none", but there was only "OP-1 Midi Device", "all" and "none" prior to that.

@tehn tehn merged commit 14dfc6f into master Aug 28, 2018
@tehn
Copy link
Member Author

@tehn tehn commented Aug 28, 2018

@antonhornquist i wasn't able to replicate the "all" "none" duplication bug. do you know a way to replicate it exactly?

the unplug update is tricky. the list is updated when you enter the DEVICES menu, so if you add/remove devices while in this menu it won't be updated until you re-enter the root DEVICES menu. i can change this to update when you enter SELECT within DEVICES, but still the update won't be live on-screen if you're inside select...

@simonvanderveldt simonvanderveldt deleted the midi branch Aug 28, 2018
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

8 participants