Add additional select for dmaker.airfresh.t2017 to xiaomi_miio #67058

Kirmas · 2022-02-22T18:06:35Z

Proposed change

Another step after #66331 and #66370 I have added select like:

Name	Comment
Auxiliary Heat Level	Can be "low", "medium", "high"
Display Orientation	Can be "forward", "left", "right"

Other change is adding XiaomiGenericSelector this new class give me chance to not create new class for every select, I use it for dmaker.airfresh.t2017 and zhimi.humidifier.ca1 - Led Brightness because I have this devices and can check - this is working. But for other humidifiers maybe someone who have it can migrate it or help me with migration to new generic class.
@rytilahti could you check this PR with more notice, maybe I do it wrong?

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

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add select entities for dmaker.airfresh.t2017 to xiaomi_miio home-assistant.io#21870

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:

Documentation added/updated for www.home-assistant.io

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

- Display Orientation("forward", "left", "right") - Auxiliary Heat Level("low", "medium", "high") - XiaomiGenericSelector was added to avoid new class creation for every new select

probot-home-assistant · 2022-02-22T18:06:40Z

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

probot-home-assistant · 2022-02-22T18:06:42Z

Hey there @rytilahti, @syssi, @starkillerOG, @bieniu, mind taking a look at this pull request as it has been labeled with an integration (xiaomi_miio) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

Kirmas · 2022-03-02T16:36:44Z

@rytilahti What do you think if I tried move all Selector to new Generic class. I think this must work by default (if I configure it right) and will do code as simple as possible. But I haven`t all type of device to check? Or will it be out off scope of this PR?

rytilahti · 2022-03-02T17:47:28Z

What do you think if I tried move all Selector to new Generic class. I think this must work by default (if I configure it right) and will do code as simple as possible. But I haven`t all type of device to check? Or will it be out off scope of this PR?

I think it makes sense to clean it up & generalize as much as possible as you did, also fine inside the scope of this PR. I'll try to find some time later tonight to take a look in the code and provide some initial feedback.

rytilahti · 2022-03-02T22:42:57Z

Great cleanups @Kirmas, you inspired me to explore a bit on how we could generalize this even more!

So the approach I was thinking involves using the type hints in the device classes of python-miio:

If a method takes only a single enum, it could be a candidate for a homeassistant select
If a method takes only a single boolean, it could be a candidate for a homeassistant option
If a method takes no parameters, it could be a candidate for a homeassistant button
And so on..

There is still the need to map the status container value to the "candidate", and some ways to mark the useful methods inside python-miio, return only the enum values that are relevant for the given instance, and maybe provide a title/description, icon etc. that could be directly used in homeassistant? Similar trick could be done for the status container information to provide sensors (this will again require some more metadata to python-miio to make it useful) :-)

This will allow creating the homeassistant entities without hard-coding the specifics inside homeassistant, making it really easy to extend on new devices as long as they are supported by homeassistant.

Check out https://gist.github.com/rytilahti/0a0a84e57c35ddb1ccdc7c9a7879c7f6 for the script, here's an example output for AirFreshT2017:

== AirFreshT2017 == 
BUTTON candidate on
BUTTON candidate off
SELECT candidate set_mode: mode: ['Off', 'Auto', 'Sleep', 'Favorite']
OPTION candidate set_display: display
OPTION candidate set_ptc: ptc
OPTION candidate set_buzzer: buzzer
OPTION candidate set_child_lock: lock
BUTTON candidate reset_dust_filter
Got unhandled type for set_favorite_speed: <class 'int'>
BUTTON candidate set_ptc_timer
BUTTON candidate get_ptc_timer
BUTTON candidate get_timer
BUTTON candidate _wrap
BUTTON candidate reset_upper_filter
SELECT candidate set_display_orientation: orientation: ['Portrait', 'LandscapeLeft', 'LandscapeRight']
SELECT candidate set_ptc_level: level: ['Low', 'Medium', 'High']

Now, this is outside of the scope of this PR, but I just wanted to give you a heads up on it. What do you think?

Kirmas · 2022-03-03T07:59:43Z

So the approach I was thinking involves using the type hints in the device classes of python-miio:
......
Now, this is outside of the scope of this PR, but I just wanted to give you a heads up on it. What do you think?

