Factories to create SerialPort

[msteigenberger]

Added new factories to create a SerialPort instance. This is to reduce the dependencies on client side. E.g. when used the TelnetSerialPort, a dependency to org.apache.net.telnet was needed. Additionally it eases the client code. NRSerialPort also makes use of the factory. The factory decides which implementation to use by the port name.

[andyrozman]

If you made this, then you should have at least made it right. Instance of serial in NRSerialPort is of type RXTXPort and not of type SerialPort. Your change will break at least my requirements. My code needs that RXTXPort is returned. If you wanted to do this you would need to specify what kind of port you are expecting (class) and that instance should be returned. So in this case we would call this.serialPortFactory.createSerialPort(port, RXTXPort.class) and then RXTXPort instance would be returned.

[msteigenberger]

Thanks for you inputs. I'm not really sure what you mean. NRSerialPort previously used an concrete implementation of SerialPort (RXTXPort), which I think is not good. My code is breaking one method: NRSerialPort#getSerialPortInstance which is returning the Interface SerialPort now instead of RXTXPort. But except this method, the behaviour of the class should be the same as bevor.

What you are talking about, is the SerialPortFactory which should get the expecting concrete implementation class. I will change that because it's really reasonable. But this won't change anything in NRSerialPort.

[andyrozman]

You have broken the first and the most important rule of refactoring of library: Never change signature of method, if method was already released.

[msteigenberger]

Yeah, you're right. I wanted to discuss exactly this. I would make this method deprecated and add another method to retrieve the SerialPort interface. I'm commiting this proposal for discussion.

[msteigenberger]

Is someone interrested in this? I think it really ads good value. I would use it in openhab to have RFC2217 support easily. In many cases the RFC2217 support is added without any code changes in the client code, if it is using NRSerialPort. Clients wouldn't introduce new dependencies.

If the connection should be done with RFC2217, the NRSerialPort can be instantiated with an rfc2217:// URL as the port name.

[madhephaestus]

I'm not super excited about a down-cast for a return type on a published interface. You know that it is a SerialPort instance anyway and that information can be retrieved with introspection and casting? This factory can use whatever interfaces it defined since you wrote it, but NRSerialPort wrapper is published with the superclass RXTXPort and should maintain that. Try rolling back your changes to NRSerialPort, then correcting your code to deal with it using casting if you need the SerialPort interfaces.

Besides that, i like this idea, and like the API for calling up ports, thanks for the hard work!

[msteigenberger]

@madhephaestus thanks for your inputs. I think you got something wrong. If I want to use my factories, I need to change the NRSerialPort to deal with the interface SerialPort and not the concrete implementation RXTXPort because the SerialPort can also be a TelnetSerialPort implementation (and in future maybe others). I think it is also far better design to not depend on a concrete implementation. The down-cast is just needed for compatibility reasons because unfortunatly NRSerialPort was already published with the public method "getSerialPortInstance" which returns a RXTXPort. I'm also not very excited about that.

The default behaviour of NRSerialPort is unchanged. It just delegates to the factory which then does the same as NRSerialPort did before.

So, I think there are just 2 possibilities:

• Keep NRSerialPort dealing with ONLY RXTXPort (which I think is a bad design) and rolling back my changes
• Applying my changes with the deprecated NRSerialPort#getSerialPortInstance method and let NRSerialPort deal with ANY SerialPort instance (also RXTXPort)

[andyrozman]

@madhephaestus. @msteigenberger Actually there is a third solution... If you look at my first comment you will see it... Like I said we would need method that would create port of specific type, in our case: this.serialPortFactory.createSerialPort(port, RXTXPort.class)

If you have such method, you could revert most of your changes to NRSerialPort, just leave creation of port with your factory with this new method and nothing else will be need to be changed.

[msteigenberger]

@andyrozman But this means that NRSerialPort is restricted to RXTXPort, which leaves us with that poor design to have a dependency to a concrete implementation of SerialPort. It won't fulfill my requirement to let NRSerialPort deal with any SerialPort implementation.

[andyrozman]

@msteigenberger But we work with concrete implementation of SerialPort, in this case (RxtxPort). NRSerialPort shouldn't create TelnetSerialPort, because it's not intended for it. So generalizing in this case doesn't do us any good, usage of Factory would at least move part of this code into your class, but when we create instance of this class we need specific SerialPort implementation.

Perhaps this.serialPortFactory.createSerialPort(port, RXTXPort.class) should not have class as parameter, but enum, which we can then later extend, if we get other "SerialPort" implementations.

[msteigenberger]

Ok, that's an argument. Although I'd like to have it more generic. I will create a new PR with letting the NRSerialPort just deal with RXTXPort.

To be honest, I don't like the enum suggestion as this enum is defined in nrjavaserial library and can't be extended from 3rd party libraries.