From a1027f8c1e62d0243a5104bd5ef4ea865e21493c Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 24 Mar 2020 12:16:06 +0100 Subject: [PATCH 1/3] Added unit testing for UTF8 decoder in Serial --- app/test/processing/app/SerialTest.java | 58 +++++++++++++++++++++ arduino-core/src/processing/app/Serial.java | 49 ++++++++++------- 2 files changed, 87 insertions(+), 20 deletions(-) create mode 100644 app/test/processing/app/SerialTest.java diff --git a/app/test/processing/app/SerialTest.java b/app/test/processing/app/SerialTest.java new file mode 100644 index 00000000000..63280811e24 --- /dev/null +++ b/app/test/processing/app/SerialTest.java @@ -0,0 +1,58 @@ +/* + * This file is part of Arduino. + * + * Copyright 2020 Arduino LLC (http://www.arduino.cc/) + * + * Arduino is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * As a special exception, you may use this file as part of a free software + * library without restriction. Specifically, if other files instantiate + * templates or use macros or inline functions from this file, or you compile + * this file and link it with other files to produce an executable, this + * file does not by itself cause the resulting executable to be covered by + * the GNU General Public License. This exception does not however + * invalidate any other reasons why the executable file might be covered by + * the GNU General Public License. + */ + +package processing.app; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class SerialTest { + class NullSerial extends Serial { + public NullSerial() throws SerialException { + super("none", 0, 'n', 0, 0, false, false); + } + + @Override + protected void message(char[] chars, int length) { + output += new String(chars, 0, length); + } + + String output = ""; + } + + @Test + public void testSerialUTF8Decoder() throws Exception { + NullSerial s = new NullSerial(); + // https://github.com/arduino/Arduino/issues/9808 + String testdata = "012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789°0123456789"; + s.processSerialEvent(testdata.getBytes()); + assertEquals(s.output, testdata); + } +} diff --git a/arduino-core/src/processing/app/Serial.java b/arduino-core/src/processing/app/Serial.java index 484ac11909b..1ea670950ce 100644 --- a/arduino-core/src/processing/app/Serial.java +++ b/arduino-core/src/processing/app/Serial.java @@ -116,7 +116,7 @@ public static boolean touchForCDCReset(String iname) throws SerialException { } } - private Serial(String iname, int irate, char iparity, int idatabits, float istopbits, boolean setRTS, boolean setDTR) throws SerialException { + protected Serial(String iname, int irate, char iparity, int idatabits, float istopbits, boolean setRTS, boolean setDTR) throws SerialException { //if (port != null) port.close(); //this.parent = parent; //parent.attach(this); @@ -131,6 +131,11 @@ private Serial(String iname, int irate, char iparity, int idatabits, float istop if (istopbits == 1.5f) stopbits = SerialPort.STOPBITS_1_5; if (istopbits == 2) stopbits = SerialPort.STOPBITS_2; + // This is required for unit-testing + if (iname.equals("none")) { + return; + } + try { port = new SerialPort(iname); port.openPort(); @@ -175,31 +180,35 @@ public synchronized void serialEvent(SerialPortEvent serialEvent) { if (serialEvent.isRXCHAR()) { try { byte[] buf = port.readBytes(serialEvent.getEventValue()); - int next = 0; - while(next < buf.length) { - while(next < buf.length && outToMessage.hasRemaining()) { - int spaceInIn = inFromSerial.remaining(); - int copyNow = buf.length - next < spaceInIn ? buf.length - next : spaceInIn; - inFromSerial.put(buf, next, copyNow); - next += copyNow; - inFromSerial.flip(); - bytesToStrings.decode(inFromSerial, outToMessage, false); - inFromSerial.compact(); - } - outToMessage.flip(); - if(outToMessage.hasRemaining()) { - char[] chars = new char[outToMessage.remaining()]; - outToMessage.get(chars); - message(chars, chars.length); - } - outToMessage.clear(); - } + processSerialEvent(buf); } catch (SerialPortException e) { errorMessage("serialEvent", e); } } } + public void processSerialEvent(byte[] buf) { + int next = 0; + while(next < buf.length) { + while(next < buf.length && outToMessage.hasRemaining()) { + int spaceInIn = inFromSerial.remaining(); + int copyNow = buf.length - next < spaceInIn ? buf.length - next : spaceInIn; + inFromSerial.put(buf, next, copyNow); + next += copyNow; + inFromSerial.flip(); + bytesToStrings.decode(inFromSerial, outToMessage, false); + inFromSerial.compact(); + } + outToMessage.flip(); + if(outToMessage.hasRemaining()) { + char[] chars = new char[outToMessage.remaining()]; + outToMessage.get(chars); + message(chars, chars.length); + } + outToMessage.clear(); + } + } + /** * This method is intented to be extended to receive messages * coming from serial port. From 8bf8bdff176520a28513d5c5ae7af2f3e2040435 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 26 Feb 2020 18:38:08 +0100 Subject: [PATCH 2/3] Use Math.min instead of ternary if in Serial data copy This makes the code slightly more compact and easier to read. --- arduino-core/src/processing/app/Serial.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arduino-core/src/processing/app/Serial.java b/arduino-core/src/processing/app/Serial.java index 1ea670950ce..b79af2d2a57 100644 --- a/arduino-core/src/processing/app/Serial.java +++ b/arduino-core/src/processing/app/Serial.java @@ -191,8 +191,7 @@ public void processSerialEvent(byte[] buf) { int next = 0; while(next < buf.length) { while(next < buf.length && outToMessage.hasRemaining()) { - int spaceInIn = inFromSerial.remaining(); - int copyNow = buf.length - next < spaceInIn ? buf.length - next : spaceInIn; + int copyNow = Math.min(buf.length - next, inFromSerial.remaining()); inFromSerial.put(buf, next, copyNow); next += copyNow; inFromSerial.flip(); From f5296a22405b560ca04c570aec7d2f869c18b33a Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 26 Feb 2020 21:57:11 +0100 Subject: [PATCH 3/3] Prevent bytes from lingering in the serial buffer This fixes a problem with the Serial UTF-8 decoder. This decoding moves data from char[] buf, into a ByteBuffer inFromSerial, then decodes them into a CharBuffer outToMessage and converts to a char[] to pass on. When the buf read contained just over a full buffer worth of bytes and contained some multi-byte characters, a situation could arise where two decodes were needed to fill up outToMessage, leaving some data in inFromSerial. If in this case no data would be left in buf, decoding would stop until more data came in from serial. This commit fixes this problem by: - Changing the outer loop to continue running when buf is empty, but inFromSerial is not. - Changing the inner loop to run at least once (so it runs when buf is empty, but inFromSerial is no). - Breaking out of the outer loop when no characters were produced (this handles the case where only an incomplete UTF-8 character remains in inFromSerial, which would otherwise prevent the loop from terminating. - Removes a `if (outToMessage.hasRemaining()` check that is now necessarily true if the break was not done. This fixes #9808. --- arduino-core/src/processing/app/Serial.java | 36 ++++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/arduino-core/src/processing/app/Serial.java b/arduino-core/src/processing/app/Serial.java index b79af2d2a57..edc5e8f0c0f 100644 --- a/arduino-core/src/processing/app/Serial.java +++ b/arduino-core/src/processing/app/Serial.java @@ -189,21 +189,41 @@ public synchronized void serialEvent(SerialPortEvent serialEvent) { public void processSerialEvent(byte[] buf) { int next = 0; - while(next < buf.length) { - while(next < buf.length && outToMessage.hasRemaining()) { + // This uses a CharsetDecoder to convert from bytes to UTF-8 in + // a streaming fashion (i.e. where characters might be split + // over multiple reads). This needs the data to be in a + // ByteBuffer (inFromSerial, which we also use to store leftover + // incomplete characters for the nexst run) and produces a + // CharBuffer (outToMessage), which we then convert to char[] to + // pass onwards. + // Note that these buffers switch from input to output mode + // using flip/compact/clear + while (next < buf.length || inFromSerial.position() > 0) { + do { + // This might be 0 when all data was already read from buf + // (but then there will be data in inFromSerial left to + // decode). int copyNow = Math.min(buf.length - next, inFromSerial.remaining()); inFromSerial.put(buf, next, copyNow); next += copyNow; + inFromSerial.flip(); bytesToStrings.decode(inFromSerial, outToMessage, false); inFromSerial.compact(); - } + + // When there are multi-byte characters, outToMessage might + // still have room, so add more bytes if we have any. + } while (next < buf.length && outToMessage.hasRemaining()); + + // If no output was produced, the input only contained + // incomplete characters, so we're done processing + if (outToMessage.position() == 0) + break; + outToMessage.flip(); - if(outToMessage.hasRemaining()) { - char[] chars = new char[outToMessage.remaining()]; - outToMessage.get(chars); - message(chars, chars.length); - } + char[] chars = new char[outToMessage.remaining()]; + outToMessage.get(chars); + message(chars, chars.length); outToMessage.clear(); } }