Xiaomi MiIO light: Philips Eyecare Smart Lamp 2 integration #12883
Conversation
| self._light.delay_off, round(time_period.total_seconds()/60)) | ||
|
|
||
|
|
||
| @asyncio.coroutine |
| hass.data[DATA_KEY][host] = device | ||
| async_add_devices([device], update_before_add=True) | ||
|
|
||
| async_add_devices(devices, update_before_add=True) |
syssi
changed the title
syssi
changed the title
syssi
changed the title
syssi
changed the title
syssi
changed the title
syssi
changed the title
| hass.data[DATA_KEY][host] = device | ||
|
|
||
| device = XiaomiPhilipsEyecareLampAmbientLight( | ||
| name + ' Ambient Light', light, model) |
There was a problem hiding this comment.
Use '{} Ambient Light'.format(name)
| devices.append(device) | ||
| hass.data[DATA_KEY][host] = device | ||
|
|
||
| device = XiaomiPhilipsEyecareLampAmbientLight( |
There was a problem hiding this comment.
Please don't reuse the same local variable name within the same scope. That's confusing.
| """Return the supported features.""" | ||
| return SUPPORT_FLAGS_SREAD1_EYECARE_LIGHT | ||
|
|
||
| @asyncio.coroutine |
There was a problem hiding this comment.
We've migrated to async/await syntax. Please drop this decorator and prefix async methods with async like async def async_update. Replace any yield from with await.
| """Fetch state from the device.""" | ||
| from miio import DeviceException | ||
| try: | ||
| state = yield from self.hass.async_add_job(self._light.status) |
There was a problem hiding this comment.
If you're going to call a sync method, you might as well just make the whole update method not async
|
|
||
| async def async_set_delayed_turn_off(self, time_period: timedelta): | ||
| """Set delayed turn off.""" | ||
| if self._additional_supported_features & SUPPORT_SET_DELAYED_TURN_OFF == 0: |
| self._light.delay_off, time_period.total_seconds()) | ||
| async def async_set_delayed_turn_off(self, time_period: timedelta): | ||
| """Set delayed turn off.""" | ||
| if self._additional_supported_features & SUPPORT_SET_DELAYED_TURN_OFF == 0: |
Unnecessary parens after 'return' keyword fixed.
| @@ -210,14 +253,13 @@ def brightness(self): | |||
| @property | |||
| def supported_features(self): | |||
| """Return the supported features.""" | |||
| return SUPPORT_BRIGHTNESS | |||
| return 0 | |||
There was a problem hiding this comment.
This should implement at least SUPPORT_BRIGHTNESS since you return the brightness in the property defined above this one?
| @property | ||
| def supported_features(self): | ||
| """Return the supported features.""" | ||
| return SUPPORT_BRIGHTNESS |
There was a problem hiding this comment.
I see you implementing color temp in the constructor. Shouldn't that be included in the supported features?
There was a problem hiding this comment.
I moved the implementation to the correct class (bulb) now.
| # pylint: disable=no-self-use | ||
| async def async_reminder_on(self): | ||
| """Enable the eye fatigue notification.""" | ||
| return |
There was a problem hiding this comment.
Is raising NotImplementedError correct here? Some devices (classes) doesn't support the feature / service. In this case the service call should be ignored. All xiaomi_miio lights uses the same DATA_KEY. If somebody calls light.xiaomi_miio_reminder_on (without an entity_id parameter) all entities will be addressed and the method needs to be accessible.
There was a problem hiding this comment.
But why wouldn't the service call filter them out?
for device in devices:
if not hasattr(device, 'async_reminder_on'):
continue
await device.async_reminder_on()
|
|
||
| async def async_reminder_on(self): | ||
| """Enable the eye fatigue notification.""" | ||
| if self._additional_supported_features & SUPPORT_REMINDER == 0: |
There was a problem hiding this comment.
Why this? You hardcode the additional features in the constructor
…nces of the supported devices are simple.
Refactoring.
Please stop adding types to this PR, it's already getting WAAY to big. try to limit PRs to 1 thing. |
|
|
||
| class XiaomiPhilipsEyecareLamp(XiaomiPhilipsGenericLight, Light): | ||
| except DeviceException as ex: | ||
| self._state = None |
There was a problem hiding this comment.
Instead of just setting state to None, the available property should return False
| if model is None: | ||
| try: | ||
| miio_device = Device(host, token) | ||
| device_info = miio_device.info() | ||
| model = device_info.model | ||
| unique_id = "{}-{}".format(model, device_info.mac_address) |
There was a problem hiding this comment.
why include model and not just mac address ? Mac address should be unique per device.
There was a problem hiding this comment.
Oh I see the primary/secondary device things now.
|
This is ok to merge once the available property is correctly set to False when the device cannot update. |
| """Representation of a Xiaomi Philips Ceiling Lamp.""" | ||
|
|
||
| def __init__(self, name, light, model, unique_id): | ||
| """Initialize the light device.""" | ||
| XiaomiPhilipsBulb.__init__(self, name, light, model, unique_id) |
There was a problem hiding this comment.
The init calls should probably be done on super() (as that will keep it working even if the name of the super class changes).
|
Great, thanks 👍 |
ghost
added
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io/pull/4825
Example entry for
configuration.yaml(if applicable):