Skip to content

Going async with denonavr#47920

Merged
bdraco merged 22 commits intohome-assistant:devfrom
ol-iver:denonavr-async
Apr 2, 2021
Merged

Going async with denonavr#47920
bdraco merged 22 commits intohome-assistant:devfrom
ol-iver:denonavr-async

Conversation

@ol-iver
Copy link
Copy Markdown
Contributor

@ol-iver ol-iver commented Mar 14, 2021 •
edited
Loading

Proposed change

With this update denonavr is going async. It is using httpx instead of requests now and is 100% async.

There are a few bugs which are already identified and fixed in dev version of denonavr or in progress:
- non functional play-pause toggle
- improvement of 403 error handling of AVR-X 2016 receivers

I would like to collect some fixes and add fixes for potential issues discovered here during review.
That's why I did not set the "local tests pass" flag yet. Please start reviewing the changes anyhow. There is a bunch of them.

Known bugs are fixed, ready to proceed.

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

probot-home-assistant bot commented Mar 14, 2021

Hey there @starkillerOG, mind taking a look at this pull request as its been labeled with an integration (denonavr) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

bdraco
bdraco reviewed Mar 15, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 15, 2021
View reviewed changes
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 15, 2021

CI should pass if you rebase

@bdraco bdraco self-requested a review March 15, 2021 07:36
@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Mar 20, 2021

CI should pass if you rebase

The config flow tests are broken because they are using internal variables which changed. I'm fixing that, but my local tox run is taking ages.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 22, 2021

Please @bdraco when you are ready for a review

@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Mar 26, 2021

@bdraco ready for review 😄 I changed my initial posting accordingly

bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco requested changes Mar 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Nice! Please see comments above

bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
bdraco
bdraco reviewed Mar 26, 2021
View reviewed changes
@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Mar 27, 2021

Nice! Please see comments above

Thanks 😄
All done now

@ol-iver ol-iver requested a review from bdraco March 27, 2021 00:47
@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Mar 31, 2021

@bdraco finally found the memory leak. It was related to an lru cache, which is cleared now on exceptions. I just did a rebase too.

@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Mar 31, 2021

btw. it could take a while until garbage collector found the instances from the cleared cache. Thus, object count might go up and down

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 31, 2021

Thanks. I'll do testing later tonight

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 1, 2021 •
edited
Loading

The change to include the exception cause when raise ConfigEntryNotReady made it in so I think its now possible to remove a lot of the code.

Suggested change (should cherry-pick cleanly) bdraco@c94efca

2021-04-01 04:28:16 WARNING (MainThread) [homeassistant.config_entries] Config entry 'Marantz NR1711' for denonavr integration not ready yet: NetworkError: [Errno 113] Connect call failed ('192.168.107.153', 8080); Retrying in background

The great part here is it will automatically take care of downgrading retries to debug with a lot less code to maintain.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 1, 2021

Leak confirmed fixed

bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Apr 1, 2021

@bdraco cherry picking worked and so did the other changes

@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Apr 1, 2021

The change to include the exception cause when raise ConfigEntryNotReady made it in so I think its now possible to remove a lot of the code.

Suggested change (should cherry-pick cleanly) bdraco@c94efca

2021-04-01 04:28:16 WARNING (MainThread) [homeassistant.config_entries] Config entry 'Marantz NR1711' for denonavr integration not ready yet: NetworkError: [Errno 113] Connect call failed ('192.168.107.153', 8080); Retrying in background

The great part here is it will automatically take care of downgrading retries to debug with a lot less code to maintain.

Handling in Config Flow needed to be adapted slightly because it cannot handle ConfigEntryNotReady.

bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
bdraco
bdraco reviewed Apr 1, 2021
View reviewed changes
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 1, 2021 

diff --git a/.coveragerc b/.coveragerc
index dcc26036c4..f7409305cb 100644
--- a/.coveragerc
+++ b/.coveragerc
@@ -174,6 +174,7 @@ omit =
     homeassistant/components/deluge/sensor.py
     homeassistant/components/deluge/switch.py
     homeassistant/components/denon/media_player.py
+    homeassistant/components/denonavr/__init__.py
     homeassistant/components/denonavr/media_player.py
     homeassistant/components/denonavr/receiver.py
     homeassistant/components/deutsche_bahn/sensor.py

Looks like there are no tests for __init__.py so lets add that to .coveragerc

@ol-iver
Copy link
Copy Markdown
Contributor Author

ol-iver commented Apr 2, 2021

@bdraco ok, fixed and all tests have passed 😄

bdraco
bdraco approved these changes Apr 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Testing Passed.

Thanks! 👍

@bdraco bdraco merged commit eed3bfc into home-assistant:dev Apr 2, 2021
MartinHjelmare
MartinHjelmare reviewed Apr 2, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Please address the comments in a new PR. Thanks!

homeassistant/components/denonavr/media_player.py
"manufacturer": self._config_entry.data[CONF_MANUFACTURER],
"name": self._config_entry.title,
"model": f"{self._config_entry.data[CONF_MODEL]}-{self._config_entry.data[CONF_TYPE]}",
"serial_number": self._config_entry.data[CONF_SERIAL_NUMBER],
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 2, 2021

Choose a reason for hiding this comment

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

Serial number is not part of device info api:
https://developers.home-assistant.io/docs/device_registry_index#device-properties

homeassistant/components/denonavr/receiver.py
self._host = host
self._show_all_inputs = show_all_inputs
self._timeout = timeout
self._entry_state = entry_state
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 2, 2021

Choose a reason for hiding this comment

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

The entry state attribute is never used.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2021
@ol-iver ol-iver deleted the denonavr-async branch May 2, 2021 12:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@bdraco bdraco bdraco approved these changes

Assignees

No one assigned

Labels

by-code-owner cla-signed code-quality integration: denonavr new-feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ol-iver @bdraco @MartinHjelmare @homeassistant