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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle zeroconf updated events #47683

Merged
merged 1 commit into from Mar 9, 2021
Merged

Handle zeroconf updated events #47683

merged 1 commit into from Mar 9, 2021

Conversation

balloob
Copy link
Member

@balloob balloob commented Mar 9, 2021

Breaking change

Proposed change

I had a case where my Elgato keylight air was unable to get an IP through DHCP so assigned itself a manual IP and started broadcasting that in Zeroconf. This updated my config entry. Afterwards it was able to recover and get an IP and updated it's discovery data. This update was ignored and my keylight did not turn on during my call this morning 馃槶

I had this code running for a while and didn't see excessive updates coming in. Pining @bdraco as his network is wild and maybe he can see more updates coming in.

The filter code was initially added to prevent calling it multiple times, but this is better deduplicated by a config flow.

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

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

@balloob balloob requested a review from bdraco March 9, 2021 18:45
@project-bot project-bot bot added this to Needs review in Dev Mar 9, 2021
@bdraco
Copy link
Member

bdraco commented Mar 9, 2021

Will test soon. I think this will be fine since we pre-filter update_record

@bdraco
Copy link
Member

bdraco commented Mar 9, 2021

Well I was wrong about that, it looks like it will generate multiple orders of magnitude more updates. I'll see if I can optimize it a bit when I have more time later tonight

@balloob
Copy link
Member Author

balloob commented Mar 9, 2021

What services do you see that are doing updates? Can you see the values that have been updated?

Maybe we should just filter it out unless we see an IP change

@bdraco
Copy link
Member

bdraco commented Mar 9, 2021

Its mostly _hap updates (homekit)

Testing on another network now....

Might be due to running an old version of homebridge (https://www.npmjs.com/package/bonjour-hap)

The library heavily congest the network with unnecessary traffic....

The bonjour-hap library was used in HAP-NodeJS in versions prior to v0.8.0. bonjour-hap does NOT correctly implement mdns service discovery as it does not correctly implement RFC 6762 or RFC 6763. The library heavily congest the network with unnecessary traffic, does not implement certain features and does not behave as expected from a mdns responder (or querier). It SHOULD NOT be used anymore. It is not maintained anymore. The code should not be taken as reference for anything.

bonjour-hap was replaced by the ciao library, which is basically a rewrite and strongly adheres to the mentioned RFCs. Ciao is used in HAP-NodeJS since v0.8.0.

Copy link
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.

The increase should be minimal as long as they aren't running and old version of homebridge https://www.npmjs.com/package/bonjour-hap which is a known issue with that library (https://www.npmjs.com/package/bonjour-hap): The library heavily congest the network with unnecessary traffic

That's not home assistant's fault so I don't think its something we need to accomodate, and has already been fixed in newer versions.

Dev automation moved this from Needs review to Reviewer approved Mar 9, 2021
@bdraco bdraco merged commit 46e5934 into dev Mar 9, 2021
Dev automation moved this from Reviewer approved to Done Mar 9, 2021
@bdraco bdraco deleted the zeroconf-include-updated branch March 9, 2021 20:14
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants