openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
912 stars 420 forks source link

Update nrjavaserial #101

Closed cvanorman closed 7 years ago

cvanorman commented 7 years ago

OH currently uses nrjavaserial-3.9.3 in the serial transport. I've had issues getting serial communications to work properly (in particular with the UPB binding) and these issues go away with the latest version (3.12.1).

Is there any particular issues or concerns with updating this library?

watou commented 7 years ago

Related issue https://github.com/openhab/openhab-core/issues/29#issuecomment-268353108

cvanorman commented 7 years ago

Ok, well, obviously I didn't look very hard. I've read through a bit of that and it seems the primary issue is jar size (as they've include commons-net and commons-lang3). I can't commit to maintaining a fork, but I could certainly produce a slim jar of 3.12.1 by removing those. This assumes that those libraries are available in OH somewhere else, though. If we need to remove the dependency entirely, that would require some more investigation into how they are using it.

The Arm64 v8hf support is not functional in 3.9.3 from what I understand, so I'm presuming that is not a blocker.

watou commented 7 years ago

Thank you for working on this @cvanorman!

ThomDietrich commented 7 years ago

This change introduced a new issue with users zwave-connection @kai @cvanorman

https://community.openhab.org/t/oh2b5-and-dev-ttyacm0-z-wave/18783/10

Finding this thread, I installed liblockdev1 and finally it works! So my guess would be that the latest nrjavaserial needs the liblockdev1 as a dependency (at least on raspberry pi 2)?

Either the dependency can be eliminated or the package needs to be installed with openhab2... However not everyone is successful with the simple installation of the package. I would suggest to revert the latest commit and look for a better solution.

kaikreuzer commented 7 years ago

Probably related to this change: https://github.com/NeuronRobotics/nrjavaserial/pull/65

And there is another problem report about it: https://github.com/NeuronRobotics/nrjavaserial/issues/60#issuecomment-225506468

@cvanorman & @watou What do you suggest to do? What issues were solved by the upgrade?

cvanorman commented 7 years ago

Well, for me, I had trouble getting it to allow any reading of serial input on Windows. I see there are a number of other open issues under OH about it:

Unfortunately, I don't see any easy solution given the current state of the nrjavaserial project. There appears to be a significant amount of maintenance required to update the native code portion to handle the variety of platforms being used (mostly around file locking). The addition of liblockdev was an attempt to fix this, but obviously broke it for others.

It may be time for OH to consider alternatives to nrjavaserial. I believe Java 8 has added some device io functionality.

kaikreuzer commented 7 years ago

I reverted the commit again. Regarding other options: There isn't really anything better, I already did a lot of research in the past. All solutions require native parts (which thus need to be compiled) and there is no project being really actively maintained.

Java 8 does not bring anything new, I assume you were referring to https://wiki.openjdk.java.net/display/dio/Main. But this is not part of SE 8 (only ME 8) and I cannot see much recent activity on this either...

cvanorman commented 7 years ago

Yes, I spent some time after that comment and I'm not seeing much that looks promising.

The locking mechanism in nrjavaserial (at least for linux) could probably be changed to a no-op fairly easily (with a compiler flag). This would not be too inconsistent with common behaviour in linux and would be easier than attempting to establish a locking mechanism that would work across multiple linux platforms.

It would have to be done on a fork (which could also trim it down from a fat jar). I have time to do this right now, but I can't guarantee time to provide long term maintenance of the library.

cvanorman commented 7 years ago

Of course, you could also just accept that it doesn't work and those that want to use an updated version of the library can uninstall the openhab-transport-serial bundle and drop the nrjavaserial 3.12.1 in their addons folder.

kaikreuzer commented 7 years ago

It would have to be done on a fork (which could also trim it down from a fat jar). I have time to do this right now, but I can't guarantee time to provide long term maintenance of the library.

This would be terrific!

splatch commented 7 years ago

There are other options for serial port access. Sadly rxtx seems to be abandoned a while ago and all work is done in forks. There are other options, which are actively maintained such jSerialComm (LGPL) or purejavacomm (BSD). Did anyone experiment with these?

cvanorman commented 7 years ago

I looked at them and the easier path is still nrjavaserial. jSerialComm suffers from the same native issues as nrjavaserial, and purejavacomm does not seem to have the wide array of platform support that is available in nrjavaserial.

cvanorman commented 7 years ago

I've forked nrjavaserial and made a few changes to it:

The jar is fully OSGi compliant (and works like a charm just dropping it into the addons folder). What would work best for including it?

kaikreuzer commented 7 years ago

Hey, cool! I have forked your fork to https://github.com/openhab/nrjavaserial, so that we have the source of what is used in openHAB available under the openHAB org at github :-)

If you could upload the jar here, I would make it available on https://bintray.com/openhab/mvn and include it in the target platform (while removing io.transport.serial at the same time).

cvanorman commented 7 years ago

File attached.

nrjavaserial-3.12.0-OH.zip

kaikreuzer commented 7 years ago

Many thanks, @cvanorman! The jar is now available at https://bintray.com/openhab/mvn/nrjavaserial It is included in this p2 repo: https://bintray.com/openhab/p2/openhab-deps-repo/1.0.10#files/openhab-deps-repo/1.0.10 which is picked up by the openhab.target by https://github.com/openhab/openhab-distro/pull/363.

The io.transport.serial bundle has been removed and the nrjavaserial bundle is instead included through https://github.com/openhab/openhab-core/pull/104.

maggu2810 commented 7 years ago

@kaikreuzer Are the issue reporting deactivated by intention (of the nrjavaserial repo)? I assume the readme should be changed as there are still references to the original repository (e.g. git clone https://github.com/NeuronRobotics/nrjavaserial.git)

I assume there will be further changes from time to time. Or do you want to keep the work as small as possible and so use the openHAB issue tracker only (so nobody see the repo as a new maintained fork)?

What about the serial implementation of the Kura project?

kaikreuzer commented 7 years ago

I didn't create the repo as a fork that is supposed to be an actively maintained version of nrjavaserial. I still hope that the original project isn't dead and will be updated again some time. The repo at openHAB is just for tracking the manual changes that have been done to the original version, so that it can be easily re-applied on any future updates.

What about the serial implementation of the Kura project?

It isn't using gnu.io, so that isn't really an option for openHAB (as all code would need to be refactored). And afaik, it is also a pretty old and neglected library...

ThomDietrich commented 7 years ago

@cvanorman is liblockdev* still needed with the changes you've added? Check https://community.openhab.org/t/nrjavaserial-upgrade-causes-serial-error-port-dev-ttyacm0-does-not-exist/18783/51

cvanorman commented 7 years ago

No, the latest version (3.12.0-OH) disables file locking on linux platforms.

There is also #111 which would affect Debian Wheezy users.

msteigenberger commented 7 years ago

Hi, I'd need RFC 2217 for my binding. Can we add rfc2217 classes again without including the dependencies? I've created a PR for this: https://github.com/openhab/nrjavaserial/pull/1

Thanks in advance!