Fix: Recalculate PID error when thermostat turns off#172
Open
DeRRudi77 wants to merge 2 commits intoAlexwijn:developfrom
Open
Fix: Recalculate PID error when thermostat turns off#172DeRRudi77 wants to merge 2 commits intoAlexwijn:developfrom
DeRRudi77 wants to merge 2 commits intoAlexwijn:developfrom
Conversation
When a connected thermostat or radiator changed to OFF, the PID error value stayed stale until the next temperature change. Three fixes: - Filter HVACMode.OFF in Area.state so OFF rooms are excluded from error, weight, and heating curve calculations - Trigger _async_control_pid(True) in _async_thermostat_changed on state changes (separate from temperature changes) - Trigger _async_control_pid(True) in _async_main_climate_changed on state changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughArea treats climate HVACMode.OFF as unavailable; thermostat/radiator change handlers now separate HVAC-mode changes from temperature-only updates, calling _async_control_pid(True) and scheduling the control loop only on HVAC-mode transitions; new tests validate Area behavior and PID-triggering logic. Changes
Sequence Diagram(s)sequenceDiagram
participant HA as HomeAssistant
participant Sat as SatClimate
participant PID as PIDController
HA->>Sat: state change (new_state)
alt HVAC-mode changed
Sat->>Sat: _async_control_pid(True)
Sat->>Sat: schedule_control_heating_loop()
else Temperature-only changed
Sat->>Sat: async_set_target_temperature(..., cascade=False)
end
Sat->>PID: control loop runs (scheduled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_area.py (1)
22-30: Mock returns the same state for anyentity_idlookup.
hass.states.get.return_value = statereturns the sameStateobject regardless of which entity is requested. This is fine for the current tests sinceArea.current_temperature'sSENSOR_TEMPERATURE_IDoverride path isn't exercised, but if a future test adds asensor_temperature_idattribute, the sensor lookup atarea.pyLine 58 will return the climate state instead of a sensor state, producing misleading results. Consider usingside_effectkeyed byentity_idfor forward-compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_area.py` around lines 22 - 30, The mock currently sets hass.states.get.return_value = state so every entity_id returns the same State; change mock_hass_with_climate_state to set hass.states.get.side_effect that checks the incoming entity_id and returns the created State only for the climate entity_id (and None or a sensible default for others) so future lookups (e.g. Area.current_temperature's SENSOR_TEMPERATURE_ID path) will not receive the climate State; locate mock_hass_with_climate_state and replace the return_value usage on hass.states.get with a side_effect closure or function that keys off the entity_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/sat/climate.py`:
- Around line 613-620: Detect and handle the edge cases around temperature-only
updates and duplicate PID resets: introduce a local flag (e.g.,
pid_reset_performed) in the state change handler so when the state branch calls
self._async_control_pid(True) and schedule_control_heating_loop() you set
pid_reset_performed=True; in the temperature branch, if the new temperature
equals self._target_temperature (so async_set_target_temperature would
early-return) explicitly call self._async_control_pid(True) and
self.schedule_control_heating_loop() only if pid_reset_performed is False
(otherwise skip to avoid a duplicate reset), and if the temperature differs,
call await self.async_set_target_temperature(...) as before. This references the
symbols async_set_target_temperature, _async_control_pid,
schedule_control_heating_loop, and self._target_temperature so you can locate
and update the logic.
In `@tests/test_climate.py`:
- Line 212: The current assertion using "assert climate.pid.last_error !=
initial_error or climate.pid.last_error == climate.error" is weak and can pass
trivially; change the test to directly assert that the PID reset handler ran
after the state change by patching or spying on climate._async_control_pid and
asserting it was called with reset=True (or alternatively assert a deterministic
side-effect such as pwm.reset() being called or climate._calculated_setpoint is
None). Locate references to climate.pid.last_error, initial_error, climate.error
and replace the weak check with an explicit mock/assert on _async_control_pid
(or the chosen side-effect) to ensure the handler invocation is tested reliably.
---
Nitpick comments:
In `@tests/test_area.py`:
- Around line 22-30: The mock currently sets hass.states.get.return_value =
state so every entity_id returns the same State; change
mock_hass_with_climate_state to set hass.states.get.side_effect that checks the
incoming entity_id and returns the created State only for the climate entity_id
(and None or a sensible default for others) so future lookups (e.g.
Area.current_temperature's SENSOR_TEMPERATURE_ID path) will not receive the
climate State; locate mock_hass_with_climate_state and replace the return_value
usage on hass.states.get with a side_effect closure or function that keys off
the entity_id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 247526f5-d100-4c07-8b91-f1a335dc12b0
📒 Files selected for processing (4)
custom_components/sat/area.pycustom_components/sat/climate.pytests/test_area.pytests/test_climate.py
Use patch.object spy instead of weak error comparison to directly verify _async_control_pid(True) is called on state change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When using SAT together with Better Thermostat (or similar climate entities), turning off the thermostat does not trigger a recalculation of the PID error value. The error stays stale until the next temperature setpoint change, causing SAT to keep heating based on outdated error values.
This affects all three ways a thermostat can be configured in SAT:
CONF_THERMOSTAT)CONF_RADIATORS)CONF_ROOMS)Root Cause
Three code paths were not properly handling HVAC mode changes to OFF:
_async_thermostat_changed— When the connected thermostat changed state (e.g. to OFF) without changing the target temperature, the handler calledasync_set_target_temperature()which early-returned because the temperature hadn't changed. PID was never recalculated._async_main_climate_changed— When a main climate/radiator changed state, the handler only calledschedule_control_heating_loop()but never_async_control_pid(). PID was never recalculated.Area.state— When a room climate turned OFF,_async_climate_changedcorrectly called_async_control_pid(True), butArea.statedid not filterHVACMode.OFF— onlySTATE_UNKNOWNandSTATE_UNAVAILABLE. So the OFF room still contributed its error value tomax_error, making the recalculation ineffective.Fix
area.py: FilterHVACMode.OFFinArea.stateso that OFF climates returnNonefor state, target_temperature, current_temperature, error, and weight — fully excluding them from all calculations.climate.py: In_async_thermostat_changed, separate state changes from temperature changes. On state change, call_async_control_pid(True)directly. On temperature change, callasync_set_target_temperatureas before.climate.py: In_async_main_climate_changed, add_async_control_pid(True)when the main climate state changes.Tests
tests/test_area.pywith 13 unit tests coveringArea.state,error,weight, and temperature properties for OFF, HEAT, UNKNOWN, and UNAVAILABLE states.tests/test_climate.pyverifying PID recalculation when the connected thermostat or radiator turns OFF.Summary by CodeRabbit
Bug Fixes
Tests