Skip to content

Use dataclass properties in yeelight discovery#60640

Merged
bdraco merged 5 commits intohome-assistant:devfrom
epenet:yeelight-discovery
Dec 1, 2021
Merged

Use dataclass properties in yeelight discovery#60640
bdraco merged 5 commits intohome-assistant:devfrom
epenet:yeelight-discovery

Conversation

@epenet
Copy link
Copy Markdown
Contributor

@epenet epenet commented Nov 30, 2021

Proposed change

Use dataclass properties in yeelight discovery.

Linked to #60540

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:

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.
  • 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:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @rytilahti, @zewelor, @shenxn, @starkillerOG, mind taking a look at this pull request as it has been labeled with an integration (yeelight) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Nov 30, 2021

I did not update async_step_ssdp yet because I am not 100% sure if these two fields come from the SSDP headers or from the UPnP information:

async def async_step_ssdp(self, discovery_info):
"""Handle discovery from ssdp."""
self._discovered_ip = urlparse(discovery_info["location"]).hostname
await self.async_set_unique_id(discovery_info["id"])

For the tests, I assumed it was UPnP but if I made a mistake then I can correct it in this PR :

SSDP_INFO = ssdp.SsdpServiceInfo(
ssdp_usn="mock_usn",
ssdp_st="mock_st",
upnp=CAPABILITIES,
)

If you do not have the opportunity to check it, then I suggest that we merge it as it is to avoid blocking #60561, and let the warning kick in when #60540 is merged.

@epenet epenet marked this pull request as ready for review November 30, 2021 15:44
@epenet epenet requested a review from rytilahti as a code owner November 30, 2021 15:44
@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Nov 30, 2021

It seems also that yeelight is missing ssdp in manifest.json
Should it be added?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Nov 30, 2021

I did not update async_step_ssdp yet because I am not 100% sure if these two fields come from the SSDP headers or from the UPnP information:

2021-11-30 06:16:05 WARNING (MainThread) [homeassistant.components.yeelight.config_flow] dinfo: {'Cache-Control': 'max-age=3600', 'Date': '', 'Ext': '', 'Server': 'POSIX UPnP/1.0 YGLC/1', 'id': '0x00000000158d5a91', 'model': 'ceiling10', 'fw_ver': '49', 'support': 'get_prop set_default set_power toggle set_bright set_scene cron_add cron_get cron_del start_cf stop_cf set_ct_abx set_name set_adjust adjust_bright adjust_ct bg_set_rgb bg_set_hsv bg_set_ct_abx bg_start_cf bg_stop_cf set_scene_bundle bg_set_default bg_set_power bg_set_bright bg_set_adjust bg_adjust_bright bg_adjust_color bg_adjust_ct bg_toggle dev_toggle', 'power': 'on', 'bright': '100', 'color_mode': '2', 'ct': '2801', 'rgb': '0', 'hue': '0', 'sat': '0', 'name': '', '_location_original': 'yeelight://192.168.107.53:55443', 'location': 'yeelight://192.168.107.53:55443', '_timestamp': datetime.datetime(2021, 11, 30, 6, 16, 4, 56326), '_host': '192.168.107.53', '_port': 49154, '_source': <SsdpSource.SEARCH: 'search'>}
2021-11-30 06:16:05 WARNING (MainThread) [homeassistant.components.yeelight.config_flow] dinfo: {'Cache-Control': 'max-age=3600', 'Date': '', 'Ext': '', 'Server': 'POSIX UPnP/1.0 YGLC/1', 'id': '0x000000000ed53acf', 'model': 'colorb', 'fw_ver': '10', 'support': 'get_prop set_default set_power toggle set_bright set_scene cron_add cron_get cron_del start_cf stop_cf set_ct_abx adjust_ct set_name set_adjust adjust_bright adjust_color set_rgb set_hsv set_music udp_sess_new udp_sess_keep_alive udp_chroma_sess_new', 'power': 'on', 'bright': '100', 'color_mode': '2', 'ct': '2801', 'rgb': '8388863', 'hue': '270', 'sat': '100', 'name': '', '_location_original': 'yeelight://192.168.106.16:55443', 'location': 'yeelight://192.168.106.16:55443', '_timestamp': datetime.datetime(2021, 11, 30, 6, 16, 4, 63776), '_host': '192.168.106.16', '_port': 49153, '_source': <SsdpSource.SEARCH: 'search'>}

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Nov 30, 2021

Thanks @bdraco, I have moved this to ssdp_headers.
However the tests now fail with AttributeError: 'dict' object has no attribute 'ssdp_headers'

I assume that I have missed a fixture somewhere when I changed all the tests but I can't find it :(

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Nov 30, 2021

It was an issue in _patch_discovery. That is fixed now.
Hassfest is still complaining about the ssdp dependency in hassfest:

Integrations: 1015
Invalid integrations: 1

Integration yeelight:
Error: R] [DEPENDENCIES] Using component ssdp but it's not in 'dependencies' or 'after_dependencies'

@epenet epenet force-pushed the yeelight-discovery branch from 0d653e2 to 353ff25 Compare December 1, 2021 08:20
@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Dec 1, 2021

I have added "after_dependencies": ["ssdp"] to the manifest and now this passes.

@bdraco bdraco merged commit 73a4dba into home-assistant:dev Dec 1, 2021
@epenet epenet deleted the yeelight-discovery branch December 1, 2021 15:22
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 1, 2021

