From f60414f8ec7aa65fc8295e6c9eb2adce6b5a607e Mon Sep 17 00:00:00 2001 From: kai-morich Date: Sun, 31 Jan 2021 20:01:12 +0100 Subject: [PATCH] improve write timeout handling Return type of write() method changed to void. The return value was redundant before, as it always was the request length or an exception was thrown. If timeout is reached, write() now throws a SerialTimeoutException with ex.bytesTransferred filled with known transferred bytes. Added CommonUsbSerialPort.getReadEndpoint() and .getWriteEndpoint() to assist in setting the optimal write buffer size with port.setWriteBufferSize(port.getWriteEndpoint().getMaxPacketSize()). By default the write buffer size is > MaxPacketSize and the Linux kernel splits writes in chunks. When the timeout occurs, it's unknown how many chunks have already been transferred and the exception typically stores 0. With optimal write buffer size, this value is known and stored in SerialTimeoutException, but due to more kernel round trips write() might take slightly longer(). --- build.gradle | 2 +- .../hoho/android/usbserial/DeviceTest.java | 199 ++++++++++++------ .../usbserial/driver/CommonUsbSerialPort.java | 58 +++-- .../driver/SerialTimeoutException.java | 15 ++ .../usbserial/driver/UsbSerialPort.java | 5 +- 5 files changed, 202 insertions(+), 77 deletions(-) create mode 100644 usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/SerialTimeoutException.java diff --git a/build.gradle b/build.gradle index e1753de..a5a05dc 100644 --- a/build.gradle +++ b/build.gradle @@ -6,7 +6,7 @@ buildscript { google() } dependencies { - classpath 'com.android.tools.build:gradle:4.1.1' + classpath 'com.android.tools.build:gradle:4.1.2' } } diff --git a/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java b/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java index 70403e6..8253500 100644 --- a/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java +++ b/usbSerialForAndroid/src/androidTest/java/com/hoho/android/usbserial/DeviceTest.java @@ -26,6 +26,7 @@ import com.hoho.android.usbserial.driver.Cp21xxSerialDriver; import com.hoho.android.usbserial.driver.FtdiSerialDriver; import com.hoho.android.usbserial.driver.ProbeTable; import com.hoho.android.usbserial.driver.ProlificSerialDriver; +import com.hoho.android.usbserial.driver.SerialTimeoutException; import com.hoho.android.usbserial.driver.UsbId; import com.hoho.android.usbserial.driver.UsbSerialDriver; import com.hoho.android.usbserial.driver.UsbSerialPort; @@ -37,6 +38,7 @@ import com.hoho.android.usbserial.util.UsbWrapper; import org.junit.After; import org.junit.AfterClass; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Rule; @@ -236,6 +238,21 @@ public class DeviceTest { assertThat(reason, data, equalTo(buf2)); } + private void purgeWriteBuffer(int timeout) throws Exception { + try { + Log.d(TAG, " purge begin"); + usb.serialPort.purgeHwBuffers(true, false); + } catch(UnsupportedOperationException ignored) {} + byte[] data = telnet.read(-1, timeout); + int len = 0; + while(data.length != 0) { + len += data.length; + Log.d(TAG, " purge read " + data.length); + data = telnet.read(-1, timeout); + } + Log.d(TAG, " purge end " + len); + } + @Test public void openClose() throws Exception { usb.open(); @@ -294,8 +311,7 @@ public class DeviceTest { @Test public void prolificBaudRate() throws Exception { - if(!(usb.serialDriver instanceof ProlificSerialDriver)) - return; + Assume.assumeTrue("only for Prolific", usb.serialDriver instanceof ProlificSerialDriver); int[] baudRates = { 75, 150, 300, 600, 1200, 1800, 2400, 3600, 4800, 7200, 9600, 14400, 19200, @@ -357,8 +373,7 @@ public class DeviceTest { @Test public void ftdiBaudRate() throws Exception { - if (!(usb.serialDriver instanceof FtdiSerialDriver)) - return; + Assume.assumeTrue("only for FTDI", usb.serialDriver instanceof FtdiSerialDriver); usb.open(); try { @@ -768,65 +783,129 @@ public class DeviceTest { @Test public void writeTimeout() throws Exception { usb.open(); - usb.setParameters(115200, 8, 1, UsbSerialPort.PARITY_NONE); - telnet.setParameters(115200, 8, 1, UsbSerialPort.PARITY_NONE); - - // Basically all devices have a UsbEndpoint.getMaxPacketSize() 64. When the timeout - // in usb.serialPort.write() is reached, some packets have been written and the rest - // is discarded. bulkTransfer() does not return the number written so far, but -1. - // With 115200 baud and 1/2 second timeout, typical values are: - // ch340 6080 of 6144 - // pl2302 5952 of 6144 - // cp2102 6400 of 7168 - // cp2105 6272 of 7168 - // ft232 5952 of 6144 - // ft2232 9728 of 10240 - // arduino 128 of 144 - int timeout = 500; - int len = 0; - int startLen = 1024; - int step = 1024; - int minLen = 4069; - int maxLen = 12288; - int bufferSize = 511; - TestBuffer buf = new TestBuffer(len); - if(usb.serialDriver instanceof CdcAcmSerialDriver) { - startLen = 16; - step = 16; - minLen = 128; - maxLen = 256; - bufferSize = 31; - } + int baudRate = 300; + if(usb.serialDriver instanceof Cp21xxSerialDriver && usb.serialDriver.getPorts().size() > 1) + baudRate = 2400; + usb.setParameters(baudRate, 8, 1, UsbSerialPort.PARITY_NONE); + telnet.setParameters(baudRate, 8, 1, UsbSerialPort.PARITY_NONE); + int purgeTimeout = 250; + if(usb.serialDriver instanceof CdcAcmSerialDriver) + purgeTimeout = 500; + purgeWriteBuffer(purgeTimeout); + // determine write buffer size + int writePacketSize = ((CommonUsbSerialPort)usb.serialPort).getWriteEndpoint().getMaxPacketSize(); + byte[] pbuf = new byte[writePacketSize]; + int writePackets = 0; try { - for (len = startLen; len < maxLen; len += step) { - buf = new TestBuffer(len); - Log.d(TAG, "write buffer size " + len); - usb.serialPort.write(buf.buf, timeout); - while (!buf.testRead(telnet.read(-1))) - ; + for (writePackets = 0; writePackets < 64; writePackets++) + usb.serialPort.write(pbuf, 1); + fail("write error expected"); + } catch(IOException ignored) {} + purgeWriteBuffer(purgeTimeout); + + int writeBufferSize = writePacketSize * writePackets; + Log.d(TAG, "write packet size = " + writePacketSize + ", write buffer size = " + writeBufferSize); + if(usb.serialDriver instanceof Cp21xxSerialDriver) { + switch(usb.serialDriver.getPorts().size()*0x10 + usb.serialPort.getPortNumber()) { + case 0x10: assertEquals("write packet size", 64, writePacketSize); assertEquals("write buffer size", 576, writeBufferSize); break; + case 0x20: assertEquals("write packet size", 64, writePacketSize); assertEquals("write buffer size", 320, writeBufferSize); break; + case 0x21: assertEquals("write packet size", 32, writePacketSize); assertTrue("write buffer size", writeBufferSize == 128 || writeBufferSize == 160); break; + default: fail("unknown port number"); } - fail("write timeout expected between " + minLen + " and " + maxLen + ", is " + len); - } catch (IOException e) { - Log.d(TAG, "usbWrite failed", e); - while (true) { - byte[] data = telnet.read(-1); - if (data.length == 0) break; - if (buf.testRead(data)) break; + } else if(usb.serialDriver instanceof Ch34xSerialDriver) { + assertEquals("write packet size", 32, writePacketSize); assertEquals("write buffer size", 64, writeBufferSize); + } else if(usb.serialDriver instanceof ProlificSerialDriver) { + assertEquals("write packet size", 64, writePacketSize); assertEquals("write buffer size", 256, writeBufferSize); + } else if(usb.serialDriver instanceof FtdiSerialDriver) { + switch(usb.serialDriver.getPorts().size()) { + case 1: assertEquals("write packet size", 64, writePacketSize); assertEquals("write buffer size", 128, writeBufferSize); break; + case 2: assertEquals("write packet size", 512, writePacketSize); assertEquals("write buffer size", 4096, writeBufferSize); break; + case 4: assertEquals("write packet size", 512, writePacketSize); assertEquals( "write buffer size", 2048, writeBufferSize); break; + default: fail("unknown port number"); } - Log.d(TAG, "received " + buf.len + " of " + len + " bytes of failing usbWrite"); - assertTrue("write timeout expected between " + minLen + " and " + maxLen + ", is " + len, len > minLen); + } else if(usb.serialDriver instanceof CdcAcmSerialDriver) { + assertEquals(64, writePacketSize); assertEquals(128, writeBufferSize); // values for ATMega32U4 + } else { + fail("unknown driver " + usb.serialDriver.getClass().getSimpleName()); + } + purgeWriteBuffer(purgeTimeout); + if(usb.serialDriver instanceof CdcAcmSerialDriver) + return; // serial processing to slow for tests below, but they anyway only check shared code in CommonUsbSerialPort + if(usb.serialDriver instanceof Cp21xxSerialDriver && usb.serialDriver.getPorts().size() > 1) + return; // write buffer size detection unreliable as baud rate to high + + usb.setParameters(9600, 8, 1, UsbSerialPort.PARITY_NONE); + telnet.setParameters(9600, 8, 1, UsbSerialPort.PARITY_NONE); + TestBuffer tbuf; + + // compare write time + // full write @ 1-2 msec + // packet write @ 5-10 msec + if(false) { + tbuf = new TestBuffer(writeBufferSize); + ((CommonUsbSerialPort) usb.serialPort).setWriteBufferSize(tbuf.buf.length); + Log.d(TAG, "full write begin"); + long begin = System.currentTimeMillis(); + usb.serialPort.write(tbuf.buf, 0); + Log.d(TAG, "full write end, duration=" + (System.currentTimeMillis() - begin)); + purgeWriteBuffer(purgeTimeout); + ((CommonUsbSerialPort) usb.serialPort).setWriteBufferSize(writePacketSize); + Log.d(TAG, "packet write begin"); + begin = System.currentTimeMillis(); + usb.serialPort.write(tbuf.buf, 0); + Log.d(TAG, "packet write end, duration=" + (System.currentTimeMillis() - begin)); + purgeWriteBuffer(purgeTimeout); + Assume.assumeTrue(false); } - // With smaller writebuffer, the timeout is used per bulkTransfer and each call 'fits' - // into this timout, but shouldn't further calls only use the remaining timeout? - ((CommonUsbSerialPort) usb.serialPort).setWriteBufferSize(bufferSize); - len = maxLen; - buf = new TestBuffer(len); - Log.d(TAG, "write buffer size " + len); - usb.serialPort.write(buf.buf, timeout); - while (!buf.testRead(telnet.read(-1))) - ; + // total write timeout + tbuf = new TestBuffer(writeBufferSize + writePacketSize); + int timeout = writePacketSize / 32 * 50; // time for 1.5 packets. write 48 byte in 50 msec at 9600 baud + ((CommonUsbSerialPort)usb.serialPort).setWriteBufferSize(writePacketSize); + usb.serialPort.write(tbuf.buf, timeout); + purgeWriteBuffer(purgeTimeout); + tbuf = new TestBuffer(writeBufferSize + 2*writePacketSize); + try { + usb.serialPort.write(tbuf.buf, timeout); // would not fail if each block has own timeout + fail("write error expected"); + } catch(SerialTimeoutException ignored) {} + purgeWriteBuffer(purgeTimeout); + + // infinite wait + //((CommonUsbSerialPort)usb.serialPort).setWriteBufferSize(writePacketSize); + usb.serialPort.write(tbuf.buf, 0); + purgeWriteBuffer(purgeTimeout); + + // SerialTimeoutException.bytesTransferred + int readWait = writePacketSize > 64 ? 250 : 50; + ((CommonUsbSerialPort)usb.serialPort).setWriteBufferSize(tbuf.buf.length); + try { + usb.serialPort.write(tbuf.buf, timeout); + fail("write error expected"); + } catch(SerialTimeoutException ex) { + for(byte[] data = telnet.read(-1, readWait); data.length != 0; + data = telnet.read(-1, readWait)) { + tbuf.testRead(data); + } + assertEquals(0, ex.bytesTransferred); + assertEquals(writeBufferSize + writePacketSize, tbuf.len); + } + purgeWriteBuffer(purgeTimeout); + ((CommonUsbSerialPort)usb.serialPort).setWriteBufferSize(writePacketSize); + tbuf.len = 0; + try { + usb.serialPort.write(tbuf.buf, timeout); + fail("write error expected"); + } catch(SerialTimeoutException ex) { + for(byte[] data = telnet.read(-1, readWait); data.length != 0; + data = telnet.read(-1, readWait)) { + tbuf.testRead(data); + } + assertEquals(writeBufferSize + writePacketSize, ex.bytesTransferred); + assertEquals(writeBufferSize + writePacketSize, tbuf.len); + } + purgeWriteBuffer(purgeTimeout); } @Test @@ -1872,6 +1951,8 @@ public class DeviceTest { usb.open(); assertTrue(usb.serialPort.isOpen()); + assertEquals(((CommonUsbSerialPort)usb.serialPort).getWriteEndpoint().getMaxPacketSize(), + ((CommonUsbSerialPort)usb.serialPort).getReadEndpoint().getMaxPacketSize()); s = usb.serialPort.getSerial(); // with target sdk 29 can throw SecurityException before USB permission dialog is confirmed // not all devices implement serial numbers. some observed values are: @@ -1898,8 +1979,8 @@ public class DeviceTest { @Test public void ftdiMethods() throws Exception { - if(!(usb.serialDriver instanceof FtdiSerialDriver)) - return; + Assume.assumeTrue("only for FTDI", usb.serialDriver instanceof FtdiSerialDriver); + byte[] b; usb.open(); usb.setParameters(115200, 8, 1, UsbSerialPort.PARITY_NONE); diff --git a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java index 0ff1623..95ca35f 100644 --- a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java +++ b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/CommonUsbSerialPort.java @@ -63,7 +63,19 @@ public abstract class CommonUsbSerialPort implements UsbSerialPort { public int getPortNumber() { return mPortNumber; } - + + /** + * Returns the write endpoint. + * @return write endpoint + */ + public UsbEndpoint getWriteEndpoint() { return mWriteEndpoint; } + + /** + * Returns the read endpoint. + * @return read endpoint + */ + public UsbEndpoint getReadEndpoint() { return mReadEndpoint; } + /** * Returns the device serial number * @return serial number @@ -191,39 +203,55 @@ public abstract class CommonUsbSerialPort implements UsbSerialPort { } @Override - public int write(final byte[] src, final int timeout) throws IOException { + public void write(final byte[] src, final int timeout) throws IOException { int offset = 0; + int requestTimeout = timeout; if(mConnection == null) { throw new IOException("Connection closed"); } while (offset < src.length) { - final int writeLength; - final int amtWritten; + final int requestLength; + final int actualLength; + final int requestDuration; synchronized (mWriteBufferLock) { final byte[] writeBuffer; - writeLength = Math.min(src.length - offset, mWriteBuffer.length); + requestLength = Math.min(src.length - offset, mWriteBuffer.length); if (offset == 0) { writeBuffer = src; } else { // bulkTransfer does not support offsets, make a copy. - System.arraycopy(src, offset, mWriteBuffer, 0, writeLength); + System.arraycopy(src, offset, mWriteBuffer, 0, requestLength); writeBuffer = mWriteBuffer; } - - amtWritten = mConnection.bulkTransfer(mWriteEndpoint, writeBuffer, writeLength, timeout); + if (requestTimeout < 0) { + actualLength = -2; + requestDuration = 0; + } else { + final long startTime = System.currentTimeMillis(); + actualLength = mConnection.bulkTransfer(mWriteEndpoint, writeBuffer, requestLength, requestTimeout); + requestDuration = (int) (System.currentTimeMillis() - startTime); + } } - if (amtWritten <= 0) { - throw new IOException("Error writing " + writeLength - + " bytes at offset " + offset + " length=" + src.length); + Log.d(TAG, "Wrote " + actualLength + "/" + requestLength + " offset " + offset + "/" + src.length + " timeout " + requestTimeout); + if (requestTimeout > 0) { + requestTimeout -= requestDuration; + if (requestTimeout == 0) + requestTimeout = -1; } - - Log.d(TAG, "Wrote amt=" + amtWritten + " attempted=" + writeLength); - offset += amtWritten; + if (actualLength <= 0) { + if(requestTimeout < 0) { + SerialTimeoutException ex = new SerialTimeoutException("Error writing " + requestLength + " bytes at offset " + offset + " of total " + src.length); + ex.bytesTransferred = offset; + throw ex; + } else { + throw new IOException("Error writing " + requestLength + " bytes at offset " + offset + " of total " + src.length); + } + } + offset += actualLength; } - return offset; } @Override diff --git a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/SerialTimeoutException.java b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/SerialTimeoutException.java new file mode 100644 index 0000000..12fc0ec --- /dev/null +++ b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/SerialTimeoutException.java @@ -0,0 +1,15 @@ +package com.hoho.android.usbserial.driver; + +import java.io.InterruptedIOException; + +/** + * Signals that a timeout has occurred on serial write. + * Similar to SocketTimeoutException. + * + * {@see InterruptedIOException#bytesTransferred} may contain bytes transferred + */ +public class SerialTimeoutException extends InterruptedIOException { + public SerialTimeoutException(String s) { + super(s); + } +} diff --git a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/UsbSerialPort.java b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/UsbSerialPort.java index fa520d4..e18a769 100644 --- a/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/UsbSerialPort.java +++ b/usbSerialForAndroid/src/main/java/com/hoho/android/usbserial/driver/UsbSerialPort.java @@ -114,10 +114,11 @@ public interface UsbSerialPort extends Closeable { * * @param src the source byte buffer * @param timeout the timeout for writing in milliseconds, 0 is infinite - * @return the actual number of bytes written + * @throws SerialTimeoutException if timeout reached before sending all data. + * ex.bytesTransferred may contain bytes transferred * @throws IOException if an error occurred during writing */ - int write(final byte[] src, final int timeout) throws IOException; + void write(final byte[] src, final int timeout) throws IOException; /** * Sets various serial port parameters.