Improve P1 fault-handling #670
Conversation
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes in this pull request involve modifications to several Python files related to the Plugwise smart home system. Key updates include the introduction of new methods in the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bouwew
force-pushed
the
try-something
branch
from
c904402 to
a77b753
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
plugwise/helper.py (1)
Line range hint
357-366: Consider optimizing the entity sorting implementation.While the current implementation works, it could be more efficient by using a single pass approach instead of multiple dictionary operations.
Consider this alternative implementation:
- def _sort_gw_entities(self) -> None: - """Place the gateway and optional heater_central devices as 1st and 2nd.""" - for dev_class in ("heater_central", "gateway"): - for entity_id, entity in dict(self.gw_entities).items(): - if entity["dev_class"] == dev_class: - tmp_entity = entity - self.gw_entities.pop(entity_id) - cleared_dict = self.gw_entities - add_to_front = {entity_id: tmp_entity} - self.gw_entities = {**add_to_front, **cleared_dict} + def _sort_gw_entities(self) -> None: + """Place the gateway and optional heater_central devices as 1st and 2nd.""" + priority_entities = {} + other_entities = {} + + for entity_id, entity in self.gw_entities.items(): + if entity["dev_class"] in ("gateway", "heater_central"): + priority_entities[entity_id] = entity + else: + other_entities[entity_id] = entity + + self.gw_entities = {**priority_entities, **other_entities}This approach:
- Uses a single pass through the entities
- Reduces dictionary operations
- Is more memory efficient
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
plugwise/helper.py(2 hunks)plugwise/legacy/smile.py(2 hunks)plugwise/smile.py(2 hunks)tests/test_adam.py(1 hunks)tests/test_p1.py(2 hunks)userdata/error/p1v4_442_single/core.domain_objects.xml(1 hunks)userdata/reboot/p1v4_442_single/core.domain_objects.xml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- userdata/reboot/p1v4_442_single/core.domain_objects.xml
- userdata/error/p1v4_442_single/core.domain_objects.xml
🔇 Additional comments (9)
tests/test_p1.py (2)
44-51: LGTM: Good error handling test coverage for reboot scenario
The test properly verifies the handling of missing XML data after a reboot request.
53-58: LGTM: Good error handling test coverage for error XML data
The test properly verifies the handling of error XML responses from the gateway.
plugwise/legacy/smile.py (2)
123-129: LGTM: Robust error handling for full update
The code properly catches KeyError and raises a more descriptive DataMissingError, improving error handling during full updates.
132-144: LGTM: Robust error handling for incremental update
The code properly catches KeyError and raises a more descriptive DataMissingError, improving error handling during incremental updates.
tests/test_adam.py (1)
140-143: LGTM: Improved error logging
The test now includes the specific error message in the debug log, providing better context for debugging.
plugwise/smile.py (2)
Line range hint 134-145: LGTM: Clear comment and proper error handling.
The comment clearly explains the requirement for cooling_enabled and the code properly checks for failed data retrieval.
145-148: Verify P1 data retrieval fallback.
The else clause provides a fallback for P1 devices by attempting to retrieve the location from gw_entities[self.gateway_id]. However, we should verify that this fallback works correctly.
✅ Verification successful
Let me gather more context about the P1 data retrieval and error handling patterns.
Let me check the P1-specific test files to understand how the error handling is tested.
Based on the gathered information, I can now provide a final response:
P1 data retrieval fallback is properly implemented and tested.
The code shows consistent error handling patterns across both modern and legacy implementations. The P1 test suite (test_p1.py) includes comprehensive test scenarios:
- Normal operation with single and triple phase setups
- Data updates after intervals
- Error scenarios including no data after reboot
- XML error responses
The fallback mechanism using gw_entities[gateway_id]["location"] is present in both modern and legacy code paths, with appropriate error handling raising DataMissingError when the data is unavailable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for P1-related error handling patterns
# Test: Look for similar error handling patterns in tests
rg -A 5 "DataMissingError.*No Plugwise actual data received"
Length of output: 362
Script:
#!/bin/bash
# Check for P1-related code patterns and error handling
ast-grep --pattern 'class $_ {
$$$
def _get_data(self, $$$) {
$$$
}
$$$
}'
# Search for P1 related tests
rg -l "P1" tests/
Length of output: 327
Script:
#!/bin/bash
# Look for gw_entities usage patterns
rg -A 5 "gw_entities\[.*gateway_id\]\[\"location\"\]"
# Check for DataMissingError usage in tests
rg -A 5 "DataMissingError" tests/
Length of output: 1270
Script:
#!/bin/bash
# Check P1 test implementations
cat tests/test_p1.py
# Look for test cases involving error scenarios
rg -A 10 "def test.*error" tests/test_p1.py tests/test_legacy_p1.py
Length of output: 3072
plugwise/helper.py (2)
340-342: LGTM: Good refactoring for improved code organization.
The extraction of functionality into separate methods _get_smartmeter_info() and _sort_gw_entities() improves code organization and follows the Single Responsibility Principle.
343-356: LGTM: Well-documented and properly implemented SmartMeter info collection.
The method clearly handles the collection of SmartMeter information and properly manages the gateway_id assignment for P1 devices.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 3435 3452 +17
=========================================
+ Hits 3435 3452 +17 ☔ View full report in Codecov by Sentry. |
bouwew
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
plugwise/helper.py (3)
343-356: Consider adding more detailed documentation.While the method effectively encapsulates the P1 smartmeter info collection logic, the gateway_id update logic (lines 350-356) could benefit from more explicit documentation explaining why and when this ID switch occurs.
def _get_smartmeter_info(self) -> None: """For P1 collect the connected SmartMeter info from the Home/buildinglocation. There is no appliance available for this device. + Note: For P1 devices, we switch the gateway_id to the smartmeter device_id + to maintain backward compatibility with existing implementations. """
357-366: Consider using constants for device classes.The method effectively sorts gateway entities, but the device classes could be defined as constants at the module level for better maintainability.
+PRIORITY_DEVICE_CLASSES = ("heater_central", "gateway") def _sort_gw_entities(self) -> None: """Place the gateway and optional heater_central devices as 1st and 2nd.""" - for dev_class in ("heater_central", "gateway"): + for dev_class in PRIORITY_DEVICE_CLASSES:
817-826: Consider refactoring the initialization logic.The initialization of binary sensors could be made more concise by extracting it into a helper method.
+def _ensure_binary_sensor(self, data: GwEntityData, sensor_name: str) -> None: + """Initialize a binary sensor if not present.""" + if sensor_name not in data["binary_sensors"]: + self._count += 1 + data["binary_sensors"][sensor_name] = False def _process_on_off_device_c_heating_state(self, data: GwEntityData) -> None: if self.smile(ANNA): data["binary_sensors"]["heating_state"] = data["c_heating_state"] if self.smile(ADAM): - if "heating_state" not in data["binary_sensors"]: - self._count += 1 - data["binary_sensors"]["heating_state"] = False - - if "cooling_state" not in data["binary_sensors"]: - self._count += 1 - data["binary_sensors"]["cooling_state"] = False + self._ensure_binary_sensor(data, "heating_state") + self._ensure_binary_sensor(data, "cooling_state")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(3 hunks)
🔇 Additional comments (2)
plugwise/helper.py (2)
340-342: LGTM! Good separation of concerns.
The addition of these method calls improves code organization by separating smartmeter info collection and gateway entity sorting into dedicated methods.
Line range hint 380-408: LGTM! Improved error handling and encapsulation.
The changes enhance the method by:
- Removing the external dependency on the
applparameter - Adding proper error handling for missing module data
- Better encapsulation of the Munch object creation
bouwew
force-pushed
the
try-something
branch
from
8bf113e to
8bdcf7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
plugwise/helper.py (2)
346-366: Consider adding error handling for entity switching.While the implementation is solid, consider adding error handling for cases where the entity switching might fail, such as when the expected entities are not present in the expected order.
def _get_smartmeter_info(self) -> None: if self.smile_type == "power": self._p1_smartmeter_info_finder() + # Ensure we have exactly two entities before switching + if len(self.gw_entities) != 2: + LOGGER.warning("Expected exactly 2 entities for P1 device, found %d", len(self.gw_entities)) + return # Switch entity_ids entity_id_0 = list(self.gw_entities)[0] entity_0 = list(self.gw_entities.values())[0]
367-376: Clean implementation with room for minor optimization.The sorting implementation is clear and effective. Consider using dict comprehension for a slightly more concise implementation of the priority sorting.
def _sort_gw_entities(self) -> None: for dev_class in PRIORITY_DEVICE_CLASSES: - for entity_id, entity in dict(self.gw_entities).items(): - if entity["dev_class"] == dev_class: - priority_entity = entity - self.gw_entities.pop(entity_id) - other_entities = self.gw_entities - priority_entities = {entity_id: priority_entity} - self.gw_entities = {**priority_entities, **other_entities} + priority_entities = { + entity_id: entity for entity_id, entity in dict(self.gw_entities).items() + if entity["dev_class"] == dev_class + } + if priority_entities: + other_entities = { + k: v for k, v in self.gw_entities.items() + if k not in priority_entities + } + self.gw_entities = {**priority_entities, **other_entities}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(6 hunks)
🔇 Additional comments (4)
plugwise/helper.py (4)
32-32: LGTM!
The new constant import is properly placed and used by the new sorting functionality.
282-286: Well-structured code organization!
The changes improve modularity by extracting SmartMeter info collection and entity sorting into dedicated methods, making the code more maintainable.
Also applies to: 340-344
827-836: LGTM! Clear state management implementation.
The binary sensor state handling is well-implemented with proper initialization and state tracking. The counter management ensures accurate tracking of sensor states.
843-845: LGTM! Clear and informative documentation.
The docstring update clearly documents the extended device support for cooling functionality.
bouwew
force-pushed
the
try-something
branch
from
971de0b to
25177a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
plugwise/helper.py (1)
Line range hint
346-380: Consider adding a return type hintThe implementation is solid with good error handling and clear comments. Consider adding a return type hint to improve code clarity.
-def _get_p1_smartmeter_info(self) -> None: +def _get_p1_smartmeter_info(self) -> None | Literal[None]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(5 hunks)
🔇 Additional comments (7)
plugwise/helper.py (7)
32-32: LGTM!
The import of PRIORITY_DEVICE_CLASSES is correctly placed with other related constants.
282-286: LGTM!
The docstring clearly explains the method's dual responsibility of collecting appliance info and P1 smartmeter info.
341-344: LGTM!
The new method calls are logically placed and maintain a clear separation of concerns: data collection followed by data organization.
383-393: LGTM!
The implementation efficiently sorts gateway entities while maintaining priority order using dictionary operations.
394-405: LGTM!
The method is well-structured with clear documentation and efficient data handling using Munch.
814-823: LGTM!
The implementation properly handles state initialization and counter management for heating and cooling states.
830-833: LGTM!
The docstring clearly documents the method's purpose and supported devices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
plugwise/helper.py (3)
Line range hint
282-380: LGTM! Well-structured P1 smartmeter info collection implementation.The implementation is clean and handles backward compatibility elegantly by switching entity IDs between gateway and smartmeter. Good error handling and clear documentation.
Consider documenting the entity ID switching behavior in a central location (e.g., README or API docs) to help future maintainers understand this design decision.
383-393: Consider a more efficient sorting implementation.The current implementation modifies the dictionary while iterating, which could be error-prone. Consider using a more efficient sorting approach.
Here's a suggested implementation:
def _sort_gw_entities(self) -> None: - for dev_class in PRIORITY_DEVICE_CLASSES: - for entity_id, entity in dict(self.gw_entities).items(): - if entity["dev_class"] == dev_class: - priority_entity = entity - self.gw_entities.pop(entity_id) - other_entities = self.gw_entities - priority_entities = {entity_id: priority_entity} - self.gw_entities = {**priority_entities, **other_entities} + def sort_key(item): + entity_id, entity = item + try: + return PRIORITY_DEVICE_CLASSES.index(entity["dev_class"]) + except ValueError: + return len(PRIORITY_DEVICE_CLASSES) + + self.gw_entities = dict(sorted(self.gw_entities.items(), key=sort_key))
814-823: Simplify binary sensor initialization.The initialization and counting logic for heating_state and cooling_state could be more concise.
Consider this more concise implementation:
- if "heating_state" not in data["binary_sensors"]: - self._count += 1 - data["binary_sensors"]["heating_state"] = False - - if "cooling_state" not in data["binary_sensors"]: - self._count += 1 - data["binary_sensors"]["cooling_state"] = False + for state in ["heating_state", "cooling_state"]: + if state not in data["binary_sensors"]: + self._count += 1 + data["binary_sensors"][state] = False
bouwew
force-pushed
the
try-something
branch
from
70fa1f2 to
5bcc778
Compare
|
All changes:
legacy/smile.py, no specific tests implemented due to focus on the code covering non-legacy devices).Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation