Skip to content

Fixing endless loop (100% cpu), adding IO exceptions when ports closes (unplug USB serial ports), refactored blocking read/write support with interrupt() capabilities, added Input/Output stream support #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 83 additions & 24 deletions src/main/cpp/_nix_based/jssc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -529,14 +529,57 @@ JNIEXPORT jboolean JNICALL Java_jssc_SerialNativeInterface_writeBytes
(JNIEnv *env, jobject, jlong portHandle, jbyteArray buffer){
jbyte* jBuffer = env->GetByteArrayElements(buffer, JNI_FALSE);
jint bufferSize = env->GetArrayLength(buffer);
jint result = write(portHandle, jBuffer, (size_t)bufferSize);
fd_set write_fd_set;
int byteRemains = bufferSize;
struct timeval timeout;
int result;
jclass threadClass = env->FindClass("java/lang/Thread");
jmethodID areWeInterruptedMethod = env->GetStaticMethodID(threadClass, "interrupted", "()Z");

while(byteRemains > 0) {

// Check if the java thread has been interrupted, and if so, throw the exception
if (env->CallStaticBooleanMethod(threadClass, areWeInterruptedMethod)) {
jclass excClass = env->FindClass("java/lang/InterruptedException");
env->ThrowNew(excClass, "Interrupted while writing to serial port");
// It shouldn't matter what we return, the exception will be thrown right away
break;
}

timeout.tv_sec = 0;
timeout.tv_usec = 100000; // 100 ms
FD_ZERO(&write_fd_set);
FD_SET(portHandle, &write_fd_set);

result = select(portHandle + 1, NULL, &write_fd_set, NULL, &timeout);
if (result < 0) {
env->ThrowNew(env->FindClass("java/io/IOException"), "Waiting for serial port to become writable failed");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) Only a suggestion. Just ignore that comment if it isn't relevant enough :)

env->ThrowNew(env->FindClass("java/io/IOException"), "Waiting for serial port to become writable failed");

I guess in this case here, we could use strerror(errno) to get a more concise error message provided by select(). Eg:

#include <errno.h>
#include <string.h>

env->ThrowNew(env->FindClass("java/io/IOException"), strerror(errno));

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome suggestion, makes it more readable

// Return value is ignored anyway, exception is handled immidiatly
break;
} else if (result == 0) {
// timeout
continue;
}

result = write(portHandle, jBuffer + (bufferSize - byteRemains), byteRemains);
if(result < 0){
env->ThrowNew(env->FindClass("java/io/IOException"), "Error writing to serial port");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) Another place where we potentially could use strerror(errno)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same awesome suggestion as before ;)

break;
} else if (result == 0) {
env->ThrowNew(env->FindClass("java/io/IOException"), "Serial port was closed unexpectedly");
break;
} else { // result > 0
byteRemains -= result;
}
}
env->ReleaseByteArrayElements(buffer, jBuffer, 0);
return result == bufferSize ? JNI_TRUE : JNI_FALSE;
return JNI_TRUE; //result == bufferSize ? JNI_TRUE : JNI_FALSE;
}

/**
* Waits until 'read()' has something to tell for the specified filedescriptor.
*/
/*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    // Default impl using 'poll'. This is more robust against fd>=1024 (eg
    // SEGFAULT problems).
    ....
        int result = poll(fds, 1, -1);

Ough :( too bad we drop poll again. So our project won't be able to upgrade to any newer jssc versions.

I let the decision up to other maintainers if we want to go this route or not 👍

Just for reference:

  • poll got introduced in Use poll in readBytes to fix segfaults with high fd numbers #90
  • We did this because we saw SEGFAULTs irregularly. It took us a long time to find that those FDs were the cause as the issue was hard to reproduce.
  • About why we have large FDs, I only can speculate. One possibility is that there is a lot going on in this process as it is a jetty, hosting half a dozen apps, all of them doing lots of network and device IO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right, to be honest, we have already commented to the authors of issue #90 that it would be awesome if the poll() technique could be used for this proposal.

We never had any problems with select() because of to many (open?) file-descriptors. So we never really dived into this problem/solution. But we encourage you to adjust/refactor our code and migrate to using the poll() technique.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an idea how we could reduce the impact without the need for "poll". I did not find a good way to show that patch here. So I opened PR_128 that we can "see" the idea. Basically it just replaces the SEGFAULT by a java exception and so at least we get a proper error message. Additionally I removed the now unused stuff around "poll".

static void awaitReadReady(JNIEnv*, jlong fd){
#if HAVE_POLL == 0
// Alternative impl using 'select' as 'poll' isn't available (or broken).
Expand Down Expand Up @@ -579,7 +622,8 @@ static void awaitReadReady(JNIEnv*, jlong fd){

#endif
}

*/

