NeuronRobotics / nrjavaserial

A Java Serial Port system. This is a fork of the RXTX project that uses in jar loading of the native code.
Other
344 stars 143 forks source link

Q: When OpenJDK related changes expected to be released? #156

Closed yuriibratchuk closed 4 years ago

yuriibratchuk commented 4 years ago

Hi Kevin @madhephaestus , I see that you are the most recent maintainer and very active contributor, so let me first say Thank you for your great work & contribution to this awesome library! ;)

There is a fix for OpenJDK support has been already submitted and available in java11 branch. It might be very important for the users who are switching away from Oracle's Java.

The last release of nrjavaserial, 3.15.0 was in November of 2018, more than a year ago.

When is it planned to release these OpenJDK-related changes?

Thank you!

Best regards, Yurii

Referenced: openjdk11 support Release schedule?

madhephaestus commented 4 years ago

Given the state of the world, i have a little time to take a look at this finally. Sorry for the delay all!

madhephaestus commented 4 years ago

Please check https://github.com/NeuronRobotics/nrjavaserial/releases/tag/4.0.0

yuriibratchuk commented 4 years ago

@madhephaestus , thanks a lot for delivering so quick release! 👍 I've checked the released changes for 4.0.0 and 4.0.1 and see that changes from https://github.com/NeuronRobotics/nrjavaserial/pull/133 were not included...

Could you please check?

Thank you!

madhephaestus commented 4 years ago

@yuriibratchuk what OS?

yuriibratchuk commented 4 years ago

@madhephaestus Windows 10

madhephaestus commented 4 years ago

@yuriibratchuk published 4.0.2 and 3.16.1 with the windows update

yuriibratchuk commented 4 years ago

@madhephaestus , thanks a lot for the quick update and new releases! ;)

Regard Java 8 - specific release, I've notice that in the latest (on this moment) release 3.18.0 https://github.com/NeuronRobotics/nrjavaserial/blob/3.18.0/src/main/c/src/SerialImp.c still contains get_java_var_long an "old" return type. According to https://github.com/NeuronRobotics/nrjavaserial/pull/133/ the type has changed from long to size_t.

Could you pls clarify, is it expected to keep OpenJDK-related in 3.x releases?

Just curious about these important changes for the users that might not be able to switch their projects quickly to java 11.

Thank you in advance!

madhephaestus commented 4 years ago

@yuriibratchuk I made a merged release, its compiled in java 8 and incorporates the java11 commits. Can you check to see if 3.19.0 works in your java11 build?

wborn commented 4 years ago

Is the only difference between 3.19.0 and 4.2.0 now that 4.2.0 is compiled with Java 11 whereas 3.19.0 with Java 8 @madhephaestus? So 3.19.0 has the Java 11 compatibility fix so it works on Java 8/11? I'm trying to figure out what version we want to use with openHAB 3 which will require Java 11. :wink:

madhephaestus commented 4 years ago

yes, thats the only difference now. I am not sure that it is important, so i want someone running a java11 deployment to try 3.19.0. If we get the OK then the 4.x branch of java 11 only can die. Ideally we have just 3.19.0 as the single source moving forward.

wborn commented 4 years ago

OK thanks for the info! I will try 3.19.0 on Java 11 then and see how well it works. :+1: Usually I test linux64 and armhf (RPi). But with a bit of effort I can also test Windows in a VM and run a arm64 image on the same RPi.

I also saw you made some changes for the lockfile in https://github.com/NeuronRobotics/nrjavaserial/commit/53a85c64ad24060c0eec61525963a6e5590ced35. Does it still use the same kind of lockfiles now because -DLIBLOCKDEV / -llockdev was removed? Was that change just because the liblockdev1-dev package was renamed to liblockfile-dev? It may relate to https://github.com/NeuronRobotics/nrjavaserial/issues/60.

madhephaestus commented 4 years ago

i have liblockfile installed and can not get the old DLIBLOCKDEV code to compile. It was only being used on the linux systems and was causing compatibility issues as it was changed at the OS level. I would be happy to revisit this issue in #60 but would like the fix to be cross platform and not depend on a system dep.

wborn commented 4 years ago

