greatscottgadgets / luna

Amaranth HDL framework for monitoring, hacking, and developing USB devices
https://greatscottgadgets.com/cynthion/
BSD 3-Clause "New" or "Revised" License
975 stars 169 forks source link

Enumeration trouble, part 1 #86

Closed jboone closed 3 years ago

jboone commented 3 years ago

I'm having trouble with a long USB 2.0 configuration descriptor -- 75 bytes.

Initially, I was seeing my Linux host complaining in dmesg about being unable to read the descriptor, after I'd added another interface with two alternate settings. Here's how the problematic descriptor looks:

with descriptors.DeviceDescriptor() as d:
    d.idVendor  = 0x16d0
    d.idProduct = 0x0f3b
    d.iManufacturer = "ShareBrained"
    d.iProduct      = "Tedium X8"
    d.iSerialNumber = "deadbeef"
    d.bNumConfigurations = 1

with descriptors.ConfigurationDescriptor() as c:

    with c.InterfaceDescriptor() as i:
        i.bInterfaceNumber = 0

        with i.EndpointDescriptor() as e:
            e.bEndpointAddress = USBDirection.IN.to_endpoint_address(1)
            e.wMaxPacketSize   = 64
            e.bmAttributes     = USBTransferType.INTERRUPT
            e.bInterval        = 4

    with c.InterfaceDescriptor() as i:
        i.bInterfaceNumber = 1
        i.bAlternateSetting = 0

    with c.InterfaceDescriptor() as i:
        i.bInterfaceNumber = 1
        i.bAlternateSetting = 1

        with i.EndpointDescriptor() as e:
            e.bEndpointAddress = USBDirection.IN.to_endpoint_address(2)
            e.wMaxPacketSize   = (1 << 11) | 24
            e.bmAttributes     = USBTransferType.ISOCHRONOUS
            e.bInterval        = 1

    with c.InterfaceDescriptor() as i:
        i.bInterfaceNumber = 2
        i.bAlternateSetting = 0

    with c.InterfaceDescriptor() as i:
        i.bInterfaceNumber = 2
        i.bAlternateSetting = 1

        with i.EndpointDescriptor() as e:
            e.bEndpointAddress = USBDirection.OUT.to_endpoint_address(3)
            e.wMaxPacketSize   = (1 << 11) | 24
            e.bmAttributes     = USBTransferType.ISOCHRONOUS
            e.bInterval        = 1

I built a test case based on FullDeviceTest that demonstrates the problem in simulation:

# Read our configuration descriptor (with subordinates).
try_config_length = 33
handshake, data = yield from self.get_descriptor(DescriptorTypes.CONFIGURATION, length=try_config_length)
self.assertEqual(handshake, USBPacketID.ACK)
self.assertEqual(bytes(data), self.descriptors.get_descriptor_bytes(DescriptorTypes.CONFIGURATION)[:try_config_length])

If I set try_config_length to 32 or less, the test case passes. If 33 or larger, it fails. Examining the simulation output in GTKWave, I tracked the apparent culprit to this line in ConstantStreamGenerator, which appears to be sized wrong (5 bits wide) for the intent of max_length_width, which I take to express the number of bits used to represent max_length. Indeed, if I change that line from:

bytes_sent     = Signal.like(self._max_length_width)

...to:

bytes_sent     = Signal(self._max_length_width)

...the test case passes. That is, until I reach try_config_length of 64, which fails for a different reason which I imagine is a separate issue.

...
  File "./enumerate_test.py", line 155, in test_enumeration
    handshake, data = yield from self.get_descriptor(DescriptorTypes.CONFIGURATION, length=try_config_length)
  File "/home/jboone/src/fpga/luna/luna/gateware/test/usb2.py", line 512, in get_descriptor
    descriptor = yield from self.control_request_in(0x80,
  File "/home/jboone/src/fpga/luna/luna/gateware/test/usb2.py", line 434, in control_request_in
    pid, packet = yield from self.in_transfer(data_pid=USBPacketID.DATA1)
  File "/home/jboone/src/fpga/luna/luna/gateware/test/usb2.py", line 350, in in_transfer
    pid, packet = yield from self.in_transaction(
  File "/home/jboone/src/fpga/luna/luna/gateware/test/usb2.py", line 323, in in_transaction
    self.assertEqual(pid, data_pid.byte())
AssertionError: 75 != <USBPacketID.128|64|DATA0|ACK|OUT: 195>

If I were to guess, this is a wMaxPacketSize0 issue, but that's just intuition speaking, not real data.

I'm holding off submitting a pull request, because I'm skeptical my ConstantStreamGenerator change is the extent of what's necessary. I just don't grasp the code well enough yet to know that the change I made was the appropriate one, as the meaning of max_length and max_length_width seem a bit muddled in my brain...

hansfbaier commented 3 years ago

This is what the symptoms look like in wireshark. Request: image Response: image

hansfbaier commented 3 years ago

Here is my UAC2 example: https://github.com/hansfbaier/luna/blob/uac2/examples/usb/usb2_audio.py

zyp commented 3 years ago

Hi, has there been any progress on this so far? We're running into the same problem and I just want to make sure that I'm not duplicating any effort if I start digging into this.

hansfbaier commented 3 years ago

@zyp No I have not investigated this. I will steer clear. Thanks for looking into that!

zyp commented 3 years ago

Per https://github.com/greatscottgadgets/luna/blob/main/luna/gateware/usb/usb2/control.py#L201, support for control responses larger than 64B is simply not implemented yet.

I hacked together a working solution, but I've got a meeting coming up in a few minutes, so I don't have time to clean it up into something PR-worthy right now, but here's the patch if anybody wants a go: https://paste.jvnv.net/view/D81fZ

image

twam commented 3 years ago

Sorry for the late replay. We already know that this is missing.

@ktemkin even had some hints on IRC and Twitter (?) how this is supposed to be fixed.

hansfbaier commented 3 years ago

@zyp Wow you're awesome, thanks!

hansfbaier commented 3 years ago

@zyp What application is that screenshot from?

zyp commented 3 years ago

@hansfbaier Total Phase Data Center.

@twam I'd appreciate an URL to those hints so I can adapt my solution accordingly.

twam commented 3 years ago

I asked about the issue on 2021-02-08 in #luna-usb on FreeNode. The 1bitsquared discord (https://1bitsquared.com/pages/chat) has a history of that in the #luna channel. I can also provide you a log/screenshots if you reach out via @twamueller on Twitter or twam@twam.info on mail.

zyp commented 3 years ago

Relevant line from the chat history:

there's a InTransferManager meant to be used in the control EP that's currently not patched in properly

I've now made an attempt at doing that, but that's a more involved change than my workaround from yesterday and I have not managed to get everything to work correctly, so I think I'll pass on this for now and stick with the workaround until somebody have time to solve it properly.

hansfbaier commented 3 years ago

@zyp I tried the fix, but got this:

[ 2988.184141] usb 5-4: new high-speed USB device number 16 using xhci_hcd
[ 2988.333853] usb 5-4: unable to read config index 0 descriptor/start: -71
[ 2988.333857] usb 5-4: can't read configurations, error -71
[ 2988.460145] usb 5-4: new high-speed USB device number 17 using xhci_hcd
[ 2988.609851] usb 5-4: unable to read config index 0 descriptor/start: -71
[ 2988.609854] usb 5-4: can't read configurations, error -71

image

ktemkin commented 3 years ago

This should be fixed, as of https://github.com/greatscottgadgets/luna/commit/bf54d334eda178c7eacb3f5df6097876e130e11e.

image

I'm optimistically closing the issue, but I very much welcome all testing. :)

twam commented 3 years ago

Thanks a lot! This is awesome news! I'll try to give it a test on the week-end!

hansfbaier commented 3 years ago

Thank you so much, Kate! I am thrilled to give it a go!

hansfbaier commented 3 years ago

I tried with the MIDI interface example (which I pushed to the pull request yesterday), and the get configuration request gets NAK'ed as it seems: image

twam commented 3 years ago

Sorry for the late feedback, but also works for me like a charm.