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

restructure and improve gateway subdevices #700

Merged
merged 35 commits into from
May 27, 2020

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented May 20, 2020

Restructure and improve gateway subdevices

  • add GatewayException
  • add SubDeviceInfo class
  • make GatewayAlarm, GatewayRadio, GatewayZigbee and GatewayLight cli compatible
  • add 'devices' property that returns already discoverd devices
  • Implement "discover_devices" such that it also initilizes subclasses containing the devices, these subclasses are returned as a list by the "discover_devices" command and can be retrieved again from the 'devices' property
  • improve and use general getter/setter functions for both the gateway itself and the subdevices
  • structure device specific subclasses
  • add GatewayDevice class for consistant initialization of gateway devices (such as GatewayAlarm, GatewayRadio, GatewayZigbee and GatewayLight)
  • expand SubDevice baseclass with: props class, status property, device_type property, update method, send method, send_arg method, get_property method, get_property_exp method and set_property method
  • implement SubDevice Specif classes: AqaraHT, AqaraMagnet, AqaraPlug and AqaraRelayTwoChannels

@coveralls
Copy link

coveralls commented May 20, 2020

Coverage Status

Coverage decreased (-0.7%) to 73.44% when pulling 0f05432 on starkillerOG:patch-3 into fe031c5 on rytilahti:master.

@bskaplou
Copy link
Contributor

@starkillerOG
;(

$ miiocli gatewayzigbee --ip 192.168.2.109 --token XXXXX get_zigbee_channel
Traceback (most recent call last):
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/bin/miiocli", line 11, in <module>
    load_entry_point('python-miio', 'console_scripts', 'miiocli')()
  File "/Users/bskaplou/github/sk/python-miio/miio/cli.py", line 44, in create_cli
    return cli(auto_envvar_prefix="MIIO")
  File "/Users/bskaplou/github/sk/python-miio/miio/click_common.py", line 59, in __call__
    return self.main(*args, **kwargs)
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/core.py", line 1256, in invoke
    Command.invoke(self, ctx)
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/bskaplou/Library/Caches/pypoetry/virtualenvs/python-miio-NRKm9xRU-py3.7/lib/python3.7/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/bskaplou/github/sk/python-miio/miio/click_common.py", line 242, in group_callback
    ctx.obj = self.device_class(*args, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'ip'

Does it work with CLI?

@bskaplou
Copy link
Contributor

bskaplou commented May 20, 2020

Too dirty but works

$ git diff && miiocli gatewayzigbee --ip 192.168.2.109 --token XXXXX get_zigbee_channel 
diff --git a/miio/gateway.py b/miio/gateway.py
index 8f024a4..27dc9bd 100644
--- a/miio/gateway.py
+++ b/miio/gateway.py
@@ -269,6 +269,17 @@ class GatewayZigbee(Device):
     def __init__(self, parent) -> None:
         self._device = parent

+    def __init__(
+        self,
+        ip: str = None,
+        token: str = None,
+        start_id: int = 0,
+        debug: int = 0,
+        lazy_discover: bool = True,
+    ) -> None:
+        self._device = Device(ip, token, start_id, debug, lazy_discover)
+
+
     @command()
     def get_zigbee_version(self):
         """timeouts on device"""
Running command get_zigbee_channel
15

@syssi
Copy link
Collaborator

syssi commented May 20, 2020

Same applies to all classes using the @command decorator.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR with many great improvements and cleanups! There are some issues that needs to be solved before proceeding, see the first round of comments inline.

miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Show resolved Hide resolved
miio/gateway.py Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Show resolved Hide resolved
@starkillerOG
Copy link
Contributor Author

@bskaplou I only work with python3, I know almost nothing about the cli implementation, so honestly I have no idea if it works with cli, probably not yet...

@starkillerOG
Copy link
Contributor Author

@bskaplou could you test using python3?

In python3 this schould work:

from miio import (gateway)
gateway = gateway.Gateway("192.168.1.IP", 'token')
devices = gateway.discover_devices()

Now select the sub device you want to control, for instance the first discovered device:

Aqara1 = devices[0]
Aqara1.update()
print(Aqara1.temperature)

@bskaplou
Copy link
Contributor

@starkillerOG Yep it works... But CLI is important because it allows to play with lib and it's well documented...

Right now CLI doesn't work because click is trying to create object with ip/token/etc passed to init

you can resurrect CLI by adding following method to each subdevice

def __init__(
     self,
     ip: str = None,
     token: str = None,
     start_id: int = 0,
     debug: int = 0,
     lazy_discover: bool = True,
) -> None:
     self._device = Device(ip, token, start_id, debug, lazy_discover)

Click will call this init with cli arguments and everything works well

@starkillerOG
Copy link
Contributor Author

Too dirty but works

$ git diff && miiocli gatewayzigbee --ip 192.168.2.109 --token XXXXX get_zigbee_channel 
diff --git a/miio/gateway.py b/miio/gateway.py
index 8f024a4..27dc9bd 100644
--- a/miio/gateway.py
+++ b/miio/gateway.py
@@ -269,6 +269,17 @@ class GatewayZigbee(Device):
     def __init__(self, parent) -> None:
         self._device = parent

+    def __init__(
+        self,
+        ip: str = None,
+        token: str = None,
+        start_id: int = 0,
+        debug: int = 0,
+        lazy_discover: bool = True,
+    ) -> None:
+        self._device = Device(ip, token, start_id, debug, lazy_discover)
+
+
     @command()
     def get_zigbee_version(self):
         """timeouts on device"""
Running command get_zigbee_channel
15

This will probably work, but if I understand correctly this will create a new device including connection, handshake etc, for each feature of the Gateway + for each subdevice. That seems to be very dirty and will probably give us lots of trouble in the future.

I know almost nothing about the cli and don't use it.....

@bskaplou could you try to figure out a way that works with the cli but only uses one instance of the device class that is beeing re-used to only have one connection and handshake etc?

miio/gateway.py Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
starkillerOG and others added 2 commits May 23, 2020 18:30
Co-authored-by: Teemu R. <tpr@iki.fi>
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
miio/gateway.py Outdated Show resolved Hide resolved
@rytilahti
Copy link
Owner

Btw, if you are developing locally, the simplest way to make sure the checks are passing is to execute tox -e lint and fix the issues before pushing in the changes!

@starkillerOG
Copy link
Contributor Author

@rytilahti thanks for the tox -e lint hint, however I get a gazillion errors mostly about the doc files ending with .rst
How do you use tox to only check one specific file, in this case gateway.py?

@rytilahti
Copy link
Owner

Can you paste an error or two? But looks like it's now passing, the only problem is that python3.6 does not support dataclasses, it seems... Please add dataclasses to the required packages (either by manually modifying pyproject.toml, or simply commanding poetry add dataclasses) and making a commit.

I think we are bound to have that until homeassistant drops python 3.6 support..
I think it's maybe time to drop 3.6 support in the next release.

@starkillerOG
Copy link
Contributor Author

@rytilahti do you know how to fix the tests failing?
somthing about the "dataclasses" module not beeing found???

@starkillerOG
Copy link
Contributor Author

_______________ ERROR collecting miio/tests/test_wifirepeater.py _______________
ImportError while importing test module '/Users/runner/runners/2.169.0/work/1/s/miio/tests/test_wifirepeater.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
miio/__init__.py:25: in <module>
    from miio.gateway import Gateway
miio/gateway.py:4: in <module>
    from dataclasses import asdict as dataclasses_asdict
E   ModuleNotFoundError: No module named 'dataclasses'
_________________ ERROR collecting miio/tests/test_yeelight.py _________________
ImportError while importing test module '/Users/runner/runners/2.169.0/work/1/s/miio/tests/test_yeelight.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
miio/__init__.py:25: in <module>
    from miio.gateway import Gateway
miio/gateway.py:4: in <module>
    from dataclasses import asdict as dataclasses_asdict
E   ModuleNotFoundError: No module named 'dataclasses'
=============================== warnings summary ===============================
/Users/runner/Library/Caches/pypoetry/virtualenvs/python-miio-5D3ggvah-py3.6/lib/python3.6/site-packages/construct/core.py:3
  /Users/runner/Library/Caches/pypoetry/virtualenvs/python-miio-5D3ggvah-py3.6/lib/python3.6/site-packages/construct/core.py:3: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import struct, io, binascii, itertools, collections, pickle, sys, os, tempfile, hashlib, importlib, imp

@rytilahti
Copy link
Owner

Please commit the poetry.lock file, too, then it should work!

@starkillerOG
Copy link
Contributor Author

running poetry add dataclasses

Using version ^0.7 for dataclasses

Updating dependencies
Resolving dependencies...

[SolverProblemError]
The current project's Python requirement (^3.6.5) is not compatible with some of the required packages Python requirement:
  - dataclasses requires Python >=3.6, <3.7

Because dataclasses (0.7) requires Python >=3.6, <3.7
 and no versions of dataclasses match >0.7,<0.8, dataclasses is forbidden.
So, because python-miio depends on dataclasses (^0.7), version solving failed.

I think we have a problem....

@rytilahti
Copy link
Owner

Ah, after reading this python-poetry/poetry#1413 (comment) the simplest fix is to modify the required python version... So please replace this:

python = "^3.6.5"

with

python = ">=3.6.5,<4.0"

@starkillerOG
Copy link
Contributor Author

nope does not work:

Using version ^0.7 for dataclasses

Updating dependencies
Resolving dependencies...

[SolverProblemError]
The current project's Python requirement (>=3.6.5,<4.0) is not compatible with some of the required packages Python requirement:
  - dataclasses requires Python >=3.6, <3.7

Because dataclasses (0.7) requires Python >=3.6, <3.7
 and no versions of dataclasses match >0.7,<0.8, dataclasses is forbidden.
So, because python-miio depends on dataclasses (^0.7), version solving failed.

@rytilahti
Copy link
Owner

rytilahti commented May 26, 2020

Argh, okay.. Well, I think it's better get rid of dataclasses and use attrs (which we are already using anyway in this project) instead.

The conversion is pretty simple:

  • Import attr instead of those dataclasses.
  • Use @attr.s(auto_attribs=True) instead of @dataclass for the props classes. The auto attributes will allow avoiding defining the properties like humidity = attr.ib(). If something is not working, just define it manually.
  • Use attr.asdict() for dumping the dict.

The good thing with this is also (which I think we should have converted them anyway, at some point) that this will allow use converters (maybe we want to convert, e.g., the open/close or on/off to booleans)?

@starkillerOG
Copy link
Contributor Author

@rytilahti I think I found a solution

@rytilahti
Copy link
Owner

@starkillerOG ah, I saw the commit. Could you still do the attrs conversion, or should I do that? It's better in the long run to make it properly anyway..

@starkillerOG
Copy link
Contributor Author

Not today, I am going to bed, but I will do the conversion tommorow, or you can merge it now and do the conversion...
what you like

@rytilahti
Copy link
Owner

Okay, no need to hurry with that, let's merge it then tomorrow (or whenever you find time to do that). Thanks for the great work already! 🥇

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to merge 💯

@rytilahti rytilahti merged commit 612996f into rytilahti:master May 27, 2020
@starkillerOG
Copy link
Contributor Author

Thanks for all your help @rytilahti

@rytilahti rytilahti mentioned this pull request Jun 4, 2020
xvlady pushed a commit to xvlady/python-miio that referenced this pull request May 9, 2021
* restructure and improve gateway subdevices

* Update gateway.py

* fix cli

* black formatting

* filter out gateway

* better error handeling

* common GatewayDevice class for __init__

* remove duplicate "gateway" from method name

* use device_type instead of device_name for mapping

* add comments

* use DeviceType.Gateway

Co-authored-by: Teemu R. <tpr@iki.fi>

* Update gateway.py

* improve discovered info

* fix formatting

* better use of dev_info

* final black formatting

* process revieuw

* generilize properties of subdevices

* Subdevice schould not derive from Device

* simplify dataclass props for subdevices

* optimization

* remove empty checks and futher simplify dataclass

* Update gateway.py

* add back SensorHT

* add back Empty response

* black formatting

* add missing docstrings

* fix except

* fix black

* Update pyproject.toml

* Update pyproject.toml

* fix dataclasses

* replace dataclasses by attr

* add get_local_status command

* remove local.status again

Co-authored-by: Teemu R. <tpr@iki.fi>
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.

5 participants