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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance #108

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Improve performance #108

merged 1 commit into from
Feb 8, 2022

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Feb 8, 2022

  • Improves performance when bulb is offline

  • Fixes python 3.8 typing compat

  • Fixes race condition during shutdown

  • Fixes memory leak in discovery

  • Fixes slow tests

  • Adds test runs to the CI

  • No longer respond to syncPilot messages since the
    bulb keeps sending them anyways and all it does
    is generate additional network congestion

fixes #107, fixes #106, fixes #105

@bdraco bdraco force-pushed the time_performance branch 8 times, most recently from c93a965 to 206cc9f Compare February 8, 2022 18:06
@sbidy
Copy link
Owner

sbidy commented Feb 8, 2022

@bdraco We should be aware of the #109. The IP can also be a DNS name. Especially in the HASS config flow if I enter a DNS name of a bulb manually.

self.ip = ip

I would suggest something like:

def _resolve_dnsname(self, dns_or_ip: str):
        """Returns a IP in case of a DNS was given."""
        try:
            return socket.gethostbyname(dns_or_ip)
        except socket.gaierror:
            raise WizLightConnectionError("DNS name can not be resloved.")

@bdraco bdraco changed the title Improve performance with bulb is offline Improve performance Feb 8, 2022
@bdraco
Copy link
Contributor Author

bdraco commented Feb 8, 2022

@bdraco We should be aware of the #109. The IP can also be a DNS name. Especially in the HASS config flow if I enter a DNS name of a bulb manually.

Usually we don't want to accept dns names since if dns is having issues the device can't be setup. As soon as discovery sees the bulb it will update it to an ip address via

https://github.com/home-assistant/core/blob/dev/homeassistant/components/wiz/config_flow.py#L61

This takes care of the case where the bulb's dhcp reservation changes.

def _resolve_dnsname(self, dns_or_ip: str):
        """Returns a IP in case of a DNS was given."""
        try:
            return socket.gethostbyname(dns_or_ip)
        except socket.gaierror:
            raise WizLightConnectionError("DNS name can not be resloved.")

If we do want to do allow a hostname, we need to use loop.getaddrinfo() since it is async safe.

- Fixes python 3.8 typing compat

- Fixes race condition during shutdown

- Fixes memory leak in discovery

- Fixes slow tests

- No longer respond to syncPilot messages since the
  bulb keeps sending them anyways and all it does
  is generate additional network congestion

- Adds test runs to the CI
@sbidy
Copy link
Owner

sbidy commented Feb 8, 2022

@bdraco We should be aware of the #109. The IP can also be a DNS name. Especially in the HASS config flow if I enter a DNS name of a bulb manually.

Usually we don't want to accept dns names since if dns is having issues the device can't be setup. As soon as discovery sees the bulb it will update it to an ip address via

https://github.com/home-assistant/core/blob/dev/homeassistant/components/wiz/config_flow.py#L61

This takes care of the case where the bulb's dhcp reservation changes.

def _resolve_dnsname(self, dns_or_ip: str):
        """Returns a IP in case of a DNS was given."""
        try:
            return socket.gethostbyname(dns_or_ip)
        except socket.gaierror:
            raise WizLightConnectionError("DNS name can not be resloved.")

If we do want to do allow a hostname, we need to use loop.getaddrinfo() since it is async safe.

Ok, that makes more sense in general. I'll make the documentation clear that the library (natively) only supports IPs as input for the wizlight class.

The HASS strings should be updated to "IP" because the "IP/Host" can be misleading for the user.

@bdraco
Copy link
Contributor Author

bdraco commented Feb 8, 2022

The HASS strings should be updated to "IP" because the "IP/Host" can be misleading for the user.

Yeah, I that makes more sense. There is probably no point in resolving it for them since they can leave it blank and do discovery anyways. We should change the string though and reject anything thats not an Ip

@bdraco
Copy link
Contributor Author

bdraco commented Feb 8, 2022

Screen Shot 2022-02-08 at 13 07 18

This is looking really efficient now. The debug logging is taking far more time than the actual protocol ops

@sbidy
Copy link
Owner

sbidy commented Feb 8, 2022

Yes, that looks really great!

For the changes in the ConfiFlow and the IP-Address input validation, I'll open a new PR to the core.

@bdraco
Copy link
Contributor Author

bdraco commented Feb 8, 2022

Screen Shot 2022-02-08 at 13 11 01

The offline case looks a lot better as well with sendto being the bulk of the time. We can't improve that though since its low level

@bdraco bdraco marked this pull request as ready for review February 8, 2022 19:11
@sbidy sbidy merged commit 36b483d into sbidy:master Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants