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

Support other serial implementations #34

Closed
wborn opened this issue Mar 17, 2023 · 33 comments
Closed

Support other serial implementations #34

wborn opened this issue Mar 17, 2023 · 33 comments

Comments

@wborn
Copy link

wborn commented Mar 17, 2023

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 (openhab/openhab-addons#7573).

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

@mvalla
Copy link
Owner

mvalla commented Mar 18, 2023

@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
Copy link
Owner

mvalla commented Mar 18, 2023

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
Copy link
Author

wborn commented Mar 18, 2023

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
Copy link
Owner

mvalla commented Mar 18, 2023

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
Copy link
Owner

mvalla commented Nov 20, 2023

@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
Copy link
Author

wborn commented Nov 20, 2023

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 openhab/openhab-core#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. 🙂

@mvalla
Copy link
Owner

mvalla commented Nov 24, 2023

@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
Copy link
Author

wborn commented Nov 24, 2023

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
Copy link
Owner

mvalla commented Nov 24, 2023

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
Copy link

splatch commented Nov 24, 2023

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
Copy link
Author

wborn commented Nov 24, 2023

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 openhab/openhab-core#3632. If it is not possible or difficult to use purejavacomm as a replacement, feel free to add an implementation using jSerialComm.

@wborn
Copy link
Author

wborn commented Nov 24, 2023

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 calimero-project/calimero-rxtx@1e74d81 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
Copy link
Owner

mvalla commented Nov 24, 2023

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
Copy link
Owner

mvalla commented Nov 24, 2023

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
Copy link
Author

wborn commented Nov 25, 2023

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
Copy link
Owner

mvalla commented Nov 25, 2023

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
Copy link
Author

wborn commented Nov 25, 2023

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
Copy link
Owner

mvalla commented Nov 25, 2023

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
Copy link

splatch commented Nov 25, 2023

@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
Copy link
Owner

mvalla commented Dec 2, 2023

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
Copy link
Author

wborn commented Dec 3, 2023

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! 🙂

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 (openhab/openhab-addons#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 (openhab/openhab-addons#7573). If you use a different serial transport implementation it should be used by all add-ons.

@splatch
Copy link

splatch commented Dec 3, 2023

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
Copy link
Owner

mvalla commented Dec 3, 2023

Updated plan:

  • 1. make serial implementation pluggable via SPI, similar to OH org.openhab.core.io.transport.serial.SerialPortProvider, to ease integration with the existing OH serial transport
  • 2. write an implementation of openwebnet4j SPI using NRJavaSerial
  • 3. write an implementation of openwebnet4j SPI using jSerialComm
  • 4. change the openwebnet binding to use OH Serial Transport and pass it to openwebnet4j
  • 5. test openwebnet binding to use OH serial transport implemented with NRJavaSerial
  • 6. (optional) write an new implementation for OH Serial Transport using jSerialCom: new bundle org.openhab.core.io.transport.serial.jserialcomm [new issue]

@mvalla
Copy link
Owner

mvalla commented Jan 7, 2024

@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
Copy link
Author

wborn commented Jan 9, 2024

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
Copy link
Owner

mvalla commented Jan 22, 2024

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:

  • the mechanism seems to work on 4.2.0.SNAPSHOT in my Eclipse Dev env
    • I tested it also after stopping/restartig the binding
  • however is very "unstable": if I close Eclipse or re-build the binding and/or the openwebnet4j lib, then hit Resolve and run again it's likely that the SP it's not found anymore. I could not find a repeatable pattern
  • I tested the mechanism on my OH 4.1.1 production env, and the Service Provider can never be found

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
Copy link
Author

wborn commented Jan 22, 2024

Nice progress! 🙂

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
Copy link
Owner

mvalla commented Jan 23, 2024

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
Copy link

splatch commented Jan 23, 2024

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
Copy link
Owner

mvalla commented Jan 23, 2024

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:

  • 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!!
  • 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?
    However the way the lib is structured is quite layered : connection to openwebnet gateway can be opened the same way being it a TCP or serial connection, so I am not sure how easy I can write the bypass method specialising it for serial connectivity.
    But if you can point me to some example code to show me what you suggest would be very helpful.

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
Copy link

splatch commented Jan 24, 2024

  • 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
Copy link
Owner

mvalla commented Jan 25, 2024

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 mvalla closed this as completed in bc7fe26 Feb 5, 2024
mvalla added a commit that referenced this issue Feb 5, 2024
@mvalla
Copy link
Owner

mvalla commented Feb 5, 2024

@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:

  • 1. make serial implementation pluggable via SPI, similar to OH org.openhab.core.io.transport.serial.SerialPortProvider, to ease integration with the existing OH serial transport
  • 2. write an default implementation for openwebnet4j SPI using RxTx
  • 3. write an implementation for openwebnet4j SPI using jSerialComm (not merged, used for testing)
  • 4. change the openwebnet binding to use OH Serial Transport and pass it to openwebnet4j (via injection)
  • 5. test openwebnet binding to use OH Serial Transport
  • 6. write an new implementation for OH Serial Transport using jSerialCom: new bundle org.openhab.core.io.transport.serial.jserialcomm [new WIP coming soon]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants