usb_midi fix to handle more "Class-Compliant" devices

[Pete]

This is an update to the usb_midi driver that seems to fix an incompatibility with some devices that are claimed to be "Class-Compliant" (in particular the iCON unit tested by Giovanni Mugnai. Thanks!) The driver was extensively developed in ticket #4463, but that was long ago closed, so I think a new ticket is preferable.

The problem seems to be the understanding of buffer/packet sizes. For receiving MIDI, the driver provides a 2048 byte buffer, and originally it passed this size to the queue request. This works fine with my M-Audio/MidiMan units, which just send each event as it's received.

The iCON device, though, seems to take that 2048 bytes as what it should buffer before it sends anything, and obligingly does so! Each USB packet sent to the device callback is 2048 bytes of 4-byte events (when that many are finally accumulated...).

The endpoint descriptor however specifies a maximum packet size of 4 bytes (one MIDI event) so the driver now passes this value from the descriptor to the queue request, rather than the buffer size. As a result the iCON now sends each event as it arrives. (The M-AUdio units have a max packet size of 64, and apparently are sensible about what to send when.)

The necessary patch is attached. [Yes, I know, it is as guideline deficient as the rest of the driver. (:-/)]

[korli]

Hi Pete,

the name of the new struct variables should be more like: in_buffer_size, out_buffer_size. Also why did you split the else and the if lines 73-74?

[Pete]

Replying to korli:

Hi Pete,

the name of the new struct variables should be more like: in_buffer_size, out_buffer_size.

Hmm. Are you objecting to the camelCase or the "...Pkt" nomenclature? As to the first, I saw no harm in starting to update the convention (:-/), but if you'd prefer consistency I can easily change it. I carefully avoided using the term "buffer" in any form, because the actual buffers remain the same size (2048 bytes), while the variables refer to the packets being transferred.

Also why did you split the else and the if lines 73-74?

If you look at the code immediately above, you'll see a 'parallel' if statement (66-72 in the diff). The else actually pairs with the if on line 65, but with the original layout it was very hard to parse this visually; the indentation of the second if block was different from the first. Adding redundant braces and matching the indentation has made the logic much clearer. (Might help to look at the original source.)

[phoudoin]

I wonder if in fact the root issue is not deeper, in the USB stack itself. There is absolutely no reason to pass the bulk request with 2048 size while the stack knows that this endpoint only support 4 bytes max per request.

Clamping it in usb_midi is one way, but maybe it's in the stack itself that it should be done. In particular because the USB stack's queue_bulk() never required to pass a buffer no bigger than the endpoint's max packet size...

[mmlr]

Replying to phoudoin:

I wonder if in fact the root issue is not deeper, in the USB stack itself. There is absolutely no reason to pass the bulk request with 2048 size while the stack knows that this endpoint only support 4 bytes max per request.

That is not how it works. The max packet size is just that, the maximum size of each packet. A request will be fulfilled using as many packets as is required to move the amount of data it is given. The stack splits these buffers correctly into multiple packets internally. It's an implementation detail if you will. It allows a device to work with a fixed and small amount of storage if needed.

Clamping it in usb_midi is one way, but maybe it's in the stack itself that it should be done. In particular because the USB stack's queue_bulk() never required to pass a buffer no bigger than the endpoint's max packet size...

That is right, the queue_bulk() call does not have buffer size restrictions. The usb_disk driver does transfer large amounts of data using single calls to queue_bulk() for example, something that would otherwise be really impractical.

The device classes usually have specifications that tell what kind of limit requests as a whole should have, or in what manner these should be communicated. Some have generic descriptors, some may use the max packet size, some use requests, others use higher level protocols to determine these limits.

Looking at the specs the packet sizes aren't specified. Looking at the linux usb midi driver (​http://lxr.linux.no/linux+v3.7.1/sound/usb/midi.c) they use the max packet size as the max transfer size by default. It is also clear from looking at this driver that there are quite a few broken devices and a lot of vendor specific protocols that are slightly different.

So using the max packet size probably makes sense. Using such short requests does impact performance of course as the overhead per request is quite considerable. However devices that allow for larger buffers will probably advertise them and lessen this issue. It is not a big issue overall, as for example USB HID transfers are usually in the "couple of bytes" range as well (though these are usually not streaming either).

[Pete]

Replying to mmlr:

Replying to phoudoin:

I wonder if in fact the root issue is not deeper, in the USB stack itself. There is absolutely no reason to pass the bulk request with 2048 size while the stack knows that this endpoint only support 4 bytes max per request.

That is not how it works. The max packet size is just that, the maximum size of each packet. A request will be fulfilled using as many packets as is required to move the amount of data it is given. The stack splits these buffers correctly into multiple packets internally. It's an implementation detail if you will. It allows a device to work with a fixed and small amount of storage if needed.

