Skip to content

Commit

Permalink
read w/o timeout now only throws exception on connection lost
Browse files Browse the repository at this point in the history
partly revert f4166f3, as there might be unkown reasons for empty response
  • Loading branch information
kai-morich committed Apr 20, 2021
1 parent 128d1a0 commit 5f94a47
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@
import java.util.EnumSet;
import java.util.List;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import static org.hamcrest.CoreMatchers.anyOf;
import static org.hamcrest.CoreMatchers.equalTo;
Expand Down Expand Up @@ -985,7 +988,7 @@ public void writeFragments() throws Exception {
@Test
public void readBufferSize() throws Exception {
// looks like devices perform USB read with full mReadEndpoint.getMaxPacketSize() size (32, 64, 512)
// if the Java byte[] is shorter than the received result, it is silently lost with read timeout!
// if the buffer is smaller than the received result, it is silently lost
//
// for buffer > packet size, but not multiple of packet size, the same issue happens, but typically
// only the last (partly filled) packet is lost.
Expand All @@ -1012,7 +1015,7 @@ public void readBufferSize() throws Exception {
else if (usb.serialDriver instanceof ProlificSerialDriver)
Assert.assertTrue("expected > 0 and < 16 byte, got " + data.length, data.length > 0 && data.length < 16);
else // ftdi, ch340, cp2105
fail("buffer to small exception expected");
Assert.assertEquals(0, data.length);
} catch (IOException ignored) {
}
if (purge) {
Expand Down Expand Up @@ -1474,48 +1477,38 @@ public void writeAsync() throws Exception {

@Test
public void readTimeout() throws Exception {
final Boolean[] closed = {Boolean.FALSE};

Runnable closeThread = () -> {
try {
Thread.sleep(100);
} catch (InterruptedException e) {
e.printStackTrace();
}
usb.close();
closed[0] = true;
};

usb.open(EnumSet.of(UsbWrapper.OpenCloseFlags.NO_IOMANAGER_THREAD));
usb.setParameters(19200, 8, 1, UsbSerialPort.PARITY_NONE);
telnet.setParameters(19200, 8, 1, UsbSerialPort.PARITY_NONE);

ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
ScheduledFuture<?> future;
byte[] writeBuf = new byte[]{1};
byte[] readBuf = new byte[1];
if (usb.serialDriver instanceof FtdiSerialDriver)
readBuf = new byte[3]; // include space for 2 header bytes
int len,i,j;
long time;

usb.open(EnumSet.of(UsbWrapper.OpenCloseFlags.NO_IOMANAGER_THREAD));
usb.setParameters(19200, 8, 1, UsbSerialPort.PARITY_NONE);
telnet.setParameters(19200, 8, 1, UsbSerialPort.PARITY_NONE);

// w/o timeout
telnet.write(writeBuf);
len = usb.serialPort.read(readBuf, 0); // not blocking because data is available
assertEquals(1, len);

time = System.currentTimeMillis();
closed[0] = false;
Executors.newSingleThreadExecutor().submit(closeThread);
future = scheduler.schedule(() -> usb.close(), 100, TimeUnit.MILLISECONDS);
try {
usb.serialPort.read(readBuf, 0); // blocking until close()
fail("read error expected");
} catch (IOException ignored) {}
assertTrue(System.currentTimeMillis()-time >= 100);
// wait for usbClose
for(i=0; i<100; i++) {
if(closed[0]) break;
Thread.sleep(1);
len = usb.serialPort.read(readBuf, 0); // blocking until close()
assertEquals(0, len);
} catch (IOException ignored) {
// typically no exception as read request canceled at the beginning of close()
// and most cases the connection is still valid in testConnection()
} catch (Exception ignored) {
// can fail with NPE if connection is closed between closed check and queueing/waiting for request
}
assertTrue("not closed in time", closed[0]);
assertTrue(System.currentTimeMillis()-time >= 100);
future.get(); // wait until close finished
scheduler.shutdown();

// with timeout
usb.open(EnumSet.of(UsbWrapper.OpenCloseFlags.NO_IOMANAGER_THREAD));
Expand Down Expand Up @@ -2083,8 +2076,8 @@ public void ftdiMethods() throws Exception {
b = usb.read(1);
long t3 = System.currentTimeMillis();
ftdiSerialPort.setLatencyTimer(lt);
assertEquals("latency 1", 99, Math.max(t2-t1, 99)); // looks strange, but shows actual value
assertEquals("latency 100", 99, Math.min(t3-t2, 99));
assertTrue("latency 1: expected < 100, got "+ (t2-t1), (t2-t1) < 100);
assertTrue("latency 100: expected > 100, got " + (t3-t2), (t3-t2) > 100);

usb.deviceConnection.close();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void open(EnumSet<OpenCloseFlags> flags) throws Exception {
if(!flags.contains(OpenCloseFlags.NO_IOMANAGER_THREAD)) {
ioManager = new SerialInputOutputManager(serialPort, this);
if(!flags.contains(OpenCloseFlags.NO_IOMANAGER_START))
Executors.newSingleThreadExecutor().submit(ioManager);
ioManager.start();
}
synchronized (readBuffer) {
readBuffer.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ protected void testConnection() throws IOException {
byte[] buf = new byte[2];
int len = mConnection.controlTransfer(0x80 /*DEVICE*/, 0 /*GET_STATUS*/, 0, 0, buf, buf.length, 200);
if(len < 0)
throw new IOException("Connection lost, USB get_status request failed");
throw new IOException("USB get_status request failed");
}

@Override
Expand Down Expand Up @@ -177,7 +177,8 @@ protected int read(final byte[] dest, final int timeout, boolean testConnection)
long endTime = testConnection ? MonotonicClock.millis() + timeout : 0;
int readMax = Math.min(dest.length, MAX_READ_SIZE);
nread = mConnection.bulkTransfer(mReadEndpoint, dest, readMax, timeout);
// Android error propagation is improvable, nread == -1 can be: timeout, connection lost, buffer to small
// Android error propagation is improvable:
// nread == -1 can be: timeout, connection lost, buffer to small, ???
if(nread == -1 && testConnection && MonotonicClock.millis() < endTime)
testConnection();

Expand All @@ -191,12 +192,10 @@ protected int read(final byte[] dest, final int timeout, boolean testConnection)
throw new IOException("Waiting for USB request failed");
}
nread = buf.position();
// Android error propagation is improvable:
// response != null & nread == 0 can be: connection lost, buffer to small, ???
if(nread == 0) {
if(dest.length % mReadEndpoint.getMaxPacketSize() != 0) {
throw new IOException("Connection lost or buffer to small");
} else {
throw new IOException("Connection lost");
}
testConnection();
}
}
return Math.max(nread, 0);
Expand Down

0 comments on commit 5f94a47

Please sign in to comment.