Opened 10 years ago

Closed 8 years ago

Last modified 6 years ago

#4463 closed enhancement (fixed)

Add multi-ports support to USB MIDI driver

Reported by: Pete Owned by: phoudoin
Priority: normal Milestone: R1
Component: Drivers/USB Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

I've made considerable revisions to Jérôme Duval's original usb_midi driver, to make it handle MIDI output as well as input and process variable-lngth MIDI events properly (ones less than 3 bytes, and long System Exclusives).

It needs further work to make it recognize multiple and multiport devices, but it works well with my MidiSport UNO.

I'd be glad if someone could commit the attached patch. Thanks.

Attachments (6)

usb_midi.2.diff (2.3 KB) - added by Pete 8 years ago.
Fix for assignment of IN and OUT Endpoints
midi_0A92.txt (1.1 KB) - added by lt_henry 8 years ago.
usb_midi.h (3.6 KB) - added by Pete 8 years ago.
devlist.c (2.9 KB) - added by Pete 8 years ago.
usb_midi.diff (39.1 KB) - added by Pete 8 years ago.
Provides full multiport USB MIDI access for Class Compliant devices
usb_midi.c (20.4 KB) - added by Pete 8 years ago.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 10 years ago by Pete

I've updated the sources to reflect the guidelines as much as possible. A replacement patch is attached.

comment:2 Changed 10 years ago by phoudoin

After a quick look, it sounds good.

While multi-devices support (/dev/midi/usb/<n>) is already there AFAIK, the last missing feature is multiports support (/dev/midi/usb/*/<n>) indeed.

I plan to work on some MIDI tickets (#4053, #4445) during the coming week-end, I'll applied your patch and, hopefully add multiports support. I've a Roland UM-2 dual midi ports at home, but I'll have to "steal-back" my keyboard from my son in order to run midi output tests... ;-)

comment:3 Changed 10 years ago by phoudoin

BTW, and IIRC, midi port names as shown in PatchBay are the device name, /dev/midi/ prefix removed?

In such case, I guess that having /dev/midi/usb/<device_number>/<port_number> could be quite confusing with complex midi configuration (several multi-ports midi USB adapters, of same or different kind). I dunno if it's easy or not, but I guess having usb/<vendor>_<product>/<device_number>/<port_number> will reduce a bit confusion.

If all our both MIDI adapters were connected, it will lead to:

/dev/midi/usb/roland_um2/0/0 (first UM2, port 1) /dev/midi/usb/roland_um2/0/1 (first UM2, port 2) /dev/midi/usb/roland_um2/1/0 (second UM2, port 1) I should have bough an UM4 ;-) /dev/midi/usb/roland_um2/1/0 (second UM2, port 2) /dev/midi/usb/m-audio_uno/0/0 (first UNO, single port) /dev/midi/usb/m-audio_midisport2x2/0/0 (first midisport 2x2, port A) /dev/midi/usb/m-audio_midisport2x2/0/1 (first midisport 2x2, port B)

Issue is having product and vendor strings...

comment:4 Changed 10 years ago by phoudoin

Ooops, wrong formatting.

  • /dev/midi/usb/roland_um2/0/0: my first UM2, port 1
  • /dev/midi/usb/roland_um2/0/1: my first UM2, port 2
  • /dev/midi/usb/roland_um2/1/0: my second UM2, port 1.
    Yes, I should have bough an UM4 instead ;-)
  • /dev/midi/usb/roland_um2/1/0: my second UM2, port 2
  • /dev/midi/usb/m-audio_uno/0/0: your first UNO, single port
  • /dev/midi/usb/m-audio_midisport2x2/0/0: your first midisport 2x2, port A
  • /dev/midi/usb/m-audio_midisport2x2/0/1: your first midisport 2x2, port B

comment:5 Changed 10 years ago by phoudoin

Owner: changed from mmlr to phoudoin
Status: newassigned

comment:6 Changed 10 years ago by Pete

You're right, of course -- multi-device support is present. I wasn't thinking. I have a MidiSport 2x2 ("Anniversary") as well, so I was going to look at adding multiport handling later, but if you get to it first please go for it! There is actually some other problem with the 2x2 that prevents it even working with the first port, but at the moment I haven't figured it out. As far as I can tell, the device is sending a continual stream of blank data, and actual MIDI events never get a look in. Slightly strange that my earlier UNO seems to be obeying the USB-MIDI protocol fine, but the almost-brand-new 2x2 isn't. (Both work under Linux.) Be glad to find out if the Roland works.

The idea of an identifying string rather than just a chain of numbers is definitely a good one. I haven't figured out quite how to get that string -- each app I've tried seems to have a different idea of what to show! listusb in Haiku gives the 2x2's name and manufacturer, but not the UNO's. lsusb under Linux gives neither, but gets "Midiman" from the vendor code for both. Linux aconnect identifies everything fully. I guess there are string descriptors in there somewhere (as stated in the spec), but the apps aren't retrieving them.

comment:7 Changed 10 years ago by phoudoin

Applied in hrev33126.

comment:8 in reply to:  7 Changed 10 years ago by phoudoin

Replying to phoudoin:

Applied in hrev33126.

Please test it, as my Roland UM-2 isn't support yet by usb_midi driver.

comment:9 Changed 10 years ago by Pete

Works as advertised with MidiSport UNO. (usb_midi driver was compiled from the 33126 changeset sources and plugged in to a couple of earlier releases, as the relevant builds aren't directly available at the moment.)

comment:10 Changed 9 years ago by phoudoin

Summary: Full-function USB MIDI driverAdd multi-ports and multi-devices support to USB MIDI driver

comment:11 Changed 8 years ago by phoudoin

Owner: changed from phoudoin to nobody
Status: in-progressassigned

comment:12 Changed 8 years ago by lt_henry

I've tried my Cakewalk (Roland) midi to usb adapter, UM-1G, Vendor and Product: 0582:0105. it didn't work, It received a constant stream of zeros. So, I swapped input/output endpoints and now it works, both input and output midi :) . However, neither original usb_midi nor this "patch" worked for another usb midi keyboard. I'm gonna provide more info about this (not at home currently).

BTW, what an awsome low latency has already Haiku!!

comment:13 in reply to:  12 Changed 8 years ago by Pete

Replying to lt_henry:

I've tried my Cakewalk (Roland) midi to usb adapter, UM-1G, Vendor and Product: 0582:0105. it didn't work, It received a constant stream of zeros. So, I swapped input/output endpoints and now it works, both input and output midi :).

I think you've cracked the case! That's exactly the problem I was getting with my MidiSport 2x2 -- a stream of zeros. Swapping the endpoints made it work for me too (but of course then kills it for my UNO interface).

So the comment in the code at that point ("May need more thought") is highly appropriate (:-)). Obviously we have to determine what kind each endpoint is, and set things appropriately. I'm looking into it now.

However, neither original usb_midi nor this "patch" worked for another usb midi keyboard. I'm gonna provide more info about this (not at home currently).

Yes please...

BTW, what an awsome low latency has already Haiku!!

Do you mean MIDI latency? Audio latency is horrendous on my current Haiku. I'm looking into that, too.

Changed 8 years ago by Pete

Attachment: usb_midi.2.diff added

Fix for assignment of IN and OUT Endpoints

comment:14 Changed 8 years ago by Pete

Has a Patch: set

comment:15 Changed 8 years ago by Pete

Summary: Add multi-ports and multi-devices support to USB MIDI driverAdd multi-ports support to USB MIDI driver

OK, I've added a check for Endpoint direction (attached patch), so that both my UNO and 2x2 interfaces work now.

However, the title of this ticket is still not addressed, because I can still only process MIDI on the first of the two ports on the 2x2. I can't find any sign of the second port, either reported by debug printouts on the driver or from listusb! Does anybody know how these are handled? (On Linux, ALSA sees both ports, but the old (OSS?) scheme also seems to find only the first.)

(I removed the "multi-devices" phrase in the heading, because, as was noted a while ago, that part has always worked perfectly well.)

comment:16 in reply to:  15 ; Changed 8 years ago by Pete

Replying to Pete:

However, the title of this ticket is still not addressed, because I can still only process MIDI on the first of the two ports on the 2x2. I can't find any sign of the second port, either reported by debug printouts on the driver or from listusb!

Well it didn't take long to find out how multi-ports are handled, now that the connection is actually working. All the packets are sent over the same endpoints, distinguished by the first nibble in the packet.

So, as the current usb_midi simply ignores that number, you can currently receive on any port, but only send on the first.

I'll start studying the reorganization needed to handle that port ID properly.

comment:17 in reply to:  14 ; Changed 8 years ago by korli

Replying to Pete:

Thanks for looking into that. I noticed a few style issues in the patch:

  • DPRINTF_INFO(("Interface count = %ld\n", conf->interface_count);) => ; is misplaced.
  • (intf->endpoint[0].descr->endpoint_address & 0x80) != 0 is the preferred style.
  • A space is also required before the ternary operator ?.

comment:18 in reply to:  16 ; Changed 8 years ago by phoudoin

Replying to Pete

Well it didn't take long to find out how multi-ports are handled, now that the connection is actually working. All the packets are sent over the same endpoints, distinguished by the first nibble in the packet.

So, as the current usb_midi simply ignores that number, you can currently receive on any port, but only send on the first.

I'll start studying the reorganization needed to handle that port ID properly.

For each port one distinct /dev/midi/*/* entry should be published in order to be handled as distinct port by midi_server. That means that depending on ports count for a device, one to several entries should published, each one with it's own channel number. But, and that's the meat of the code redesign, you still have one single receiving "thread" for all ports/device entries, and on the other side parallel writes on ports of the same device may needs to be serialized, but I'm less sure about it (I hope the USB stack already queues bulk requests on same endpoint).

By multi-devices, I was implying support for both multiple "hardware" devices *and* multiple /dev/midi/*/* entries for multi-ports hardware device. Sorry about the confusion.

comment:19 in reply to:  17 Changed 8 years ago by Pete

Replying to korli:

Replying to Pete:

Thanks for looking into that. I noticed a few style issues in the patch:

Thanks for the proofreading. I'll fix.

I'm not sure the patch is worth committing at this point, though. I think the test should be more general and fail-safe. For instance, maybe a USB keyboard controller only sends MIDI, but doesn't receive. I suspect the current scheme wouldn't like that.

comment:20 in reply to:  18 Changed 8 years ago by Pete

Replying to phoudoin:

Without repeating your comments, I think that's pretty much what I understand needs to be done. I don't think there'll be much difficulty in merging two write streams. What I have to get clear in my mind is how to create two "devices" from one hardware response.

By multi-devices, I was implying support for both multiple "hardware" devices *and* multiple /dev/midi/*/* entries for multi-ports hardware device. Sorry about the confusion.

Ah. Sorry if I misunderstood.

comment:21 Changed 8 years ago by lt_henry

Ok Pete, I've applied your patch and the Roland adaptor is working (as it did with my hack), however, midi keyboard instead of receiving a stream of zeros it receives one byte when plugged then nothing happens when hitting the keys, the same behaviour as default haiku driver :\ I left some logging as attachment, hope to help in anyway, tell me if there is something else I can do.

Last edited 8 years ago by lt_henry (previous) (diff)

Changed 8 years ago by lt_henry

Attachment: midi_0A92.txt added

Changed 8 years ago by Pete

Attachment: usb_midi.h added

Changed 8 years ago by Pete

Attachment: devlist.c added

comment:22 Changed 8 years ago by Pete

I think I have finally fulfilled the objective of this ticket! The usb_midi driver now handles Class Oompliant USB-MIDI devices with multiple MIDI ports (or 'cables' as the standard calls them). It also works [tested with simulation only] with devices that may be IN or OUT only, using Philippe's modified DeviceWatcher.cpp (hrev40419). It also needs his updated USBx.h in hrev40429/hrev40433.

A device/port pathname is now of the form "/dev/midi/usb/0-0" with the digit after the hyphen indicating the actual port.

I've attached a complete diff (replacing earlier ones). As the diff is rather larger than the combined size of the files themselves, I've attached the actual source code as well. (Hope that's approved procedure...) I've tried to clean up the sources a bit, but as the original is not exactly Haiku-Style, it still is nowhere near conformant. Hope that's OK.

Anybody wishing to try this on their own USB-MIDI device, please note that this is only likely to work with 'Class Compliant' devices. I've discovered that many are not! [Some Communication with Clemens Ladisch, who wrote the Linux usb_midi driver, reveals a raft of oddballs.] Please give it a try, though, and report on the results.

comment:23 Changed 8 years ago by anevilyak

Owner: changed from nobody to phoudoin
Version: R1/pre-alpha1R1/Development

Reassigning to Philippe since he's most likely to deal with it anyways.

comment:24 in reply to:  21 Changed 8 years ago by Pete

Replying to lt_henry:

Ok Pete, I've applied your patch and the Roland adaptor is working (as it did with my hack), however, midi keyboard instead of receiving a stream of zeros it receives one byte when plugged then nothing happens when hitting the keys, the same behaviour as default haiku driver :\ I left some logging as attachment, hope to help in anyway, tell me if there is something else I can do.

Sorry -- didn't notice/look at your attachment until today. It doesn't tell me much, I'm afraid. Everything looks normal except for that 'Control Pipe' error, which doesn't mean anything to me either (:-/). I looked up the vendor and product codes, and it seems to be an EGO KeyControl of some kind (the exact number is not actually in the list). According to Ladisch (see above), some EGO -- aka ESI -- units are nominally compliant, but maybe not all.

I doubt the newly updated driver is going to do any better on it, but that had better still work with the Roland! Please give it a whirl.

It also might be some help to do 'listusb -v /dev/bus/usb/nnn' whatever nnn is when you plug the EGO in. That would at least tell us how standard the interface looks. Maybe also repeat the syslog trace; I think the current output is much as before but it might show something.

Thanks.

comment:25 in reply to:  17 Changed 8 years ago by Pete

Replying to korli:

I noticed a few style issues in the patch:

  • DPRINTF_INFO(("Interface count = %ld\n", conf->interface_count);) => ; is misplaced.

Just a point of curiosity -- why do you consider it "misplaced"? (:-/) Don't see anything about it in the guidelines.

That's the way I've always done it with such 'elidable' macros. With that convention inactive macros completely disappear, not even leaving an empty-statement semicolon. The only hazard is if it's the sole statement in a conditional, but I've always added braces in a situation like that for clarity anyway.

It's no big deal, and I've changed all my added printouts to conform to the older ones, but I'd be interested to know if there is some strong reason for terminating with "));"

Changed 8 years ago by Pete

Attachment: usb_midi.diff added

Provides full multiport USB MIDI access for Class Compliant devices

comment:26 Changed 8 years ago by Pete

Updated the diff, because I realized I forgot a couple of the style fixes Korli asked for!

Changed 8 years ago by Pete

Attachment: usb_midi.c added

comment:27 Changed 8 years ago by korli

Pete, I had a look at usb_midi.diff (which I suppose is the latest diff).

I found little oddities:

  • function declarations like "add_port_info(usbmidi_port_info* port);" should have their return type on the same line, not the previous line.
  • before find_free_device_number(void) a blank line is missing
  • in the following line packet->cn value isn't checked against ports size:
    usbmidi_port_info* port = midiDevice->ports[packet->cn];
    
  • in the following lines, the style isn't correct:
    for (i=0; in_cables || out_cables; i++) {
    
    if (in_cables) in_cables--; 
    if (out_cables) out_cables--; 
    
    if (!port) break;
    
    for (cable=0; cable < 16; cable++) {
    
    for (i=0; i < intf->endpoint_count && i < 2; i++) {
    
  • this line should be an actual check (open_fd isn't a boolean).
    if (port->open_fd) { 
    
  • ports deletion seems a bit broken if the array isn't null terminated. Iterating on the index would be preferred.
    while (*port) { 
                 delete_sem((*port)->open_fd->sem_cb); 
                 /* done here to ensure read is freed */ 
                 port++; 
    } 
    

I'd switch to cpp after this patch to ease things.

comment:28 in reply to:  27 Changed 8 years ago by Pete

Replying to korli:

Pete, I had a look at usb_midi.diff (which I suppose is the latest diff).

Yes.

I found little oddities:

Heh -- If you look at the original source, you'll see it's really one big style violation...! It was written, I think, long before the guidelines were laid down, and in fact is mostly built on top of Takayuki's original usb_joy code. In general I tried to conform to the style it was written in, rather than 'Haikuize' the new bits. (Rewriting the whole thing is more than I wanted to take on right now.)

  • function declarations like "add_port_info(usbmidi_port_info* port);" should have their return type on the same line, not the previous line.

This is as in the original.

  • in the following line packet->cn value isn't checked against ports size:
    usbmidi_port_info* port = midiDevice->ports[packet->cn];
    

I don't think the USB protocol could ever cause a mismatch here. I don't think one needs to check every variable against limits!

I think some of your other points are a result of conforming to the existing style, but I'll check through them.

I'd switch to cpp after this patch to ease things.

Desirable, yes, but not a small job. (I'd really hope that needed updates like this won't be canned just because of older style. To my mind, consistency is better than a mixed style. Jerome was happy to keep the old forms when he did it...!)

comment:29 Changed 8 years ago by phoudoin

Status: assignedin-progress

comment:30 Changed 8 years ago by phoudoin

Patch applied with small changes (coding style, missing sanity checks) in hrev41874. I can't test it myself, so please you lucky guys with multi-ports usb midi device report any result.

comment:31 in reply to:  30 ; Changed 8 years ago by Pete

Replying to phoudoin:

Patch applied with small changes (coding style, missing sanity checks) in hrev41874. I can't test it myself, so please you lucky guys with multi-ports usb midi device report any result.

Thanks for doing this, Philippe. I guess you never actually compiled your changeset, though...? (:-)) You forgot that it's bloody C (:-)) and moved a declaration (with assignment) after a "statement"! (Happens that the "statement" was just the semicolon after a debug macro, but it was enough...)

I'm not sure what the best procedure here is. Doesn't seem worth attaching a complete new patch, so I'll just put the text of the fix here:

--- /BFS_User/haiku/haiku/CHANGESET41874/haiku/trunk/src/add-ons/kernel/drivers/midi/usb_midi/usb_midi.c	2011-06-03 02:30:02.036700160 -0700
+++ usb_midi.c	2011-06-04 21:53:09.073400320 -0700
@@ -199,11 +199,12 @@
 	size_t bytes_left = midiDevice->actual_length;
 	while (bytes_left) {	/* buffer may have several packets */
 		int pktlen = CINbytes[packet->cin];
+		usbmidi_port_info* port = midiDevice->ports[packet->cn];
+
 		DPRINTF_DEBUG((MY_ID "received packet %x:%d %x %x %x\n",
 			packet->cin, packet->cn,
 			packet->midi[0], packet->midi[1], packet->midi[2]));
 
-		usbmidi_port_info* port = midiDevice->ports[packet->cn];
 			/* port matching 'cable' */
 		if (port == NULL) {
 			DPRINTF_ERR((MY_ID "no port matching cable number %d!\n",

Can you do whatever's needed, please?

Once I edited as above, and built it, it works fine with my MidiSport 2x2

Thanks again.

comment:32 in reply to:  31 Changed 8 years ago by phoudoin

Resolution: fixed
Status: in-progressclosed

Replying to Pete:

I guess you never actually compiled your changeset, though...? (:-)) You forgot that it's bloody C (:-)) and moved a declaration (with assignment) after a "statement"! (Happens that the "statement" was just the semicolon after a debug macro, but it was enough...)

I did, but only with gcc4 (my bad) - which allow c++ declaration even in C - before committing hrev41874. Short after, the gcc2 Build-O-Matic failed, and I've fix the issue in hrev41877.

Sorry, I should have report here in which revision the patch was correctly applied. I've wrongly assumed you're following the commit mailing-list.

so I'll just put the text of the fix here: Can you do whatever's needed, please?

That's exactly what I did in hrev41877. Already ;-)

Once I edited as above, and built it, it works fine with my MidiSport 2x2

Nice, then this ticket can be closed. Thanks for your contribution.

comment:33 Changed 8 years ago by Pete

I thought this would have gotten into the alpha3 release, but having finally managed to get a bootable stick of this, I find it still has the ancient single-port version (which even KDLs if I boot with my Midisport 2x2 connected!). How come?

comment:34 Changed 6 years ago by phoudoin

I wonder why we read from port's ring buffer one byte at a time? Any reason?

Last edited 6 years ago by phoudoin (previous) (diff)

comment:35 in reply to:  34 Changed 6 years ago by Pete

Replying to phoudoin:

I wonder wy we read from port's ring buffer one byte at a time? Any reason?

" 'Cause that's the way Daddy does it..." (:-)) ('Daddy' in this case I guess being Jérôme...)

However, if you check, it turns out that the server only ever *asks* for a byte at a time, so it's a perfect match.

Note: See TracTickets for help on using tickets.