Ahh. I'm glad you jumped in on this... I'm still not quite clear on what's going on, though. How does the iCON device "get the idea" that it should wait until it has accumulated 2048 bytes before sending any? My M-Audio units have a max packet size of 64, yet they don't wait for 16 events before sending.

Looking at the specs the packet sizes aren't specified. Looking at the linux usb midi driver (​http://lxr.linux.no/linux+v3.7.1/sound/usb/midi.c) they use the max packet size as the max transfer size by default. It is also clear from looking at this driver that there are quite a few broken devices and a lot of vendor specific protocols that are slightly different.

Indeed... (:-/)

So using the max packet size probably makes sense.

So, in summary, I guess my patch is probably right(?). (At some point I guess we have to copy all the Linux quirk workarounds, but that's probably down the road.)

[Pete]

Up[dated (and git-formatted) patch for usb_midi driver

[Pete]

I found a stupid (long-standing) logic reversal in the driver: it was identifying an input-only port as output only! This rather messed up my connection to my new Axiom 49v2 controller...

This has been fixed, and a revised patch is attached. It replaces my attachment from 4 months ago (as I wasn't using the approved git mechanism then).

I'd be glad if somebody could commit this, so I don't have to keep sending special binaries to those folks I know are using MIDI. Thanks.

[phoudoin]

Patch applied in hrev45600. Thanks.

[Pete]

Replying to phoudoin:

Patch applied in hrev45600. Thanks.

Many thanks, Philippe.

I've since found, unfortunately, that we probably are still not quite there... (:-() Last night I plugged my digital piano into the Axiom's MIDI IN port, and was hammering on both keyboards, when the stream from the piano just stopped! It would resume again a few seconds later, but soon after would freeze once more. I doubt that M-Audio could get away with a bug like that in the firmware, so it almost certainly is the driver.

If I go back to separate USB devices -- the Axiom and my MidiSport 2x2 -- I don't seem to have any problem, so I suspect it's the two ports on one device. The 2x2 also has two ports, so I'll feed that from both keyboards, and I'll also check that it doesn't happen in Linux.

I'll leave this ticket closed until I know more. [BTW, is the approved procedure to reopen this one for such a continuation, or to open a new one?]

[Added later] Maybe not... I've had the same setup running for a couple of hours today, working it as hard as I can, and haven't seen the slightest glitch. Yet it was fairly reproducible last night. [And, yes, I did double check then that all the connectors were firmly seated! (:-)] So, until it happens again...

[phoudoin]

If I understand you well, the issue raise when you plug a second device to the same multiport MIDI in devices, right?

Sounds like a bug in input midi channel, either multiplexing or deframing.

Regarding ticket reopen policy, I guess the issue don't seems related at all to *this* ticket context.But far more to #4463, which should be the one to reopen, then.

[Pete]

Replying to phoudoin:

If I understand you well, the issue raise when you plug a second device to the same multiport MIDI in devices, right?

Exactly. And of course today it has just happened again. So I promptly switched to the debug version of the driver -- and can no longer get it to happen! It begins to sound as if there's some uncontrolled state which may occur at boot-up or something.

I'll just have to keep watchful for a while, I guess. (And leave to debug driver in place.) May be a slow business.

Sounds like a bug in input midi channel, either multiplexing or deframing.

Yes, but I don't see where. I walked through the code yesterday, and it all looks very straightforward. The input buffer gets filled by the USB stack, and the callback gets called, invoking the 'interpret buffer' routine, which writes to the appropriate circular buffer to be picked up by the server. How *one* of the two 'cables' could be ignored while the other sails along is hard to understand.

I'm beginning to wonder if it *is* in fact an Axiom bug....

Regarding ticket reopen policy, I guess the issue don't seems related at all to *this* ticket context.But far more to #4463, which should be the one to reopen, then.

OK, thanks. When the time comes...

[Pete]

Replying to Pete:

Fortunately it *did* happen again (repeatably), with the debug driver in place. And I'm seriously beginning to think that Haiku -- and usb_midi -- are off the hook! I can see each packet arriving, and then there'll be an illegal (but well-formed -- CIN = 0) packet on "cable 2" (the channel from the MIDI-IN connector), and then no more. Meanwhile packets on cable 0 (Axiom keyboard) keep arriving as normal. Given that the only entities that know anything about USB-MIDI "cables" are the driver itself and the Axiom, the only conclusion is that the Axiom is not sending them.

I have a support request in to M-Audio. We'll see what they say next week.

[Pete]

Replying to Pete:

Fortunately it *did* happen again (repeatably), with the debug driver in place. And I'm seriously beginning to think that Haiku -- and usb_midi -- are off the hook!

Just to close this out... This is definitely just an Axiom bug. I tested on Linux instead (2 different machines) and see exactly the same problem.

I have a support request in to M-Audio. We'll see what they say next week.

And so far have only heard back from the usual tech support zombies. I expect I'll just have to live with it.