Add support for Anna P1 #809
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
WalkthroughAdds Anna P1 support: new fixtures, test data, and domain objects; detects Anna P1 in Smile gateway metadata and sets an Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Initialiser
participant Detect as _smile_detect
participant Env as GatewayEnv
participant Smile as SmileState
Init->>Detect: load gateway/vendor XML
Detect->>Env: read gateway_environment.electricity_consumption_tariff_structure
Detect->>Detect: read vendor_model -> resolve model
alt tariff present & vendor_model == "smile_thermo"
Detect->>Smile: set anna_p1 = True
Detect->>Smile: set model = "Gateway", model_id = resolved model
Note right of Smile: if "Smile Anna" in name → rename to "Smile Anna P1"
else
Detect->>Smile: ensure anna_p1 = False
end
Detect-->>Init: return updated smile metadata
sequenceDiagram
participant Flow as P1 Processing
participant Check as Eligibility
participant Module as _get_module_data
participant Map as EntityMapping
participant Collect as _collect_appliance_data
Flow->>Check: is (smile.type == "power") OR smile.anna_p1?
alt eligible
Check->>Module: fetch modules with key="electricity"
Module-->>Check: return first matching module_data
Check->>Map: select entity_id source
alt anna_p1 == True
Map-->>Check: use home_loc_id as entity_id
else
Map-->>Check: use gateway_id as entity_id
end
Check->>Collect: collect measurements & states for chosen entity
Collect-->>Flow: return appliance node
else
Check-->>Flow: skip P1 handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #809 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3344 3377 +33
=========================================
+ Hits 3344 3377 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugwise/data.py (1)
13-13: Consider clarifying debug log message.The debug logging addition is helpful for diagnostics, but the "HOI" abbreviation may be unclear.
Consider clarifying the log message:
- LOGGER.debug("HOI entity = %s", entity) + LOGGER.debug("Processing heater_central entity: %s", entity)Or add a comment explaining "HOI" if it's established terminology in the codebase.
Also applies to: 310-310
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
fixtures/anna_p1/data.json(1 hunks)plugwise/__init__.py(3 hunks)plugwise/common.py(1 hunks)plugwise/constants.py(1 hunks)plugwise/data.py(2 hunks)plugwise/helper.py(2 hunks)plugwise/smile.py(1 hunks)tests/data/anna/anna_p1.json(1 hunks)tests/test_anna.py(2 hunks)userdata/anna_p1/core.domain_objects.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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_p1.jsonuserdata/anna_p1/core.domain_objects.xmlfixtures/anna_p1/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)
tests/test_anna.py (2)
tests/test_init.py (4)
load_testdata(81-90)connect_wrapper(366-401)device_test(480-601)disconnect(453-456)plugwise/smilecomm.py (1)
close_connection(148-150)
🔇 Additional comments (13)
plugwise/smile.py (1)
364-364: LGTM: Formatting change only.The method signature has been reformatted to a single line with no functional changes.
plugwise/constants.py (1)
37-37: LGTM: Clean constant addition.The new
ANNA_P1constant follows the established pattern and supports the Anna P1 device identification.tests/test_anna.py (1)
361-365: LGTM: Improved formatting.The multi-line formatting improves readability for the function call with multiple parameters.
plugwise/helper.py (2)
153-154: LGTM: Correctly extends P1 data collection for Anna P1.The condition now triggers P1 smartmeter data collection for both pure P1 devices and Anna P1 thermostats with P1 ports.
327-332: LGTM: Enables dual data collection for Anna P1.The conditional return logic correctly handles Anna P1 devices by:
- Processing P1 smartmeter data when
anna_p1is True- Continuing to non-P1 appliance data collection (skipping early return)
- This allows Anna P1 to collect both thermostat and P1 measurements
plugwise/__init__.py (3)
78-78: LGTM: Clean flag initialization.The
anna_p1flag is properly initialized to False, following the established pattern for other boolean flags.
193-203: LGTM: Robust Anna P1 detection logic.The detection correctly identifies Anna thermostats with P1 smartmeter connections by:
- Checking for P1-specific XML structure (
electricity_consumption_tariff_structure)- Verifying the model is
smile_thermo(Anna)- Setting the
anna_p1flag appropriatelyThe variable rename from
v_modeltovendor_modelalso improves code clarity.
244-244: LGTM: Clean model identification and naming.The code correctly:
- Assigns
model_idfrom the detected model- Renames "Smile Anna" to "Smile Anna P1" when the P1 flag is set
This provides clear differentiation between standard Anna and Anna P1 configurations.
Also applies to: 247-248
tests/data/anna/anna_p1.json (1)
1-104: LGTM: Comprehensive test data for Anna P1 scenario.The test data correctly models an Anna P1 setup with:
- Anna thermostat with standard thermostat capabilities
- OpenTherm heater gateway for central heating
- P1 smartmeter with complete electricity and gas measurements
- Gateway properly named "Smile Anna P1" with model_id "smile_thermo"
The data structure is consistent with other test fixtures and provides good coverage for Anna P1 functionality.
plugwise/common.py (1)
66-68: Substring matching is safe — no false positives detected.The change from exact equality to substring containment is intentional and well-scoped. The runtime device names are constrained to: "Adam", "Smile Anna", "Smile Anna P1", "Smile P1", and "Stretch" (from SMILES dictionary). The substring logic correctly handles the Anna P1 variant case (lines 247-248 in init.py dynamically update smile.name to "Smile Anna P1" when needed), allowing check_name(ANNA) to match both "Smile Anna" and "Smile Anna P1" without causing false positives against other device names.
fixtures/anna_p1/data.json (1)
1-102: Fixture data structure looks solid. The four device entries (Anna thermostat, OpenTherm heater gateway, P1 smart meter, and Smile Anna P1 gateway) are well-formed with complete sensor and property data. Location references align with domain_objects.xml, and the naming aligns with PR objectives for Anna P1 support.userdata/anna_p1/core.domain_objects.xml (2)
1-50: XML structure and templates are well-formed. The file provides comprehensive domain objects including templates, regulations, modules, and locations. Cross-references appear consistent and all entity IDs are properly formatted.
806-812: Remove this review comment—the identified collision is not a practical concern.The JSON fixtures (
fixtures/anna_p1/data.jsonandtests/data/anna/anna_p1.json) are not used anywhere in the test suite. Thetest_connect_anna_p1()test loads only the XML userdata (userdata/anna_p1/core.domain_objects.xml) viaload_testdata(), which reads fromuserdata/{smile_setup}/core.domain_objects.xml. Since the JSON fixtures are unused and the XML is loaded independently, they are never loaded together, so the ID reuse does not cause conflicts.Likely an incorrect or invalid review comment.
making it possible to search for a more specific module tag
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise/helper.py (2)
162-177: Update comment to reflect Anna P1 behavior.The comment on lines 162-163 describes entity_id switching for P1 backward compatibility, but doesn't mention the Anna P1 case where this switching is bypassed. When
anna_p1is true, the smartmeter entity_id useshome_loc_idwithout the swap, while the traditional P1 path usesgateway_id.Consider updating the comment to clarify both behaviors:
- def _get_p1_smartmeter_info(self) -> None: - """For P1 collect the connected SmartMeter info from the Home/building location. - - Note: For P1, the entity_id for the gateway and smartmeter are - switched to maintain backward compatibility with existing implementations. - """ + def _get_p1_smartmeter_info(self) -> None: + """For P1 collect the connected SmartMeter info from the Home/building location. + + Note: For traditional P1 (not Anna P1), the entity_id for the gateway and smartmeter + are switched to maintain backward compatibility. For Anna P1, the smartmeter uses + the home location ID directly. + """
330-336: Consider defensive check fordev_class.The restructured logic correctly handles Anna P1 by allowing both smartmeter data retrieval and continued appliance data processing. However,
entity.get("dev_class")could potentially returnNone, which would make the comparisondev_class == "smartmeter"safe (None != "smartmeter") but could be more explicit.If you prefer more explicit None handling, consider:
- dev_class = entity.get("dev_class") smile_is_power = self.smile.type == "power" - if (smile_is_power or self.smile.anna_p1) and dev_class == "smartmeter": + if (smile_is_power or self.smile.anna_p1) and entity.get("dev_class") == "smartmeter": data.update(self._power_data_from_location())This eliminates the intermediate variable if it's only used once, or keeps the current pattern which is also valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(4 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 (1)
plugwise/common.py (1)
_get_module_data(238-281)
⏰ 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 (3)
plugwise/helper.py (3)
167-168: LGTM! Proper filtering for electricity modules.Adding
key="electricity"appropriately filters the module data retrieval to electricity-related modules, which aligns with the Anna P1 smartmeter functionality.
153-154: No changes required—anna_p1 is properly initialized.The
anna_p1attribute is initialized toFalsein the constructor and can be set toTrueduring initialization, ensuring it always exists before the_all_appliances()method is called. The condition at line 153 is safe fromAttributeError.
189-193: The original review comment concern is invalid.The code safely handles both P1 and Anna P1 paths because device type architecture prevents the identified issue. The check at line 526 (
entity_id == self._gateway_id and self.check_name(ADAM)) ensuresselect_gateway_modeis only populated for ADAM thermostat devices. P1 devices return early at line 335-336 and never reach this code; Anna P1 devices fail the ADAM check and also never populate gateway_mode. Since only ADAM devices populate this field, and ADAM devices are never subject to the entity swap at lines 189-191, the modified_gateway_idis never used in the API URI construction at line 286. The entity ID replacement for P1/Anna P1 backward compatibility is architecturally isolated from gateway mode operations.
|
Summary by CodeRabbit
New Features
Tests
Data
Documentation