From 18f4c1d8a750878c4f86ac3d7319b8aa462a79f9 Mon Sep 17 00:00:00 2001 From: Sebastian Zagrodzki Date: Mon, 12 Aug 2019 21:38:32 +0200 Subject: [PATCH] Don't skip empty transfers. In isochronous transfers, make sure (#72) Don't skip empty transfers. Update the fake libusb to honor the endpoint max packet sizes. Update tests that were taking advantage of the fact that libusb allowed unlimited amount of data in any transfer packet. --- endpoint.go | 4 ---- endpoint_stream_test.go | 25 ++++++++++++------------ endpoint_test.go | 6 +++--- fakelibusb_test.go | 43 ++++++++++++++++++++++++++++++++++++----- transfer.go | 6 +++++- transfer_test.go | 12 ++++++++++-- 6 files changed, 68 insertions(+), 28 deletions(-) diff --git a/endpoint.go b/endpoint.go index fdb43fb..116a876 100644 --- a/endpoint.go +++ b/endpoint.go @@ -86,10 +86,6 @@ func (e *endpoint) String() string { } func (e *endpoint) transfer(ctx context.Context, buf []byte) (int, error) { - if len(buf) == 0 { - return 0, nil - } - t, err := newUSBTransfer(e.ctx, e.h, &e.Desc, len(buf)) if err != nil { return 0, err diff --git a/endpoint_stream_test.go b/endpoint_stream_test.go index 588f21a..b6c9b5e 100644 --- a/endpoint_stream_test.go +++ b/endpoint_stream_test.go @@ -70,7 +70,7 @@ func TestEndpointReadStream(t *testing.T) { } defer stream.Close() var got int - want := goodTransfers * 1024 + want := goodTransfers * 512 // EP max packet size is 512 buf := make([]byte, 1024) for got <= want { num, err := stream.Read(buf) @@ -130,31 +130,30 @@ func TestEndpointWriteStream(t *testing.T) { if err != nil { t.Fatalf("%s.Endpoint(1): %v", intf, err) } - pktSize := 1024 - stream, err := ep.NewStream(pktSize, 5) + bufSize := 1024 + stream, err := ep.NewStream(bufSize, 5) if err != nil { - t.Fatalf("%s.NewStream(%d, 5): %v", ep, pktSize, err) + t.Fatalf("%s.NewStream(%d, 5): %v", ep, bufSize, err) } defer stream.Close() for i := 0; i < 5; i++ { - if n, err := stream.Write(make([]byte, pktSize*2)); err != nil { + if n, err := stream.Write(make([]byte, bufSize*2)); err != nil { t.Fatalf("stream.Write: got error %v", err) - } else if n != pktSize*2 { - t.Fatalf("stream.Write: %d, want %d", n, pktSize*2) + } else if n != bufSize*2 { + t.Fatalf("stream.Write: %d, want %d", n, bufSize*2) } } - want := pktSize * 10 if err := stream.Close(); err != nil { t.Fatalf("stream.Close: got error %v", err) } - if got := stream.Written(); got != want { + if got, want := stream.Written(), 10240; got != want { // 5 stream.Writes, each with 2048 bytes t.Errorf("stream.Written: got %d, want %d", got, want) } done <- struct{}{} - if num != 10 { - t.Errorf("received transfers: got %d, want %d", num, 10) + if w := stream.Written(); total != w { + t.Errorf("received data: got %d, but stream.Written returned %d, results should be identical", total, w) } - if total != want { - t.Errorf("received data: got %d, want %d", total, want) + if wantXfers := 20; num != wantXfers { // transferred 10240 bytes, device max packet size is 512 + t.Errorf("received transfers: got %d, want %d", num, wantXfers) } } diff --git a/endpoint_test.go b/endpoint_test.go index 89d6b9d..d6be076 100644 --- a/endpoint_test.go +++ b/endpoint_test.go @@ -76,9 +76,9 @@ func TestEndpoint(t *testing.T) { }{ { desc: "empty buffer", - buf: nil, - ret: 10, - wantSubmit: false, + buf: []byte{}, + ret: 0, + wantSubmit: true, want: 0, }, { diff --git a/fakelibusb_test.go b/fakelibusb_test.go index f1c9321..faf4845 100644 --- a/fakelibusb_test.go +++ b/fakelibusb_test.go @@ -38,6 +38,12 @@ type fakeTransfer struct { // length is the number of bytes used from the buffer (write) or available // in the buffer (read). length int + // ep is the endpoint that this transfer was created for. + ep *EndpointDesc + // isoPackets is the number of isochronous transfers performed in a single libusb transfer + isoPackets int + // maxLength is the maximum number of bytes this transfer could contain + maxLength int } func (t *fakeTransfer) setData(d []byte) { @@ -199,11 +205,27 @@ func (f *fakeLibusb) setAlt(d *libusbDevHandle, intf, alt uint8) error { return nil } -func (f *fakeLibusb) alloc(_ *libusbDevHandle, _ *EndpointDesc, _ int, bufLen int, done chan struct{}) (*libusbTransfer, error) { +func (f *fakeLibusb) alloc(_ *libusbDevHandle, ep *EndpointDesc, isoPackets int, bufLen int, done chan struct{}) (*libusbTransfer, error) { f.mu.Lock() defer f.mu.Unlock() + maxLen := ep.MaxPacketSize + if isoPackets > 0 { + if ep.TransferType != TransferTypeIsochronous { + return nil, fmt.Errorf("alloc(..., ep: %s, isoPackets: %d, ...): endpoint is not an isochronous type endpoint, iso packets must be 0", ep, isoPackets) + } + maxLen = isoPackets * ep.MaxPacketSize + } + if bufLen > maxLen { + bufLen = maxLen + } t := newFakeTransferPointer() - f.ts[t] = &fakeTransfer{buf: make([]byte, bufLen), done: done} + f.ts[t] = &fakeTransfer{ + buf: make([]byte, bufLen), + ep: ep, + isoPackets: isoPackets, + maxLength: maxLen, + done: done, + } return t, nil } func (f *fakeLibusb) cancel(t *libusbTransfer) error { @@ -225,17 +247,28 @@ func (f *fakeLibusb) buffer(t *libusbTransfer) []byte { return f.ts[t].buf } func (f *fakeLibusb) data(t *libusbTransfer) (int, TransferStatus) { f.mu.Lock() defer f.mu.Unlock() - return f.ts[t].length, f.ts[t].status + ret := f.ts[t].length + if maxRet := f.ts[t].maxLength; ret > maxRet { + ret = maxRet + } + return ret, f.ts[t].status } func (f *fakeLibusb) free(t *libusbTransfer) { f.mu.Lock() defer f.mu.Unlock() delete(f.ts, t) } -func (f *fakeLibusb) setIsoPacketLengths(*libusbTransfer, uint32) {} +func (f *fakeLibusb) setIsoPacketLengths(t *libusbTransfer, length uint32) { + f.mu.Lock() + defer f.mu.Unlock() + maxLen := f.ts[t].isoPackets * int(length) + if bufLen := len(f.ts[t].buf); maxLen > bufLen { + maxLen = bufLen + } + f.ts[t].maxLength = maxLen +} // waitForSubmitted can be used by tests to define custom behavior of the transfers submitted on the USB bus. -// TODO(sebek): add fields in fakeTransfer to differentiate between different devices/endpoints used concurrently. func (f *fakeLibusb) waitForSubmitted(done <-chan struct{}) *fakeTransfer { select { case t, ok := <-f.submitted: diff --git a/transfer.go b/transfer.go index 45abf73..7eec5e1 100644 --- a/transfer.go +++ b/transfer.go @@ -131,7 +131,11 @@ func newUSBTransfer(ctx *Context, dev *libusbDevHandle, ei *EndpointDesc, bufLen if bufLen < isoPktSize { isoPktSize = bufLen } - isoPackets = bufLen / isoPktSize + if isoPktSize > 0 { + isoPackets = bufLen / isoPktSize + } else { + isoPackets = 1 + } debug.Printf("New isochronous transfer - buffer length %d, using %d packets of %d bytes each", bufLen, isoPackets, isoPktSize) } diff --git a/transfer_test.go b/transfer_test.go index 2a9b943..d4ed7c9 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -44,7 +44,7 @@ func TestNewTransfer(t *testing.T) { tt: TransferTypeBulk, maxPkt: 512, buf: 1024, - wantLength: 1024, + wantLength: 512, }, { desc: "iso out transfer, 3 * 1024B packets", @@ -52,7 +52,15 @@ func TestNewTransfer(t *testing.T) { tt: TransferTypeIsochronous, maxPkt: 3 * 1024, buf: 10000, - wantLength: 10000, + wantLength: 9216, + }, + { + desc: "iso out transfer, 512B packets", + dir: EndpointDirectionOut, + tt: TransferTypeIsochronous, + maxPkt: 512, + buf: 2048, + wantLength: 2048, }, } { xfer, err := newUSBTransfer(ctx, nil, &EndpointDesc{