@rytilahti Looks like grate idea for the future. Also if we will get device list and spec from the library its give us a chance add device support only on our side. (python_miio has a lot of device that not integrated in the HA and this is pity).
For me now It is too big project, I think you understand, but I will tried to do something small. (Maybe trying to finish yeelight inside python_miio support, now I have problem how to organize multi lamp device architecture. Or I can moving some const to python_miio library. Or adding rytilahti/python-miio#1068 to python_miio) , after mergin this one. And if will be silent near me.
Sorry for bad English, a bit noisy (explosions are heard), I'm nervous.

rytilahti · 2022-03-04T01:41:14Z

@Kirmas I just wanted to point out how we can solve this better in the future, and this PR with its cleanups is a nice improvement towards towards that future. And please, please do not overburden yourself & just take care and stay safe, that's more important than any of this! Also, feel free to contact me via discord or via email if you wish to discuss about anything.

Kirmas · 2022-05-22T07:51:10Z

@rytilahti As we spoke in private messages. I have added tests for select. Could you review and approve?

homeassistant/components/xiaomi_miio/select.py

homeassistant/components/xiaomi_miio/strings.select.json

rytilahti · 2022-06-16T18:15:46Z

tests/components/xiaomi_miio/test_select.py

+from miio.airfresh_t2017 import (
+    DisplayOrientation as AirfreshT2017DisplayOrientation,
+    PtcLevel as AirfreshT2017PtcLevel,
+)
+from miio.airhumidifier import LedBrightness as AirhumidifierLedBrightness
+from miio.airhumidifier_miot import LedBrightness as AirhumidifierMiotLedBrightness
+from miio.tests.test_airfresh_t2017 import DummyAirFreshT2017
+from miio.tests.test_airhumidifier import DummyAirHumidifier
+from miio.tests.test_airhumidifier_miot import DummyAirHumidifierMiot


Let's avoid importing anything from python-miio here, and simply create some dummy enums that fulfill the same purpose.

My personal opinion is that the tests should verify that XiaomiGenericSelector and relevant pieces work in select.py, no matter which device is exposing those.

But I'll take another look at the test code soon, I just wanted to give you some quick comments asap.

@rytilahti I dont really understand how to do this with out import Dummy device. (Maybe only ones?) Because I need coordinator to check _handle_coordinator_update function. And other milo interfaces.

Maybe if I remove airhumidifier_miot , test_airfresh_t2017 and use only airhumidifier class it will be enough?

@rytilahti Tests was updated. Could you look at please.

homeassistant/components/xiaomi_miio/select.py

tests/components/xiaomi_miio/test_select.py

homeassistant/components/xiaomi_miio/select.py

MartinHjelmare

Nice!

Add additional select for dmaker.airfresh.t2017 to xiaomi_miio

dcf6de4

- Display Orientation("forward", "left", "right") - Auxiliary Heat Level("low", "medium", "high") - XiaomiGenericSelector was added to avoid new class creation for every new select

Kirmas requested review from rytilahti, syssi and bieniu as code owners February 22, 2022 18:06

homeassistant added the cla-signed label Feb 22, 2022

probot-home-assistant bot added integration: xiaomi_miio new-feature labels Feb 22, 2022

probot-home-assistant bot assigned bieniu and rytilahti Feb 22, 2022

probot-home-assistant bot assigned syssi Feb 22, 2022

project-bot bot added this to Needs review in Dev Feb 22, 2022

This was referenced Feb 22, 2022

Add documentation for xiaomi_miio's dmaker.airfresh.{a1,t2017} support home-assistant/home-assistant.io#21538

Merged

Add additional switch for dmaker.airfresh.a1/t2017 to xiaomi_miio #67033

Merged

separate blocks

16e7472

Kirmas mentioned this pull request Mar 2, 2022

Add select entities for dmaker.airfresh.t2017 to xiaomi_miio home-assistant/home-assistant.io#21870

Merged

8 tasks

Did code more simple.

9d02a47

Kirmas added 2 commits March 2, 2022 22:44

Use XiaomiGenericSelector for all devices

0bb41bf

remove secont variable definition

91b76b4

rytilahti mentioned this pull request Apr 25, 2022

Rename set_wifi_led_* to set_led_* for xiaomi_miio #68629

Closed

22 tasks

Kirmas added 2 commits May 8, 2022 13:19

Merge branch 'dev' into add_dmaker_airfresh_part_5

1f9ba83

add tests for select

00caeb7

Add tests for coordinator update

0397cab

rytilahti reviewed Jun 16, 2022

View reviewed changes

Kirmas added 8 commits June 18, 2022 08:53

Newline was added

a5ce9f4

Using NamedTuple to do code more readable

13f874a

rename options to enum_class

ff7f028

Use enum _member_map_ as option map

8928db2

Merge branch 'dev' into add_dmaker_airfresh_part_5

93bebc6

fix for black

f6179d2

Fix for test

50a6f3c

Do tests more general

d0c1b1a

MartinHjelmare requested changes Aug 1, 2022

View reviewed changes

tests/components/xiaomi_miio/test_select.py Outdated Show resolved Hide resolved

Dev automation moved this from Needs review to Review in progress Aug 1, 2022

use patching to avoid IO.

21c4717

Kirmas requested a review from MartinHjelmare August 2, 2022 05:26

MartinHjelmare reviewed Aug 5, 2022

View reviewed changes

tests/components/xiaomi_miio/test_select.py Outdated Show resolved Hide resolved

tests/components/xiaomi_miio/test_select.py Outdated Show resolved Hide resolved

tests/components/xiaomi_miio/test_select.py Outdated Show resolved Hide resolved

Use one of the rial device MODEL for test

2087c8b

Kirmas requested a review from MartinHjelmare August 6, 2022 07:07

MartinHjelmare reviewed Aug 16, 2022

View reviewed changes

homeassistant/components/xiaomi_miio/select.py Outdated Show resolved Hide resolved

homeassistant/components/xiaomi_miio/select.py Outdated Show resolved Hide resolved

Kirmas added 2 commits August 16, 2022 17:34

remove attr function as not used

6cfadf0

More safe

2ca43f3

MartinHjelmare approved these changes Aug 16, 2022

View reviewed changes

Dev automation moved this from Review in progress to Reviewer approved Aug 16, 2022

Kirmas added 2 commits August 16, 2022 17:44

fix for mypy

fa278c9

Merge branch 'dev' into add_dmaker_airfresh_part_5

aa9dc73

MartinHjelmare merged commit 65f86ce into home-assistant:dev Aug 16, 2022

Dev automation moved this from Reviewer approved to Done Aug 16, 2022

Kirmas deleted the add_dmaker_airfresh_part_5 branch August 16, 2022 15:35

github-actions bot locked and limited conversation to collaborators Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional select for dmaker.airfresh.t2017 to xiaomi_miio #67058

Add additional select for dmaker.airfresh.t2017 to xiaomi_miio #67058

Kirmas commented Feb 22, 2022 •

edited

probot-home-assistant bot commented Feb 22, 2022

probot-home-assistant bot commented Feb 22, 2022

Kirmas commented Mar 2, 2022 •

edited

rytilahti commented Mar 2, 2022

rytilahti commented Mar 2, 2022 •

edited

Kirmas commented Mar 3, 2022 •

edited

rytilahti commented Mar 4, 2022

Kirmas commented May 22, 2022

rytilahti Jun 16, 2022

Kirmas Jun 26, 2022

Kirmas Aug 1, 2022

MartinHjelmare left a comment

Add additional select for dmaker.airfresh.t2017 to xiaomi_miio #67058

Add additional select for dmaker.airfresh.t2017 to xiaomi_miio #67058

Conversation

Kirmas commented Feb 22, 2022 • edited

Proposed change

Type of change

Additional information

Checklist

probot-home-assistant bot commented Feb 22, 2022

probot-home-assistant bot commented Feb 22, 2022

Kirmas commented Mar 2, 2022 • edited

rytilahti commented Mar 2, 2022

rytilahti commented Mar 2, 2022 • edited

Kirmas commented Mar 3, 2022 • edited

rytilahti commented Mar 4, 2022

Kirmas commented May 22, 2022

rytilahti Jun 16, 2022

Choose a reason for hiding this comment

Kirmas Jun 26, 2022

Choose a reason for hiding this comment

Kirmas Aug 1, 2022

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

Kirmas commented Feb 22, 2022 •

edited

Kirmas commented Mar 2, 2022 •

edited

rytilahti commented Mar 2, 2022 •

edited

Kirmas commented Mar 3, 2022 •

edited