Optimize updating #835
Optimize updating #835
Conversation
WalkthroughPropagates module_id from module XML into appliance and gateway entities, adds lifecycle tracking and pruning for groups/appliances/locations/zones, introduces thermostat ranking and ZigBee availability checks, and updates many fixtures/tests and types to include module_id. Changes
Sequence Diagram(s)sequenceDiagram
participant SmileAPI
participant Common
participant Helper
participant Module
participant Gw as gw_entities
Note over SmileAPI,Common: module_id propagation & lifecycle changes
SmileAPI->>Common: get_all_gateway_entities()
Common->>Module: _get_module_data(module element)
Module-->>Common: module_data (includes module_id)
Common->>Helper: create/appliance handlers (appl.module_id = module_data.module_id)
Helper->>Helper: _get_appliances() (collect, mark _new_appliances)
Helper->>Helper: _match_and_rank_thermostats()
Helper->>Helper: _get_zigbee_availability(entity)
Helper->>Gw: _create_gw_entities() (include module_id, availability, zones)
rect rgb(230,245,255)
Note over Common: Group lifecycle handling
Common->>Common: _get_groups() (collect members, append _new_groups)
Common->>Common: skip unchanged, prune removed groups from gw_entities
end
Gw-->>SmileAPI: populated entities (with module_id, availability, zones)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugwise/common.py (1)
196-236: Critical: Fix variable name in group tracking logic.Line 209 uses
self._existing_appliancesbut should useself._existing_groupsfor group tracking. This is a copy-paste error that will cause the group existence check to reference the wrong state, potentially leading to incorrect duplicate group handling.🔎 Apply this diff to fix the variable name:
group_id = group.attrib["id"] self._new_groups.append(group_id) - if group_id in self._existing_appliances: + if group_id in self._existing_groups: continuetests/data/adam/adam_plus_anna_new.json (1)
1-404: Fix the formatting to pass the Biome (prettier) check.The pipeline failure indicates that the JSON formatting doesn't meet the project's style requirements. Run the suggested command to auto-fix:
pre-commit run --show-diff-on-failure --hook-stage manual biome
🧹 Nitpick comments (2)
plugwise/helper.py (2)
102-185: Remove debug markers from log messages.Lines 166 and 169 contain "HOI" prefix in debug messages, which appears to be leftover debug/test text.
🔎 Apply this diff to clean up the debug messages:
if not removed: - LOGGER.debug("HOI no new or removed appliance(s).") + LOGGER.debug("No new or removed appliance(s).") return False else: - LOGGER.debug("HOI removed appliance(s): %s", removed) + LOGGER.debug("Removed appliance(s): %s", removed) for appliance in removed:
423-431: Fix typo in comment and consider adding explicit return statement.Line 426 has a typo: "availabilty" should be "availability".
Additionally, this method modifies
datain place but has no return statement. While Python functions without explicit returns implicitly returnNone, the method signature suggests it might be intended to returnGwEntityData. Consider either:
- Adding
-> Noneto the signature to clarify intent, or- Adding
return dataif the modified data should be returned🔎 Apply this diff to fix the typo and clarify return type:
def _get_zigbee_availability( self, data: GwEntityData, entity: GwEntityData - ) -> GwEntityData: - # Check zigbee device availabilty + ) -> None: + # Check zigbee device availability if "module_id" in entity: module_id = entity["module_id"] locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node' if (module := self._domain_objects.find(locator)) is not None: data["available"] = module.find("reachable").text == "true"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
plugwise/common.py(9 hunks)plugwise/constants.py(3 hunks)plugwise/data.py(1 hunks)plugwise/helper.py(8 hunks)plugwise/legacy/helper.py(2 hunks)plugwise/legacy/smile.py(1 hunks)plugwise/smile.py(2 hunks)tests/data/adam/adam_plus_anna_new.json(16 hunks)tests/test_adam.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/legacy/helper.pyplugwise/helper.py
📚 Learning: 2024-12-13T11:26:00.100Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 671
File: plugwise/legacy/data.py:97-106
Timestamp: 2024-12-13T11:26:00.100Z
Learning: In `plugwise/legacy/data.py`, the legacy thermostat does not support cooling, so handling of the cooling state is unnecessary in this code.
Applied to files:
plugwise/smile.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
tests/data/adam/adam_plus_anna_new.json
📚 Learning: 2025-11-09T09:22:20.172Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 814
File: plugwise/constants.py:254-262
Timestamp: 2025-11-09T09:22:20.172Z
Learning: In `plugwise/constants.py`, `ACTIVE_KEYS` contains only the keys that exist as original actuators in the raw Plugwise device data. Keys like `setpoint_high` and `setpoint_low` are NOT original actuators—they are derived/translated later for Home Assistant integration, so they should not be included in `ACTIVE_KEYS` even though they appear in the `ActuatorData` TypedDict.
Applied to files:
tests/data/adam/adam_plus_anna_new.json
🧬 Code graph analysis (4)
plugwise/legacy/smile.py (2)
plugwise/helper.py (1)
_get_appliances(102-185)plugwise/legacy/helper.py (1)
_get_appliances(86-138)
plugwise/legacy/helper.py (1)
plugwise/helper.py (2)
_get_appliances(102-185)_get_locations(224-256)
plugwise/data.py (1)
plugwise/helper.py (1)
_get_zigbee_availability(423-431)
plugwise/helper.py (2)
plugwise/common.py (3)
_create_gw_entities(151-171)_reorder_devices(173-181)check_name(69-74)plugwise/constants.py (1)
ThermoLoc(507-513)
🪛 GitHub Actions: Latest commit
tests/data/adam/adam_plus_anna_new.json
[error] 212-404: Biome (prettier) formatting failed. The formatter would rewrite multiple lines to fix formatting in this JSON data. Run 'pre-commit run --show-diff-on-failure --hook-stage manual biome' to apply fixes.
🔇 Additional comments (16)
plugwise/constants.py (1)
289-289: LGTM! Clean type extension for module_id tracking.The addition of
module_idfields toApplianceType,ModuleData, andGwEntityDatais consistent and properly typed as optional fields, supporting the PR's objective to track module identifiers for differential updates.Also applies to: 421-421, 544-544
plugwise/helper.py (4)
76-81: LGTM! State tracking initialization supports differential updates.The new state-tracking lists (
_existing_appliances,_new_appliances, etc.) enable detection of added/removed entities, which is central to the PR's optimization goals.
797-828: LGTM! Zone tracking with differential updates.The refactored
_scan_thermostatsproperly tracks existing and new zones, removing stale zones when detected. The early return for non-Adam environments is appropriate.
830-880: LGTM! Clear thermostat ranking logic.The new
_match_and_rank_thermostatsand_rank_thermostatmethods provide a clean separation of concerns for thermostat priority handling. The ranking dictionary and priority logic are well-structured.
163-172: The appliance update logic correctly returnsFalsefor subsequent runs but only on first initialization—this is intentional, not a bug.The full method shows that
Trueis returned only when_existing_appliancesis empty (the initial run). On subsequent calls:
- If
_existing_applianceshas items and no removals occurred, it returnsFalse(no changes)- If
_existing_applianceshas items and removals occurred, it processes the removals and returnsFalse- New appliances added after the first run are handled by
_create_gw_entities()during iteration, but the method still returnsFalseat the endThe caller uses
if self._get_appliances():to detect the initial population. The return value doesn't track subsequent changes—it only signals the first-time setup. New appliances are created inline via_create_gw_entities()rather than signaled through the return value, and removed appliances are handled viagw_entities.pop()regardless of the return value.tests/test_adam.py (1)
37-37: LGTM! Test expectation updated for module_id fields.The increased entity count from 230 to 241 reflects the addition of
module_idfields across entities, consistent with the data model changes in this PR.plugwise/legacy/smile.py (1)
84-84: LGTM! Method rename aligns with refactored helper.The change from
_all_appliances()to_get_appliances()is consistent with the broader refactoring inplugwise/helper.py.plugwise/data.py (1)
202-203: LGTM! ZigBee availability integration.The call to
_get_zigbee_availability(data, entity)properly integrates ZigBee node availability checking into the entity data collection flow. The method modifiesdatain place, which is consistent with other data collection methods in this file.plugwise/legacy/helper.py (1)
86-86: LGTM! Method renames align with non-legacy module.The renames from
_all_appliancesto_get_appliancesand_all_locationsto_get_locationsmaintain consistency with the refactoredplugwise/helper.py.Also applies to: 89-89, 140-140
plugwise/smile.py (2)
91-92: LGTM! Proper initialization of public attributes.Initializing
gw_entitiesand_zonesas empty dictionaries in__init__is good practice, making these attributes explicitly available throughout the object's lifetime.
112-120: LGTM! Conditional appliance gathering enables differential updates.The refactored
get_all_gateway_entitiesnow:
- Conditionally processes appliances via
_get_appliances()(returnsTrueon first run only)- Always runs
_scan_thermostats()to ensure zones are current- Calls
_get_groups()for group dataThis structure supports the PR's goal of avoiding redundant entity detection on every update.
plugwise/common.py (2)
57-57: LGTM! Group state tracking initialization.The addition of
_existing_groupsand_new_groupstracking lists enables differential group updates, consistent with the PR's optimization goals.Also applies to: 59-59
121-121: LGTM! Consistent module_id propagation.The
module_idfield is properly extracted from module data and propagated through appliance info builders (_appl_heater_central_info,_appl_thermostat_info) intogw_entitiesvia_create_gw_entities.Also applies to: 146-146, 163-163
tests/data/adam/adam_plus_anna_new.json (3)
18-18: LGTM:module_idfields added consistently to device entries.The addition of
module_idfields to individual device entries aligns with the PR objective to enable differential update tracking. The fields are consistently placed and properly formatted.Also applies to: 33-33, 51-51, 79-79, 104-104, 126-126, 147-147, 166-166, 185-185, 251-251, 273-273
214-218: Array reformatting improves readability.The multi-line array formatting for
gateway_modes,regulation_modes,preset_modes,zone_profiles, andthermostatsmakes the test fixture more readable and git diffs clearer. This will be properly formatted when the Biome formatting issue is resolved.Also applies to: 226-231, 320-326, 349-353, 369-375, 390-395, 398-402
193-207: Groups correctly omitmodule_id; this is not an oversight.Groups are logical collections of physical devices and should not have
module_id. The codebase explicitly constructs groups with onlydev_class,model,name,members, andvendorfields. Themodule_idfield is reserved for physical devices/modules and is used for zigbee availability checks. Other test files (e.g.,stretch_v31.json) follow the same pattern, confirming this is the intended design.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugwise/helper.py (1)
222-254: Consider whether location name changes need to be detected.The skip logic at lines 229-230 prevents reprocessing existing locations. While thermostat data in locations is updated elsewhere (
_scan_thermostats), if a location's name changes on the gateway, it won't be reflected in_loc_data.If location names are expected to be stable after creation, this is acceptable. Otherwise, consider comparing and updating mutable properties.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.py(9 hunks)plugwise/helper.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (2)
_get_locations(140-165)_appliance_info_finder(192-205)plugwise/common.py (3)
_create_gw_entities(151-171)_reorder_devices(173-181)check_name(69-74)plugwise/constants.py (2)
GwEntityData(529-592)ThermoLoc(507-513)
🔇 Additional comments (14)
plugwise/common.py (7)
57-59: LGTM! State tracking initialization is correct.The addition of
_existing_groupsand_new_groupstracking lists aligns with the PR objective to optimize updates by detecting changes rather than reprocessing all entities.
121-121: LGTM! Module ID propagation is consistent.The
module_idis correctly extracted frommodule_dataand assigned to the appliance object, enabling better device tracking across updates.
146-146: LGTM! Module ID propagation for thermostats is correct.Consistent with the heater/central path, ensuring uniform module tracking across device types.
163-163: LGTM! Module ID integration into entity data is correct.The
module_idis properly included in the entity creation dictionary and will only be added togw_entitieswhen it has a value, maintaining data consistency.
272-272: LGTM! Module ID initialization is correct.The
module_idfield is properly initialized toNonein theModuleDatadictionary, consistent with other fields.
296-296: LGTM! Module ID extraction from XML is correct.The
module_idis properly extracted from the module'sidattribute, only when a module is found.
196-236: Verify whether group membership updates are needed between calls.The refactored
_get_groupsskips reprocessing existing groups (lines 209-210). This means that if a group's appliance members change between updates, the entity stored ingw_entities[group_id]["members"]won't be refreshed, potentially resulting in stale membership data.Clarify the intended behavior: Is skipping existing groups an optimization assuming members never change, or should group members be re-collected on every update?
plugwise/helper.py (7)
76-81: LGTM! State tracking initialization is consistent.The six tracking lists for appliances, locations, and zones follow a consistent pattern and properly support the optimized update logic described in the PR objectives.
113-126: LGTM! Explicit attribute initialization improves code robustness.Initializing all appliance attributes upfront prevents potential
AttributeErrorexceptions and makes the data structure more predictable.
127-151: LGTM! Skip logic and location handling are well-commented.The logic correctly handles edge cases for OpenTherm Gateway, orphaned thermostats, and location assignments. The comments clearly explain the intent.
185-220: LGTM! P1 smartmeter integration is correct.The refactored method properly handles P1 smartmeter entity creation with module_id propagation (line 209) and maintains backward compatibility with the gateway/smartmeter entity_id swap.
288-288: LGTM! Module ID propagation for plug types is consistent.The
module_idassignment follows the same pattern as other appliance types, ensuring uniform tracking.
795-878: LGTM! Thermostat ranking logic is well-structured.The refactored
_scan_thermostatsnow correctly:
- Limits execution to Adam devices only (line 802)
- Implements sophisticated thermostat ranking with promotion/demotion logic
- Tracks zones using the same state management pattern as other entities
- Properly handles multiple thermostats per location with priority-based ranking
The ranking algorithm correctly handles edge cases like multiple thermostats with equal priority and promotion scenarios where former primaries are demoted to secondary.
102-183: Return value logic is correct. The function returnsTrueonly on first run (when_existing_appliancesis empty) andFalseon subsequent runs. This intentional behavior is used inplugwise/smile.pyto conditionally initialize thermostat offset functionality only during the first appliance collection. The code is working as designed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugwise/helper.py (3)
163-170: Simplify redundant conditional branches.Both branches return
False, so the conditional check fornot removedis unnecessary.🔎 Apply this diff to simplify the logic:
removed = list(set(self._existing_appliances) - set(self._new_appliances)) if self._existing_appliances: - if not removed: - return False - else: - for appliance in removed: - self.gw_entities.pop(appliance) - return False + for appliance in removed: + self.gw_entities.pop(appliance) + return False
820-823: Use safe dictionary pop to prevent potential KeyError.While
_existing_zonesshould only contain valid zone IDs, using.pop(location_id, None)provides defensive error handling and is more resilient to edge cases.🔎 Apply this diff for safer removal:
removed = list(set(self._existing_zones) - set(self._new_zones)) if self._existing_zones and removed: for location_id in removed: - self._zones.pop(location_id) + self._zones.pop(location_id, None)
833-838: Consider extracting thermostat priority mapping as a class or module constant.The
thermo_matchingdictionary defines static priority values that don't change across invocations. Extracting it as a class attribute or module-level constant would improve code organization and make the priorities more discoverable.Example extraction as class attribute:
Add near the top of the class (after line 75):
_THERMOSTAT_PRIORITIES: dict[str, int] = { "thermostat": 2, "zone_thermometer": 2, "zone_thermostat": 2, "thermostatic_radiator_valve": 1, }Then simplify the method:
def _match_and_rank_thermostats(self) -> dict[str, ThermoLoc]: """Helper-function for _scan_thermostats(). Match thermostat-appliances with locations, rank them for locations with multiple thermostats. """ for location_id, location in self._loc_data.items(): for entity_id, entity in self.gw_entities.items(): self._rank_thermostat( entity_id, entity, location_id, location, self._THERMOSTAT_PRIORITIES )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (2)
_get_locations(140-165)_appliance_info_finder(192-205)plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)plugwise/constants.py (1)
ThermoLoc(507-513)
🔇 Additional comments (4)
plugwise/helper.py (4)
76-81: LGTM! Clean state-tracking initialization.The initialization of paired
_existing_*and_new_*lists for appliances, locations, and zones provides a clear pattern for delta-tracking across update cycles.
185-220: LGTM! Proper integration of P1 smartmeter as an entity.The refactoring from
_get_p1_smartmeter_infoto_add_p1_smartmeter_infocorrectly integrates P1 smartmeter data collection directly into the entity creation flow, including propermodule_idpropagation.
421-429: Previous return type concern appears resolved.The past review comment flagged a return type annotation issue, but the current code correctly shows
-> Nonewith in-place mutation of thedataparameter. The implementation is now consistent with the signature.
845-879: LGTM! Thermostat ranking logic correctly handles priorities and demotion.The implementation properly ranks thermostats by priority level, correctly handles multiple primaries at the same priority, and appropriately demotes former primaries when a higher-priority thermostat is found. The use of the walrus operator for checking and assigning
tl_primaryis appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise/common.py (1)
196-235: Approve the group tracking optimization, but fix the member collection.The refactored
_get_groupsmethod successfully implements incremental updates by tracking existing groups and pruning removed ones. However, there's a critical issue with member collection.Issue: Line 216 calls
self._add_member(item)with only one argument, but the helper method at line 236 expects(element, members). Thememberslist defined at line 206 is never populated because it's not passed to the helper.🔎 Fix the member collection by passing the members list:
group_appliances = group.findall("appliances/appliance") for item in group_appliances: - self._add_member(item) + self._add_member(item, members)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.py(8 hunks)plugwise/smile.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/smile.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
🪛 GitHub Actions: Latest commit
plugwise/common.py
[error] 236-236: N805 First argument of a method should be named self
[error] 238-238: F821 Undefined name self
[error] 239-239: F821 Undefined name item
[error] 236-239: Ruff check failed with 3 errors in plugwise/common.py. No fixes available (use '--unsafe-fixes' to enable hidden fixes).
🔇 Additional comments (6)
plugwise/common.py (6)
57-57: LGTM: Group tracking state initialized.The new instance variables
_existing_groupsand_new_groupsappropriately support incremental group updates.Also applies to: 59-59
121-121: LGTM: Module ID propagated to heater/central appliance.The assignment correctly propagates
module_idfrommodule_datato the appliance object.
146-146: LGTM: Module ID propagated to thermostat appliance.Consistent with the heater/central path at line 121.
163-163: LGTM: Module ID included in gateway entities.The field is correctly added to the
gw_entitiesdictionary, completing the module ID propagation chain.
275-275: LGTM: Module ID field initialized.The
module_idfield is properly initialized in theModuleDatadictionary.
299-299: LGTM: Module ID extracted from XML.The module ID is correctly extracted from the module element's
idattribute.
bouwew
force-pushed
the
different-update
branch
from
3256e85 to
abf6cec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise/helper.py (1)
225-259: Fix potential KeyError when removing locations.At line 256,
self._zones.pop(location_id)can raiseKeyErrorfor locations that have no corresponding zone entry. Not all locations are ThermoZones—only those with thermostats have zone entries.🔎 Apply this diff to safely remove zone entries:
removed = list(set(self._existing_locations) - set(self._new_locations)) if self._existing_locations and removed: for location_id in removed: self._loc_data.pop(location_id) + self._zones.pop(location_id, None) - # Remove line 257 if it exists, or ensure line 256 uses the safe pop with defaultNote: The above assumes line 257 is
self._zones.pop(location_id). If the code structure differs slightly, useself._zones.pop(location_id, None)for the zones removal.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/common.py(8 hunks)plugwise/constants.py(4 hunks)plugwise/helper.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (2)
plugwise/common.py (3)
_create_gw_entities(151-171)_reorder_devices(173-181)check_name(69-74)plugwise/constants.py (2)
GwEntityData(535-598)ThermoLoc(513-519)
🔇 Additional comments (13)
plugwise/constants.py (2)
95-100: LGTM! Well-structured thermostat ranking constant.The
THERMO_MATCHINGconstant provides a clear priority scheme for thermostat device types, with higher values indicating higher priority. This enables the ranking logic inhelper.pyto determine primary vs. secondary thermostats per location.
427-427: LGTM! Consistent module_id field additions.The
module_idfield is correctly added to bothModuleDataandGwEntityDataTypedDicts as an optional string, enabling end-to-end propagation of module identifiers through the data collection pipeline.Also applies to: 550-550
plugwise/common.py (4)
57-57: LGTM! State tracking for optimized group updates.The
_existing_groupsand_new_groupslists enable incremental updates by tracking which groups are already processed, consistent with the PR's optimization objectives.Also applies to: 59-59
196-234: LGTM! Efficient group processing with incremental updates.The refactored
_get_groupsmethod now:
- Tracks new groups and skips reprocessing existing ones (lines 208-210)
- Prunes removed groups from
gw_entities(lines 228-231)- Updates state trackers for the next run (lines 233-234)
This optimization aligns perfectly with the PR objective to avoid redetecting all entities on every update.
236-239: LGTM! Helper validates members before appending.The
_add_memberhelper safely checks that each group member exists ingw_entitiesbefore adding it to the members list, preventing invalid references.
121-121: LGTM! Consistent module_id propagation throughout the data flow.The
module_idis correctly:
- Initialized to
Nonein_get_module_data(line 275)- Populated from the module XML attribute when found (line 299)
- Propagated to appliance objects in heater_central and thermostat paths (lines 121, 146)
- Included in
gw_entitiesentries (line 163)This enables tracking of OpenTherm/driver modules across the system.
Also applies to: 146-146, 163-163, 275-275, 299-299
plugwise/helper.py (7)
77-82: LGTM! State tracking enables incremental updates.The tracking lists for appliances, locations, and zones support the PR's core optimization: only updating changed entities instead of redetecting everything on each update cycle.
103-179: LGTM! Optimized appliance collection with change detection.The refactored
_get_appliancesmethod (renamed from_all_appliances) now:
- Tracks new appliances and skips reprocessing existing ones (lines 158-160)
- Detects and removes stale appliances (lines 171-174)
- Returns
Trueon first full discovery,Falseon subsequent updates (lines 175-179)This aligns with the PR objective to avoid full redetection on every update.
181-223: LGTM! P1 smartmeter handling maintains backward compatibility.The renamed
_add_p1_smartmeter_infomethod correctly:
- Collects module data from the Home location
- Propagates
module_id(line 212)- Maintains backward compatibility by swapping entity IDs for non-Anna P1 devices (lines 219-221)
- Detects changes via module_id to avoid redundant re-adds (lines 196-200, though see separate comment about KeyError risk)
426-434: LGTM! ZigBee availability integrated into entity data.The
_get_zigbee_availabilitymethod correctly:
- Checks for
module_idin the entity- Looks up the ZigBee node reachability in
_domain_objects- Updates the
datadict in-place withavailablestatus- Uses the correct return type
-> None(addressed from past review)
800-831: LGTM! Thermostat scanning optimized with incremental zone updates.The refactored
_scan_thermostatsmethod now:
- Early-returns for non-Adam devices (lines 806-807)
- Uses the new ranking logic to determine primary/secondary thermostats per location (line 809)
- Creates zones only for locations with ranked thermostats (lines 811-823)
- Prunes removed zones (lines 825-828)
- Updates state trackers (lines 830-831)
This optimization avoids rescanning all thermostats on every update.
833-877: LGTM! Robust thermostat ranking with priority-based primary selection.The
_match_and_rank_thermostatsand_rank_thermostatmethods implement a sound ranking algorithm:
- Match thermostats to their locations (lines 838-842)
- Use
THERMO_MATCHINGpriorities to rank thermostat types (line 841)- Automatically demote former primary thermostats when a higher-priority device is found (lines 871-873)
- Support multiple primary thermostats of the same priority (lines 864-865)
- Correctly handle secondary thermostats (line 877)
Based on learnings, the code properly handles location-thermostat relationships in the Plugwise system.
120-120: LGTM! Module_id propagated to plug devices.The
module_idis correctly extracted frommodule_dataand assigned to appliance objects for plug-type devices (line 293) and included in gateway entity initialization (line 120), maintaining consistency with the broader module_id propagation effort.Also applies to: 293-293
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/data/adam/adam_multiple_devices_per_zone.json (1)
1-683: Run the formatter to fix formatting differences.The pipeline indicates that Biome would format this file differently. Please run the formatter to resolve the formatting issues.
#!/bin/bash # Run the formatter on the file to fix formatting npx @biomejs/biome format --write tests/data/adam/adam_multiple_devices_per_zone.jsontests/data/anna/anna_v4.json (1)
1-100: Run the formatter to fix formatting differences.The pipeline indicates that Biome would format this file differently. Please run the formatter to resolve the formatting issues.
#!/bin/bash # Run the formatter on the file to fix formatting npx @biomejs/biome format --write tests/data/anna/anna_v4.jsontests/data/adam/adam_plus_anna.json (1)
1-144: Run the formatter to fix formatting differences.The pipeline indicates that Biome would format this file differently. Please run the formatter to resolve the formatting issues.
#!/bin/bash # Run the formatter on the file to fix formatting npx @biomejs/biome format --write tests/data/adam/adam_plus_anna.jsontests/data/adam/adam_zone_per_device.json (1)
1-667: Run the formatter to fix formatting differences.The pipeline indicates that Biome would format this file differently. Please run the formatter to resolve the formatting issues.
#!/bin/bash # Run the formatter on the file to fix formatting npx @biomejs/biome format --write tests/data/adam/adam_zone_per_device.jsontests/data/adam/adam_jip.json (1)
1-462: Fix the formatting to match project's formatter requirements.The pipeline failure indicates that Biome/prettier would format this file differently. Run the formatter to resolve the CI/CD blocker.
#!/bin/bash # Description: Run the formatter to fix the formatting issue # Check if biome is available, otherwise try prettier if command -v biome &> /dev/null; then biome format --write tests/data/adam/adam_jip.json elif command -v prettier &> /dev/null; then prettier --write tests/data/adam/adam_jip.json else echo "Neither biome nor prettier found. Check package.json for the correct formatter command." cat package.json 2>/dev/null | jq '.scripts | to_entries[] | select(.key | contains("format") or contains("lint"))' fi
♻️ Duplicate comments (2)
plugwise/helper.py (2)
195-200: Fix potential KeyError when checking smartmeter module_id.The direct dictionary access at lines 197-198 can raise
KeyErrorif the keys don't exist ingw_entities. This issue was previously flagged but remains unaddressed.🔎 Apply this diff to safely check module_id
module_id = module_data["module_id"] if module_id in ( - self.gw_entities[self._gateway_id].get("module_id"), + self.gw_entities.get(self._gateway_id, {}).get("module_id"), self.gw_entities.get(self._home_loc_id, {}).get("module_id"), ): return
254-257: Fix potential KeyError when removing locations without zones.Not all locations have corresponding zone entries in
_zones. Usingpop()without a default will raiseKeyErrorif a location without a zone is removed. This issue was previously flagged but remains unaddressed.🔎 Apply this diff to safely remove entries
removed = list(set(self._existing_locations) - set(self._new_locations)) if self._existing_locations and removed: for location_id in removed: self._loc_data.pop(location_id) + self._zones.pop(location_id, None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugwise/__init__.py(1 hunks)plugwise/helper.py(11 hunks)tests/data/adam/adam_heatpump_cooling.json(42 hunks)tests/data/adam/adam_jip.json(19 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(25 hunks)tests/data/adam/adam_onoff_cooling_fake_firmware.json(4 hunks)tests/data/adam/adam_plus_anna.json(6 hunks)tests/data/adam/adam_zone_per_device.json(26 hunks)tests/data/anna/anna_v4.json(2 hunks)tests/test_adam.py(7 hunks)tests/test_anna.py(17 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/data/adam/adam_onoff_cooling_fake_firmware.json
- tests/data/adam/adam_heatpump_cooling.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
tests/data/anna/anna_v4.jsontests/data/adam/adam_jip.jsontests/data/adam/adam_zone_per_device.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_multiple_devices_per_zone.jsonplugwise/helper.py
📚 Learning: 2025-11-09T09:22:20.172Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 814
File: plugwise/constants.py:254-262
Timestamp: 2025-11-09T09:22:20.172Z
Learning: In `plugwise/constants.py`, `ACTIVE_KEYS` contains only the keys that exist as original actuators in the raw Plugwise device data. Keys like `setpoint_high` and `setpoint_low` are NOT original actuators—they are derived/translated later for Home Assistant integration, so they should not be included in `ACTIVE_KEYS` even though they appear in the `ActuatorData` TypedDict.
Applied to files:
tests/data/adam/adam_jip.jsontests/data/adam/adam_zone_per_device.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_multiple_devices_per_zone.json
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
🪛 GitHub Actions: Latest commit
tests/data/anna/anna_v4.json
[error] 1-1: Biome (prettier) formatter would have printed different content: formatting differences detected in 'tests/data/anna/anna_v4.json format'. Run the formatter to fix.
tests/data/adam/adam_jip.json
[error] 1-1: Biome (prettier) formatter would have printed different content: formatting differences detected in 'tests/data/adam/adam_jip.json format'. Run the formatter to fix.
tests/data/adam/adam_zone_per_device.json
[error] 1-1: Biome (prettier) formatter would have printed different content: formatting differences detected in 'tests/data/adam/adam_zone_per_device.json format'. Run the formatter to fix.
tests/data/adam/adam_plus_anna.json
[error] 1-1: Biome (prettier) formatter would have printed different content: formatting differences detected in 'tests/data/adam/adam_plus_anna.json format'. Run the formatter to fix.
tests/data/adam/adam_multiple_devices_per_zone.json
[error] 1-1: Biome (prettier) formatter would have printed different content: formatting differences detected in 'tests/data/adam/adam_multiple_devices_per_zone.json format'. Run the formatter to fix.
🔇 Additional comments (9)
plugwise/__init__.py (1)
337-337: LGTM! Debug logging addition is helpful for troubleshooting.The added debug log provides visibility into the HOI data returned during updates, which aligns with the PR's optimization objective of selective updates.
plugwise/helper.py (5)
77-82: LGTM! State tracking fields enable incremental updates.The new tracking structures
_existing_appliances,_existing_locations,_existing_zonesand their_new_*counterparts enable the PR's optimization goal of detecting and updating only changed entities rather than re-detecting everything on each update.
225-259: LGTM! Location tracking enables incremental location updates.The new
_get_locationsmethod implements tracking and pruning of locations similar to appliances, supporting the PR's incremental update optimization.
801-832: LGTM! Thermostat scanning now leverages zone tracking.The refactored
_scan_thermostatsmethod now uses the zone tracking structures (_existing_zones,_new_zones) to detect and remove orphaned zones, consistent with the PR's optimization strategy.
834-878: LGTM! Thermostat matching and ranking logic is well-structured.The new
_match_and_rank_thermostatsand_rank_thermostatmethods properly organize thermostat-location associations with priority-based ranking, supporting Adam's multi-thermostat zones.
103-179: Return value semantics verified as correct.The method returns
Trueon first run andFalseon incremental updates. Both call sites handle the return value appropriately:
plugwise/smile.py:112uses it in a boolean condition to execute special processing only on first runplugwise/legacy/smile.py:84ignores it (acceptable, as the method still executes)tests/test_anna.py (1)
32-32: LGTM! Entity count updates reflect module_id additions.The entity count assertions have been consistently incremented by +2 across all Anna test scenarios, aligning with the addition of
module_idfields to the test data fixtures. This is a straightforward test expectation update with no logic changes.Also applies to: 104-104, 133-133, 161-161, 189-189, 219-219, 244-244, 290-290, 316-316, 339-339, 359-359, 390-390, 443-443, 508-508, 531-531, 554-554, 575-575
tests/test_adam.py (1)
37-37: LGTM! Entity count updates reflect module_id additions.The entity count assertions have been updated across all Adam test scenarios to reflect the addition of
module_idfields to the test data fixtures. The varying increments (+1 to +21) correspond to the number of devices with new module_id fields in each test scenario. This is a straightforward test expectation update with no logic changes.Also applies to: 231-231, 305-305, 342-342, 366-366, 391-391, 432-432
tests/data/adam/adam_jip.json (1)
95-95: module_id additions look correct.The addition of
module_idfields to device entries is consistent with the PR's objective to optimize update tracking. All applicable devices have received the new metadata field with properly formatted UUID values.Also applies to: 119-119, 144-144, 167-167, 190-190, 214-214, 242-242, 339-339, 420-420, 445-445
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugwise/helper.py (1)
252-256: Add safe removal of zone entries.When removing locations, you also need to remove any corresponding zone entries. Not all locations have zones (zones are only created for locations with thermostats), so use
.pop()with a default to preventKeyError.🔎 Apply this diff to safely remove zone entries
removed = list(set(self._existing_locations) - set(self._new_locations)) if self._existing_locations and removed: for location_id in removed: self._loc_data.pop(location_id) + self._zones.pop(location_id, None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
🔇 Additional comments (9)
plugwise/helper.py (9)
77-82: State trackers initialized correctly.The six new tracking lists support the differential update optimization described in the PR objectives. Initialization as empty lists is appropriate.
171-179: Clarify the return value semantics.The method now returns
bool, but the semantics are unclear:
- Returns
Falsewhen there are removals (line 175)- Returns
Truewhen_existing_appliancesis empty, indicating a first/full scan (implicit at line 179)This appears to signal whether a full initial scan occurred vs. a differential update, but the naming and usage context would benefit from clarification. Consider adding a docstring update or comment explaining when
TruevsFalseis returned and how callers should interpret this.
195-200: Duplicate prevention logic looks safe.The check uses
.get()with appropriate defaults to safely access dictionary entries, preventing theKeyErrorconcern from the previous review. This correctly prevents re-adding the smartmeter if it already exists with the same module_id.
232-236: Home location tracking updated correctly.The code ensures
_home_locationis refreshed on every update, which aligns with the commit message indicating this is an explicit fix to keep the home location state current during differential updates.
425-433: ZigBee availability collection implemented correctly.The method signature correctly indicates
-> Nonesince it modifiesdatain-place, which addresses the previous review concern about return type consistency.
800-831: Thermostat scanning refactored correctly.The simplified Adam-driven workflow correctly delegates ranking to
_match_and_rank_thermostats, creates zones for locations with primary thermostats, and safely handles zone removal with proper state tracking.
844-877: Thermostat ranking logic implemented correctly.The ranking algorithm properly handles multiple thermostats per location:
- Equal priority: adds to primary list (line 865)
- Higher priority: demotes existing primaries to secondary and crowns new primary (lines 869-875)
- Lower priority: adds to secondary list (line 877)
This correctly implements the per-location ranking system described in the AI summary.
158-162: Differential update optimization implemented correctly.The logic tracks new appliances and skips re-processing entities that already exist in
_existing_appliances(lines 159-161). This achieves the PR objective of avoiding full redetection on every update, with only truly new appliances being passed to_create_gw_entities.
238-250: Location data caching implemented correctly.The optimization skips rebuilding location data when the location already exists with an unchanged name (lines 239-243). The data structure (lines 245-250) correctly includes all fields needed for thermostat ranking.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugwise/helper.py (2)
682-682: Remove or update informal debug statement.The debug log contains "HOI" which appears to be an informal/development debug message. Update it to a professional message like "Gateway outdoor temperature: %s" or remove it if redundant.
🔎 Suggested fix
- LOGGER.debug("HOI outdoor_temp = %s", value) + LOGGER.debug("Gateway outdoor temperature: %s", value)
836-846: Fix the return type annotation.The method signature indicates it returns
dict[str, ThermoLoc](line 836), but the implementation doesn't return anything. It only modifiesself._loc_datain-place through the_rank_thermostatcalls.🔎 Apply this diff to fix the return type
- def _match_and_rank_thermostats(self) -> dict[str, ThermoLoc]: + def _match_and_rank_thermostats(self) -> None:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (2)
plugwise/common.py (3)
_create_gw_entities(151-171)_reorder_devices(173-181)check_name(69-74)plugwise/constants.py (2)
GwEntityData(535-598)ThermoLoc(513-519)
🔇 Additional comments (9)
plugwise/helper.py (9)
77-82: LGTM! State tracking structures support differential updates.The new tracking lists (
_existing_appliances,_new_appliances,_existing_locations,_new_locations,_existing_zones,_new_zones) align with the PR objective to detect and update only changed devices, locations, and zones rather than redetecting everything on each update.
103-182: LGTM! Differential update logic correctly implemented.The refactored
_get_appliances()method now:
- Returns
boolto indicate full vs. incremental detection- Tracks existing vs. new appliances to detect removals
- Skips recreation when entity exists with unchanged name (lines 158-163)
- Properly manages state transitions (lines 180-182)
This aligns with the PR objective to avoid redetecting all devices on every update.
198-203: LGTM! Safe dictionary access prevents KeyError.The module_id check correctly uses
.get()with defaults to safely access potentially missing keys, preventing the KeyError issue flagged in previous reviews. The early return avoids duplicate smartmeter entries.
228-261: LGTM! Differential location tracking implemented correctly.The
_get_locations()method properly:
- Identifies the Home location (lines 235-239)
- Skips recreation when location exists with unchanged name (lines 242-246)
- Initializes location data structure with thermostat tracking fields (lines 248-253)
- Handles removal of stale locations from
_loc_data(lines 255-259)Note: The previous review concern about
KeyErrorwhen removing locations without zones has been resolved—zones are no longer popped here.
128-145: Orphaned device filtering logic is clear and well-documented.The updated logic correctly:
- Skips
open_therm_gatewayappliances and orphaned thermostats (lines 128-134)- Assigns default location for non-thermostat devices (lines 138-141)
- Filters out thermostat-types without locations (lines 144-145)
The comments effectively explain the filtering rationale, referencing Core #81712.
148-152: LGTM! Plug device classification enhancement.The logic correctly extends the device class name for Plugwise and Aqara smart plugs by checking the description field, addressing Pw-Beta Issue #739.
283-299: LGTM! Module ID propagation for plug devices.The addition of
module_idassignment (line 295) aligns with the PR's objective to propagate module identifiers through the module/appliance/entity flow for differential updates.
428-436: LGTM! ZigBee availability check correctly mutates in-place.The method correctly:
- Uses
-> Nonereturn type since it modifiesdatain-place- Safely checks for
module_idpresence before accessing- Integrates ZigBee availability into entity data
803-834: LGTM! Thermostat scanning refactored for differential updates.The refactored method correctly:
- Delegates matching/ranking to
_match_and_rank_thermostats()(line 812)- Creates zones only for locations with primary thermostats (lines 813-826)
- Handles removal of stale zones with proper state tracking (lines 828-834)
The Adam-specific guard (lines 809-810) and zone creation logic align with the PR objective to update only changed zones.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
===========================================
- Coverage 100.00% 99.77% -0.23%
===========================================
Files 21 21
Lines 3435 3513 +78
===========================================
+ Hits 3435 3505 +70
- Misses 0 8 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise/helper.py (1)
258-262: Potential KeyError when pruning locations.Based on the learnings, not all locations have corresponding zone entries in
_zones. Line 262 removes from_loc_databut doesn't safely remove from_zones. This could raiseKeyErrorif a location without a zone is removed.🔎 Apply this diff to safely remove zone entries
removed = list(set(self._existing_locations) - set(self._new_locations)) if self._existing_locations and removed: for location_id in removed: self._loc_data.pop(location_id) + self._zones.pop(location_id, None)Based on learnings, ...
🧹 Nitpick comments (1)
plugwise/helper.py (1)
685-685: Replace informal debug message.The debug message contains "HOI" which appears to be an informal development placeholder. Consider replacing with a descriptive message like "Gateway outdoor temperature: %s" or removing if redundant.
🔎 Suggested update
- LOGGER.debug("HOI outdoor_temp = %s", value) + LOGGER.debug("Gateway outdoor temperature: %s", value)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
fixtures/adam_heatpump_cooling/data.json(21 hunks)fixtures/adam_jip/data.json(10 hunks)fixtures/adam_multiple_devices_per_zone/data.json(16 hunks)fixtures/adam_onoff_cooling_fake_firmware/data.json(1 hunks)fixtures/adam_plus_anna/data.json(4 hunks)fixtures/adam_plus_anna_new/data.json(11 hunks)fixtures/adam_plus_anna_new_regulation_off/data.json(9 hunks)fixtures/adam_zone_per_device/data.json(16 hunks)fixtures/anna_elga_2/data.json(2 hunks)fixtures/anna_elga_2_cooling/data.json(2 hunks)fixtures/anna_elga_2_schedule_off/data.json(2 hunks)fixtures/anna_elga_no_cooling/data.json(2 hunks)fixtures/anna_heatpump_cooling/data.json(2 hunks)fixtures/anna_heatpump_cooling_fake_firmware/data.json(2 hunks)fixtures/anna_heatpump_heating/data.json(2 hunks)fixtures/anna_loria_cooling_active/data.json(2 hunks)fixtures/anna_loria_driessens/data.json(2 hunks)fixtures/anna_loria_heating_idle/data.json(2 hunks)fixtures/anna_p1/data.json(3 hunks)fixtures/anna_v4/data.json(2 hunks)fixtures/anna_v4_dhw/data.json(2 hunks)fixtures/anna_v4_no_tag/data.json(2 hunks)fixtures/anna_without_boiler_fw441/data.json(1 hunks)fixtures/legacy_anna/data.json(2 hunks)fixtures/legacy_anna_2/data.json(2 hunks)fixtures/m_adam_cooling/data.json(6 hunks)fixtures/m_adam_heating/data.json(6 hunks)fixtures/m_adam_jip/data.json(10 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(16 hunks)fixtures/m_anna_heatpump_cooling/data.json(2 hunks)fixtures/m_anna_heatpump_idle/data.json(2 hunks)fixtures/p1v4_442_single/data.json(1 hunks)fixtures/p1v4_442_triple/data.json(1 hunks)plugwise/helper.py(10 hunks)plugwise/legacy/helper.py(4 hunks)plugwise/legacy/smile.py(1 hunks)tests/data/adam/adam_heatpump_cooling.json(21 hunks)tests/data/adam/adam_jip.json(10 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(16 hunks)tests/data/adam/adam_onoff_cooling_fake_firmware.json(1 hunks)tests/data/adam/adam_plus_anna.json(4 hunks)tests/data/adam/adam_plus_anna_new.json(11 hunks)tests/data/adam/adam_zone_per_device.json(16 hunks)tests/data/anna/anna_v4.json(2 hunks)tests/test_anna.py(16 hunks)tests/test_legacy_anna.py(2 hunks)tests/test_p1.py(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- fixtures/anna_loria_cooling_active/data.json
- fixtures/m_anna_heatpump_idle/data.json
- fixtures/p1v4_442_triple/data.json
- tests/test_anna.py
- fixtures/anna_v4_dhw/data.json
- fixtures/m_adam_multiple_devices_per_zone/data.json
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/data/adam/adam_heatpump_cooling.json
- tests/data/adam/adam_plus_anna.json
- tests/data/anna/anna_v4.json
- tests/data/adam/adam_jip.json
- tests/data/adam/adam_multiple_devices_per_zone.json
- plugwise/legacy/smile.py
- plugwise/legacy/helper.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
fixtures/adam_multiple_devices_per_zone/data.jsonfixtures/m_adam_heating/data.jsonfixtures/anna_elga_no_cooling/data.jsonfixtures/anna_heatpump_cooling_fake_firmware/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_elga_2/data.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsonfixtures/adam_onoff_cooling_fake_firmware/data.jsontests/data/adam/adam_onoff_cooling_fake_firmware.jsonfixtures/legacy_anna_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/m_adam_cooling/data.jsonfixtures/anna_heatpump_heating/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_v4/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/m_anna_heatpump_cooling/data.jsontests/data/adam/adam_zone_per_device.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/adam_heatpump_cooling/data.jsonfixtures/legacy_anna/data.jsonplugwise/helper.pyfixtures/adam_jip/data.jsontests/data/adam/adam_plus_anna_new.jsonfixtures/m_adam_jip/data.json
📚 Learning: 2024-10-06T16:03:27.250Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-10-06T16:03:27.250Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
Applied to files:
fixtures/anna_elga_2/data.jsonfixtures/legacy_anna_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/adam_heatpump_cooling/data.json
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (2)
plugwise/common.py (3)
_create_gw_entities(151-171)_reorder_devices(173-181)check_name(69-74)plugwise/constants.py (2)
GwEntityData(535-598)ThermoLoc(513-519)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (47)
fixtures/anna_heatpump_heating/data.json (2)
46-46: LGTM! Module ID addition is structurally correct.The module_id field has been properly added to the OpenTherm device with correct JSON syntax and formatting. The value follows the expected 32-character hex string format.
72-72: LGTM! Module ID addition is structurally correct.The module_id field has been properly added to the Anna thermostat with correct JSON syntax and formatting. The value follows the expected 32-character hex string format, consistent with the other module_id addition.
Note: The gateway device (lines 2-19) doesn't receive a module_id, which appears intentional—likely because it acts as a coordinator rather than a physical module.
fixtures/adam_onoff_cooling_fake_firmware/data.json (1)
73-73: LGTM! Module ID addition aligns with PR objectives.The addition of the
module_idfield to the thermostat entry is syntactically correct and consistent with the PR's goal of enabling differential updates by tracking device relationships.fixtures/anna_elga_2_schedule_off/data.json (2)
22-22: LGTM! Module ID fields added correctly.The
module_idfields have been properly added to both the OpenTherm heater and Anna thermostat entries with valid UUID-format values, supporting the differential update mechanism described in the PR objectives.Also applies to: 48-48
75-92: The gateway device intentionally lacks amodule_idfield. This is consistent with the codebase design pattern, as all gateway devices across the entire fixture suite in python-plugwise similarly excludemodule_id. The field is reserved for specific appliance types (heater_central, thermostat) and is not part of the gateway schema.Likely an incorrect or invalid review comment.
fixtures/anna_elga_no_cooling/data.json (1)
44-44: LGTM! module_id additions align with Anna thermostat fixtures.The addition of
module_idfields to heater_central and thermostat devices in this fixture matches the consistent pattern seen across other Anna fixtures.Likely an incorrect or invalid review comment.
fixtures/p1v4_442_single/data.json (1)
22-22: LGTM: Module ID added to smartmeter device.The addition of
module_idto the smartmeter entry aligns with the PR's objective of enabling differential updates through module tracking.fixtures/anna_heatpump_cooling_fake_firmware/data.json (1)
40-40: LGTM: Module IDs added to heater and thermostat devices.The additions of
module_idfields to both device entries are consistent with the PR's module tracking enhancements.Also applies to: 67-67
fixtures/m_adam_cooling/data.json (1)
19-19: LGTM: Module IDs added across multiple devices.The systematic addition of
module_idfields to six device entries (OpenTherm, Emma, Tom Badkamer, Anna, Jip, Lisa) is consistent with the PR's module tracking implementation.Also applies to: 39-39, 67-67, 90-90, 153-153, 175-175
fixtures/anna_elga_2/data.json (1)
16-16: LGTM: Module IDs added to heater and thermostat devices.The additions of
module_idfields are consistent with the PR's module tracking feature implementation.Also applies to: 42-42
fixtures/anna_loria_heating_idle/data.json (1)
12-12: LGTM: Module IDs added to Anna and OpenTherm devices.The additions of
module_idfields to both device entries support the PR's differential update functionality.Also applies to: 81-81
tests/data/adam/adam_zone_per_device.json (1)
9-9: LGTM: Module IDs added across all device entries.The systematic addition of
module_idfields to all 14 device entries in this test fixture is consistent with the PR's module tracking implementation and ensures test data reflects the new schema.Also applies to: 97-97, 151-151, 173-173, 199-199, 251-251, 318-318, 344-344, 370-370, 398-398, 455-455, 481-481, 510-510, 551-551, 580-580
fixtures/anna_p1/data.json (1)
12-12: LGTM: Module IDs added to Anna, OpenTherm, and P1 devices.The additions of
module_idfields to all three device types (thermostat, heater, smartmeter) are consistent with the PR's module tracking enhancements.Also applies to: 46-46, 82-82
fixtures/m_anna_heatpump_cooling/data.json (1)
46-46: LGTM: Module IDs added to heater and thermostat devices.The additions of
module_idfields complete the fixture update pattern, ensuring consistency across all test data files for the module tracking feature.Also applies to: 72-72
fixtures/anna_v4_no_tag/data.json (1)
12-12: LGTM! Module ID metadata added for differential update support.The addition of
module_idfields to the Anna thermostat and OpenTherm heater entries is consistent with the PR's objective to enable differential updates by tracking module-level metadata.Also applies to: 76-76
fixtures/m_adam_jip/data.json (1)
66-66: LGTM! Comprehensive module_id enrichment across all device types.The addition of
module_idfields to all device entries (zone thermostats, TRVs, plugs, heater, gateway, and thermometers) provides the metadata foundation for the PR's differential update optimization.Also applies to: 90-90, 115-115, 138-138, 161-161, 185-185, 213-213, 287-287, 354-354, 379-379
tests/test_legacy_anna.py (1)
33-33: LGTM! Test expectations updated to reflect module_id additions.The entity count increase from 44 to 46 appropriately reflects the additional
module_idmetadata now included in the legacy Anna fixtures.Also applies to: 68-68
tests/test_p1.py (1)
31-31: LGTM! Test expectations updated for P1 fixtures.The entity count adjustments (33→34 and 42→43) correctly reflect the
module_idmetadata additions in the respective P1 fixture data.Also applies to: 81-81
fixtures/anna_without_boiler_fw441/data.json (1)
12-12: LGTM! Module ID added to Anna thermostat.The
module_idfield addition is consistent with the broader schema expansion to support differential device updates.fixtures/adam_plus_anna/data.json (1)
45-45: LGTM! Module IDs added across multiple device types.The
module_idenrichment for the heater, plugs, and thermostat devices supports the PR's differential update mechanism.Also applies to: 62-62, 105-105, 118-118
fixtures/adam_multiple_devices_per_zone/data.json (1)
9-9: LGTM! Extensive module_id enrichment for multi-device scenario.The comprehensive addition of
module_idfields across all devices in this complex multi-zone fixture properly supports the differential update tracking mechanism.Also applies to: 100-100, 154-154, 176-176, 202-202, 231-231, 254-254, 321-321, 347-347, 373-373, 401-401, 458-458, 484-484, 513-513, 554-554, 600-600
fixtures/anna_v4/data.json (1)
12-12: LGTM! Module ID metadata added for Anna and OpenTherm devices.The addition of
module_idfields enables module-level tracking for the differential update optimization, consistent with the PR's objectives.Also applies to: 76-76
fixtures/anna_elga_2_cooling/data.json (1)
22-22: LGTM: Module ID additions support differential update tracking.The
module_idfields added to the OpenTherm and Anna device entries are well-formed and consistent with the PR's objective of enabling differential updates rather than full redetection on every update cycle.Also applies to: 48-48
fixtures/m_adam_heating/data.json (1)
24-24: LGTM: Module IDs added across device types.The
module_idadditions to six devices (OpenTherm, Emma, Tom Badkamer, Anna, Jip, Lisa Badkamer) extend the fixture to support the new differential update model. All values are properly formatted.Also applies to: 44-44, 72-72, 95-95, 152-152, 174-174
fixtures/anna_heatpump_cooling/data.json (1)
40-40: LGTM: Module IDs added to heatpump cooling scenario.The
module_idfields for the OpenTherm heater/cooler and Anna thermostat are correctly formatted and align with the broader fixture updates across the PR.Also applies to: 67-67
fixtures/legacy_anna_2/data.json (1)
12-12: LGTM: Legacy fixture updated with module IDs.The addition of
module_idfields to the legacy Anna fixture ensures consistency with the new data model across both legacy and current fixtures.Also applies to: 55-55
fixtures/legacy_anna/data.json (1)
25-25: LGTM: Module IDs added to legacy fixture.The
module_idadditions to this legacy Anna fixture maintain consistency with the data model updates across the codebase.Also applies to: 47-47
fixtures/adam_zone_per_device/data.json (1)
9-9: LGTM: Comprehensive module ID additions across zone-per-device fixture.The
module_idfields added to 14 devices across various device classes (Plugs, Lisas, Toms, etc.) extend the fixture to fully support differential update tracking in the zone-per-device scenario.Also applies to: 97-97, 151-151, 173-173, 199-199, 228-228, 251-251, 318-318, 344-344, 370-370, 398-398, 455-455, 481-481, 510-510, 551-551, 580-580
fixtures/anna_loria_driessens/data.json (1)
36-36: LGTM: Module IDs added to Anna Loria Driessens fixture.The
module_idadditions to the Anna thermostat and OpenTherm heater/cooler entries maintain data model consistency across user-specific fixtures.Also applies to: 87-87
fixtures/adam_plus_anna_new_regulation_off/data.json (1)
18-18: LGTM: Module IDs added across regulation-off scenario.The
module_idadditions to nine devices (OpenTherm, various Plugs, thermostats, valve actuator) enable differential update tracking in the Adam plus Anna regulation-off scenario. All values are properly formatted.Also applies to: 33-33, 52-52, 77-77, 99-99, 120-120, 139-139, 158-158, 215-215
fixtures/adam_plus_anna_new/data.json (1)
18-18: LGTM! Module ID tracking added.The addition of
module_idfields across device entries provides unique module identifiers that support the differential update logic introduced in this PR. The UUIDs are unique and the pattern is consistent.Also applies to: 33-33, 51-51, 79-79, 104-104, 126-126, 147-147, 166-166, 185-185, 242-242, 264-264
fixtures/adam_heatpump_cooling/data.json (1)
61-61: LGTM! Consistent module ID additions.The
module_idfields are added consistently across all device entries, maintaining the same pattern as other fixtures in this PR.Also applies to: 88-88, 111-111, 166-166, 187-187, 208-208, 230-230, 285-285, 336-336, 358-358, 379-379, 467-467, 522-522, 611-611, 633-633, 656-656, 676-676, 692-692, 752-752, 776-776, 799-799
fixtures/adam_jip/data.json (1)
67-67: LGTM! Module ID schema extension.Consistent addition of
module_idfields to support module tracking.Also applies to: 91-91, 116-116, 139-139, 162-162, 186-186, 214-214, 288-288, 355-355, 380-380
tests/data/adam/adam_plus_anna_new.json (1)
18-18: LGTM! Test data updated with module IDs.Test data correctly mirrors the fixture structure with module_id additions.
Also applies to: 33-33, 51-51, 79-79, 104-104, 126-126, 147-147, 166-166, 185-185, 242-242, 264-264
plugwise/helper.py (12)
77-82: LGTM! Tracking state initialization.The initialization of tracking lists for existing and new appliances, locations, and zones properly supports the differential update optimization described in the PR objectives.
103-103: LGTM! Return type supports differential updates.The method now returns
boolto indicate first run (True) vs. subsequent updates with removals (False), which aligns with the PR's goal of optimizing updates.Also applies to: 174-182
114-126: LGTM! Explicit field initialization.Explicitly initializing all appliance fields, including the new
module_id, prevents missing key errors and makes the data structure clear.
130-145: LGTM! Proper orphaned device handling.The logic correctly skips orphaned thermostats without actuator functionalities or locations, preventing incomplete device processing.
158-163: LGTM! Core differential update optimization.This early-exit optimization skips entity creation/updates for appliances that haven't changed, which is exactly the optimization described in the PR objectives.
184-226: LGTM! Safe module ID checking.The method now uses
.get()with defaults to safely check for module_id existence, addressing the previous KeyError concern.
244-249: LGTM! Location differential update optimization.Consistently applies the same early-exit pattern for unchanged locations as used for appliances.
298-298: LGTM! Module ID propagation for plugs.Correctly propagates
module_idfrom module data to plug appliances.
431-439: LGTM! Proper ZigBee availability checking.The method correctly checks ZigBee device availability via module_id lookup and updates the data in-place. The return type annotation is correct.
806-837: LGTM! Refactored thermostat scanning.The updated logic properly uses the new ranking mechanism and creates zones only for locations with thermostats, while tracking removals for differential updates.
839-848: LGTM! Thermostat matching and ranking.The method correctly matches thermostats with locations and ranks them. The return type annotation is correct.
850-883: LGTM! Thermostat ranking logic.The ranking logic correctly promotes and demotes thermostats based on priority, with proper handling of primary/secondary lists. The type annotation for
location: ThermoLocis correct.tests/data/adam/adam_onoff_cooling_fake_firmware.json (1)
73-73: JSON syntax is valid and module_id addition aligns with PR goals.The
module_idfield is syntactically correct and consistent with the PR's objective of supporting differential updates. Optional verification recommended: confirm the module_id valuee2d52aece0524fde9c346953e5e1acb2matches the expected hardware module for this test scenario, and verify whether other devices in this fixture intentionally omit the module_id field.
bouwew
force-pushed
the
different-update
branch
2 times, most recently
from
7da0835 to
a020c41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugwise/helper.py (2)
234-274: Add safe default to zone removal.Line 845 uses
self._zones.pop(location_id)without a default value. Since only locations with thermostats have zone entries inself._zones, removing a location without a thermostat will raise a KeyError. Use.pop(location_id, None)instead.
190-232: Fix unsafe dictionary access at line 206: use.get()for consistency and to prevent KeyError.Line 206 uses unsafe bracket notation
self.gw_entities[self._gateway_id].get("module_id"), which will raiseKeyErrorifself._gateway_idis not yet in the dictionary. This is inconsistent with the safe pattern at line 207:self.gw_entities.get(self._home_loc_id, {}).get("module_id").Change line 206 to:
self.gw_entities.get(self._gateway_id, {}).get("module_id")Also check line 229 which uses
.pop(self._gateway_id)without safety verification.
♻️ Duplicate comments (1)
plugwise/helper.py (1)
696-696: Remove or update informal debug statement.The debug log contains "HOI" which appears to be an informal/development debug message. Replace with a professional message or remove if redundant.
🔎 Suggested fix
- LOGGER.debug("HOI outdoor_temp = %s", value) + LOGGER.debug("Gateway outdoor temperature: %s", value)Based on past review comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
fixtures/adam_heatpump_cooling/data.jsonfixtures/adam_jip/data.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/adam_onoff_cooling_fake_firmware/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/anna_elga_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_elga_no_cooling/data.jsonfixtures/anna_heatpump_cooling/data.jsonfixtures/anna_heatpump_cooling_fake_firmware/data.jsonfixtures/anna_heatpump_heating/data.jsonfixtures/anna_loria_cooling_active/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_v4/data.jsonfixtures/anna_v4_dhw/data.jsonfixtures/anna_v4_no_tag/data.jsonfixtures/anna_without_boiler_fw441/data.jsonfixtures/legacy_anna/data.jsonfixtures/legacy_anna_2/data.jsonfixtures/m_adam_cooling/data.jsonfixtures/m_adam_heating/data.jsonfixtures/m_adam_jip/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsonfixtures/m_anna_heatpump_cooling/data.jsonfixtures/m_anna_heatpump_idle/data.jsonfixtures/p1v4_442_single/data.jsonfixtures/p1v4_442_triple/data.jsonplugwise/__init__.pyplugwise/common.pyplugwise/constants.pyplugwise/data.pyplugwise/helper.pyplugwise/legacy/helper.pyplugwise/smile.pytests/data/adam/adam_heatpump_cooling.jsontests/data/adam/adam_jip.jsontests/data/adam/adam_multiple_devices_per_zone.jsontests/data/adam/adam_onoff_cooling_fake_firmware.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_plus_anna_new.jsontests/data/adam/adam_zone_per_device.jsontests/data/anna/anna_v4.jsontests/test_adam.pytests/test_anna.pytests/test_legacy_anna.pytests/test_p1.pyuserdata/updated/adam_plus_anna_new/core.domain_objects.xml
✅ Files skipped from review due to trivial changes (1)
- fixtures/anna_heatpump_cooling_fake_firmware/data.json
🚧 Files skipped from review as they are similar to previous changes (33)
- fixtures/m_anna_heatpump_idle/data.json
- tests/data/adam/adam_zone_per_device.json
- fixtures/anna_elga_2_cooling/data.json
- tests/data/adam/adam_onoff_cooling_fake_firmware.json
- fixtures/legacy_anna/data.json
- fixtures/m_adam_cooling/data.json
- tests/data/adam/adam_multiple_devices_per_zone.json
- fixtures/anna_heatpump_heating/data.json
- fixtures/anna_elga_2/data.json
- fixtures/m_adam_multiple_devices_per_zone/data.json
- plugwise/data.py
- fixtures/anna_elga_2_schedule_off/data.json
- plugwise/legacy/helper.py
- fixtures/p1v4_442_single/data.json
- fixtures/p1v4_442_triple/data.json
- fixtures/anna_without_boiler_fw441/data.json
- fixtures/adam_plus_anna_new_regulation_off/data.json
- fixtures/adam_heatpump_cooling/data.json
- tests/data/adam/adam_heatpump_cooling.json
- fixtures/anna_v4_dhw/data.json
- fixtures/adam_onoff_cooling_fake_firmware/data.json
- tests/test_legacy_anna.py
- fixtures/anna_p1/data.json
- fixtures/anna_loria_heating_idle/data.json
- fixtures/m_anna_heatpump_cooling/data.json
- tests/data/anna/anna_v4.json
- fixtures/anna_v4_no_tag/data.json
- fixtures/anna_v4/data.json
- fixtures/anna_heatpump_cooling/data.json
- plugwise/init.py
- fixtures/legacy_anna_2/data.json
- fixtures/adam_jip/data.json
- tests/data/adam/adam_plus_anna_new.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
fixtures/adam_zone_per_device/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/anna_loria_cooling_active/data.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/anna_elga_no_cooling/data.jsontests/data/adam/adam_plus_anna.jsonfixtures/m_adam_heating/data.jsontests/data/adam/adam_jip.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/anna_loria_driessens/data.jsonplugwise/helper.pyfixtures/m_adam_jip/data.json
📚 Learning: 2024-10-06T16:03:27.250Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-10-06T16:03:27.250Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
Applied to files:
fixtures/anna_loria_cooling_active/data.jsonfixtures/anna_loria_driessens/data.json
📚 Learning: 2024-12-13T11:26:00.100Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 671
File: plugwise/legacy/data.py:97-106
Timestamp: 2024-12-13T11:26:00.100Z
Learning: In `plugwise/legacy/data.py`, the legacy thermostat does not support cooling, so handling of the cooling state is unnecessary in this code.
Applied to files:
plugwise/smile.py
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-12-21T10:56:14.461Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:14.461Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/smile.py (1)
plugwise/helper.py (3)
_get_appliances(114-188)_get_appliances_with_offset_functionality(366-375)_scan_thermostats(817-848)
🔇 Additional comments (21)
fixtures/anna_elga_no_cooling/data.json (2)
44-44: LGTM - module_id added for heater_central device.The addition of the
module_idfield is correctly formatted and aligns with the PR objective to track device modules for differential updates.
70-70: The module_id addition is correct.The
module_idfield is consistently present on peripheral devices (thermostat, heater_central, smartmeter) across all fixtures but intentionally absent from gateway devices. The thermostat'smodule_idat line 70 follows the established pattern.fixtures/adam_plus_anna/data.json (1)
45-45: LGTM! Consistent module_id enrichment for physical devices.The addition of
module_idfields to physical device entries (OpenTherm heater, Plugs, and ThermoTouch) is consistent and correctly excludes gateway and climate zone entries. This data enrichment enables the differential update optimization objective of the PR.Also applies to: 62-62, 105-105, 118-118
fixtures/anna_loria_driessens/data.json (1)
36-36: LGTM! Module tracking added to physical devices.The
module_idadditions to the thermostat and heater entries follow the same consistent pattern observed across other fixtures.Also applies to: 87-87
fixtures/m_adam_heating/data.json (1)
24-24: LGTM! Module tracking consistently applied.The
module_idfields are appropriately added to physical devices (OpenTherm, thermostats, valves, thermometers) while correctly excluding gateways, climate zones, and group entities.Also applies to: 44-44, 72-72, 95-95, 152-152, 174-174
fixtures/anna_loria_cooling_active/data.json (1)
12-12: LGTM! Consistent module_id enrichment.The
module_idadditions maintain the established pattern of enriching physical device entries only.Also applies to: 81-81
tests/data/adam/adam_plus_anna.json (1)
45-45: LGTM! Test data mirrors fixture data correctly.The test data
module_idadditions are consistent with the corresponding fixture file (fixtures/adam_plus_anna/data.json), using identical module_id values for the same devices.Also applies to: 62-62, 105-105, 118-118
tests/data/adam/adam_jip.json (1)
67-67: LGTM! Comprehensive module_id coverage in test data.The test data correctly enriches all physical device entries with
module_idfields while appropriately excluding gateways and climate zones, providing comprehensive coverage for testing the differential update functionality.Also applies to: 91-91, 116-116, 139-139, 162-162, 186-186, 214-214, 288-288, 355-355, 380-380
plugwise/constants.py (3)
295-295: LGTM! ApplianceType literal extended for module tracking.The addition of "module_id" to the ApplianceType literal ensures type safety when accessing the new module_id field on appliance data structures.
427-427: LGTM! ModuleData extended with optional module_id.The
module_id: str | Nonefield addition to ModuleData properly supports the new module tracking capability while maintaining backward compatibility through optional typing.
550-550: LGTM! GwEntityData extended with optional module_id.The
module_id: str | Nonefield addition to GwEntityData aligns with the data enrichment across fixtures and enables module-level tracking for gateway entities.tests/test_p1.py (1)
31-31: LGTM! Test expectations correctly reflect module_id enrichment in P1 fixtures.The entity count increases (33→34, 42→43) are confirmed: both p1v4_442_single and p1v4_442_triple fixtures now include module_id fields in their P1 smartmeter entities, and the _create_gw_entities method counts each entity attribute (including module_id) as a separate item.
plugwise/smile.py (1)
91-92: LGTM! New attributes support lifecycle management.The addition of
_zonesandgw_entitiesas instance attributes enables the differential update strategy by caching zone and entity state across update cycles.plugwise/common.py (3)
196-241: LGTM! Group lifecycle management implemented correctly.The refactored
_get_groups()method properly implements differential updates:
- Tracks new vs. existing groups
- Skips unchanged groups (line 213-217) by comparing both ID and name
- Removes obsolete groups (line 234-237)
- Validates members exist before adding them via
_add_member(line 222)The lifecycle management pattern is consistent with the PR's differential update strategy.
242-245: LGTM! Member validation prevents orphaned references.The
_add_memberhelper correctly validates that member entities exist ingw_entitiesbefore adding them to the members list, preventing orphaned references in group definitions.
121-121: LGTM! Module ID propagation implemented consistently.The
module_idfield is correctly propagated frommodule_datato appliance objects in:
_appl_heater_central_info(line 121)_appl_thermostat_info(line 146)_create_gw_entities(line 163)This ensures module_id flows through the entire data pipeline to
gw_entities.Also applies to: 146-146, 163-163
plugwise/helper.py (4)
114-188: LGTM! Appliance lifecycle management with proper return signaling.The refactored
_get_appliances()method correctly:
- Returns
boolto signal whether removals were detected (line 114)- Tracks new vs. existing appliances (lines 164-169)
- Skips re-processing unchanged appliances by comparing name
- Removes obsolete appliances (lines 180-184)
- Returns
Falsewhen removals detected,TrueotherwiseThis enables the caller (smile.py) to conditionally update dependent data structures.
444-450: LGTM! ZigBee availability check properly implemented.The
_get_zigbee_availability()method correctly:
- Checks for module_id presence before querying
- Locates the ZigBee node in domain_objects
- Updates entity availability based on reachable status
Past review flagged the return type (now correctly
-> None).
817-848: LGTM! Zone lifecycle management integrated with thermostat scanning.The refactored
_scan_thermostats()method properly:
- Guards execution to Adam devices only (line 823)
- Calls thermostat matching/ranking (line 826)
- Creates zones for locations with primary thermostats (lines 828-840)
- Tracks and removes obsolete zones (lines 842-848)
The lifecycle pattern is consistent with groups and appliances.
850-894: LGTM! Thermostat ranking logic properly implemented.The
_match_and_rank_thermostats()and_rank_thermostat()methods correctly:
- Match thermostats to locations by location_id
- Rank thermostats by priority (lines 882-893)
- Handle primary/secondary designation with demotion logic
- Update location data structures in-place
Past review comments confirmed the type annotations are now correct.
fixtures/m_adam_jip/data.json (1)
66-66: LGTM!The
module_idfields are correctly added to all physical devices (zone thermostats, thermostatic radiator valves, plugs, zone thermometers, and the heater central). The pattern is consistent:
- Physical devices with
zigbee_mac_addressreceivemodule_id- Virtual ThermoZone entries (dev_class "climate") appropriately don't have
module_id- The gateway device doesn't have a
module_id, which is correct since it's the coordinatorAll UUIDs are unique 32-character hex strings. This fixture update aligns well with the PR objective of supporting differential updates via module identification.
Also applies to: 90-90, 115-115, 138-138, 161-161, 185-185, 213-213, 287-287, 354-354, 379-379
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugwise/helper.py (2)
165-169: Consider defensive check before accessing gw_entities.Line 167 accesses
self.gw_entities[appl.entity_id]["name"]assuming that ifappl.entity_idis in_existing_appliances, it must also be ingw_entities. While this assumption may hold under normal operation, adding a defensive check would make the code more robust against potential inconsistencies.🔎 Suggested defensive pattern
if ( appl.entity_id in self._existing_appliances + and appl.entity_id in self.gw_entities and self.gw_entities[appl.entity_id]["name"] == appl.name ): continue
114-119: Consider documenting the return value.The method signature changed to return
bool, but the docstring doesn't explain what the boolean indicates. Based on the implementation, it appears to returnTrueon first run andFalseon subsequent runs, which could be useful context for callers.🔎 Suggested docstring addition
def _get_appliances(self) -> bool: """Collect all appliances with relevant info. Also, collect the P1 smartmeter info from a location as this one is not available as an appliance. + + Returns: + bool: True if this is the first run (initial discovery), False otherwise. """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-12-21T10:56:14.461Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:14.461Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (1)
_get_appliances(86-140)plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)plugwise/constants.py (1)
GwEntityData(535-598)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (2)
plugwise/helper.py (2)
313-313: Good addition: module_id propagation for plugs.This correctly propagates the
module_idfor plug devices, which aligns with the PR's objective to improve tracking and enable incremental updates.
268-274: Fix incorrect variable name in condition.Line 269 checks
self._existing_applianceswhen it should checkself._existing_locations. This appears to be a copy-paste error from the similar pattern in_get_appliances().While this bug may not manifest in typical operation (because
_get_locations()is called from within_get_appliances()after appliances are populated), it's incorrect and could cause issues in edge cases or if the call order changes.🔎 Proposed fix
removed = list(set(self._existing_locations) - set(self._new_locations)) -if self._existing_appliances and removed: +if self._existing_locations and removed: for location_id in removed: self._loc_data.pop(location_id)⛔ Skipped due to learnings
Learnt from: bouwew Repo: plugwise/python-plugwise PR: 678 File: plugwise/legacy/helper.py:69-69 Timestamp: 2024-12-22T09:37:24.648Z Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugwise/helper.py (3)
114-192: Well-structured lifecycle tracking with proper state management.The method correctly:
- Skips recreation when entities are unchanged (lines 165-169)
- Removes stale entities (lines 180-184)
- Updates tracking state before returning (lines 186-187)
- Signals new entities via boolean return (lines 189-192)
Optional: Add defensive check at line 167
For extra safety, consider using
.get()to prevent KeyError if tracking state becomes inconsistent:if ( appl.entity_id in self._existing_appliances - and self.gw_entities[appl.entity_id]["name"] == appl.name + and self.gw_entities.get(appl.entity_id, {}).get("name") == appl.name ): continueThis guards against edge cases where
_existing_appliancesandgw_entitiesmight become out of sync.
240-280: Consistent lifecycle tracking pattern for locations.The location tracking follows the same pattern as appliances: skip unchanged, remove stale, update state.
Optional: Add defensive check at line 263 for consistency
Similar to the appliances check, consider using
.get()for extra safety:if ( loc.loc_id in self._existing_locations - and self._loc_data[loc.loc_id]["name"] == loc.name + and self._loc_data.get(loc.loc_id, {}).get("name") == loc.name ): continueThis maintains consistency with the defensive coding pattern used in
_add_p1_smartmeter_info(lines 211-213).
822-860: Lifecycle tracking extended to zones with consistent pattern.The zone tracking follows the established pattern and correctly manages zone creation/removal.
Optional: Add defensive check at line 837 for consistency
For consistency with the broader defensive coding pattern:
if ( location_id in self._existing_zones - and self.gw_entities[location_id]["name"] == location["name"] + and self.gw_entities.get(location_id, {}).get("name") == location["name"] ): continueThis ensures robustness if zones in
_zoneshaven't been merged intogw_entitiesyet, or if the merge timing varies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-12-21T10:56:14.461Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:14.461Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (1)
_get_appliances(86-140)plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)plugwise/constants.py (1)
GwEntityData(535-598)
🔇 Additional comments (4)
plugwise/helper.py (4)
88-93: LGTM! Clean lifecycle tracking initialization.The tracking lists follow a clear pattern for detecting changes across update cycles.
194-238: LGTM! Proper duplicate detection with safe key access.The module_id-based change detection (lines 209-215) correctly uses
.get()with defaults to handle missing entities on first run or during entity transitions.
319-319: LGTM! Module ID propagation for plug devices.Correctly assigns module_id to enable change tracking for plug-type appliances.
450-456: LGTM! Clean ZigBee availability check.The method correctly:
- Guards with module_id existence check
- Locates the ZigBee node by module_id
- Updates entity availability in-place
The in-place modification pattern is consistent with other helper methods in this class.
bouwew
force-pushed
the
different-update
branch
from
94ac21b to
a224806
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise/common.py (1)
224-232: Consider guarding against emptymemberslist leading to orphaned group entries.If all appliances in a group are removed (i.e., none pass the
_add_membercheck),memberswill be empty and the group won't be added togw_entities. However, if it was previously in_existing_groups, the removal logic at lines 234-237 won't trigger because the group_id is still in_new_groups. This could leave stale state in_existing_groups.Actually, re-reading: Line 224
if group_type in GROUP_TYPES and members:guards against adding empty-member groups, and the group_id is still appended to_new_groupsat line 212. If the group becomes empty, it won't be (re)added togw_entities, but if it was there before from a prior run with valid members, it won't be removed.🔎 Suggested fix to handle groups that become empty
if group_type in GROUP_TYPES and members: self.gw_entities[group_id] = { "dev_class": group_type, "model": "Group", "name": group_name, "members": members, "vendor": "Plugwise", } self._count += 5 + elif group_id in self._existing_groups: + # Group existed but now has no valid members - treat as removed + self._new_groups.remove(group_id)
♻️ Duplicate comments (2)
plugwise/smile.py (1)
112-117: Staletherms_with_offset_funcafter appliance removal.When
_get_appliances()returnsFalse(no new appliances detected), the offset functionality list is not refreshed. If a thermostat with offset capability is removed, it remains intherms_with_offset_funcwhile the entity is deleted fromgw_entities, causing inconsistent state whereset_offset()validation could incorrectly pass for a non-existent device.Consider always refreshing the offset list when running for a thermostat system, or clearing it when appliances are removed.
🔎 Suggested fix
- if self._get_appliances() and self._is_thermostat: + appliances_changed = self._get_appliances() + if self._is_thermostat: self.therms_with_offset_func = ( self._get_appliances_with_offset_functionality() )Or alternatively, ensure that
therms_with_offset_funcis cleared/resynced in the removal path within_get_appliances().plugwise/helper.py (1)
855-858: Use safepop()to avoid KeyError on zone removal.Line 858 uses
self._zones.pop(location_id)without a default value. If a location_id is in_existing_zonesbut not in_zones(edge case, possibly from prior error or external modification), this will raiseKeyError.🔎 Suggested fix
if self._existing_zones and removed: for location_id in removed: - self._zones.pop(location_id) + self._zones.pop(location_id, None)
🧹 Nitpick comments (2)
plugwise/helper.py (2)
160-169: Potential issue with skipping gateway/heater_central updates unconditionally.Lines 161-165 skip reprocessing for
gatewayandheater_centraldevices if they already exist, regardless of any attribute changes. While these device types may be stable, this could miss firmware updates or other metadata changes that should be reflected ingw_entities.Consider whether these device classes should also check for name or other key attribute changes before skipping, similar to other device types:
self._new_appliances.append(appl.entity_id) if appl.entity_id in self._existing_appliances and ( - appl.pwclass in ("gateway", "heater_central") - or self.gw_entities[appl.entity_id]["name"] == appl.name + self.gw_entities[appl.entity_id]["name"] == appl.name ): continueAlternatively, if the intent is that gateway/heater_central metadata never changes mid-session, document this assumption.
836-842: Verify zone existence before accessinggw_entitiesin skip condition.Similar to the group handling in
common.py, line 839 accessesself.gw_entities[location_id]["name"]assuming the entity exists. Whilelocation_id in self._existing_zonesshould imply existence, consider defensive access.🔎 Optional safer access pattern
if ( location_id in self._existing_zones - and self.gw_entities[location_id]["name"] == location["name"] + and self._zones.get(location_id, {}).get("name") == location["name"] ): continueNote: The check should compare against
_zonesrather thangw_entitiessince zones are stored in_zones, not directly ingw_entities.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
fixtures/adam_heatpump_cooling/data.jsonfixtures/adam_jip/data.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/adam_onoff_cooling_fake_firmware/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/anna_elga_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_elga_no_cooling/data.jsonfixtures/anna_heatpump_cooling/data.jsonfixtures/anna_heatpump_cooling_fake_firmware/data.jsonfixtures/anna_heatpump_heating/data.jsonfixtures/anna_loria_cooling_active/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_v4/data.jsonfixtures/anna_v4_dhw/data.jsonfixtures/anna_v4_no_tag/data.jsonfixtures/anna_without_boiler_fw441/data.jsonfixtures/legacy_anna/data.jsonfixtures/legacy_anna_2/data.jsonfixtures/m_adam_cooling/data.jsonfixtures/m_adam_heating/data.jsonfixtures/m_adam_jip/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsonfixtures/m_anna_heatpump_cooling/data.jsonfixtures/m_anna_heatpump_idle/data.jsonfixtures/p1v4_442_single/data.jsonfixtures/p1v4_442_triple/data.jsonplugwise/common.pyplugwise/constants.pyplugwise/data.pyplugwise/helper.pyplugwise/legacy/helper.pyplugwise/smile.pypyproject.tomltests/data/adam/adam_heatpump_cooling.jsontests/data/adam/adam_jip.jsontests/data/adam/adam_multiple_devices_per_zone.jsontests/data/adam/adam_onoff_cooling_fake_firmware.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_plus_anna_new.jsontests/data/adam/adam_zone_per_device.jsontests/data/anna/anna_v4.jsontests/test_adam.pytests/test_anna.pytests/test_legacy_anna.pytests/test_p1.py
🚧 Files skipped from review as they are similar to previous changes (33)
- fixtures/anna_without_boiler_fw441/data.json
- fixtures/legacy_anna/data.json
- fixtures/anna_v4_dhw/data.json
- fixtures/anna_elga_2/data.json
- fixtures/anna_heatpump_cooling/data.json
- fixtures/m_adam_cooling/data.json
- fixtures/adam_plus_anna/data.json
- fixtures/adam_multiple_devices_per_zone/data.json
- pyproject.toml
- fixtures/anna_elga_no_cooling/data.json
- fixtures/anna_elga_2_schedule_off/data.json
- fixtures/m_adam_heating/data.json
- fixtures/m_anna_heatpump_cooling/data.json
- tests/test_anna.py
- tests/data/adam/adam_plus_anna.json
- fixtures/legacy_anna_2/data.json
- fixtures/adam_onoff_cooling_fake_firmware/data.json
- fixtures/anna_loria_cooling_active/data.json
- fixtures/m_anna_heatpump_idle/data.json
- tests/data/adam/adam_plus_anna_new.json
- fixtures/anna_heatpump_cooling_fake_firmware/data.json
- fixtures/anna_loria_driessens/data.json
- fixtures/adam_jip/data.json
- tests/test_adam.py
- tests/data/adam/adam_zone_per_device.json
- tests/data/adam/adam_onoff_cooling_fake_firmware.json
- fixtures/adam_plus_anna_new/data.json
- tests/data/adam/adam_jip.json
- plugwise/data.py
- fixtures/adam_heatpump_cooling/data.json
- fixtures/adam_plus_anna_new_regulation_off/data.json
- plugwise/constants.py
- fixtures/p1v4_442_single/data.json
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
fixtures/anna_elga_2_cooling/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsonfixtures/adam_zone_per_device/data.jsontests/data/adam/adam_heatpump_cooling.jsonplugwise/helper.pyfixtures/anna_heatpump_heating/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_loria_heating_idle/data.jsontests/data/adam/adam_multiple_devices_per_zone.jsonfixtures/m_adam_jip/data.json
📚 Learning: 2024-10-06T16:03:27.250Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-10-06T16:03:27.250Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
Applied to files:
fixtures/anna_elga_2_cooling/data.jsontests/data/adam/adam_heatpump_cooling.jsonfixtures/anna_loria_heating_idle/data.json
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/legacy/helper.pyplugwise/helper.py
📚 Learning: 2025-12-21T10:56:14.461Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:14.461Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
📚 Learning: 2024-12-13T11:26:00.100Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 671
File: plugwise/legacy/data.py:97-106
Timestamp: 2024-12-13T11:26:00.100Z
Learning: In `plugwise/legacy/data.py`, the legacy thermostat does not support cooling, so handling of the cooling state is unnecessary in this code.
Applied to files:
plugwise/smile.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (2)
_get_appliances(86-140)_appliance_info_finder(191-204)plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)plugwise/constants.py (1)
GwEntityData(535-598)
🔇 Additional comments (33)
fixtures/anna_heatpump_heating/data.json (2)
46-46: LGTM! Module ID added for OpenTherm device.The
module_idfield is properly formatted and aligns with the PR's objective of introducing module_id tracking for optimized updates.
72-72: LGTM! Module ID added for Anna thermostat.The
module_idfield is properly formatted and consistent with the broader pattern of adding module_id tracking across the codebase.fixtures/anna_elga_2_cooling/data.json (3)
22-22: LGTM! Module ID added for OpenTherm device.The module_id field addition is correctly formatted and placed, supporting the PR's optimization objective to track device changes.
48-48: LGTM! Module ID added for Anna thermostat device.The module_id field addition is correctly formatted and consistently placed, matching the structure used for the OpenTherm device.
75-92: Verify whether gateway device should have module_id.The gateway device (dev_class: "gateway") did not receive a module_id field, while the heater_central and thermostat devices did. Please confirm this is intentional—gateway devices may not require module_id tracking if they represent the hub itself rather than connected modules.
tests/data/adam/adam_multiple_devices_per_zone.json (1)
9-9: LGTM! module_id additions are consistent and well-structured.The additions properly augment physical device entries (Plugs, Lisa thermostats, Tom/Floor valves) with module_id tracking, while correctly excluding virtual entities (ThermoZones, Groups, Gateway). This directly supports the PR's optimization objective of tracking module changes rather than redetecting all entities on every update.
Also applies to: 100-100, 147-147, 154-154, 169-169, 176-176, 195-195, 202-202, 224-224, 231-231, 247-247, 254-254, 314-314, 321-321, 340-340, 347-347, 366-366, 373-373, 394-394, 401-401, 451-451, 458-458, 477-477, 484-484, 506-506, 513-513, 547-547, 554-554, 593-593, 600-600
fixtures/m_adam_multiple_devices_per_zone/data.json (1)
9-9: LGTM! Fixture data properly synchronized with test expectations.The module_id additions mirror those in the corresponding test data file, maintaining consistency between fixtures and tests. The selective application to physical devices only is correct.
Also applies to: 100-100, 147-147, 169-169, 195-195, 224-224, 247-247, 314-314, 340-340, 366-366, 394-394, 451-451, 477-477, 506-506, 547-547, 593-593
fixtures/m_adam_jip/data.json (1)
66-66: LGTM! Consistent module_id additions across physical devices.The
module_idfields have been systematically added to all physical device entries (thermostats, radiator valves, plug, heater, thermometer) with properly formatted UUID values. The exclusion ofmodule_idfrom the gateway and climate zones (logical constructs) appears intentional and aligns with the PR objective of tracking module-level changes for optimization.Also applies to: 90-90, 115-115, 138-138, 161-161, 185-185, 213-213, 287-287, 354-354, 379-379
tests/data/anna/anna_v4.json (1)
12-12: LGTM! Consistent module_id enrichment.The addition of
module_idfields to device entries aligns with the PR's objective of tracking entity identifiers for optimized update detection. The format is consistent with the broader data model enhancements across the codebase.Also applies to: 76-76
fixtures/anna_loria_heating_idle/data.json (1)
12-12: LGTM! Consistent module_id additions.The
module_idfields are properly added to both the thermostat and heater_central devices, maintaining consistency with the PR's data model enhancements.Also applies to: 81-81
fixtures/adam_zone_per_device/data.json (2)
9-9: LGTM! Comprehensive module_id enrichment across device types.The
module_idfield has been consistently added across plugs, thermostatic radiator valves, zone thermostats, and other ZigBee devices, supporting the PR's goal of improved entity tracking.Also applies to: 97-97, 151-151, 173-173, 199-199, 228-228, 251-251, 318-318, 344-344, 370-370, 398-398, 455-455, 481-481, 510-510, 551-551, 580-580
297-309: Verify intentional omission of module_id for OnOff device.The
heater_centraldevice "OnOff" at lines 297-309 does not include amodule_idfield, while other devices do. Based on retrieved learnings, this device has minimal information due to a known bug.Please confirm whether the absence of
module_idfor this device is intentional, possibly due to the known bug affecting heater_central devices with minimal information.tests/test_legacy_anna.py (1)
33-33: LGTM! Test expectations updated for module_id enrichment.The
entity_itemsassertions have been correctly updated from 44 to 46, reflecting the addition ofmodule_idfields in the corresponding legacy Anna fixtures.Also applies to: 68-68
tests/data/adam/adam_heatpump_cooling.json (1)
61-61: LGTM! Comprehensive module_id coverage for heatpump cooling scenario.The
module_idfield has been systematically added across all ZigBee and OpenTherm devices, including Lisa thermostats, plugs, and the ThermoTouch, ensuring complete entity tracking for this fixture.Also applies to: 88-88, 111-111, 166-166, 187-187, 208-208, 230-230, 285-285, 336-336, 358-358, 379-379, 467-467, 522-522, 633-633, 656-656, 676-676, 692-692, 752-752, 776-776, 799-799
tests/test_p1.py (1)
31-31: LGTM! Test expectations updated for P1 module_id enrichment.The
entity_itemsassertions have been correctly incremented by 1 in both P1 tests, reflecting the addition ofmodule_idto the smartmeter devices in the corresponding fixtures.Also applies to: 81-81
fixtures/anna_p1/data.json (1)
12-12: LGTM! Complete module_id coverage for Anna P1 fixture.The
module_idfield has been added to all three device types in this fixture (ThermoTouch, OpenTherm heater, and P1 smartmeter), ensuring comprehensive entity tracking for the Anna P1 scenario.Also applies to: 46-46, 82-82
fixtures/p1v4_442_triple/data.json (1)
26-26: LGTM! Module_id added to P1 smartmeter.The
module_idfield has been correctly added to the smartmeter device, aligning with the test expectation update in test_connect_p1v4_442_triple. The gateway device appropriately does not receive a module_id, consistent with the pattern across other fixtures.fixtures/anna_v4/data.json (1)
12-12: LGTM!The
module_idfields are correctly added to the thermostat (Anna) and heater_central (OpenTherm) device entries, aligning with the broader module_id propagation changes in this PR.Also applies to: 76-76
plugwise/legacy/helper.py (1)
111-111: LGTM!Initializing
module_id = Nonefor legacy appliances ensures compatibility with_create_gw_entities()which now expects themodule_idattribute. Legacy devices correctly default toNonesince they don't support module tracking.Also applies to: 243-243
fixtures/anna_v4_no_tag/data.json (1)
12-12: LGTM!Consistent
module_idadditions matching theanna_v4fixture, ensuring test data alignment across variants.Also applies to: 76-76
plugwise/smile.py (1)
91-93: LGTM!Initializing
_zonesandgw_entitiesas empty dicts in__init__is appropriate for the delta-tracking update model, where state is preserved across updates rather than reset each time.plugwise/common.py (5)
57-59: LGTM!Initialization of group tracking lists enables the delta-update optimization for group lifecycle management.
121-121: LGTM!Module ID is correctly propagated through the appliance data paths:
- From
module_datatoappl.module_idfor heater_central and thermostat entities- From
appl.module_idtogw_entitiesduring entity creationThis enables consistent module tracking across all entity types.
Also applies to: 146-146, 163-163
242-245: LGTM!The
_add_memberhelper correctly validates member existence ingw_entitiesbefore adding to the members list, preventing orphaned member references.
277-286: LGTM!Module data initialization and population is correctly implemented:
module_idinitialized toNonein the default dict- Populated from
module.attrib["id"]when a module is foundAlso applies to: 305-305
209-217: The condition at line 215 is adequately guarded and the concern is not substantiated.The check
group_id in self._existing_groupsat line 214 ensures the group existed in the previous cycle. At this point in the execution (during the loop),gw_entitieshas not yet been modified—the.pop()calls that remove groups happen later at line 237, after the condition check. No.clear()calls are found ongw_entitiesin the codebase, and the initialization/update flow shows_existing_groupsandgw_entitiesare kept in sync at line 239. The access is safe within this execution context.plugwise/helper.py (7)
88-93: LGTM!Proper initialization of lifecycle tracking lists for appliances, locations, and zones enables the delta-update optimization described in the PR objectives.
181-193: Logic for tracking new vs. changed appliances is correct.The return value semantics:
- Returns
Truewhen there are genuinely new appliances (not previously in_existing_appliances)- Returns
Falsewhen all appliances already existed (even if some were removed)This aligns with the usage in
smile.pyto determine whether to refresh offset functionality. However, as noted in the smile.py review, this doesn't handle the case where appliances are removed but none are added.
195-239: Well-structured P1 smartmeter change detection.The
_add_p1_smartmeter_infomethod correctly:
- Extracts module data from the home location
- Detects smartmeter changes via
module_idcomparison- Uses safe
.get()access patterns to avoid KeyError- Handles both Anna P1 and standalone P1 gateway entity ID swapping
261-281: LGTM!Location lifecycle management correctly:
- Tracks new locations
- Skips unchanged locations (same name)
- Removes stale locations from
_loc_data- Updates tracking state appropriately
320-320: LGTM!Module ID correctly propagated for plug-type devices, consistent with thermostat and heater_central handling.
452-458: LGTM!The
_get_zigbee_availabilitymethod correctly:
- Guards with
"module_id" in entitycheck- Looks up the ZigBee node by module ID
- Sets availability based on the
reachableelementThe implicit
Nonereturn (no explicit return statement) is acceptable since this method modifiesentityin-place.
863-906: LGTM!The thermostat matching and ranking logic is well-structured:
- Iterates all locations and entities to find thermostats
- Ranks by
thermo_matchingpriority- Correctly handles promotion/demotion of primary/secondary thermostats
- Uses location's
ThermoLocfields directly (proper typing after past review fixes)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise/common.py (1)
224-235: Fix lifecycle logic for groups without valid members.When a new group (not in
_existing_groups) has an invalid type or no members, it is appended to_new_groupsat line 212 but never added togw_entities. The elif at line 233 only removes from_new_groupsif the group was in_existing_groups. This causes a desync: the group_id persists in_existing_groups(via line 242) but is absent fromgw_entities, leading to aKeyErrorat line 215 on the next update cycle.🔎 Proposed fix to handle new groups without valid members
if group_type in GROUP_TYPES and members: self.gw_entities[group_id] = { "dev_class": group_type, "model": "Group", "name": group_name, "members": members, "vendor": "Plugwise", } self._count += 5 elif group_id in self._existing_groups: # Group existed but now has no valid members -> remove self._new_groups.remove(group_id) + else: + # New group without valid type/members -> don't track + self._new_groups.remove(group_id)
🧹 Nitpick comments (3)
plugwise/common.py (3)
308-308: Consider defensive attribute access.Direct use of
module.attrib["id"]will raiseKeyErrorif the attribute is missing. While the XPath query at line 297 targets elements with anid, using.get("id")would be more defensive against malformed XML.🔎 Proposed defensive access pattern
- module_data["module_id"] = module.attrib["id"] + module_data["module_id"] = module.attrib.get("id")
237-243: Consider defensive removal to handle potential desync.Line 240 uses
self.gw_entities.pop(group_id)which will raiseKeyErrorifgroup_idis not present. While the lifecycle logic should ensure removed groups exist ingw_entities, using.pop(group_id, None)would gracefully handle any desynchronization.🔎 Proposed defensive removal
removed = list(set(self._existing_groups) - set(self._new_groups)) if self._existing_groups and removed: for group_id in removed: - self.gw_entities.pop(group_id) + self.gw_entities.pop(group_id, None)
245-248: Consider defensive attribute access.Line 247 uses
element.attrib["id"]which raisesKeyErrorif the element lacks anidattribute. Using.get("id")would be safer and naturally short-circuit the membership check whenidis absent.🔎 Proposed defensive access pattern
def _add_member(self, element: etree.Element, members: list[str]) -> None: """Check and add member to list.""" - if (member_id := element.attrib["id"]) in self.gw_entities: + if (member_id := element.attrib.get("id")) is not None and member_id in self.gw_entities: members.append(member_id)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
fixtures/adam_heatpump_cooling/data.jsonfixtures/adam_jip/data.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/adam_onoff_cooling_fake_firmware/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/anna_elga_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_elga_no_cooling/data.jsonfixtures/anna_heatpump_cooling/data.jsonfixtures/anna_heatpump_cooling_fake_firmware/data.jsonfixtures/anna_heatpump_heating/data.jsonfixtures/anna_loria_cooling_active/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_v4/data.jsonfixtures/anna_v4_dhw/data.jsonfixtures/anna_v4_no_tag/data.jsonfixtures/anna_without_boiler_fw441/data.jsonfixtures/legacy_anna/data.jsonfixtures/legacy_anna_2/data.jsonfixtures/m_adam_cooling/data.jsonfixtures/m_adam_heating/data.jsonfixtures/m_adam_jip/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsonfixtures/m_anna_heatpump_cooling/data.jsonfixtures/m_anna_heatpump_idle/data.jsonfixtures/p1v4_442_single/data.jsonfixtures/p1v4_442_triple/data.jsonplugwise/__init__.pyplugwise/common.pyplugwise/constants.pyplugwise/data.pyplugwise/helper.pyplugwise/legacy/helper.pyplugwise/smile.pypyproject.tomltests/data/adam/adam_heatpump_cooling.jsontests/data/adam/adam_jip.jsontests/data/adam/adam_multiple_devices_per_zone.jsontests/data/adam/adam_onoff_cooling_fake_firmware.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_plus_anna_new.jsontests/data/adam/adam_zone_per_device.jsontests/data/anna/anna_v4.jsontests/test_adam.pytests/test_anna.pytests/test_legacy_anna.pytests/test_p1.py
🚧 Files skipped from review as they are similar to previous changes (34)
- fixtures/anna_elga_2_cooling/data.json
- fixtures/m_adam_heating/data.json
- fixtures/m_adam_multiple_devices_per_zone/data.json
- fixtures/adam_onoff_cooling_fake_firmware/data.json
- fixtures/adam_multiple_devices_per_zone/data.json
- fixtures/anna_elga_no_cooling/data.json
- fixtures/anna_v4_no_tag/data.json
- fixtures/anna_without_boiler_fw441/data.json
- fixtures/anna_heatpump_cooling_fake_firmware/data.json
- fixtures/anna_p1/data.json
- plugwise/constants.py
- fixtures/anna_heatpump_cooling/data.json
- pyproject.toml
- fixtures/adam_plus_anna/data.json
- fixtures/legacy_anna/data.json
- tests/test_adam.py
- fixtures/p1v4_442_triple/data.json
- fixtures/adam_heatpump_cooling/data.json
- fixtures/anna_v4_dhw/data.json
- tests/data/adam/adam_jip.json
- plugwise/legacy/helper.py
- fixtures/m_anna_heatpump_idle/data.json
- fixtures/adam_plus_anna_new_regulation_off/data.json
- fixtures/anna_heatpump_heating/data.json
- fixtures/anna_loria_cooling_active/data.json
- fixtures/adam_plus_anna_new/data.json
- fixtures/anna_loria_heating_idle/data.json
- fixtures/p1v4_442_single/data.json
- fixtures/m_adam_cooling/data.json
- tests/data/adam/adam_onoff_cooling_fake_firmware.json
- fixtures/legacy_anna_2/data.json
- tests/data/anna/anna_v4.json
- fixtures/anna_v4/data.json
- tests/data/adam/adam_plus_anna_new.json
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
fixtures/anna_loria_driessens/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/m_anna_heatpump_cooling/data.jsonplugwise/helper.pytests/data/adam/adam_zone_per_device.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_heatpump_cooling.jsontests/data/adam/adam_multiple_devices_per_zone.jsonfixtures/anna_elga_2/data.jsonfixtures/m_adam_jip/data.jsonfixtures/adam_jip/data.json
📚 Learning: 2024-10-06T16:03:27.250Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 630
File: tests/data/adam/adam_g_o.json:0-0
Timestamp: 2024-10-06T16:03:27.250Z
Learning: The minimal information for the `heater_central` device is due to a known bug still present in the code.
Applied to files:
fixtures/anna_loria_driessens/data.jsonfixtures/anna_elga_2_schedule_off/data.jsontests/data/adam/adam_heatpump_cooling.jsonfixtures/anna_elga_2/data.json
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.pyplugwise/smile.py
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/helper.pyplugwise/data.pyplugwise/__init__.pyplugwise/common.pyplugwise/smile.py
📚 Learning: 2025-12-21T10:56:19.146Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:19.146Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
📚 Learning: 2024-12-13T11:26:00.100Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 671
File: plugwise/legacy/data.py:97-106
Timestamp: 2024-12-13T11:26:00.100Z
Learning: In `plugwise/legacy/data.py`, the legacy thermostat does not support cooling, so handling of the cooling state is unnecessary in this code.
Applied to files:
plugwise/smile.py
🧬 Code graph analysis (3)
plugwise/helper.py (2)
plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)plugwise/constants.py (1)
GwEntityData(537-600)
plugwise/data.py (1)
plugwise/helper.py (1)
_update_zigbee_availability(452-458)
plugwise/__init__.py (1)
plugwise/exceptions.py (1)
PlugwiseError(24-25)
🪛 Gitleaks (8.30.0)
tests/test_p1.py
[high] 31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (18)
tests/test_p1.py (2)
31-31: Test expectation correctly updated to reflect module_id additions.The entity count increase from 33 to 34 aligns with the PR's addition of module_id fields to device fixtures.
Note on static analysis: The Gitleaks warning about line 31 is a false positive. The gateway_id value on line 30 (
a455b61e52394b2db5081ce025a430f3) is a test fixture identifier, not a real API key or secret.
81-81: Test expectation correctly updated.The entity count increase from 42 to 43 is consistent with the change in the single-phase test and correctly reflects the module_id field additions.
fixtures/adam_jip/data.json (1)
67-67: Consistent addition of module_id fields to physical devices.The module_id fields are correctly added to all physical device entries (10 devices: 3 Lisa thermostats, 4 Tom valves, 1 Aqara plug, 1 heater, 1 Jip thermometer) while appropriately excluding logical entities (climate zones and gateway). All 10 module_id values are unique and follow a consistent UUID format. The values are correctly mapped to their corresponding module entities in the system, extracted from XML attributes in
common.pyand used to locate individual modules for targeted updates.fixtures/m_adam_jip/data.json (1)
66-66: LGTM!The module_id field additions to device entries are well-formed and consistent with the broader schema extension for module tracking across the PR.
Also applies to: 90-90, 115-115, 138-138, 161-161, 185-185, 213-213, 251-251, 287-287, 354-354, 379-379
tests/test_legacy_anna.py (1)
33-33: LGTM!The entity count assertions have been correctly updated to reflect the module_id field additions in the legacy Anna fixtures, resulting in 2 additional entity items per test case.
Also applies to: 68-68
plugwise/__init__.py (1)
335-335: LGTM!The error message enhancement improves observability by including the original exception details, making debugging easier while maintaining the existing exception handling flow.
tests/test_anna.py (1)
32-32: LGTM!The entity count assertions have been correctly updated across all Anna test cases to reflect the module_id field additions in the fixtures. The count adjustments are consistent with the expected entity increases, including the expected reduction at line 219 during an incremental update cycle.
Also applies to: 104-104, 133-133, 161-161, 189-189, 219-219, 244-244, 290-290, 316-316, 339-339, 359-359, 390-390, 443-443, 508-508, 531-531, 554-554
tests/data/adam/adam_zone_per_device.json (1)
9-9: LGTM!The module_id field additions to test data are well-formed and consistent with the broader schema extension for module tracking.
Also applies to: 97-97, 151-151, 173-173, 199-199, 228-228, 251-251, 318-318, 344-344, 370-370, 398-398, 455-455, 481-481, 510-510, 551-551, 580-580
fixtures/adam_zone_per_device/data.json (1)
9-9: LGTM!The module_id field additions are consistent with the test data and properly formatted, supporting the broader schema extension.
Also applies to: 97-97, 151-151, 173-173, 199-199, 228-228, 251-251, 318-318, 344-344, 370-370, 398-398, 455-455, 481-481, 510-510, 551-551, 580-580
fixtures/m_anna_heatpump_cooling/data.json (1)
46-46: LGTM!The module_id field additions for the heater/cooler and thermostat devices are well-formed and consistent with the PR's schema extension.
Also applies to: 72-72
plugwise/smile.py (3)
91-92: LGTM!The initialization of
_zonesandgw_entitiescaches sets up the infrastructure for preserving entity data across updates, supporting the PR's objective of optimized incremental updates.
141-141: LGTM!The error message enhancement improves observability by including the original KeyError details, making it easier to diagnose data retrieval failures.
112-117: The unconditional call to_scan_thermostats()is safe due to internal guards.The method includes an early return check (
if not self.check_name(ADAM): returnat line 830 ofplugwise/helper.py) that ensures it only performs work when the device is an ADAM thermostat gateway. This internal guard makes the unconditional invocation safe regardless of the conditional block's state. The change allows thermostat state checking to occur independently of appliance collection or thermostat device status, which is the intended behavior.plugwise/common.py (5)
57-59: LGTM! Group lifecycle tracking initialized.The private state variables
_existing_groupsand_new_groupsproperly support incremental group updates as intended by the PR objectives.
121-121: LGTM! Module ID propagated for heater_central.Properly wires module_id from module_data into the appliance structure.
146-146: LGTM! Module ID propagated for thermostats.Properly wires module_id from module_data into the appliance structure.
163-163: LGTM! Module ID included in gateway entities.Completes the module_id propagation chain into
gw_entities.
284-284: LGTM! Module ID field initialized.Properly initializes the
module_idfield in the module_data structure.
| if ( | ||
| group_id in self._existing_groups | ||
| and self.gw_entities[group_id]["name"] == group_name | ||
| ): | ||
| continue |
There was a problem hiding this comment.
Add defensive check before accessing gw_entities.
Line 215 accesses self.gw_entities[group_id]["name"] assuming that if group_id is in _existing_groups, it must exist in gw_entities. If these collections become desynchronized (e.g., if gw_entities is cleared elsewhere while _existing_groups persists), this will raise a KeyError.
🔎 Proposed fix to prevent KeyError
self._new_groups.append(group_id)
if (
group_id in self._existing_groups
+ and group_id in self.gw_entities
and self.gw_entities[group_id]["name"] == group_name
):
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| group_id in self._existing_groups | |
| and self.gw_entities[group_id]["name"] == group_name | |
| ): | |
| continue | |
| if ( | |
| group_id in self._existing_groups | |
| and group_id in self.gw_entities | |
| and self.gw_entities[group_id]["name"] == group_name | |
| ): | |
| continue |
🤖 Prompt for AI Agents
In plugwise/common.py around lines 213 to 217, the code assumes self.gw_entities
contains group_id when checking self.gw_entities[group_id]["name"], which can
raise KeyError if the two collections desynchronize; add a defensive check to
ensure group_id exists in self.gw_entities before accessing it (e.g., check "and
group_id in self.gw_entities and self.gw_entities[group_id].get('name') ==
group_name"), and keep the existing continue behavior when the check passes.
| def _update_zigbee_availability(self, entity: GwEntityData) -> None: | ||
| """Update zigbee device availability status.""" | ||
| if "module_id" in entity: | ||
| module_id = entity["module_id"] | ||
| locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node' | ||
| if (module := self._domain_objects.find(locator)) is not None: | ||
| entity["available"] = module.find("reachable").text == "true" |
There was a problem hiding this comment.
Guard access to the reachable child element to prevent AttributeError.
Line 458 calls module.find("reachable").text without checking if find() returns None. While the parent module element is guarded at line 457, the child reachable element is not. If the reachable element is missing, this will raise an AttributeError.
🔎 Proposed fix
def _update_zigbee_availability(self, entity: GwEntityData) -> None:
"""Update zigbee device availability status."""
if "module_id" in entity:
module_id = entity["module_id"]
locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node'
if (module := self._domain_objects.find(locator)) is not None:
- entity["available"] = module.find("reachable").text == "true"
+ if (reachable := module.find("reachable")) is not None:
+ entity["available"] = reachable.text == "true"🤖 Prompt for AI Agents
In plugwise/helper.py around lines 452 to 458, the code accesses
module.find("reachable").text without checking whether the reachable element
exists, which can raise AttributeError; update the logic to first retrieve the
reachable element into a variable, check it is not None (and optionally that
.text is not None), then set entity["available"] based on reachable.text ==
"true"; if reachable is missing, set entity["available"] to False (or a safe
default) to avoid exceptions.
Try 4
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise/helper.py (1)
444-450: Guard against AttributeError when reachable element is missing.Line 450 calls
.texton the result ofmodule.find("reachable")without checking if the element exists. While the parentmoduleelement is guarded at line 449, thereachablechild element is not. If thereachableelement is missing from the XML, this will raiseAttributeError.🔎 Proposed fix to safely access the reachable element
def _update_zigbee_availability(self, entity: GwEntityData) -> None: """Update zigbee device availability status.""" if "module_id" in entity: module_id = entity["module_id"] locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node' if (module := self._domain_objects.find(locator)) is not None: - entity["available"] = module.find("reachable").text == "true" + if (reachable := module.find("reachable")) is not None: + entity["available"] = reachable.text == "true"
🧹 Nitpick comments (1)
plugwise/common.py (1)
214-218: Consider adding defensive check before accessing gw_entities.Line 216 assumes that if
group_idis in_existing_groups, it must also be ingw_entities. While the PR objectives indicategw_entitiesshould be preserved across updates, adding a defensive check would prevent potentialKeyErrorif these collections become desynchronized (e.g., through future code changes or error conditions).🔎 Proposed defensive guard
if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): continue
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/common.pyplugwise/helper.pyplugwise/smile.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/smile.pyplugwise/helper.py
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/smile.pyplugwise/helper.pyplugwise/common.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-12-21T10:56:19.146Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:19.146Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (3)
plugwise/smile.py (6)
plugwise/helper.py (2)
_get_appliances(115-196)_scan_thermostats(816-853)plugwise/legacy/helper.py (1)
_get_appliances(86-140)plugwise/common.py (1)
_get_groups(196-237)plugwise/data.py (1)
_all_entity_data(32-40)plugwise/legacy/data.py (1)
_all_entity_data(18-23)plugwise/exceptions.py (1)
DataMissingError(12-13)
plugwise/helper.py (3)
plugwise/constants.py (2)
ThermoLoc(515-521)GwEntityData(537-600)plugwise/legacy/helper.py (2)
_get_appliances(86-140)_appliance_info_finder(191-204)plugwise/common.py (2)
_create_gw_entities(151-171)_reorder_devices(173-181)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (17)
plugwise/smile.py (4)
109-112: LGTM: Offset functionality collection properly gated.The conditional ensures
therms_with_offset_funcis refreshed only when appliances have changed (added/removed) and the system is a thermostat gateway, keeping the list synchronized withgw_entities.
114-114: LGTM: Thermostat scanning now runs unconditionally.Moving
_scan_thermostats()outside the conditional block ensures zone updates are processed on every call, regardless of whether appliances changed. The method has its own internal guards for Adam-specific logic.
118-127: LGTM: Offset functionality collection is correctly implemented.The XPath query correctly navigates the XML structure to find appliances with
temperature_offsetcapability, and the list comprehension properly extracts their IDs.
149-149: LGTM: Enhanced error message for better debugging.Including the underlying exception details (
{err}) in the error message will help diagnose which specific key is missing from the data.plugwise/common.py (6)
57-61: LGTM: Lifecycle tracking infrastructure for groups.The addition of
_existing_groupsand_new_groupsenables detecting group additions and removals, consistent with the pattern used for appliances, locations, and zones elsewhere in the codebase.
121-121: LGTM: Module ID propagation is consistent.The
module_idis correctly propagated frommodule_datathrough the appliance object togw_entities, enabling module tracking throughout the data flow.Also applies to: 146-146, 163-163
208-209: LGTM: Defensive check prevents processing groups without IDs.The guard correctly skips groups with missing IDs, preventing potential issues downstream.
220-237: LGTM: Group lifecycle management is correctly implemented.The logic properly:
- Validates group type and members before creating entities
- Removes groups that are no longer present
- Updates tracking lists to maintain state across update cycles
This aligns with the PR objective to detect and apply only changes to groups.
239-247: LGTM: Member collection helper is well-implemented.The
_collect_membershelper cleanly encapsulates the logic for finding and validating group members, using the walrus operator effectively and ensuring only valid members (present ingw_entities) are included.
283-283: LGTM: Module ID added to module_data structure.The
module_idfield is properly initialized and populated from the module element'sidattribute, completing the module tracking data structure.Also applies to: 307-307
plugwise/helper.py (7)
89-94: LGTM: Comprehensive lifecycle tracking infrastructure.The initialization of
_existing_*and_new_*tracking lists for appliances, locations, and zones, along with the typedgw_entitiesinitialization, provides the foundation for incremental updates per the PR objectives.Also applies to: 102-102
115-196: LGTM: Appliance lifecycle tracking correctly implemented.The method now:
- Returns
Truewhen appliances are added or removed, enabling callers to detect changes- Properly tracks new vs. existing appliances
- Cleans up
_new_applianceslist if appliance info collection fails- Removes stale appliances from
gw_entities- Maintains tracking state across update cycles
This aligns with the PR objective to update only changed appliances rather than redetecting everything.
198-242: LGTM: P1 smartmeter handling extracted and optimized.The new method properly:
- Extracts smartmeter module data from the home location
- Detects smartmeter changes via
module_idcomparison- Skips unnecessary updates when the module hasn't changed (optimization aligning with PR objectives)
- Handles the entity ID swapping logic for backward compatibility
Based on learnings and past review confirmation, the direct access to
gw_entities[self._gateway_id]is safe because the gateway entity is guaranteed to exist at this point in the execution flow.
264-284: LGTM: Location lifecycle tracking properly implemented.The location tracking logic correctly:
- Skips unchanged locations by comparing names
- Initializes
_loc_datastructure for new/changed locations- Removes stale locations from
_loc_data- Updates tracking lists to maintain state
Note that zone cleanup happens separately in
_scan_thermostats, which is appropriate since zones are derived from locations with thermostats.
67-67: LGTM: Prevents duplicate "_plug" suffix.The guard ensures
_plugis only added once to the device class name, preventing issues if the function is called multiple times or if the class name already has the suffix.
323-323: LGTM: Module ID propagation for plug devices.Plug-type appliances now receive
module_idfrom their module data, consistent with other appliance types.
828-853: LGTM: Zone lifecycle tracking correctly implemented.The zone tracking logic properly:
- Appends location IDs to
_new_zonesfor locations with thermostats- Skips unchanged zones by comparing names in
gw_entities(zones uselocation_idasentity_id)- Creates zone entities in
_zonesdict with thermostat mappings- Safely removes stale zones using
pop(location_id, None)- Updates tracking lists to maintain state
Based on learnings, the access to
self.gw_entities[location_id]at line 831 is safe because zones are added togw_entitiesusinglocation_idas the key, so if it's in_existing_zones, it should be present from the previous update cycle.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise/data.py (1)
197-198: Verify that the called method handles missing XML elements safely.This call to
_update_zigbee_availabilityinvokes a method inhelper.py(lines 443-449) that accessesmodule.find("reachable").textwithout checking iffind()returnsNone. As flagged in a previous review, if thereachableelement is missing from the XML, this will raise anAttributeError. Adding this call here increases exposure to that bug. Ensure the method inhelper.pyis fixed to guard against missing elements before relying on it during update cycles.Based on past review comments, the fix should wrap the
.textaccess in a null-check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/data.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/data.py
🧬 Code graph analysis (1)
plugwise/data.py (1)
plugwise/helper.py (1)
_update_zigbee_availability(444-450)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/data.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/data.py
🧬 Code graph analysis (1)
plugwise/data.py (1)
plugwise/helper.py (1)
_update_zigbee_availability(444-450)
🔇 Additional comments (1)
plugwise/data.py (1)
106-120: Previous issues resolved - implementation now correct.The past critical issues in this method have been addressed:
- Line 115 now correctly checks
entity["binary_sensors"]instead of the top-level entity dict- The else branch that unconditionally incremented the counter by 2 on every update has been removed
- Early return for non-gateway entities improves clarity
The method now only increments the counter when new keys are actually added, which aligns with this PR's goal of optimizing updates.
| # Zigbee node availability | ||
| self._update_zigbee_availability(entity) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "data.py" -o -name "helper.py" | head -20Repository: plugwise/python-plugwise
Length of output: 161
🏁 Script executed:
git ls-files | grep -E "(data\.py|helper\.py)" | head -20Repository: plugwise/python-plugwise
Length of output: 153
🏁 Script executed:
cat -n plugwise/data.py | sed -n '190,230p'Repository: plugwise/python-plugwise
Length of output: 1869
🏁 Script executed:
cat -n plugwise/helper.py | sed -n '440,460p'Repository: plugwise/python-plugwise
Length of output: 991
Fix two critical issues in ZigBee availability check.
This call to _update_zigbee_availability has two unresolved critical problems:
-
Missing None check for
reachableelement: At helper.py line 450,module.find("reachable").textis called without checking iffind()returnsNone. Whilemoduleis guarded (line 449), the childreachableelement is not, causing anAttributeErrorif the element is missing. -
Counter inconsistency:
_check_availability(data.py line 220) incrementsself._countwhen settingentity["available"], but_update_zigbee_availabilitydoes not. Since both methods set the same key and are called on the same entity in_get_entity_data(lines 194-198), this inconsistency breaks the counter logic that tracks changes.
Guard the reachable element access and add counter increment to _update_zigbee_availability:
if (module := self._domain_objects.find(locator)) is not None:
reachable = module.find("reachable")
if reachable is not None:
entity["available"] = reachable.text == "true"
self._count += 1🤖 Prompt for AI Agents
In plugwise/data.py around lines 197 to 198, the call to
_update_zigbee_availability can raise AttributeError because it accesses
module.find("reachable").text without checking for None, and it also fails to
increment self._count when updating entity["available"], causing counter
inconsistency with _check_availability; fix by first assigning reachable =
module.find("reachable") and only set entity["available"] when reachable is not
None, and whenever you set entity["available"] here also increment self._count
to keep the counter logic consistent.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise/common.py (1)
217-221: Add defensive check before accessing gw_entities.Line 219 accesses
self.gw_entities[group_id]["name"]assuming that ifgroup_idis in_existing_groups, it must exist ingw_entities. If these collections become desynchronized (e.g., ifgw_entitiesis cleared elsewhere while_existing_groupspersists), this will raise aKeyError.This issue was flagged in a previous review but has not been addressed.
🔎 Proposed fix to prevent KeyError
if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): continueBased on past review comments, this defensive check is necessary to prevent KeyError if the collections desynchronize.
🧹 Nitpick comments (3)
plugwise/common.py (1)
213-213: Remove temporary debug logs.The debug statements with "HOI" prefix appear to be temporary development logs and should be removed before merging.
🔎 Proposed cleanup
if members := self._collect_members(group): - LOGGER.debug("HOI members: %s", members) self._new_groups.append(group_id) - LOGGER.debug("HOI get_existing_groups: %s", self._existing_groups) if (Also applies to: 216-216
plugwise/smile.py (2)
110-110: Remove temporary debug logs.The debug statements with "HOI" prefix appear to be temporary development logs and should be removed before merging.
🔎 Proposed cleanup
- LOGGER.debug("HOI 1 get_all_gateway_entities data: %s", self.gw_entities) if self._get_appliances() and self._is_thermostat: self.therms_with_offset_func = ( self._get_appliances_with_offset_functionality() ) self._scan_thermostats() - LOGGER.debug("HOI 2 get_all_gateway_entities data: %s", self.gw_entities) self._get_groups()Also applies to: 117-117
122-122: Fix grammar in docstring."all appliance" should be "all appliances".
🔎 Proposed fix
- """Helper-function collecting all appliance that have offset_functionality.""" + """Helper-function collecting all appliances that have offset_functionality."""
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/common.pyplugwise/smile.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/common.pyplugwise/smile.py
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/smile.py
🧬 Code graph analysis (1)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
🔇 Additional comments (7)
plugwise/common.py (5)
58-60: LGTM: Group lifecycle tracking state initialization.The initialization of
_existing_groupsand_new_groupsproperly supports the new differential update logic for groups.
122-122: LGTM: Module ID propagation is consistent.The
module_idfield is correctly propagated frommodule_datathrough appliance objects togw_entities, aligning with the data structure changes inplugwise/constants.py.Also applies to: 147-147, 164-164
234-240: LGTM: Group pruning logic is correct.The logic correctly identifies removed groups and cleans them from
gw_entities, then updates the tracking state for the next update cycle.
242-250: LGTM: Member collection helper is well-structured.The
_collect_membershelper safely checks member existence ingw_entitiesbefore adding them to the list, preventing references to non-existent entities.
310-310: Verify that module elements always have an 'id' attribute.Line 310 directly accesses
module.attrib["id"]without checking if the"id"attribute exists. If a module element lacks this attribute, aKeyErrorwill be raised.Review the XML schema or device behavior to confirm that module elements always include an
"id"attribute, or add a defensive check.🔎 Optional defensive fix
If there's any possibility of missing
idattributes:- module_data["module_id"] = module.attrib["id"] + module_data["module_id"] = module.get("id")plugwise/smile.py (2)
111-116: LGTM: Offset functionality collection is properly synchronized.The condition ensures
therms_with_offset_funcis refreshed whenever appliances change (added or removed), addressing the synchronization concern from the previous review.
152-152: LGTM: Enhanced error message improves debugging.Including the original exception details in the error message will help with troubleshooting data retrieval failures.
| for item in offset_appls: | ||
| therm_list.append(item.get("id")) |
There was a problem hiding this comment.
Handle missing 'id' attribute defensively.
Line 128 uses item.get("id") which returns None if the "id" attribute is missing. Appending None to therm_list could cause issues when the list is used later for validation or lookups in set_offset().
Add a check to skip items without an id attribute.
🔎 Proposed fix
for item in offset_appls:
- therm_list.append(item.get("id"))
+ if (item_id := item.get("id")) is not None:
+ therm_list.append(item_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for item in offset_appls: | |
| therm_list.append(item.get("id")) | |
| for item in offset_appls: | |
| if (item_id := item.get("id")) is not None: | |
| therm_list.append(item_id) |
🤖 Prompt for AI Agents
In plugwise/smile.py around lines 127 to 128, the loop appends item.get("id")
which can be None if the "id" key is missing; update the loop to check for a
truthy id before appending (skip items where id is None or missing) so
therm_list contains only valid ids, optionally logging or counting skipped items
for diagnostics.
Instead of redetecting all devices, locations and zones at every update, only update the devices, locations and zones that have changed.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.