Skip to content

Fix Endpoint descriptor size for MIDI Device#688

Merged
hathach merged 1 commit into
hathach:masterfrom
duddie:master
Mar 3, 2021
Merged

Fix Endpoint descriptor size for MIDI Device#688
hathach merged 1 commit into
hathach:masterfrom
duddie:master

Conversation

@duddie
Copy link
Copy Markdown
Contributor

@duddie duddie commented Feb 28, 2021

Endpoint descriptor should be 9 bytes in length (not 7) and have two extra bytes at the end: bRefresh and bSynchAddress

According to MIDI USB specification 1.0 (6.2.1 Standard MS Bulk Data Endpoint Descriptor)

This is actual error before fix:

Endpoint Descriptor (Audio/MIDI 1.0):

0x07 bLength
0x05 bDescriptorType
*** ERROR: Invalid descriptor length 0x07
Hex dump:
0x07 0x05 0x81 0x02 0x40 0x00 0x00

Endpoint descriptor should be 9 bytes in length (not 7) and have two extra bytes at the end: bRefresh and bSynchAddress

According to MIDI USB specification 1.0 (6.2.1 Standard MS Bulk Data Endpoint Descriptor)
Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endpoint descriptor is always 7 (USB specs). There is probably some mis-handling somewhere.If you provide further details on how you test it, people could help to clear it up ( I am assuming you are using midi_test example, if you don't please use that example )

@pigrew
Copy link
Copy Markdown
Collaborator

pigrew commented Mar 3, 2021

endpoint descriptor is always 7 (USB specs). There is probably some mis-handling somewhere.If you provide further details on how you test it, people could help to clear it up ( I am assuming you are using midi_test example, if you don't please use that example )

It does appear that for audio class, the descriptors are length 9. This is mentioned as a note in the USB 2.0 spec (section 9.6):

9.6 Standard USB Descriptor Definitions

The standard descriptors defined in this specification may only be modified or extended by revision of the
Universal Serial Bus Specification.

Note: An extension to the USB 1.0 standard endpoint descriptor has been published in Device Class
Specification for Audio Devices Revision 1.0. This is the only extension defined outside USB Specification
that is allowed. Future revisions of the USB Specification that extend the standard endpoint descriptor will
do so as to not conflict with the extension defined in the Audio Device Class Specification Revision 1.0.

So, to be standards compliant, descriptors should be modified to have length 9 for all audio-class endpoints. I don't know if this could cause any compatibility issues (or not), but it seems like this has been this way since 1998.

I think the endpoint descriptors for other audio classes also need to be extended to 9.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Mar 3, 2021

Thanks @pigrew @duddie for the issue, this is really a kick in the ass 🤦 🤦 . How can i miss this !!!!

image

@hathach
Copy link
Copy Markdown
Owner

hathach commented Mar 3, 2021

I just double checked with Audio 2.0 and 3.0 specs. USB-IF fixed this by using only 7 bytes for endpoint descriptor, that would be an untold story.

Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the PR, that was me being too confident. Sighed !!! PR is good for merge, though I think we need to put a bit of note in the comment to clarify it

@hathach hathach merged commit 68db108 into hathach:master Mar 3, 2021
@pigrew
Copy link
Copy Markdown
Collaborator

pigrew commented Mar 3, 2021

@hathach ,

Eeek, yeah, I didn't even notice that USB-MIDI 2.0 and USB AUDIO 2.0 and 3.0 use the 7-byte descriptor:

USB-MIDI spec 2.0: 5.3.1:

Note: In the USB MIDI 1.0 specification, the Standard MIDI Streaming Data Endpoint Descriptor
includes two additional fields, bRefresh, and bSynchAddress. These fields are not included in
USB MIDI 2.0.

It looks like the class driver here is for USB-MIDI 1.0, so this patch is correct. This library's other USB audio class uses audio 2.0, using the 7-byte descriptor, so that's correct as well.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Mar 4, 2021

@pigrew I just now know about the exist of USB MIDI v2.0 which just come out in May 2020. It is probably too soon for us to support. We probably stick to MIDI v1.0 until its support is stable. I think Audio v1 is probably the only spec that use 9-byte endpoint descriptor. It must be written at the same time with the USB 2.0 specs, and they are just too late to adjust, I guess.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Mar 4, 2021

@dhalbert Circuitpython should also update its MIDI endpoints descriptor as well, the correct endpoint size is 0x09. Just notice cpy is also 0x07.

0x07, 0x05, 0x06, 0x02, 0x40, 0x00, 0x00, // <adafruit_usb_descriptor.standard.EndpointDescriptor object at 0x7fe81cffbdc0>
0x05, 0x25, 0x01, 0x01, 0x01, // <adafruit_usb_descriptor.midi.DataEndpointDescriptor object at 0x7fe81cffbdf0>
0x07, 0x05, 0x86, 0x02, 0x40, 0x00, 0x00, // <adafruit_usb_descriptor.standard.EndpointDescriptor object at 0x7fe81cffbe80>
0x05, 0x25, 0x01, 0x01, 0x03, // <adafruit_usb_descriptor.midi.DataEndpointDescriptor object at 0x7fe81cffbeb0>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants