Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One device only #32

Closed
wants to merge 6 commits into from
Closed

One device only #32

wants to merge 6 commits into from

Conversation

rospogrigio
Copy link
Owner

Now this is THE big change, and it needs thorough testing and fixing for sure.
Plus, it is incomplete and @postlund I need your help because my brain hurts but still I could not get Options schemas default. I had to comment out a couple of lines in _convert_entity , please help me with this because the whole Schema thing is still a bit awkward to me. (I had to hardcode the options params for my cover to work).
Oh, and I also had to comment a couple of references of the update_listener because it made this fail to start so it has to be restored as well, sorry about this (did not understand what's going wrong and my head aches very badly 😆 ).
Plus, it should be adapted to config_flow (not tested).

One thing to be noted is that now the pytuya class is called TuyaInterface, while TuyaCache has been moved to init and renamed as TuyaDevice. Next step will be for TuyaDevice to introduce persistent connection.

Here is the new YAML format (very sleek!):

localtuya:
  - host: 192.168.1.x
    device_id: xxxxx
    local_key: xxxxx
    friendly_name: Plug Stereo Wifi
    protocol_version: 3.3
    entities:
      - platform: switch
        friendly_name: Plug Stereo
        id: 1
      - platform: switch
        friendly_name: Plug Stereo USB
        id: 7
      - platform: sensor
        friendly_name: Plug Voltage
        id: 20
        scaling: 0.1
        unit_of_measurement: 'V'

  - host: 192.168.1.x
    device_id: xxxxx
    local_key: xxxxx
    friendly_name: Tapparelle studio
    protocol_version: 3.3
    entities:
      - platform: cover
        friendly_name: Tapparelle studio Cover
        id: 1
      - platform: switch
        friendly_name: Tapparelle studio Backlight
        id: 101

@postlund please help fixing this, I don't know whether it's better to merge this for now so you can work on a PR of your own, please advice, thank you!

@postlund
Copy link
Collaborator

Would it be possible for you to extract the unrelated stuff, I.e. everything related to TuyaCache and the name change to separate PR? It has nothing to do with the "one-device-config" part and makes it hard to review. Again, small PRs are a lot easier to review.

I just glanced quickly and one thing I noted is that the import part is not done right. The import_from_yaml method only supports one platform type currently and also creates a config entry for every platform that is set up. The way you do it now, you forward the setup of each entity in the YAML config to each platform (async_setup), which in the end means you are importing each platform independently and also create one config entry for each platform. So if you specify two entities with different platforms, this will results in two config entries. Since they have the same unique id, they will be merged (maybe?) somehow. But this is not the way to do it. We should remove the async_setup method from all platforms and just call import_from_yaml in __init__.py (we could possibly in-line it) and make some minor adjustments to the step_import to deal with different platform for each entity. That should almost be it.

@rospogrigio
Copy link
Owner Author

Would it be possible for you to extract the unrelated stuff, I.e. everything related to TuyaCache and the name change to separate PR? It has nothing to do with the "one-device-config" part and makes it hard to review. Again, small PRs are a lot easier to review.

Well I thought a lot how to do it but in my head it's quite not feasible, because moving TuyaCache goes together with the config file refactoring (at least, in my head). I could try but I'd need more time.

[...] Since they have the same unique id, they will be merged (maybe?) somehow. But this is not the way to do it.

This cannot happen, since each entity has local_deviceId**_entityDpsId** as unique ID, so different entities always have different unique IDs.

@postlund
Copy link
Collaborator

postlund commented Sep 21, 2020

Would it be possible for you to extract the unrelated stuff, I.e. everything related to TuyaCache and the name change to separate PR? It has nothing to do with the "one-device-config" part and makes it hard to review. Again, small PRs are a lot easier to review.

Well I thought a lot how to do it but in my head it's quite not feasible, because moving TuyaCache goes together with the config file refactoring (at least, in my head). I could try but I'd need more time.

Just extracting the class from each platform to __init__.py (as this class should be the same everywhere, otherwise we should make them the same) and updating the references should be fairly simple. It doesn't change anything, just moves code around so it should be trivial and not collide with anything else. This is a good refactoring step as it creates large chunks of code changes, but it's easy to review as it's trivial changes.

[...] Since they have the same unique id, they will be merged (maybe?) somehow. But this is not the way to do it.

This cannot happen, since each entity has local_deviceId**_entityDpsId** as unique ID, so different entities always have different unique IDs.

I believe I wasn't clear enough here. I was referring to the unique id of the config entry itself, not the entities defined within the config entry. EDIT: This id is of course the same as the device id.

@rospogrigio
Copy link
Owner Author

Just extracting the class from each platform to __init__.py (as this class should be the same everywhere, otherwise we should make them the same) and updating the references should be fairly simple. It doesn't change anything, just moves code around so it should be trivial and not collide with anything else. This is a good refactoring step as it creates large chunks of code changes, but it's easy to review as it's trivial changes.

OK then, let me try to split this PR in two parts so that we can figure out how to fix it.

@postlund
Copy link
Collaborator

Sounds great! I have prepared a commit to introduce supports for reloading config from YAML without restarting home assistant, but I need this PR to be finished before I can finish that though.

@rospogrigio
Copy link
Owner Author

OK @postlund , I split this PR into #38, which I have already merged since -as you correctly said- it was only a matter of moving code here and there and everything remained functional (fan and light still untested, though).
Now the rest of the code is here, can you review it and help me out?
Thank you

@postlund
Copy link
Collaborator

I will have a look! Can you rebase this on top of master so that all of those changes disappear? Otherwise we don't gain anything from the extraction.

@rospogrigio
Copy link
Owner Author

Sure, how should I do that? Don't want to mess things around this time... should I just launch git merge master or use a different command?
Let me know, thanks!

@postlund
Copy link
Collaborator

Avoid merges first hand, you could just try git rebase master and fix conflicts along the way.

@rospogrigio
Copy link
Owner Author

Closing this PR, which has been split into #38 and #40.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants