USBInTransferManager does not toggle DATA when sending ZLP

[ronyrus]

ZLP is sent with the DATAX PID, which is supposed to be toggled from the previous value.
It seems USBInTransferManager is not doing that.

When sending ZLP, we do need to toggle DATA, but we don't need to swap the buffers.
Right now, these two functions are done by the same signal.
Hence we need to split them.

[miek]

Great find! I can confirm this in practice too.

I setup a test by patching the bulk_in_speed_test applet to limit the transfer size on the device side:

diff --git a/applets/bulk_in_speed_test.py b/applets/bulk_in_speed_test.py
index d332a62..a2b6c39 100755
--- a/applets/bulk_in_speed_test.py
+++ b/applets/bulk_in_speed_test.py
@@ -199,10 +199,16 @@ else:
             )
             usb.add_endpoint(stream_ep)
 
+            TRANSFER_SIZE = 8191
+            counter = Signal(range(TRANSFER_SIZE))
+            with m.If(stream_ep.stream.ready):
+                m.d.usb += counter.eq(Mux(stream_ep.stream.last, 0, counter + 1))
+
             # Send entirely zeroes, as fast as we can.
             m.d.comb += [
                 stream_ep.stream.valid    .eq(1),
-                stream_ep.stream.payload  .eq(0)
+                stream_ep.stream.payload  .eq(0),
+                stream_ep.stream.last     .eq(counter == TRANSFER_SIZE-1),
             ]
 
             # Connect our device as a high speed device by default.
@@ -245,6 +251,7 @@ def run_speed_test():
 
             # Count the data exchanged in this packet...
             total_data_exchanged += transfer.getActualLength()
+            print(transfer.getActualLength())
 
             # ... and if we should terminate, abort.
             if _should_terminate():

Which gives the following output as expected:

$ python3 bulk_in_speed_test.py
INFO    | __init__    | Building and uploading gateware to attached LUNA r0.4...
INFO    | __init__    | Upload complete.
INFO    | bulk_in_speed_test| Giving the device time to connect...
INFO    | bulk_in_speed_test| Starting bulk in speed test.
8191
8191
[...]
8191
8191
INFO    | bulk_in_speed_test| Exchanged 1.056639MB total at 44.90738769524466MB/s.

If I change the TRANSFER_SIZE to 8192 (a multiple of the packet size) to force ZLPs to be generated, the output is:

$ python3 bulk_in_speed_test.py
INFO    | __init__    | Building and uploading gateware to attached LUNA r0.4...
INFO    | __init__    | Upload complete.
INFO    | bulk_in_speed_test| Giving the device time to connect...
INFO    | bulk_in_speed_test| Starting bulk in speed test.
16384
16384
[...]
16384
16384
INFO    | bulk_in_speed_test| Exchanged 1.06496MB total at 44.79981934546913MB/s.

Suggesting that the ZLPs are being dropped by the host because the PID is incorrect, and the transfer size is limited to 16384 as defined by the host.

After applying your patch, the transfer size is correct for the second test.

[miek]

I think there might be a similar bug in exiting the WAIT_FOR_DATA state. It does need to swap the buffers, but I'm not sure it should be toggling the data pid there?

[miek]

I think there might be a similar bug in exiting the WAIT_FOR_DATA state. It does need to swap the buffers, but I'm not sure it should be toggling the data pid there?

Nevermind, that is fine - it's just done there instead of right after the ACK because it couldn't swap the buffer yet. I may follow up with a commit to simplify things after merging the PR.

[ronyrus]

I was AFK for some time, sorry for the lack of response.
Yeah, other cases seems to be OK and I agree, it would be great to simplify the code in this area.