Add support for the cgllc.airmonitor.b1 #562
Conversation
Added support for cgllc.airmonitor.b1
miio/airqualitymonitor.py
Outdated
| ) -> None: | ||
| super().__init__(ip, token, start_id, debug, lazy_discover) | ||
|
|
||
| if model == None: |
There was a problem hiding this comment.
Please don't add I/O at the constructor. The correct model should be passed to the constructor if needed.
syssi
changed the title
| @@ -206,11 +241,12 @@ def __init__( | |||
|
|
|||
| if model in AVAILABLE_PROPERTIES: | |||
| self.model = model | |||
| else: | |||
| elif model is not None: | |||
| self.model = MODEL_AIRQUALITYMONITOR_V1 | |||
There was a problem hiding this comment.
When a model is provided, but not in available_properties fallback is still there.
|
Thanks for your suggestions! Please let me know if anything else should be changed. |
miio/airqualitymonitor.py
Outdated
| self._model = info.model | ||
| self._mac_address = info.mac_address | ||
| self._firmware_version = info.firmware_version | ||
| self._hardware_version = info.hardware_version |
There was a problem hiding this comment.
Where are you planning to use these? To me it looks like they are not used at all, so it'd be better simply to remove the last three lines, no?
If it's for homeassistant integration, it should simply query the info itself and parse the information out from it (instead of accessing these now-private variables) :-)
There was a problem hiding this comment.
It is instead for the HA integration. But I don't like the idea of querying the info again. Also I don't see any trouble in saving the info on device-level. So now the remaining question to me is: where to store them? I suppose all of the Xiaomi Air Quality devices have the same response here?
There was a problem hiding this comment.
How about creating the following in the AirQualityMonitor class (for mac_address, model, firmware_version and hardware_version):
@property
def mac_address(self) -> Optional[int]:
"""Return mac address."""
return self._mac_address
There was a problem hiding this comment.
I'm thinking if it would make simply sense to extend Device's info() to cache the result (and add a kwarg refresh if there's a need to force re-fetching), as I assume the information returned is quite static (see https://github.com/rytilahti/python-miio/blob/master/miio/device.py#L33).
But that should be done in a separate PR, for the time being I would do the caching downstream (as you already do in the homeassistant PR) and we can think about adapting the behavior of info() (https://github.com/rytilahti/python-miio/blob/master/miio/device.py#L348) to cache the results for some time period or until explicitly asked for a refresh.
@syssi do you have an opinion on this?
There was a problem hiding this comment.
First it started with getting the device model, which is needed in case of te B1 to simply get te status. That's why I added the info( ) call in the first place to the init, and then moved it to the status method. Here the info( ) is only called once, when model is None.
There was a problem hiding this comment.
Yes, I got that, but what I'm trying to say is that those internal (info) variables should not be exposed like this only for a single device type. There is already DeviceInfo container holding this information, so the homeassistant part should simply call info (once) and save those details for its own use.
There was a problem hiding this comment.
I reversed some changes (model, firmware etc). All should be fine now (I hope).
There was a problem hiding this comment.
Looks good, thanks! Let's get it merged! 🎉
rytilahti
changed the title
Added support for cgllc.airmonitor.b1