Fazecast / jSerialComm

Platform-independent serial port access for Java
GNU Lesser General Public License v3.0
1.34k stars 287 forks source link

Port discovery via `SerialPort.getCommPorts()` unreliable for versions > 2.7.0 #403

Closed miho closed 2 years ago

miho commented 2 years ago

Hi,

thanks for creating this library. I used it in several projects and it worked well.

The port discovery is unreliable for versions > 2.7.0. Ports are still listed as available even if devices (e.g. FTDI based) are unplugged. This only seems to happen if ports have been opened prior to unplugging devices. With version 2.7.0 it works as expected and ports disappear even if they haven't properly closed prior to unplugging the corresponding device.

UPDATE maybe the default behavior has just changed. https://github.com/Fazecast/jSerialComm/issues/399 seems to indicate that registering a listener that might perform an auto-close on disconnect is the new way to force the behavior of previous versions.

How to reproduce

Here's a small code snipped that I used to reproduce the behavior.

    public static void main(String[] args) throws InterruptedException, IOException {
        System.setProperty("fazecast.jSerialComm.appid", "CNC_CLIENT");

        var scanner = new Scanner(System.in);
        System.out.print("enter port name to use for testing:");
        String portName = scanner.nextLine().toUpperCase();

        System.out.print("connect the device (e.g. Arduino UNO) and press enter...");
        scanner.nextLine();

        System.out.println("opening port " + portName);
        // open port (device must be connected)
        SerialPort.getCommPort(portName).openPort();

        System.out.println("port '" + portName +"' should vanish from the list of available ports if unplugged.");

        // unplug the device
        System.out.println("press enter if the device is unplugged...");
        scanner.nextLine();

        // list available ports ()
        while(!Thread.currentThread().isInterrupted()) {

            var systemNamesOfPorts =
                Arrays.stream(SerialPort.getCommPorts()).map(p->p.getSystemPortName()).toList();

            System.out.println("connected ports: " + systemNamesOfPorts);

            if(systemNamesOfPorts.contains(portName)) {
                System.out.println("error: port " + portName + " is still available.");
            }

            Thread.sleep(500);
        }
    }
miho commented 2 years ago

Seems to be related to https://github.com/Fazecast/jSerialComm/issues/399. Maybe I should read it first...

hedgecrw commented 2 years ago

You're entirely correct about the functionality. This is one of those cases where solving a number of other problems creates a new problem, but in this case, I think the way it's working now is more objectively "correct" since an open port descriptor does indicate that at the very least, a user's code believes that a given port currently exists (for instance, it may have been manually specified in getCommPort(String portName), or it may be a physical, non-USB-based port which is generally unable to be enumerated without trying to actually open/communicate with the port).

The majority of issues people have had with serial port enumeration over the years stemmed from the fact that the Java-side and native-side of the code knew nothing about each other's port usage, so there was no globally consistent view of what ports existed on the system and/or what ports were currently in-use. The current implementation maintains a single globally-correct list of all serial ports that have been auto-enumerated, manually-specified, and/or are in current use. This helps greatly in ensuring that multiple threads aren't able to accidentally step on one another's toes or access things that they shouldn't be able to; but because of this change, any ports that are still open on the Java-side of the code will appear in the enumeration list.

So yes, your suggestion of implementing a data listener that auto-closes the port on a disconnected event would be a good idea if that is what you'd like your application to do. Some users, however, had problems with the fact that the code used to do that automatically for multiple reasons, including 1) physical serial ports do not always need to be closed/re-opened to continue working, even if the connected device is unplugged and then plugged back in, and some people want their application to auto-continue whatever it was doing without managing a device disconnect, or 2) faulty cables would sometimes cause read/write errors or erroneous control line status changes which would then cause the port to automatically shut down even though it was still present and could continue to be used with no problems if the error or control line status change was simply ignored.

There are definitely ways I could alter the implementation to only return currently-enumerable ports (ignoring manually-specified ports or currently open ports); however, part of me thinks that keeping it as-is may help users more easily identify bugs related to port access in their code (such as holding onto ports that have become disconnected or forgetting to close them, which has caused a fairly large number of non-bug issues to have been filed over the years). Definitely interested in hearing your (and anyone else's) thoughts on the pros/cons of either approach.

miho commented 2 years ago

Thanks for your detailed explanation.

Giving users the option to keep ports open is absolutely fine for me. We just need a reliable way to auto-close a port. We could use the listener approach in the future. But I see a problem with this approach.

Here's why:

The fact that we can only use a single listener per port might be an issue. We could add the auto-close functionality by registering a listener in a custom factory method. Consumers of the port objects might want to listen to data in addition to that without needing to care about the listener that is handling auto-close. And that's going to cause trouble.

hedgecrw commented 2 years ago

@miho, can you clarify for me the way you are using jSerialComm? Specifically, are you creating your own library that uses jSerialComm under the hood, and your library then provides some of the jSerialComm functionality to its own end users? In other words, is your library responsible for enumerating the ports, opening/configuring it and setting up any event listeners, and then your end users consume your wrapped version of the port?

What I'm trying to understand is an example of when a user would have access to a SerialPort from jSerialComm but not know if an event listener had been set up on it. And furthermore, is it not important for your end users to know when a port has become disconnected?

NorbertSandor commented 2 years ago

"when a user would have access to a SerialPort from jSerialComm" - Isn't it a solution if "users" get access to SerialPort only through a custom interface? I use this approach and seems to work well...

hedgecrw commented 2 years ago

@NorbertSandor, I don't understand your statement. What "custom interface" are you using to get a SerialPort object?

The thing I'm trying to figure out is if people are wrapping jSerialComm in their own libraries, and if so, in what way are they doing it, and what items are they pre-configuring and/or hiding from users of their libraries.

NorbertSandor commented 2 years ago

Sorry for the confusion (or maybe I misunderstood something), I reflected to this: "you creating your own library that uses jSerialComm under the hood, and your library then provides some of the jSerialComm functionality to its own end users"

I don't expose the native jSerialComm SerialPort object to users, only a wrapper interface which hides the internals (even that it is using jSerialComm under the hood). This way I can restrict or extend the available features, eg. it is possible to implement multiple event listeners, etc.

hedgecrw commented 2 years ago

Oh okay, understood. So in your case, handling the "port disconnected event" shouldn't be a problem since you're already providing your own interface for users to register multiple event listeners, correct?

miho commented 2 years ago

We use JSerialComm in multiple ways:

But, if there's to much work to do on your side (e.g., firing events via JNI, etc.) then we can certainly provide a single listener implementation that is also an observable, i.e., one could register multiple listeners with this custom SerialPort listener instance.

hedgecrw commented 2 years ago

@miho, understood. Thanks very much for the explanation. I think at this time, if it's possible for you to implement this functionality on your end, that would be preferable. For one, allowing multiple data listeners in the library itself would make it not directly compatible with previous library versions (which I try to avoid), and more importantly, the internal event listening functionality is not as simple as just firing a user's callback when an event occurs, due to the fact that some of the events involve actually reading data, looking for delimiters, internally buffering, etc. So maintaining that functionality while allowing multiple event listeners will require quite a bit of rethinking.

Now that I'm aware that people are wrapping this library and using it in the way you described, I will try to think about this issue and see if I can come up with an elegant way to support multiple data listeners in a future version.

miho commented 2 years ago

@hedgecrw thank you very much for your response. The listener issue is not too important since we now know that the current behavior and listener API are stable and we can rely on them. Providing a custom listener works for us. If you are addressing the listener API in the future let us know. We are happy to try out and discuss prototypes. I will close this issue now. Thanks.