Add multi-ports support to USB MIDI driver

[Pete]

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.

[Pete]

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

[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... ;-)

[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...

[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

[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.

[phoudoin]

Applied in hrev33126.

[phoudoin]

Replying to phoudoin:

Applied in hrev33126.

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

[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.)

[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!!

[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.

[Pete]

Fix for assignment of IN and OUT Endpoints

[Pete]

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.)

[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.

[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 ?.

[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.

[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.

[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.

[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.

[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.

[anevilyak]

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

[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.

[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 "));"

[Pete]

Provides full multiport USB MIDI access for Class Compliant devices

[Pete]

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

[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.

[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...!)

[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.

[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.

[phoudoin]

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.

[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?

[phoudoin]

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

[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.