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

Add Xiaomi Miio gateway illuminance sensor and gateway light #37959

Merged
merged 17 commits into from
Aug 31, 2020

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jul 18, 2020

Breaking change

Proposed change

  • Add the illuminace sensor for the Xiaomi Miio gateway.
  • Add the gateway light for the Xiaomi Miio component

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

Config flow

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link

This pull request needs to be manually signed off by @home-assistant/core before it can get merged.
(message by ReviewEnforcer)

Copy link
Contributor

@shenxn shenxn left a comment

Choose a reason for hiding this comment

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

I've tested on my local dev environment and it works well. There is one thing though. I have a AC partner (lumi.acpartner.v3) that does not have illuminance sensor. It ends up with a sensor always given 0 lux in HA. Maybe we should check for positive illuminance value before adding the entity or filter using gateway model.

homeassistant/components/xiaomi_miio/sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title Xiaomi Miio: Add gateway illuminance sensor Add Xiaomi Miio gateway illuminance sensor Jul 19, 2020
@starkillerOG
Copy link
Contributor Author

@shenxn I did not know the lumi.acpartner.v3 also worked with the xiaomi miio code.
Have you also tested it using weather sensors?
does the alarm feature work?

If so I will list it in the documentation and will add it to zeroconf discovery

@shenxn
Copy link
Contributor

shenxn commented Jul 19, 2020

@starkillerOG There is no weather sensors for lumi.acpartner.v3 but the alarm works well.

@javicalle
Copy link
Contributor

I've been thinking about how to add the light to the gateway implementation and my first impulse was to define the entities within the Gateway filera ther than the Light file.
In my opinion, those entities are meaningless outside the gateway 'domain'.

What do you think about my approach?

@starkillerOG
Copy link
Contributor Author

@shenxn that is probably because you do not have temperature sensors connected to your acpartner.v3.
Do you have any zigbee devices connected to the acpartner.v3?
If you have could you test the following python code:

from miio import Gateway

gateway = Gateway("192.168.1.IP", "TokenTokenToken")

gateway.discover_devices()
devices = gateway.devices

for dev in devices.values():
    try:
        dev.update()
    except Exception as ex:
        print("got exception during update for %s: %s" % (dev.device_type, ex))
    print(dev)

@starkillerOG
Copy link
Contributor Author

@javicalle had not thought about it to much actually.
But it does make sense to put the gateway features in the gateway.py file instead of spread around the sensor.py light.py etc files.
I might indeed move everything to the gateway.py file.
Sounds good.

@javicalle are you already working on the light implementation or schould I add it?

@javicalle
Copy link
Contributor

@javicalle are you already working on the light implementation or schould I add it?

No, I haven't been working on it, sorry.
If you have already made progress, go ahead.

@shenxn
Copy link
Contributor

shenxn commented Jul 19, 2020

@starkillerOG Yes I do have some zigbee devices and I can get device states using the code.

@starkillerOG
Copy link
Contributor Author

Greath I will list the AC partner as supported including subdevices in the documentation update I am working on

@starkillerOG starkillerOG changed the title Add Xiaomi Miio gateway illuminance sensor Add Xiaomi Miio gateway illuminance sensor and gateway light Jul 19, 2020
@starkillerOG
Copy link
Contributor Author

starkillerOG commented Jul 19, 2020

I also added the gateway light entity.
The HomeAssistant code for the ligth entity can be simplified once this cleanup PR is merged in the upsteam lib: rytilahti/python-miio#770

@starkillerOG
Copy link
Contributor Author

@shenxn I made sure the gateway lux sensor and gateway light entities do not get setup when the gateway is an AC partner (since those don't have the lux sensor or LEDs build in).

@starkillerOG
Copy link
Contributor Author

This is ready for revieuw/merge
@rytilahti could you take a look at it?

@starkillerOG
Copy link
Contributor Author

@rytilahti I just rebased, could you approve this PR?

Copy link
Member

@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.

Just a very minor changes are needed from my side.

But let me ask, have you thought about if the light platform could use some refactoring to ease the maintenance burden in the future? At the moment there are so many separate light implementations in this file, I'd rather like to see that we consolidate at least the most common functionality to simplify the code base, if possible.

homeassistant/components/xiaomi_miio/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_miio/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_miio/light.py Outdated Show resolved Hide resolved
starkillerOG and others added 3 commits July 29, 2020 00:22
Co-authored-by: Teemu R. <tpr@iki.fi>
Co-authored-by: Teemu R. <tpr@iki.fi>
@starkillerOG
Copy link
Contributor Author

Just a very minor changes are needed from my side.

But let me ask, have you thought about if the light platform could use some refactoring to ease the maintenance burden in the future? At the moment there are so many separate light implementations in this file, I'd rather like to see that we consolidate at least the most common functionality to simplify the code base, if possible.

I have thought about it, but all those other classes are of the old Xiaomi Philips Light platform that still uses YAML config https://www.home-assistant.io/integrations/light.xiaomi_miio/.
I have not looked at how these lights actually work and since I do not own them I was not planning on figuring out how this exactly works, so I decided to leave those classes alone.

I think that schould be cleaned up when the Xiaomi Philips Light platform gets converted to Config flow.

@rytilahti
Copy link
Member

Yes, agreed. It will make sense to go through all the lights (and purifiers, and other..) at some point to find which parts (or API) they share and refactor python-miio to offer common interface where possible.

The separate platform files are getting really large and share many common things, so for maintenance those should be refactored, maybe be introducing something like XiaomiEntity which already implements the most common parts (state fetching, availability, ..).

I'll try to see if I can find some time to untangle that puzzle, so it is not something that's to be taken care of this PR, so I'm okay for merging this. I'll add a label to see if someone wants to chime in on something that needs to be changed before merging.

@rytilahti rytilahti added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Jul 28, 2020
@starkillerOG
Copy link
Contributor Author

@rytilahti I do not see this PR in the dev project where PRs are tracked, I think something went wrong with the labels...

@starkillerOG
Copy link
Contributor Author

Can someone approve this?

homeassistant/components/xiaomi_miio/light.py Outdated Show resolved Hide resolved
homeassistant/components/xiaomi_miio/light.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Aug 31, 2020
@starkillerOG
Copy link
Contributor Author

@MartinHjelmare Any more suggestions?

@starkillerOG
Copy link
Contributor Author

@MartinHjelmare can this be merged now?

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit f187091 into home-assistant:dev Aug 31, 2020
leikoilja pushed a commit to leikoilja/core that referenced this pull request Sep 6, 2020
…sistant#37959)

Co-authored-by: Xiaonan Shen <s@sxn.dev>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants