Support multiple ports per MIDI device #886

ryanlaws · 2019-10-03T04:32:12Z

Trying to fix #536 but this has issues:

~~MIDI device selection list not growing with ports~~
~~Unplugging devices doesn't remove them correctly~~
Includes are dirtier
Device list code is generally dirtier
Probably needs more error logging
I'm a C noob so I may have added memory leaks
I'm a C noob so the code style is probably off

catfact · 2019-10-03T08:08:05Z

thanks @ryanlaws! super helpful.

maybe this is a good candidate for an upstream feature branch. if we can get data to/from multiple ports then that's a great start, and the other issues can be taken on separately.

i won't have time to really look at this for a few days i'm afraid, but nothing strikes me as outrageous on the surface.

ryanlaws · 2019-10-03T14:20:54Z

Thanks @catfact ! The first and second items I think hurt the UX a bit, so I'm going to dig in a bit more anyway. Hopefully they aren't over my head. Otherwise, which upstream branch should I target instead?

ryanlaws · 2019-10-04T07:07:48Z

Alrighty! I think I've taken care of the 2 issues I cared about. This could use some cleanup - specifically, if code standards allow I think the MIDI cases could be moved to the top of either switch and then prior assumptions and terseness could remain intact - but your time is limited and it's probably not worth holding up review.

ryanlaws · 2019-10-04T13:23:57Z

OK, I'm happy now.

tehn · 2019-10-04T18:41:27Z

super excited to check this out. i'm expecting i'll have time to dive in sunday. thank you so much!

ryanlaws · 2019-10-04T18:56:32Z

You're so welcome! Thank you and I appreciate any feedback you offer!

tehn · 2019-10-06T19:04:44Z

do we have a list of known multi-midi devices? i may be at a loss to test if i don't have one of them, but can make sure the changes don't break normal behavior

tehn · 2019-10-06T19:27:24Z

(i think) the name always gets a port index at the end?

it'd be nice that if there is only one port, that we don't always add "1" to the end of the name.

ryanlaws · 2019-10-06T20:32:57Z

do we have a list of known multi-midi devices? i may be at a loss to test if i don't have one of them, but can make sure the changes don't break normal behavior

For what it's worth, I've tested with a Korg Monologue and an Edirol UM-2, both of which have 2 ports. I think @catfact said he had something on loan that should be able to repro. The referenced issue mentioned another piece of equipment too.

(i think) the name always gets a port index at the end?
it'd be nice that if there is only one port, that we don't always add "1" to the end of the name.

Great idea - I was rolling this one around myself, but I couldn't think of an elegant way to do it. I think this would definitely be an improvement.

ryanlaws · 2019-10-06T21:13:27Z

OK, let me know if that works for you! It's a little ugly but it worked OK in my testing.

This has issues: - MIDI device selection list not growing with ports - Unplugging devices doesn't remove them correctly - Includes are dirtier - Device list code is generally dirtier - Probably needs more error logging - I'm a C noob so I may have added memory leaks - I'm a C noob so the code style is probably off

The port index was being appended for devices which only have one port and this doesn't necessarily make sense. This is being removed by passing a bool indicating whether the port being referenced is the sole port of the MIDI device, and only in this case will the index be appended.

catfact · 2019-10-09T23:06:36Z

@tehn or anyone get a change to check this out?
(i didn't. on sudden business travelling. have beatstep pro at home that should be an appropriate thing to test with.)

tehn · 2019-10-09T23:41:58Z

works with all my single point midi things!

no multi point here but if said to work, we could merge

tehn · 2019-10-12T21:11:14Z

@artfwo perhaps you want to take a look before i merge this? (since i recall you worked on the original midi code?)

artfwo · 2019-10-13T08:28:12Z

LGTM. i also don't have any multi-port devices at hand right now to test this. i would perhaps change dev_new signature to something like:

dev_new(
    device_t type,
    const char *path,
    const char *name,
    bool multi_port_device,
    unsigned int port_index,
)

i.e. swap the multi_port_device flag and index and change the former to be true, if there are multiple ports, but it's a purely cosmetic suggestion and certainly isn't a blocker for merging.

Rename is_sole_device to multiport_device and default to false to improve readability.

ryanlaws · 2019-10-13T14:47:58Z

Good idea @artfwo . Updated.

artfwo

👍

ryanlaws · 2019-10-13T15:17:38Z

Thank you!

ngwese · 2019-10-14T03:08:36Z

I have several multi-port midi devices. I'll give it a test right now.

ngwese · 2019-10-14T03:42:06Z

I'm happy to report this is working great with an iConnectivity mio2 - I can now send MIDI back between two norns, have two norns share access to MIDI din based devices, and send MIDI between norns and ansible (in MIDI mode). ...all selectable by picking different ports on the mio2 from the norns system menus.

Super awesome.

ryanlaws · 2019-10-14T06:22:26Z

Thank you for testing! I'm super stoked that this is opening up possibilities for you!

tehn · 2019-10-18T15:47:32Z

matron/src/device/device_list.c

        break;
    default:
        fprintf(stderr, "dev_list_add(): error posting event (unknown type)\n");
        return;
    }
