Optimize updating #835
Optimize updating #835
Conversation
WalkthroughAdds module_id propagation and lifecycle tracking: module IDs flow from module data through appliances into gw_entities; group lifecycle (detect/prune) introduced; thermostat ranking and ZigBee availability checks added; many fixtures/tests updated to include module_id and adjusted expected entity counts. Changes
Sequence Diagram(s)sequenceDiagram
participant SmileAPI
participant Helper
participant Common
participant Module
participant Gw as gw_entities
Note over SmileAPI,Gw: High-level module_id propagation, group lifecycle, thermostat ranking
SmileAPI->>Helper: _get_appliances()
Helper->>Helper: collect appliances, append to _new_appliances
Helper->>Common: call appliance handlers / create gw entries
Common->>Module: _get_module_data(module element)
Module-->>Common: module_data (includes module_id)
Common->>Helper: set appl.module_id = module_data.module_id
Helper->>Gw: _create_gw_entities() (include module_id)
rect rgb(220,240,250)
Note over Common: Group lifecycle handling
Common->>Common: _get_groups() (append _new_groups, skip unchanged)
Common->>Common: prune removed groups (existing vs new)
end
rect rgb(240,220,240)
Note over Helper: Thermostat ranking & ZigBee availability
Helper->>Helper: _match_and_rank_thermostats()
Helper->>Helper: _rank_thermostat() per location
Helper->>Helper: _get_zigbee_availability(entity)
Helper->>Gw: update entity availability & zones
end
SmileAPI->>Gw: get_all_gateway_entities()
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.63% -0.37%
===========================================
Files 21 21
Lines 3435 3514 +79
===========================================
+ Hits 3435 3501 +66
- Misses 0 13 +13 ☔ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise/helper.py (1)
836-861: Critical: Incorrect dict access causes KeyError.Line 839 accesses
self.gw_entities[location_id]["name"]but zones are stored inself._zones, notself.gw_entities. The condition at line 838 checkslocation_id in self._existing_zones, which confirms this is zone-related tracking.This will raise
KeyErroron the second update cycle when zones exist from the previous run.🔎 Proposed fix
self._match_and_rank_thermostats() for location_id, location in self._loc_data.items(): if location["primary_prio"] != 0: self._new_zones.append(location_id) if ( location_id in self._existing_zones - and self.gw_entities[location_id]["name"] == location["name"] + and self._zones[location_id]["name"] == location["name"] ): continue
🧹 Nitpick comments (2)
plugwise/helper.py (2)
114-193: Approve the optimization approach with a minor defensive coding suggestion.The return type change and lifecycle tracking logic correctly implement the PR's goal of incremental updates. The method now returns
Truewhen appliances are added or removed, enabling callers to react to changes.The skip logic (lines 161-165) optimizes by detecting unchanged appliances via name comparison, which is reasonable for most use cases. However, line 163 directly accesses
self.gw_entities[appl.entity_id]["name"]which could raiseKeyErrorif the tracking state becomes inconsistent.🔎 Optional: Add defensive dict access
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 + or self.gw_entities[appl.entity_id].get("name") == appl.name ): continueNote: The optimization skips updates when only the name is unchanged. This means changes to other attributes (firmware, location, etc.) without a name change won't trigger an update. This is acceptable given the PR's optimization goals, but be aware of this trade-off.
261-281: Consistent location lifecycle tracking with minor defensive coding suggestion.The location tracking follows the same pattern as appliances, correctly detecting additions and removals. The separation of location and zone cleanup is intentional—zones are pruned later in
_scan_thermostats()since they depend on thermostat presence.Similar to line 163, line 264 directly accesses
self._loc_data[loc.loc_id]["name"]which could raiseKeyErrorif tracking state becomes inconsistent.🔎 Optional: Add defensive dict access
self._new_locations.append(loc.loc_id) if ( loc.loc_id in self._existing_locations - and self._loc_data[loc.loc_id]["name"] == loc.name + and self._loc_data[loc.loc_id].get("name") == loc.name ): continue
📜 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 (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 (4)
plugwise/helper.py (4)
88-93: LGTM! Clean initialization of lifecycle tracking.The tracking lists are properly initialized to support incremental updates of appliances, locations, and zones.
195-217: LGTM! Robust duplicate detection.The new
_add_p1_smartmeter_info()method correctly detects when a smartmeter module_id already exists, preventing duplicate entity creation. The safe.get()access pattern (lines 212-214) properly handles both the current gateway location and legacy home location cases without KeyError risk.
320-320: LGTM! Module ID propagation for plugs.Correctly extends module_id tracking to plug-type appliances, consistent with the pattern used for other device types.
452-458: LGTM! Clean ZigBee availability tracking.The method correctly updates entity availability based on ZigBee node reachability. The in-place modification pattern is consistent with other helper methods in this class.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugwise/helper.py (1)
275-278: Consider using pop() with a default for defensive coding.While
_loc_data.pop(location_id)should be safe here (removed locations should exist in_loc_datafrom previous runs), using.pop(location_id, None)would make the code more defensive against unexpected states.🔎 Optional diff for consistency with _zones.pop pattern
if self._existing_locations and removed: for location_id in removed: - self._loc_data.pop(location_id) + self._loc_data.pop(location_id, None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.pyplugwise/helper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/common.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 (2)
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 (7)
plugwise/helper.py (7)
88-93: LGTM! Lifecycle tracking initialization is well-structured.The initialization of tracking lists for appliances, locations, and zones supports the targeted update optimization described in the PR objectives.
228-228: LGTM! Module ID propagation is correctly implemented.The module_id is properly assigned for both P1 smartmeter and plug devices, supporting the enhanced tracking described in the AI summary.
Also applies to: 320-320
181-193: LGTM! Appliance lifecycle tracking is correctly implemented.The removal detection, state updates, and return logic properly handle the optimization goals. The method now returns a boolean indicating whether structural changes occurred, enabling targeted updates.
452-458: LGTM! ZigBee availability check is well-implemented.The method correctly updates entity availability based on ZigBee node reachability when module_id is present.
855-861: LGTM! Zone removal logic is correctly implemented.The zone lifecycle tracking properly handles removals with safe
pop(location_id, None)and maintains consistent state across updates.
268-273: LGTM! Location data structure is well-defined.The location data initialization properly sets up the structure for thermostat ranking and tracking, consistent with the
ThermoLoctype.
160-165: Confirm whether name updates should be skipped for gateway/heater_central appliances.The condition at lines 161-165 skips processing for gateway and heater_central appliances whenever they already exist, regardless of whether their names have changed. For other appliance types, processing is only skipped if the name is unchanged. This means name changes for gateway/heater_central entities won't be reflected in
gw_entitiessince_create_gw_entities()is never called for them on subsequent updates.If name updates for these priority devices should be supported, the condition needs adjustment.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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)
_get_zigbee_availability(452-458)
⏰ 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
| else: | ||
| entity["binary_sensors"]["plugwise_notification"] = bool( | ||
| self._notifications | ||
| ) | ||
| entity["notifications"] = self._notifications | ||
| self._count += 2 |
There was a problem hiding this comment.
Fix incorrect counter increment on update path.
The counter is incremented by 2 in the else branch when updating existing plugwise_notification and notifications keys. The counter should only be incremented when adding new data points, not when updating existing ones. This causes incorrect tracking of changes, which is particularly important given this PR's goal of optimizing updates.
🔎 Proposed fix
else:
entity["binary_sensors"]["plugwise_notification"] = bool(
self._notifications
)
entity["notifications"] = self._notifications
- self._count += 2📝 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.
| else: | |
| entity["binary_sensors"]["plugwise_notification"] = bool( | |
| self._notifications | |
| ) | |
| entity["notifications"] = self._notifications | |
| self._count += 2 | |
| else: | |
| entity["binary_sensors"]["plugwise_notification"] = bool( | |
| self._notifications | |
| ) | |
| entity["notifications"] = self._notifications |
🤖 Prompt for AI Agents
In plugwise/data.py around lines 122 to 127, the else branch unconditionally
adds 2 to self._count when assigning
entity["binary_sensors"]["plugwise_notification"] and entity["notifications"],
which is wrong for updates; change the logic to only increment self._count for
each key that did not previously exist (check presence before assigning and
increment by 1 per newly added key), e.g. test for the existence of
entity["binary_sensors"].get("plugwise_notification") and "notifications" in
entity before updating and only add to self._count for keys that are being
created.
| # Zigbee node availability | ||
| self._get_zigbee_availability(entity) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the _get_zigbee_availability method
rg -n "_get_zigbee_availability" --type=py -A 20Repository: plugwise/python-plugwise
Length of output: 2674
🏁 Script executed:
# Also check the _check_availability method to understand counter logic
rg -n "_check_availability" --type=py -A 15Repository: plugwise/python-plugwise
Length of output: 2211
🏁 Script executed:
# Search for module.find("reachable") to see error handling
rg -n "module\.find\(" --type=py -B 2 -A 3Repository: plugwise/python-plugwise
Length of output: 2539
🏁 Script executed:
# Check where entity["available"] is set to understand the pattern
rg -n "entity\[.available.\].*=" --type=py -B 2 -A 2Repository: plugwise/python-plugwise
Length of output: 1000
Fix error handling for missing reachable element in ZigBee availability check.
Line 458 in helper.py calls module.find("reachable").text without checking if the find() returns None, which will raise an AttributeError if the reachable element is missing. The parent module element is guarded (line 457), but the child reachable element is not.
Additionally, verify the counter logic: _check_availability increments self._count when setting entity["available"] (line 227 in data.py), but _get_zigbee_availability does not increment the counter. Both methods can set this key on the same entity, which could lead to counter inconsistency—clarify whether this is intentional or if both should maintain consistent counter behavior.
🤖 Prompt for AI Agents
In plugwise/data.py around lines 204-205 and helper.py around line 458, guard
access to the ZigBee "reachable" child element before calling .text (replace
module.find("reachable").text with a None-check and treat missing reachable as
not reachable or skip setting availability), and make counter behavior
consistent: either increment self._count whenever you set entity["available"] =
True (add the increment in _get_zigbee_availability when it sets availability)
or refactor so only _check_availability updates self._count and other callers
only set the available flag; pick one approach and apply it consistently so the
counter cannot be out of sync.
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.