Topics

[Proposal] Allow factory methods in ConnectionTypeList


Svata Dedic
 

Hi,

As part of the implementation, I've prototyped the following improvement. Before finalizing (cleaning up) the implementation, I'd like to confirm that the idea is acceptable - if so, then a regular PR review will be done on cleaned-up code.


Background:
----------
I am playing with an alternative implementation of XPressNet support. I have Nano-X (home) and DR-5000; Nano-X (supported by JMRI) does not work properly, DR-5000 (not listed ! as supported for XpressNet at the moment) also has some issues.

I write necessary adjustments in new types of connection. Part of the fixes supposedly apply to Lenz LZV100; so some experimental connections for that type as well (the fixes are not easy to backport - and possibly not desirable either, for stability reasons).

State:
------
NanoX and DR5k basically use the same settings for serial or network communication *as already implemented* for LI100, LIUSB or LIUSBEthernet; they also use the same message encapsulation. That makes:

Nano-X: GEN-LI (both Paco Canada's design)
DR-5000: Li-USB, Li-USB-Ethernet
LZV100 alternative: LI100, LI-USB, LI-USB-Ethernet

I don't know who would run NanoX one with Lenz LI100 or LI-USB, but it could be possible ;) So let's have 6 ConnectionConfig classes.


Issue:
------
In the current nomenclature, following example of LI100, LI101, LI100F and others one has to create ConnectionConfig subclass to:
* create Adapter class
* interface with user for configuration
With each ConnectionConfig, a (de)serializer class must exist. In my specific case, no options are added. The only relevant behavioral change of ConnectionConfig is the adapter subclass creation.

From the configuration perspective, despite 6 ConnectionConfigs, I have just two protocol families:
* serial/USB (1st superclass)
* network TCP/IP (2nd superclass)

That's 12 classes, essentially empty (all work done by superclasses), except for:
- label,
- factory method

But to support UI configuration + adapter creation, I need "just" need two parametrized classes: each inherits the relevant UI implementations, parameter drives the appropriate Adapter creation.

Proposal:
==========
Instead of requring several n*2 (empty) classes, to just encode a single constant, and a single creation, I'd like to propose to **support static factory methods** in ConnectionTypeList.getAvailableProtocolClasses.

If used, those 12 classes will reduce to 4; There would not have to be 6 ConnectionConfig classes, but just 2 of them.

The current syntax for Strings returned from getAvailableProtocolClasses is "fully qualified java class name". I propose to change it to:

item := className | className ':' methodName
className := javaIdentifier | javaIdentifier '.' javaIdentifier
methodName := javaIdentifier

So the line could read:
jmri.jmrix.lenzplus.config.LenzSerialConnectionConfig:liusbDR5000

The change should be backwards compatible (changed implementation will read old data), but older implementation will refuse to load new connection configs that use this feature.

Prototypical implementation is here: https://github.com/svatoun/JMRI/blob/prototype/xpressnet-sequence-fix3/java/src/jmri/jmrix/JmrixConfigPane.java#L285

Serial-style adapter, that works for all serial AND usb flavours (= 5 ConnectionTypes) is at https://github.com/svatoun/JMRI/blob/prototype/xpressnet-sequence-fix3/java/src/jmri/jmrix/lenzplus/config/LenzSerialConnectionConfig.java

Persistence is done as usual, but with an additional attribute that creates the appropriate Adapter:
https://github.com/svatoun/JMRI/blob/prototype/xpressnet-sequence-fix3/java/src/jmri/jmrix/lenzplus/config/configurexml/LenzSerialConnectionConfigXml.java

---------

Of course I can make a copy-paste of the ConnectionConfig + serializer into 12 classes. But somehow I feel there's already more than enough code duplications in the codebase.

Questions:
- is such an extension acceptable ?
- is the proposed syntax acceptable ?
- are there other parts of code that interpret ConnectionTypeList data I am not aware of (= may need an update) ?

Waiting for feedback,
-Svata