RogerSelwyn / Home_Assistant_SkyQ_MediaPlayer

Home Assistant SkyQ Media player component
MIT License
101 stars 17 forks source link

[BUG] Devices discovered but the connection fails are still shown #155

Closed JSVHolmes closed 12 months ago

JSVHolmes commented 1 year ago

Describe the bug

SkyQ devices are being discovered across a bridged network interface (I assume from a multicast?), the connection fails but the devices are shown in home assistant anyway, they keep building up as each re-attempt to discover results in a new set of devices. Seems a similar to #151.

When attempting to add a device the submit button spins, but then doesn't do anything.

I believe the problem is caused by the pass on line 117 here: https://github.com/RogerSelwyn/Home_Assistant_SkyQ_MediaPlayer/blob/3b5642c2da6784d516678d9fef6286318a79ed6e/custom_components/skyq/config_flow.py#L114-L124

As a failed connection will still result in a device being discovered. I suggest moving the rest of the function into the try: and then in the except CannotConnect: cancel the discovery of the device.

I modified the function locally in config_flow.py to this:

    async def async_step_ssdp(self, discovery_info: ssdp.SsdpServiceInfo) -> FlowResult:
        """Handle a discovered device."""
        host = str(urlparse(discovery_info.ssdp_location).hostname)
        try:
            await self._async_setuniqueid(host)
            name = discovery_info.ssdp_server

            context = self.context
            context[CONF_HOST] = host
            context[CONF_NAME] = name
            return await self.async_step_confirm()
        except CannotConnect:
            _LOGGER.warning("Skipping Device: %s", host)

With this change the additional devices no longer appear, instead its a warning in the logs. Not sure if this is the best way of doing it, but it seems to work for me.

To reproduce

Waiting long enough with a bridged connection (e.g tap interface via OpenVPN), usually a few mins, then every few hours they reappear. Admittedly I have a complicated network setup so it may be hard to reproduce.

Expected bahavior

Devices that fail to connect should not be listed in Home Assistant as discovered devices.

What version of SkyQ has the issue?

v2.11.3

What was the last working version of Sky Q Integration?

No response

What version of Home Assistant Core has the issue?

core-2023.7.2

Configuration type

UI

Configuration UI

No response

Configuration YAML

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

RogerSelwyn commented 1 year ago

Away on holiday so will look when I get back. May be difficult to replicate, but I’ll digest your change to see if it will impact anything else.

RogerSelwyn commented 1 year ago

Should be fixed with - https://github.com/RogerSelwyn/Home_Assistant_SkyQ_MediaPlayer/releases/tag/v2.11.4

JSVHolmes commented 12 months ago

Seems to be working now for me, discovered devices that fail to connect are reported as a warning, thanks for the fix.