/* OK */
/*
* Reading data from the port
Expand All @@ -588,36 +632,51 @@ static void awaitReadReady(JNIEnv*, jlong fd){
*/
JNIEXPORT jbyteArray JNICALL Java_jssc_SerialNativeInterface_readBytes
(JNIEnv *env, jobject, jlong portHandle, jint byteCount){

// TODO: Errors should be communicated by raising java exceptions; Will break
// backwards compatibility.

fd_set read_fd_set;
jbyte *lpBuffer = new jbyte[byteCount];
jbyteArray returnArray = NULL;
int byteRemains = byteCount;

struct timeval timeout;
int result;
jclass threadClass = env->FindClass("java/lang/Thread");
jmethodID areWeInterruptedMethod = env->GetStaticMethodID(threadClass, "interrupted", "()Z");

while(byteRemains > 0) {
int result = 0;

awaitReadReady(env, portHandle);

errno = 0;
result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
if (result < 0) {
// man read: On error, -1 is returned, and errno is set to indicate the error.
// TODO: May candidate for raising a java exception. See comment at begin of function.
// Check if the java thread has been interrupted, and if so, throw the exception
if (env->CallStaticBooleanMethod(threadClass, areWeInterruptedMethod)) {
jclass excClass = env->FindClass("java/lang/InterruptedException");
env->ThrowNew(excClass, "Interrupted while reading from serial port");
// It shouldn't matter what we return, the exception will be thrown right away
break;
}
else if (result == 0) {
// AFAIK this happens either on EOF or on EWOULDBLOCK (see 'man read').
// TODO: Is "just continue" really the right thing to do? I will keep it that
// way because the old code did so and I don't know better.

timeout.tv_sec = 0;
timeout.tv_usec = 100000; // 100 ms
FD_ZERO(&read_fd_set);
FD_SET(portHandle, &read_fd_set);

result = select(portHandle + 1, &read_fd_set, NULL, NULL, &timeout);
if (result < 0) {
env->ThrowNew(env->FindClass("java/io/IOException"), "Error while waiting for input from serial port");
// Return value is ignored anyway, exception is handled immidiatly
break;
} else if (result == 0) {
// timeout
continue;
}
else {

result = read(portHandle, lpBuffer + (byteCount - byteRemains), byteRemains);
if(result < 0){
env->ThrowNew(env->FindClass("java/io/IOException"), "Error reading from serial port");
break;
} else if (result == 0) {
env->ThrowNew(env->FindClass("java/io/IOException"), "Serial port was closed unexpectedly");
break;
} else { // result > 0
byteRemains -= result;
}
}

returnArray = env->NewByteArray(byteCount);
jbyteArray returnArray = env->NewByteArray(byteCount);
env->SetByteArrayRegion(returnArray, 0, byteCount, lpBuffer);
delete[] lpBuffer;
return returnArray;
Expand Down
3 changes: 2 additions & 1 deletion src/main/cpp/jssc_SerialNativeInterface.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

221 changes: 221 additions & 0 deletions src/main/java/jssc/SerialPortStream.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
/* jSSC (Java Simple Serial Connector) - serial port communication library.
* © Alexey Sokolov (scream3r), 2010-2014.
*
* This file is part of jSSC.
*
* jSSC is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* jSSC 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 Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with jSSC. If not, see <http://www.gnu.org/licenses/>.
*
* If you use jSSC in public project you can inform me about this by e-mail,
* of course if you want it.
*
* e-mail: [email protected]
* web-site: http://scream3r.org | http://code.google.com/p/java-simple-serial-connector/
*
* SerialPortStream.java is written and contributed under the same license
* to the jSSC project by FactorIT B.V. in 2022 (factoritbv on github)
*
*/
package jssc;

import java.net.*;
import java.util.*;
import java.util.concurrent.*;
import java.io.*;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) Looks as those imports are unused. Should we remove them?

import java.net.*;
import java.util.concurrent.*;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is better to remove them


public class SerialPortStream {

private SerialPort m_serial_port;
private SerialPortInputStream m_serial_port_input_stream;
private SerialPortOutputStream m_serial_port_output_stream;
private volatile boolean m_closed;

private class SerialPortInputStream extends InputStream {
private byte[] m_buffer;
private int m_buffer_pos = 0;
private int m_buffer_len = 0;

public SerialPortInputStream() {
m_buffer_len = 0;
m_buffer_pos = 0;
}

public void close() throws IOException {
SerialPortInputStream.this.close();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private class SerialPortInputStream extends InputStream {
    ...
    public void close() throws IOException {
        SerialPortInputStream.this.close();
    }

I suspect this to end up in an infinite recursion. Isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, you are perfectly right, this should be SerialPortStream.this.close() of course.

I'm afraid this was caused by our turbo refactoring as well ;)

}

public int read() throws IOException {
try {
// See if we run out of available bytes, and try to re-fill the buffer
if (m_buffer_pos >= m_buffer_len) {
m_buffer_len = m_serial_port.getInputBufferBytesCount();
if (m_buffer_len == 0) {
// Nothing available, just block until the first byte comes available and return directly
return (int)m_serial_port.readBytes(1)[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(int)m_serial_port.readBytes(1)[0]

(WARN) I suspect this to return bad values if the byte value is 0x80 or larger.

Legend for the table below:

byte: The byte value returned by readBytes(1)[0]
expect: The value I would expect it to return
actual: The value I suspect it to return (ToBeVerified)

byte expect actual comment
42 42 42 OK, because below 0x80
200 200 -56 Not the value I would expect (doc says the range is 0-255)
255 255 -1 Not so fun as doc says that -1 indicates EOF

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could be right, however, we never run into problem with this code. But maybe we only focussed on ASCII tranmissions. Feel free to adjust and cast/handle the in the right type.

}
// Fetch the available bytes at once
m_buffer = m_serial_port.readBytes(m_buffer_len);
// Reset the position in the buffer
m_buffer_pos = 0;
}
return m_buffer[m_buffer_pos++];
} catch (SerialPortException ex) {
throw new IOException(ex);
}
}

public int available() throws IOException {
try {
return m_serial_port.getInputBufferBytesCount();
} catch (SerialPortException ex) {
throw new IOException(ex);
}
}
}

private class SerialPortOutputStream extends OutputStream {

public void close() throws IOException {
SerialPortStream.this.close();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) I mention this as it looks like an oversight. If it is by intent then feel free to ignore this comment :)

It seems as the only thing SerialPortOutputStream.close() does, is to call its caller (which doesn't make sense to me).

class SerialPortStream {
    void close() {
        ...
        try {
            m_serial_port_output_stream.close();
        }
        ...
    }
    ...

    // Seems as it only calls the method which did call us.
    class SerialPortOutputStream {
        void close() {
            SerialPortStream.this.close();
        }
    }
    ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually makes sense, because as with the Socket class you will perform call .getInputStream() somewhere in the application code and feed that to a another Stream or Reader. When that is closed, the InputStream will be closed, but that should trigger a complete close (also the OutputStream and the SerialPort) self.

A sanity check in the main close() function can be added to check if the Input/Output stream is closed already, but actually the Java docs specify it does not harm to close an Input/Output stream more than once (it is and stays closed).


public void write(byte[] b) throws IOException {
try {
m_serial_port.writeBytes(b);
} catch (Exception ex) {
throw new IOException(ex);
}
}

public void write(byte[] b, int off, int len) throws IOException {
byte[] buffer = Arrays.copyOfRange(b,off,off+len);
try {
m_serial_port.writeBytes(buffer);
} catch (Exception ex) {
throw new IOException(ex);
}
}

public void write(int b) throws IOException {
try {
m_serial_port.writeBytes(new byte[]{(byte)b});
} catch (Exception ex) {
throw new IOException(ex);
}
}
}

public SerialPortStream(SerialPort serial_port, int baudrate) throws IOException {
super();
m_serial_port = serial_port;
try {
// Open the serial port if it is still closed
if (!m_serial_port.isOpened()) {
if (!m_serial_port.openPort()) {
throw new IOException("Could not open serial port [" + m_serial_port.getPortName() + "]");
}
// For the USB CDC class seems to be mandatory to set the line coding.
// So we use the most common values (9600 baud, 8 bits chars, 1 close bit, no parity)
if (!m_serial_port.setParams(baudrate,8,1,0)) {
throw new IOException("Could not set params for serial port [" + m_serial_port.getPortName() + "]");
}
}
} catch (SerialPortException ex) {
// Redirect exeption to an IO exception
throw new IOException(ex);
}
m_serial_port_input_stream = new SerialPortInputStream();
m_serial_port_output_stream = new SerialPortOutputStream();
}

public SerialPortStream(SerialPort serial_port) throws IOException {
this(serial_port,9600);
}

public SerialPortStream(String serial_port_name, int baudrate) throws IOException {
this(new SerialPort(serial_port_name),baudrate);
}

public SerialPortStream(String serial_port_name) throws IOException {
this(serial_port_name,9600);
}

public SerialPort getSerialPort() {
return m_serial_port;
}

public String getName() {
return m_serial_port.getPortName();
}

public InputStream getInputStream() {
return m_serial_port_input_stream;
}

public OutputStream getOutputStream() {
return m_serial_port_output_stream;
}

public synchronized void close() throws IOException {
// Immidiatly return when connection was already lost and cleaned up in the past
if (m_closed) return;

// Update the connection status
m_closed = true;

boolean failure = false;

// Try to close and clean up the input stream
try {
m_serial_port_input_stream.close();
} catch (IOException ex) {
failure = true;
}

// Try to close and clean up the output stream
try {
m_serial_port_output_stream.close();
} catch (IOException ex) {
failure = true;
}

if (failure) {
throw new IOException("Error occured while closing and cleaning up the in/output streams");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) Typo? occured -> occurred

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good fix

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new IOException("Error occured while closing and cleaning up the in/output streams");

In case we throw here, we won't call m_serial_port.closePort(). Is this intended?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no this is indeed wrong, I've merged some internal test-classes to get to this result. Better to make sure m_serial_port.closePort() is executed before this io exception is thrown.

}

// Only try to close the serial port if it is still open
if (m_serial_port.isOpened()) {
try {
m_serial_port.closePort();
} catch (SerialPortException ex) {
throw new IOException(ex);
}
}
}

public synchronized boolean isClosed() {
if (!m_closed) {
try {
m_serial_port_input_stream.available();
} catch (IOException ex) {
try {
this.close();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just wondering) To me it sounds strange to call close() in scope of isClosed() which looks like a simple getter.

Don't get me wrong. Maybe it's perfectly fine :) I just mention it as it "feels" strange to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IOException should be handled correctly by the caller function, which included calling closing of the port. However, it does not breaks much here. Since there is boolean double-check anyway that registers if the port was already closed (before) or not.

I think that calling close() is more fail-safe for less experienced users, and does not really adds more overhead, however, it is not a must-have. There I agree on.

} catch (IOException ex2) {
// ignore
}
}
}
return m_closed;
}
}