Add a ThermoZone to Anna #935
Conversation
WalkthroughThe climate entity creation logic is simplified to accept any device with DEV_CLASS == "climate" instead of gateway-based filtering. Internal location handling is refactored from deriving self._location to storing device_id as self._location_id. Test fixtures reorganize climate data by separating overall climate control from individual thermostat representations, and entity references are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve a coherent refactor with mixed complexity: the core logic change removes gateway-based filtering and refactors location handling (moderate logic density), while the bulk of the diff consists of fixture reorganization and corresponding test updates (repetitive patterns across multiple files). The fixture changes follow a consistent structure across multiple files, reducing per-file cognitive load, though the logic change requires careful verification of API call updates and entity creation behavior. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
custom_components/plugwise/climate.py(5 hunks)custom_components/plugwise/manifest.json(1 hunks)tests/components/plugwise/fixtures/anna_heatpump_heating/data.json(2 hunks)tests/components/plugwise/fixtures/anna_v4/data.json(1 hunks)tests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.json(1 hunks)tests/components/plugwise/fixtures/m_anna_heatpump_idle/data.json(1 hunks)tests/components/plugwise/snapshots/test_climate.ambr(10 hunks)tests/components/plugwise/snapshots/test_sensor.ambr(9 hunks)tests/components/plugwise/test_climate.py(5 hunks)tests/components/plugwise/test_select.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (1)
device(91-93)
⏰ 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). (3)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Prepare and validate pre-commit
🔇 Additional comments (12)
custom_components/plugwise/climate.py (3)
75-75: LGTM! Climate entity creation broadened correctly.The change from gateway-specific filtering to
device[DEV_CLASS] == "climate"successfully removes the gateway-name gating and allows climate entities to be created for any device with the appropriate device class. This aligns with the PR objectives.
259-259: LGTM! API calls consistently use location_id.All climate control API calls (
set_temperature,set_schedule_state,set_preset) have been correctly updated to useself._location_idinstead of the previousself._location. The changes are consistent throughout the file.Also applies to: 269-269, 293-293
113-113: ```bash
#!/bin/bash
echo "=== Locate climate.py ==="
fd --type f climate.py -x echo {}echo -e "\n=== Preview class definition and init ==="
rg -n -C3 'class .*Climate' custom_components/plugwise/climate.py
rg -n -C5 'def init' -C5 custom_components/plugwise/climate.pyecho -e "\n=== Check for self._location_id usage ==="
rg -n -C3 'self._location_id' custom_components/plugwise/climate.pyecho -e "\n=== Check for data mapping for location field ==="
rg -n -C3 'data["location"]' custom_components/plugwise/climate.py</blockquote></details> <details> <summary>tests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.json (3)</summary><blockquote> `72-73`: **Thermostat sensors switched to single setpoint: looks good** Consolidating to sensors.setpoint for the thermostat matches the zone’s hi/low split. Please confirm no code still expects setpoint_low/high on the thermostat device. --- `81-82`: **Adding vendor is correct** Manufacturer info on the thermostat device improves device_info; no issues spotted. --- `83-109`: **New ThermoZone device schema looks coherent; confirm dual 'thermostat' vs 'thermostats' usage** The zone object (dev_class: climate) with active_preset, schedules, control_state=cooling, and sensors setpoint_high/low/temperature aligns with the PR goals. It also includes: - thermostat: bounds + setpoints - thermostats: mapping to primary thermostat device If both keys are required by plugwise 1.8.0a0, all good. Please confirm both are expected in the library response and by our parser. </blockquote></details> <details> <summary>tests/components/plugwise/snapshots/test_sensor.ambr (6)</summary><blockquote> `4266-4318`: **Anna illuminance snapshot entries align with thermostat sensors** Device class, unit (lx), and unique_id mapping to the thermostat device are consistent. --- `4320-4373`: **Anna setpoint entries match thermostat’s single setpoint** Correct temperature device_class, translation_key=setpoint, precision=1, and state=20.5 as per fixture. --- `4375-4429`: **Anna temperature snapshot is consistent** Unique_id and state align with thermostat temperature; duplication with zone temperature is expected if both are exposed. Please confirm exposing both thermostat and zone temperatures is intentional to avoid UX confusion. --- `4431-4485`: **Living room cooling setpoint mapping looks correct** Uses translation_key=cooling_setpoint, unique_id -> setpoint_high, precision=1, state=30.0; consistent with zone data. Ensure translation keys cooling_setpoint exist in the integration constants/translations for UI strings. --- `4487-4541`: **Living room heating setpoint mapping looks correct** Uses translation_key=heating_setpoint, unique_id -> setpoint_low, precision=1, state=20.5; consistent with zone data. Similarly, verify heating_setpoint translations/keys are present. --- `4543-4597`: **Living room temperature snapshot aligns with zone device** Unique_id ties to the ThermoZone device, correct temperature device_class and unit. Value change vs cooling fixture is expected across fixtures. Confirm corresponding fixture for anna_heatpump_heating provides this value to keep snapshots stable. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| "requirements": [ | ||
| "https://test-files.pythonhosted.org/packages/e7/1c/fd68b64e588cae5d1426693e24b01ff6076d3510b1e361214bd84d990c28/plugwise-1.8.0a0.tar.gz#plugwise==1.8.0a0" |
There was a problem hiding this comment.
Do not use test-files.pythonhosted.org URLs in production.
The test-files.pythonhosted.org domain is PyPI's test environment and is not intended for production dependencies. Using this URL can lead to:
- Installation failures if the test file is removed
- Security concerns as test files may not undergo the same vetting
- Unpredictable availability
Either:
- Wait for the official plugwise 1.8.0 release on PyPI and use the standard version string
- If testing is required, use a git+https URL to the source repository instead
Example for option 2:
- "requirements": [
- "https://test-files.pythonhosted.org/packages/e7/1c/fd68b64e588cae5d1426693e24b01ff6076d3510b1e361214bd84d990c28/plugwise-1.8.0a0.tar.gz#plugwise==1.8.0a0"
- ],
+ "requirements": [
+ "git+https://github.com/plugwise/python-plugwise.git@v1.8.0a0#plugwise==1.8.0a0"
+ ],📝 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.
| "requirements": [ | |
| "https://test-files.pythonhosted.org/packages/e7/1c/fd68b64e588cae5d1426693e24b01ff6076d3510b1e361214bd84d990c28/plugwise-1.8.0a0.tar.gz#plugwise==1.8.0a0" | |
| "requirements": [ | |
| "git+https://github.com/plugwise/python-plugwise.git@v1.8.0a0#plugwise==1.8.0a0" | |
| ], |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)custom_components/plugwise/manifest.json(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/plugwise/manifest.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
9-9: No empty links
(MD042, no-empty-links)
⏰ 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). (3)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Prepare and validate pre-commit
CHANGELOG.md
Outdated
|
|
||
| - Fix mypy errors in Core (not accepted in HA Core) | ||
| - Introduce Snapshot testing for all platforms (requirement from HA Core) | ||
| - Add a ThermoZone to Anna via plugwise [v1.8.0]() |
There was a problem hiding this comment.
Fill in the plugwise release URL.
[v1.8.0]() is an empty Markdown link and is flagged by markdownlint (MD042). Point it to the actual plugwise v1.8.0 release (or drop the brackets) so lint passes and the changelog stays navigable.
As per static analysis hints
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: No empty links
(MD042, no-empty-links)
🤖 Prompt for AI Agents
In CHANGELOG.md around line 9, the markdown entry uses an empty link
`[v1.8.0]()`, which fails markdownlint MD042; replace the empty link by pointing
it to the actual Plugwise v1.8.0 release URL (e.g.
https://github.com/plugwise/plugwise/releases/tag/v1.8.0) or remove the link
brackets so the version is plain text, keeping the rest of the changelog line
unchanged.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }) | ||
| # --- | ||
| # name: test_anna_2_climate_snapshot[platforms0-True-m_anna_heatpump_cooling][climate.anna-entry] | ||
| # name: test_anna_2_climate_snapshot[platforms0-True-m_anna_heatpump_cooling][climate.living_room-entry] |
There was a problem hiding this comment.
This would be a huge red flag/breaking change - we change the entity_id AND the unique_id
| CLIMATE_DOMAIN, | ||
| SERVICE_SET_HVAC_MODE, | ||
| {ATTR_ENTITY_ID: "climate.anna", ATTR_HVAC_MODE: HVACMode.AUTO}, | ||
| {ATTR_ENTITY_ID: "climate.living_room", ATTR_HVAC_MODE: HVACMode.AUTO}, |
There was a problem hiding this comment.
Same breaking change applies here
bouwew
force-pushed
the
anna_thermozone
branch
from
f5ac623 to
ebaee5f
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)
custom_components/plugwise/climate.py (1)
159-159: Critical: Unique ID change requires migration logic.The unique_id now uses the ThermoZone device_id instead of the thermostat device_id, which breaks existing installations. This change was flagged in past review comments as "a huge red flag/breaking change."
For a production release, you need:
- Migration logic to preserve the old unique_id OR
- A migration path that allows Home Assistant to recognize the entity as the same (using
previous_unique_idin entity registry) OR- Clear documentation that users will lose their entity history and need to reconfigure
Since this PR is marked as DRAFT, this might be intentional for testing, but it must be addressed before merging.
Based on learnings (from past review comment).
tests/components/plugwise/snapshots/test_sensor.ambr (1)
4544-4581: Remove entity_category from ATTR_TEMPERATURE sensor to expose as primaryThe ATTR_TEMPERATURE sensor (sensor.py line 123-128) is currently marked with entity_category=EntityCategory.DIAGNOSTIC, which incorrectly hides the room's primary ambient temperature reading from the main sensor view. Other temperature readings like OUTDOOR_TEMP correctly have no entity_category, exposing them as primary. Since ATTR_TEMPERATURE represents the current room temperature used by climate entities (not an internal setpoint or diagnostic), it should follow the same pattern: remove the entity_category line entirely so it appears as a primary sensor.
This change also applies to the second snapshot entry at lines 4584-4597.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)custom_components/plugwise/climate.py(5 hunks)tests/components/plugwise/fixtures/anna_heatpump_heating/data.json(2 hunks)tests/components/plugwise/fixtures/anna_v4/data.json(1 hunks)tests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.json(1 hunks)tests/components/plugwise/fixtures/m_anna_heatpump_idle/data.json(1 hunks)tests/components/plugwise/snapshots/test_climate.ambr(10 hunks)tests/components/plugwise/snapshots/test_sensor.ambr(9 hunks)tests/components/plugwise/test_climate.py(5 hunks)tests/components/plugwise/test_select.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/plugwise/test_climate.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (1)
device(91-93)
⏰ 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: Prepare and validate pre-commit
🔇 Additional comments (13)
tests/components/plugwise/fixtures/anna_heatpump_heating/data.json (1)
83-110: Verify the temperature sensor values are intentionally different.The new ThermoZone device shows
temperature: 23.3(line 96) while the Anna thermostat device showstemperature: 19.3(line 73). This 4°C difference should be verified:
- Is this intentional test data representing zone vs. local thermostat readings?
- Should the zone temperature be derived from or match the thermostat temperature?
- Is this inconsistency present in the actual backend implementation?
Please verify the temperature handling logic and confirm whether having different temperature readings for the zone and thermostat is the expected behavior.
custom_components/plugwise/climate.py (2)
79-79: LGTM - Simplified entity creation logic.The change from gateway-based filtering to a direct
dev_class == "climate"check is cleaner and aligns with the new ThermoZone architecture. This makes the code more maintainable and extensible.
152-152: Verify that ThermoZone device_id is compatible with Plugwise Smile API methods.The change from
self._location(previously derived from device LOCATION data) toself._location_id = device_id(ThermoZone device identifier) is a material API contract change. The Plugwise Smile API documentation suggests location_id and zone_id/device_id are used interchangeably, but web sources do not explicitly confirm that the ThermoZone device_id format is accepted byset_temperature,set_schedule_state, andset_presetmethods.Confirm this works by:
- Running integration tests with climate control operations (set temperature, change schedule state, switch presets)
- Verifying no API errors occur when these methods receive the ThermoZone device_id
tests/components/plugwise/fixtures/m_anna_heatpump_idle/data.json (1)
69-69: Verify the cooling_activation_outdoor_temperature discrepancy.The
cooling_activation_outdoor_temperaturediffers between the Anna thermostat device (21.0 at line 69) and the ThermoZone device (25.0 at line 94). This 4°C difference should be verified:
- Is this intentional to represent different thresholds for the physical device vs. the zone?
- Should these values be synchronized?
- Does the backend implementation handle this correctly?
Please verify whether having different cooling activation thresholds is the expected behavior or if this is test data inconsistency.
Also applies to: 94-94
tests/components/plugwise/fixtures/anna_v4/data.json (1)
76-100: Fixture structure is consistent with heat-only configuration.The anna_v4 fixture correctly uses a single setpoint (line 92) instead of setpoint_high/low, which aligns with a heating-only configuration where HVAC_MODE.HEAT_COOL is not applicable. The ThermoZone device structure is consistent with the other fixtures.
tests/components/plugwise/test_select.py (1)
123-123: Entity ID change aligns with the broader architectural shift.The change from
select.anna_thermostat_scheduletoselect.living_room_thermostat_scheduleis consistent with the climate entity renaming throughout this PR. This is part of the breaking changes that need to be clearly documented for users.tests/components/plugwise/snapshots/test_climate.ambr (1)
578-825: Snapshots confirm the breaking entity ID and unique ID changes.These snapshot updates demonstrate the breaking changes:
entity_idchanges fromclimate.annatoclimate.living_roomunique_idchanges to use the ThermoZone device ID (c784ee9fdab44e1395b8dee7d7a497d5-climate)friendly_namechanges from "Anna" to "Living room"As noted in the past review comment, these are "huge red flag/breaking changes" that will impact existing users. The snapshots are correct reflections of the new implementation, but the breaking nature must be addressed through:
- Migration logic (preferred) or
- Clear upgrade documentation with manual steps
Based on learnings (from past review comment).
tests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.json (1)
73-73: Verify the 7°C temperature discrepancy between thermostat and zone.The cooling fixture shows a significant temperature difference:
- Anna thermostat: 19.3°C (line 73)
- Living room zone: 26.3°C (line 96)
This 7°C difference is more pronounced than in other fixtures (~4°C). Please verify:
- Is this intentional test data for cooling scenarios?
- Could this be a copy-paste error from another fixture?
- Does the backend correctly handle such large discrepancies?
- Should the Anna temperature be updated to reflect a warm reading consistent with the cooling state?
Also applies to: 96-96
tests/components/plugwise/snapshots/test_sensor.ambr (5)
4376-4413: Snapshot entries are consistent; remove unclear verification requestsThe
sensor.anna_temperatureentries in the snapshot appear stable with no ID conflicts. Since this is an auto-generated snapshot file, focus on verifying that the underlying sensor implementation correctly generates these identifiers—don't manually audit snapshot structure.
4266-4301: anna_illuminance is intentionally present and correctVerification confirms the illuminance sensor is actively defined in the integration (
custom_components/plugwise/sensor.pylines 405-411) as a standard sensor description with DIAGNOSTIC categorization. It's not device-specific filtering—it's part of the base sensor list. The snapshot correctly reflects this behavior. No changes needed.
4432-4469: No action required; review comment is based on incorrect assumptions about sensor scope.The sensors are not duplicates.
sensor.anna_setpoint(key="setpoint") andsensor.living_room_cooling_setpoint(key="setpoint_high") are intentionally distinct:
anna_setpoint(unique_id ending in-setpoint) is the gateway-level main setpoint for device3cb70739...living_room_cooling_setpoint(unique_id ending in-setpoint_high) is the zone-level cooling setpoint for devicec784ee9f...They are from different devices with different purposes, not duplicate surfaces of the same data. No gating or hiding is necessary.
Likely an incorrect or invalid review comment.
4488-4525: Heating setpoint wiring is correct; no migrations or visibility concerns foundThe snapshot entries are properly configured: heating_setpoint correctly uses translation_key='heating_setpoint' with unique_id ending in '-setpoint_low', and cooling_setpoint uses translation_key='cooling_setpoint' with unique_id ending in '-setpoint_high'.
Verification shows:
- No migration mappings exist for setpoint entities in
async_migrate_entity_entry, indicating they were always named with current patterns- anna_setpoint entity does not exist in the codebase
- No ThermoZone visibility hiding logic found
4320-4357: Verify migration coverage for anna_setpoint entitiesThe entity registry migration infrastructure exists and handles specific entity type migrations (slave_boiler_state → secondary_boiler_state, relative_humidity → humidity, plug → relay). However, no migration rules are defined for setpoint-related entities, including anna_setpoint.
Current investigation found:
async_migrate_entity_entry()andasync_migrate_sensor_entities()have no handling for "setpoint", "setpoint_high", or "setpoint_low" keys- No references to old entity IDs like "anna_cooling_setpoint" or "anna_heating_setpoint" in the codebase
- The snapshot shows anna_setpoint with current unique_id format (
{id}-setpoint)To resolve this concern: Confirm whether anna had separate cooling/heating setpoint entities exposed in previous releases. If yes, migration rules must be added to
async_migrate_entity_entry()to map old IDs to the new consolidated structure. If no, the concern can be dismissed.
| ## Ongoing | ||
|
|
||
| - Add a ThermoZone to Anna | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the breaking changes and provide migration guidance.
This change introduces breaking changes for Anna users (entity_id and unique_id changes from anna to living_room, as evidenced by the test snapshots). The changelog should:
- Clearly mark this as a BREAKING change
- Explain that existing
climate.annaentities will becomeclimate.living_room - Provide migration steps for users (automations, dashboards, etc. will need updates)
- Explain the new ThermoZone concept and architecture
Consider adding something like:
## Ongoing
-- Add a ThermoZone to Anna
+- **BREAKING**: Add a ThermoZone to Anna - climate entities are now zone-based
+ - Climate entity IDs change from `climate.anna` to `climate.living_room` (or the zone name)
+ - Users will need to update automations, dashboards, and scripts that reference the old entity IDs
+ - The Anna device now represents the physical thermostat, while a new ThermoZone device represents the climate control zone🤖 Prompt for AI Agents
In CHANGELOG.md around lines 5 to 8, mark the addition of ThermoZone as a
BREAKING change and expand the entry to document what changed and how to
migrate: state that entity_id and unique_id for Anna changed from anna to
living_room (e.g., climate.anna -> climate.living_room), add clear migration
steps for users to update automations, dashboards, and YAML/config references,
summarize the new ThermoZone concept and architecture at a high level, and
include any notes about compatibility or scripts/commands to help bulk-rename or
map old IDs to new ones.
|
Closing this, the P1 data on the Anna P1 is shown in the Home location, not in the Livingroom location. |
Summary by CodeRabbit
Release Notes
New Features
Changes