@epenet
Saw this trace this morning on my integration branch

2021-12-01 05:59:03 ERROR (MainThread) [homeassistant] Error doing job: Task exception was never retrieved:   File "/Users/bdraco/home-assistant/venv/bin/hass", line 8, in <module>
    sys.exit(main())
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/__main__.py", line 318, in main
    exit_code = runner.run(runtime_conf)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/runner.py", line 121, in run
    return loop.run_until_complete(setup_and_run_hass(runtime_config))
  File "/opt/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 629, in run_until_complete
    self.run_forever()
  File "/opt/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 596, in run_forever
    self._run_once()
  File "/opt/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 1882, in _run_once
    handle._run()
  File "/opt/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/events.py", line 80, in _run
    self._context.run(self._callback, *self._args)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/helpers/event.py", line 1273, in run_action
    hass.async_run_hass_job(job, utc_point_in_time)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/core.py", line 433, in async_run_hass_job
    hassjob.target(*args)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/components/yeelight/scanner.py", line 160, in _async_start_flow
    asyncio.create_task(
  File "/opt/homebrew/Cellar/python@3.9/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/tasks.py", line 361, in create_task
    task = loop.create_task(coro)
Traceback (most recent call last):
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/data_entry_flow.py", line 203, in async_init
    flow, result = await task
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/data_entry_flow.py", line 230, in _async_init
    result = await self._async_handle_step(flow, flow.init_step, data, init_done)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/data_entry_flow.py", line 325, in _async_handle_step
    result: FlowResult = await getattr(flow, method)(user_input)
  File "/Users/bdraco/home-assistant/venv/lib/python3.9/site-packages/homeassistant/components/yeelight/config_flow.py", line 83, in async_step_ssdp
    self._discovered_ip = urlparse(discovery_info.ssdp_headers["location"]).hostname
AttributeError: 'CaseInsensitiveDict' object has no attribute 'ssdp_headers'

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Dec 1, 2021

@frenck had the same issue.
Can you get a fuller trace with the complete ssdp/upnp data?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 1, 2021

2021-12-01 06:16:41 DEBUG (MainThread) [homeassistant.components.yeelight.scanner] Discovered via SSDP: {'Cache-Control': 'max-age=3600', 'Date': '', 'Ext': '', 'Server': 'POSIX UPnP/1.0 YGLC/1', 'id': '0x00000000158d5a91', 'model': 'ceiling10', 'fw_ver': '49', 'support': 'get_prop set_default set_power toggle set_bright set_scene cron_add cron_get cron_del start_cf stop_cf set_ct_abx set_name set_adjust adjust_bright adjust_ct bg_set_rgb bg_set_hsv bg_set_ct_abx bg_start_cf bg_stop_cf set_scene_bundle bg_set_default bg_set_power bg_set_bright bg_set_adjust bg_adjust_bright bg_adjust_color bg_adjust_ct bg_toggle dev_toggle', 'power': 'off', 'bright': '100', 'color_mode': '2', 'ct': '2801', 'rgb': '0', 'hue': '0', 'sat': '0', 'name': '', '_location_original': 'yeelight://192.168.107.53:55443', 'location': 'yeelight://192.168.107.53:55443', '_timestamp': datetime.datetime(2021, 12, 1, 6, 16, 41, 467365), '_host': '192.168.107.53', '_port': 49154, '_source': <SsdpSource.SEARCH: 'search'>}
2021-12-01 06:16:41 DEBUG (MainThread) [homeassistant.components.yeelight.scanner] Yeelight discovered with {'Cache-Control': 'max-age=3600', 'Date': '', 'Ext': '', 'Server': 'POSIX UPnP/1.0 YGLC/1', 'id': '0x00000000158d5a91', 'model': 'ceiling10', 'fw_ver': '49', 'support': 'get_prop set_default set_power toggle set_bright set_scene cron_add cron_get cron_del start_cf stop_cf set_ct_abx set_name set_adjust adjust_bright adjust_ct bg_set_rgb bg_set_hsv bg_set_ct_abx bg_start_cf bg_stop_cf set_scene_bundle bg_set_default bg_set_power bg_set_bright bg_set_adjust bg_adjust_bright bg_adjust_color bg_adjust_ct bg_toggle dev_toggle', 'power': 'off', 'bright': '100', 'color_mode': '2', 'ct': '2801', 'rgb': '0', 'hue': '0', 'sat': '0', 'name': '', '_location_original': 'yeelight://192.168.107.53:55443', 'location': 'yeelight://192.168.107.53:55443', '_timestamp': datetime.datetime(2021, 12, 1, 6, 16, 41, 467365), '_host': '192.168.107.53', '_port': 49154, '_source': <SsdpSource.SEARCH: 'search'>}

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 1, 2021

Yeelight is a bit special because it runs SSDP discovery on an alternate port

https://github.com/home-assistant/core/blob/dev/homeassistant/components/yeelight/scanner.py#L157

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Dec 1, 2021

I can take a look tomorrow - feel free to add more information here in the meantime.

bdraco added a commit to bdraco/home-assistant that referenced this pull request Dec 1, 2021
@bdraco bdraco mentioned this pull request Dec 1, 2021
22 tasks
cgarwood pushed a commit that referenced this pull request Dec 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants