Energy-device-related improvements #647
Conversation
WalkthroughThe changes in this pull request involve updates to the changelog, modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #647 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 3374 3370 -4
=========================================
- Hits 3374 3370 -4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)plugwise/constants.py(1 hunks)plugwise/helper.py(5 hunks)plugwise/util.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🔇 Additional comments (10)
plugwise/util.py (2)
Line range hint 122-150: Verify potential key collisions in the data dictionary.
The addition of the SPECIALS case follows the existing pattern and is well-structured. However, storing special values directly in the data dictionary could potentially lead to key collisions with other top-level keys.
Let's verify potential key collisions:
#!/bin/bash
# Description: Check for potential key collisions between SPECIALS and other top-level keys in the data dictionary
# Search for dictionary key definitions and usages
echo "Searching for data dictionary key definitions..."
rg "data\[.+\]" --type py
# Search for SPECIALS constant definition
echo -e "\nSearching for SPECIALS constant definition..."
ast-grep --pattern 'SPECIALS = $_'Consider adding validation for special values before storing them in the data dictionary.
118-118: Add test coverage for the fallback case.
The # pragma: no cover comment indicates that this fallback case is not covered by tests. This path should be tested to ensure proper handling of unknown or non-Plugwise devices.
Let's verify the current test coverage:
Would you like me to help generate test cases for this uncovered code path?
✅ Verification successful
Let me gather more information about the test coverage and the check_model function.
Let me try different commands to gather the necessary information.
Let me try one final search with different patterns.
Based on the gathered information, I can now provide a final response:
The # pragma: no cover comment is justified in this case
The check_model function has a well-structured flow where the uncovered return statement is a valid fallback case that only executes when:
- The device is not a Plugwise device (
vendor_name != "Plugwise") - The device is not an Aqara plug (
"lumi.plug" not in name)
This fallback case preserves the original device name when no specific model mapping exists, which is the correct behavior for unknown third-party devices. The lack of test coverage is acceptable here since:
- It's a defensive programming pattern for future compatibility
- Testing would require mocking unknown device types, which adds little value
- The behavior is straightforward - just returning the input parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test cases in the test files
rg -A 5 "check_model" --glob "tests/*.py"
Length of output: 10826
Script:
#!/bin/bash
# First, let's look at the check_model function implementation
ast-grep --pattern 'def check_model($_) {
$$$
}'
# Then search for test files related to util.py
fd test_util.py
# Finally, let's look at any existing test cases
rg -l "test.*check_model" tests/
Length of output: 105
Script:
#!/bin/bash
# Look for the check_model function implementation with proper Python syntax
ast-grep --pattern 'def check_model($params):
$$$'
# Search for test files in a different way
find . -name "test_util.py"
# Look for any test files containing "check_model"
rg "check_model" --type py --glob "test_*.py"
Length of output: 128
Script:
#!/bin/bash
# Search for check_model function with context
rg -A 10 "def check_model" --type py
# Search for any test files
find . -type f -path "*/tests/*" -name "test_*.py"
# Look for test files content
rg "from.*util import" --type py --glob "test_*.py"
Length of output: 915
plugwise/constants.py (1)
434-441: Verify data format compatibility for electricity measurements.
The conversion from float to int for electricity-related measurements suggests these values are always whole numbers. While this change makes sense for interval and point measurements which typically represent discrete units, we should ensure this aligns with the actual data format from the Plugwise devices.
Let's verify the data format:
Consider adding validation in the data processing layer to handle potential floating-point values from devices, ensuring graceful handling if devices ever report non-integer values.
Also applies to: 442-448, 455-455
plugwise/helper.py (7)
272-274: Correctly skip OpenTherm Gateway appliances
The code correctly skips appliances of type "open_therm_gateway", ensuring they are not included in data processing.
298-300: Properly exclude orphaned thermostat devices
The code correctly skips orphaned thermostats without a location, ensuring only active devices are processed.
312-313: Enhance appliance information collection
The call to _appliance_info_finder gathers detailed information about each appliance, improving the data collection process.
319-321: Exclude orphaned or removed plug devices
The code correctly skips plug-type devices without a Zigbee MAC address, indicating they are orphaned or removed.
365-368: Retrieve module data for P1 smart meter devices
The code correctly retrieves module data for the P1 smart meter by specifying the appropriate locator and module type.
371-372: Assign firmware and hardware versions from module data
The code correctly assigns firmware and hardware version information to the appliance from the retrieved module data.
398-415:
Syntax error in match-case statement
The line case _ as s if s.endswith("_plug"): contains invalid syntax in the match-case statement. The correct syntax should be case s if s.endswith("_plug"): since as is not used in this context.
Apply this diff to fix the syntax error:
-case _ as s if s.endswith("_plug"):
+case s if s.endswith("_plug"):Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
plugwise/constants.py (1)
434-457: Consider adding unit documentation.Consider adding comments to document that these integer values represent watts for instantaneous measurements.
Add documentation like this:
class SmileSensors(TypedDict, total=False): """Smile Sensors class.""" battery: float cooling_activation_outdoor_temperature: float cooling_deactivation_threshold: float dhw_temperature: float domestic_hot_water_setpoint: float temperature: float - electricity_consumed: int + electricity_consumed: int # Instantaneous power in watts - electricity_consumed_interval: int + electricity_consumed_interval: int # Power over interval in wattsplugwise/helper.py (2)
356-372: Consider adding error handling for module data access.While the P1 smartmeter info collection has been improved, it could benefit from additional error handling when accessing module data.
def _p1_smartmeter_info_finder(self, appl: Munch) -> None: """Collect P1 DSMR SmartMeter info.""" loc_id = next(iter(self.loc_data.keys())) location = self._domain_objects.find(f'./location[@id="{loc_id}"]') locator = "./logs/point_log/electricity_point_meter" mod_type = "electricity_point_meter" module_data = self._get_module_data(location, locator, mod_type) + if not module_data: + LOGGER.warning("No module data found for P1 smartmeter") + return None appl.dev_id = self.gateway_id appl.firmware = module_data["firmware_version"] appl.hardware = module_data["hardware_version"] appl.location = loc_id appl.mac = None appl.model = module_data["vendor_model"] appl.model_id = None # don't use model_id for SmartMeter appl.name = "P1" appl.pwclass = "smartmeter" appl.vendor_name = module_data["vendor_name"] appl.zigbee_mac = None
395-409: Consider adding type hints for module_data dictionary.The code handling plug-type devices could benefit from type hints for the module_data dictionary to improve code maintainability and catch potential type-related issues early.
+from typing import TypedDict + +class ModuleData(TypedDict): + contents: bool + firmware_version: str | None + hardware_version: str | None + vendor_model: str | None + vendor_name: str | None + zigbee_mac_address: str | None def _appliance_info_finder(self, appl: Munch, appliance: etree) -> Munch: """Collect info for all appliances found.""" match appl.pwclass: case _ as s if s.endswith("_plug"): locator = "./logs/interval_log/electricity_interval_meter" mod_type = "electricity_interval_meter" - module_data = self._get_module_data(appliance, locator, mod_type) + module_data: ModuleData = self._get_module_data(appliance, locator, mod_type)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
plugwise/constants.py(1 hunks)plugwise/helper.py(5 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (5)
plugwise/constants.py (1)
434-457: Type changes look good, but need verification.
The change from float to int for instantaneous power measurements is logically correct as power meters typically report whole number values in watts. The preservation of float type for cumulative measurements is also appropriate as energy totals can have decimal places.
Let's verify that all code handling these values expects integers:
plugwise/helper.py (4)
272-275: LGTM: Filtering out OpenThermGateway appliance.
The code correctly filters out OpenThermGateway appliances from data collection, which helps maintain cleaner data by excluding non-essential gateway devices.
298-301: LGTM: Proper handling of orphaned thermostats.
The code appropriately skips orphaned thermostat devices by checking for null location, preventing potential issues with disconnected or removed devices.
Line range hint 378-410: LGTM: Improved appliance info collection with better validation.
The code has been enhanced with better validation and error handling for different appliance types. The use of pattern matching makes the code more maintainable and easier to extend.
390-394: LGTM: Proper handling of orphaned heater_central devices.
The code correctly identifies and skips orphaned heater_central devices by comparing device IDs, preventing potential issues with disconnected devices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
plugwise/helper.py (2)
356-375: Consider enhancing error handling for P1 smartmeter info collection.While the implementation is generally good, the error handling could be improved. The function returns
Noneon error but this return value isn't handled by the caller.Consider this improvement:
- LOGGER.error("No module data found for SmartMeter") # pragma: no cover - return None # pragma: no cover + LOGGER.error("No module data found for SmartMeter") + raise ValueError("Required SmartMeter module data not found")
719-730: Consider consolidating duplicate error handling logic.The error handling pattern is repeated for both Plug and wireless thermostat cases.
Consider refactoring to reduce duplication:
- if not module_data["contents"]: - # Try collecting for a wireless thermostat - locator = "./logs/point_log[type='thermostat']/thermostat" - mod_type = "thermostat" - module_data = self._get_module_data(appliance, locator, mod_type) - if not module_data["contents"]: - LOGGER.error("No module data found for Plug or wireless thermostat") # pragma: no cover - return None # pragma: no cover + if not module_data["contents"]: + # Try collecting for a wireless thermostat + locator = "./logs/point_log[type='thermostat']/thermostat" + mod_type = "thermostat" + module_data = self._get_module_data(appliance, locator, mod_type) + + if not module_data["contents"]: + LOGGER.error("No module data found for Plug or wireless thermostat") + return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
plugwise/helper.py(6 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (3)
plugwise/helper.py (3)
272-275: LGTM: Filtering out OpenThermGateway appliance.
The code correctly excludes OpenThermGateway appliances from data collection, which helps maintain clean and relevant device data.
298-301: LGTM: Proper handling of orphaned thermostats.
The code appropriately filters out orphaned thermostat devices that don't have an associated location, preventing potential issues with unlinked devices.
Line range hint 381-413: LGTM: Improved plug device handling with proper validation.
The code effectively handles plug-type devices with proper validation of module data. The early return for orphaned devices (no module data) prevents processing of invalid devices.
bouwew
force-pushed
the
fix-p1-typing
branch
from
027ca2d to
0544640
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
plugwise/helper.py (2)
356-375: Improve error handling in P1 SmartMeter info collection.While the error handling is good, consider these improvements:
- The error message could be more specific about why the module data is missing.
- The
return Noneon error is not used by callers, consider raising an exception instead.def _p1_smartmeter_info_finder(self, appl: Munch) -> None: """Collect P1 DSMR SmartMeter info.""" loc_id = next(iter(self.loc_data.keys())) location = self._domain_objects.find(f'./location[@id="{loc_id}"]') locator = "./logs/point_log/electricity_point_meter" mod_type = "electricity_point_meter" module_data = self._get_module_data(location, locator, mod_type) if not module_data["contents"]: - LOGGER.error("No module data found for SmartMeter") # pragma: no cover - return None # pragma: no cover + msg = f"No module data found for SmartMeter: missing {mod_type} at {locator}" + LOGGER.error(msg) # pragma: no cover + raise ValueError(msg) # pragma: no cover
719-730: Improve error handling in wireless availability check.While the implementation is good, consider these improvements:
- Make the error message more specific by including which device was being checked.
- The
return Noneon error is not used by callers, consider removing it.def _wireless_availability(self, appliance: etree, data: DeviceData) -> None: """Helper-function for _get_measurement_data().""" if self.smile(ADAM): # Try collecting for a Plug locator = "./logs/interval_log/electricity_interval_meter" mod_type = "electricity_interval_meter" module_data = self._get_module_data(appliance, locator, mod_type) if not module_data["contents"]: # Try collecting for a wireless thermostat locator = "./logs/point_log[type='thermostat']/thermostat" mod_type = "thermostat" module_data = self._get_module_data(appliance, locator, mod_type) if not module_data["contents"]: - LOGGER.error("No module data found for Plug or wireless thermostat") # pragma: no cover - return None # pragma: no cover + device_id = appliance.attrib.get("id", "unknown") + LOGGER.error("No module data found for device %s (tried Plug and wireless thermostat)", device_id) # pragma: no cover
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
plugwise/helper.py(6 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (3)
plugwise/helper.py (3)
272-275: LGTM: Clean implementation of OpenThermGateway filtering.
The addition of this filter improves data collection by excluding OpenThermGateway appliances.
298-301: LGTM: Proper handling of orphaned thermostats.
The addition of this filter prevents processing of orphaned thermostat-type devices that don't have an associated location.
Line range hint 381-413: LGTM: Well-structured appliance info collection.
The implementation provides robust handling of different appliance types with proper validation:
- Clear type-based branching using match-case
- Proper validation of module data for plug-type devices
- Good handling of orphaned devices
CoMPaTech
left a comment
CoMPaTech
left a comment
There was a problem hiding this comment.
The XML values depict floats, even if Plugwise doesn't decide to use them, we still should adhere to what is in the XML data?
bouwew
force-pushed
the
fix-p1-typing
branch
from
d222eee to
8c33533
Compare
bouwew
force-pushed
the
fix-p1-typing
branch
from
8c33533 to
3955072
Compare
|
The P1 sensor all-float-typing will be implemented in a new PR. |
|
Summary:
_energy_device_info_finder()function, redistribute and improve the related code.Summary by CodeRabbit
Release Notes
New Features
SPECIALS, improving measurement processing.Bug Fixes
Documentation
Chores