GeezerVison

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (30f217b) 81.63% compared to head (19bca91) 81.74%.

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   81.63%   81.74%   +0.11%     
==========================================
  Files          28       28              
  Lines        2417     2421       +4     
  Branches      683      685       +2     
==========================================
+ Hits         1973     1979       +6     
+ Misses        383      382       -1     
+ Partials       61       60       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bdraco friendly ping to keep you in the loop.

I'm not sure we can take the device update out without breaking the power strips, unless we pass the device type in. As soon as I get home, I'll review this and tested, as you may already be doing that and I just can't see it on my mobile.

All the child plugs show as unavailable without the update

The config entry sets up ok, but everything is unavailable

diff --git a/kasa/discover.py b/kasa/discover.py
index 9883ad2..2ce408d 100755
--- a/kasa/discover.py
+++ b/kasa/discover.py
@@ -326,6 +326,11 @@ class Discover:
         if ip in protocol.discovered_devices:
             dev = protocol.discovered_devices[ip]
             dev.host = host
+            # If the device has children we have
+            # to update the device since we will
+            # otherwise have missing children
+            if dev.sys_info.get("child_num"):
+                await dev.update()
             return dev
         elif ip in protocol.unsupported_devices:
             raise UnsupportedDeviceException(

This fixes the powerstrips. It would be better to abstract this as a property in the smart device object

The power strips are really poorly responsive since we switch to the 5s interval because it updates all the children every time one is switched

Also it looks like setting up devices across subnets no longer works with since the switch to UDP

Actually that one is on the same subnet 😢

The device sets up fine if I change the timeout to 10

--- a/kasa/discover.py
+++ b/kasa/discover.py
@@ -258,7 +258,7 @@ class Discover:
         host: str,
         *,
         port: Optional[int] = None,
-        timeout=5,
+        timeout=10,
         credentials: Optional[Credentials] = None,
     ) -> SmartDevice:
         """Discover a single device by the given IP address.

I fixed the power strips being overloaded with too many requests in home-assistant/core#103668

diff --git a/kasa/discover.py b/kasa/discover.py
index 9883ad2..2ce408d 100755
--- a/kasa/discover.py
+++ b/kasa/discover.py
@@ -326,6 +326,11 @@ class Discover:
         if ip in protocol.discovered_devices:
             dev = protocol.discovered_devices[ip]
             dev.host = host
+            # If the device has children we have
+            # to update the device since we will
+            # otherwise have missing children
+            if dev.sys_info.get("child_num"):
+                await dev.update()
             return dev
         elif ip in protocol.unsupported_devices:
             raise UnsupportedDeviceException(

This fixes the powerstrips. It would be better to abstract this as a property in the smart device object

Rather than doing it that way I've included a check to see if the type is SmartStrip. This is to future proof for klap/tuya devices that will update_from_discover_info() with a limited set of properties. It also seems simpler than adding a new property to the SmartDevice. kwarg: update_parent_devices: bool = True,

  dev.host = host
            # Call device update on devices that have children
            if update_parent_devices and isinstance(dev, SmartStrip):
                await dev.update()
            return dev

kwarg: update_parent_devices defaults to True

Actually that one is on the same subnet 😢

I don't anticipate subnet related issues with UDP discover_single() because it sets the target ip to the host IP (no 255s). This should be able to go across subnets because it's not a broadcast message.

Rather than doing it that way I've included a check to see if the type is SmartStrip. This is to future proof for klap/tuya devices that will update_from_discover_info() with a limited set of properties.

I think this will still break with any devices that have children that are not smart strips since its the children that need to be updated. I don't think there are any other devices that I have that have children though

I couldn’t see children properties on any other device types. Do you think there are others? Maybe the smartlightstrip?

I think only tplink can answer that, but they all have child_num set so I think its safer to check that as it better shows the intent.

suggested change bdraco@638a9b5

@sdb9696 Can you fix the conflicts here?

I will revert HA to use this and than test after dinner

@sdb9696 Can you fix the conflicts here?

Ok that's done and includes your suggested changes for adding the haschildren property. It will also call device.update() for the new klap protocol because that update is required to access the child_num setting.

It will also call device.update() for the new klap protocol because that update is required to access the child_num setting.

Are there any klap devices that have children that we know of at this point?

Are there any klap devices that have children that we know of at this point?

Not that we know of.

I think its ok to avoid the update for the klap devices unless its otherwise needed. We can always add support for klap devices with children in a future update

But you can't access the child_num setting until an update has occurred. It will give an error on all klap devices regardless of whether they have children or not.

It looks like we call update_from_discover_info for klap devices as well. Does self._last_update = info not get set for these devices ?

It looks like we call update_from_discover_info for klap devices as well. Does self._last_update = info not get set for these devices ?

It's a limited set of properties, not the full sys_info list. If you try to access properties not in the limited list it will error.

It looks like we call update_from_discover_info for klap devices as well. Does self._last_update = info not get set for these devices ?

It's a limited set of properties, not the full sys_info list. If you try to access properties not in the limited list it will error.

Thats annoying. Since we only support children on the power strips, maybe go back to what you had before but move it behind the has_children property and take off @requires_update?

It looks like we call update_from_discover_info for klap devices as well. Does self._last_update = info not get set for these devices ?

It's a limited set of properties, not the full sys_info list. If you try to access properties not in the limited list it will error.

Thats annoying. Since we only support children on the power strips, maybe go back to what you had before but move it behind the has_children property and take off @requires_update?

So the has_children property in smartdevice checks if it’s a lightstrip instance instead of looking at child_num?

So the has_children property in smartdevice checks if it’s a lightstrip instance instead of looking at child_num?

Its the only the power strips (DeviceType.Strip) that have children (yes the names are confusing) not the light strips (DeviceType.LightStrip)

Something like this

diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py
index 6461f33..d019912 100755
--- a/kasa/smartdevice.py
+++ b/kasa/smartdevice.py
@@ -411,11 +411,14 @@ class SmartDevice:
         sys_info = self._sys_info
         return str(sys_info["model"])
 
-    @property  # type: ignore
-    @requires_update
+    @property
     def has_children(self) -> bool:
         """Check if the device has children devices."""
-        return bool(self._sys_info.get("child_num"))
+        # Ideally we would check for the 'child_num' key in sys_info,
+        # but devices that speak klap do not populate this key via
+        # update_from_discover_info so we check for the devices
+        # we know have children instead.
+        return self.is_strip
 
     @property  # type: ignore
     @requires_update

So the has_children property in smartdevice checks if it’s a lightstrip instance instead of looking at child_num?

Its the only the power strips (DeviceType.Strip) that have children (yes the names are confusing) not the light strips (DeviceType.LightStrip)

Something like this

diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py
index 6461f33..d019912 100755
--- a/kasa/smartdevice.py
+++ b/kasa/smartdevice.py
@@ -411,11 +411,14 @@ class SmartDevice:
         sys_info = self._sys_info
         return str(sys_info["model"])
 
-    @property  # type: ignore
-    @requires_update
+    @property
     def has_children(self) -> bool:
         """Check if the device has children devices."""
-        return bool(self._sys_info.get("child_num"))
+        # Ideally we would check for the 'child_num' key in sys_info,
+        # but devices that speak klap do not populate this key via
+        # update_from_discover_info so we check for the devices
+        # we know have children instead.
+        return self.is_strip
 
     @property  # type: ignore
     @requires_update

Yes sorry, meant strip. Makes sense, will update tomorrow am.

Ok that's done

Took a few tries, but the power strip did eventually set up so this is good to go.