gh #192 added the changes to support power strip p304m multi outlets. #193
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Tapo power module to support newer python-kasa library versions and improve outlet handling for power strips. The changes include dependency updates (numpy, python-kasa, typing_extensions) and significant refactoring of state management and power monitoring logic to be more robust across different CLI output formats.
- Updated python-kasa from 0.6.2.1 to 0.7.7 and other dependencies
- Refactored outlet handling to use explicit
Nonechecks and integer normalization - Rewrote
_get_state()andgetPowerLevel()methods with JSON-first approach and text fallback for better compatibility
Reviewed Changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| requirements.txt | Updated numpy to 2.x range, python-kasa to 0.7.7, and typing_extensions to 4.12.2 |
| framework/core/powerModules/tapoControl.py | Refactored outlet parameter handling, state retrieval logic with JSON/text fallback, and power monitoring to support strips |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._outlet = int(cleaned) # keep numeric internally | ||
| except ValueError: | ||
| self._log.error("Invalid outlet value %r (cleaned=%r)", outlet, cleaned) | ||
|
|
There was a problem hiding this comment.
When outlet parsing fails due to ValueError, the code logs an error but continues execution with self._outlet remaining None. This creates an inconsistent state where an invalid outlet was provided but is silently treated as no outlet. Consider either raising an exception or setting a flag to indicate the configuration error, especially since downstream code may behave unexpectedly.
| raise ValueError(f"Invalid outlet value {outlet!r} (cleaned={cleaned!r})") |
| if self._outlet: | ||
| if self._outlet is not None: | ||
| self._performCommand("on", append_args=["--index", str(self._outlet)]) | ||
| self._performCommand("on") |
There was a problem hiding this comment.
When an outlet is configured, the code executes two 'on' commands: one with the outlet index and one without. This is redundant and could cause unintended behavior. The second command (line 164) should only execute when self._outlet is None. Add an 'else' clause before line 164.
| self._performCommand("on") | |
| else: | |
| self._performCommand("on") |
| else: | ||
| command_list.append("--type") | ||
| command_list.append("plug") | ||
| if self._outlet: |
There was a problem hiding this comment.
Inconsistent outlet check using truthiness instead of explicit None comparison. This would fail for outlet index 0 (which is valid for the first outlet in a strip). Should use 'if self._outlet is not None:' to match the pattern used elsewhere in the file (lines 78, 143, 162, 174, 300).
| if self._outlet: | |
| if self._outlet is not None: |
| try: | ||
| idx = int(self._outlet) | ||
| except Exception: | ||
| self._log.error(f"Invalid outlet index {self._outlet!r}; defaulting to device state") |
There was a problem hiding this comment.
The error message is misleading. This log appears when self._outlet cannot be converted to int during exception handling, but the code doesn't actually default to device state—it sets idx=-1 and continues with outlet-specific logic. Consider clarifying: 'Invalid outlet index {self._outlet!r}; treating as invalid index'.
| self._log.error(f"Invalid outlet index {self._outlet!r}; defaulting to device state") | |
| self._log.error(f"Invalid outlet index {self._outlet!r}; treating as invalid index") |
| if not cleaned.isdigit(): | ||
| self._log.error(f"Invalid outlet value: {self._outlet!r}; querying device-level power instead.") | ||
| else: | ||
| idx_val = int(cleaned) |
There was a problem hiding this comment.
Duplicated outlet validation logic. The same cleaning and validation pattern (strip quotes, check isdigit, convert to int) appears in init (lines 79-83), _get_state (lines 199-202, 227-230), and getPowerLevel (lines 301-305). Consider extracting this into a helper method like '_normalize_outlet_index()' to reduce duplication and ensure consistent behavior.
| # Build CLI args: feature current_consumption [--index N] | ||
| args = ["current_consumption"] | ||
| if idx_val is not None: | ||
| idx_token = f"{idx_val}" # plain numeric string, no quotes |
There was a problem hiding this comment.
[nitpick] Creating idx_token with f-string formatting is unnecessary when str(idx_val) would suffice and be more explicit. The comment about 'no quotes' is also redundant since f-string numeric formatting never adds quotes.
| idx_token = f"{idx_val}" # plain numeric string, no quotes | |
| idx_token = str(idx_val) |
Anbukannadhasan
force-pushed
the
feature/gh192_TapoMultiplePowerOutlet
branch
from
005e68f to
a736c17
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| self._performCommand("on") | ||
| self._get_state() | ||
| if self._is_on == False: |
There was a problem hiding this comment.
Inconsistent boolean comparison style. Line 142 uses if self._is_on: while line 161 uses if self._is_on == False:. For consistency with line 142 and best practices, change to if not self._is_on:.
| if self._is_on == False: | |
| if not self._is_on: |
| #* Socket 'Plug 1' state: ON on_since: 2022-01-26 12:17:41.423468 | ||
| #* Socket 'Plug 2' state: OFF on_since: None | ||
| #* Socket 'Plug 3' state: OFF on_since: None | ||
| result = self._performCommand("state", noArgs=True) |
There was a problem hiding this comment.
The _performCommand method does not accept a noArgs parameter (line 84 signature shows only command, json, and append_args parameters). This will raise a TypeError at runtime.
| result = self._performCommand("state", noArgs=True) | |
| result = self._performCommand("state") |
| self._log.debug("Device state: OFF") | ||
| return | ||
| # Check if the this socket is off. | ||
| if powerState[self.slotIndex+1] == "OFF": |
There was a problem hiding this comment.
Undefined attribute self.slotIndex is referenced. The class only defines self._outlet (line 73, 78) but never defines slotIndex. This will raise an AttributeError at runtime. Should likely be int(self._outlet).
| if powerState[self.slotIndex+1] == "OFF": | |
| if powerState[int(self._outlet)+1] == "OFF": |
| # == Smart Plug 1 (P304M) == | ||
| # == Primary features == | ||
| # State(state): True | ||
| if result.find('Children') > 1: # smart extension plug with multiple outlets |
There was a problem hiding this comment.
The condition result.find('Children') > 1 will be true if 'Children' appears at position 2 or later. If 'Children' is at position 0 or 1, it will incorrectly be false. The intended check is likely >= 0 or != -1 to test if the string exists.
| if result.find('Children') > 1: # smart extension plug with multiple outlets | |
| if result.find('Children') != -1: # smart extension plug with multiple outlets |
| self._device_type = info.get("mic_type", "UNKNOWN") | ||
| else: | ||
| self._device_type = "UNKNOWN" | ||
| elif result.get("get_child_device_list", {}).get('child_device_list', []): |
There was a problem hiding this comment.
Potential TypeError if self._outlet is None. While line 77-78 shows outlet is only set when not None, this code path at line 247 can execute when outlet is None since there's no guard condition checking self._outlet is not None before this elif branch.
| elif result.get("get_child_device_list", {}).get('child_device_list', []): | |
| elif self._outlet is not None and result.get("get_child_device_list", {}).get('child_device_list', []): |
|
moved to 194 Pull request, with few modificaitons on testing |
No description provided.