ESPHome DHW active fix#139
Conversation
WalkthroughUpdates add two Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)custom_components/sat/esphome/__init__.py (2)
🔇 Additional comments (3)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/sat/esphome/__init__.py (1)
1-1: Remove duplicateannotationsimport.The
annotationsfeature is imported twice in the same statement, which is redundant.🔧 Proposed fix
-from __future__ import annotations, annotations +from __future__ import annotations
🤖 Fix all issues with AI agents
In @custom_components/sat/esphome/__init__.py:
- Line 144: The code is tracking DATA_DHW_ACTIVE using switch.DOMAIN but
hot_water_active reads it from binary_sensor.DOMAIN; update the entity tracking
call that references DATA_DHW_ACTIVE (the call to self._get_entity_id) to use
binary_sensor.DOMAIN instead of switch.DOMAIN so the coordinator listens for
binary_sensor state changes for dhw_active and matches the hot_water_active
property.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdcustom_components/sat/esphome/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/esphome/__init__.py (2)
custom_components/sat/coordinator.py (3)
get(546-561)DeviceState(30-32)_get_entity_id(564-565)custom_components/sat/serial/__init__.py (1)
get(156-158)
🔇 Additional comments (4)
custom_components/sat/esphome/__init__.py (2)
18-18: LGTM!The new constant
DATA_DHW_ACTIVEfollows the established naming convention and aligns with the PR's objective to add DHW active sensor tracking.
87-88: LGTM!The property now correctly reads DHW active status from a binary sensor instead of the DHW enable switch. This properly distinguishes between the user-controlled enable/disable switch and the actual DHW activity state reported by the boiler.
README.md (2)
118-122: LGTM!The addition of
initial_valueparameters for both DHW and central heating setpoints addresses the PR's objective to prevent undefined behavior on certain boiler models. The default values (45°C for DHW, 10°C for CH) are safe and reasonable starting points.
158-159: LGTM!The documentation correctly adds the
dhw_activebinary sensor to the ESPHome configuration example, enabling users to expose the DHW active status to the SAT component.
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)
custom_components/sat/esphome/__init__.py (1)
1-1: Critical syntax error: duplicate import.The
annotationsmodule is imported twice in the same statement. This will cause a syntax error.🐛 Proposed fix
-from __future__ import annotations, annotations +from __future__ import annotations
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/esphome/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/esphome/__init__.py (2)
custom_components/sat/serial/__init__.py (1)
get(156-158)custom_components/sat/coordinator.py (3)
get(546-561)DeviceState(30-32)_get_entity_id(564-565)
🔇 Additional comments (3)
custom_components/sat/esphome/__init__.py (3)
18-18: LGTM! New constant follows naming conventions.The new
DATA_DHW_ACTIVEconstant is well-named and consistent with other data constants in the file.
87-88: Excellent fix! Correctly distinguishes DHW state from DHW command.The change from
DATA_DHW_ENABLE(user control switch) toDATA_DHW_ACTIVE(actual boiler state binary sensor) properly detects when the boiler is actively producing domestic hot water, which aligns with the PR objective to detect when the boiler is "stuck on" for DHW production.
143-144: LGTM! Entity tracking correctly aligned with property domains.Both changes are correct:
- Line 143 fixes a pre-existing bug:
flame_activeproperty (line 84) reads frombinary_sensor.DOMAIN, so tracking must use the same domain to receive state change events.- Line 144 correctly tracks
DATA_DHW_ACTIVEfrombinary_sensor.DOMAIN, matching thehot_water_activeproperty (line 88).This resolves the critical issue flagged in the previous review.
|
Thanks! |
2 participants
Currently, ESPHome-based OpenTherm gateways' data on DHW usage is not working correctly;
dhw_onis a toggle switch for DHW, but not thebinary_sensor.I've added
dhw_activesensor for example ESPHome configuration and changed data retrieval logic in ESPHome part of the component so that it could understand that the boiler is "stuck on" due to making domestic hot water.I've also added initial values for ESPHome configuration for
t_set(both CH and DHW), as some boilers (like Baxi with Bertelli main board) exhibit undefined behavior if they receive zeroes during writing of these parameters, like heating water until bimetallic failsafe thermostat triggers at 115°C and stops gas flow in almost-a-disaster-scenario.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.