smarthomej / addons

SmartHome/J addons for openHAB
Eclipse Public License 2.0
59 stars 23 forks source link

[tuya] Add support for gateway and sub devices #497

Closed presidentio closed 7 months ago

presidentio commented 1 year ago

This PR resolves #483 and #493

First of all, @J-N-K thank you for a great binding. It works well, and UX is great. Everything is discovered automatically.
Most of my Tuya things are ZigBee based and communicate through the gateway. In this PR I added support for them

End-user changes:

I had to refactor/extract code from existing classes to avoid code duplication. I tried to do not change the existing logic.

I've tested it with read-only devices: water leak sensor, motion sensor, door/window opening sensor

Please, let me know if you have any concerns or suggestions, and I will try to resolve them

presidentio commented 1 year ago

@J-N-K, will you have time to take a look?

J-N-K commented 1 year ago

I hope I'll find time at the weekend.

presidentio commented 1 year ago

Thanks for the review! As far as I can see the TuyaGatewayHandler does nothing, except distributing the device status messages to the child handlers and passing the disconnected messages. Is that really needed or could we integrate that in the TuyaDevice and thing handler itself? -> Unfortunately no. I tried what you suggested, but most of my devices including the gateway have a limit for the number of connections(1-2 at maximum). Due to that, I can't create more than one TuyaDevice for different sub-devices. So, I decided that we need to represent gateway as a bridge in the Openhab and it is responsible only for the communication. TuyaSubDeviceHandler is responsible for channels. Why do we need the TuyaDeviceManager? -> TuyaGatewayHandler has the same flow of creating and managing TuyaDevice, so I decided to extract everything about TuyaDevice management into a separate class to do not duplicate the code

presidentio commented 1 year ago

@J-N-K Please, review my answers

J-N-K commented 12 months ago

Thanks, I'll try to have a look this weekend.

J-N-K commented 12 months ago

Sorry again for letting you wait so long for a good review. I have thought a lot about the design and there are several things that can't (or shouldn't) be done the way it is implemented now. One thing is that ThingHandlers should never deal with the thing registry.

Instead of the bridge/thing design I would like to propose a different approach that also solves the issue with the multiple connections.

We create a service called TuyaDeviceManager which is passed from the ThingHandlerFactory to the things handler which then requests a TuyaDevice from there (instead of calling new TuyaDevice):

    tuyaDeviceManager.getTuyaDevice(....);

The manager checks if the already have a TuyaDevice for that gateway id and then returns the already existing one, otherwise it creates a new one. When a thing handler is disposed, it unregisters itself from the factory (we can also call it the TuyaDeviceManager).

The gateway id is the same as the device id for ordinary devices, and the id of the gateway for sub devices. It can be set during discovery (as a new config parameter). If it is not set, it is considered to be the same as the device id, that makes it backward compatible.

The TuyaDiscoveryService can then just check if it is a gateway device and call the new listSubDevices method and add the sub devices to the inbox (with all required parameters).

This should all be possible with about 2050 lines of code. The only addition needed then is in TuyaDevice, which needs to be enhanced to keep a map of listeners (instead of a single one), which is managed by the TuyaDeviceManager (via a add/remove methods). The key is the device. Depending on the device id the TuyaDevice can then select the correct listener for the responses (or notify all listener if the device goes offline).

WDYT?

presidentio commented 11 months ago

@J-N-K Thanks for the deep review. From your answer, I see two concerns: 1) ThingHandler shouldn't deal with ThingRegistry. Perhaps you are right. For me, it was acceptable as ThingHandler in fact doesn't call ThingRegistry, instead it passes it to DiscoveryService which is responsible for managing things. Your solution of registering the bridge along with sub-devices will work, but we will lose sub-devices discovery for a created bridge. I'm fine changing this aspect 2) Introducing bridge. Your solution will work, but I envision several types of issues: concurrency issues while managing TuyaDevice and duplicated configuration across multiple devices(gateway IP will be specified in each sub-device). From my experience with Openhab and documentation, it's recommended to represent a physical device with a bridge if it's responsible for communication with other devices. So, I'd like to keep the existing implementation as it agrees with the recommendations and has a simpler design

presidentio commented 11 months ago

Any updates here?

edsonaj commented 9 months ago

Hi, has this been solved in any way?

presidentio commented 9 months ago

@edsonaj It's fully functional in this branch, I have used it for several months

edsonaj commented 9 months ago

@presidentio but your branch is for 3.2, right? not 4.0

presidentio commented 9 months ago

I have a branch for 4.x https://github.com/presidentio/addons/tree/4.0.x-tuya-gateway-support, but didn't create a PR as this one doesn't have progress

stale[bot] commented 7 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

m-jarzebowski commented 4 months ago

Any update on that feature? What is missing for the merge?

Ilia-SB commented 3 months ago

I would also like to ask if this has been addressed. This binding is great, but I really miss subdevices support.