Refactor each dongle into separate bundle

[openhab]

Refactor each dongle into separate bundle

[kaikreuzer]

Sounds indeed very much like the same requirement as we had for BLE.
Please do not go for fragments - the very same can be nicely done with proper bundles.

I had done all this refactoring in the bluetooth PR, so please have a look at eclipse-archived/smarthome#4535.

The only issue at the moment is that the "device" thing types currently need a reference to all existing dongle-bridge-ids in their XML (like here), while it would be nicer to simply say you require "some zigbee bridge". But this is imho ok for now and once the framework supports this, it will be a very simply change in the bindings.

You might also want to think about the "north side" of the binding as well: While in theory your "main" bundle can support any Zigbee device by reading its clusters and supporting them, I can imagine (from what I have learned about Zigbee in practice) that you might require special "hacks" for certain devices or also implementations for proprietary clusters of some devices. So it probably makes sense to go the same way as the Bluetooth binding here as well: The "base" bundle supports everything straight-forward, but it allows additional bundles to bring specific handlers and thing type definitions for special devices.

Let me know if you have any questions about this approach (that are not answered by the code in the Bluetooth PR)!

[cdjackson]

@kaikreuzer I've started to look at splitting the bundles and have hit an issue where ThingHandler.inititialize() is not being called.

Firstly, let me explain what I'm doing. The architecture is a little different to the Bluetooth implementation you did as I've implemented an abstract thing handler rather than an interface. The abstract handler is in the main ZigBee bundle, and then this is extended in each dongle bundle. The reason behind this is that 99% of the code is common - all the dongle handler needs to do is to initialise the dongle specific code.

From the log below, everything looks ok, and tracing through the code, it also looks ok until it hits the safe caller lambda where it should run the ThingHandler.inititialize() method, but this isn't happening.

Do you have any ideas why this would not get called? Is there anything wrong with the proposed architecture that might be causing this (or any comments in general for that matter ;) )?

I can push the refactored code if you like to look at it.

Thanks
Chris

2018-02-04 12:13:17.981 [DEBUG] [.c.thing.internal.ThingManager:366  ] - Thing 'zigbee:emberCoordinator:422e0a5e' is tracked by ThingManager.
2018-02-04 12:13:17.981 [DEBUG] [.c.thing.internal.ThingManager:936  ] - Not registering a handler at this point since no handler factory for thing 'zigbee:emberCoordinator:422e0a5e' found.
2018-02-04 12:13:17.982 [DEBUG] [.c.thing.internal.ThingManager:926  ] - Not registering a handler at this point. No handler factory for thing 'zigbee:emberCoordinator:422e0a5e' found.
2018-02-04 12:13:18.346 [DEBUG] [.c.thing.internal.ThingManager:867  ] - Thing handler factory 'EmberHandlerFactory' added
2018-02-04 12:13:18.351 [DEBUG] [.c.thing.internal.ThingManager:921  ] - Not registering a handler at this point. The thing types of bundle org.openhab.binding.zigbee.ember are not fully loaded yet.
2018-02-04 12:13:18.363 [DEBUG] [s.c.i.service.ReadyServiceImpl:54   ] - Added ready marker esh.xmlBindingInfo=org.openhab.binding.zigbee.ember
2018-02-04 12:13:18.364 [DEBUG] [c.x.o.XmlDocumentBundleTracker:347  ] - Reading the XML document '/ESH-INF/thing/ember_coordinator.xml' in module 'org.openhab.binding.zigbee.ember'...
2018-02-04 12:13:18.369 [DEBUG] [.c.thing.internal.ThingManager:496  ] - Calling 'EmberHandlerFactory.registerHandler()' for thing 'zigbee:emberCoordinator:422e0a5e'.
2018-02-04 12:13:18.379 [DEBUG] [o.b.z.d.ZigBeeDiscoveryService:60   ] - Creating ZigBee discovery service for zigbee:emberCoordinator:422e0a5e
2018-02-04 12:13:18.379 [DEBUG] [o.b.z.d.ZigBeeDiscoveryService:64   ] - Activating ZigBee discovery service for zigbee:emberCoordinator:422e0a5e
2018-02-04 12:13:18.408 [DEBUG] [.c.thing.internal.ThingManager:597  ] - Configuration of thing zigbee:emberCoordinator:422e0a5e needs [port, baud], has [baud, panid, extendedPanid, port, channel, initialise, networkKey].
2018-02-04 12:13:18.411 [DEBUG] [.c.thing.internal.ThingManager:624  ] - Calling initialize handler for thing 'zigbee:emberCoordinator:422e0a5e' at 'org.openhab.binding.zigbee.ember.handler.EmberHandler@6a6049fc'.
2018-02-04 12:13:20.035 [DEBUG] [e.s.m.t.i.GenericThingProvider:841  ] - ThingHandlerFactory added org.openhab.binding.zigbee.ember.internal.EmberHandlerFactory@17305a3f

[kaikreuzer]

@cdjackson I'll try to look into this tomorrow and check what's going wrong there.

[kaikreuzer]

To be honest, I have no clue.
As your log shows the line

2018-02-04 12:13:18.411 [DEBUG] [.c.thing.internal.ThingManager:624  ] - Calling initialize handler for thing 'zigbee:emberCoordinator:422e0a5e' at 'org.openhab.binding.zigbee.ember.handler.EmberHandler@6a6049fc'.

everything seems to be in place and the initialize() method should definitely be called.
This reminds me a bit of this report, where we also expect a call through the SafeCaller that for some reason never happens... @SJKA, Could there be any relation? If so, @cdjackson's situation might provide us with a reproducible situation that could help in the analysis.

@cdjackson It should help in any case if you could provide a link to your code.

[cdjackson]

Thanks Kai,
The code is in the split_bundles branch (please excuse the mess for now ;) ).

Chris

[sjsf]

@cdjackson if you can reproduce it - could you please do the same with TRACE logging enabled for org.eclipse.smarthome.core.internal.common.SafeCallManagerImpl?

[cdjackson]

It is 100% reproducible so I’ll get some logs.

…

On 9 Feb 2018, at 11:41, Simon Kaufmann ***@***.***> wrote: @cdjackson <https://github.com/cdjackson> if you can reproduce it - could you please do the same with TRACE logging enabled for org.eclipse.smarthome.core.internal.common.SafeCallManagerImpl? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_kQ7r4RofbGlqQ978mc7KZGZgFlS9gks5tTC7igaJpZM4QzYtB>.

[sjsf]

stop, nevermind, just found your branch and did it myself

[sjsf]

I ran into a classloader issue:

12:49:41.655 DEBUG o.e.s.c.t.i.ThingManager[:597] - Configuration of thing zigbee:emberCoordinator:422e0a5e needs [port, baud], has [baud, panid, extendedPanid, port, channel, initialise, networkKey].
12:49:41.655 DEBUG o.e.s.c.t.i.ThingManager[:624] - Calling initialize handler for thing 'zigbee:emberCoordinator:422e0a5e' at 'org.openhab.binding.zigbee.ember.handler.EmberHandler@237f8ae'.
12:49:41.655 INFO  s.event.ThingStatusInfoEvent[:51] - 'zigbee:emberCoordinator:422e0a5e' updated: INITIALIZING
12:49:41.658 INFO  s.e.ThingStatusInfoChangedEvent[:51] - 'zigbee:emberCoordinator:422e0a5e' changed from UNINITIALIZED to INITIALIZING
12:49:41.717 ERROR o.e.s.c.t.i.ThingRegistryImpl[:228] - Could not inform the ThingTracker 'org.eclipse.smarthome.core.thing.internal.ThingManager@3c2e361f' about the 'THING_ADDED' event!
java.lang.IllegalArgumentException: interface com.zsmartsystems.zigbee.core.ZigBeeNetworkNodeListener is not visible from class loader
	at java.lang.reflect.Proxy$ProxyClassFactory.apply(Proxy.java:581)
	at java.lang.reflect.Proxy$ProxyClassFactory.apply(Proxy.java:557)
	at java.lang.reflect.WeakCache$Factory.get(WeakCache.java:230)
	at java.lang.reflect.WeakCache.get(WeakCache.java:127)
	at java.lang.reflect.Proxy.getProxyClass0(Proxy.java:419)
	at java.lang.reflect.Proxy.newProxyInstance(Proxy.java:719)
	at org.eclipse.smarthome.core.internal.common.SafeCallerBuilderImpl.build(SafeCallerBuilderImpl.java:65)
	at org.eclipse.smarthome.core.thing.internal.ThingManager.doInitializeHandler(ThingManager.java:635)
	at org.eclipse.smarthome.core.thing.internal.ThingManager.initializeHandler(ThingManager.java:562)
	at org.eclipse.smarthome.core.thing.internal.ThingManager.registerAndInitializeHandler(ThingManager.java:919)
	at org.eclipse.smarthome.core.thing.internal.ThingManager.thingAdded(ThingManager.java:369)

That's definitely a reason why initialize() isn't called and clearly a bug in the ThingManager. I'll fix is immediately in ESH.

[sjsf]

And now another one (but this time it's on your side):

java.lang.LinkageError: loader constraint violation: loader (instance of org/eclipse/osgi/internal/loader/EquinoxClassLoader) previously initiated loading for a different type with name "com/zsmartsystems/zigbee/transport/ZigBeeTransportTransmit"
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.defineClass(ModuleClassLoader.java:276)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.defineClass(ClasspathManager.java:655)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findClassImpl(ClasspathManager.java:578)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClassImpl(ClasspathManager.java:538)
	at org.eclipse.osgi.internal.loader.classpath.ClasspathManager.findLocalClass(ClasspathManager.java:525)
	at org.eclipse.osgi.internal.loader.ModuleClassLoader.findLocalClass(ModuleClassLoader.java:328)
	at org.eclipse.osgi.internal.loader.BundleLoader.findLocalClass(BundleLoader.java:368)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:446)
	at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:395)

I think this is because you have the zigbee library included independently in both bundles. But I reckon this is due to the work-in-progress anyway.

[sjsf]

I have created eclipse-archived/smarthome#5063. With that, ZigBeeCoordinatorHandler.startZigBee(...) gets called and the thing dutifully switches to UNKNOWN (I don't have a stick plugged in).

Does this fix the problem you described in #47 (comment) for you too? Or do you face another issue?

[cdjackson]

Thanks. Yes - I thought I might get this ;) Once your PR is merged in ESH I’ll move onto the next issue - thanks @SJKA.

[cdjackson]

So this fundamentally works now. (thanks @SJKA).

@kaikreuzer before I go too much further I'd like to understand how to deploy this. Currently I'm testing by just adding the JARs into the TP (they have OGSi manifests), but I assume we can define features something along the following lines?

  <feature name="openhab-binding-zigbee" version="${project.version}">
    <feature>openhab-runtime-base</feature>
    <bundle start-level="80>mvn:com.zsmartsystems.zigbee/com.zsmartsystems.zigbee/${zigbee.version}</bundle>
    <bundle start-level="80">mvn:org.openhab.binding/org.openhab.binding.zigbee/${project.version}</bundle>
  </feature>

  <feature name=“ openhab-binding-zigbee-ember" version="${project.version}">
    <feature>openhab-transport-serial</feature>
    <feature>mvn:org.openhab.binding/org.openhab.binding.zigbee/${project.version}</feature>
    <bundle start-level="80">mvn:org.openhab.binding/org.openhab.binding.zigbee.ember/${project.version}</bundle>
    <bundle start-level="80">mvn:com.zsmartsystems.zigbee/com.zsmartsystems.zigbee.dongle.ember/${zigbee.version}</bundle>
  </feature>

Does this then end up with 2 features visible and available for download in PaperUI or is there another definition of features available to users somewhere? I'd prefer to only give the user the option for installing the "end" bundles (ie not the org.openhab.binding.zigbee binding independently as that's likely to cause confusion I think).

Also, are you happy with the pom hierarchy? I've got a parent pom for the repository separate poms per binding (it might need some further tuning, but I'm mostly interested in ensuring I have the structure/concepts right before I take the final leap and refactor everything against the current codebase).

Code is updated in the same branch as above. I've included the JARs in the repo for now which are added into the TP.

Thanks.

[kaikreuzer]

I don't have a final solution for this either.
For the moment, I have decided to put it in a flat hierarchy side by side and simply keep a single feature that includes all that is relevant for a solution, so users don't have to bother at that point. I'd say that the same makes sense for Zigbee as well.

[cdjackson]

Closed by #159