serial: add serial transport API and implementations #5313
Conversation
|
~~Isn't this a duplicate of #4465?~~~ Edit: Sorry, it is still too early in the morning and I am tired... I only saw now that this isn't an issue, but actually a PR - so ignore all my comments...
Edit: Ok, you were speaking about an implementation - right, this seems to be the only option with EPL. |
Add the SODA's Device Kit implementation that is part of the Eclipse Kura project and provides an EPL based serial communication library. Related to: #5313 Signed-off-by: Markus Rathgeb <[email protected]>
There was a problem hiding this comment.
Thanks @maggu2810, that's an excellent work!
I haven't yet actively tested the code and I am astonished that in the end rxtx and javax.comm are so similar that you were able to put a facade on top of both.
Just out of curiosity, did you check current bindings in openhab2-addons whether they use any more specific rxtx features that might not be possible to use through this facade? If so, I don't have an issue if they have to stick to rxtx directly, so no worries.
My only wish would be to rename the bundles, trying to limit the number of dots in their names...
|
|
||
| <name>Eclipse SmartHome Serial Transport for javax.comm</name> | ||
|
|
||
| <!-- BEG: workaround until the respective three bundles has been added to our third party p2 repo --> |
There was a problem hiding this comment.
workaround can be removed now
| Bundle-ManifestVersion: 2 | ||
| Bundle-Name: Eclipse SmartHome Serial Transport API | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
| Bundle-SymbolicName: org.eclipse.smarthome.io.transport.serial.api;singleton:=true |
There was a problem hiding this comment.
I would prefer org.eclipse.smarthome.io.transport.serial as a bundle + package name - we usually never use api in such situations.
| </parent> | ||
|
|
||
| <groupId>org.eclipse.smarthome.io</groupId> | ||
| <artifactId>org.eclipse.smarthome.io.transport.serial.gnu.io</artifactId> |
There was a problem hiding this comment.
Maybe rename to org.eclipse.smarthome.io.transport.serial.rxtx?
| </parent> | ||
|
|
||
| <groupId>org.eclipse.smarthome.io</groupId> | ||
| <artifactId>org.eclipse.smarthome.io.transport.serial.javax.comm</artifactId> |
There was a problem hiding this comment.
Maybe rename to org.eclipse.smarthome.io.transport.serial.javaxcomm?
| } else { | ||
| return null; | ||
| } | ||
| // return getIdentifiers().filter(id -> id.getName().equals(name)).findFirst().orElse(null); |
There was a problem hiding this comment.
Hm, I am not lucky about that.
There is a problem caused by our usage of the nullness annotations and the miss of EEAs.
The interface is marked as "non null by default".
The optional type is a generic one.
The type parameter T of Optional<T> is set to SerialPortIdentifier (the variable opt is of type Optional<SerialPortIdentifier>).
As types are now default non null, the tooling consider T as @NonNull SerialPortIdentifier.
The optional's method orElse looks like public T orElse(T other) (see Optional API).
Now, the tooling things the method looks like public @NonNull SerialPortIdentifier orElse(@NonNullSerialPortIdentifier other).
But that is not correct: "other - the value to be returned if there is no value present, may be null".
So, the EEA needs to state, that regardless if T is nullable or not the method looks like:
public @NonNull T orElse(@Nullable T other);So, instead of using the method that is part of the Java API and does exactly what we want, we have to write other code just to fulfill the tooling (or better "to workaround a wrong tooling behavior").
|
You might also want to add something to the docs, at least add the io.transport.serial package to https://github.com/eclipse/smarthome/blob/master/docs/documentation/development/bindings/dependencies.md#eclipse-smarthome-packages. |
Please have a look at https://en.wikibooks.org/wiki/Serial_Programming/Serial_Java#RxTx_2 If something has been written against java(x).comm it should nowadays be enough to replace the one package with the other one (gnu.io). That has also been the reason why I did not choose rxtx but gnu.io as other implementations should (IMHO) be compatible, too. I can have a look at. -- edit begin -- Thinking about the implementation postfixes / names again: -- edit end -- One comment about the ".api" postfix. |
Sure, keeping RxTx directly is still possible. But we should perhaps try to use the newly interface (and implementations) if possible.
I didn't check the other addons yet. I tested the recent proprietary EnOcean binding implementation (just a little bit) and it seems to do its job. If there are other methods needed, we could add them later easily. Are there special addons that you would be happy if I could have a look at. |
Thanks, an interesting Java Serial history lesson :-)
I'd prefer this naming. At the moment, we otherwise have two different semantics of the "dots": Package/bundle hierarchy on the one hand and external package name characters on the other hand - I prefer to keep it for hierarchy usage only and thus I'd hope for names that only introduce a single sub-name behind
Strictly speaking, wouldn't his part rather be an spi? I think we should not really have api/spi in the package names at all - after all, we are using OSGi and we have defined "internal" packages that are not exposed and that all other packages are exported and thus are automatically an api/spi. For a more detailed information for implementors, I would rather suggest to follow up on #4736 (@SJKA feel free to comment as well).
Yes, fully agree. Once this is merged, I expect every new contribution on ESH and openHAB side to use the ESH API and not RXTX directly.
No, I don't have anything specific in mind right now. |
maggu2810
changed the title
Signed-off-by: Markus Rathgeb <[email protected]>
|
Remaining tasks:
Remaining parts should be addressed. Will give a ping as soon as finished. |
| * @author Markus Rathgeb - Initial contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public interface SerialPortEvent { |
There was a problem hiding this comment.
Wouldn't it make more logical if we provide an enum instead?
There was a problem hiding this comment.
Hi @amitjoy,
the intention of this serial transport has been to provide a thin abstraction layer for the specific implementations. There is the Java Communications API and that API is using the package javax.comm. There has also been RXTX that provides (v2.1 upwards) that but using another package only (gnu.io).
The most implementations I know about support that official API under one of that packages.
To keep the abstraction layer thin I would like to keep that numbers as in the "usual" implementations.
Sure, we could create an enum and assign an internal number to that enumerations but then we need to translate from enum to int on every call and from int to enum on every return where that values will be used (e.g. method that will be added on request).
That is the background...
Should we really add the overhead at runtime just to have enums instead of int and to be strictly speaking just to have some stronger type constraints?
There was a problem hiding this comment.
From the coding perspective, it would be more readable to have an enum instead. But I feel, the design perspective is different in this scenario. Yes, you are right. It would be a bit of overhead since it will always be a runtime dynamic execution. You can leave it as it is.
| * @author Markus Rathgeb - Initial contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public interface SerialPortEventListener { |
There was a problem hiding this comment.
Hm, you are right, this interface currently has only one function, so it could be a good candidate.
But personally I don't like to mark a listener as a function interface because it is about a special interface what a listener has to be provide and that could vary.
See also the [documentation of the functional interface annotation]:
If a type is annotated with this annotation type, compilers are required to generate an error message unless:
and
However, the compiler will treat any interface meeting the definition of a functional interface as a functional interface regardless of whether or not a FunctionalInterface annotation is present on the interface declaration.
The only real "win" is to get a compiler error if someone adds another function to that interface.
But if we ever want another function in that interface we will do it and have to remove that annotation again.
So, what is the big win to add it now?
[documentation of the functional interface annotation]: However, the compiler will treat any interface meeting the definition of a functional interface as a functional interface regardless of whether or not a FunctionalInterface annotation is present on the interface declaration.
There was a problem hiding this comment.
You are absolutely right. From the provider perspective it doesn't add real value but from the consumer's perspective, it does. The annotation benefits the provider because it will generate a compile error if the interface is not functional - and it benefits the consumer because it documents that the interface is meant to be used in lambdas. That's the only benefit. I am leaving it to you now.
| * | ||
| * @param owner name of the owner that port should be assigned to | ||
| * @param timeout time in milliseconds to block waiting for opening the port | ||
| * @return a serial port |
There was a problem hiding this comment.
@throws NPE or annotations on owner
There was a problem hiding this comment.
Have you realized that the whole class is annotated by NonNullByDefault? No need to add the NonNull annotation to any members, only Nullable would be needed.
There was a problem hiding this comment.
Didn't notice at all. Thanks for the clarification.
| } else { | ||
| return null; | ||
| } | ||
| // return getIdentifiers().filter(id -> id.getName().equals(name)).findFirst().orElse(null); |
There was a problem hiding this comment.
I still prefer the commented way
There was a problem hiding this comment.
Me, too and that has been the reason I didn't delete that line but comment it until we ever have an agreement about EEAs (if you are interested in have a look at the other issues and PRs).
Here the reason why it is this way ATM: #5313 (comment)
| * @param listener the listener | ||
| * @throws TooManyListenersException if too many listeners has been added | ||
| */ | ||
| void addEventListener(SerialPortEventListener listener) throws TooManyListenersException; |
There was a problem hiding this comment.
annotation on listener denoting it cannot be null
There was a problem hiding this comment.
Have you realized that the whole class is annotated by NonNullByDefault? No need to add the NonNull annotation to any members, only Nullable would be needed.
There was a problem hiding this comment.
Didn't notice that at all. Sorry for the noise. 👍
| * @param timeout | ||
| * @throws UnsupportedCommOperationException if the operation is not supported | ||
| */ | ||
| void enableReceiveTimeout(int timeout) throws UnsupportedCommOperationException; |
There was a problem hiding this comment.
@throws IllegalArgumentException if {@code timeout} is less than zero
There was a problem hiding this comment.
I will have at the respective implementations (see comment above) about their documentation and implementation. After that I will add a documentation and catch an invalid input in the abstraction layers or throw an exception.
Thanks for the hint.
There was a problem hiding this comment.
- nrjavaserial throws an IAE on a negative value
- dkcomm does nothing on a negative value
- purejavacomm throws an IAE on a negative value
- jSerialComm
does not support that operation, so a wrapper should throw antakes the negative value, but I cannot find at which place the value is usedUnsupportedCommOperationException - serial.io I do not have access to the sources, the JavaDoc does not state how a negative value is handled
- java-simple-serial-connector the input and output stream handling needs to be wrapped by the "wrapper implementation" also that function, too
There was a problem hiding this comment.
Let's define that an IAE should be thrown.
I changed the documentation of the interface and fixed the wrapper implementations.
Signed-off-by: Markus Rathgeb <[email protected]>
Signed-off-by: Markus Rathgeb <[email protected]>
|
@kaikreuzer IMHO it is ready for a final look at... |
Signed-off-by: Markus Rathgeb <[email protected]>
| @@ -304,7 +304,22 @@ | |||
|
|
|||
| <feature name="esh-io-transport-serial" version="${project.version}"> | |||
There was a problem hiding this comment.
Just for my understanding: This feature can now be regarded as legacy and solutions should always either use esh-io-transport-serial-javacomm or esh-io-transport-serial-rxtx instead, is this correct?
If so, I wouldn't mind to completely remove it - for me there isn't any need for this to be backward compatible.
There was a problem hiding this comment.
Bundles needs the serial API and "some" implementation.
So, features that require the serial transport depends on esh-io-transport-serial.
If no implementation is already installed it falls back to esh-io-transport-serial-rxtx and install also this one.
If the dependencies are already satisfied, because a product or user installs esh-io-transport-serial-javacomm the dependencies of esh-io-transport-serial are already installed and esh-io-transport-serial-rxtx is not installed.
We depend on an unspecific esh-io-transport-serial feature, that also fallback to a specific implementation if one is missing.
Have you a better idea how to model the unspecific dependency to "any" implementation?
AFAIK it is modeled similar in upstream Karaf for e.g. a "http" implementation that just defaults to the "jetty" one.
There was a problem hiding this comment.
If the dependencies are already satisfied, because a product or user installs esh-io-transport-serial-javacomm the dependencies of esh-io-transport-serial are already installed
I still haven't fully grasped the Karaf feature dependency management here - sorry, if I ask stupid questions.
But esh-io-transport-serial defines esh-io-transport-serial-rxtx as its dependency, so how can this be already installed, if a user only installed esh-io-transport-serial-javacomm? To me, it appears as if esh-io-transport-serial is tied to rxtx and not being unspecific (while the previous "require feature=serial" looked pretty unspecific).
There was a problem hiding this comment.
The feature resolution is done using OSGi requirements and capabilities.
All the ones in the manifests of the bundles are taken into account and also that ones that are added to the feature itself.
So, package imports and exports define requirements and capabilities and also the explicit provide capability and require capability ones are taken into account.
As bundles sometimes miss that information it could be added to the feature, too (as it is sometimes not possible to modify the bundles).
We also define a lot of capabilities in the features instead of adding that to the bundles...
If a feature or a dependency is using the attribute dependency="true" it will be installed only if there are requirements that are not satisfied without that one and satisfied with that one.
If a bundle requires an OSGi service that implements a specific interface the bundle should contain a specific require capability section.
If a bundle contains an OSGi service that implements a specific interface the bundel should contain a specific provide capability section.
Here you point me with your comment to the situation that our manifest misses the information (e.g. the serialbutton binding) that it needs such a service implementation ... I added the requirement for a specific implementation to the unspecific serial feature now.
So, features itself that needs a serial implementation should depend on "...-serial" that is does not specify the implementation itself. It only brings in one using dependency="true" if the capability is not already satisfied.
If an user install before or later a feature that brings in the requirement, the fallback feature can be dropped again, if that feature is removed another time the fallback will be used again...
We already use the dependency attribute for the two different Guice bundles, so the bigger one is only installed if required (and the smaller one could be dropped if the bigger one is installed).
There was a problem hiding this comment.
Ok, I think I got it. Many thanks for the elaborate explanation!
Signed-off-by: Markus Rathgeb <[email protected]>
|
@maggu2810 Latest openHAB distro with this change now logs at startup. Did we miss anything? |
|
I assume it is called by the line Service-Component: OSGI-INF/*.xml of the serial bundle and the fact, that there will be no component in the |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/2-3-0-snapshot-1250-classicui-essentially-blank/43252/10 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-2-3-problem-with-serial-binding-dependency/45715/5 |
As a framework we should not depend on a special serial transport implementation.
I started a thin abstract layer between the specific implementation and its usage.
There is an API bundle that defines the interface but is not bound to any specific implementation.
Using OSGi service dependency injection this is the only package that needs to be referenced and the consumer does not need to know the specific implementation.
I also added two implementations wrappers. One for "gnu.io" and one for "javax.comm".
There is a "org.eclipse.soda.dk.comm" implementation that uses "javax.comm" and is the only serial port communication I know that is licensed using the EPL.
So, I really prefer to leave it to solutions to choose the implementation for serial access and support a easily switch between e.g. nrjavaserial and that one.