From 0521902d5cacc52729e50e544fb7eb23cdf570f0 Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Fri, 25 Aug 2023 16:06:24 -0500 Subject: [PATCH 1/6] Split module update to avoid large output The KL-125(US) bulb appears to crash if a request is made to it which produces an output large than 4096 bytes. This commit splits up the module update based on the expected response size to avoid making a request which will overflow the device's output buffer. --- kasa/modules/module.py | 9 +++++++++ kasa/modules/usage.py | 5 +++++ kasa/protocol.py | 1 + kasa/smartdevice.py | 17 +++++++++++++++-- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/kasa/modules/module.py b/kasa/modules/module.py index 7340d7e11..3fdf2c48a 100644 --- a/kasa/modules/module.py +++ b/kasa/modules/module.py @@ -43,6 +43,15 @@ def query(self): queries to the query that gets executed when Device.update() gets called. """ + @property + def query_response_size(self): + """Estimated maximum size of query response + + The inheriting modules implement this to estimate how large a query response + will be so that queries can be split should an estimated response be too large + """ + return 256 # Estimate for modules that don't specify + @property def data(self): """Return the module specific raw data from the last update.""" diff --git a/kasa/modules/usage.py b/kasa/modules/usage.py index d1f96e7e4..a70e96a0f 100644 --- a/kasa/modules/usage.py +++ b/kasa/modules/usage.py @@ -21,6 +21,11 @@ def query(self): return req + @property + def query_response_size(self): + """ Estimated maximum requery response size """ + return 2048 + @property def daily_data(self): """Return statistics on daily basis.""" diff --git a/kasa/protocol.py b/kasa/protocol.py index 461dd85ad..b3bad7266 100755 --- a/kasa/protocol.py +++ b/kasa/protocol.py @@ -34,6 +34,7 @@ class TPLinkSmartHomeProtocol: INITIALIZATION_VECTOR = 171 DEFAULT_PORT = 9999 + MAX_RESPONSE_SIZE = 4096 DEFAULT_TIMEOUT = 5 BLOCK_SIZE = 4 diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index 5c24c943e..1c264b7b7 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -337,20 +337,33 @@ async def _modular_update(self, req: dict) -> None: ) self.add_module("emeter", Emeter(self, self.emeter_type)) + request_list = [] + est_response_size = 0 + modules_to_skip = MODEL_MODULE_SKIPLIST.get(self.model, []) for module_name, module in self.modules.items(): if not module.is_supported: _LOGGER.debug("Module %s not supported, skipping" % module) continue - modules_to_skip = MODEL_MODULE_SKIPLIST.get(self.model, []) if module_name in modules_to_skip: _LOGGER.debug(f"Module {module} is excluded for {self.model}, skipping") continue + est_response_size += module.query_response_size + if est_response_size > TPLinkSmartHomeProtocol.MAX_RESPONSE_SIZE: + request_list.append( req ) + req = {} + est_response_size = module.query_response_size + q = module.query() _LOGGER.debug("Adding query for %s: %s", module, q) req = merge(req, q) + request_list.append(req) - self._last_update = await self.protocol.query(req) + responses = [await self.protocol.query(request) for request in request_list] + update = {} + for response in responses: + update = merge(update, response) + self._last_update = update def update_from_discover_info(self, info): """Update state from info from the discover call.""" From 4ff74ac334cd4c88d4ede59c1bc9e76d865db5b5 Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Sun, 27 Aug 2023 15:52:52 -0500 Subject: [PATCH 2/6] Make maximum response size a 'device' property Move maximum response size from a 'Protocol' property to a 'Device' property, defaulting to 16K. Some bulbs, eg KL-125US, are known to have smaller buffers, so bulbs are set to 4k. --- kasa/protocol.py | 1 - kasa/smartbulb.py | 5 +++++ kasa/smartdevice.py | 9 +++++++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/kasa/protocol.py b/kasa/protocol.py index b3bad7266..461dd85ad 100755 --- a/kasa/protocol.py +++ b/kasa/protocol.py @@ -34,7 +34,6 @@ class TPLinkSmartHomeProtocol: INITIALIZATION_VECTOR = 171 DEFAULT_PORT = 9999 - MAX_RESPONSE_SIZE = 4096 DEFAULT_TIMEOUT = 5 BLOCK_SIZE = 4 diff --git a/kasa/smartbulb.py b/kasa/smartbulb.py index 2d2f28ca0..88ec2993c 100644 --- a/kasa/smartbulb.py +++ b/kasa/smartbulb.py @@ -543,3 +543,8 @@ async def save_preset(self, preset: SmartBulbPreset): return await self._query_helper( self.LIGHT_SERVICE, "set_preferred_state", preset.dict(exclude_none=True) ) + + @property + def max_response_payload_size(self) -> int: + """Returns the maximum response size the device can safely construct""" + return 4096 diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index 1c264b7b7..0efe67768 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -349,7 +349,7 @@ async def _modular_update(self, req: dict) -> None: continue est_response_size += module.query_response_size - if est_response_size > TPLinkSmartHomeProtocol.MAX_RESPONSE_SIZE: + if est_response_size > self.max_response_payload_size: request_list.append( req ) req = {} est_response_size = module.query_response_size @@ -359,7 +359,7 @@ async def _modular_update(self, req: dict) -> None: req = merge(req, q) request_list.append(req) - responses = [await self.protocol.query(request) for request in request_list] + responses = [await self.protocol.query(request) for request in request_list if request] update = {} for response in responses: update = merge(update, response) @@ -671,6 +671,11 @@ def get_plug_by_index(self, index: int) -> "SmartDevice": ) return self.children[index] + @property + def max_response_payload_size(self) -> int: + """Returns the maximum response size the device can safely construct""" + return 16*1024 + @property def device_type(self) -> DeviceType: """Return the device type.""" From 8cf4a362af8da4b461bd57b6482d5a9660f9de65 Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Wed, 30 Aug 2023 00:18:36 -0500 Subject: [PATCH 3/6] Lint and fix tests Run linter, fix response deep-copying making tests unhappy, updated call count in emeter test. --- kasa/modules/module.py | 4 ++-- kasa/modules/usage.py | 2 +- kasa/smartbulb.py | 2 +- kasa/smartdevice.py | 15 +++++++++------ kasa/tests/test_smartdevice.py | 4 +++- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/kasa/modules/module.py b/kasa/modules/module.py index 3fdf2c48a..e7090c84a 100644 --- a/kasa/modules/module.py +++ b/kasa/modules/module.py @@ -45,12 +45,12 @@ def query(self): @property def query_response_size(self): - """Estimated maximum size of query response + """Estimated maximum size of query response. The inheriting modules implement this to estimate how large a query response will be so that queries can be split should an estimated response be too large """ - return 256 # Estimate for modules that don't specify + return 256 # Estimate for modules that don't specify @property def data(self): diff --git a/kasa/modules/usage.py b/kasa/modules/usage.py index a70e96a0f..6174f5bcf 100644 --- a/kasa/modules/usage.py +++ b/kasa/modules/usage.py @@ -23,7 +23,7 @@ def query(self): @property def query_response_size(self): - """ Estimated maximum requery response size """ + """Estimated maximum query response size.""" return 2048 @property diff --git a/kasa/smartbulb.py b/kasa/smartbulb.py index 88ec2993c..312bcb5ce 100644 --- a/kasa/smartbulb.py +++ b/kasa/smartbulb.py @@ -546,5 +546,5 @@ async def save_preset(self, preset: SmartBulbPreset): @property def max_response_payload_size(self) -> int: - """Returns the maximum response size the device can safely construct""" + """Returns the maximum response size the device can safely construct.""" return 4096 diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index 0efe67768..a897e41e8 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -350,7 +350,7 @@ async def _modular_update(self, req: dict) -> None: est_response_size += module.query_response_size if est_response_size > self.max_response_payload_size: - request_list.append( req ) + request_list.append(req) req = {} est_response_size = module.query_response_size @@ -359,10 +359,13 @@ async def _modular_update(self, req: dict) -> None: req = merge(req, q) request_list.append(req) - responses = [await self.protocol.query(request) for request in request_list if request] - update = {} + responses = [ + await self.protocol.query(request) for request in request_list if request + ] + + update: Dict = {} for response in responses: - update = merge(update, response) + update = {**update, **response} self._last_update = update def update_from_discover_info(self, info): @@ -673,8 +676,8 @@ def get_plug_by_index(self, index: int) -> "SmartDevice": @property def max_response_payload_size(self) -> int: - """Returns the maximum response size the device can safely construct""" - return 16*1024 + """Returns the maximum response size the device can safely construct.""" + return 16 * 1024 @property def device_type(self) -> DeviceType: diff --git a/kasa/tests/test_smartdevice.py b/kasa/tests/test_smartdevice.py index 0839bc06f..083f63e32 100644 --- a/kasa/tests/test_smartdevice.py +++ b/kasa/tests/test_smartdevice.py @@ -39,7 +39,9 @@ async def test_initial_update_emeter(dev, mocker): dev._last_update = None spy = mocker.spy(dev.protocol, "query") await dev.update() - assert spy.call_count == 2 + len(dev.children) + # Devices with small buffers may require 3 queries + expected_queries = 2 if dev.max_response_payload_size > 4096 else 3 + assert spy.call_count == expected_queries + len(dev.children) @no_emeter From c851c16bb2480cae031089be8000ff670a979c73 Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Wed, 30 Aug 2023 13:05:47 -0500 Subject: [PATCH 4/6] Account for initial sysinfo size --- kasa/smartdevice.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index a897e41e8..79212717a 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -338,7 +338,7 @@ async def _modular_update(self, req: dict) -> None: self.add_module("emeter", Emeter(self, self.emeter_type)) request_list = [] - est_response_size = 0 + est_response_size = 1024 if "system" in req else 0 modules_to_skip = MODEL_MODULE_SKIPLIST.get(self.model, []) for module_name, module in self.modules.items(): if not module.is_supported: From 188f64ac725d9e318da0737ea30704cb8ac8c6ef Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Thu, 14 Sep 2023 11:24:10 -0500 Subject: [PATCH 5/6] Address review comments --- kasa/modules/module.py | 2 +- kasa/modules/usage.py | 2 +- kasa/smartbulb.py | 2 +- kasa/smartdevice.py | 11 ++++------- kasa/tests/test_smartdevice.py | 13 ++++++++++++- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/kasa/modules/module.py b/kasa/modules/module.py index e7090c84a..ff806a997 100644 --- a/kasa/modules/module.py +++ b/kasa/modules/module.py @@ -44,7 +44,7 @@ def query(self): """ @property - def query_response_size(self): + def estimated_query_response_size(self): """Estimated maximum size of query response. The inheriting modules implement this to estimate how large a query response diff --git a/kasa/modules/usage.py b/kasa/modules/usage.py index 6174f5bcf..9877795dd 100644 --- a/kasa/modules/usage.py +++ b/kasa/modules/usage.py @@ -22,7 +22,7 @@ def query(self): return req @property - def query_response_size(self): + def estimated_query_response_size(self): """Estimated maximum query response size.""" return 2048 diff --git a/kasa/smartbulb.py b/kasa/smartbulb.py index 312bcb5ce..a09487d26 100644 --- a/kasa/smartbulb.py +++ b/kasa/smartbulb.py @@ -545,6 +545,6 @@ async def save_preset(self, preset: SmartBulbPreset): ) @property - def max_response_payload_size(self) -> int: + def max_device_response_size(self) -> int: """Returns the maximum response size the device can safely construct.""" return 4096 diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index 79212717a..67136ea3a 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -28,9 +28,6 @@ _LOGGER = logging.getLogger(__name__) -# Certain module queries will crash devices; this list skips those queries -MODEL_MODULE_SKIPLIST = {"KL125(US)": ["cloud"]} # Issue #345 - class DeviceType(Enum): """Device type enum.""" @@ -348,11 +345,11 @@ async def _modular_update(self, req: dict) -> None: _LOGGER.debug(f"Module {module} is excluded for {self.model}, skipping") continue - est_response_size += module.query_response_size - if est_response_size > self.max_response_payload_size: + est_response_size += module.estimated_query_response_size + if est_response_size > self.max_device_response_size: request_list.append(req) req = {} - est_response_size = module.query_response_size + est_response_size = module.estimated_query_response_size q = module.query() _LOGGER.debug("Adding query for %s: %s", module, q) @@ -675,7 +672,7 @@ def get_plug_by_index(self, index: int) -> "SmartDevice": return self.children[index] @property - def max_response_payload_size(self) -> int: + def max_device_response_size(self) -> int: """Returns the maximum response size the device can safely construct.""" return 16 * 1024 diff --git a/kasa/tests/test_smartdevice.py b/kasa/tests/test_smartdevice.py index 083f63e32..c70f544b2 100644 --- a/kasa/tests/test_smartdevice.py +++ b/kasa/tests/test_smartdevice.py @@ -40,7 +40,7 @@ async def test_initial_update_emeter(dev, mocker): spy = mocker.spy(dev.protocol, "query") await dev.update() # Devices with small buffers may require 3 queries - expected_queries = 2 if dev.max_response_payload_size > 4096 else 3 + expected_queries = 2 if dev.max_device_response_size > 4096 else 3 assert spy.call_count == expected_queries + len(dev.children) @@ -166,6 +166,17 @@ async def test_features(dev): assert dev.features == set() +async def test_max_device_response_size(dev): + """Make sure every device return has a set max response size.""" + assert dev.max_device_response_size > 0 + + +async def test_estimated_response_sizes(dev): + """Make sure every module has an estimated response size set.""" + for mod in dev.modules.values(): + assert mod.estimated_query_response_size > 0 + + @pytest.mark.parametrize("device_class", smart_device_classes) def test_device_class_ctors(device_class): """Make sure constructor api not broken for new and existing SmartDevices.""" From 2712af891dcbb8315d63dd028aeb8ab313ad2936 Mon Sep 17 00:00:00 2001 From: Chris OBryan <13701027+cobryan05@users.noreply.github.com> Date: Thu, 14 Sep 2023 11:24:16 -0500 Subject: [PATCH 6/6] Remove module skip list This commit removes the module skip list as it seems this workaround is no longer necessary when splitting up large queries. --- kasa/smartdevice.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/kasa/smartdevice.py b/kasa/smartdevice.py index 67136ea3a..81ea135be 100755 --- a/kasa/smartdevice.py +++ b/kasa/smartdevice.py @@ -336,14 +336,10 @@ async def _modular_update(self, req: dict) -> None: request_list = [] est_response_size = 1024 if "system" in req else 0 - modules_to_skip = MODEL_MODULE_SKIPLIST.get(self.model, []) for module_name, module in self.modules.items(): if not module.is_supported: _LOGGER.debug("Module %s not supported, skipping" % module) continue - if module_name in modules_to_skip: - _LOGGER.debug(f"Module {module} is excluded for {self.model}, skipping") - continue est_response_size += module.estimated_query_response_size if est_response_size > self.max_device_response_size: