sbidy / pywizlight

A python connector for WiZ devices
MIT License
463 stars 79 forks source link

Improve performance #108

Closed bdraco closed 2 years ago

bdraco commented 2 years ago

fixes #107, fixes #106, fixes #105

sbidy commented 2 years ago

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

https://github.com/sbidy/pywizlight/blob/cb6c00109fbbc18c7b0bf99466a0909fcaec1529/pywizlight/bulb.py#L301

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 commented 2 years ago

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

sbidy commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago
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 commented 2 years ago

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 commented 2 years ago
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