I don't mind that the lockfiles are no longer enabled because we also had users having issues with it. To mitigate that we already used a recompiled version in which lockfile support was disabled.

wborn commented 4 years ago

While testing 3.19.0 on Linux x86_64 I noticed there are still lock files being generated/used in /run/lock. So maybe not all (Linux) .so files were updated for this change @madhephaestus?

It should be using the regular Linux x86_64 .so file:

$ lsof userdata/tmp/libNRJavaSerial_wouter_0/*
COMMAND   PID   USER  FD   TYPE DEVICE SIZE/OFF     NODE NAME
java    15041 wouter mem    REG  259,2    75640 46007374 userdata/tmp/libNRJavaSerial_wouter_0/libNRJavaSerial.so
$ md5sum userdata/tmp/libNRJavaSerial_wouter_0/*
a80c5d3149c704f9253c07199e38706a  userdata/tmp/libNRJavaSerial_wouter_0/libNRJavaSerial.so
madhephaestus commented 4 years ago

I just recompiled master and checked the binaries, no change.

I also removed liblockfile-dev and recompiled, so there is no possible way the dep is being used. The compile worked, and the binaries are identical. AFAIK, RXTX had always used lock files, but had not used a lockfile library. We added the library, but this caused compile issues across the platforms. The failover when the lockfile library is not availible, it uses the old built in lock file system. Is this the behavior your seeing?

wborn commented 4 years ago

That indeed explains it. I wasn't aware there was still a build in lock file system being used. We currently use a recompiled version using -DDISABLE_LOCKFILES instead of -DLIBLOCKDEV and without -llockdev. But your changes obviously didn't add -DDISABLE_LOCKFILES.

madhephaestus commented 4 years ago

are the lockfiles causing an issue? should they be disabled on all platforms?

wborn commented 4 years ago

I will do some more testing to see if the build in lockfiles are really causing us issues. It's better to have some sort of locking mechanism in place instead of none to prevent issues.

wborn commented 4 years ago

I've completed testing 3.19.0 with Java 11 on Linux (x86_64, armhf, arm64) and Windows (64-bit). It seems to work fine on those platforms and doesn't crash the Java 11 VM. :+1: So I think there's currently not really a need for maintaining the 4.x version.

The only real issue I ran into (Linux x86_64) was that a few times the following error was printed:

initialise_event_info_struct: initialise failed! (similar to issue https://github.com/NeuronRobotics/nrjavaserial/issues/111)

See code:

https://github.com/NeuronRobotics/nrjavaserial/blob/13fd9e19e5d03bcf8d52e694675311bdbcddf45f/src/main/c/src/SerialImp.c#L4204

It didn't crash the JVM, but afterwards the serial connection couldn't be used. I don't know exactly what the root cause is or how to reproduce it. It occurred once after removing a connection and then recreating it. But I couldn't reproduce it this way afterwards.

The built in lockfiles seem to work pretty well. They didn't cause issues with Docker or after killing the application. It would just print "Removing stale lock file" and continue after restarting the application.

Sometimes I do see some warnings regarding ports already been locked:

RXTX fhs_lock() Error: opening lock file: /var/lock/LCK..ttyUSB0: File exists. It is mine

I know how to reproduce that so I will look into if it can be prevented by rewriting some code in openHAB because it can be disturbing for end users when it occurs frequently.

wborn commented 4 years ago

I had a look at our code and it doesn't seem we can prevent the warning ("RXTX fhs_lock() Error: opening lock file: /var/lock/LCK..ttyUSB0: File exists. It is mine"). It is always printed whenever there is a port open (locked) and we call CommPortIdentifier.getPortIdentifiers() while gnu.io.rxtx.SerialPorts isn't set. In that case we want it to register scanned ports so we know what all available ports are:

https://github.com/NeuronRobotics/nrjavaserial/blob/13fd9e19e5d03bcf8d52e694675311bdbcddf45f/src/main/java/gnu/io/RXTXCommDriver.java#L413-L422

madhephaestus commented 4 years ago

Great!! @wborn Thank you for the testing, im very glad to hear we can move forward with the single set of sources.

As for the print issue, maybe that should be its own issue?

wborn commented 4 years ago

It's probably best to do that since it has been there in previous versions as well. I've created #166 for this @madhephaestus.