Skip to content
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

Pure Java Comm - Initial Contribution #3632

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kgoderis
Copy link
Contributor

Alternative Comm Port Provider based on https://github.com/nyholku/purejavacomm. PureJavaComm aims to be a drop-in replacement for Sun's (now Oracle) abandoned JavaComm and an easier to deploy alternative to RXTX. Note that RXTX native binaries are not provided for Apple's (newer) ARM64 architecture, and therefore this is the only alternative to get serial ports working

Signed-off-by : Karel Goderis karel.goderis@me.com

@kgoderis kgoderis requested a review from a team as a code owner May 26, 2023 06:54
@kgoderis
Copy link
Contributor Author

@splatch Why is there an error on the DCO for this PR?

@splatch
Copy link
Contributor

splatch commented May 29, 2023

@splatch Why is there an error on the DCO for this PR?

Looks like bot does not recognize sign off for you commit. Can you do git commit -a -s on branch you used and force push to see if DCO will be happier?
It could be that earlier amend you did, did not preserve all necessary internals beyond commit message.

@kgoderis
Copy link
Contributor Author

@splatch Why is there an error on the DCO for this PR?

Looks like bot does not recognize sign off for you commit. Can you do git commit -a -s on branch you used and force push to see if DCO will be happier? It could be that earlier amend you did, did not preserve all necessary internals beyond commit message.

mmh..... "nothing to commit, working tree clean"

@splatch
Copy link
Contributor

splatch commented May 30, 2023

mmh..... "nothing to commit, working tree clean"

@kgoderis Sorry, it should be git commit --amend -s, my bad.

@kgoderis
Copy link
Contributor Author

mmh..... "nothing to commit, working tree clean"

@kgoderis Sorry, it should be git commit --amend -s, my bad.

Pfff.... It was because of a space between "off" and ":". LOL

@J-N-K
Copy link
Member

J-N-K commented May 30, 2023

Just for my understanding: Is there any limitation compared to the implementation we already have? Otherwise I would say we could also replace the code we use now. It seems to be confusing to have code that does not work on some architectures when we have very similar code that does.

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label May 30, 2023
@splatch
Copy link
Contributor

splatch commented May 30, 2023

Just for my understanding: Is there any limitation compared to the implementation we already have?

There is, pure java comm provides just a serial port API, nrjavaserial brings rfc2217 which handles serial over tcp. Whole rfc2217 support in nrjavaserial provides API compatible with rxtx: https://github.com/NeuronRobotics/nrjavaserial/blob/af0e83d320920cf0ca3cc8fd25a4f25f482fd0d6/src/main/java/gnu/io/rfc2217/TelnetSerialPort.java#L58.
Not sure if you can pair both without running into troubles.

@kgoderis
Copy link
Contributor Author

kgoderis commented Jun 4, 2023

@J-N-K Side question : is there a way to delay the loading of bundles that use serial transport bundles? I noticed that (in this case with pure java comm) some dependant bundles receive an empty serial port list from the SerialPortManager at first; one has to pause and reinitialize things in order to get connected the serial port

@kgoderis
Copy link
Contributor Author

Just for my understanding: Is there any limitation compared to the implementation we already have? Otherwise I would say we could also replace the code we use now. It seems to be confusing to have code that does not work on some architectures when we have very similar code that does.

@J-N-K http://www.sparetimelabs.com/purejavacomm/purejavacomm.php

@kgoderis
Copy link
Contributor Author

@openhab/core-maintainers When will this be merged ?

@splatch
Copy link
Contributor

splatch commented Jun 15, 2023

@kgoderis I think you may improve feature a bit by making certain features conditional. Given that purejavacomm can be default serial provider for mac, we could utilize karaf features with conditional elements:
https://github.com/apache/karaf/blob/karaf-4.4.3/features/core/src/test/resources/org/apache/karaf/features/internal/service/f06.xml#L26L33

Looking at it I think its possible to define a operating system condition through https://docs.osgi.org/specification/osgi.core/7.0.0/framework.namespaces.html#framework.namespaces.osgi.native, however I never really tested that . I been using Bundle-NativeCode several times, but never to detect OSX. ;-)

Standardized operating system labels are here: https://docs.osgi.org/reference/osnames.html

Signed-off-by: Karel Goderis <karel.goderis@me.com>
@kgoderis
Copy link
Contributor Author

@splatch Ok, I tried something, and it is compiling, but I have no clue if that is any indication that things are working.

@kgoderis
Copy link
Contributor Author

@openhab/core-maintainers Can someone verify the above?

@@ -232,6 +232,15 @@
<bundle>mvn:org.eclipse.kura/org.eclipse.soda.dk.comm.x86_64/1.2.201</bundle>
</feature>

<conditional>
<condition>req:osgi.native;filter:="(&amp;(osgi.native.osname=MacOSX)(osgi.native.processor=AArch64))"</condition>
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that conditional element can be specified under <feature> element to determine conditional dependencies. From this point of view proper place might be a upper level feature such as openhab.tp-serial (if one exists) which can make two conditions 1) for macos/aarch64 - which you made, calling feature openhab.tp-serial-purejavacomm, and 2) for anything else than macos/aarch64 pulling in openhab.tp-serial-rxtx.

Copy link
Member

Choose a reason for hiding this comment

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

See for example:

<conditional>
<condition>req:osgi.native;filter:="(osgi.native.osname=Linux)"</condition>
<bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.linuxsysfs/${project.version}</bundle>
</conditional>
<bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.ser2net/${project.version}</bundle>
<conditional>
<condition>req:osgi.native;filter:="(osgi.native.osname=Windows*)"</condition>
<bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.windowsregistry/${project.version}</bundle>
</conditional>

@wborn
Copy link
Member

wborn commented Aug 19, 2023

Would be nice to know how well it performs compared to nrjavaserial. As I understand JNA based implementations perform a lot worse compared to implementations using JNI.

https://en.wikipedia.org/wiki/Java_Native_Access#Performance:

Benchmarks show JNA averages ten times slower than JNI.

@wborn
Copy link
Member

wborn commented Aug 27, 2023

Some bindings do not use the OH serial transport, so you won't be able to use these with your ARM64 Apples.

See: openhab/openhab-addons#7573

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-x-oh4-x-alternative-java-serial-provider/128462/34

<parent>
<groupId>org.openhab.core.bundles</groupId>
<artifactId>org.openhab.core.reactor.bundles</artifactId>
<version>4.0.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<version>4.0.0-SNAPSHOT</version>
<version>4.2.0-SNAPSHOT</version>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants