Refactor each dongle into separate bundle #47
Comments
|
Sounds indeed very much like the same requirement as we had for BLE. 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)! |
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
|
@kaikreuzer I've started to look at splitting the bundles and have hit an issue where 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 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 |
|
@cdjackson I'll try to look into this tomorrow and check what's going wrong there. |
|
To be honest, I have no clue. everything seems to be in place and the @cdjackson It should help in any case if you could provide a link to your code. |
|
Thanks Kai, Chris |
|
@cdjackson if you can reproduce it - could you please do the same with TRACE logging enabled for |
|
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>.
|
|
stop, nevermind, just found your branch and did it myself |
|
I ran into a classloader issue: That's definitely a reason why |
|
And now another one (but this time it's on your side): 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. |
|
I have created eclipse-archived/smarthome#5063. With that, Does this fix the problem you described in #47 (comment) for you too? Or do you face another issue? |
|
Thanks. Yes - I thought I might get this ;)
Once your PR is merged in ESH I’ll move onto the next issue - thanks @SJKA.
|
|
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. |
|
I don't have a final solution for this either. |
|
Closed by #159 |
The binding needs to be split into separate packages to make it more manageable/extensible, and reduce installation footprint. There should be a single bundle containing the majority of the code (ZigBee stack, thing handler, converters...) and another bundle with just the dongle driver JAR, the bridge handler and XML.
@kaikreuzer your thoughts would be very welcome with the following...
My initial thinking was to use fragments for the dongle bundles - this should solve getting all the bits in one place, the XML file should be read fine. The HandlerFactory could have a method to directly register a new handler (eg
registerBridge(BridgeTypeUID, BridgeHandler))... Given the discovery service is instantiated from the factory, we can pass in the same list of bridge UIDs...I think this ought to work ok and hopefully you won't tell me it violates too many OSGi principals ;)
This is a similar issue to the discussion we had a few months back for BLE and I think you said you were looking at other possible extensions to the framework to support this sort of structure (??) so any further suggestions welcome.
I'm not in a huge hurry to do this, but when you find an elusive "few minutes" I'd welcome your thoughts...
The text was updated successfully, but these errors were encountered: