nyholku / purejavacomm

Pure Java implementation of JavaComm SerialPort
http://www.sparetimelabs.com/purejavacomm/index.html
BSD 3-Clause "New" or "Revised" License
362 stars 146 forks source link

Non-responsive bluetooth serial port hangs application #106

Closed GlennED closed 6 years ago

GlennED commented 6 years ago

The following is a code snippet from the CommPortIdentifier class method open:

if (info != null) port = info.m_Driver.getCommPort(m_PortName, info.m_PortType); else port = new PureJavaSerialPort(m_PortName, timeout); // FIXME timeout here is not used

The following code snippet from the PureJavaSerialPort class:

    PureJavaSerialPort(String name, int timeout) throws PortInUseException {
    super();

    boolean usepoll = false;
    if (JTermios.canPoll()) {
        String key1 = "purejavacomm.use_poll";
        String key2 = "purejavacomm.usepoll";
        if (System.getProperty(key1) != null) {
            usepoll = Boolean.getBoolean(key1);
            log = log && log(1, "use of '%s' is deprecated, use '%s' instead\n", key1, key2);
        } else if (System.getProperty(key2) != null)
            usepoll = Boolean.getBoolean(key2);
        else
            usepoll = true;
    }
    USE_POLL = usepoll;

    RAW_READ_MODE = Boolean.getBoolean("purejavacomm.rawreadmode");

    this.name = name;

    int tries = (timeout + 5) / 10;
    while ((m_FD = open(name, O_RDWR | O_NOCTTY | O_NONBLOCK)) < 0) {
        int errno = errno();
        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {
            e.printStackTrace();
            Thread.currentThread().interrupt();
        }
        if (tries-- < 0)
            throw new PortInUseException("Unknown Application", errno);
    }

It appears that the "timeout" value is 3000 and this results in the "tries" to be 300. Since a bluetooth port does not respond when the bluetooth device is off it takes an extremely long time to return from the "open" method. The comment "FIXME timeout here is not used" suggests that the "tries" value can be passed as an argument instead of the "timeout" parameter.

I will try this myself but if you know any reason why this is not a good idea please let me know. Thanks.

nyholku commented 6 years ago

Hi,

I think that comment is outdated as clearly 'timeout' is used in the code to calculate 'tries'. I suggest you try timeout 0 this will get you timeout after the first try fails which is probably what you want with bluetooth devices.

johnstosh commented 6 years ago

FYI, the code can be changed to calculate the elapsed time instead of assuming that the open call takes zero time.

I can write an example if that would explain better.

On Mon, Mar 19, 2018, 4:09 PM nyholku notifications@github.com wrote:

Hi,

I think that comment is outdated as clearly 'timeout' is used in the code to calculate 'tries'. I suggest you try timeout 0 this will get you timeout after the first try fails which is probably what you want with bluetooth devices.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nyholku/purejavacomm/issues/106#issuecomment-374354866, or mute the thread https://github.com/notifications/unsubscribe-auth/ABmim483RiLmYrlMYDSpoIa3jiiIwXGpks5tgBCJgaJpZM4Sw0Jq .

nyholku commented 6 years ago

Sure it can but I'm not sure it is worth the risk of changing that code which works for many. IIRC in Windows it sometimes takes about ten calls to open a port when you are re-opening the port. So while it would look perfectly safe to just change the loop logic to calculate the time I'm slightly hesitant to change that. If you make a pull request I will consider it.

GlennED commented 6 years ago

If there was some way in advance to determine if the serial port is "bluetooth" then it would be possible to change the timeout value to 0 for just the "bluetooth" serial ports. I need to support a mix of physical rs-232 serial ports, USB virtual serial ports and bluetooth serial ports.

nyholku commented 6 years ago

That would be great but alas I know of no easy method to do that in Windows let alone all the other platforms....

On 20 Mar 2018, at 13:29, GlennED notifications@github.com wrote:

If there was some way in advance to determine if the serial port is "bluetooth" then it would be possible to change the timeout value to 0 for just the "bluetooth" serial ports. I need to support a mix of physical rs-232 serial ports, USB virtual serial ports and bluetooth serial ports.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

GlennED commented 6 years ago

I am interested in seeing your example code to experiment with this improvement. Thanks.

From: johnstosh [mailto:notifications@github.com] Sent: Monday, March 19, 2018 9:52 PM To: nyholku/purejavacomm purejavacomm@noreply.github.com Cc: Davidson, Glenn gdavidson@seabird.com; Author author@noreply.github.com Subject: Re: [nyholku/purejavacomm] Non-responsive bluetooth serial port hangs application (#106)

FYI, the code can be changed to calculate the elapsed time instead of assuming that the open call takes zero time.

I can write an example if that would explain better.

johnstosh commented 6 years ago

At our office, when waiting on hardware, we will use System.currentTimeMillis() like the following

PureJavaSerialPort(String name, int timeout) throws PortInUseException {
    super();

    boolean usepoll = false;
    if (JTermios.canPoll()) {
        String key1 = "purejavacomm.use_poll";
        String key2 = "purejavacomm.usepoll";
        if (System.getProperty(key1) != null) {
            usepoll = Boolean.getBoolean(key1);
            log = log && log(1, "use of '%s' is deprecated, use '%s'

instead\n", key1, key2); } else if (System.getProperty(key2) != null) { usepoll = Boolean.getBoolean(key2); } else { usepoll = true; } } USE_POLL = usepoll;

    RAW_READ_MODE = Boolean.getBoolean("purejavacomm.rawreadmode");

    this.name = name;

    long before = System.currentTimeMillis();
    int tries = (timeout + 5) / 10;
    while ((m_FD = open(name, O_RDWR | O_NOCTTY | O_NONBLOCK)) < 0) {
        int errno = errno();

        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {
            e.printStackTrace();
            Thread.currentThread().interrupt();
        }

        // old way
        if (tries-- < 0) {
            throw new PortInUseException("Unknown Application", errno);
        }

        // new way
        long after = System.currentTimeMillis();
        if (after - before > timeout) {
            throw new PortInUseException("Unknown Application", errno);
        }
    }

}

-- Johnny Muczynski 734-262-2045

On Tue, Mar 20, 2018 at 7:52 AM GlennED notifications@github.com wrote:

I am interested in seeing your example code to experiment with this improvement. Thanks.

From: johnstosh [mailto:notifications@github.com] Sent: Monday, March 19, 2018 9:52 PM To: nyholku/purejavacomm purejavacomm@noreply.github.com Cc: Davidson, Glenn gdavidson@seabird.com; Author < author@noreply.github.com> Subject: Re: [nyholku/purejavacomm] Non-responsive bluetooth serial port hangs application (#106)

FYI, the code can be changed to calculate the elapsed time instead of assuming that the open call takes zero time.

I can write an example if that would explain better.

On Mon, Mar 19, 2018, 4:09 PM nyholku notifications@github.com wrote:

Hi,

I think that comment is outdated as clearly 'timeout' is used in the code to calculate 'tries'. I suggest you try timeout 0 this will get you timeout after the first try fails which is probably what you want with bluetooth devices.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/nyholku/purejavacomm/issues/106#issuecomment-374354866>,

or mute the thread < https://github.com/notifications/unsubscribe-auth/ABmim483RiLmYrlMYDSpoIa3jiiIwXGpks5tgBCJgaJpZM4Sw0Jq>

.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub< https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nyholku_purejavacomm_issues_106-23issuecomment-2D374435138&d=DwMFaQ&c=9mghv0deYPYDGP-W745IEdQLV1kHpn4XJRvR6xMRXtA&r=io53Eio047z69sc97M5WWQW5UfT0w-PoMFF5wwW40YA&m=wHEyiFcNxwN_CfsAvzl_ZOBuC5O9tjNJBSFRMWKWTRk&s=YfkcZIq9cn_P8mgD9XgY927775TMAMYjPWv7k2K5YDA&e=>, or mute the thread< https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AZV-5F4NURxc3ANdU9VZ9pq2XijzNfdRD3ks5tgFK7gaJpZM4Sw0Jq&d=DwMFaQ&c=9mghv0deYPYDGP-W745IEdQLV1kHpn4XJRvR6xMRXtA&r=io53Eio047z69sc97M5WWQW5UfT0w-PoMFF5wwW40YA&m=wHEyiFcNxwN_CfsAvzl_ZOBuC5O9tjNJBSFRMWKWTRk&s=8Htbk8gClOs_QuFDhIyejB9LxuvfdSYo67uFawxee1o&e=>.

Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nyholku/purejavacomm/issues/106#issuecomment-374570053, or mute the thread https://github.com/notifications/unsubscribe-auth/ABmim-KpDlYlntarffXe4MP1EW3HHH6gks5tgO1ggaJpZM4Sw0Jq .

GlennED commented 6 years ago

John,

I have tried your solution and it works perfectly. I don’t believe that there are any potential negative side-effects.

From: johnstosh [mailto:notifications@github.com] Sent: Tuesday, March 20, 2018 9:20 AM To: nyholku/purejavacomm purejavacomm@noreply.github.com Cc: Davidson, Glenn gdavidson@seabird.com; Author author@noreply.github.com Subject: Re: [nyholku/purejavacomm] Non-responsive bluetooth serial port hangs application (#106)

At our office, when waiting on hardware, we will use System.currentTimeMillis() like the following

PureJavaSerialPort(String name, int timeout) throws PortInUseException { super();

boolean usepoll = false; if (JTermios.canPoll()) { String key1 = "purejavacomm.use_poll"; String key2 = "purejavacomm.usepoll"; if (System.getProperty(key1) != null) { usepoll = Boolean.getBoolean(key1); log = log && log(1, "use of '%s' is deprecated, use '%s' instead\n", key1, key2); } else if (System.getProperty(key2) != null) { usepoll = Boolean.getBoolean(key2); } else { usepoll = true; } } USE_POLL = usepoll;

RAW_READ_MODE = Boolean.getBoolean("purejavacomm.rawreadmode");

this.name = name;

long before = System.currentTimeMillis(); int tries = (timeout + 5) / 10; while ((m_FD = open(name, O_RDWR | O_NOCTTY | O_NONBLOCK)) < 0) { int errno = errno();

try { Thread.sleep(10); } catch (InterruptedException e) { e.printStackTrace(); Thread.currentThread().interrupt(); }

// old way if (tries-- < 0) { throw new PortInUseException("Unknown Application", errno); }

// new way long after = System.currentTimeMillis(); if (after - before > timeout) { throw new PortInUseException("Unknown Application", errno); } }

}

-- Johnny Muczynski 734-262-2045

Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

johnstosh commented 6 years ago

There won't be any negative side effects for your situation.

Holku is concerned it will have surprise failures for other users because of the timeout being shorter for them.

On Tue, Mar 20, 2018, 9:42 AM GlennED notifications@github.com wrote:

John,

I have tried your solution and it works perfectly. I don’t believe that there are any potential negative side-effects.

From: johnstosh [mailto:notifications@github.com] Sent: Tuesday, March 20, 2018 9:20 AM To: nyholku/purejavacomm purejavacomm@noreply.github.com Cc: Davidson, Glenn gdavidson@seabird.com; Author < author@noreply.github.com> Subject: Re: [nyholku/purejavacomm] Non-responsive bluetooth serial port hangs application (#106)

At our office, when waiting on hardware, we will use System.currentTimeMillis() like the following

PureJavaSerialPort(String name, int timeout) throws PortInUseException { super();

boolean usepoll = false; if (JTermios.canPoll()) { String key1 = "purejavacomm.use_poll"; String key2 = "purejavacomm.usepoll"; if (System.getProperty(key1) != null) { usepoll = Boolean.getBoolean(key1); log = log && log(1, "use of '%s' is deprecated, use '%s' instead\n", key1, key2); } else if (System.getProperty(key2) != null) { usepoll = Boolean.getBoolean(key2); } else { usepoll = true; } } USE_POLL = usepoll;

RAW_READ_MODE = Boolean.getBoolean("purejavacomm.rawreadmode");

this.name = name;

long before = System.currentTimeMillis(); int tries = (timeout + 5) / 10; while ((m_FD = open(name, O_RDWR | O_NOCTTY | O_NONBLOCK)) < 0) { int errno = errno();

try { Thread.sleep(10); } catch (InterruptedException e) { e.printStackTrace(); Thread.currentThread().interrupt(); }

// old way if (tries-- < 0) { throw new PortInUseException("Unknown Application", errno); }

// new way long after = System.currentTimeMillis(); if (after - before > timeout) { throw new PortInUseException("Unknown Application", errno); } }

}

-- Johnny Muczynski 734-262-2045

Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nyholku/purejavacomm/issues/106#issuecomment-374601790, or mute the thread https://github.com/notifications/unsubscribe-auth/ABmim45iX__0yeA2kFZ9tFBiARslBDsTks5tgQdUgaJpZM4Sw0Jq .

nyholku commented 6 years ago

More precisely I'm concerned about the possibilities things I cannot see!

This approach looks " long after = System.currentTimeMillis(); if (after - before > timeout)" looks perfectly reasonable and difficult to see how that could fail.

However, when I wrote that code I had a reason to code it the way it was coded and I'm slightly concerned if the 'new' way is 100% compatible with old way.

But it is difficult to say how the 'new' way could cause problems for old users.

GlennED commented 6 years ago

Thanks everyone for your help with this issue.