Skip to content

Add ability to ignore heos discovery#34653

Merged
andrewsayre merged 8 commits intohome-assistant:devfrom
bdraco:heos_ability_to_ignore
May 26, 2020
Merged

Add ability to ignore heos discovery#34653
andrewsayre merged 8 commits intohome-assistant:devfrom
bdraco:heos_ability_to_ignore

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Apr 24, 2020

Proposed change

Add ability to ignore heos discovery

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:

# Example configuration.yaml

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
Copy Markdown

Hey there @andrewsayre, mind taking a look at this pull request as its been labeled with a integration (heos) you are listed as a codeowner for? Thanks!

Copy link
Copy Markdown
Member

@andrewsayre andrewsayre left a comment

Choose a reason for hiding this comment

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

I'm not seeing how the title of the PR matches the changes as no tests were added that confirms this behavior. Here are some things to consider:

  1. HEOS devices do not consistently provide a serial number through discovery or the API, so this isn't a suitable candidate for unique_id.
  2. You aren't supporting multiple device scenarios. A single config entry brings in all HEOS devices on the local network (whether configured manually or detected through discovery). The user selects the specific device in the user step. Again, using serial number (even if reliably available) is problematic as the other discovered devices will still show up will have a different serial number.
  3. A unique_id needs to be set from all entry points -- not just from discovery.
  4. Tests need to be added to show that this "ignores" discovered items.

I suggest you explore using player_id, which is available in the API to form a unique_id. You will also need to keep track of all the player IDs to check against in multi-device scenarios.

Comment thread homeassistant/components/heos/config_flow.py Outdated
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Apr 24, 2020

CI is currently broken due to unrelated issue
ImportError: libavfilter.so.6: cannot open shared object file: No such file or directory

Copy link
Copy Markdown
Member

@andrewsayre andrewsayre left a comment

Choose a reason for hiding this comment

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

The referenced arch issue helped me understand what is trying to be accomplished. Good/bad news: I think this can be greatly simplified. Since HEOS only supports a single config entry, we can simply set a static unique ID on the entry. Let's just set it to the DOMAIN constant and update the existing test to validate it's being set and make sure we handle setting the unique ID in all cases:

  • Call await self.async_set_unique_id(DOMAIN) from async_step_ssdp, async_step_import, and async_step_user
  • Update existing entries as the first code in async_setup_entry:
# For backwards compat 
 if entry.unique_id is None: 
     hass.config_entries.async_update_entry( 
         entry, unique_id=DOMAIN 
     ) 

Sorry for sending you on a wild goose chase!

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Apr 29, 2020

Will come back to this as soon as I can. I've been focused on the CI failures which are now fixed (I hope)

@bdraco bdraco force-pushed the heos_ability_to_ignore branch 3 times, most recently from dcc9ebf to 84a30c3 Compare May 3, 2020 15:30
@bdraco bdraco requested a review from andrewsayre May 3, 2020 17:45
Comment thread homeassistant/components/heos/config_flow.py Outdated
Comment thread homeassistant/components/heos/config_flow.py Outdated
Comment thread tests/components/heos/conftest.py Outdated
@bdraco bdraco force-pushed the heos_ability_to_ignore branch from 0c18bc5 to c6cfcb5 Compare May 25, 2020 17:47
@andrewsayre andrewsayre added this to the 0.110.4 milestone May 26, 2020
@andrewsayre andrewsayre merged commit 9a53240 into home-assistant:dev May 26, 2020
balloob pushed a commit that referenced this pull request May 28, 2020
* Add ability to ignore heos discovery

* Fetch player_id, update tests

* Handle failure state

* Update tests as there are two players in the mock now

* Adjust and add more tests

* Strip out player id lookup

* reverts per review

* one more revert
@balloob balloob mentioned this pull request May 28, 2020
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.

Denon HEOS Discovery still not ignorable in 0.110.0

4 participants