mvalla / openwebnet4j

openwebnet4j is a Java library for the Open Web Net protocol
https://openwebnet4j.github.io/
Eclipse Public License 2.0
6 stars 6 forks source link

Support other serial implementations #34

Closed wborn closed 9 months ago

wborn commented 1 year ago

It would be nice when the library supports using other serial implementations instead of gnu.io.

That way openHAB could use its own serial transport with it, which would solve several issues (https://github.com/openhab/openhab-addons/issues/7573).

This could for instance be implemented similar to the calimero-rxtx implementation (see also this comment). See also https://github.com/openhab/openhab-addons/pull/14051 for how such a mechanism can be used for using the openHAB serial transport.

mvalla commented 1 year ago

@wborn this makes sense. However this has not been on my priority since in the overall list for some bindings it appears is not easy/possible to avoid gnu.io and use the transport instead (for example pentair, oceanic). If the replace of gnu.io is a target for OH 4.0, I will try to follow and complete the migration also for openwebnet4j.

mvalla commented 1 year ago

also @wborn : what if gnu.io and org.openhab.core.io.transport.serial are not fully aligned, for example: gnu.io.SerialPortEvent.HARDWARE_ERROR is not present in org.openhab.core.io.transport.serial.SerialPortEvent

wborn commented 1 year ago

We can probably add anything that's missing from gnu.io to the OH serial transport to make it work with all add-ons. That's how we got it to support the majority of add-ons.

mvalla commented 1 year ago

ok, perfect, I will maybe open a PR for that on openhab-core when I will try to migrate from gnu.io to org.openhab.core.io.transport.serial

mvalla commented 11 months ago

@wborn if I want to start looking at this issue, which serial implementation should I look at that could replace gnu.io in OH ? I understand the approach and the adaptation mechanism, but I want also to be able to test a different serial implementation directly from this lib. So which one should I look for as a replacement for gnu.io ?

wborn commented 11 months ago

In OH you want to use the serial transport so we can easily switch between implementations (like openhab-addons#14051).

If you want to add some implementations in this repo you could add an implementation using nrjavaserial and one using purejavacomm, see also https://github.com/openhab/openhab-core/pull/3632. If you need an OSGi compatible artifact you may want to use the opensmarthouse fork.

If you have lots of time you could also add an implementation using jSerialComm. :slightly_smiling_face:

mvalla commented 11 months ago

@wborn I started looking at purejavacomm. But I found that also there SerialPortEvent.HARDWARE_ERROR is not present, so if I want to write an implementation using that lib instead of nrjavaserial, I need to remove some features... Not good. Also is seems the project is not updated (last commit June 2022) and it laks even the Issue tab. Has purejavacomm ever been tested within OH ?

wborn commented 11 months ago

It looks like @splatch and others use purejavacomm to workaround nrjavaserial issues, see also:

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

mvalla commented 11 months ago

Ok. I also looked to jSerialComm and it really it looks a much more active and complete project if compared to purejavacomm. It appears it also considers the HW error case (I think it's possibile to use LISTENING_EVENT_PORT_DISCONNECTED, but I will have to test it). @wborn can you suggest I take jSerialComm instead of purejavacomm as a testing alternate lib to nrjavaserial?

Also question: in case I implement support for other serial implementations here, will I be still be able to configure OH4 to use nrjavaserial as a Serial provider? How? By simply loading via Karaf another provider bundle instead of the default provider? (just to be sure my current system setup that works well with nrjavaserial is still supported)

splatch commented 11 months ago

There is not much "new" things within serial APIs, so purejavacomm activity reflects activity in this area. Have a look on this part: https://github.com/Fazecast/jSerialComm/blob/master/src/main/c/Posix/SerialPort_Posix.c#L4, I would consider 2013 "recent" in terms of the serial port apis. ;-) For sure jserialcomm gives you better visibility over what happens with serial port, if you need "just" data stream any implementation will work.

With regard to library design questions - try to isolate serial port work into your own basic interfaces. Then you can supply serial library with any implementation. See: https://github.com/Code-House/bacnet4j-wrapper/blob/1.3.x/mstp/src/main/java/org/code_house/bacnet4j/wrapper/mstp/MstpNetworkBuilder.java#L29 (this is a library contract) https://github.com/ConnectorIO/connectorio-addons/blob/addons-3.0.0-alpha-1/bundles/org.connectorio.addons.binding.bacnet/src/main/java/org/connectorio/addons/binding/bacnet/internal/handler/network/mstp/ManagedMstpNetworkBuilder.java#L42 (use within openhab using openhab serial apis) https://github.com/Code-House/bacnet4j-wrapper/pull/19/files - pure purejavacomm implementation example

wborn commented 11 months ago

can you suggest I take jSerialComm instead of purejavacomm as a testing alternate lib to nrjavaserial?

I haven't used any of them myself and don't know all the limitations of each implementation. I mentioned purejavacomm because it seems to be the most commonly used alternative implementation in OH and because it may also be supported in OH with https://github.com/openhab/openhab-core/pull/3632. If it is not possible or difficult to use purejavacomm as a replacement, feel free to add an implementation using jSerialComm.

wborn commented 11 months ago

in case I implement support for other serial implementations here, will I be still be able to configure OH4 to use nrjavaserial as a Serial provider? How? By simply loading via Karaf another provider bundle instead of the default provider?

You define an interface for providers to implement serial communications in this project. It will be similar to SerialComm in https://github.com/calimero-project/calimero-rxtx/commit/1e74d815b05ef1a14268095df2b512e760305d09 or the SerialPort in the OH serial transport. Only add the methods that you really need for serial communications in this library. The smaller the interface the easier it will be to add more implementations.

Then you implement a provider and make it possible in your library to switch between serial port provider interfaces. Other libraries often use the Service Provider Interface (SPI) for this, see: https://www.baeldung.com/java-spi If that is too complex at first, you can also also make it possible in your library to configure the fully qualified classname of the provider. Then you can create an instance of the provider using Class.forName:

package com.acme;

import java.lang.reflect.Constructor;

import org.junit.jupiter.api.Test;

public class ProviderTest {

    interface SerialCommProvider {
        String getProviderName();
    }

    public static class JSerialCommImpl implements SerialCommProvider {
        public String getProviderName() {
            return "JSerialCommImpl";
        }
    }

    public static class OHSerialCommImpl implements SerialCommProvider {
        public String getProviderName() {
            return "OHSerialCommImpl";
        }
    }

    String providerFQCN = "";

    public void createProviderFQCN() throws Exception {
        Class<?> providerClass = Class.forName(providerFQCN);
        Constructor<?> constructor = providerClass.getConstructor();
        SerialCommProvider provider = (SerialCommProvider) constructor.newInstance();
        System.out.println(provider.getProviderName());
    }

    @Test
    void createJSerialCommProviderFQCN() throws Exception {
        providerFQCN = "com.acme.ProviderTest$JSerialCommImpl";
        createProviderFQCN();
    }

    @Test
    void createOHSerialCommProviderFQCN() throws Exception {
        providerFQCN = "com.acme.ProviderTest$OHSerialCommImpl";
        createProviderFQCN();
    }
}

Even simpler would be to allow for setting an SerialCommProvider instance into one of your library objects.

When using openHAB the library would be reconfigured to use a class that implements your library interface using the OH serial transport.

mvalla commented 11 months ago

Thanks both, I will look at your suggestions.

Actually my concern is more: Will a serial port provider based on nrjavaserial still be available within OH4 and later versions so if I want I can use that lib with my configuration ?

mvalla commented 11 months ago

It looks like also jSerialComm is also aready used in openhab-addons here: https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.binding.proteusecometer and without using a Serial Transport. In fact I am really tempted to add an implementation here based on JSerialComm and not purejavacomm

wborn commented 11 months ago

Will a serial port provider based on nrjavaserial still be available within OH4 and later versions so if I want I can use that lib with my configuration ?

I think it will be there for a while. Each implementation has their own pros/cons so it is nice that OH allows for supporting multiple using the serial transport. That allows for switching between implementations whenever you run into bugs, performance, feature or platform compatibility issues.

mvalla commented 11 months ago

So in the end this is my plan:

  1. I will make here serial implementation pluggable via OH org.openhab.core.io.transport.serial.SerialPortProvider, to ease integration with the existing OH serial transport
  2. write here an implementation of SerialPortProvider using nrjavaserial
  3. change the openwebnet binding to use OH Serial Transport
  4. if I have time I will also write here an implementation of SerialPortProvider using jSerialComm
  5. if the last implementation works (and having more time...), I could also write a org.openhab.core.io.transport.serial.jserialcomm bundle: it should be easy after writing here the implementation of SerialPortProvider using jSerialComm

@wborn and @splatch does this plan make sense to you?

wborn commented 11 months ago

I will make here serial implementation pluggable via OH org.openhab.core.io.transport.serial.SerialPortProvider, to ease integration with the existing OH serial transport

It would probably be better to add the implementation using the OH serial transport to the binding in openhab-addons. That way you don't need to manage OH dependencies in your library. Nobody will use that implementation except for openHAB. Adding the implementation to the binding will also make sure it keeps working if the OH serial transport interfaces change.

mvalla commented 11 months ago

Yes sorry I did not explain it correctly, I rephrase:

  1. I will make here serial implementation pluggable using an interface similar to OH org.openhab.core.io.transport.serial.SerialPortProvider, to ease integration with the existing OH serial transport.

Of course I want my lib to have serial implementation pluggable (offering nrjavaserial as default) but still be completely indipendent from OH

splatch commented 11 months ago

@mvalla I think way you draw will be best. It will keep library open for standalone use and also provide a way for trouble-less integration with OH.

mvalla commented 11 months ago

Ok @wborn @splatch : I have a working branch for this lib that can use both NRJavaSerial and jSerialComm. I wrote wrappers and use Service Provider Interface (SPI) approach and works well. Now I started looking at opensmarthouse-purejavacomm, but it looks like is much less supported than jSerialComm (does not have even an Issues page!). Also PureJavaComm does not support the PORT_DISCONNECTED events that I think are useful nowdays that Serial ports devices are via USB and therefore can be unplugged.

Also there is already another OH binding using jSerialComm: ProteusEcoMeter. Would it make sense to create a new serial transport using jSerialComm and make the necessary changes to use it the openwebnet binding? Maybe the new transport could also be shared with ProteusEcoMeter binding. What is your opinion?

wborn commented 11 months ago

I have a working branch for this lib that can use both NRJavaSerial and jSerialComm. I wrote wrappers and use Service Provider Interface (SPI) approach and works well.

That is great news! :slightly_smiling_face:

Now I started looking at opensmarthouse-purejavacomm, but it looks like is much less supported than jSerialComm (does not have even an Issues page!).

The original repo is https://github.com/nyholku/purejavacomm but it seems to be very inactive. Maybe it was more active when @splatch decided to fork it?

Would it make sense to create a new serial transport using jSerialComm and make the necessary changes to use it the openwebnet binding?

It sounds like PureJavaComm lacks functionality that is available in both NRJavaSerial and jSerialComm. So in that case it might also be a useful addition.

Maybe the new transport could also be shared with ProteusEcoMeter binding.

I don't know why the messages got corrupt (https://github.com/openhab/openhab-addons/pull/11333), but it could be that jSerialComm uses different default configuration values compared to NRJavaSerial. In the end there should only be one serial transport (org.openhab.core.io.transport.serial) with several implementations that is used by all add-ons (https://github.com/openhab/openhab-addons/issues/7573). If you use a different serial transport implementation it should be used by all add-ons.

splatch commented 11 months ago

The original repo is https://github.com/nyholku/purejavacomm but it seems to be very inactive. Maybe it was more active when @splatch decided to fork it?

The 1.0.4 version was not deployed to maven central so @cdjackson pulled it into opensmarthouse where we have pipeline.

mvalla commented 11 months ago

Updated plan:

mvalla commented 10 months ago

@wborn and @splatch : so I use the SPI approach, and provide in the openwebnet4j lib a default implementation that uses NRJavaSerial (this is a requirement for me as I want the lib to remain only dependent on NRJavaSerial JAR).

When the lib is used in OH by the binding, the serial implementation provided by OH Serial Trasport will also be available. How to make sure the OH Serial Transport implementation is used , and not the one provided within the lib?

wborn commented 10 months ago

If your ServiceLoader.load call returns no values you could use the NRJavaSerial implementation by default. In your POM you can add nrjavaserial as dependency and not add one for jSerialComm or mark it as optional. Then it should work out of the box with nrjavaserial and if the ServiceLoader finds another implementation it will use that instead.

mvalla commented 9 months ago

If your ServiceLoader.load call returns no values you could use the NRJavaSerial implementation by default.

Done that, thanks @wborn !

So @wborn and @splatch thaks to your suggestions, I managed to write in the openwebnet binding the adapter to use OH Serial Transport. This is the class: SerialTransportAdapter

As you can see I used the @ServiceProvider(value = SerialPortProvider.class) annotation, so the service provider gets also declared in a META-INF file generated in the jar: file: META-INF/services/org.openwebnet4j.communication.serial.spi.SerialPortProvider content: org.openhab.binding.openwebnet.internal.serial.SerialTransportAdapter

But now I think I finished in the OSGi/SPI swamp:

any suggestion? Here's my current work in progress branch: https://github.com/mvalla/openhab-addons/tree/openwebnet-other-serial-impl/bundles/org.openhab.binding.openwebnet

wborn commented 9 months ago

Nice progress! :slightly_smiling_face:

If you use SPI with OSGi you need to make sure the proper bundle manifest entries are added. See:

https://aries.apache.org/documentation/modules/spi-fly.html

We also have a few bundles in openHAB that use such bundle manifest entries, see:

https://github.com/search?q=repo%3Aopenhab%2Fopenhab-addons+osgi.extender%3Dosgi.serviceloader&type=code

mvalla commented 9 months ago

So the suggestion is to simply add the Require-Capability binding manifest header like is done in the KNX binding pom.xml??

I don’t need to include SPI Fly jar myself nor add any other new manifest header to the openwebnet4j library (consumer), correct? (I would not want to tight the lib to this SPI Fly correction…)

sorry if this seems obvious to you but I did not fully understand all the SPI/OSGi stuff…

splatch commented 9 months ago

Just a rough idea - why pulling openhab adapter to openwebnet4j if adapter you do is necessary only for binding? If its helpful for binding you can bake it into binding code instead of making it external dependency to the binding. This way you don't need to fight with SPI-fly/OSGi that much. I've done it this way for bacnet binding, I just pass pre-populated transport type to network builder. The configuration of OH serial transport will most likely be passed directly from bridge configuration during thing handler initialize call. Making this stuff split in two places/repositories will contribute to maintenance cost of your adapter.

mvalla commented 9 months ago

why pulling openhab adapter to openwebnet4j if adapter you do is necessary only for binding? If its helpful for binding you can bake it into binding code instead of making it external dependency to the binding

Thanks @splatch for your comment. However I am not sure I understand it correctly:

If you can confirm the SPI mechanisms can be adjusted for OSGi by just adding some more manifest headers… if this is the case I prefer to compete this approach since I already implemented it.

splatch commented 9 months ago
  • the adapter is already part of the binding, and - as suggested previously in this thread - I already use the SPI mechanism to find the adapter (which is a service provider) from within the openwebnet4j library. The library is completely independent from the binding. Problem is serviceLoader finding mechanism is broken by OSGi!!

You're absolutely right about SPI loading being a trouble maker under OSGi. Since your serial transport adapter is part of binding and not library you do not have any benefit of OSGi services produced by SPI-fly.

  • what you suggest now would be to write a new “bypass method” in openwebnet4j lib in order to pass to the lib the SerialPort to be used directly by the binding, correct?

Bypassing is a strong statement, I'd say its more a manual injection rather than implicit. Sometimes being explicit is needed and better as it makes things just simpler. I had a look on your branch and now you pass just a port name to USBConnector and hide entire logic of serial port handling down there. If you bring another variant of constructor, which accepts SerialPortProvider or even SerialPortManager, then you can completely bypass ServiceLoader call. After all, you initialize USBConnector from binding, and you can provide a way which avoids SPI lookup.

I don't know openwebnet4j at all, hence my advice might be completely wrong from library design point of view.

mvalla commented 9 months ago

ok @splatch I got your suggestion now, and will try to inject the SerialPortManager into the lib.

However this way we loose the possibility to have hot-replacement of serial port management: once the SerialPortManager is injected, it will not change even if the old serial transport is removed, and a new serial transport is loaded via OSGi. Not a big deal, but of course we loose the dynamic feature offered by ServiceLoader. Probably also the OpenWebNet binding will require to be deactived/re-activated in order to use the new serial transport.

mvalla commented 9 months ago

@wborn, @splatch : I completed this by providing both SPI ServiceLoader and injection.

I also jsut submitted a new PR for openwebnet binding using injection mechanism.

Tested extensibely wih my own USB serial hw and a proof-of-concept of a new transport based on jSerialComm.

Updated plan: