raspberrypi / pico-feedback

24 stars 2 forks source link

[RESOLVED] USB Host raises TRANS_COMPLETE when it shouldn't #380

Open shreeve opened 6 months ago

shreeve commented 6 months ago

I am working on a TinyUSB-free USB Host library for the rp2040, called https://github.com/shreeve/pico-usb and am seeing an issue that I can't seem to resolve. I really like the TinyUSB library, but it is very large and pretty confusing. I'd like to create a much simpler, canonical pico-sdk based library.

In the following screenshot, the following registers are defined as:

INTR = raw USB IRQ interrupts
INTS = USB IRQ interrupts that we've requested
SSR = sie_status register
SCR = sie_ctrl register
DAR = addr_endp register (device and endpoint address)
ECR = EPX control register
BCR = EPX buffer control register
2024-02-22 00-25-13 - Monosnap

The issue that I'm having is that in the setup packet being transferred at the top of the image, bcr is set to 0x2462 which means that: the next packet is a DATA1 (the leading 2), that buf0 is AVAILABLE (the 4), and I'm requesting 0x62 (98 decimal) bytes. This should raise the BUFF_STATUS interrupt when the initial packet completes, but it should not raise the TRANS_COMPLETE interrupt, since bcr did not include USB_BUF_CTRL_LAST.

After the initial 0x40 (64 decimal) bytes are received, when I process this initial BUFF_STATUS, I set the bcr (shown as ~bcr~) to 0x6422 to indicate that the following packet will be the last one (since we only need to transfer the remaining 0x22 (34 decimal) bytes. The leading 6 includes the USB_BUF_CTRL_LAST bit (itself being a 4). I would expect the resulting packet based on the bcr of 0x6422 to raise the TRANS_COMPLETE interrupt and let me finish this request. However, the first packet raises TRANS_COMPLETE and I can't figure out why.

The datasheet shows that there are supposed to be only FOUR conditions that raise TRANS_COMPLETE:

2024-02-22 00-52-31 - RP2040 pdf

I don't think these four conditions apply to the first screenshot above.

My code is found at: https://github.com/shreeve/pico-usb/blob/main/host/host.c

If there's a way to not have TRANS_COMPLETE not be raised when it shouldn't, I would greatly appreciate the help. I hope it isn't some lower level issue like one of the few USB errata. My guess is that I'm just doing something wrong, but given that I have found absolutely zero other USB Host libraries for the rp2040 outside of TinyUSB, it's hard to know.

I'm not sure if this is even the correct place to post this question, but @lurch mentioned it might be worth a shot. If @kilograham or @aallan have any input that would be great also! Ideally, if this can be fixed, the code could be helpful to have a more flexible and lighter USB Host library to offer on the rp2040.

Thanks!

shreeve commented 6 months ago

For trying to force the error to be exposed more quickly, I forced the max ep0size to 64, called get_configuration_descriptor first, and requested 98 bytes from it also. The commit with these three tweaks is: https://github.com/shreeve/pico-usb/commit/f89ebb3f89076dd3fdbfe167f54dd82cf0220248

I am testing this on a Pico W connected via one Pico Debug Probe to debug the Pico W that is running as a USB Host. I'm then connected through the USB Phy to another Pico Debug Probe that I'm using as a test USB Device.

aallan commented 6 months ago

This sounds like a @kilograham question, or potentially @liamfraser as he's been deeply involved in USB things, if either has the time right now to look into it. Sorry, but you're into deeper into the USB magic than I've poked at myself.

liamfraser commented 6 months ago

Trans complete is also raised on a short packet. So if you said you were going to receive 64 bytes and received less than that, trans complete is raised. For anything other than setup packets, you can choose to ignore trans complete and just use the buffer done interrupts. You can detect a short packet by checking the length in the buffer control register and comparing it to the length you wanted to receive.

shreeve commented 6 months ago

Ah! @liamfraser, that's probably it! If so, then the documentation should be updated to reflect when TRANS_COMPLETE will be raised. Is this list correct?

  1. A SETUP packet is sent without a following IN or OUT transaction
  2. An IN or OUT packet is sent and the LAST_BUFF bit is set in the buffer control register
  3. An IN or OUT packet is sent with zero length or is a short packet (less than the amount requested in the buffer control register).

@aallan - I will see if the short packet was the issue.

shreeve commented 6 months ago

@liamfraser and @aallan - Yes, this was the exact issue! THANK YOU!

I've been banging my head for a few days on this and that completely solved the issue.

I've now implemented double-buffering support also in the code at: https://github.com/shreeve/pico-usb/blob/01332748e4eb6fe656518a7c074901a19d2c04f9/host/host.c -- I still need to review and test it, but it's essentially there. I just need to beat it up and make sure it's solid.

aallan commented 6 months ago

@liamfraser Are there any documentation changes that need doing as a result of this one?

shreeve commented 5 months ago

FWIW, here's the list that I came up with for when TRANS_COMPLETE is raised (see datasheet, p. 401):

  1. A SETUP packet is sent without {RECEIVE,SEND}_DATA_BITS in SCR
  2. A packet is transferred with LAST bit set in BCR
  3. A short packet (less than maxsize) is transferred

@aallan - If @liamfraser can confirm this is correct, it would be very helpful to update the datasheet.

Thanks!

liamfraser commented 5 months ago

Yep, I can get this done. It's easier to update in bulk so let's document the outcome of your #387 issue also.

aallan commented 5 months ago

Thanks @liamfraser!