Optimize updating #835
Optimize updating #835
Conversation
WalkthroughPropagates module_id from module XML into appliances and gateway entities, adds lifecycle tracking and pruning for groups/appliances/locations/zones, introduces thermostat ranking and ZigBee availability checks, reorders availability/switching-group updates, and updates many fixtures/tests and types to include module_id. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBorder":"#5B8DB6","signalColor":"#9CC3E5","noteBackground":"#F6FBFF"}}}%%
sequenceDiagram
participant SmileAPI
participant Common
participant Helper
participant Data
participant Gw as gw_entities
Note over SmileAPI,Common: module_id propagation + lifecycle tracking
SmileAPI->>Common: get_all_gateway_entities()
Common->>Common: _get_module_data() → module_data (includes module_id)
Common->>Common: _appl_heater_central_info / _appl_thermostat_info (set appl.module_id)
Common->>Helper: _get_appliances() (mark _new/_existing appliances)
Helper->>Helper: _add_p1_smartmeter_info / _match_and_rank_thermostats / _get_zigbee_availability()
Helper->>Data: _get_entity_data(entity) (ZigBee / switching-group availability updates)
Common->>Common: _get_groups() (collect members via _collect_members, prune removed groups)
Common->>Gw: _create_gw_entities() (include module_id, availability, zone metadata)
Gw-->>SmileAPI: return populated gw_entities
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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-25T12:02:58.781ZApplied to files:
🧬 Code graph analysis (1)plugwise/common.py (1)
⏰ 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). (2)
🔇 Additional comments (9)
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.80% -0.20%
===========================================
Files 21 21
Lines 3435 3511 +76
===========================================
+ Hits 3435 3504 +69
- Misses 0 7 +7 ☔ 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/data.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/data.py
🧬 Code graph analysis (1)
plugwise/data.py (1)
plugwise/helper.py (1)
_update_zigbee_availability(444-450)
🔇 Additional comments (1)
plugwise/data.py (1)
106-120: Previous issues resolved - implementation now correct.The past critical issues in this method have been addressed:
- Line 115 now correctly checks
entity["binary_sensors"]instead of the top-level entity dict- The else branch that unconditionally incremented the counter by 2 on every update has been removed
- Early return for non-gateway entities improves clarity
The method now only increments the counter when new keys are actually added, which aligns with this PR's goal of optimizing updates.
plugwise/data.py
Outdated
| # Zigbee node availability | ||
| self._update_zigbee_availability(entity) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "data.py" -o -name "helper.py" | head -20Repository: plugwise/python-plugwise
Length of output: 161
🏁 Script executed:
git ls-files | grep -E "(data\.py|helper\.py)" | head -20Repository: plugwise/python-plugwise
Length of output: 153
🏁 Script executed:
cat -n plugwise/data.py | sed -n '190,230p'Repository: plugwise/python-plugwise
Length of output: 1869
🏁 Script executed:
cat -n plugwise/helper.py | sed -n '440,460p'Repository: plugwise/python-plugwise
Length of output: 991
Fix two critical issues in ZigBee availability check.
This call to _update_zigbee_availability has two unresolved critical problems:
-
Missing None check for
reachableelement: At helper.py line 450,module.find("reachable").textis called without checking iffind()returnsNone. Whilemoduleis guarded (line 449), the childreachableelement is not, causing anAttributeErrorif the element is missing. -
Counter inconsistency:
_check_availability(data.py line 220) incrementsself._countwhen settingentity["available"], but_update_zigbee_availabilitydoes not. Since both methods set the same key and are called on the same entity in_get_entity_data(lines 194-198), this inconsistency breaks the counter logic that tracks changes.
Guard the reachable element access and add counter increment to _update_zigbee_availability:
if (module := self._domain_objects.find(locator)) is not None:
reachable = module.find("reachable")
if reachable is not None:
entity["available"] = reachable.text == "true"
self._count += 1🤖 Prompt for AI Agents
In plugwise/data.py around lines 197 to 198, the call to
_update_zigbee_availability can raise AttributeError because it accesses
module.find("reachable").text without checking for None, and it also fails to
increment self._count when updating entity["available"], causing counter
inconsistency with _check_availability; fix by first assigning reachable =
module.find("reachable") and only set entity["available"] when reachable is not
None, and whenever you set entity["available"] here also increment self._count
to keep the counter logic consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise/common.py (1)
217-221: Add defensive check before accessing gw_entities.Line 219 accesses
self.gw_entities[group_id]["name"]assuming that ifgroup_idis in_existing_groups, it must exist ingw_entities. If these collections become desynchronized (e.g., ifgw_entitiesis cleared elsewhere while_existing_groupspersists), this will raise aKeyError.This issue was flagged in a previous review but has not been addressed.
🔎 Proposed fix to prevent KeyError
if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): continueBased on past review comments, this defensive check is necessary to prevent KeyError if the collections desynchronize.
🧹 Nitpick comments (3)
plugwise/common.py (1)
213-213: Remove temporary debug logs.The debug statements with "HOI" prefix appear to be temporary development logs and should be removed before merging.
🔎 Proposed cleanup
if members := self._collect_members(group): - LOGGER.debug("HOI members: %s", members) self._new_groups.append(group_id) - LOGGER.debug("HOI get_existing_groups: %s", self._existing_groups) if (Also applies to: 216-216
plugwise/smile.py (2)
110-110: Remove temporary debug logs.The debug statements with "HOI" prefix appear to be temporary development logs and should be removed before merging.
🔎 Proposed cleanup
- LOGGER.debug("HOI 1 get_all_gateway_entities data: %s", self.gw_entities) if self._get_appliances() and self._is_thermostat: self.therms_with_offset_func = ( self._get_appliances_with_offset_functionality() ) self._scan_thermostats() - LOGGER.debug("HOI 2 get_all_gateway_entities data: %s", self.gw_entities) self._get_groups()Also applies to: 117-117
122-122: Fix grammar in docstring."all appliance" should be "all appliances".
🔎 Proposed fix
- """Helper-function collecting all appliance that have offset_functionality.""" + """Helper-function collecting all appliances that have offset_functionality."""
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/common.pyplugwise/smile.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/common.pyplugwise/smile.py
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/smile.py
🧬 Code graph analysis (1)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
🔇 Additional comments (7)
plugwise/common.py (5)
58-60: LGTM: Group lifecycle tracking state initialization.The initialization of
_existing_groupsand_new_groupsproperly supports the new differential update logic for groups.
122-122: LGTM: Module ID propagation is consistent.The
module_idfield is correctly propagated frommodule_datathrough appliance objects togw_entities, aligning with the data structure changes inplugwise/constants.py.Also applies to: 147-147, 164-164
234-240: LGTM: Group pruning logic is correct.The logic correctly identifies removed groups and cleans them from
gw_entities, then updates the tracking state for the next update cycle.
242-250: LGTM: Member collection helper is well-structured.The
_collect_membershelper safely checks member existence ingw_entitiesbefore adding them to the list, preventing references to non-existent entities.
310-310: Verify that module elements always have an 'id' attribute.Line 310 directly accesses
module.attrib["id"]without checking if the"id"attribute exists. If a module element lacks this attribute, aKeyErrorwill be raised.Review the XML schema or device behavior to confirm that module elements always include an
"id"attribute, or add a defensive check.🔎 Optional defensive fix
If there's any possibility of missing
idattributes:- module_data["module_id"] = module.attrib["id"] + module_data["module_id"] = module.get("id")plugwise/smile.py (2)
111-116: LGTM: Offset functionality collection is properly synchronized.The condition ensures
therms_with_offset_funcis refreshed whenever appliances change (added or removed), addressing the synchronization concern from the previous review.
152-152: LGTM: Enhanced error message improves debugging.Including the original exception details in the error message will help with troubleshooting data retrieval failures.
| for item in offset_appls: | ||
| therm_list.append(item.get("id")) |
There was a problem hiding this comment.
Handle missing 'id' attribute defensively.
Line 128 uses item.get("id") which returns None if the "id" attribute is missing. Appending None to therm_list could cause issues when the list is used later for validation or lookups in set_offset().
Add a check to skip items without an id attribute.
🔎 Proposed fix
for item in offset_appls:
- therm_list.append(item.get("id"))
+ if (item_id := item.get("id")) is not None:
+ therm_list.append(item_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for item in offset_appls: | |
| therm_list.append(item.get("id")) | |
| for item in offset_appls: | |
| if (item_id := item.get("id")) is not None: | |
| therm_list.append(item_id) |
🤖 Prompt for AI Agents
In plugwise/smile.py around lines 127 to 128, the loop appends item.get("id")
which can be None if the "id" key is missing; update the loop to check for a
truthy id before appending (skip items where id is None or missing) so
therm_list contains only valid ids, optionally logging or counting skipped items
for diagnostics.
This reverts commit 10130a8.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise/common.py (1)
212-238: Fix group tracking logic to prevent KeyError.The group tracking logic has a critical mismatch:
_new_groupstracks all groups with members, butgw_entitiesonly contains groups with validgroup_typeand members. This causesKeyErrorin two scenarios:Scenario 1 - Line 217: A group with members but invalid
group_typeis added to_new_groups(line 213) but not togw_entities. On the next scan, line 217 tries to accessself.gw_entities[group_id]assuming the group exists because it's in_existing_groups.Scenario 2 - Line 235: Similarly, line 235 tries to pop a group from
gw_entitiesthat was in_existing_groupsbut never actually created ingw_entities.🔎 Recommended fix: Only track groups actually created in gw_entities
if members := self._collect_members(group): - self._new_groups.append(group_id) + pass # Members collected, check if we should create entity if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): + self._new_groups.append(group_id) continue group_type = group.find("type").text 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 + self._new_groups.append(group_id) removed = list(set(self._existing_groups) - set(self._new_groups)) if self._existing_groups and removed: for group_id in removed: - self.gw_entities.pop(group_id) + self.gw_entities.pop(group_id, None)This ensures
_new_groupsonly contains groups that exist ingw_entities, maintaining the invariant thatgroup_id in _existing_groupsimpliesgroup_id in gw_entities.
🧹 Nitpick comments (2)
plugwise/common.py (2)
13-13: Consider removing the unused import.The
LOGGERimport doesn't appear to be used in this file.🔎 Proposed fix
from plugwise.constants import ( ANNA, GROUP_TYPES, - LOGGER, NONE, PRIORITY_DEVICE_CLASSES,
208-208: Add defensive checks for required XML elements.Lines 208 and 221 call
.textonfind()results without verifying the element exists. If the XML schema doesn't guaranteenameandtypeelements for all groups, this will raiseAttributeError.🔎 Proposed defensive pattern
for group in self._domain_objects.findall("./group"): group_id = group.get("id") - group_name = group.find("name").text - if group_id is None: + name_elem = group.find("name") + type_elem = group.find("type") + if group_id is None or name_elem is None or type_elem is None: continue + group_name = name_elem.text if members := self._collect_members(group): self._new_groups.append(group_id) if ( group_id in self._existing_groups and self.gw_entities[group_id]["name"] == group_name ): continue - group_type = group.find("type").text + group_type = type_elem.textAlso applies to: 221-221
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.pyplugwise/smile.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/smile.py
🧰 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/common.py
🧬 Code graph analysis (1)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
🔇 Additional comments (2)
plugwise/common.py (2)
240-248: LGTM! Clean helper method for member collection.The
_collect_membershelper is well-implemented:
- Correctly validates member existence in
gw_entitiesbefore adding- Safe handling of potential
Nonefromitem.get("id")- Clean use of walrus operator
122-122: LGTM! Module ID propagation is consistent and well-integrated.The module_id propagation flow is correctly implemented:
- Initialized in
module_data(line 284)- Extracted from XML module element (line 308)
- Propagated to appliance objects (lines 122, 147)
- Included in gateway entities (line 164)
This aligns with the PR objective to enable module-level identification for devices and supports the targeted update strategy.
Also applies to: 147-147, 164-164, 284-284, 308-308
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugwise/common.py (1)
214-218: Past review comment not addressed: missing defensive check.The past review comment flagged that Line 215 accesses
self.gw_entities[group_id]["name"]without verifying thatgroup_idexists ingw_entities. If these collections desynchronize (e.g., ifgw_entitiesis cleared elsewhere while_existing_groupspersists), this will raise aKeyError.The defensive check
and group_id in self.gw_entitiesshould be added before accessinggw_entities[group_id], as suggested in the proposed fix above.plugwise/data.py (1)
189-192: Critical: _update_zigbee_availability has unresolved issues from past reviews.Past review comments identified two critical problems with
_update_zigbee_availability(helper.py:443-449) that remain unresolved:
Missing None check for
reachableelement: At helper.py line 449,module.find("reachable").textis called without checking iffind()returnsNone. While the parentmoduleis guarded (line 448), the childreachableelement is not, causing anAttributeErrorif the element is missing.Counter inconsistency:
_check_availability(lines 208-222) incrementsself._countwhen settingentity["available"](line 217), but_update_zigbee_availabilitydoes not. Since both methods set the same key and can be called on the same entity in the same update cycle (lines 190 and 199-206), this breaks the counter logic that tracks changes—critical for this PR's optimization goal.🔎 Required fixes in helper.py
def _update_zigbee_availability(self, entity: GwEntityData) -> None: """Update zigbee device availability status.""" if "module_id" in entity: module_id = entity["module_id"] locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node' if (module := self._domain_objects.find(locator)) is not None: - entity["available"] = module.find("reachable").text == "true" + if (reachable := module.find("reachable")) is not None: + entity["available"] = reachable.text == "true" + self._count += 1#!/bin/bash # Verify the current implementation in helper.py rg -n "def _update_zigbee_availability" --type=py -A 10
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.pyplugwise/data.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/common.pyplugwise/data.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/data.py
🧬 Code graph analysis (2)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
plugwise/data.py (2)
plugwise/helper.py (1)
_update_zigbee_availability(444-450)plugwise/common.py (1)
_entity_switching_group(183-194)
⏰ 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 (3)
plugwise/common.py (1)
121-121: LGTM: module_id propagation is consistent.The module_id field is correctly initialized, extracted from XML, propagated through appliance data structures, and stored in gw_entities. This aligns with the TypedDict updates in constants.py.
Also applies to: 146-146, 163-163, 283-283, 307-307
plugwise/data.py (2)
109-120: LGTM: notification handling correctly guards and tracks changes.The method now correctly:
- Guards with an early return for non-gateway entities (lines 111-112)
- Checks the nested
entity["binary_sensors"]dict for the key (line 115)- Only increments the counter when creating new keys (line 120)
This addresses the issues from past review comments.
197-206: LGTM: availability checks correctly repositioned.The availability checks for smartmeter and OpenTherm entities have been correctly moved to follow the Adam/Anna-specific processing. The logic correctly guards the OpenTherm check with
entity["name"] != "OnOff"(line 203).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise/helper.py (1)
444-450: Guard access to thereachablechild element to prevent AttributeError.Line 450 calls
module.find("reachable").textwithout checking iffind()returnsNone. While the parentmoduleelement is guarded at line 449, the childreachableelement is not. If thereachableelement is missing, this will raise anAttributeError.🔎 Proposed fix
def _update_zigbee_availability(self, entity: GwEntityData) -> None: """Update zigbee device availability status.""" if "module_id" in entity: module_id = entity["module_id"] locator = f'./module[@id="{module_id}"]/protocols/zig_bee_node' if (module := self._domain_objects.find(locator)) is not None: - entity["available"] = module.find("reachable").text == "true" + if (reachable := module.find("reachable")) is not None: + entity["available"] = reachable.text == "true"
🧹 Nitpick comments (1)
plugwise/helper.py (1)
161-166: Consider defensive access forgw_entitieslookup.While the current flow ensures
gw_entities[appl.entity_id]exists (removal happens after the loop), using.get()would guard against edge cases where state becomes inconsistent.🔎 Optional defensive improvement
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 - ): + if appl.entity_id in self._existing_appliances and ( + appl.pwclass in ("gateway", "heater_central") + or self.gw_entities.get(appl.entity_id, {}).get("name") == appl.name + ): continue
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/helper.pypyproject.toml
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 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/helper.py
📚 Learning: 2025-12-25T12:02:58.781Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 835
File: plugwise/helper.py:837-841
Timestamp: 2025-12-25T12:02:58.781Z
Learning: In the Python Plugwise library, zones are not separate appliances in gw_entities. When a zone is added to self.gw_entities, the location_id is used as the entity_id. In code reviews, validate that zone checks rely on location_id being present in _existing_zones and that lookups use self.gw_entities[location_id]. Do not treat zones as independent appliances.
Applied to files:
plugwise/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:19.146Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 838
File: plugwise/helper.py:210-211
Timestamp: 2025-12-21T10:56:19.146Z
Learning: In plugwise/helper.py, when parsing location XML elements, the `type` element is always present for a location, so null checks are not required when accessing `location.find("type").text`.
Applied to files:
plugwise/helper.py
⏰ 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: Run mypy
🔇 Additional comments (7)
plugwise/helper.py (7)
89-102: LGTM! Lifecycle tracking state properly initialized.The new tracking lists (
_existing_*,_new_*) and explicit initialization ofgw_entitiesas an empty dict support the incremental update pattern described in the PR objectives.
184-196: Change detection logic is correct.The set-difference approach properly identifies new and removed appliances, the state is unconditionally synchronized, and the boolean return correctly signals whether entities changed.
212-219: Module ID change detection looks correct.The early return when module_id matches prevents redundant entity updates. Per previous review discussion,
_gateway_idis guaranteed to be present at this point in the control flow.
264-284: Location lifecycle tracking is correctly implemented.Unchanged locations are skipped, new/renamed locations get fresh thermostat tracking data, and stale locations are properly cleaned up. The reset of primary/secondary lists on name change is correct since
_match_and_rank_thermostatsrebuilds the rankings.
828-853: Zone lifecycle tracking correctly implemented.The skip logic for unchanged zones, creation of new zone entries, and cleanup of removed zones are all properly structured. Based on learnings, zones use
location_idas theentity_idingw_entities, so the access at line 831 is valid whenlocation_idis in_existing_zones.
887-904: Good fix for thermostat list growth prevention.The added membership checks (
entity_id not in location["primary"],entity_id not in location["secondary"],item not in location["secondary"]) correctly prevent duplicate entries from accumulating in the thermostat lists across update cycles. This addresses the commit objective "Avoid growing of primary and secondary thermostat lists."
323-323: Module ID propagation for plugs enables ZigBee availability tracking.Propagating
module_idto plug entities allows_update_zigbee_availabilityto check reachability status.
bouwew
force-pushed
the
different-update
branch
from
0b72765 to
13fa8eb
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)
215-230: Fix desynchronization between _new_groups and gw_entities.Line 215 unconditionally appends
group_idto_new_groups, but Line 222 only creates the group entity ifgroup_typeis inGROUP_TYPES. This means a group with an invalid type gets tracked in_new_groupsbut is never added togw_entities, causing desynchronization between these collections.Additionally, the skip logic at lines 216-220 prevents re-creating an existing group, which is correct, but the group should still be tracked in
_new_groupsto prevent it from being pruned at lines 233-235.🔎 Proposed fix: only track groups that are valid and created/preserved
+ group_type = group.find("type").text + if group_type not in GROUP_TYPES: + continue + - group_name = group.find("name").text - group_type = group.find("type").text - self._new_groups.append(group_id) if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): + self._new_groups.append(group_id) continue - if group_type in GROUP_TYPES: + self._new_groups.append(group_id) self.gw_entities[group_id] = { "dev_class": group_type, "model": "Group",This ensures that:
- Invalid group types are filtered out early
- Only valid groups (created or preserved) are tracked in
_new_groups- The defensive check prevents KeyError when accessing existing groups
♻️ Duplicate comments (1)
plugwise/common.py (1)
216-220: Add defensive check before accessing gw_entities.Line 218 accesses
self.gw_entities[group_id]["name"]assuming that ifgroup_idis in_existing_groups, it must exist ingw_entities. If these collections become desynchronized (e.g., if a group is removed fromgw_entitieselsewhere or wasn't created due to invalid type), this will raise aKeyError.🔎 Proposed fix to prevent KeyError
self._new_groups.append(group_id) if ( group_id in self._existing_groups + and group_id in self.gw_entities and self.gw_entities[group_id]["name"] == group_name ): continueBased on learnings, when checking group existence, verify the entity is present in gw_entities before accessing its attributes to avoid KeyError from desynchronization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/common.py
🧰 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/common.py
🧬 Code graph analysis (1)
plugwise/common.py (1)
plugwise/constants.py (1)
GwEntityData(537-600)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Process test complexity
- GitHub Check: Build and publish Python 🐍 distributions 📦 to TestPyPI
🔇 Additional comments (9)
plugwise/common.py (9)
57-61: LGTM! Tracking state properly initialized.The addition of
_existing_groupsand_new_groupstracking lists supports the incremental update optimization objective. The type annotation forgw_entitiesis correct.
121-121: LGTM! Module ID propagation is correct.The module_id is properly propagated from module_data to the appliance object, aligning with the PR's objective to track module associations.
146-146: LGTM! Module ID propagation is correct.The module_id is properly propagated from module_data to the appliance object for thermostats.
163-163: LGTM! Module ID included in gateway entities.The module_id is correctly included in the gw_entities dictionary, matching the GwEntityData TypedDict structure.
284-284: LGTM! Module data initialization updated.The module_id field is properly initialized in the module_data dictionary.
308-308: LGTM! Module ID assigned from XML.The module_id is correctly extracted from the module's XML attribute.
240-248: LGTM! Helper method correctly implemented.The
_collect_membershelper properly checks member existence ingw_entitiesbefore adding them to the list. The method signature and variable references are correct, resolving the issues flagged in previous reviews.
232-238: LGTM! Group pruning and tracking update logic is correct.The logic correctly identifies and removes groups that are no longer present, then updates the tracking lists. However, this relies on
_new_groupsbeing correctly synchronized withgw_entities, which should be addressed by fixing the desynchronization issue noted earlier.Verify that after applying the desynchronization fix, the pruning logic correctly removes only groups that should be removed and preserves groups that still exist.
207-211: LGTM! Guard clauses are appropriate.The defensive check for
Nonegroup_id and the early return for groups with no members prevent invalid or empty groups from being processed. The use of the walrus operator is idiomatic and efficient.
|
Closing as no significant improvement in processing time. |
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.