-    event_post(ev);
+    if (ev != NULL) {
+        ev->monome_add.dev = d;


seems this is using the monome_add structure for HID and CROW

not sure it's relevant, but want to mention this comment from when HID was getting fixed up:
#662 (comment)
Although... I can't remember specifics of the debug I did at that point.

tehn · 2019-10-18T15:47:32Z

matron/src/device/device_list.c

+    if(dq.tail == dn) { dq.tail = dn->prev; }
+    remque(dn);
+    dq.size--;
+


fairly sure the problem is something with these three lines 138-140...

ryanlaws changed the title ~~Support multiple ports per MIDI device (fix #536)~~ Support multiple ports per MIDI device Oct 3, 2019

ryanlaws added 7 commits Oct 3, 2019

Convert stray tabs to spaces

9a8b52a

Move up menu to show selected device

930fc4e

Remove all MIDI ports when device removed

5342e65

Get next node before freeing current

2ea4dcd

Clean up

db7caca

ryanlaws added 2 commits Oct 13, 2019

Flip multiple device flag

Loading status checks…

7cb2345

Rename is_sole_device to multiport_device and default to false to improve readability.

Update port count check

Loading status checks…

e4ec7a8

artfwo approved these changes Oct 13, 2019

View changes

artfwo merged commit 8810899 into monome:master Oct 13, 2019
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ryanlaws deleted the ryanlaws:support-multiple-devices branch Oct 13, 2019

tehn mentioned this pull request Oct 16, 2019

occasional segfault on USB device removal #920

Closed

tehn reviewed Oct 18, 2019

View changes

monome / norns

Support multiple ports per MIDI device #886

Support multiple ports per MIDI device #886

ryanlaws commented Oct 3, 2019 •

edited

catfact commented Oct 3, 2019

ryanlaws commented Oct 3, 2019 •

edited

ryanlaws commented Oct 4, 2019

ryanlaws commented Oct 4, 2019

tehn commented Oct 4, 2019

ryanlaws commented Oct 4, 2019

tehn commented Oct 6, 2019

tehn commented Oct 6, 2019

ryanlaws commented Oct 6, 2019 •

edited

ryanlaws commented Oct 6, 2019

catfact commented Oct 9, 2019

tehn commented Oct 9, 2019

tehn commented Oct 12, 2019

artfwo commented Oct 13, 2019 •

edited

ryanlaws commented Oct 13, 2019

artfwo left a comment

ryanlaws commented Oct 13, 2019

ngwese commented Oct 14, 2019

ngwese commented Oct 14, 2019

ryanlaws commented Oct 14, 2019 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

monome / norns

Support multiple ports per MIDI device #886

Support multiple ports per MIDI device #886

Conversation

ryanlaws commented Oct 3, 2019 • edited

catfact commented Oct 3, 2019

ryanlaws commented Oct 3, 2019 • edited

ryanlaws commented Oct 4, 2019

ryanlaws commented Oct 4, 2019

tehn commented Oct 4, 2019

ryanlaws commented Oct 4, 2019

tehn commented Oct 6, 2019

tehn commented Oct 6, 2019

ryanlaws commented Oct 6, 2019 • edited

ryanlaws commented Oct 6, 2019

catfact commented Oct 9, 2019

tehn commented Oct 9, 2019

tehn commented Oct 12, 2019

artfwo commented Oct 13, 2019 • edited

ryanlaws commented Oct 13, 2019

artfwo left a comment

ryanlaws commented Oct 13, 2019

ngwese commented Oct 14, 2019

ngwese commented Oct 14, 2019

ryanlaws commented Oct 14, 2019 • edited

This comment has been minimized.

tehn Oct 18, 2019 Member

This comment has been minimized.

okyeron Oct 19, 2019 Contributor

This comment has been minimized.

tehn Oct 18, 2019 Member

ryanlaws commented Oct 3, 2019 •

edited

ryanlaws commented Oct 3, 2019 •

edited

ryanlaws commented Oct 6, 2019 •

edited

artfwo commented Oct 13, 2019 •

edited

ryanlaws commented Oct 14, 2019 •

edited

tehn Oct 18, 2019
Member

okyeron Oct 19, 2019
Contributor

tehn Oct 18, 2019
Member