-
-
Notifications
You must be signed in to change notification settings - Fork 566
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
Gateway: add name + model property to subdevice & add loads of subdevices #724
Conversation
@rytilahti ready to be merged |
@rytilahti Is this alright now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a very small change and this can be merged! Run also tox -e lint
(or preferably, install the precommit hooks) to avoid linting issues in general :-)
One comment about the most recent commit, I think we need to add some sort of warning log indicating that the device has been detected but is not implemented (based on missing properties, or somehow else in case the device has no properties at all?). Otherwise this will cause bug reports of correctly detected but not yet supported devices. |
Well those devices will log an debug message when the update method is called if that has not been specified by the device. And these devices will not have enteties so I think we will be fine. In princeple we also want people that have a device that is not supported yet to report here so they can figure out which properties the device has and then implement those. |
Also fromatting Also improve subdevice class representation
I reduced the not implemented warning to log level because there are tons of devices like motion sensor, door sensor, remote switches, buttons and cube that do not have any properties, they only provide events. And since event support requires subscriptions that is still quite far away. So I don't think it is fair to bombard people with warnings of not implemented devices since we don't have a way to implement them yet before the subscription PR (#709) is done. |
@rytilahti I think this is now ready to be merged. I have also tested the code and for my gateway it works. |
_name = "Vima cylinder lock" | ||
|
||
|
||
class IkeaBulb82(SubDevice): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think we need to rethink the approach for the metadata at some point, considering how many of these ikea bulbs likely share similar features, it is not really worth to have separate implementations for it. I think we will want to have a container class defining the supported devices (and other metadata) per class to avoid the code duplication.
The gateway.py should also be split up to more manageable pieces (under miio/gateway/
package), there are already way too many lines of code (and different things) for a single file.
I think it is okay to merge this now though, refactoring can follow in separate PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the gateway.py file grew a lot and is getting way too big.
Was already thinking the same thing, at least all subdevices schould be moved to a gateway_subdevices.py file or something....
for the IkeaBulbs and simular classes, I think it would be best to define a new class IkeaBulb
that inherits properties from SubDevice
class, then all individual model classes like IkeaBulb82
can inherit all common functions from the IkeaBulb
class.
Not sure if that is what you were trying to say....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe split up all subdevices in files which have one common class that they inherit from. so all IkeaBulbs in one file, all cubes in one, all buttons in one etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, having a common base class would make it much cleaner already. I don't think having a gateway_subdevices.py
is a good idea, it's much better to have a structure like:
miio/gateway/
gateway.py
gateway_zigbee.py
gateway_light.py
gateway_radio.py
and so on. To extend that example, here's another idea that would make it much nicer:
miio/gateway/
gateway.py
subdevice.py
miio/gateway/aqara/
cube.py
switch.py
miio/gateway/ikea/
bulb.py
Yes, having a guide how to add support for new devices would be great!
We need a way to differentiate between "no known properties" and "no properties" to avoid useless warnings, but this can be also done separately from this PR. |
@rytilahti could you please push to a new version of python-miio? |
I'll see if I can find some time tomorrow to prepare a release, but I can't make any promises on that. |
…ices (rytilahti#724) * Gateway: add name property to subdevice * add zigbee_model & name * add loads of devices * backup for unknown name * Add lots of subdevice classes * add aditional devices and order * fix black formatting * fix flake8 stylings * add last known devices * remove testing code which does not work * Add not implemented warning Also fromatting Also improve subdevice class representation * reduce not implemented warning to info
In this way we can easily define how the name of subdevices schould be set in HomeAssistant. Ideally we would at some point figure out how to get the names set in the Mi Home app...
But for now use the device_type + sid as a name