Rework code: output a single dict, add gw_data items as Smile-properties #698
Conversation
WalkthroughThe pull request introduces a significant rework of the code to output a single dictionary and add Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (35)
fixtures/m_adam_multiple_devices_per_zone/data.json (5)
43-48: Consider adding validation for thermostat bounds.Multiple thermostat configurations have an upper bound of 100.0°C, which seems unrealistically high for residential heating. Consider using more realistic temperature bounds (e.g., 30°C or 35°C) to catch potential issues during testing.
Also applies to: 80-85, 124-129, 272-277, 419-424, 471-476, 525-530
572-576: Enhance notification data structure.The notification structure uses a UUID as a key which makes it difficult to test deterministically. Consider using a more predictable key format for testing purposes.
"notifications": { - "af82e4ccf9c548528166d38e560662a4": { + "test_notification_1": { "warning": "Node Plug (with MAC address 000D6F000D13CB01, in room 'n.a.') has been unreachable since 23:03 2020-01-18. Please check the connection and restart the device." } },
191-191: Add boundary testing for battery levels.The fixture includes various battery levels (34%, 37%, 51%, etc.) but doesn't cover edge cases. Consider adding test cases for:
- Critical battery level (≤ 10%)
- Full battery (100%)
- Invalid battery levels (negative or >100%)
Also applies to: 219-219, 384-384, 465-465, 493-493, 519-519, 547-547
565-567: Update firmware version format.The firmware version "3.0.15" should follow a consistent format with a timestamp like other devices in the fixture (e.g., "2019-06-21T02:00:00+02:00").
- "firmware": "3.0.15", + "firmware": "2023-01-01T00:00:00+00:00",
25-32: Standardize schedule names.The available schedules contain inconsistent naming patterns:
- Some use spaces ("GF7 Woonkamer" has double spaces)
- Different capitalization ("CV Roan" vs "off")
Consider standardizing the schedule names for consistency.Also applies to: 60-67, 254-261, 399-406
plugwise/common.py (1)
50-50: Initialize_cooling_present.
self._cooling_present: boolis declared without an initial value. To avoid potential usage before assignment, consider initializing it explicitly (e.g.,self._cooling_present = False).plugwise/helper.py (4)
83-83: Uninitialized_zones.The type hint is good, but
_zonesis declared without an initial value. Consider something likeself._zones = {}to prevent potential runtime issues.
452-452: Maintain consistency in_notifications.Storing notifications in
self._notificationsis consistent. Ensure_notificationsis initialized in the constructor or a similar place to prevent accidental usage before assignment.
628-632: Docstring formatting style.The docstring clarifies Elga cooling states. Although it starts with a comment line, it’s still understandable. Optional style improvement: remove the leading
#to standardize docstring conventions.
654-658: Docstring for Loria/Thermastage logic.This clarifies how cooling is determined for heat pumps. The multi-line docstring is helpful. Same minor docstring style note applies if you want to keep it consistent across the codebase.
plugwise/legacy/data.py (1)
20-20: Uninitialized_smile_props.
self._smile_props: SmilePropsis typed but not initialized. Consider settingself._smile_props = SmileProps()or a similar initial assignment to avoid referencing it prematurely.plugwise/data.py (3)
44-50: Consider using a dictionary literal for better readability.The sequential assignments to
_smile_propscould be combined into a single dictionary literal for better readability and maintainability.- self._smile_props["gateway_id"] = self._gateway_id - self._smile_props["item_count"] = self._count - self._smile_props["reboot"] = True - self._smile_props["smile_name"] = self.smile_name + self._smile_props.update({ + "gateway_id": self._gateway_id, + "item_count": self._count, + "reboot": True, + "smile_name": self.smile_name + })
123-131: Document the reason for incrementing count by 2.The count increment changed from 1 to 2, presumably because we're adding both a binary sensor and notifications. This change should be documented to explain why we're incrementing by 2.
data["binary_sensors"]["plugwise_notification"] = bool(self._notifications) data["notifications"] = self._notifications - self._count += 2 + # Increment by 2: one for binary sensor and one for notifications + self._count += 2
306-309: Add error handling for missing gateway_id.The method assumes
self._gateway_idexists inself.gw_entities. Consider adding error handling for cases where the gateway entity might not exist.def check_reg_mode(self, mode: str) -> bool: """Helper-function for device_data_climate().""" - gateway = self.gw_entities[self._gateway_id] + gateway = self.gw_entities.get(self._gateway_id) + if not gateway: + return False return ( "regulation_modes" in gateway and gateway["select_regulation_mode"] == mode )plugwise/legacy/helper.py (2)
75-77: Consider using a more specific type for smile_version.The Version type is used without a version specifier. Consider adding type bounds or constraints if specific version ranges are expected.
Line range hint
181-189: Add error handling for None values.The method should handle cases where smile_version or other values might be None.
- "firmware": str(self.smile_version), + "firmware": str(self.smile_version) if self.smile_version is not None else None,plugwise/__init__.py (2)
Line range hint
324-332: Enhance error message with more context.The error message could be more specific about what data is missing.
- raise PlugwiseError("No Plugwise data received") from err + raise PlugwiseError(f"Failed to update Plugwise data: {err}") from err
210-226: Consider extracting version validation logic.The version parsing and validation logic could be extracted into a separate method for better maintainability.
+ def _validate_version(self, model: str) -> None: + """Validate the smile version and model.""" + if model == "Unknown" or self.smile_version is None: + LOGGER.error( + "Unable to find model or version information, please create" + " an issue on http://github.com/plugwise/python-plugwise" + ) + raise UnsupportedDeviceError + async def _smile_detect(self, result: etree, dsmrmain: etree) -> None: # ... existing code ... - if model == "Unknown" or self.smile_version is None: - LOGGER.error( - "Unable to find model or version information, please create" - " an issue on http://github.com/plugwise/python-plugwise" - ) - raise UnsupportedDeviceError + self._validate_version(model)fixtures/smile_p1_v2/data.json (1)
25-33: Consider using UUID format for gateway IDThe gateway ID
aaaa0000aaaa0000aaaa0000aaaa00aaappears to be a placeholder. Consider using a UUID format consistent with other device IDs for better maintainability of test fixtures.- "aaaa0000aaaa0000aaaa0000aaaa00aa": { + "a455b61e52394b2db5081ce025a430f3": {fixtures/p1v4_442_single/data.json (1)
41-41: Consider normalizing Unicode characters in vendor nameThe vendor name contains Unicode characters (
\uff08and\uff09) which might cause issues in some environments. Consider normalizing to standard ASCII parentheses.- "vendor": "SHENZHEN KAIFA TECHNOLOGY \uff08CHENGDU\uff09 CO., LTD." + "vendor": "SHENZHEN KAIFA TECHNOLOGY (CHENGDU) CO., LTD."fixtures/legacy_anna/data.json (1)
2-10: Maintain consistent ID format across fixturesThe gateway ID format differs from other fixtures. Consider using the same format (either UUID or the 'a' pattern) consistently across all test fixtures for better maintainability.
- "0000aaaa0000aaaa0000aaaa0000aa00": { + "a270735e4ccd45239424badc0578a2b1": {fixtures/anna_loria_heating_idle/data.json (1)
4-4: Remove trailing space in schedule name.The schedule name "Test " contains a trailing space which might cause comparison issues.
- "available_schedules": ["Winter", "Test ", "off"], + "available_schedules": ["Winter", "Test", "off"],fixtures/anna_loria_driessens/data.json (1)
22-28: Implement i18n for schedule names.Schedule names are hardcoded in a specific language. Consider implementing proper internationalization:
- Extract schedule names to language-specific resource files
- Use translation keys instead of hardcoded strings
fixtures/adam_plus_anna/data.json (1)
87-91: Enhance error notification structure.The OpenTherm communication error notification should include:
- Timestamp of the error
- Error severity level
- Suggested troubleshooting steps
fixtures/stretch_v31/data.json (1)
101-116: Add metadata for report groups.Consider enhancing the report group structure with:
- Creation timestamp
- Last modified timestamp
- Report generation frequency
- Report type/category
fixtures/m_adam_heating/data.json (1)
183-197: Consider adding low battery warning threshold.The zone thermostat "Lisa Badkamer" shows low battery (14%) but there's no configuration for the warning threshold.
Consider adding a
low_battery_thresholdconfiguration to standardize when thelow_batterybinary sensor is triggered.fixtures/stretch_v27_no_domain/data.json (1)
113-131: Standardize device class naming.The device class "zz_misc" appears to be a placeholder or temporary classification. Consider using more descriptive and standardized device classes.
fixtures/m_adam_jip/data.json (2)
214-236: Gateway configuration looks good but consider adding version constraints.The gateway configuration includes all necessary fields including the new notifications field. However, consider adding version constraints for firmware compatibility.
"dev_class": "gateway", "firmware": "3.2.8", + "min_firmware": "3.0.0", + "recommended_firmware": "3.2.8", "hardware": "AME Smile 2.0 board",
227-227: Empty notifications object may need documentation.The empty notifications object might benefit from a comment explaining its purpose and expected structure.
- "notifications": {}, + "notifications": { + // Example structure: + // "notification_id": { + // "warning": "Description of the warning" + // } + },fixtures/adam_jip/data.json (2)
Line range hint
567-589: Gateway notification structure provides good example.The notification structure here provides a good template for handling device unreachability warnings. Consider documenting this as the standard format.
Consider extracting common notification types to a constants file:
# Example constants.py NOTIFICATION_TYPES = { 'DEVICE_UNREACHABLE': 'Node {device_name} (with MAC address {mac}, in room {room}) has been unreachable since {timestamp}. Please check the connection and restart the device.' }
Line range hint
584-584: Warning message contains hardcoded values.The warning message contains a hardcoded timestamp and MAC address. Consider using placeholders for better maintainability.
- "warning": "Node Plug (with MAC address 000D6F000D13CB01, in room 'n.a.') has been unreachable since 23:03 2020-01-18. Please check the connection and restart the device." + "warning": "Node {device_type} (with MAC address {mac_address}, in room '{room}') has been unreachable since {timestamp}. Please check the connection and restart the device.", + "parameters": { + "device_type": "Plug", + "mac_address": "000D6F000D13CB01", + "room": "n.a.", + "timestamp": "23:03 2020-01-18" + }fixtures/adam_zone_per_device/data.json (1)
25-31: Consider localizing schedule names.The schedule names appear to be in a mix of languages. Consider using a translation system for better internationalization.
"available_schedules": [ - "CV Roan", - "Bios Schema met Film Avond", - "GF7 Woonkamer", - "Badkamer Schema", - "CV Jessie", + {"id": "cv_roan", "name": "CV Roan"}, + {"id": "bios_schema", "name": "Bios Schema met Film Avond"}, + {"id": "gf7_woonkamer", "name": "GF7 Woonkamer"}, + {"id": "badkamer_schema", "name": "Badkamer Schema"}, + {"id": "cv_jessie", "name": "CV Jessie"}, "off" ],fixtures/anna_v4_dhw/data.json (1)
4-4: Standardize language usage in configuration.The schedule names are in Dutch ("Standaard", "Thuiswerken") while the rest of the configuration is in English. Consider standardizing the language usage for better maintainability.
Consider translating schedule names to English or providing language-specific configuration files.
Also applies to: 61-66
fixtures/anna_elga_no_cooling/data.json (1)
31-36: Review cooling activation threshold configuration.The cooling activation temperature (21.0°C) is set very close to the current room temperature (19.3°C), with a deactivation threshold of 4.0°C. This narrow margin might cause frequent switching between heating and cooling modes.
Consider implementing hysteresis or a wider temperature band to prevent system oscillation.
Also applies to: 73-75
fixtures/anna_heatpump_cooling/data.json (1)
42-50: Consider adding documentation for heat pump specific parameters.The heat pump configuration includes several specialized parameters that would benefit from documentation:
dhw_temperaturemodulation_level(integer percentage?)water_pressure(units?)- Various temperature readings and their relationships
Consider adding a schema file or documentation to explain these parameters and their valid ranges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
CHANGELOG.md(1 hunks)fixtures/adam_heatpump_cooling/all_data.json(0 hunks)fixtures/adam_heatpump_cooling/data.json(1 hunks)fixtures/adam_jip/all_data.json(0 hunks)fixtures/adam_jip/data.json(1 hunks)fixtures/adam_multiple_devices_per_zone/all_data.json(0 hunks)fixtures/adam_multiple_devices_per_zone/data.json(1 hunks)fixtures/adam_onoff_cooling_fake_firmware/all_data.json(0 hunks)fixtures/adam_onoff_cooling_fake_firmware/data.json(1 hunks)fixtures/adam_plus_anna/all_data.json(0 hunks)fixtures/adam_plus_anna/data.json(1 hunks)fixtures/adam_plus_anna_new/all_data.json(0 hunks)fixtures/adam_plus_anna_new/data.json(1 hunks)fixtures/adam_plus_anna_new_regulation_off/all_data.json(0 hunks)fixtures/adam_plus_anna_new_regulation_off/data.json(1 hunks)fixtures/adam_zone_per_device/all_data.json(0 hunks)fixtures/adam_zone_per_device/data.json(1 hunks)fixtures/anna_elga_2/all_data.json(0 hunks)fixtures/anna_elga_2/data.json(1 hunks)fixtures/anna_elga_2_cooling/all_data.json(0 hunks)fixtures/anna_elga_2_cooling/data.json(1 hunks)fixtures/anna_elga_2_schedule_off/all_data.json(0 hunks)fixtures/anna_elga_2_schedule_off/data.json(1 hunks)fixtures/anna_elga_no_cooling/all_data.json(0 hunks)fixtures/anna_elga_no_cooling/data.json(1 hunks)fixtures/anna_heatpump_cooling/all_data.json(0 hunks)fixtures/anna_heatpump_cooling/data.json(1 hunks)fixtures/anna_heatpump_cooling_fake_firmware/all_data.json(0 hunks)fixtures/anna_heatpump_cooling_fake_firmware/data.json(1 hunks)fixtures/anna_heatpump_heating/all_data.json(0 hunks)fixtures/anna_heatpump_heating/data.json(1 hunks)fixtures/anna_loria_cooling_active/all_data.json(0 hunks)fixtures/anna_loria_cooling_active/data.json(1 hunks)fixtures/anna_loria_driessens/all_data.json(0 hunks)fixtures/anna_loria_driessens/data.json(1 hunks)fixtures/anna_loria_heating_idle/all_data.json(0 hunks)fixtures/anna_loria_heating_idle/data.json(1 hunks)fixtures/anna_v4/all_data.json(0 hunks)fixtures/anna_v4/data.json(1 hunks)fixtures/anna_v4_dhw/all_data.json(0 hunks)fixtures/anna_v4_dhw/data.json(1 hunks)fixtures/anna_v4_no_tag/all_data.json(0 hunks)fixtures/anna_v4_no_tag/data.json(1 hunks)fixtures/anna_without_boiler_fw441/all_data.json(0 hunks)fixtures/anna_without_boiler_fw441/data.json(1 hunks)fixtures/legacy_anna/all_data.json(0 hunks)fixtures/legacy_anna/data.json(1 hunks)fixtures/legacy_anna_2/all_data.json(0 hunks)fixtures/legacy_anna_2/data.json(1 hunks)fixtures/m_adam_cooling/all_data.json(0 hunks)fixtures/m_adam_cooling/data.json(1 hunks)fixtures/m_adam_heating/all_data.json(0 hunks)fixtures/m_adam_heating/data.json(1 hunks)fixtures/m_adam_jip/all_data.json(0 hunks)fixtures/m_adam_jip/data.json(1 hunks)fixtures/m_adam_multiple_devices_per_zone/all_data.json(0 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(1 hunks)fixtures/m_anna_heatpump_cooling/all_data.json(0 hunks)fixtures/m_anna_heatpump_cooling/data.json(1 hunks)fixtures/m_anna_heatpump_idle/all_data.json(0 hunks)fixtures/m_anna_heatpump_idle/data.json(1 hunks)fixtures/p1v4_442_single/all_data.json(0 hunks)fixtures/p1v4_442_single/data.json(1 hunks)fixtures/p1v4_442_triple/all_data.json(0 hunks)fixtures/p1v4_442_triple/data.json(1 hunks)fixtures/smile_p1_v2/all_data.json(0 hunks)fixtures/smile_p1_v2/data.json(1 hunks)fixtures/smile_p1_v2_2/all_data.json(0 hunks)fixtures/smile_p1_v2_2/data.json(1 hunks)fixtures/stretch_v23/all_data.json(0 hunks)fixtures/stretch_v23/data.json(1 hunks)fixtures/stretch_v27_no_domain/all_data.json(0 hunks)fixtures/stretch_v27_no_domain/data.json(1 hunks)fixtures/stretch_v31/all_data.json(0 hunks)fixtures/stretch_v31/data.json(1 hunks)plugwise/__init__.py(11 hunks)plugwise/common.py(2 hunks)plugwise/constants.py(2 hunks)plugwise/data.py(5 hunks)plugwise/helper.py(12 hunks)plugwise/legacy/data.py(2 hunks)plugwise/legacy/helper.py(5 hunks)
⛔ Files not processed due to max files limit (12)
- plugwise/legacy/smile.py
- plugwise/smile.py
- plugwise/smilecomm.py
- plugwise/util.py
- pyproject.toml
- scripts/manual_fixtures.py
- tests/test_adam.py
- tests/test_anna.py
- tests/test_generic.py
- tests/test_init.py
- tests/test_legacy_anna.py
- tests/test_p1.py
💤 Files with no reviewable changes (37)
- fixtures/m_adam_heating/all_data.json
- fixtures/anna_loria_driessens/all_data.json
- fixtures/stretch_v23/all_data.json
- fixtures/adam_plus_anna/all_data.json
- fixtures/anna_elga_2/all_data.json
- fixtures/p1v4_442_single/all_data.json
- fixtures/anna_heatpump_cooling_fake_firmware/all_data.json
- fixtures/adam_onoff_cooling_fake_firmware/all_data.json
- fixtures/anna_v4_no_tag/all_data.json
- fixtures/adam_multiple_devices_per_zone/all_data.json
- fixtures/anna_heatpump_cooling/all_data.json
- fixtures/legacy_anna/all_data.json
- fixtures/anna_elga_no_cooling/all_data.json
- fixtures/stretch_v27_no_domain/all_data.json
- fixtures/stretch_v31/all_data.json
- fixtures/anna_elga_2_schedule_off/all_data.json
- fixtures/smile_p1_v2/all_data.json
- fixtures/smile_p1_v2_2/all_data.json
- fixtures/anna_loria_cooling_active/all_data.json
- fixtures/adam_plus_anna_new_regulation_off/all_data.json
- fixtures/m_adam_multiple_devices_per_zone/all_data.json
- fixtures/m_adam_cooling/all_data.json
- fixtures/p1v4_442_triple/all_data.json
- fixtures/anna_v4_dhw/all_data.json
- fixtures/m_adam_jip/all_data.json
- fixtures/adam_plus_anna_new/all_data.json
- fixtures/m_anna_heatpump_cooling/all_data.json
- fixtures/anna_v4/all_data.json
- fixtures/anna_elga_2_cooling/all_data.json
- fixtures/anna_heatpump_heating/all_data.json
- fixtures/m_anna_heatpump_idle/all_data.json
- fixtures/anna_without_boiler_fw441/all_data.json
- fixtures/adam_jip/all_data.json
- fixtures/adam_zone_per_device/all_data.json
- fixtures/anna_loria_heating_idle/all_data.json
- fixtures/legacy_anna_2/all_data.json
- fixtures/adam_heatpump_cooling/all_data.json
🧰 Additional context used
📓 Learnings (1)
fixtures/m_adam_heating/data.json (1)
Learnt from: bouwew
PR: plugwise/python-plugwise#630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-11-12T13:49:49.714Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
🔇 Additional comments (61)
fixtures/m_adam_multiple_devices_per_zone/data.json (1)
1-584: LGTM! Well-structured test fixture with comprehensive device configurations.The JSON structure is well-organized and provides a realistic test dataset for various device types and configurations.
plugwise/common.py (5)
12-12: Import statement looks good.Importing
NONEfromplugwise.constantsaligns with its usage in_heater_id. No concerns here.
30-43: Consider unifying the behavior for legacy and non-legacy.The
get_zigbee_datafunction updateszigbee_mac_addressandreachableonly in the non-legacy (elif) path. You might want to clarify whetherreachableshould also be considered in the legacy path to keep behavior consistent. Also, ifrouter.find("mac_address")orcoord.find("mac_address")isNone, using.textmay cause anAttributeError. Ensure these elements are always present or guard againstNone.
53-53: Default assignment for_heater_idlooks appropriate.Setting
self._heater_idtoNONEprovides a clear default state.
55-55: gw_entities initialization is fine.Initializing an empty dictionary for
gw_entitieswill help ensure no KeyErrors when populating it later.
245-245: Function call is well-integrated.Calling
get_zigbee_datafrom_get_module_datacentralizes Zigbee parsing logic, improving maintainability. No issues here.plugwise/helper.py (6)
47-47: Import forcollect_power_valuesis consistent.You’re introducing a new helper function usage from
plugwise.util. This is good for modularity.
49-49: Newly importedcount_data_items.Same as above, modularizing counting logic into a utility function is beneficial.
82-82: Verify usage of_gateway_id.Assigning
_gateway_idtoNONEensures a default fallback. Double-check external references to ensure they use_gateway_idproperly.
393-393:collect_power_valuesusage is appropriate.This call cleanly delegates logic to a dedicated function, enhancing readability and reducing duplication.
429-429: Counting data items.Good approach in calling
count_data_itemsto consistently track your_countvariable.
570-570: Conditional for_is_thermostatand_gateway_id.This check precisely focuses on whether the entity is the gateway in a thermostat scenario. Looks fine.
plugwise/legacy/data.py (4)
10-10: Import statements look correct.Adding
SmilePropsaligns well with the new property-based approach.
26-26: Clear docstring explanation.The docstring clarifies the method’s purpose: collecting data into
_smile_propsandgw_entities.
29-30: Assigning gateway properties.Storing
gateway_idanditem_countin_smile_propsis consistent with the new property-based data model.
33-33: Addingheater_idto_smile_props.This is consistent with the property-based approach. Ensures relevant data is consolidated in a single structure.
plugwise/data.py (1)
30-31: LGTM! Good use of type hints.The initialization of
_smile_propsand_zoneswith proper type hints improves code clarity and type safety.plugwise/constants.py (2)
396-405: LGTM! Well-structured TypedDict definition.The SmileProps TypedDict is well-defined with clear field names and types. The use of
total=Falseis appropriate as not all fields will be present in every instance.
553-553: LGTM! Consistent type definition.The addition of the notifications field to GwEntityData maintains type consistency across the codebase.
plugwise/__init__.py (1)
86-117: LGTM! Well-designed property accessors.The properties provide a clean and type-safe interface to access _smile_props values. Good use of docstrings and proper handling of optional values.
fixtures/smile_p1_v2_2/data.json (1)
1-34: LGTM! Data structure is consistent with other fixturesThe file maintains consistent structure with other fixtures while providing variant test data.
fixtures/anna_without_boiler_fw441/data.json (1)
1-61: LGTM! Well-structured thermostat configurationThe file provides a comprehensive test fixture for thermostat functionality with appropriate value ranges and consistent decimal precision.
fixtures/anna_heatpump_cooling_fake_firmware/data.json (1)
33-38: Verify vector length validation.The dot product calculation correctly validates vector lengths before computation, but consider adding input type validation to ensure both inputs are numeric arrays.
fixtures/anna_loria_cooling_active/data.json (1)
66-71: Inconsistent temperature resolution values.The resolution values differ between
max_dhw_temperature(0.01) andmaximum_boiler_temperature(0.01), while in other fixtures they are 1.0. This inconsistency might affect temperature control precision.Also applies to: 72-77
fixtures/m_anna_heatpump_idle/data.json (1)
75-76: Validate cooling temperature thresholds.The cooling activation temperature (25.0°C) and deactivation threshold (4.0°C) have a large gap. Consider if this is intentional or if tighter bounds would be more energy efficient.
✅ Verification successful
Cooling temperature thresholds are within normal ranges
The cooling activation temperature (25.0°C) and deactivation threshold (4.0°C) are consistent with patterns seen across other fixtures, where similar gaps are common. The values appear to be intentionally configured this way.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check cooling threshold ranges across fixtures rg '"cooling_activation_outdoor_temperature":\s*(\d+\.?\d*)' fixtures/ rg '"cooling_deactivation_threshold":\s*(\d+\.?\d*)' fixtures/Length of output: 1776
fixtures/m_anna_heatpump_cooling/data.json (2)
48-54: Validate sensor readings consistency.The temperature readings show expected relationships:
- outdoor_air_temperature (28.0°C) matches the gateway's outdoor_temperature (28.2°C)
- water_temperature (22.7°C) is higher than intended_boiler_temperature (0.0°C) during cooling
23-29: Verify state consistency.The binary sensors show that cooling is active (cooling_state: true) while heating is inactive (heating_state: false), which is correct. However, we should ensure these states are mutually exclusive across all scenarios.
✅ Verification successful
States are properly mutually exclusive ✅
The comprehensive analysis of all JSON fixtures confirms that heating and cooling states are never simultaneously active, maintaining proper system state consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any cases where both heating and cooling are active rg -A 5 '"cooling_state":\s*true' fixtures/ | rg '"heating_state":\s*true'Length of output: 76
Script:
#!/bin/bash # Create a Python script to analyze JSON files cat << 'EOF' > analyze_states.py import json import glob import os def check_states(data): if isinstance(data, dict): cooling = data.get('cooling_state', False) heating = data.get('heating_state', False) if cooling and heating: return True for value in data.values(): if check_states(value): return True elif isinstance(data, list): return any(check_states(item) for item in data) return False conflicts = [] for json_file in glob.glob('fixtures/**/*.json', recursive=True): try: with open(json_file) as f: data = json.load(f) if check_states(data): conflicts.append(json_file) except json.JSONDecodeError: print(f"Error parsing {json_file}") if conflicts: print("Files with conflicting states:") for file in conflicts: print(file) else: print("No conflicts found - heating and cooling states are mutually exclusive") EOF python analyze_states.pyLength of output: 103
fixtures/anna_heatpump_heating/data.json (3)
33-37: Verify temperature bounds are within safe operational limits.The temperature bounds configuration:
- DHW temperature: 35.0°C to 60.0°C
- Boiler temperature: 0.0°C to 100.0°C
These ranges appear reasonable, but please verify they align with the manufacturer's specifications for safe operation.
Also applies to: 39-43
10-10: Replace placeholder MAC address with a valid format.The MAC address appears to be a placeholder value. In production, ensure MAC addresses follow the standard format (XX:XX:XX:XX:XX:XX).
73-74: Consider adding validation for schedule names.The schedule names contain non-ASCII characters. Ensure the system properly handles Unicode strings in schedule names across all interfaces.
fixtures/anna_loria_driessens/data.json (1)
70-70: Consider adding data validation for sensor readings.The sensor readings (temperatures, modulation levels) should be validated against expected ranges:
- DHW temperature: 49.5°C
- Water temperature: 23.3°C
- Return temperature: 23.0°C
Also applies to: 88-95
fixtures/adam_onoff_cooling_fake_firmware/data.json (2)
44-44: Review safety implications of regulation modes.The regulation modes include maintenance operations (
bleeding_cold,bleeding_hot). Ensure these modes:
- Have proper safety interlocks
- Cannot be activated during normal operation
- Are properly documented for maintenance personnel
Also applies to: 53-59
106-109: Consider implementing proper error handling for thermostats array.The
thermostats.secondaryarray is empty. Ensure the system properly handles cases where:
- No secondary thermostats are present
- Primary thermostat becomes unavailable
fixtures/adam_plus_anna/data.json (1)
117-122: Add validation for electricity consumption metrics.Consider adding validation for electricity consumption values:
- Ensure consumed values are non-negative
- Verify interval values are consistent with total consumption
- Add maximum threshold alerts
fixtures/m_adam_cooling/data.json (3)
12-16: Verify maximum boiler temperature bounds for cooling mode.The maximum boiler temperature bounds (25.0-95.0°C) seem more appropriate for heating than cooling mode. Consider adjusting these bounds for cooling operation.
74-96: LGTM: Gateway configuration for cooling mode is complete.The gateway configuration correctly includes:
- Cooling in regulation modes
- Current regulation mode set to cooling
- Appropriate gateway modes
137-169: Verify climate control configuration for cooling mode.The living room climate control shows:
- Appropriate control_state: "cooling"
- Consistent climate_mode: "cool"
- Temperature setpoint (23.5°C) within bounds
fixtures/m_adam_heating/data.json (2)
11-31: Verify DHW temperature configuration.The domestic hot water (DHW) configuration shows:
- Maximum temperature of 60°C is appropriate for safety
- Setpoint within safe bounds
74-96: LGTM: Gateway configuration for heating mode is complete.The gateway configuration correctly includes:
- Heating in regulation modes
- Current regulation mode set to heating
- Appropriate gateway modes
fixtures/adam_plus_anna_new_regulation_off/data.json (1)
172-175: LGTM: Gateway configuration for regulation off mode is complete.The gateway configuration correctly shows:
- Regulation mode set to off
- Maintained gateway modes for user control
fixtures/adam_plus_anna_new/data.json (1)
229-231: Verify climate mode and control state consistency.The climate configuration shows:
- climate_mode: "auto"
- control_state: "heating"
This is a valid combination indicating proper automatic temperature control.fixtures/stretch_v27_no_domain/data.json (1)
73-91: Consider updating firmware versions.Multiple Circle type F devices show firmware dates from 2011:
- "2011-05-13T09:19:23+02:00"
- "2011-06-27T10:52:18+02:00"
Consider checking if newer firmware versions are available for these devices.
Run the following script to check for newer firmware versions:
Also applies to: 173-191
fixtures/m_adam_jip/data.json (1)
332-337: Verify sensor value ranges.The sensor values for the central heater should be validated against expected operational ranges:
- water_temperature: 37.3°C seems low for a central heater
- water_pressure: 1.4 bar is within normal range but close to low pressure
✅ Verification successful
Temperature and pressure values are normal for idle state
The current values are appropriate for a system in idle mode:
- Water temperatures (37.3°C/37.1°C) are normal when no heating is requested (modulation_level: 0.0)
- Water pressure of 1.4 bar is within safe operating range
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for typical water temperature ranges in the codebase rg -A 2 "water_temperature.*setpoint"Length of output: 1850
Script:
#!/bin/bash # Search for temperature-related patterns and ranges rg -i "temperature.*range|min.*temperature|max.*temperature" # Look for other test fixtures with similar data structure fd "data.json" --exec grep -A 5 "water_temperature"Length of output: 69981
fixtures/stretch_v23/data.json (1)
2-11: Gateway configuration is missing fields present in other fixtures.The gateway configuration is missing several fields that are present in other fixture files:
- notifications
- regulation_modes
- select_regulation_mode
fixtures/adam_zone_per_device/data.json (1)
579-584: Warning message format matches previous fixture.Good consistency in notification format with adam_jip/data.json.
fixtures/adam_multiple_devices_per_zone/data.json (2)
50-54: Multiple primary thermostats configuration looks good.The support for multiple primary thermostats in a zone is well-structured and consistent with the PR's objectives.
569-592: Gateway configuration matches expected structure.The gateway configuration includes all required fields and maintains consistency with other fixtures.
fixtures/adam_heatpump_cooling/data.json (3)
285-313: LGTM: Gateway configuration is complete and well-structured.The gateway configuration includes all necessary elements:
- Firmware version
- Hardware details
- MAC addresses
- Regulation modes
- Gateway modes
- Notifications handling
10-11: Verify climate mode consistency.The climate modes vary between "cool", "auto", and "cool" across different zones. This mix of modes needs verification to ensure it aligns with the system's capabilities.
Let's check the climate mode distribution:
Also applies to: 128-129, 242-243, 416-417, 467-468, 551-552, 681-682, 783-784
✅ Verification successful
Climate mode distribution is consistent with system capabilities
The mix of "auto" and "cool" modes across different zones in this heat pump cooling system is valid and expected behavior. The same pattern is consistently reflected in the test data, confirming this is the intended configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check climate mode distribution across zones rg '"climate_mode":\s*"[^"]+"' -o | sort | uniq -cLength of output: 4832
22-27: Inconsistent temperature bounds across thermostats.Most thermostats have bounds of 0.0-99.9°C, but the Woonkamer thermostat has bounds of 1.0-35.0°C. This inconsistency could cause issues with temperature control and user experience.
Let's verify if this is intentional:
Also applies to: 140-145, 396-401, 479-484, 563-568, 693-698, 795-800
✅ Verification successful
Different temperature bounds are intentionally set based on thermostat type
The variation in temperature bounds is an intentional design pattern:
- Anna (main room) thermostats: 4.0-30.0°C
- Zone thermostats: 0.0-99.9°C or 0.0-100.0°C
- Some specific models: 1.0-35.0°C
This consistent pattern across multiple test fixtures indicates the bounds are deliberately set based on the thermostat's role and capabilities, not an inconsistency that needs to be fixed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check temperature bounds across all thermostats rg -A 5 '"thermostat":\s*{' | grep -E "lower_bound|upper_bound"Length of output: 163
Script:
#!/bin/bash # Search for thermostat configurations with context rg -U --json "thermostat" | jq -r 'select(.type == "match") | .data.lines.text' | grep -A 5 "thermostat"Length of output: 65639
Script:
#!/bin/bash # Find all JSON files in fixtures directory and extract thermostat configurations fd -e json . fixtures/ -x sh -c 'echo "=== $1 ==="; cat "$1" | jq -r ".thermostat? // empty"' sh {}Length of output: 1825
Script:
#!/bin/bash # Extract thermostat bounds from all JSON files fd -e json . fixtures/ -x sh -c 'echo "=== $1 ==="; cat "$1" | jq -r ".. | select(.thermostat?) | .thermostat | select(.lower_bound != null) | \"lower_bound: \(.lower_bound), upper_bound: \(.upper_bound)\""' sh {}Length of output: 3944
CHANGELOG.md (1)
7-7: LGTM: Clear and well-formatted changelog entry.The new entry follows the established format and clearly describes the changes made in PR #690.
fixtures/legacy_anna_2/data.json (2)
10-10: Verify location references.The location IDs appear to be UUIDs that cross-reference between devices. Ensure these references are valid and consistent.
Also applies to: 31-31, 46-46
✅ Verification successful
Location references are valid and consistent ✓
All devices (thermostat, gateway, heater_central) properly reference the same location ID and can see each other within that location. The cross-referencing system is working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Extract and verify location references jq -r '.[] | select(.location != null) | [.location, .dev_class] | @tsv' fixtures/legacy_anna_2/data.json | \ while IFS=$'\t' read -r loc dev; do echo "Checking references to location $loc from device type $dev" jq --arg loc "$loc" '.[] | select(.location == $loc) | .dev_class' fixtures/legacy_anna_2/data.json doneLength of output: 1310
20-25: Verify temperature bounds and setpoints.The thermostat configuration shows overlapping temperature control mechanisms:
temperature_offset: -2.0 to 2.0thermostat: 4.0 to 30.0Please verify that these ranges are correct and don't conflict with each other.
Also applies to: 26-31
✅ Verification successful
Temperature ranges are correctly configured and serve complementary purposes
The temperature ranges in the configuration are valid and don't conflict:
- Thermostat range (4°C to 30°C): Primary temperature control for the room
- Temperature offset (-2°C to +2°C): Fine-tuning adjustment to the main setpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for temperature range definitions across the codebase rg -A 2 "lower_bound.*temperature|upper_bound.*temperature" --type pyLength of output: 71
Script:
#!/bin/bash # Search for thermostat configurations in fixture files fd -e json -x cat {} \; | jq -c 'select(.thermostat != null) | .thermostat' # Search for any temperature-related settings rg -g '!*.json' -i "temperature|thermostat" -A 2Length of output: 92249
fixtures/p1v4_442_triple/data.json (1)
28-53: Validate electricity readings and units.The electricity readings include various measurements that should be validated:
- Cumulative values should be non-negative
- Interval values should be within expected ranges
- Point values should correlate with phase readings
✅ Verification successful
Electricity readings validation successful
All electrical measurements are valid and consistent:
- Cumulative values are non-negative
- Interval values are within expected ranges
- Phase readings (5546W total) correlate with net point value (5553W)
- Phase voltages show normal residential levels (233-235V)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Analyze electricity readings across all fixture files for f in fixtures/*/data.json; do echo "Analyzing $f" jq -r '.[] | select(.sensors.electricity_consumed_peak_cumulative != null) | [.sensors.electricity_consumed_peak_cumulative, .sensors.electricity_consumed_peak_point, (.sensors.electricity_phase_one_consumed // 0 + .sensors.electricity_phase_two_consumed // 0 + .sensors.electricity_phase_three_consumed // 0)] | @csv' "$f" doneLength of output: 19579
fixtures/anna_v4_dhw/data.json (1)
61-72: Validate temperature bounds for DHW and boiler.Please verify that the temperature bounds are safe and comply with regulations:
- DHW: 30.0°C to 60.0°C
- Boiler: 0.0°C to 100.0°C
✅ Verification successful
Temperature bounds are within safe operating ranges
The temperature bounds in the file are consistent with standard safety practices and match the common patterns across the codebase:
- DHW range (30-60°C) prevents legionella growth while avoiding scalding risks
- Boiler range (0-100°C) provides safe operation margins below water's boiling point
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for temperature safety guidelines in documentation rg -l "temperature.*safety|temperature.*regulation" --type md # Extract and compare temperature bounds across fixtures for f in fixtures/*/data.json; do echo "Analyzing $f" jq -r '.[] | select(.max_dhw_temperature != null) | [.max_dhw_temperature.lower_bound, .max_dhw_temperature.upper_bound, .maximum_boiler_temperature.lower_bound, .maximum_boiler_temperature.upper_bound] | @csv' "$f" doneLength of output: 15434
fixtures/anna_v4/data.json (1)
1-88: Configuration structure looks good.The file follows the same structure as anna_v4_dhw/data.json with appropriate state variations.
fixtures/anna_v4_no_tag/data.json (1)
1-88: Configuration structure looks good.The file follows the same structure as other Anna configurations with appropriate mode variations.
fixtures/anna_elga_2/data.json (2)
21-21: Verify outdoor temperature synchronization between devices.The outdoor temperature values are inconsistent:
- Heater sensor: 6.0°C
- Gateway sensor: 6.38°C
Consider implementing a single source of truth for outdoor temperature readings.
Also applies to: 27-27, 81-81, 87-87
5-11: Verify the consistency of binary sensor states.The heater shows potentially conflicting states:
- Both
heating_stateandflame_stateare true whilecompressor_stateis also truecooling_enabledis false butcompressor_stateis trueThis combination of states might indicate a synchronization issue or invalid state transition.
✅ Verification successful
State combination is valid for this hybrid heating system
The combination of
heating_state: true,flame_state: true, andcompressor_state: trueis valid for this device model ("Generic heater/cooler"). This is a known state combination for hybrid heating systems where both traditional heating and heat pump operation can work simultaneously.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar state combinations in other fixtures rg -l --json '"heating_state": true.*"flame_state": true.*"compressor_state": true' || echo "No other instances found"Length of output: 540
Script:
#!/bin/bash # Find all JSON files first echo "=== Searching for JSON files with state combinations ===" rg -A 10 '"heating_state":' --type json echo -e "\n=== Looking for schema or documentation files ===" fd -e md -e json -e yaml -e yml . | grep -i "schema\|doc\|readme"Length of output: 43828
Script:
#!/bin/bash # Check README files for state documentation echo "=== Checking README files for state documentation ===" for f in $(fd -e md); do echo "=== Content of $f ===" cat "$f" | grep -A 5 -i "state\|mode\|heating\|cooling" doneLength of output: 22475
fixtures/anna_elga_2_schedule_off/data.json (1)
39-41: Configuration looks good for schedule-off mode.The system is properly configured with:
- Schedule set to "off"
- Climate mode set to "heat_cool"
- Control state "idle"
- All actuators inactive
Also applies to: 49-49
fixtures/anna_elga_2_cooling/data.json (1)
5-11: Cooling mode configuration is properly structured.The system shows correct state combinations for active cooling:
- All cooling-related binary sensors are true
- Heating-related sensors are false
- Setpoints and current temperature values align with cooling operation
- Outdoor temperature (31.0°C) is above cooling activation threshold (26.0°C)
Also applies to: 51-56
| "d03738edfcc947f7b8f4573571d90d2d": { | ||
| "dev_class": "switching", | ||
| "members": [ | ||
| "059e4d03c7a34d278add5c7a4a781d19", | ||
| "cfe95cf3de1948c0b8955125bf754614" | ||
| ], | ||
| "model": "Switchgroup", | ||
| "name": "Schakel", | ||
| "switches": { | ||
| "relay": true | ||
| }, | ||
| "vendor": "Plugwise" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Validation logic for switching groups is missing and should be implemented
The codebase analysis confirms that there's no validation logic for:
- Verifying the existence of member IDs
- Checking if members support switching operations
- Enforcing group size limits
The original review suggestions are valid and should be implemented to ensure data integrity and prevent operational issues.
🔗 Analysis chain
Enhance switching group validation.
For the switching group:
- Validate that all member IDs exist
- Ensure members support the switching operation
- Consider adding a maximum group size limit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for group member validation patterns
ast-grep --pattern 'function validate_group_members($members) {
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Search for validation-related code
echo "=== Searching for group validation patterns ==="
rg -i "validate.*group|group.*validate" -g "!*.{json,md}" -A 5
echo -e "\n=== Searching for member validation patterns ==="
rg -i "validate.*member|member.*validate" -g "!*.{json,md}" -A 5
echo -e "\n=== Searching for group-related size limits or constants ==="
rg -i "group.*size|max.*group|group.*limit" -g "!*.{json,md}" -A 5
echo -e "\n=== Finding potential validation files ==="
fd -e js -e ts "valid|group|switch"
Length of output: 694
| "sensors": { | ||
| "intended_boiler_temperature": 0.0, | ||
| "water_temperature": 30.0 | ||
| }, |
There was a problem hiding this comment.
Verify intended boiler temperature for regulation off mode.
The intended boiler temperature is 0.0°C which seems incorrect. Even in regulation off mode, it should maintain a minimum safe temperature to prevent freezing.
Consider implementing a minimum safe temperature even when regulation is off.
| "notifications": { | ||
| "97a04c0c263049b29350a660b4cdd01e": { | ||
| "warning": "The Smile P1 is not connected to a smart meter." | ||
| } | ||
| }, |
There was a problem hiding this comment.
Resolve contradiction between warning and sensor data.
The gateway shows a warning that "The Smile P1 is not connected to a smart meter" (lines 14-18), but the device provides detailed electricity and gas readings (lines 27-53). This appears to be inconsistent.
Please verify if this is the intended behavior or if the warning should be removed.
Also applies to: 27-53
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #698 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 21 +1
Lines 3449 3408 -41
=========================================
- Hits 3449 3408 -41 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
plugwise/legacy/smile.py (7)
43-44: Clarify and document new constructor parameters
The_requestand_smile_propsparameters introduce a new async request logic and configuration data. It would be helpful to add docstrings explaining their roles and any constraints on their usage or lifecycle.
62-63: Ensure consistent error handling for the request
self._requestmay raise exceptions if the device is unreachable. Currently, there is partial handling under KeyError andConnectionFailedError, but consider verifying or enhancing it to handle all relevant exception types.
81-86: Handle potential request errors
Retrieving_domain_objects,_locations,_modules, and_appliancescould fail if the device is offline. Currently, only a KeyError path is handled. Consider capturing other relevant exceptions (ConnectionFailedError) for a robust and user-friendly error boundary.
106-106: Refine debug logging
“HOI First update” might be a leftover debugging string. Consider rewording it for clarity or removing it if no longer needed.
123-128: Partial update checks
For partial updates,_locationsis skipped. Verify if that is intentional and that location data remains valid until the next full update.
138-138: Returning mutable state
return self.gw_entitiesreturns a mutable dictionary. If consumers might unintentionally modify it, consider returning a copy or a read-only proxy.
298-298: Consider surfacing more descriptive errors
Re-raisingConnectionFailedErrorcaptures reachability issues well. To aid debugging, consider logging details (like URI/method) or re-raising with added context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/legacy/smile.py(5 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run pytest using Python 3.12
- GitHub Check: Run pytest using Python 3.13.0
🔇 Additional comments (9)
plugwise/legacy/smile.py (7)
22-22: Validate usage of newly imported entities
SmilePropsandThermoLocare newly imported. Ensure these types are leveraged consistently throughout the codebase and that their usage aligns with the new data-structure design.
53-53: Double-check version usage
Storingsmile_version: Version | Noneis fine, but confirm that downstream logic gracefully handles theNonecase (e.g., if version-based features are disabled or defaulted).
72-72: Confirm version logic
Assigningself.smile_versiondirectly is correct, but confirm if additional validation or fallback (e.g., default version) is needed.
76-76: Initialization clarity
self._first_update = Truehelps differentiate first-run logic, but ensure there are no race conditions if multiple updates run concurrently on startup.
101-101: Updated return type
Theasync_updatemethod returningdict[str, GwEntityData]aligns well with the new single dict output structure. Just confirm that other references match this updated signature.
108-108: Day-change logic
Using weekday (%w) logic to trigger a full update is straightforward but might skip certain edge cases if the system time changes abruptly. If that’s acceptable, no further action is needed.
136-136: State management
self._first_update = Falselooks correct. Just be mindful of any concurrency if multiple updates can overlap.pyproject.toml (2)
7-7: Version bump to 1.7.0a9 aligns with architectural changes.The version change from 1.6.4 to 1.7.0a9 follows semantic versioning principles and correctly reflects the significant architectural changes described in the PR objectives (restructuring data handling, relocating classes).
Let's verify the version change is documented in the changelog:
✅ Verification successful
Version 1.7.0a9 is properly documented under "Ongoing" section
The version bump aligns with the significant architectural changes documented in the "Ongoing" section of CHANGELOG.md, particularly PR #690 which implements the core restructuring work.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if version change is documented rg -A 5 "1\.7\.0" CHANGELOG.md || echo "Version 1.7.0 not found in changelog"Length of output: 115
Script:
#!/bin/bash # Check the structure of CHANGELOG.md echo "=== First 20 lines of CHANGELOG.md ===" head -n 20 CHANGELOG.md echo -e "\n=== Searching for unreleased sections ===" rg -i "unreleased|development|next" CHANGELOG.mdLength of output: 1706
Line range hint
18-19: Consider updating Python version specifications for consistency.The project requires Python >=3.12.0 but includes classifiers for both Python 3.12 and 3.13. The tool configurations (pylint, mypy) are set to Python 3.13. Consider aligning these version specifications for consistency.
Let's check the Python version requirements across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise/legacy/smile.py (1)
122-127: Consider using an enum for_target_smilevalues.The match statement improves readability, but using string literals for
_target_smilevalues could be error-prone. Consider using an enum for type safety.from enum import Enum, auto class SmileTarget(Enum): SMILE_V2 = auto() # Add other targets here # Then use it in the match statement: match self._target_smile: case SmileTarget.SMILE_V2: self._modules = await self._request(MODULES) case target if target in REQUIRE_APPLIANCES: self._appliances = await self._request(APPLIANCES)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/legacy/smile.py(5 hunks)
🔇 Additional comments (5)
plugwise/legacy/smile.py (5)
22-22: LGTM: Import of SmileProps aligns with PR objectives.The addition of
SmilePropssupports the architectural change of converting gateway items to Smile properties.
43-44: LGTM: New parameters improve encapsulation.The addition of
_requestand_smile_propsas constructor parameters improves dependency injection and aligns with the PR's goal of restructuring gateway data handling.
101-107: LGTM: Improved update logic with explicit full update conditions.The return type change to
dict[str, GwEntityData]aligns with the PR's goal of using a single dictionary. The update logic is now more explicit with the_first_updateflag.
135-137: LGTM: Proper state management in update method.The update method correctly manages state by updating
_first_updateand_previous_day_numberbefore returning the gateway entities.
76-76: Verify the initialization of_first_update.The
_first_updateflag controls when to perform full updates. Ensure this doesn't cause unnecessary full updates in edge cases like rapid consecutive calls toasync_update.✅ Verification successful
_first_updateimplementation is safe and efficientThe flag is properly implemented to ensure full updates only occur on first run and day changes, while using incremental updates for all other cases. This prevents unnecessary full updates during rapid consecutive calls to
async_update.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing update throttling or debouncing mechanisms rg -A 5 "first_update|update_interval|throttle|debounce" --type pythonLength of output: 103
Script:
#!/bin/bash # Search for _first_update usage and update-related logic rg -A 10 "_first_update|async_update|update" "*.py" # Also search specifically in the smile.py file cat plugwise/legacy/smile.pyLength of output: 11795
CoMPaTech
left a comment
CoMPaTech
left a comment
There was a problem hiding this comment.
Lot's to digest :) But looking very thorough! Good work
|
Checked off sonar cloud http complaint as well |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (19)
plugwise/legacy/helper.py (1)
181-181: HandleNonefirmware versions carefully.
Ifself.smile_versionisNone, callingstr(self.smile_version)will produce"None", which might be confusing to end users. Consider handling this condition explicitly.- "firmware": str(self.smile_version), + "firmware": str(self.smile_version) if self.smile_version is not None else "unknown",plugwise/__init__.py (2)
191-191: Passsmile_versioncarefully.
Ensure that subsequent operations inSmileLegacyAPIorSmileAPIhandle a potentiallyNoneversion.
226-226: String conversion of major version.
version_major = str(self.smile_version.major)is valid. Confirm no further usage demands minor/patch details.plugwise/data.py (1)
306-309: Enhance error handling in check_reg_mode.The function assumes
gatewaywill have aselect_regulation_modekey. Consider adding a defensive check.def check_reg_mode(self, mode: str) -> bool: """Helper-function for device_data_climate().""" gateway = self.gw_entities[self._gateway_id] - return ( - "regulation_modes" in gateway and gateway["select_regulation_mode"] == mode - ) + return ( + "regulation_modes" in gateway + and "select_regulation_mode" in gateway + and gateway["select_regulation_mode"] == mode + )fixtures/p1v4_442_single/data.json (1)
41-41: Fix Unicode characters in vendor name.The vendor name contains Unicode escape sequences
\uff08and\uff09instead of regular parentheses.- "vendor": "SHENZHEN KAIFA TECHNOLOGY \uff08CHENGDU\uff09 CO., LTD." + "vendor": "SHENZHEN KAIFA TECHNOLOGY (CHENGDU) CO., LTD."fixtures/anna_without_boiler_fw441/data.json (1)
42-42: Consider masking sensitive MAC address.The MAC address is exposed in plain text. Consider masking part of the MAC address in test fixtures to prevent potential security risks.
- "mac_address": "D40FB200FA1C", + "mac_address": "D4:0F:B2:XX:XX:XX",fixtures/legacy_anna/data.json (1)
5-6: Use realistic test data for location IDs.The location IDs use a pattern of zeros and 'a's which is clearly test data. Consider using realistic UUIDs for better test coverage.
- "location": "0000aaaa0000aaaa0000aaaa0000aa00", + "location": "550e8400-e29b-41d4-a716-446655440000",Also applies to: 31-33
fixtures/anna_v4/data.json (2)
1-88: Consider adding JSON schema validation.The file contains well-structured device configurations, but lacks formal schema validation. This could lead to potential issues with malformed data during testing.
Consider adding a JSON schema file to validate:
- Required fields for each device type
- Value ranges for temperature settings
- Enumerated values for modes and states
- Data types for all fields
Would you like me to generate a JSON schema file for validation?
61-72: Document temperature units and ranges.The temperature-related configurations would benefit from explicit documentation of:
- Units (Celsius/Fahrenheit)
- Valid ranges and their real-world implications
- Resolution constraints
fixtures/anna_elga_2_cooling/data.json (1)
39-39: Ensure consistent schedule naming across fixtures.The schedule name "Thermostat schedule" differs from "Winter" used in other fixture files. Consider standardizing schedule names across test fixtures for consistency.
fixtures/anna_loria_cooling_active/data.json (1)
64-71: Schema differences between vendors need documentation.The Atlantic vendor device includes additional features not present in Techneco devices:
- DHW modes: ["off", "auto", "boost", "eco", "comfort"]
- Additional switch: cooling_ena_switch
Consider documenting these vendor-specific differences in the schema documentation.
Also applies to: 90-93
fixtures/anna_loria_heating_idle/data.json (1)
4-4: Remove trailing whitespace in schedule name.The schedule name "Test " contains trailing whitespace which should be removed for consistency.
- "available_schedules": ["Winter", "Test ", "off"], + "available_schedules": ["Winter", "Test", "off"],plugwise/common.py (1)
50-55: Add type hints for class attributes.Consider adding type hints for all class attributes to improve code maintainability and IDE support.
Apply this diff:
- self._cooling_present: bool + self._cooling_present: bool = False self._count: int self._domain_objects: etree - self._heater_id: str = NONE + self._heater_id: str = NONE self._on_off_device: bool - self.gw_entities: dict[str, GwEntityData] = {} + self.gw_entities: dict[str, GwEntityData] = {}plugwise/helper.py (3)
82-84: Consider using a dataclass for initialization.The initialization of multiple related attributes could be simplified using a dataclass to improve code organization and maintainability.
+from dataclasses import dataclass + +@dataclass +class SmileHelperState: + gateway_id: str = NONE + zones: dict[str, GwEntityData] = field(default_factory=dict) + gw_entities: dict[str, GwEntityData] = field(default_factory=dict) class SmileHelper(SmileCommon): def __init__(self) -> None: - self._gateway_id: str = NONE - self._zones: dict[str, GwEntityData] - self.gw_entities: dict[str, GwEntityData] + self._state = SmileHelperState()
628-632: Improve docstring formatting.The docstring formatting could be improved to follow standard conventions.
- """# Anna+Elga: base cooling_state on the elga-status-code. - - For Elga, 'elga_status_code' in (8, 9) means cooling is enabled. - 'elga_status_code' = 8 means cooling is active, 9 means idle. - """ + """Base cooling_state on the elga-status-code for Anna+Elga. + + For Elga: + - 'elga_status_code' in (8, 9) means cooling is enabled + - 'elga_status_code' = 8 means cooling is active + - 'elga_status_code' = 9 means idle + """
654-658: Improve docstring formatting.Similar to the previous comment, the docstring formatting could be improved.
- """Loria/Thermastage: base cooling-related on cooling_state and modulation_level. - - For the Loria or Thermastage heatpump connected to an Anna cooling-enabled is - indicated via the Cooling Enable switch in the Plugwise App. - """ + """Base cooling-related settings on cooling_state and modulation_level for Loria/Thermastage. + + For Loria or Thermastage heatpump connected to an Anna: + Cooling is enabled via the Cooling Enable switch in the Plugwise App. + """fixtures/anna_loria_driessens/data.json (1)
22-28: Standardize schedule naming convention.The schedule names mix languages and formats:
- "Verwarmen@9-23u" (Dutch)
- "VAKANTIE (winter)" (Dutch)
- "VERWARMEN" (Dutch)
- "KOELEN" (Dutch)
- "off" (English)
Consider standardizing to a single language and format for consistency.
fixtures/adam_jip/data.json (1)
Line range hint
581-585: Enhance notification message format.The warning message contains embedded device details that could be structured better:
"warning": "Node Plug (with MAC address 000D6F000D13CB01, in room 'n.a.') has been unreachable since 23:03 2020-01-18..."Consider restructuring as:
"warning": { "device_type": "Node Plug", "mac_address": "000D6F000D13CB01", "room": "n.a.", "last_seen": "2020-01-18T23:03:00", "message": "Device has been unreachable" }fixtures/adam_heatpump_cooling/data.json (1)
34-73: Consider optimizing temperature bounds for energy efficiency.While the configuration is valid, consider reviewing these temperature settings for optimal energy efficiency:
- DHW temperature upper bound (65.0°C) could potentially be lowered
- Maximum boiler temperature setpoint (35.0°C) seems high for a cooling system
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
CHANGELOG.md(1 hunks)fixtures/adam_heatpump_cooling/all_data.json(0 hunks)fixtures/adam_heatpump_cooling/data.json(1 hunks)fixtures/adam_jip/all_data.json(0 hunks)fixtures/adam_jip/data.json(1 hunks)fixtures/adam_multiple_devices_per_zone/all_data.json(0 hunks)fixtures/adam_multiple_devices_per_zone/data.json(1 hunks)fixtures/adam_onoff_cooling_fake_firmware/all_data.json(0 hunks)fixtures/adam_onoff_cooling_fake_firmware/data.json(1 hunks)fixtures/adam_plus_anna/all_data.json(0 hunks)fixtures/adam_plus_anna/data.json(1 hunks)fixtures/adam_plus_anna_new/all_data.json(0 hunks)fixtures/adam_plus_anna_new/data.json(1 hunks)fixtures/adam_plus_anna_new_regulation_off/all_data.json(0 hunks)fixtures/adam_plus_anna_new_regulation_off/data.json(1 hunks)fixtures/adam_zone_per_device/all_data.json(0 hunks)fixtures/adam_zone_per_device/data.json(1 hunks)fixtures/anna_elga_2/all_data.json(0 hunks)fixtures/anna_elga_2/data.json(1 hunks)fixtures/anna_elga_2_cooling/all_data.json(0 hunks)fixtures/anna_elga_2_cooling/data.json(1 hunks)fixtures/anna_elga_2_schedule_off/all_data.json(0 hunks)fixtures/anna_elga_2_schedule_off/data.json(1 hunks)fixtures/anna_elga_no_cooling/all_data.json(0 hunks)fixtures/anna_elga_no_cooling/data.json(1 hunks)fixtures/anna_heatpump_cooling/all_data.json(0 hunks)fixtures/anna_heatpump_cooling/data.json(1 hunks)fixtures/anna_heatpump_cooling_fake_firmware/all_data.json(0 hunks)fixtures/anna_heatpump_cooling_fake_firmware/data.json(1 hunks)fixtures/anna_heatpump_heating/all_data.json(0 hunks)fixtures/anna_heatpump_heating/data.json(1 hunks)fixtures/anna_loria_cooling_active/all_data.json(0 hunks)fixtures/anna_loria_cooling_active/data.json(1 hunks)fixtures/anna_loria_driessens/all_data.json(0 hunks)fixtures/anna_loria_driessens/data.json(1 hunks)fixtures/anna_loria_heating_idle/all_data.json(0 hunks)fixtures/anna_loria_heating_idle/data.json(1 hunks)fixtures/anna_v4/all_data.json(0 hunks)fixtures/anna_v4/data.json(1 hunks)fixtures/anna_v4_dhw/all_data.json(0 hunks)fixtures/anna_v4_dhw/data.json(1 hunks)fixtures/anna_v4_no_tag/all_data.json(0 hunks)fixtures/anna_v4_no_tag/data.json(1 hunks)fixtures/anna_without_boiler_fw441/all_data.json(0 hunks)fixtures/anna_without_boiler_fw441/data.json(1 hunks)fixtures/legacy_anna/all_data.json(0 hunks)fixtures/legacy_anna/data.json(1 hunks)fixtures/legacy_anna_2/all_data.json(0 hunks)fixtures/legacy_anna_2/data.json(1 hunks)fixtures/m_adam_cooling/all_data.json(0 hunks)fixtures/m_adam_cooling/data.json(1 hunks)fixtures/m_adam_heating/all_data.json(0 hunks)fixtures/m_adam_heating/data.json(1 hunks)fixtures/m_adam_jip/all_data.json(0 hunks)fixtures/m_adam_jip/data.json(1 hunks)fixtures/m_adam_multiple_devices_per_zone/all_data.json(0 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(1 hunks)fixtures/m_anna_heatpump_cooling/all_data.json(0 hunks)fixtures/m_anna_heatpump_cooling/data.json(1 hunks)fixtures/m_anna_heatpump_idle/all_data.json(0 hunks)fixtures/m_anna_heatpump_idle/data.json(1 hunks)fixtures/p1v4_442_single/all_data.json(0 hunks)fixtures/p1v4_442_single/data.json(1 hunks)fixtures/p1v4_442_triple/all_data.json(0 hunks)fixtures/p1v4_442_triple/data.json(1 hunks)fixtures/smile_p1_v2/all_data.json(0 hunks)fixtures/smile_p1_v2/data.json(1 hunks)fixtures/smile_p1_v2_2/all_data.json(0 hunks)fixtures/smile_p1_v2_2/data.json(1 hunks)fixtures/stretch_v23/all_data.json(0 hunks)fixtures/stretch_v23/data.json(1 hunks)fixtures/stretch_v27_no_domain/all_data.json(0 hunks)fixtures/stretch_v27_no_domain/data.json(1 hunks)fixtures/stretch_v31/all_data.json(0 hunks)fixtures/stretch_v31/data.json(1 hunks)plugwise/__init__.py(11 hunks)plugwise/common.py(2 hunks)plugwise/constants.py(2 hunks)plugwise/data.py(5 hunks)plugwise/helper.py(12 hunks)plugwise/legacy/data.py(2 hunks)plugwise/legacy/helper.py(5 hunks)
⛔ Files not processed due to max files limit (12)
- plugwise/legacy/smile.py
- plugwise/smile.py
- plugwise/smilecomm.py
- plugwise/util.py
- pyproject.toml
- scripts/manual_fixtures.py
- tests/test_adam.py
- tests/test_anna.py
- tests/test_generic.py
- tests/test_init.py
- tests/test_legacy_anna.py
- tests/test_p1.py
💤 Files with no reviewable changes (37)
- fixtures/stretch_v23/all_data.json
- fixtures/adam_plus_anna/all_data.json
- fixtures/p1v4_442_single/all_data.json
- fixtures/m_adam_heating/all_data.json
- fixtures/adam_zone_per_device/all_data.json
- fixtures/anna_loria_driessens/all_data.json
- fixtures/adam_onoff_cooling_fake_firmware/all_data.json
- fixtures/adam_jip/all_data.json
- fixtures/m_adam_cooling/all_data.json
- fixtures/anna_loria_cooling_active/all_data.json
- fixtures/anna_heatpump_heating/all_data.json
- fixtures/anna_elga_2/all_data.json
- fixtures/adam_plus_anna_new_regulation_off/all_data.json
- fixtures/legacy_anna/all_data.json
- fixtures/stretch_v27_no_domain/all_data.json
- fixtures/m_anna_heatpump_idle/all_data.json
- fixtures/adam_multiple_devices_per_zone/all_data.json
- fixtures/adam_heatpump_cooling/all_data.json
- fixtures/anna_elga_no_cooling/all_data.json
- fixtures/adam_plus_anna_new/all_data.json
- fixtures/m_adam_jip/all_data.json
- fixtures/m_adam_multiple_devices_per_zone/all_data.json
- fixtures/anna_elga_2_schedule_off/all_data.json
- fixtures/stretch_v31/all_data.json
- fixtures/legacy_anna_2/all_data.json
- fixtures/anna_loria_heating_idle/all_data.json
- fixtures/anna_v4_no_tag/all_data.json
- fixtures/anna_heatpump_cooling/all_data.json
- fixtures/smile_p1_v2/all_data.json
- fixtures/anna_elga_2_cooling/all_data.json
- fixtures/anna_v4/all_data.json
- fixtures/anna_v4_dhw/all_data.json
- fixtures/anna_without_boiler_fw441/all_data.json
- fixtures/p1v4_442_triple/all_data.json
- fixtures/anna_heatpump_cooling_fake_firmware/all_data.json
- fixtures/smile_p1_v2_2/all_data.json
- fixtures/m_anna_heatpump_cooling/all_data.json
👮 Files not reviewed due to content moderation or server errors (5)
- fixtures/anna_v4_no_tag/data.json
- fixtures/anna_elga_no_cooling/data.json
- fixtures/anna_elga_2_schedule_off/data.json
- fixtures/m_anna_heatpump_idle/data.json
- fixtures/m_anna_heatpump_cooling/data.json
🧰 Additional context used
📓 Learnings (1)
fixtures/m_adam_heating/data.json (1)
Learnt from: bouwew
PR: plugwise/python-plugwise#630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-11-12T13:49:49.714Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
🔇 Additional comments (80)
fixtures/adam_onoff_cooling_fake_firmware/data.json (4)
1-112: LGTM! Well-structured device configuration data.The JSON structure is clean, consistent, and follows best practices with proper use of UUIDs as unique identifiers and a clear hierarchy of device configurations.
2-38: Verify cooling and heating states consistency.The central heater configuration shows both cooling_enabled and cooling_state as true, while heating_state is false, which is the expected behavior for a cooling operation. However, please verify that:
- The modulation_level of 0.0 is expected during cooling operation
- The water_temperature (24.5°C) and return_temperature (24.9°C) values are reasonable for the cooling mode
80-111: Verify climate control state consistency.The climate unit shows:
- control_state: "idle"
- climate_mode: "auto"
- active_preset: "away"
- setpoint: 21.5°C
Please verify if this combination of states is valid:
- Should the control_state be "idle" when cooling is active in the system?
- Is the "away" preset compatible with maintaining a 21.5°C setpoint?
39-67: Verify firmware version and regulation modes.The gateway configuration shows firmware version "3.2.8". Please verify:
- Is this the latest available firmware version for the AME Smile 2.0 board?
- Do all regulation modes listed (especially "bleeding_cold" and "bleeding_hot") match the current firmware's capabilities?
✅ Verification successful
Test fixture configuration is valid for its purpose
This is a test fixture specifically designed to test cooling functionality, as indicated by the directory name "adam_onoff_cooling_fake_firmware". While firmware version 3.2.8 is not the latest version, it is a valid version used in other test fixtures. The unique regulation modes, including "cooling", are intentionally configured for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other firmware versions in the fixtures echo "Searching for firmware versions in fixtures..." rg -g '*.json' '"firmware":\s*"[0-9.]*"' ./fixturesLength of output: 2600
Script:
#!/bin/bash # Search for regulation modes in fixtures echo "Searching for regulation modes..." rg -g '*.json' '"regulation_modes":\s*\[.*\]' ./fixturesLength of output: 713
plugwise/legacy/helper.py (4)
38-40: Use of specialized utilities for power and data counting is a solid approach.
These imports fromplugwise.util(collect_power_valuesandcount_data_items) promote modularity and cleaner code. Ensure that any exceptional cases (e.g., malformed inputs causing errors in these utilities) are handled, either here or in the calling functions.
75-75: Consistency in version naming.
Renamingself.smile_fw_versiontoself.smile_versionmakes the naming more uniform. Verify that all references to the old property name have been updated to prevent attribute errors.
312-312: Recommend confirming the return structure ofcollect_power_values.
Using the external functioncollect_power_valuesdirectly allows for better modularity. Ensure that all calling sites correctly handle any edge cases or errors that might arise from this utility, especially if incomplete data is provided.
345-345: Confirm the logic for item counting.
count_data_itemsupdates the entity count in a single step, reducing potential duplication. Double-check that subsequent code doesn't inadvertently rely on the old counting mechanism, particularly in dependent methods like_get_entity_data.plugwise/__init__.py (16)
20-21: Expanded imports for new data structures.
IncorporatingGwEntityDataandSmilePropscentralizes entity and property handling. Confirm that these imports are used consistently across relevant modules.
34-34: Explicit import forSmileComm.
BringingSmileComminto__init__.pyclarifies dependencies and code organization. No issues identified.
55-60: Constructor alignment with super class.
Passinghost,password,port,username, andwebsessiondirectly tosuper().__init__clarifies the flow and reduces duplication. Ensure that defaults (e.g., timeouts) remain correctly configured insideSmileComm.
72-72: Introducing a dedicated_smile_props.
Storing relevant Smile properties in_smile_propsaligns with the new design and simplifies property access. Double-check that all read/write operations are consistent throughout the codebase.
86-92: Propertycooling_presentwith a safe fallback.
This property gracefully returnsFalseif"cooling_present"is missing from_smile_props. This is a good defensive approach.
93-97: Gateway ID property lacks a fallback.
gateway_iddirectly indexes_smile_props["gateway_id"]without a fallback in case the key is absent. Confirm thatgateway_idalways gets set; otherwise, this might raise aKeyError.
98-104: Heater ID property with partial fallback.
Here, you handle the missing"heater_id"case by returningNONE. This approach is consistent with fallback logic.
105-109: Item count is exposed elegantly.
Theitem_countproperty references_smile_props["item_count"]. Ensure_smile_props["item_count"]is consistently set and updated alongside changes ingw_entities.
110-117:rebootproperty for non-legacy devices.
Returningnot self.smile_legacyclarifies that modern devices can reboot. This is correct usage, as legacy might not support reboots.
163-165: Combined usage of_requestand_smile_props.
When instantiatingSmileAPIorSmileLegacyAPI, passing_requestand_smile_propssignals a clear separation of networking logic (in_request) vs. data storage (in_smile_props). Maintain consistent usage patterns to avoid confusion.
181-182: Re-confirm references to_smile_propswithinSmileLegacyAPI.
Sharing data through_smile_propsensures a single source of truth. Double-check that unexpected mutation inSmileLegacyAPIwon't break theSmileclass state.
210-210: Extracting firmware version from gateway info.
Reading'firmware_version'fromgateway.find("firmware_version").textis appropriate. Validate that the XML field is always present or handle the situation if it's absent or invalid.
218-218: Check formodel == "Unknown"orself.smile_version is None.
This explicit check prevents the code from continuing when insufficient device info is provided. Good defensive logic.
298-298: Legacy detection with updated approach.
Parsing firmware fromsystem.find("./gateway/firmware").textin legacy code ensures consistency with modern logic. Confirm that older legacy devices indeed place firmware version under<gateway><firmware>inSYSTEMdata.
309-309: Fallback toSTATUSfor some legacy setups.
This approach ensures coverage of devices providing version details under<system><version>. Continue to monitor for devices that might require additional fallback.
324-326: Switching fromPlugwiseDatatodict[str, GwEntityData].
Returning a standard dictionary clarifies data structures and reduces maintenance overhead. Ensure all consumer code adjusts accordingly to accept the new return type.plugwise/legacy/data.py (4)
10-10: Importing new types for cohesive data management.
Bringing inGwEntityDataandSmilePropssupports the unified approach for gateway and property data introduced across the refactoring.
20-20: Introducing_smile_props.
Definingself._smile_props: SmilePropsinsideSmileLegacyDataaligns with the new design scheme, centralizing gateway-related properties.
26-26: Invocation of_update_gw_entities.
_update_gw_entities()ensures the gateway entities are refreshed before populating_smile_props. Confirm no concurrency issues whengw_entitiesupdates simultaneously in other parts of the code.
29-33: Populating_smile_propswith gateway attributes.
Storinggateway_id,item_count,smile_name, and optionallyheater_idcentralizes metadata. This is consistent with the rest of the refactor. Validate that all references to these keys (e.g., in__init__.pyor any consumer code) are aligned with these new storage patterns.plugwise/data.py (3)
30-31: LGTM: New attributes align with PR objectives.The addition of
_smile_propsand_zonesattributes supports the consolidation of data handling into a single dictionary structure.
123-131: Review the notification handling changes.The changes integrate notifications into the gateway data structure and modify the counter increment. Consider the following:
- The condition for notification handling has been improved to include power-type smiles
- Counter increment changed from +1 to +2 to account for both binary sensor and notifications dictionary
44-50: Verify data migration from gw_data to _smile_props.The gateway data has been moved to
_smile_props. Let's verify that all consumers of this data have been updated accordingly.✅ Verification successful
Migration from
gw_datato_smile_propsis complete and correctThe codebase shows a clean migration with all necessary properties moved to
_smile_propsand no remaining references togw_datain the actual code. Both main and legacy implementations have been properly updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to gw_data rg "gw_data" -A 3Length of output: 377
Script:
#!/bin/bash # Search for any variations of gw_data rg -i "gw.*data" -A 3 # Search for _smile_props usage rg "_smile_props" -A 3Length of output: 30551
fixtures/smile_p1_v2_2/data.json (1)
1-34: Verify test data completeness.The fixture provides comprehensive test data for both smart meter and gateway entities. However, consider adding test cases for:
- Notifications (currently missing in gateway data)
- Binary sensors (currently missing in gateway data)
fixtures/smile_p1_v2/data.json (1)
1-34: LGTM: Test data maintains consistency.The fixture follows the same structure as smile_p1_v2_2/data.json, providing good test coverage for different device IDs.
fixtures/p1v4_442_single/data.json (1)
3-15: LGTM: Comprehensive gateway test data.The gateway entity includes notifications and binary sensors, providing good test coverage for the consolidated data structure.
fixtures/anna_without_boiler_fw441/data.json (1)
26-31: Verify temperature bounds and resolution.Please ensure that the thermostat bounds (4.0-30.0°C) and resolution (0.1°C) align with the actual device specifications.
fixtures/legacy_anna_2/data.json (2)
56-59: Validate zero sensor values.Multiple sensor values are set to 0.0, which might indicate missing data or sensor errors. Consider adding test cases for non-zero values to ensure proper handling of actual readings.
3-3: Resolve preset inconsistency.The
active_presetis null whilepreset_modescontains valid options andselect_scheduleis set to "off". This might indicate an invalid state.Also applies to: 13-14
fixtures/p1v4_442_triple/data.json (1)
14-18: Resolve contradiction between warning and sensor data.The gateway shows a warning that "The Smile P1 is not connected to a smart meter", but the device provides detailed electricity and gas readings. This appears to be inconsistent.
Also applies to: 27-53
fixtures/anna_v4_dhw/data.json (1)
67-72: Review boiler temperature bounds for safety.The maximum boiler temperature bounds (0-100°C) seem unusually wide. Consider reviewing these values:
- Lower bound of 0°C might be too low and could cause freezing issues
- Upper bound of 100°C might exceed safety limits for residential boilers
✅ Verification successful
Temperature bounds are appropriate for this device type
The maximum boiler temperature bounds (0-100°C) are consistent with other modern heating systems in the codebase, particularly for Anna v4 and heat pump configurations. The setpoint of 70°C is well within safe operating ranges for residential boilers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other boiler temperature bounds across the codebase rg '"maximum_boiler_temperature".*"lower_bound".*"upper_bound"' --type jsonLength of output: 77
Script:
#!/bin/bash # Find all JSON files that might contain boiler settings echo "=== Searching for JSON files with boiler settings ===" rg -i "boiler.*temperature" --type json -A 5 -B 5 echo -e "\n=== Looking for documentation about boiler settings ===" fd -e md -e txt | xargs rg -i "boiler.*temperature|temperature.*limit" echo -e "\n=== Checking for similar configuration files ===" fd -e json | xargs rg -i "temperature.*bound|bound.*temperature"Length of output: 68908
fixtures/anna_v4/data.json (1)
1-88: Verify test coverage for all device configurations.The test fixtures cover various device configurations:
- Basic heating setup (anna_v4)
- Cooling capabilities (anna_elga_2)
- Different vendors (Bosch/Techneco)
However, consider adding test fixtures for:
- Edge cases in temperature ranges
- All possible device states
- Error conditions
Run this script to analyze test coverage:
fixtures/anna_elga_2/data.json (2)
17-24: Verify temperature ranges for different heating systems.The configuration includes multiple temperature settings with different ranges:
- DHW: 30.0-60.0°C (0.01°C resolution)
- Boiler: 0.0-100.0°C (1.0°C resolution)
- Room: 4.0-30.0°C (0.1°C resolution)
Consider documenting:
- Why these specific ranges were chosen
- How they relate to real-world heating systems
- Impact of different resolutions
Also applies to: 61-66
44-51: Validate cooling threshold logic.The cooling configuration includes activation temperature (24.0°C) and deactivation threshold (2.0°C), but their relationship should be validated to prevent issues like:
- Short cycling of the cooling system
- Conflicts between heating and cooling modes
Run this script to verify cooling thresholds across fixtures:
✅ Verification successful
Cooling thresholds are valid and consistent with other fixtures
The cooling activation temperature (24.0°C) and deactivation threshold (2.0°C) are within the normal range observed across similar configurations, which use activation temperatures between 21-26°C and thresholds between 2-6°C.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cooling threshold logic echo "Analyzing cooling thresholds..." for file in fixtures/anna*/data.json; do echo "File: $file" jq -r '.[] | select(.sensors.cooling_activation_outdoor_temperature != null) | { name: .name, activation: .sensors.cooling_activation_outdoor_temperature, deactivation_threshold: .sensors.cooling_deactivation_threshold, climate_mode: .climate_mode }' "$file" doneLength of output: 7075
fixtures/anna_elga_2_cooling/data.json (1)
50-57: Verify temperature sensor values are within operational bounds.The temperature readings show:
- Room temperature: 24.9°C
- Setpoint high: 23.0°C
The current room temperature (24.9°C) is above the cooling setpoint (23.0°C) while cooling is active, which correctly represents a cooling scenario.
fixtures/anna_heatpump_cooling/data.json (1)
23-30: Verify modulation level during cooling operation.The device shows:
- Cooling state: active
- Modulation level: 40%
- Other heating states: inactive
This represents a valid combination of states during cooling operation.
Also applies to: 45-45
fixtures/anna_heatpump_cooling_fake_firmware/data.json (1)
7-7: Verify firmware version format consistency.The firmware version "4.10.10" follows a different pattern compared to other fixtures. Ensure this represents a valid version number for testing firmware-specific behavior.
✅ Verification successful
Firmware version format is consistent with testing purposes
The version "4.10.10" follows the same semantic versioning pattern (major.minor.patch) as other fixtures, and its presence in a fixture explicitly named "fake_firmware" suggests it's intentionally different for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check firmware version patterns across fixtures echo "Analyzing firmware version patterns:" rg -g "*.json" '"firmware": "4' fixtures/Length of output: 1295
fixtures/anna_loria_heating_idle/data.json (1)
57-62: Verify idle state consistency.The idle state shows consistent values:
- All heating/cooling states: false
- Modulation level: 0.0
- No active temperature control
This represents a valid combination for an idle system.
Also applies to: 85-85
fixtures/stretch_v31/data.json (2)
88-100: Verify member references in switching group.The switching group references two members:
- 059e4d03c7a34d278add5c7a4a781d19 (Wasmachine)
- cfe95cf3de1948c0b8955125bf754614 (Droger)
Both members exist in the file and support switching operations.
101-116: Verify member references in report group.The report group references five members, all of which exist in the file:
- 059e4d03... (Wasmachine)
- 587131... (Boiler)
- aac7b7... (Vaatwasser)
- cfe95c... (Droger)
- e1c884... (Koelkast)
fixtures/m_adam_cooling/data.json (2)
74-89: Gateway configuration is correctly set for cooling mode.The gateway configuration shows:
- Cooling mode is available in regulation_modes
- select_regulation_mode is set to "cooling"
Also applies to: 90-92
145-148: Climate mode settings are consistent with cooling operation.The climate devices are correctly configured:
- climate_mode: "cool"
- control_state: "cooling"
Also applies to: 229-231
fixtures/m_adam_heating/data.json (2)
88-90: Gateway configuration is correctly set for heating mode.The gateway configuration shows:
- Heating mode is available in regulation_modes
- select_regulation_mode is set to "heating"
145-147: Climate mode settings are consistent with heating operation.The climate devices are correctly configured:
- climate_mode: "heat"/"auto"
- control_state: "preheating"/"idle"
Also applies to: 178-180
fixtures/adam_plus_anna_new_regulation_off/data.json (2)
19-22: Verify safe temperature settings in regulation off mode.The intended boiler temperature is set to 0.0°C which could potentially lead to freezing. Consider implementing a minimum safe temperature even when regulation is off.
229-230: Climate mode settings are consistent with regulation off.The climate devices are correctly configured:
- climate_mode: "off"
- control_state: "idle"
Also applies to: 262-263
fixtures/adam_plus_anna_new/data.json (2)
172-174: Gateway configuration is correctly set for normal operation.The gateway configuration shows:
- Heating mode is available in regulation_modes
- select_regulation_mode is set to "heating"
229-230: Climate mode settings are consistent with normal operation.The climate devices are correctly configured:
- climate_mode: "auto"
- control_state: "heating"/"preheating"
Also applies to: 262-263
fixtures/stretch_v27_no_domain/data.json (2)
71-89: Inconsistent device class naming.The device class "zz_misc" appears to be a temporary or placeholder name. Consider using a more descriptive and standardized name.
1-271: LGTM! Well-structured JSON data.The JSON structure is well-organized with consistent attribute naming and proper nesting of objects.
plugwise/common.py (1)
30-42: LGTM! Well-structured helper function.The function handles both legacy and new cases clearly with proper error handling.
fixtures/m_adam_jip/data.json (1)
1-370: Verify temperature setpoints and readings.Several temperature-related values require verification:
- Low setpoints: Multiple zones have setpoints of 9.0°C and 13.0°C, which seem unusually low for comfort.
- High readings: Several temperature sensors show readings of 30.0°C, which might indicate overheating.
Please verify if these values are intentional or if they need adjustment.
fixtures/stretch_v23/data.json (2)
293-297: Verify high power consumption for TV device.The TV device shows unusually high power consumption:
- Consumed: 33.3 units
- Interval: 4.93 units
Please verify if these values are expected for this device type.
1-354: LGTM! Well-structured device data.The JSON structure effectively organizes various device types with consistent attribute naming and proper power consumption tracking.
plugwise/constants.py (2)
Line range hint
396-406: Well-structured TypedDict for gateway properties!The
SmilePropsclass is well-designed with appropriate field types and total=False for optional fields, making it flexible for partial data representation.
553-553: Good addition of notifications field!The notifications field with type
dict[str, dict[str, str]]is well-structured for handling nested notification data, aligning with the JSON structure in the fixtures.fixtures/m_adam_multiple_devices_per_zone/data.json (1)
Line range hint
1-593: Well-structured device configuration data!The JSON structure maintains consistency across different device types while properly representing their unique characteristics. The mock MAC addresses are appropriately used for testing purposes.
fixtures/adam_zone_per_device/data.json (1)
1-590: Consistent with established data structure!The fixture maintains the same well-structured format as other fixtures, properly representing device configurations with different locations.
plugwise/helper.py (1)
47-47: LGTM! Good modularization.The functions
collect_power_valuesandcount_data_itemshave been properly extracted into utility functions, improving code modularity.Also applies to: 49-49
fixtures/anna_heatpump_heating/data.json (3)
33-38: Verify the DHW temperature setpoint is within safe operating range.The maximum DHW temperature setpoint of 53.0°C with bounds [35.0, 60.0] might be too low for legionella prevention in some regions. Many safety guidelines recommend periodic heating to 60°C or higher.
Consider:
- Adding a periodic high-temperature cycle for legionella prevention
- Validating against local safety regulations
39-44: Validate maximum boiler temperature configuration.The maximum boiler temperature has a lower bound of 0.0°C which could allow for unsafe conditions:
- Risk of freezing damage to the system
- Potential condensation issues
Consider setting a minimum safe temperature (typically 20-25°C) to prevent system damage.
82-87: Review temperature offset configuration.The temperature offset configuration looks good with reasonable bounds (-2.0 to 2.0) and appropriate resolution (0.1).
fixtures/anna_loria_driessens/data.json (1)
78-83: Review maximum boiler temperature configuration.The maximum boiler temperature configuration has tight constraints:
- Upper and setpoint both at 45.0°C
- Lower bound at 25.0°C
This is a safe configuration that prevents both freezing and overheating.fixtures/adam_plus_anna/data.json (2)
87-91: Review notification structure and error handling.The notification structure provides good error tracking:
"notifications": { "6fb89e35caeb4b1cb275184895202d84": { "error": "There is no OpenTherm communication with the boiler." } }The UUID-based notification system allows for proper tracking and management of multiple issues.
62-67: Validate electricity consumption metrics.The sensor data includes both interval and cumulative measurements:
"sensors": { "electricity_consumed": 10.3, "electricity_consumed_interval": 0.0, "electricity_produced": 0.0, "electricity_produced_interval": 0.0 }This provides good granularity for power monitoring and billing purposes.
fixtures/adam_jip/data.json (1)
Line range hint
473-479: Monitor low battery levels.Several devices show battery levels below 70%:
- "battery": 62 (current device)
- Other devices show similar low levels
Consider implementing a proactive battery replacement strategy when levels drop below 75% to prevent device failures.
fixtures/adam_multiple_devices_per_zone/data.json (2)
25-31: Standardize schedule naming convention.Similar to previous files, schedule names lack consistency:
"available_schedules": [ "CV Roan", "Bios Schema met Film Avond", "GF7 Woonkamer", "Badkamer Schema", "CV Jessie", "off" ]Refer to previous comment about standardizing schedule names.
366-370: Review valve position and temperature differential.The configuration shows:
- Valve position: 100 (fully open)
- Temperature difference: 3.5°C
- Current temp: 26.0°C vs Setpoint: 21.5°C
This suggests potential inefficiency in the heating system. Consider implementing PID control for better temperature regulation.
fixtures/adam_heatpump_cooling/data.json (3)
285-313: Gateway configuration looks good!The gateway device configuration is well-structured with all necessary attributes including:
- Firmware version (3.2.8)
- Hardware details (AME Smile 2.0 board)
- Supported modes for both heating and cooling
- Properly obfuscated MAC addresses
212-233: Consistent thermostat configuration across zones.The thermostat configurations show good consistency in their structure:
- Uniform resolution (0.01) across all thermostats
- Consistent temperature offset ranges (-2.0 to 2.0)
- Well-defined setpoints for each zone
Also applies to: 652-672
153-171: Smart plug configurations are consistent and well-structured.All smart plug devices follow a consistent pattern with:
- Power monitoring capabilities
- Relay state tracking
- Firmware version 2020-05-13T02:00:00+02:00
Also applies to: 575-594
CHANGELOG.md (1)
Line range hint
1-807: Changelog follows best practices.The changelog is well-maintained with:
- Clear version numbering
- Descriptive entries for each change
- Proper linking to issues and PRs
|
All changes:
async_update()so that the testcode can be simpler --> 2 smile-helper-functions can be removedSummary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Refactoring
Bug Fixes
Version Update