Refactor heating control to cycle-centric architecture#124
Refactor heating control to cycle-centric architecture#124
Conversation
… improved multiple states determination
WalkthroughPer-entry SatConfig/SatEntryData and SatDataUpdateCoordinatorFactory introduced; flame-based model replaced by cycle-driven subsystems (CycleTracker/CycleHistory); persistent PID/PWM/Boiler and DynamicMinimumSetpoint added; entities and transports rewired to use entry_data; several legacy modules and PWM service removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sensor as Device/Sensor
participant Coordinator as SatDataUpdateCoordinator
participant Tracker as CycleTracker
participant History as CycleHistory
participant HA as Home Assistant
Note over Sensor,Tracker: Cycle detection & publication
Sensor->>Coordinator: async_update_control(ControlLoopSample)
Coordinator->>Tracker: update(ControlLoopSample)
activate Tracker
Tracker->>Tracker: detect ON/OFF transitions
alt ON -> OFF
Tracker->>Tracker: build Cycle, compute metrics (p50/p90)
Tracker->>History: record_cycle(Cycle)
Tracker->>HA: fire EVENT_SAT_CYCLE_ENDED (payload)
else OFF -> ON
Tracker->>Tracker: mark started_since, accumulate samples
Tracker->>HA: fire EVENT_SAT_CYCLE_STARTED
end
deactivate Tracker
sequenceDiagram
autonumber
participant Climate as SatClimate
participant Areas as Areas
participant PID as PID
participant HC as HeatingCurve
participant Coordinator as SatDataUpdateCoordinator
participant Boiler as Boiler
Note over Climate,Boiler: Control loop compute & device update
Climate->>Areas: areas.errors()
Areas-->>Climate: TemperatureStates/errors
Climate->>HC: value()
Climate->>PID: update()
PID-->>Climate: output (setpoint adjustment)
Climate->>Coordinator: set_control_intent(intent)
Coordinator->>Boiler: update(device_state, last_cycle)
activate Boiler
Boiler->>Boiler: determine status & modulation_reliability
Boiler-->>Coordinator: status, modulation_reliable
deactivate Boiler
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
custom_components/sat/coordinator.py (1)
260-268:minimum_setpointassumes presence ofCONF_MINIMUM_SETPOINTin config
minimum_setpointcurrently does:return max(float(self._config_data.get(CONF_MINIMUM_SETPOINT)), MINIMUM_SETPOINT)If
CONF_MINIMUM_SETPOINTis missing orNoneinconfig_data(e.g. due to a migrated or manually edited entry),float(None)will raise aTypeErrorand break the coordinator.Safer pattern:
raw = self._config_data.get(CONF_MINIMUM_SETPOINT) try: configured = float(raw) if raw is not None else None except (TypeError, ValueError): configured = None if configured is None: return MINIMUM_SETPOINT return max(configured, MINIMUM_SETPOINT)You can keep this concise, but some validation/fallback around the
float()cast would make the integration more robust against bad or legacy configs.custom_components/sat/climate.py (1)
868-944: PID control now hard-requires outside temperature; ensure that’s acceptable
async_control_pidnow returns early with a warning whencurrent_outside_temperatureisNone:if self.current_outside_temperature is None: _LOGGER.warning("Current outside temperature is not available. Skipping PID control.") returnPreviously, PID might have continued with a default or stale curve. This change is reasonable, but it means:
- Misconfigured or temporarily unavailable outside sensors will completely block PID updates (and therefore setpoint recalculation).
async_control_heating_loopalso depends onself.current_outside_temperatureandself.heating_curve.value, so both loops will effectively stall until the sensor recovers.If that’s the desired fail‑safe, this is fine; otherwise, you may want to fall back to a default heating curve or cached outside temperature rather than skipping PID entirely.
🧹 Nitpick comments (15)
custom_components/sat/cycles.py (1)
228-228: Update type hint to explicit Optional.The
timestampparameter should use explicitOptionalorT | Nonesyntax per PEP 484.Apply this diff:
- def update(self, boiler_state: BoilerState, timestamp: float = None) -> None: + def update(self, boiler_state: BoilerState, timestamp: Optional[float] = None) -> None:tests/test_climate.py (1)
4-61: Tests correctly exercise new async API and state shape; consider float-comparison robustnessThe updated tests align with the new control flow (
async_control_pid(reset=True)followed byasync_control_heating_loop()) and with the new state structure (max_error.value,heating_curve.value,pwm.state.duty_cycle,pwm.state.last_duty_cycle_percentage,requested_setpoint). Usingtemplate.DOMAIN/sensor.DOMAINin the parametrized config is also consistent with Home Assistant conventions.Given how many assertions use exact float equality (e.g., duty‑cycle percentages and heating‑curve values), you may want to switch those to
pytest.approxto make the tests resilient to harmless rounding changes in the PID/curve math; otherwise, the current expectations look coherent with the refactored API.Also applies to: 63-111, 113-160
custom_components/sat/heating_curve.py (1)
4-4: Optional return types now correctly reflect nullable stateSwitching the three properties to
Optional[float]matches how the internal fields are initialized and used (reset toNoneuntil autotune/update runs). This is a straightforward typing/style improvement with no behavioral change.If you’re tightening typing further elsewhere, you might also annotate the backing attributes (
_optimal_coefficient,_coefficient_derivative,_last_heating_curve_value) asOptional[float]for consistency, but that’s purely a nicety.Also applies to: 83-85, 87-89, 91-92
custom_components/sat/pid.py (1)
3-8: Tighten imports from.constand drop unused local constantRuff’s F405 warning about
HEATING_SYSTEM_UNDERFLOORstems from the wildcard import; it also makes it harder to see which constants this module actually needs. Additionally,MAX_BOILER_TEMPERATURE_AGE = 300is no longer used now that boiler‑temperature tracking is gone.Consider:
- Replacing
from .const import *with explicit imports for the constants used here (e.g.,DEADBAND,HEATING_SYSTEM_UNDERFLOOR, and any others referenced below).- Removing the now‑unused
MAX_BOILER_TEMPERATURE_AGEdefinition.This will clear the linter warning and keep the module’s dependencies explicit.
Also applies to: 13-15, 31-46, 207-215
custom_components/sat/sensor.py (1)
13-14: Cycle and boiler sensors align with new enums; verify coordinator defaults and avoid star importThe new
SatCycleSensorand the adjustedSatBoilerSensor.availablelogic look consistent with the cycle‑centric model:
SatCycleSensor.native_valuereturnsCycleClassification.INSUFFICIENT_DATA.namewhen there is nolast_cycle, otherwise the classification name, which matches the new enum.SatBoilerSensor.availabletreatingBoilerStatus.INSUFFICIENT_DATAas “unavailable” is reasonable as long asdevice_statusis always aBoilerStatusmember (neverNone).Two follow‑ups worth considering:
- Coordinator defaults: Double‑check that
self._coordinator.device_statusis initialized toBoilerStatus.INSUFFICIENT_DATA(or anotherBoilerStatusmember) and neverNone, sonative_value’sdevice_status.nameaccess is always safe.- Imports:
from .const import *is triggering Ruff F403/F405 and hides which names come fromconst. Prefer explicitly importing the constants and enums used in this file (DOMAIN,CLIMATE,COORDINATOR,CONF_MODE,MODE_SERIAL,MODE_SIMULATOR,CONF_MINIMUM_CONSUMPTION,CONF_MAXIMUM_CONSUMPTION,CONF_NAME,CycleClassification,BoilerStatus, etc.) to improve clarity and keep linters happy.Also applies to: 44-50, 254-269, 271-283
custom_components/sat/minimum_setpoint.py (3)
88-105: Guard on deviation from recent base can be simplifiedThe
valueproperty’s extra safeguard (Lines 98–102) can be expressed more simply and readably:if self._last_base_setpoint is not None: allowed_minimum = max( self._config.minimum_setpoint, self._last_base_setpoint - self._config.max_deviation_from_recent_base, ) minimum_setpoint = max(minimum_setpoint, allowed_minimum)This avoids the nested
if minimum_setpoint < allowed_minimumwhile preserving behavior.
203-214: Unnecessaryint()aroundround()when computing the regime bucket
round(base_setpoint / width)already returns anintin Python 3 when called withoutndigits, so the outerint(...)is redundant:- bucket = int(round(base_setpoint / width)) + bucket = round(base_setpoint / width)This addresses the Ruff
RUF046hint without changing semantics.
20-64: Config / helper fields appear unused; consider pruning or wiring through
MinimumSetpointConfig.target_min_on_time_secondsandminimum_relax_factor_when_inactive, and the helper_relax_minimum_when_uncertain, do not appear to be used anywhere in this module.Dead configuration fields and helpers make the behavior harder to reason about and invite confusion over which relax paths are actually active (the tuning logic currently only uses
_relax_toward_basewithminimum_relax_factor_when_untunable/minimum_relax_factor_when_uncertain).Either:
- Wire these into the tuning logic where they were intended to apply, or
- Remove them to keep the configuration surface and code path minimal.
Given this is performance/maintainability, it can be deferred but is worth cleaning up.
Also applies to: 352-366
custom_components/sat/pwm.py (2)
91-101: Use explicitis Nonecheck for heating curve availabilityIn
updateyou currently guard with:if not self._heating_curve.value or requested_setpoint is None or boiler_state.flow_temperature is None:This treats a zero heating curve value as “missing”, which is unlikely but obscures intent. A clearer and safer version would be:
if self._heating_curve.value is None or requested_setpoint is None or boiler_state.flow_temperature is None: ...This avoids conflating falsy numeric values with absence and makes the precondition clearer.
239-244: Clarify units forlast_duty_cycle_percentageinPWMState
PWM.statecurrently exposes:return PWMState( duty_cycle=self._duty_cycle, last_duty_cycle_percentage=round(self._last_duty_cycle_percentage * 100, 2) if self._last_duty_cycle_percentage is not None else None, )So the exposed value is 0–100 (%) while the internal
_last_duty_cycle_percentageis 0–1. This is fine, but make sure all consumers (sensors, UI, tests) expect a percentage, not a fraction. If the old API provided a fraction, this is a breaking change and should be documented accordingly.custom_components/sat/coordinator.py (2)
13-16: Star import from.constobscures which symbols are required
from .const import *together with use ofCONF_MINIMUM_SETPOINT,MINIMUM_SETPOINT, and others makes it harder to track dependencies and triggers linter warnings (Ruff F403/F405).Consider explicitly importing only the names you need in this module (e.g.,
BoilerStatus,CONF_MINIMUM_SETPOINT,MINIMUM_SETPOINT, mode constants). This will:
- Make static analysis more accurate.
- Avoid accidental name collisions.
- Document dependencies more clearly.
Also applies to: 262-262
330-344: Optional: document unusedclimateargument forasync_control_heating_loop
async_control_heating_loopstill takes aclimate: Optional[SatClimate] = Noneargument but no longer uses it. If this is kept only for backwards compatibility with existing callers, a short comment explaining that it is intentionally unused would prevent future cleanups from accidentally removing a still‑relied‑upon parameter.If compatibility is not required, you could drop the argument and update call sites instead.
custom_components/sat/boiler.py (1)
129-205: Consider splitting_derive_status/_modulation_directionto reduce complexityBoth
_derive_statusand_modulation_directionhave many branches and early returns, which is natural for a state machine but does trip Pylint’s R0911/R0912 warnings.If this becomes hard to maintain, you might consider:
- Extracting small predicates for the flame‑off phases (e.g.
_status_flame_off(state, previous, now)) vs flame‑on phases.- Moving the modulation‑based vs gradient‑based direction logic into separate helpers, or at least documenting expectations for
_modulation_reliablevs unreliable cases.Not urgent, but will make future behavior changes less risky.
Also applies to: 318-350
custom_components/sat/climate.py (2)
195-199: Attribute vs control path for minimum setpoint may diverge in static modeIn
extra_state_attributesyou expose:"minimum_setpoint": self.minimum_setpoint.value,while the control path uses:
@property def minimum_setpoint_value(self) -> float: if self._dynamic_minimum_setpoint: return self.minimum_setpoint.value return self._coordinator.minimum_setpointSo when
_dynamic_minimum_setpointisFalse:
- PWM/control logic uses the coordinator’s static
minimum_setpoint.- The attribute
minimum_setpointstill reports the dynamic controller’s learned value (which you continue to update insideasync_control_heating_loop).If that divergence is intentional (e.g. surfacing experimental dynamic hints while not yet using them), it would help to document it. Otherwise, consider aligning the attribute with the actual control path:
"minimum_setpoint": self.minimum_setpoint_value,so the UI/state always reflects the value that will actually be applied.
Also applies to: 415-431, 627-634
963-1016: Dynamic minimum setpoint update is tied to PWM, but runs regardless of config flagInside
async_control_heating_loop:if self.pulse_width_modulation_enabled: self.pwm.enable() self.pwm.update(...) self.minimum_setpoint.update( cycles=self._coordinator.cycles, boiler_state=self._coordinator.state, last_cycle=self._coordinator.last_cycle, requested_setpoint=self._calculated_setpoint, ) else: self.pwm.disable()Notes:
- The dynamic minimum setpoint controller is updated only when PWM is enabled, which is sensible (tuning based on cycles where PWM is actually in play).
- However,
self.minimum_setpoint.update(...)is called regardless of_dynamic_minimum_setpoint; in purely static mode the dynamic controller’s state is still being learned but never used for control, only (currently) for theminimum_setpointattribute.This is not a correctness bug, but if dynamic minimum setpoint is expected to be fully disabled when the option is off, you might want to guard the update as well:
if self._dynamic_minimum_setpoint: self.minimum_setpoint.update(...)Otherwise, consider documenting that the dynamic learner always runs and is just not consulted for control when the option is off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
custom_components/sat/__init__.py(3 hunks)custom_components/sat/area.py(1 hunks)custom_components/sat/binary_sensor.py(3 hunks)custom_components/sat/boiler.py(2 hunks)custom_components/sat/climate.py(22 hunks)custom_components/sat/config_flow.py(1 hunks)custom_components/sat/const.py(1 hunks)custom_components/sat/coordinator.py(7 hunks)custom_components/sat/cycles.py(1 hunks)custom_components/sat/flame.py(0 hunks)custom_components/sat/heating_curve.py(2 hunks)custom_components/sat/minimum_setpoint.py(1 hunks)custom_components/sat/mqtt/__init__.py(1 hunks)custom_components/sat/pid.py(5 hunks)custom_components/sat/pwm.py(8 hunks)custom_components/sat/relative_modulation.py(2 hunks)custom_components/sat/sensor.py(4 hunks)custom_components/sat/simulator/__init__.py(2 hunks)custom_components/sat/util.py(3 hunks)tests/test_climate.py(8 hunks)
💤 Files with no reviewable changes (1)
- custom_components/sat/flame.py
🧰 Additional context used
🧬 Code graph analysis (13)
custom_components/sat/__init__.py (4)
custom_components/sat/serial/sensor.py (1)
async_setup_entry(86-105)tests/test_init.py (1)
test_setup_update_unload_entry(11-26)custom_components/sat/simulator/sensor.py (1)
async_setup_entry(11-19)custom_components/sat/number.py (1)
async_setup_entry(10-14)
custom_components/sat/relative_modulation.py (1)
custom_components/sat/const.py (1)
RelativeModulationState(222-226)
custom_components/sat/area.py (2)
custom_components/sat/heating_curve.py (2)
update(27-30)value(91-92)custom_components/sat/pid.py (1)
update(66-83)
custom_components/sat/pwm.py (3)
custom_components/sat/const.py (1)
PWMStatus(216-219)custom_components/sat/heating_curve.py (3)
HeatingCurve(11-92)update(27-30)value(91-92)custom_components/sat/boiler.py (2)
BoilerState(17-27)update(113-127)
custom_components/sat/util.py (5)
custom_components/sat/coordinator.py (4)
minimum_setpoint(260-262)get(407-422)maximum_setpoint(265-268)cycles(152-153)custom_components/sat/minimum_setpoint.py (2)
DynamicMinimumSetpoint(72-409)MinimumSetpointConfig(21-63)custom_components/sat/pwm.py (2)
PWM(35-244)PWMConfig(27-32)custom_components/sat/switch/__init__.py (1)
maximum_setpoint(40-41)custom_components/sat/helpers.py (1)
convert_time_str_to_seconds(19-28)
custom_components/sat/mqtt/__init__.py (6)
custom_components/sat/coordinator.py (1)
device_id(123-124)custom_components/sat/simulator/__init__.py (1)
device_id(31-32)custom_components/sat/fake/__init__.py (1)
device_id(29-30)custom_components/sat/serial/__init__.py (1)
device_id(38-39)custom_components/sat/switch/__init__.py (1)
device_id(28-29)custom_components/sat/esphome/__init__.py (1)
device_id(54-55)
custom_components/sat/sensor.py (3)
custom_components/sat/coordinator.py (1)
last_cycle(156-157)custom_components/sat/cycles.py (1)
last_cycle(149-150)custom_components/sat/const.py (1)
CycleClassification(184-190)
custom_components/sat/coordinator.py (2)
custom_components/sat/boiler.py (4)
BoilerState(17-27)Boiler(30-350)update(113-127)status(80-84)custom_components/sat/cycles.py (7)
CycleTracker(204-380)CycleHistory(72-201)CycleStatistics(64-69)Cycle(37-60)update(228-256)last_cycle(149-150)statistics(153-160)
custom_components/sat/pid.py (3)
custom_components/sat/area.py (3)
update(142-148)update(154-157)error(65-73)custom_components/sat/heating_curve.py (1)
update(27-30)custom_components/sat/errors.py (1)
Error(37-40)
custom_components/sat/minimum_setpoint.py (4)
custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/const.py (2)
CycleClassification(184-190)CycleKind(177-181)custom_components/sat/cycles.py (5)
CycleStatistics(64-69)Cycle(37-60)cycles_last_hour(109-115)duty_ratio_last_15m(118-131)sample_count_4h(100-106)custom_components/sat/helpers.py (1)
clamp(90-95)
custom_components/sat/cycles.py (5)
custom_components/sat/const.py (2)
CycleKind(177-181)CycleClassification(184-190)custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/coordinator.py (5)
last_cycle(156-157)flame_active(179-180)hot_water_active(187-188)setpoint(165-166)return_temperature(199-200)custom_components/sat/simulator/__init__.py (2)
flame_active(63-64)setpoint(51-52)
custom_components/sat/binary_sensor.py (3)
custom_components/sat/const.py (2)
BoilerStatus(193-213)CycleClassification(184-190)custom_components/sat/coordinator.py (2)
device_status(132-134)last_cycle(156-157)custom_components/sat/cycles.py (1)
last_cycle(149-150)
custom_components/sat/climate.py (3)
custom_components/sat/const.py (2)
RelativeModulationState(222-226)PWMStatus(216-219)custom_components/sat/util.py (1)
create_dynamic_minimum_setpoint_controller(48-54)custom_components/sat/pwm.py (6)
reset(65-74)state(240-244)enabled(232-233)enable(82-84)disable(86-89)status(236-237)
🪛 Pylint (4.0.3)
custom_components/sat/pwm.py
[refactor] 35-35: Too many instance attributes (19/7)
(R0902)
custom_components/sat/boiler.py
[refactor] 30-30: Too many instance attributes (22/7)
(R0902)
[refactor] 31-31: Too many arguments (12/5)
(R0913)
[refactor] 31-31: Too many positional arguments (12/5)
(R0917)
[refactor] 129-129: Too many return statements (17/6)
(R0911)
[refactor] 129-129: Too many branches (16/12)
(R0912)
[refactor] 318-318: Too many return statements (7/6)
(R0911)
custom_components/sat/minimum_setpoint.py
[refactor] 21-21: Too many instance attributes (18/7)
(R0902)
[refactor] 101-102: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 235-235: Too many arguments (6/5)
(R0913)
[refactor] 235-235: Too many positional arguments (6/5)
(R0917)
[refactor] 235-235: Too many return statements (10/6)
(R0911)
[refactor] 235-235: Too many branches (13/12)
(R0912)
custom_components/sat/cycles.py
[refactor] 37-37: Too many instance attributes (16/7)
(R0902)
[refactor] 258-258: Too many local variables (23/15)
(R0914)
[refactor] 341-341: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/util.py
52-52: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
53-53: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
70-70: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
71-71: CONF_AUTOMATIC_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
72-72: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
custom_components/sat/sensor.py
13-13: from .const import * used; unable to detect undefined names
(F403)
262-262: CycleClassification may be undefined, or defined from star imports
(F405)
268-268: CONF_NAME may be undefined, or defined from star imports
(F405)
282-282: BoilerStatus may be undefined, or defined from star imports
(F405)
custom_components/sat/config_flow.py
592-592: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
custom_components/sat/coordinator.py
14-14: from .const import * used; unable to detect undefined names
(F403)
262-262: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
262-262: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
330-330: Unused method argument: climate
(ARG002)
330-330: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/pid.py
213-213: HEATING_SYSTEM_UNDERFLOOR may be undefined, or defined from star imports
(F405)
custom_components/sat/minimum_setpoint.py
213-213: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
custom_components/sat/cycles.py
78-78: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
209-209: Avoid specifying long messages outside the exception class
(TRY003)
228-228: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/simulator/__init__.py
87-87: Unused method argument: timestamp
(ARG002)
87-87: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
87-87: Unused method argument: climate
(ARG002)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
593-593: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
606-606: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
610-610: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
812-812: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
813-813: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
937-937: DEADBAND may be undefined, or defined from star imports
(F405)
937-937: DEADBAND may be undefined, or defined from star imports
(F405)
994-994: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
🔇 Additional comments (18)
custom_components/sat/util.py (3)
13-15: LGTM! Import updates align with the new architecture.The imports correctly reflect the transition to the new configuration-based approach with
DynamicMinimumSetpoint,MinimumSetpointConfig, andPWMConfig.
48-54: LGTM! Factory function correctly updated.The function now returns
DynamicMinimumSetpointwith properMinimumSetpointConfiginitialization. The configuration uses sensible defaults for advanced tuning parameters.
67-78: LGTM! PWM controller factory updated correctly.The function signature and implementation properly reflect the new PWM API that accepts
PWMConfigandheating_curveparameters.custom_components/sat/cycles.py (4)
30-70: LGTM! Well-designed immutable data structures.The dataclasses use
frozen=Trueandslots=Truefor immutability and memory efficiency. TheCycledataclass comprehensively captures all relevant cycle information.
75-98: LGTM! Proper input validation.The constructor validates all window parameters to ensure they're positive, which is essential for the rolling window calculations.
99-202: LGTM! Rolling window statistics correctly implemented.The properties properly prune expired data before computing statistics, and
duty_ratio_last_15mcorrectly clamps the result to [0.0, 1.0].
258-380: LGTM! Comprehensive cycle classification logic.The cycle building and classification logic correctly handles various edge cases and provides detailed classification for minimum setpoint tuning. The classification distinguishes between underheat, overshoot, and short-cycling scenarios.
custom_components/sat/area.py (1)
154-157: LGTM! Updated to match new PID API.The
updatemethod correctly callsarea.pid.update(area.error, area.heating_curve.value)to match the refactored PID controller signature that no longer requiresboiler_temperature.custom_components/sat/__init__.py (1)
66-80: LGTM! Simplified unload logic.The unload flow is cleaner and correctly handles platform unloading, Sentry cleanup, and domain data removal.
custom_components/sat/config_flow.py (1)
592-595: LGTM! Simplified PID configuration UI.The removal of version selectors simplifies the configuration flow. The conditional logic correctly shows PID parameters only when automatic gains are disabled.
custom_components/sat/binary_sensor.py (1)
196-219: LGTM! Health sensors updated to use cycle-based classification.The
SatCycleHealthSensorcorrectly useslast_cycle.classificationto determine health status, flagging problems when the classification indicates underheat, overshoot, or short-cycling.custom_components/sat/simulator/__init__.py (1)
87-87: LGTM! Signature updated to match base class.The method signature correctly matches the updated base class interface. The
timestampandclimateparameters are unused in this simulator implementation but are required for interface compatibility.custom_components/sat/const.py (1)
177-214: New cycle/boiler enums and UNHEALTHY_CYCLES look coherent; watch for legacy string comparisonsThe new
CycleKind,CycleClassification,BoilerStatus,RelativeModulationState, andUNHEALTHY_CYCLESdefinitions are internally consistent and give you a clean, typed surface for the cycle‑centric model.Given the shift from older status types, just ensure any remaining callers aren’t still comparing raw strings (e.g.,
"central_heating") or the removedFlameStatusvalues instead of these Enums; that’s the main source of potential regressions here.Also applies to: 222-234
custom_components/sat/coordinator.py (1)
101-120: Cycle/boiler wiring on updates looks soundInitializing and wiring the new components:
_boiler: Boiler = Boiler()_cycles: CycleHistory = CycleHistory()_cycle_tracker: CycleTracker = CycleTracker(self._cycles)and then updating both from
async_add_listenercallbacks and fromasync_control_heating_loopusing the synthesizedBoilerStateplus a monotonictimestampensures that:
- Cycle tracking always sees the latest derived boiler state.
- Boiler’s internal status machine and modulation reliability get a consistent time base.
This composition looks coherent and should integrate well with the rest of the refactor.
Also applies to: 330-344
custom_components/sat/boiler.py (1)
30-127: Boiler status derivation and transition tracking look internally consistentThe new
Boilerclass cleanly separates:
- Input state (
BoilerState,last_cycle,timestamp).- Derived status via
_derive_status, with clear branches for:
- Power/availability.
- Flame‑off phases (overshoot cooling, anti‑cycling, cooling, pump start, waiting for flame, post‑cycle settling, idle).
- Flame‑on phases (DHW vs space heating, preheat, at‑band, modulation up/down, generic heating).
- Transition tracking (
_track_flame_transitions) that sets_last_flame_on_at,_last_flame_off_at, and_last_flame_off_was_overshootin a way that matches the downstream checks.From a correctness perspective this looks solid; the main cost is complexity, which is acceptable given the richer state machine you’re modeling.
custom_components/sat/climate.py (3)
180-199: PWM enable logic: good separation of forced/static/dynamic paths
pulse_width_modulation_enablednow:
- Immediately disables PWM when
_calculated_setpointisNone.- Forces PWM on when setpoint management is unsupported or
_force_pulse_width_modulationis true (_is_pwm_forced).- Respects
_overshoot_protectionto allow full disabling via config.- Switches between:
_should_enable_static_pwm()when_dynamic_minimum_setpointis false, comparing coordinatorminimum_setpointto_calculated_setpointwith and withoutBOILER_DEADBANDdepending on currentpwm.enabled._should_enable_dynamic_pwm()when_dynamic_minimum_setpointis true, using the learnedminimum_setpoint.valueplus last cycle health (UNHEALTHY_CYCLES).This cleanly separates configuration‑driven behavior and uses the dynamic controller only when requested. The “keep current PWM state when
last_cycleisNone” rule in_should_enable_dynamic_pwmshould also help prevent flapping at startup.Also applies to: 565-613
211-236: Debounced PID scheduling looks correct and should reduce thrashThe new
schedule_control_pid/async_control_pidflow:
- Forces an immediate run when
reset=True, cancelling any pending scheduled run.- Otherwise, schedules a single run 10 seconds later via
async_call_later, with_control_pid_unsubensuring only one pending run at a time.- Clears
_control_pid_unsubat the start ofasync_control_pidso subsequent changes can schedule again.And
async_control_piditself:
- Skips when
current_outside_temperatureis unavailable (with a clear warning).- Resets PID and area PIDs when sensor data is too old.
- Ensures heating curves are updated with the latest outside and target temperatures before PID updates.
This matches the intended debounce behavior and should significantly reduce redundant PID work on noisy sensor changes without sacrificing responsiveness on “reset” events (e.g. HVAC mode/target temperature changes).
Also applies to: 886-903
886-903: Good use of required-values guard before running the heating loopIn
async_control_heating_loopyou added:required_values = ( self.target_temperature, self.heating_curve.value, self.current_temperature, self.current_outside_temperature, ) if any(value is None for value in required_values): returnCombining this with the PID logic that ensures the heating curve is updated before use keeps the heating loop from acting on partial or inconsistent data. This is a good safety guard for a cycle‑based control system.
Also applies to: 945-975
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
custom_components/sat/relative_modulation.py (1)
31-39: Relative modulationstateandenabledare still inverted w.r.t. PWM.Current logic:
if self._pulse_width_modulation_enabled: return RelativeModulationState.OFF return RelativeModulationState.PWM_OFFand:
return self.state != RelativeModulationState.OFFmeans:
- PWM enabled → state =
OFF→enabled == False- PWM disabled → state =
PWM_OFF→enabled == TrueDownstream,
SatClimate.relative_modulation_valueusesenabledto decide whether to useMINIMUM_RELATIVE_MODULATIONor_maximum_relative_modulation, so this inversion causes the system to clamp to the minimum modulation when PWM is actually active.You likely want
enabledto beTruewhen PWM is enabled (and not in COLD/HOT_WATER), which implies swapping the two return values:- if self._pulse_width_modulation_enabled: - return RelativeModulationState.OFF - - return RelativeModulationState.PWM_OFF + if self._pulse_width_modulation_enabled: + return RelativeModulationState.PWM_OFF + + return RelativeModulationState.OFFThis aligns
enabledwith “relative modulation allowed / active” and matches the prior review feedback.custom_components/sat/pwm.py (1)
17-24: PWM config/state refactor is solid; butmaximum_cyclescan still produce invalid (negative) OFF times.The new
PWMConfig/PWMStatetypes andstateexposure look good and make PWM much easier to introspect.The earlier concern about
maximum_cyclesdriving invalid timings is still present though:
Thresholds are derived solely from
maximum_cycles:self._on_time_lower_threshold = 180 self._on_time_upper_threshold = 3600 / max(1, self._config.maximum_cycles) self._on_time_maximum_threshold = self._on_time_upper_threshold * 2In the low‑duty special case with flame active:
on_time = self._on_time_lower_threshold # 180 s off_time = self._on_time_maximum_threshold - self._on_time_lower_thresholdFor sufficiently large
maximum_cycles(e.g. > ~40),_on_time_maximum_thresholdbecomes smaller than_on_time_lower_threshold, makingoff_timenegative. That then feeds into transitions usingself._duty_cycle[1]and log messages that assume non‑negative durations.You can harden this by enforcing sane relationships between thresholds and clamping OFF time at 0, e.g.:
- self._on_time_lower_threshold: float = 180 - self._on_time_upper_threshold: float = 3600 / max(1, self._config.maximum_cycles) - self._on_time_maximum_threshold: float = self._on_time_upper_threshold * 2 + self._on_time_lower_threshold: float = 180 + self._on_time_upper_threshold: float = 3600 / max(1, self._config.maximum_cycles) + # Ensure the maximum threshold is never below the lower threshold + self._on_time_maximum_threshold: float = max( + self._on_time_upper_threshold * 2, + self._on_time_lower_threshold, + ) @@ - if boiler.flame_active and not boiler.hot_water_active: - on_time = self._on_time_lower_threshold - off_time = self._on_time_maximum_threshold - self._on_time_lower_threshold + if boiler.flame_active and not boiler.hot_water_active: + on_time = self._on_time_lower_threshold + off_time = max(0, self._on_time_maximum_threshold - self._on_time_lower_threshold) @@ - on_time = self._on_time_maximum_threshold - off_time = 0 + on_time = self._on_time_maximum_threshold + off_time = 0(and optionally validating
maximum_cyclesat config time so these invariants always hold).This keeps duty times non‑negative and avoids pathological behavior at extreme configuration values.
Also applies to: 47-57, 172-187, 223-227
🧹 Nitpick comments (6)
custom_components/sat/services.py (1)
6-6: Import change is fine; consider tightening the service schema forentity_id.The switch to only import
SERVICE_RESET_INTEGRALis consistent with dropping the PWM service. However, the service schema still useslist[str]as the validator:schema=vol.Schema({vol.Required("entity_id"): list[str]})In Voluptuous, you typically want something like
[str]or Home Assistant'scv.entity_idshelper, notlist[str](a typing alias), which may not be interpreted as a “list of strings” validator.If you want a list of entity IDs without re‑introducing
cv, consider:schema = vol.Schema({vol.Required("entity_id"): [str]})This keeps the behavior explicit and avoids potential validator quirks.
Also applies to: 23-28
custom_components/sat/const.py (1)
175-238: Cycle enums andUNHEALTHY_CYCLESlook coherent; consider makingBoilerStatusastrenum for consistency.The new cycle‑related enums and
UNHEALTHY_CYCLEStuple are consistent and easy to consume across the codebase.One minor consistency nit:
BoilerStatusis declared as a plainEnum, whereasCycleKind,CycleClassification,PWMStatus, andRelativeModulationStateall inherit fromstr, Enum. IfBoilerStatusvalues are ever exposed to Home Assistant as state/attributes or used in logging without.value, having it inherit fromstr, Enumwould make its behavior align with the others and avoid surprises when stringifying:-class BoilerStatus(Enum): +class BoilerStatus(str, Enum):Not blocking, but worth considering for API consistency.
custom_components/sat/coordinator.py (1)
95-104: Boiler/cycle wiring andminimum_setpointfloor look good;climatearg inasync_control_heating_loopis unused.The move to a composed
Boiler+CycleHistory/CycleTrackerand the newstate/cycles/last_cycleproperties looks coherent, and tighteningminimum_setpointwith:return max(float(self._config_data.get(CONF_MINIMUM_SETPOINT)), MINIMUM_SETPOINT)is a nice safety improvement.
One small cleanup:
async_control_heating_loopnow takes aclimate: Optional[SatClimate] = Noneargument, and callers passclimate=self, but the parameter is not used anywhere in the method. This is causing the static analysisARG002warning and adds noise to the public API.Unless you expect to use the
climateparameter soon, consider removing it from the signature (and call sites) to keep the coordinator API minimal.Also applies to: 118-120, 132-149, 151-158, 260-263, 330-344
custom_components/sat/cycles.py (3)
231-232: Refactor timestamp handling for correctness and type safety.Two issues:
- Line 231: PEP 484 prohibits implicit
Optional. The parameter should be explicitly typed asOptional[float] = None.- Line 232: The
timestamp or monotonic()pattern incorrectly treats0.0as falsy, which would usemonotonic()even when a valid timestamp of0.0is provided. Use an explicitNonecheck instead.Apply this diff:
- def update(self, boiler_state: BoilerState, timestamp: float = None) -> None: - timestamp = timestamp or monotonic() + def update(self, boiler_state: BoilerState, timestamp: Optional[float] = None) -> None: + timestamp = timestamp if timestamp is not None else monotonic() previously_active = self._last_flame_active currently_active = boiler_state.flame_active
284-291: Extract cycle kind thresholds to module-level constants.The hardcoded thresholds
0.8,0.2, and0.1should be defined as module-level constants (similar toOVERSHOOT_MARGIN_CELSIUSand others) for better maintainability and documentation.Add these constants after line 30:
UNDERSHOOT_MARGIN_CELSIUS: float = 2.0 # max_flow <= setpoint - margin -> underheat + +# Cycle kind classification thresholds +CYCLE_KIND_DOMINANT_THRESHOLD: float = 0.8 # Fraction to be considered dominant mode +CYCLE_KIND_MINOR_THRESHOLD: float = 0.2 # Fraction below which mode is negligible +CYCLE_KIND_MIXED_THRESHOLD: float = 0.1 # Minimum fraction for mixed modeThen update the logic:
- if fraction_dhw > 0.8 and fraction_heating < 0.2: + if fraction_dhw > CYCLE_KIND_DOMINANT_THRESHOLD and fraction_heating < CYCLE_KIND_MINOR_THRESHOLD: kind = CycleKind.DOMESTIC_HOT_WATER - elif fraction_heating > 0.8 and fraction_dhw < 0.2: + elif fraction_heating > CYCLE_KIND_DOMINANT_THRESHOLD and fraction_dhw < CYCLE_KIND_MINOR_THRESHOLD: kind = CycleKind.CENTRAL_HEATING - elif fraction_dhw > 0.1 and fraction_heating > 0.1: + elif fraction_dhw > CYCLE_KIND_MIXED_THRESHOLD and fraction_heating > CYCLE_KIND_MIXED_THRESHOLD: kind = CycleKind.MIXED else: kind = CycleKind.UNKNOWN
374-382: Extract short-cycling multiplier to a named constant.The magic number
2.0on line 376 should be defined as a module-level constant to document that short-cycling detection requires double the normal low-load threshold.Add this constant after line 25:
LOW_LOAD_MAX_DUTY_RATIO_15_M: float = 0.50 +SHORT_CYCLING_MULTIPLIER: float = 2.0 # Short-cycling requires 2x normal low-load cycles/hourThen update line 376:
short_cycling_context = ( - statistics.cycles_last_hour > LOW_LOAD_MIN_CYCLES_PER_HOUR * 2.0 + statistics.cycles_last_hour > LOW_LOAD_MIN_CYCLES_PER_HOUR * SHORT_CYCLING_MULTIPLIER and statistics.duty_ratio_last_15m < LOW_LOAD_MAX_DUTY_RATIO_15_M )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
custom_components/sat/climate.py(22 hunks)custom_components/sat/const.py(1 hunks)custom_components/sat/coordinator.py(7 hunks)custom_components/sat/cycles.py(1 hunks)custom_components/sat/pwm.py(8 hunks)custom_components/sat/relative_modulation.py(2 hunks)custom_components/sat/services.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
custom_components/sat/relative_modulation.py (1)
custom_components/sat/const.py (1)
RelativeModulationState(226-230)
custom_components/sat/pwm.py (4)
custom_components/sat/const.py (1)
PWMStatus(220-223)custom_components/sat/heating_curve.py (3)
HeatingCurve(11-92)value(91-92)reset(21-25)custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/setpoint_adjuster.py (1)
reset(20-22)
custom_components/sat/cycles.py (4)
custom_components/sat/const.py (2)
CycleKind(181-185)CycleClassification(188-194)custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/coordinator.py (5)
last_cycle(156-157)flame_active(179-180)hot_water_active(187-188)setpoint(165-166)return_temperature(199-200)
🪛 Pylint (4.0.3)
custom_components/sat/pwm.py
[refactor] 35-35: Too many instance attributes (19/7)
(R0902)
custom_components/sat/cycles.py
[refactor] 39-39: Too many instance attributes (16/7)
(R0902)
[refactor] 263-263: Too many local variables (23/15)
(R0914)
[refactor] 346-346: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
239-239: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
600-600: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
613-613: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
617-617: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
819-819: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
820-820: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
1001-1001: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/coordinator.py
14-14: from .const import * used; unable to detect undefined names
(F403)
262-262: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
262-262: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
330-330: Unused method argument: climate
(ARG002)
330-330: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/cycles.py
80-80: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
211-211: Avoid specifying long messages outside the exception class
(TRY003)
231-231: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (4)
custom_components/sat/cycles.py (4)
1-30: LGTM! Well-structured imports and constants.The module-level constants for thresholds and margins are well-documented and appropriately scoped.
32-72: LGTM! Well-designed immutable data structures.The use of
frozen=Trueandslots=Trueis excellent for immutability and memory efficiency. TheCycledataclass intentionally contains many attributes to provide comprehensive cycle telemetry, which is appropriate for this domain.
164-203: LGTM! Robust cycle recording and window management.The record and prune logic correctly maintains rolling windows, and defensive programming with
max(0.0, cycle.duration)ensures robustness even though the duration is already guaranteed non-negative.
293-343: LGTM! Comprehensive cycle state aggregation.The aggregation logic correctly handles optional values, computes averages and extremes, and constructs a complete
Cycleobject. The local_avghelper is appropriately scoped.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
custom_components/sat/minimum_setpoint.py (1)
66-70: Guard against re‑processing the same completed cycle inupdate/ tuning
DynamicMinimumSetpoint.updateincrementsregime_state.completed_cyclesand calls_maybe_tune_minimumon every invocation wherelast_cycleis non‑None(Lines 134–136), butRegimeStatehas no notion of which cycle was last processed. If the caller passes the last completedCyclerepeatedly between new completions (a common pattern for “last completed cycle” APIs), this will:
- Inflate
completed_cyclesfar beyond the true number of physical cycles.- Apply the same classification adjustment multiple times for a single real cycle, quickly driving
minimum_setpointup or down.- Make the warmup guard (
completed_cycles <= warmup_cycles_before_tuning) ineffective after the first few calls.This is effectively the same issue that was raised on a previous commit and still appears unresolved in this version.
A robust fix is to track the last processed cycle per regime and only tune once per completed cycle:
@@ -@dataclass(slots=True) -class RegimeState: - minimum_setpoint: float - completed_cycles: int = 0 +@dataclass(slots=True) +class RegimeState: + minimum_setpoint: float + completed_cycles: int = 0 + # Last completed cycle end time that was already used for tuning (per regime) + last_processed_cycle_end: Optional[float] = None @@ - # Update the count of cycles and possibly adjust the learned minimum when a cycle has just completed. - if last_cycle is not None: - regime_state.completed_cycles += 1 - self._maybe_tune_minimum(regime_state, boiler_state, cycles, last_cycle, base_setpoint=requested_setpoint) + # Update the count of cycles and possibly adjust the learned minimum once per completed cycle. + if last_cycle is not None: + # Use cycle.end as a stable per-cycle identifier. + if regime_state.last_processed_cycle_end != last_cycle.end: + regime_state.completed_cycles += 1 + regime_state.last_processed_cycle_end = last_cycle.end + self._maybe_tune_minimum( + regime_state, + boiler_state, + cycles, + last_cycle, + base_setpoint=requested_setpoint, + )This keeps
completed_cyclesaligned with real cycles and prevents runaway tuning on a single bad cycle, while remaining per‑regime and not affecting persistence (the new field relies on its default on load).Also applies to: 112-139, 235-332
🧹 Nitpick comments (2)
custom_components/sat/minimum_setpoint.py (2)
88-104: Minor simplification ofvalueclamping logicThe guard against drifting too far below recent bases is correct, but can be slightly simplified without changing behavior:
- if self._last_base_setpoint is not None: - allowed_minimum = max(self._config.minimum_setpoint, self._last_base_setpoint - self._config.max_deviation_from_recent_base) - if minimum_setpoint < allowed_minimum: - minimum_setpoint = allowed_minimum + if self._last_base_setpoint is not None: + allowed_minimum = max( + self._config.minimum_setpoint, + self._last_base_setpoint - self._config.max_deviation_from_recent_base, + ) + minimum_setpoint = max(minimum_setpoint, allowed_minimum)This matches the linter suggestion and keeps the intent obvious.
333-347: Optional: tighten_is_tunable_regimefor “low‑load” semanticsCurrent gating (
hot_water_active,is_inactive,sample_count_4h,cycles_last_hour) is reasonable, but to better match the “low‑load” idea you might also:
- Incorporate
low_load_maximum_duty_ratio_15mhere (see previous comment).- Optionally rename or document
low_load_minimum_cycles_per_hourto clarify whether “low‑load” here means “few, long cycles” vs. “many, short ones”.Not a blocker, but clarifying this now will make future PID/learning tweaks easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/minimum_setpoint.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/minimum_setpoint.py (4)
custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/const.py (2)
CycleClassification(188-194)CycleKind(181-185)custom_components/sat/cycles.py (5)
CycleStatistics(66-71)Cycle(39-62)cycles_last_hour(111-117)duty_ratio_last_15m(120-133)sample_count_4h(102-108)custom_components/sat/helpers.py (1)
clamp(90-95)
🪛 Pylint (4.0.3)
custom_components/sat/minimum_setpoint.py
[refactor] 21-21: Too many instance attributes (18/7)
(R0902)
[refactor] 101-102: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 235-235: Too many arguments (6/5)
(R0913)
[refactor] 235-235: Too many positional arguments (6/5)
(R0917)
[refactor] 235-235: Too many return statements (10/6)
(R0911)
[refactor] 235-235: Too many branches (13/12)
(R0912)
🪛 Ruff (0.14.5)
custom_components/sat/minimum_setpoint.py
213-213: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
🔇 Additional comments (2)
custom_components/sat/minimum_setpoint.py (2)
203-234: Regime keying and initial minimum reuse look soundBucketing setpoints into coarse
regime_band_widthbands and seeding new regimes from the nearest existing bucket is a nice way to keep behavior smooth across adjacent bands while still allowing differentiated learning. Logic for parsing bucket indices and clamping the initial minimum is clear and robust to malformed keys.
383-406: Large‑jump damping and clamping are implemented correctlyThe large‑jump damping logic around
base_setpointtransitions is well‑contained: it no‑ops when there is no previous base, respectsmax_setpoint_jump_without_damping, damps toward the absolute minimum usinglarge_jump_damping_factor, and finally clamps via_clamp_setpoint. This should effectively avoid dragging a low‑load minimum into a very different regime.
…rom_hass`, improve `cycles_last_hour` calculation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
custom_components/sat/climate.py (1)
211-252: FixEVENT_HOMEASSISTANT_STOPlistener to avoid unsafehass.async_create_taskusage.The current STOP handler:
self.hass.bus.async_listen( EVENT_HOMEASSISTANT_STOP, lambda _: self.hass.async_create_task(self.async_will_remove_from_hass()) )is what the pipeline flags as calling
hass.async_create_taskfrom a non-event-loop thread. It also relies on a lambda rather than a proper async listener.A safer and more idiomatic pattern is to register an async handler that the event bus can schedule itself:
- self.hass.bus.async_listen(EVENT_HOMEASSISTANT_STOP, lambda _: self.hass.async_create_task(self.async_will_remove_from_hass())) + self.hass.bus.async_listen(EVENT_HOMEASSISTANT_STOP, self._handle_ha_stop) + + async def _handle_ha_stop(self, _event: Event) -> None: + """Handle Home Assistant stop event and run cleanup.""" + await self.async_will_remove_from_hass()This removes direct
async_create_taskusage from the listener, satisfies HA’s thread-safety checks, and ensures your cleanup is awaited on shutdown.Verify the recommended pattern for registering async event listeners and avoiding unsafe `hass.async_create_task` calls in Home Assistant custom integrations (particularly for `EVENT_HOMEASSISTANT_STOP` handlers).
🧹 Nitpick comments (4)
custom_components/sat/cycles.py (2)
183-205:_current_time_hintchoice of “now” is acceptable but slightly under-doc’d.Using the newest timestamp from the existing deques as a synthetic “now” keeps pruning and stats deterministic without needing wall-clock time, but it means windows are effectively anchored to the last recorded cycle, not real time. That’s reasonable for per-cycle classification; consider adding a short comment explaining this design choice so future readers don’t assume it’s actual wall-clock
monotonic()time.
347-386: Align_classify_cycletype hints with actualOptionalusage.The implementation correctly handles missing data (
average_setpoint is None,max_flow_temperature is None) and returnsINSUFFICIENT_DATA/UNCERTAINappropriately, but the signature still types these asfloat:def _classify_cycle( statistics: CycleStatistics, duration: float, max_flow_temperature: float, average_setpoint: float, ) -> CycleClassification:Given you explicitly check for
None, it would be more accurate to type them asOptional[float]and pass them as such from_build_cycle_state. This will quiet type-checkers and make the contract clearer to readers.custom_components/sat/climate.py (2)
571-620: PWM enable/disable logic is much clearer; consider tiny doc tweaks.The new
pulse_width_modulation_enabledplus_is_pwm_forced,_should_enable_static_pwm, and_should_enable_dynamic_pwmhelpers make the decision-making around PWM explicit and easier to reason about:
- Require a valid
_calculated_setpointbefore even considering PWM.- Force PWM when setpoint management isn’t supported or explicitly forced.
- For static minimum setpoint, use a simple hysteresis (
BOILER_DEADBAND) around the coordinator’s minimum.- For dynamic minimum setpoint, consider both
minimum_setpoint.valueand recentlast_cycle.classificationviaUNHEALTHY_CYCLES.Behavior-wise this is sound. As a small polish, a brief comment on the intended hysteresis in
_should_enable_static_pwmand on the “no history yet: keep current PWM state” strategy in_should_enable_dynamic_pwmwould help future readers understand why PWM doesn’t flap at startup.
795-824:_async_control_setpointmatches PWM state model; first branch is currently unreachable.The refactored
_async_control_setpointaligns well with the new PWM semantics:
- Normal path: when PWM is disabled or
PWMStatus.IDLE, use the filtered_calculated_setpoint.- PWM active: use
minimum_setpoint_valuewhenPWMStatus.ON, andMINIMUM_SETPOINTwhen PWM is effectively “off”.- Always delegates the final setpoint application to the coordinator.
Given
async_control_heating_loopalready early-returns ifhvac_mode != HVACMode.HEAT, the top branch that sets_setpoint = MINIMUM_SETPOINTfor non-HEAT modes is currently unreachable. Not harmful, but you could simplify by either removing that branch or documenting that_async_control_setpointis intended to be reusable from other call sites in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/sat/climate.py(22 hunks)custom_components/sat/cycles.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
custom_components/sat/cycles.py (4)
custom_components/sat/const.py (2)
CycleKind(181-185)CycleClassification(188-194)custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/coordinator.py (5)
last_cycle(156-157)flame_active(179-180)hot_water_active(187-188)setpoint(165-166)return_temperature(199-200)
custom_components/sat/climate.py (4)
custom_components/sat/const.py (2)
RelativeModulationState(226-230)PWMStatus(220-223)custom_components/sat/util.py (2)
create_pid_controller(21-45)create_dynamic_minimum_setpoint_controller(48-54)custom_components/sat/pwm.py (6)
reset(65-74)state(238-242)enabled(230-231)enable(82-136)disable(138-141)status(234-235)custom_components/sat/pid.py (4)
reset(49-64)update(66-83)output(264-266)last_updated(202-204)
🪛 GitHub Actions: Run PyTest Unit Tests
custom_components/sat/climate.py
[error] 237-237: Detected that custom integration 'sat' calls hass.async_create_task from a thread other than the event loop. This is unsafe and will cause Home Assistant to crash. For more information, see https://developers.home-assistant.io/docs/asyncio/thread-safety/#hassasync_create_task
🪛 Pylint (4.0.3)
custom_components/sat/cycles.py
[refactor] 39-39: Too many instance attributes (16/7)
(R0902)
[refactor] 264-264: Too many local variables (23/15)
(R0914)
[refactor] 347-347: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/cycles.py
80-80: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
212-212: Avoid specifying long messages outside the exception class
(TRY003)
232-232: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
239-239: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
600-600: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
613-613: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
617-617: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
819-819: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
820-820: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
1001-1001: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
🔇 Additional comments (13)
custom_components/sat/cycles.py (2)
74-120: CycleHistory windows andcycles_last_hournormalization look correct.Validation of the rolling windows and
cycles_last_hourrate is solid: you validate positive window sizes, prune before computing, and correctly normalize the cycle count byself._cycles_window_seconds(so the value is now truly “cycles per hour” regardless of window size). This addresses the earlier concern about raw counts vs. rates and keeps the API semantics coherent.
264-345: Cycle aggregation and kind/fraction logic are consistent with boiler model.The way you derive DHW vs. space-heating fractions, compute averages over non-
Nonesamples, and collect min/max temps is logically sound and robust to missing telemetry. The thresholds forCycleKind(> 0.8,< 0.2, etc.) are simple and easy to reason about, and thesample_count/minimum_samples_per_cycleguard avoids noisy micro-cycles polluting stats.custom_components/sat/climate.py (11)
223-236: Startup lifecycle wiring for PID/heating loop and minimum setpoint looks coherent.Conditionally registering
_register_event_listeners,async_control_heating_loop, andasync_control_pid(reset=True)viaEVENT_HOMEASSISTANT_STARTEDwhen HA isn’t yet running, and otherwise awaiting them immediately, gives you predictable startup behavior while keeping everything on the event loop. The subsequent calls to coordinator/areas/minimum_setpointasync_added_to_hassare ordered sensibly.
395-438: State exposure for dynamic minimum setpoint and PWM state is consistent.Adding
minimum_setpoint: self.minimum_setpoint.valueand switching PWM-related attributes toself.pwm.status.name/self.pwm.state.duty_cyclekeeps external state aligned with the new controllers and enum/state model. This should make it much easier to debug dynamic minimum setpoint behavior and PWM decisions from the UI.
520-530: Requested setpoint computation is straightforward and guarded.
requested_setpointnow cleanly returnsMINIMUM_SETPOINTwhen the heating curve has no value and otherwise usesround(self.heating_curve.value + self.pid.output, 1). Combined with the new debounce inasync_control_pid, this gives a clear separation between curve+PID math and later clamping/filtering.
634-641:minimum_setpoint_valuenicely encapsulates dynamic vs. static behavior.Routing the minimum setpoint through
minimum_setpoint.valuewhen_dynamic_minimum_setpointis enabled, and otherwise falling back toself._coordinator.minimum_setpoint, centralizes this choice and keeps_async_control_setpointand other callers simple.
669-693: Usingschedule_control_pidfor sensor changes is a good debouncing strategy.Switching the inside/outside/humidity sensor handlers to call
schedule_control_pid()(andschedule_control_heating_loop()) instead of driving control directly is a good way to coalesce rapid sensor updates and avoid thrashing the PID/heating loop.
705-750: Climate entity change handling withschedule_control_pidcovers key cases.The updated
_async_climate_changedlogic correctly:
- Forces a PID reset when the entity state or target temperature changes.
- Schedules a non-reset PID run when only the current temperature changes (and no linked sensor).
- Keeps room setpoints in
_roomsin sync when appropriate.- Always schedules the heating loop afterward.
This matches the new debounced PID model and should reduce unnecessary recalculations while still reacting promptly to meaningful changes.
751-760: Tracking additional temperature sensors via_async_temperature_changeis consistent.The handler updates HA state and then schedules both PID and heating loop via the debouncers, which is consistent with the other sensor handlers and should keep behavior predictable across different sensor sources.
921-950: PID control logic and stale-sensor handling are sound.Within
async_control_pid:
- You bail out cleanly if
current_outside_temperatureis missing.- You reset PID/area PIDs when
pid.last_updatedexceeds_sensor_max_value_age.- You keep heating-curve updates in sync with the latest target/outside temps.
- You distinguish reset vs. non-reset flows (
pid.update_resetvspid.update) and only autotune the heating curve when in the deadband.Overall this is a solid control loop and matches the new requested/minimum setpoint pipeline.
952-1011: Heating loop guard conditions and PWM application are coherent.The updated
async_control_heating_loop:
- Skips work when any required input (
target_temperature,heating_curve.value,current_temperature,current_outside_temperature) is missing.- Exits early when not in
HVACMode.HEAT.- Delegates boiler-specific logic to the coordinator.
- Smooths
_calculated_setpointwith a low-pass filter and clamps viaclamp(..., MINIMUM_SETPOINT, self._coordinator.maximum_setpoint).- Drives PWM via
pulse_width_modulation_enabledand then passesself.pwm.statusinto_async_control_setpoint.These steps are consistent and keep boiler control within sane bounds.
1073-1078: Resetting PID viaschedule_control_pid(True)on HVAC mode changes is appropriate.Resetting the PID controller (and, via
reset_control_state, PWM and_calculated_setpoint) whenever HVAC mode changes ensures you don’t carry stale integral or filtered setpoints across OFF/HEAT transitions.
1154-1165: Target temperature changes correctly trigger PID reset and forced heating loop.On
async_set_target_temperature, you:
- Avoid redundant work when the target is unchanged.
- Cascade updates to radiators and (optionally) the boiler thermostat.
- Reset the PID via
schedule_control_pid(True)andreset_control_state().- Force an immediate heating-loop run with
schedule_control_heating_loop(force=True).That sequence is consistent with the debounced control design and should make target changes feel responsive.
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/climate.py (1)
968-968: SameHassJobcoroutine issue: wrapasync_control_heating_loopin a callback.Line 968 has the same issue as line 909: passing a coroutine function (
self.async_control_heating_loop) directly toHassJobwill raiseValueErrorat runtime.Apply the same fix:
- self._control_heating_loop_unsub = async_call_later(self.hass, 10, HassJob(self.async_control_heating_loop)) + self._control_heating_loop_unsub = async_call_later( + self.hass, 10, + HassJob(lambda: self.hass.async_create_background_task(self.async_control_heating_loop())) + )
♻️ Duplicate comments (3)
custom_components/sat/minimum_setpoint.py (2)
26-36: [Duplicate] Clarify inverted comments and use the definedlow_load_maximum_duty_ratio_15mthreshold.As flagged in previous reviews, the comments for
increase_stepanddecrease_stepremain inverted relative to how they are applied in_maybe_tune_minimum(lines 304–321):
TOO_SHORT_UNDERHEAT(underheat) → decreases the minimum (line 311:regime_state.minimum_setpoint -= self._config.decrease_step)TOO_SHORT_OVERSHOOT/SHORT_CYCLING_OVERSHOOT(overshoot/short-cycling) → increases the minimum (line 321:regime_state.minimum_setpoint += self._config.increase_step)The config comments currently say the opposite. Update them to reflect actual behavior.
Additionally,
low_load_maximum_duty_ratio_15mis defined but never used. Add a check in_is_tunable_regime(lines 334–348) to gate tuning whenstatistics.duty_ratio_last_15mexceeds this threshold.Based on past review comments.
112-140: [Duplicate] Prevent repeated tuning on the same completed cycle.As flagged in previous reviews, the
updatemethod incrementsregime_state.completed_cyclesand calls_maybe_tune_minimumevery timelast_cycleis non-None(lines 134–137), but there is no guard to ensure each physical cycle is processed only once.Since
last_cycleinCycleHistoryis the last completed cycle and remains constant until a new one finishes, your heating loop will callupdate(...)many times for the sameCycle, re-applying the same classification adjustment on each call.Consequences:
- For a single bad cycle (e.g.,
TOO_SHORT_UNDERHEAT),minimum_setpointwill be stepped up/down on every control iteration (e.g., every 30s) until another cycle completes.- Over a minute of iterations, a step size of
1.0could move the learned minimum by many degrees based on a single cycle.Fix: Track the last processed cycle identity (e.g.,
last_cycle.endorid(last_cycle)) inRegimeStateorDynamicMinimumSetpoint, and skip_maybe_tune_minimumwhen the cycle has already been applied.Based on past review comments.
Apply this fix to add per-regime cycle tracking:
@dataclass(slots=True) class RegimeState: minimum_setpoint: float completed_cycles: int = 0 + last_processed_cycle_end: Optional[float] = None# Mark a cycle as completed. - regime_state.completed_cycles += 1 - # Update the count of cycles and possibly adjust the learned minimum when a cycle has just completed. - self._maybe_tune_minimum(regime_state, boiler_state, cycles, last_cycle, base_setpoint=requested_setpoint) + if last_cycle.end != regime_state.last_processed_cycle_end: + regime_state.completed_cycles += 1 + self._maybe_tune_minimum(regime_state, boiler_state, cycles, last_cycle, base_setpoint=requested_setpoint) + regime_state.last_processed_cycle_end = last_cycle.endAlso update storage load/save logic (lines 141–203) to persist/restore
last_processed_cycle_end.custom_components/sat/climate.py (1)
893-909: [Duplicate] FixHassJobwith coroutine function: wrap in callback that schedules the coroutine.As flagged in previous reviews, line 909 passes
self.async_control_pid(a coroutine function) toHassJob. Whenasync_call_latercalls it, it will return a coroutine object, which Home Assistant forbids passing toHassJob(raisesValueError).Current Home Assistant best practice: Wrap the coroutine in a callback that uses
hass.async_create_background_task.Based on past review comments.
Apply this fix:
- self._control_pid_unsub = async_call_later(self.hass, 10, HassJob(self.async_control_pid)) + def _schedule_control_pid_callback() -> None: + self.hass.async_create_background_task(self.async_control_pid()) + + self._control_pid_unsub = async_call_later(self.hass, 10, HassJob(_schedule_control_pid_callback))Or use a lambda:
- self._control_pid_unsub = async_call_later(self.hass, 10, HassJob(self.async_control_pid)) + self._control_pid_unsub = async_call_later( + self.hass, 10, + HassJob(lambda: self.hass.async_create_background_task(self.async_control_pid())) + )
🧹 Nitpick comments (1)
custom_components/sat/minimum_setpoint.py (1)
350-364: Remove unused_relax_minimum_when_uncertainmethod.This method is defined but never called in the current code. If it's intended for future use, consider removing it for now to keep the codebase clean, or add a TODO comment explaining its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/sat/climate.py(22 hunks)custom_components/sat/minimum_setpoint.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/minimum_setpoint.py (6)
custom_components/sat/boiler.py (1)
BoilerState(17-27)custom_components/sat/const.py (2)
CycleClassification(188-194)CycleKind(181-185)custom_components/sat/coordinator.py (6)
cycles(152-153)minimum_setpoint(260-262)last_cycle(156-157)get(407-422)setpoint(165-166)hot_water_active(187-188)custom_components/sat/cycles.py (7)
CycleStatistics(66-71)Cycle(39-62)last_cycle(152-153)statistics(156-163)cycles_last_hour(111-118)duty_ratio_last_15m(121-134)sample_count_4h(102-108)custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/climate.py (2)
requested_setpoint(524-529)setpoint(520-521)
🪛 GitHub Actions: Run PyTest Unit Tests
custom_components/sat/climate.py
[error] 237-237: Detected that custom integration 'sat' calls hass.async_create_task from a thread other than the event loop, which may cause Home Assistant to crash or data to corrupt. For more information, see https://developers.home-assistant.io/docs/asyncio_thread_safety/#hassasync_create_task
🪛 Pylint (4.0.3)
custom_components/sat/minimum_setpoint.py
[refactor] 21-21: Too many instance attributes (18/7)
(R0902)
[refactor] 101-102: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 236-236: Too many arguments (6/5)
(R0913)
[refactor] 236-236: Too many positional arguments (6/5)
(R0917)
[refactor] 236-236: Too many return statements (10/6)
(R0911)
[refactor] 236-236: Too many branches (13/12)
(R0912)
🪛 Ruff (0.14.5)
custom_components/sat/minimum_setpoint.py
214-214: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
239-239: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
600-600: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
613-613: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
617-617: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
819-819: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
820-820: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
944-944: DEADBAND may be undefined, or defined from star imports
(F405)
1001-1001: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
🔇 Additional comments (2)
custom_components/sat/climate.py (2)
572-621: Nice refactoring: PWM enable logic is now clearer with helper methods.The extraction of
_is_pwm_forced,_should_enable_static_pwm, and_should_enable_dynamic_pwmimproves readability and maintainability. The logic flow is easier to follow, and the separation of concerns between static and dynamic minimum setpoint handling is clean.
659-693: Good change: debouncing PID control on sensor changes reduces redundant work.Switching from direct
_async_control_pid()calls toschedule_control_pid()in sensor change handlers (lines 669, 677, 690) avoids stacking multiple PID updates when sensors change rapidly. The added debug logs are also helpful for troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
custom_components/sat/pwm.py (1)
48-57: Negativeoff_timeissue with highmaximum_cyclesalready flagged.The threshold calculation issue where high
maximum_cyclescan produce negativeoff_timehas been identified in a previous review. The suggested fix to clamp_on_time_maximum_thresholdand usemax(0, ...)foroff_timeshould be applied.custom_components/sat/minimum_setpoint.py (2)
28-35: Config comments are inverted relative to actual behavior.The comments on
increase_stepanddecrease_stepare reversed compared to how they're applied in_maybe_tune_minimum:
TOO_SHORT_UNDERHEAT→minimum_setpoint -= decrease_step(Line 352)TOO_SHORT_OVERSHOOT/SHORT_CYCLING_OVERSHOOT→minimum_setpoint += increase_step(Line 362)Additionally,
low_load_maximum_duty_ratio_15mis defined but never used in tuning logic.
140-156: Tuning occurs on everyon_cycle_endcall without deduplication.The
on_cycle_endmethod incrementscompleted_cyclesand calls_maybe_tune_minimumevery time it's invoked. If the samelast_cycleobject is passed multiple times (e.g., due to multiple event triggers before a new cycle completes), the same cycle will be tuned repeatedly.custom_components/sat/climate.py (1)
253-253: STOP listener still doesn't properly await cleanup.The lambda
lambda _: self.async_will_remove_from_hass()returns a coroutine that is never awaited. Home Assistant's event bus won't schedule this coroutine, so cleanup won't run on shutdown.Apply this diff:
- self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, lambda _: self.async_will_remove_from_hass()) + async def _handle_ha_stop(_event): + await self.async_will_remove_from_hass() + + self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, _handle_ha_stop)
🧹 Nitpick comments (10)
custom_components/sat/const.py (1)
197-217: Consider inheriting fromstrfor consistency with other enums.
BoilerStatusinherits only fromEnum, whileCycleKind,CycleClassification,PWMStatus, andRelativeModulationStateall use the(str, Enum)pattern. This inconsistency may cause issues with serialization or comparison in Home Assistant state attributes.-class BoilerStatus(Enum): +class BoilerStatus(str, Enum):custom_components/sat/binary_sensor.py (1)
200-223: Consider using theUNHEALTHY_CYCLESconstant for maintainability.The health check logic manually excludes
GOOD,UNCERTAIN, andINSUFFICIENT_DATA, which is the inverse of theUNHEALTHY_CYCLEStuple defined inconst.py. Using the constant would make this easier to maintain if classifications change.-from .const import CONF_MODE, MODE_SERIAL, CONF_NAME, DOMAIN, COORDINATOR, CLIMATE, CONF_WINDOW_SENSORS, BoilerStatus, CycleClassification +from .const import CONF_MODE, MODE_SERIAL, CONF_NAME, DOMAIN, COORDINATOR, CLIMATE, CONF_WINDOW_SENSORS, BoilerStatus, CycleClassification, UNHEALTHY_CYCLESThen update the
is_onproperty:@property def is_on(self) -> bool: """Return the state of the sensor.""" if self._coordinator.last_cycle is None: return False - return self._coordinator.last_cycle.classification not in (CycleClassification.GOOD, CycleClassification.UNCERTAIN, CycleClassification.INSUFFICIENT_DATA) + return self._coordinator.last_cycle.classification in UNHEALTHY_CYCLEScustom_components/sat/pwm.py (1)
139-142: Redundant assignment indisable().
reset()already setsself._enabled = Falseat line 68, so the assignment at line 142 is redundant.def disable(self) -> None: """Disable the PWM control.""" self.reset() - self._enabled = Falsecustom_components/sat/minimum_setpoint.py (1)
116-116: Unusedboiler_stateparameter.The
boiler_stateparameter is accepted but never used inon_cycle_start. Consider removing it or documenting why it's reserved for future use.- def on_cycle_start(self, boiler_state: BoilerState, cycles: CycleStatistics, last_cycle: Optional[Cycle], requested_setpoint: Optional[float], outside_temperature: Optional[float]) -> None: + def on_cycle_start(self, cycles: CycleStatistics, last_cycle: Optional[Cycle], requested_setpoint: Optional[float], outside_temperature: Optional[float]) -> None:custom_components/sat/coordinator.py (2)
331-344: Unusedclimateparameter inasync_control_heating_loop.The
climateparameter is accepted but never used. Static analysis also flags this. If it's intended for future use, consider documenting this or removing it.- async def async_control_heating_loop(self, climate: Optional[SatClimate] = None, pwm_state: Optional[PWMState] = None, timestamp: float = None) -> None: + async def async_control_heating_loop(self, pwm_state: Optional[PWMState] = None, timestamp: float = None) -> None:Note: If you keep the parameter, update the type hint to use explicit
Optional[float]instead of implicitfloat = Noneper PEP 484:timestamp: Optional[float] = None
119-120: Listener callbacks invoke properties that may trigger side effects.The listeners call
self.stateandself.last_cyclewhich construct new objects on each invocation. While this works, it creates objects that are immediately discarded if the tracker/boiler only needs specific fields. Consider whether this pattern causes unnecessary allocations in high-frequency update scenarios.custom_components/sat/boiler.py (1)
364-365: Consider using a bounded deque instead of manual slicing.The modulation values list is manually sliced to keep the last 50 values. Using a
dequewithmaxlen=50would be more efficient and idiomatic.+from collections import deque ... - self._modulation_values_when_flame_on: List[float] = [] + self._modulation_values_when_flame_on: deque[float] = deque(maxlen=50) ... - self._modulation_values_when_flame_on.append(value) - if len(self._modulation_values_when_flame_on) > 50: - self._modulation_values_when_flame_on = self._modulation_values_when_flame_on[-50:] + self._modulation_values_when_flame_on.append(value)custom_components/sat/climate.py (1)
237-251: Event listeners for cycle events invoke synchronous lambdas that call synchronous methods.The
on_cycle_startandon_cycle_endmethods are synchronous, and the lambdas correctly invoke them synchronously. However, these listeners are not unsubscribed when the entity is removed, which could cause issues if the entity is removed and re-added.Consider storing the unsubscribe callbacks and calling them in
async_will_remove_from_hass:self._cycle_started_unsub = self.hass.bus.async_listen(EVENT_SAT_CYCLE_STARTED, ...) self._cycle_ended_unsub = self.hass.bus.async_listen(EVENT_SAT_CYCLE_ENDED, ...)custom_components/sat/cycles.py (2)
246-246: Use explicitOptionaltype hint per PEP 484.The
timestampparameter uses implicitOptional(timestamp: float = None), which is discouraged.- def update(self, boiler_state: BoilerState, pwm_state: Optional[PWMState] = None, timestamp: float = None) -> None: + def update(self, boiler_state: BoilerState, pwm_state: Optional[PWMState] = None, timestamp: Optional[float] = None) -> None:
310-391: Consider extracting aggregation logic to reduce method complexity.The
_build_cycle_statemethod has many local variables (flagged by Pylint). While the logic is correct, extracting the aggregation of setpoints/temperatures into a helper method would improve readability.def _aggregate_samples(self, samples: List[CycleSample]) -> dict: """Aggregate numeric fields from samples.""" def _avg(values: List[Optional[float]]) -> Optional[float]: filtered = [v for v in values if v is not None] return sum(filtered) / len(filtered) if filtered else None setpoints = [s.boiler_state.setpoint for s in samples] flow_temps = [s.boiler_state.flow_temperature for s in samples] # ... etc return { "average_setpoint": _avg(setpoints), # ... etc }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
custom_components/sat/area.py(6 hunks)custom_components/sat/binary_sensor.py(4 hunks)custom_components/sat/boiler.py(1 hunks)custom_components/sat/climate.py(23 hunks)custom_components/sat/const.py(1 hunks)custom_components/sat/coordinator.py(7 hunks)custom_components/sat/cycles.py(1 hunks)custom_components/sat/minimum_setpoint.py(1 hunks)custom_components/sat/pwm.py(8 hunks)custom_components/sat/simulator/__init__.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
custom_components/sat/simulator/__init__.py (2)
custom_components/sat/coordinator.py (1)
async_control_heating_loop(331-349)custom_components/sat/pwm.py (1)
PWMState(18-24)
custom_components/sat/minimum_setpoint.py (4)
custom_components/sat/boiler.py (1)
BoilerState(21-34)custom_components/sat/const.py (2)
CycleClassification(188-194)CycleKind(181-185)custom_components/sat/cycles.py (4)
CycleStatistics(67-72)Cycle(40-63)duty_ratio_last_15m(125-138)sample_count_4h(106-112)custom_components/sat/helpers.py (1)
clamp(90-95)
custom_components/sat/pwm.py (3)
custom_components/sat/const.py (1)
PWMStatus(220-223)custom_components/sat/boiler.py (1)
BoilerState(21-34)custom_components/sat/setpoint_adjuster.py (1)
reset(20-22)
custom_components/sat/boiler.py (2)
custom_components/sat/const.py (2)
BoilerStatus(197-217)CycleClassification(188-194)custom_components/sat/cycles.py (1)
Cycle(40-63)
custom_components/sat/climate.py (4)
custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/const.py (3)
RelativeModulationState(226-230)BoilerStatus(197-217)PWMStatus(220-223)custom_components/sat/util.py (1)
create_dynamic_minimum_setpoint_controller(48-54)custom_components/sat/pwm.py (4)
state(239-244)enable(83-137)disable(139-142)status(235-236)
custom_components/sat/area.py (1)
custom_components/sat/pid.py (4)
update(66-83)update_reset(85-99)output(264-266)reset(49-64)
custom_components/sat/binary_sensor.py (3)
custom_components/sat/const.py (2)
BoilerStatus(197-217)CycleClassification(188-194)custom_components/sat/coordinator.py (2)
device_status(133-135)last_cycle(157-158)custom_components/sat/cycles.py (1)
last_cycle(156-157)
🪛 Pylint (4.0.3)
custom_components/sat/minimum_setpoint.py
[refactor] 23-23: Too many instance attributes (19/7)
(R0902)
[refactor] 105-106: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 116-116: Too many arguments (6/5)
(R0913)
[refactor] 116-116: Too many positional arguments (6/5)
(R0917)
[refactor] 140-140: Too many arguments (6/5)
(R0913)
[refactor] 140-140: Too many positional arguments (6/5)
(R0917)
[refactor] 279-279: Too many arguments (6/5)
(R0913)
[refactor] 279-279: Too many positional arguments (6/5)
(R0917)
[refactor] 279-279: Too many return statements (10/6)
(R0911)
[refactor] 279-279: Too many branches (13/12)
(R0912)
custom_components/sat/pwm.py
[refactor] 36-36: Too many instance attributes (19/7)
(R0902)
custom_components/sat/boiler.py
[refactor] 21-21: Too many instance attributes (8/7)
(R0902)
[refactor] 37-37: Too many instance attributes (24/7)
(R0902)
[refactor] 38-38: Too many arguments (14/5)
(R0913)
[refactor] 38-38: Too many positional arguments (14/5)
(R0917)
[refactor] 152-152: Too many return statements (18/6)
(R0911)
[refactor] 152-152: Too many branches (17/12)
(R0912)
[refactor] 295-295: Too many return statements (7/6)
(R0911)
[refactor] 381-381: Too many return statements (7/6)
(R0911)
custom_components/sat/cycles.py
[refactor] 40-40: Too many instance attributes (16/7)
(R0902)
[refactor] 218-218: Too many instance attributes (8/7)
(R0902)
[refactor] 310-310: Too many local variables (23/15)
(R0914)
[refactor] 394-394: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/simulator/__init__.py
88-88: Unused method argument: timestamp
(ARG002)
88-88: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
88-88: Unused method argument: climate
(ARG002)
88-88: Unused method argument: pwm_state
(ARG002)
custom_components/sat/minimum_setpoint.py
116-116: Unused method argument: boiler_state
(ARG002)
138-138: f-string without any placeholders
Remove extraneous f prefix
(F541)
custom_components/sat/coordinator.py
14-14: from .const import * used; unable to detect undefined names
(F403)
263-263: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
263-263: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
331-331: Unused method argument: climate
(ARG002)
331-331: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
237-237: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
245-245: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
539-539: HEATING_MODE_ECO may be undefined, or defined from star imports
(F405)
613-613: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
627-627: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
631-631: BoilerStatus may be undefined, or defined from star imports
(F405)
635-635: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
683-683: f-string without any placeholders
Remove extraneous f prefix
(F541)
695-695: f-string without any placeholders
Remove extraneous f prefix
(F541)
709-709: f-string without any placeholders
Remove extraneous f prefix
(F541)
820-820: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
821-821: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
837-837: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
838-838: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
962-962: DEADBAND may be undefined, or defined from star imports
(F405)
962-962: DEADBAND may be undefined, or defined from star imports
(F405)
1018-1018: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/cycles.py
81-81: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
246-246: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (21)
custom_components/sat/const.py (3)
175-178: LGTM!The new event constants for cycle lifecycle are well-named and follow the existing naming convention.
181-195: LGTM!
CycleKindandCycleClassificationenums are well-structured with the(str, Enum)pattern, enabling clean serialization for Home Assistant state attributes.
226-238: LGTM!
RelativeModulationStatefollows the(str, Enum)pattern andUNHEALTHY_CYCLEScorrectly identifies the problematic cycle classifications for health detection.custom_components/sat/simulator/__init__.py (2)
88-112: Unused parameters are acceptable for interface compliance.The static analysis flags
timestamp,climate, andpwm_stateas unused (ARG002). This is expected since the simulator has its own internal state management and these parameters are only needed for API compatibility with the base class. No action needed.
88-88: Parameter order inconsistency is not causing practical issues.While the method signatures differ between the coordinator (base class) parameter order
(climate, pwm_state, timestamp)and simulator (subclass) order(timestamp, climate, pwm_state), all calls in the codebase use either keyword arguments or no arguments. No positional argument calls exist that would trigger the binding issue described in the review. The parameter order difference is a design inconsistency, but it does not result in the "incorrect argument binding" problem claimed.Likely an incorrect or invalid review comment.
custom_components/sat/binary_sensor.py (2)
27-29: LGTM!Good defensive check to ensure required keys exist before proceeding with entity setup.
190-192: LGTM!The boiler health check correctly uses the new
BoilerStatusenum.custom_components/sat/area.py (2)
181-188: LGTM!The
outputmethod correctly handles the case where no areas have valid heating curve values by falling back toMINIMUM_SETPOINT.
58-64: RemoveHVACMode.OFFfrom state check or clarify supported entity types.The check at line 61 includes
HVACMode.OFF("off") when filtering sensor states, which is contradictory given the constant nameSENSOR_TEMPERATURE_IDand docstring say "sensor." Temperature sensors never have state "off"—only climate entities do.Two options:
- If only temperature sensors should be supported: remove
HVACMode.OFFfrom the check (it's unnecessary and misleading).- If both temperature sensors AND climate entities are supported: rename the constant to something like
TEMPERATURE_OVERRIDE_ENTITY_IDor add documentation clarifying which entity types are acceptable. Additionally, add a config schema constraint to enforce the supported domains.Currently, the code accepts any entity_id without validation, creating ambiguity about intent.
custom_components/sat/pwm.py (2)
17-34: LGTM!Well-designed immutable dataclasses using
frozen=Trueandslots=Truefor efficiency.PWMStatecleanly encapsulates the observable state, andPWMConfigcleanly encapsulates configuration.
238-244: LGTM!The
stateproperty correctly wraps internal state into an immutablePWMStateobject, with proper handling of the optionallast_duty_cycle_percentage.custom_components/sat/coordinator.py (2)
102-104: LGTM: Clean initialization of cycle-centric components.The new
Boiler,CycleHistory, andCycleTrackercomposition replaces the old flame-centric tracking cleanly. The wiring is straightforward and the responsibility separation is clear.
152-158: LGTM: Newcyclesandlast_cycleproperties.Clean property implementations that delegate to the underlying
CycleHistory. The API surface aligns well with the cycle-centric architecture.custom_components/sat/boiler.py (2)
152-227: LGTM: Comprehensive and well-structured status derivation.The
_derive_statusmethod provides a clear decision tree for determining boiler status. The ordering of checks (OFF → overshoot cooling → anti-cycling → stalled ignition → etc.) follows logical precedence. While Pylint flags many return statements, this is acceptable given the nature of state machine logic.
289-293: Potential negative time delta not guarded correctly.Line 290-291 checks
if time_since_off < 0: return False, but this check is redundant sincetime_since_off < self._anti_cycling_min_off_seconds(Line 293) would also beFalsefor negative values less than the positive threshold. However, this is fine as defensive coding.custom_components/sat/climate.py (2)
584-638: LGTM: Well-structured PWM enable/disable logic.The refactored PWM logic with
_is_pwm_forced,_should_enable_static_pwm, and_should_enable_dynamic_pwmhelpers improves readability and maintainability. The decision tree is clear and the conditions are logically sound.
1010-1018: Low-pass filter and clamping logic is correct.The exponential moving average filter (
alpha = 0.2) smooths the requested setpoint appropriately, and the subsequent clamping ensures bounds are respected.custom_components/sat/cycles.py (4)
75-168: LGTM: CycleHistory implements robust rolling window statistics.The
CycleHistoryclass correctly manages rolling windows for cycle tracking with proper pruning, duty ratio calculation, and median computation. Thecycles_last_hourproperty now correctly normalizes by window duration (Line 122).
254-284: LGTM: OFF→ON transition handling with demand duration tracking.The cycle start detection correctly computes
off_with_demand_durationby checking if demand was present during the OFF period. The logic properly resets tracking when demand isn't present.
393-437: Classification logic is sound with good edge case handling.The
_classify_cyclemethod handles the various scenarios correctly:
- Returns
INSUFFICIENT_DATAwhen duration or setpoint is missing- Returns
UNCERTAINfor short burns without temperature data- Correctly identifies overshoot/underheat conditions
- Short-cycling detection uses appropriate thresholds
The PWM-aware short threshold adjustment (Lines 413-414) is a nice touch.
331-338: Cycle kind thresholds could miss edge cases.The thresholds for determining
CycleKinduse> 0.8and< 0.2with an intermediate check for> 0.1. Consider what happens when both fractions are exactly0.1— this falls through toUNKNOWN. This seems intentional but verify it matches the expected behavior.
| async def async_added_to_hass(self, hass: HomeAssistant, device_id: str) -> None: | ||
| """Called when entity is added to Home Assistant, restore persisted flags.""" | ||
| if self._store is None: | ||
| self._store = Store(hass, STORAGE_VERSION, f"sat.boiler.{device_id}") | ||
|
|
||
| if not flame_active: | ||
| self._handle_flame_inactive() | ||
| elif self._adjusting_to_lower_setpoint: | ||
| self._handle_adjusting_to_lower_setpoint(boiler_temperature, boiler_temperature_derivative, setpoint) | ||
| elif self._active: | ||
| self._handle_tracking(boiler_temperature, boiler_temperature_derivative, setpoint) | ||
| data = await self._store.async_load() or {} | ||
| stored_flag = data.get("modulation_reliable") | ||
| if stored_flag is not None: | ||
| self._modulation_reliable = bool(stored_flag) | ||
|
|
||
| self._last_setpoint = setpoint | ||
| self._last_boiler_temperature = boiler_temperature | ||
| async_track_time_interval(hass, self.async_save_options, timedelta(minutes=15)) |
There was a problem hiding this comment.
Unsubscribe handle for periodic save is not stored or cleaned up.
async_track_time_interval returns an unsubscribe callback that should be stored and called during cleanup. Currently, the periodic save will continue even if the boiler is removed.
Apply this diff:
+ self._save_unsub: Optional[Callable[[], None]] = None
...
- async_track_time_interval(hass, self.async_save_options, timedelta(minutes=15))
+ self._save_unsub = async_track_time_interval(hass, self.async_save_options, timedelta(minutes=15))Then add cleanup in async_save_options or a new async_will_remove_from_hass:
if self._save_unsub is not None:
self._save_unsub()
self._save_unsub = None
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
custom_components/sat/cycles.py (1)
124-138: Minor: Replace EN DASH with HYPHEN-MINUS in docstring.The docstring uses an EN DASH (
–) instead of a standard HYPHEN-MINUS (-) which can cause linting warnings (Ruff RUF002).@property def duty_ratio_last_15m(self) -> float: - """Duty ratio (0.0–1.0) over the duty window, derived from recorded cycles.""" + """Duty ratio (0.0-1.0) over the duty window, derived from recorded cycles."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/cycles.py(1 hunks)
🧰 Additional context used
🪛 Pylint (4.0.3)
custom_components/sat/cycles.py
[refactor] 40-40: Too many instance attributes (16/7)
(R0902)
[refactor] 218-218: Too many instance attributes (8/7)
(R0902)
[refactor] 310-310: Too many local variables (23/15)
(R0914)
[refactor] 394-394: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/cycles.py
81-81: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
246-246: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (7)
custom_components/sat/cycles.py (7)
1-31: LGTM!Clean imports with proper
TYPE_CHECKINGguard to avoid circular dependencies. Constants are well-documented with clear thresholds for cycle classification logic.
39-64: LGTM!Well-structured frozen dataclass representing cycle metrics. The 16 attributes are justified for a comprehensive DTO capturing all relevant cycle statistics.
393-437: LGTM!The classification logic handles various cycle scenarios with clear decision boundaries. The multiple return statements create a readable decision tree that covers edge cases appropriately (e.g., falling back to
UNCERTAINwhen both overshoot and underheat conditions are ambiguous).
310-391: LGTM!The cycle building logic correctly aggregates samples, handles
Nonevalues gracefully in averages and min/max calculations, and uses sensible thresholds for kind classification. The local_avghelper is a clean pattern for filtered averaging.
254-308: LGTM!The state machine correctly handles all flame transitions. The OFF→ON transition properly captures off-with-demand duration before starting a new cycle, and the ON→OFF transition finalizes and records the cycle. The demand detection logic (lines 262-268) appropriately excludes DHW periods.
75-216: LGTM!
CycleHistoryis well-implemented with proper validation, efficientdequeusage for rolling windows, and correct pruning semantics. The_current_time_hintpattern elegantly handles the absence of a system clock by deriving time from recorded data.
413-414: No issues found withduty_cycleaccess.The type definition
Optional[Tuple[int, int]]guarantees that whenduty_cycleis not None (checked by the truthy condition), it always contains exactly 2 elements. Therefore, accessingpwm_state.duty_cycle[0]is safe and no additional length check is needed.Likely an incorrect or invalid review comment.
… heating loop control, and clean up unused lines.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
custom_components/sat/area.py (1)
163-179: PotentialNonepassed toPID.updatewherefloatis expected.The guard at lines 168 and 177 only checks
area.error is not Nonebut notarea.heating_curve.value. Sinceheating_curve.valuecan beNone(asState.valueisOptional[float]), andPID.updateexpects afloat, this can cause a type error.Apply this diff to add the guard:
def update(self, entity_id: str) -> None: if (area := self.get(entity_id)) is None: _LOGGER.warning(f"Could not update PID controller for entity {entity_id}. Areas: {self._areas}.") return - if area.error is not None: + if area.error is not None and area.heating_curve.value is not None: _LOGGER.info(f"Updating error to {area.error.value} from {area.id} (Reset: False)") area.pid.update(area.error, area.heating_curve.value) def update_reset(self, entity_id: str) -> None: if (area := self.get(entity_id)) is None: _LOGGER.warning(f"Could not update PID controller for entity {entity_id}") return - if area.error is not None: + if area.error is not None and area.heating_curve.value is not None: _LOGGER.info(f"Updating error to {area.error.value} from {area.id} (Reset: True)") area.pid.update_reset(area.error, area.heating_curve.value)custom_components/sat/cycles.py (2)
246-246: Fix implicit Optional type annotation.The
timestampparameter defaults toNonebut is typed asfloat, violating PEP 484.Apply this diff:
- def update(self, boiler_state: BoilerState, pwm_state: Optional[PWMState] = None, timestamp: float = None) -> None: + def update(self, boiler_state: BoilerState, pwm_state: Optional[PWMState] = None, timestamp: Optional[float] = None) -> None:
297-298: Convert Cycle dataclass to dict before firing event.Home Assistant's event bus requires JSON-serializable data. The frozen
Cycledataclass will fail serialization during event persistence.Apply this fix:
+from dataclasses import asdict + ... - cycle_state = self._build_cycle_state(timestamp) - self._hass.bus.fire(EVENT_SAT_CYCLE_ENDED, {"cycle": cycle_state}) + cycle_state = self._build_cycle_state(timestamp) + self._hass.bus.fire(EVENT_SAT_CYCLE_ENDED, {"cycle": asdict(cycle_state)})custom_components/sat/minimum_setpoint.py (4)
28-35: Fix inverted comments and use the unused duty ratio parameter.The comments for
increase_stepanddecrease_stepare inverted relative to their actual usage in_maybe_tune_minimum. Also,low_load_maximum_duty_ratio_15mis defined but never checked.Apply these fixes:
- # How quickly the learned minimum moves when we detect a clear error - increase_step: float = 1.0 # when minimum is too low (underheat / too short) - decrease_step: float = 1.0 # when minimum is too high (overshoot / short-cycling) + # How quickly the learned minimum moves when we detect a clear error + increase_step: float = 1.0 # raise minimum when overshooting or short-cycling + decrease_step: float = 1.0 # lower minimum when underheatingAnd in
_is_tunable_regime(after line 385), add:if statistics.last_hour_count < self._config.low_load_minimum_cycles_per_hour: return False + + if statistics.duty_ratio_last_15m > self._config.low_load_maximum_duty_ratio_15m: + return False return True
137-137: Fix malformed f-string.The f-string uses C-style format specifiers without actual placeholders.
Apply this diff:
- _LOGGER.debug(f"Updated regime %s minimum_setpoint=%.1f after starvation.", regime_state.minimum_setpoint) + _LOGGER.debug("Updated regime minimum_setpoint=%.1f after starvation.", regime_state.minimum_setpoint)
221-221: Fix return type annotation.
_regime_foralways returns aRegimeState(creating one if missing), so the return type should not beOptional.- def _regime_for(self, cycles: CycleStatistics, requested_setpoint: float, outside_temperature: Optional[float]) -> Optional[RegimeState]: + def _regime_for(self, cycles: CycleStatistics, requested_setpoint: float, outside_temperature: Optional[float]) -> RegimeState:
257-275: Fix regime key parsing bug.The code attempts to extract the setpoint band with
split(":", 1)[1], but for keys like"10:cold:normal", this returns"cold:normal", causingint()to fail.Apply this diff:
def _initial_minimum_for_regime(self, regime_key: str, requested_setpoint: float) -> float: # If we already have regimes, reuse the nearest one (unchanged) if self._regimes: try: - target_bucket = int(regime_key.split(":", 1)[1]) + target_bucket = int(regime_key.split(":", 1)[0]) except (IndexError, ValueError): target_bucket = 0 def bucket_of(key: str) -> int: try: - return int(key.split(":", 1)[1]) + return int(key.split(":", 1)[0]) except (IndexError, ValueError): return 0custom_components/sat/climate.py (4)
683-683: Remove extraneous f-string prefix.The f-string has no placeholders and uses %-style formatting instead.
- _LOGGER.debug(f"Inside sensor changed (%.2f°C).", float(new_state.state)) + _LOGGER.debug("Inside sensor changed (%.2f°C).", float(new_state.state))
695-695: Remove extraneous f-string prefix.- _LOGGER.debug(f"Outside sensor changed (%.2f°C).", self.current_outside_temperature) + _LOGGER.debug("Outside sensor changed (%.2f°C).", self.current_outside_temperature)
709-709: Remove extraneous f-string prefix.- _LOGGER.debug(f"Humidity sensor changed (%.2f%%).", float(new_state.state)) + _LOGGER.debug("Humidity sensor changed (%.2f%%).", float(new_state.state))
912-928: Fix async_call_later to use sync callback that schedules coroutine.Home Assistant's
async_call_laterrequires a sync callback. Passing an async function directly toHassJobwill fail.Apply this fix:
- self._control_pid_unsub = async_call_later(self.hass, 10, HassJob(self.async_control_pid)) + def _schedule_pid(): + self.hass.async_create_task(self.async_control_pid()) + + self._control_pid_unsub = async_call_later(self.hass, 10, _schedule_pid)
🧹 Nitpick comments (2)
custom_components/sat/area.py (2)
89-93: Redundant multiplication by 1.0.
raw_weight = effective_delta * 1.0is a no-op. If this is a placeholder for a configurable weight factor, consider adding a named constant or comment. Otherwise, simplify:delta = target_temperature - current_temperature effective_delta = max(delta - 0.2, 0.0) - raw_weight = effective_delta * 1.0 - - return round(max(0.0, min(raw_weight, 2.0)), 3) + return round(max(0.0, min(effective_delta, 2.0)), 3)
156-161: Consider using a dictionary for O(1) lookups.The
get()method performs a linear search. For typical home setups with few areas, this is fine. If the number of areas grows, consider using adict[str, Area]keyed byentity_idfor O(1) lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
custom_components/sat/area.py(6 hunks)custom_components/sat/climate.py(24 hunks)custom_components/sat/cycles.py(1 hunks)custom_components/sat/minimum_setpoint.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
custom_components/sat/area.py (3)
custom_components/sat/state.py (1)
State(10-12)custom_components/sat/helpers.py (1)
float_value(60-69)custom_components/sat/errors.py (2)
Error(37-40)max(29-33)
custom_components/sat/minimum_setpoint.py (4)
custom_components/sat/boiler.py (1)
BoilerState(21-34)custom_components/sat/const.py (2)
CycleClassification(188-194)CycleKind(181-185)custom_components/sat/cycles.py (1)
Cycle(40-63)custom_components/sat/helpers.py (1)
clamp(90-95)
custom_components/sat/cycles.py (4)
custom_components/sat/const.py (2)
CycleKind(181-185)CycleClassification(188-194)custom_components/sat/helpers.py (1)
clamp(90-95)custom_components/sat/pwm.py (2)
PWMState(18-24)enabled(231-232)custom_components/sat/boiler.py (1)
BoilerState(21-34)
custom_components/sat/climate.py (4)
custom_components/sat/helpers.py (2)
convert_time_str_to_seconds(19-28)clamp(90-95)custom_components/sat/coordinator.py (5)
get(408-423)minimum_setpoint(261-263)state(138-150)setpoint(166-167)update(44-49)custom_components/sat/minimum_setpoint.py (2)
reset(109-113)value(89-107)custom_components/sat/heating_curve.py (4)
reset(21-25)value(91-92)update(27-30)autotune(37-63)
🪛 Pylint (4.0.3)
custom_components/sat/minimum_setpoint.py
[refactor] 23-23: Too many instance attributes (19/7)
(R0902)
[refactor] 104-105: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 115-115: Too many arguments (6/5)
(R0913)
[refactor] 115-115: Too many positional arguments (6/5)
(R0917)
[refactor] 139-139: Too many arguments (6/5)
(R0913)
[refactor] 139-139: Too many positional arguments (6/5)
(R0917)
[refactor] 276-276: Too many arguments (6/5)
(R0913)
[refactor] 276-276: Too many positional arguments (6/5)
(R0917)
[refactor] 276-276: Too many return statements (10/6)
(R0911)
[refactor] 276-276: Too many branches (13/12)
(R0912)
custom_components/sat/cycles.py
[refactor] 40-40: Too many instance attributes (16/7)
(R0902)
[refactor] 218-218: Too many instance attributes (8/7)
(R0902)
[refactor] 310-310: Too many local variables (23/15)
(R0914)
[refactor] 394-394: Too many return statements (8/6)
(R0911)
🪛 Ruff (0.14.5)
custom_components/sat/minimum_setpoint.py
115-115: Unused method argument: boiler_state
(ARG002)
137-137: f-string without any placeholders
Remove extraneous f prefix
(F541)
custom_components/sat/cycles.py
81-81: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
246-246: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
custom_components/sat/climate.py
180-180: CONF_MODE may be undefined, or defined from star imports
(F405)
180-180: MODE_SWITCH may be undefined, or defined from star imports
(F405)
180-180: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
237-237: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
245-245: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
539-539: HEATING_MODE_ECO may be undefined, or defined from star imports
(F405)
613-613: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
627-627: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
631-631: BoilerStatus may be undefined, or defined from star imports
(F405)
635-635: BOILER_DEADBAND may be undefined, or defined from star imports
(F405)
683-683: f-string without any placeholders
Remove extraneous f prefix
(F541)
695-695: f-string without any placeholders
Remove extraneous f prefix
(F541)
709-709: f-string without any placeholders
Remove extraneous f prefix
(F541)
820-820: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
821-821: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
837-837: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
838-838: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
962-962: DEADBAND may be undefined, or defined from star imports
(F405)
962-962: DEADBAND may be undefined, or defined from star imports
(F405)
1018-1018: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
🔇 Additional comments (23)
custom_components/sat/area.py (5)
1-20: LGTM!Imports and module-level constants are appropriate. The logging setup follows standard patterns.
36-50: LGTM!Properties properly handle
Nonecases and usefloat_valuefor safe type conversion.
59-64: Questionable fallback logic and HVACMode check.Two concerns:
Line 61: Comparing
sensor_state.stateagainstHVACMode.OFFseems semantically incorrect.HVACMode.OFFis an HVAC mode enum value, not a typical sensor state. If the intent is to check for "off" string state, use the string literal directly.Line 64: Falling back to
target_temperaturewhencurrent_temperatureis unavailable may mask issues. The error calculation (target - current) would incorrectly be 0, potentially causing incorrect PID behavior.Consider this adjustment for line 61:
- if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]: + if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, "off"]:For line 64, verify if fallback to target temperature is intentional behavior when current temperature is unavailable.
98-104: LGTM!Proper null guards before calling
pid.update_integral, preventing potential type errors.
181-188: LGTM!The
output()method correctly filters out areas withNoneheating curve values before computing the maximum, providing a safe fallback toMINIMUM_SETPOINT.custom_components/sat/cycles.py (5)
1-31: LGTM: Module setup and constants are well-defined.The imports are appropriate, constants are clearly named with sensible defaults, and the module structure is clean.
33-73: LGTM: Dataclass design is appropriate.The
CycleSample,Cycle, andCycleStatisticsdataclasses are well-structured withfrozen=Trueandslots=Truefor immutability and memory efficiency. WhileCyclehas 16 attributes (triggering a Pylint warning), they are all logically related fields for representing a complete heating cycle, making this an appropriate data transfer object.
75-216: LGTM: CycleHistory implementation is robust.The rolling window management is well-implemented:
- Proper validation of window parameters
- Correct normalization of
cycles_last_hourto cycles per hour (line 122)- Efficient pruning of stale data
- Clean separation of concerns between different rolling windows
The static analysis hints about exception messages (TRY003) are minor style suggestions that can be addressed later if desired.
218-309: LGTM: Cycle detection logic is well-structured.The state machine for tracking flame transitions (OFF→ON, ON→ON, ON→OFF) is clear and correct:
- Properly records off-with-demand duration before cycle starts
- Accumulates samples during ON phase
- Finalizes and records cycle on OFF transition
- Fires appropriate events for cycle lifecycle
The cycle building and classification logic is comprehensive and handles edge cases appropriately.
310-434: LGTM: Cycle building and classification logic is comprehensive.The
_build_cycle_statemethod correctly:
- Validates minimum sample requirements
- Computes cycle kind based on DHW/heating fractions
- Calculates accurate averages and extremes
- Handles None values appropriately
The
_classify_cyclemethod provides robust classification with appropriate handling of:
- Insufficient data scenarios
- PWM-aware short burn thresholds
- Low-load short-cycling detection
- Overshoot vs. underheat detection
custom_components/sat/minimum_setpoint.py (5)
1-21: LGTM: Module setup is clean.Imports are appropriate and STORAGE_VERSION is properly defined for future migration handling.
67-114: LGTM: Initialization and value property are well-designed.The regime management and value calculation logic is sound:
- Properly initializes state
- Returns active regime minimum with appropriate fallbacks
- Includes safety guard against excessive drift from recent base setpoints
The Pylint suggestion at lines 104-105 (using
max()instead of an if-block) is a minor style optimization that can be addressed later.
139-156: LGTM: on_cycle_end integrates well with event-driven architecture.The method correctly:
- Determines the active regime
- Handles large setpoint jumps with damping
- Increments cycle counter
- Delegates to tuning logic
Unlike previous versions that called this from a polling loop, the current event-driven design (triggered by
EVENT_SAT_CYCLE_ENDED) ensures this method is invoked once per cycle, eliminating the repeated-tuning concern from earlier reviews.
157-220: LGTM: Persistence logic is robust.The storage implementation correctly:
- Initializes Store with versioning support
- Validates and clamps loaded data
- Handles missing or invalid fields gracefully
- Schedules periodic saves every 15 minutes
276-427: LGTM: Tuning logic is comprehensive and well-reasoned.The
_maybe_tune_minimummethod correctly:
- Respects warmup periods before tuning
- Validates cycle suitability (heating-dominant, close to current minimum)
- Applies appropriate adjustments based on classification:
- Lowers minimum for underheat (setpoint too high)
- Raises minimum for overshoot/short-cycling (setpoint too low)
- Relaxes toward base for uncertain cycles
- Includes safeguards against excessive drift
The helper methods (
_is_tunable_regime,_relax_toward_base,_maybe_damp_on_large_jump) provide clean separation of concerns. The Pylint warnings about complexity are acceptable given the nuanced logic required for adaptive setpoint control.custom_components/sat/climate.py (8)
1-202: LGTM: Climate entity initialization is well-structured.The setup correctly:
- Imports necessary dependencies including
partialfor deferred execution- Initializes the dynamic minimum setpoint controller
- Creates PWM controller with heating curve dependency
- Sets up debouncing infrastructure with unsub handles
- Properly configures preset modes and supported features
237-251: LGTM: Cycle event listeners correctly integrate minimum setpoint controller.The event-driven approach properly:
- Listens for cycle start/end events
- Passes all required context (boiler state, statistics, cycle, setpoint, temperature)
- Uses sync lambdas calling sync methods (no coroutine issues)
262-382: LGTM: Event listener registration and state restoration are correct.The lifecycle management properly:
- Registers all necessary state change listeners
- Handles missing or unavailable sensors gracefully
- Restores previous state with sensible fallbacks
- Tracks coordinator updates appropriately
584-639: LGTM: PWM enablement logic is well-structured and comprehensive.The refactored decision logic cleanly separates concerns:
_is_pwm_forced: Checks for relay-only mode or explicit forcing_should_enable_static_pwm: Handles legacy fixed minimum setpoint with hysteresis_should_enable_dynamic_pwm: Uses cycle history and classification for intelligent enablementThe dynamic logic appropriately enables PWM for unhealthy cycles, stalled ignition, or when near the learned minimum, while maintaining the current state when no new information is available.
686-687: LGTM: Sensor change handlers correctly use debounced scheduling.The handlers properly call
schedule_control_pid()instead of directly invoking async methods, ensuring debounced PID updates to reduce computational overhead from rapid sensor changes.Also applies to: 701-701, 712-713
813-890: LGTM: Control methods integrate well with new architecture.The refactored methods correctly:
- Use
_last_requested_setpointfor setpoint decisions- Apply PWM-aware logic for normal vs. minimum setpoints
- Reset both PWM and minimum setpoint controllers when needed
- Handle relative modulation with manufacturer-specific adjustments
930-1042: LGTM: PID and heating loop control methods are well-integrated.The refactored control flow correctly:
- Implements debounced execution with proper unsub management
- Validates required inputs before proceeding
- Applies low-pass filtering to requested setpoint to reduce oscillation
- Integrates PWM, minimum setpoint, and coordinator state appropriately
- Handles reset scenarios by clearing
_last_requested_setpoint- Uses autotuning when in deadband range
The separation between
async_control_pid(error calculation) andasync_control_heating_loop(setpoint application) is clean and logical.
1077-1184: LGTM: HVAC mode and temperature setpoint methods handle resets correctly.The methods properly:
- Reset PID controllers and minimum setpoint state on mode/temperature changes
- Use
schedule_control_pid(reset=True)for immediate reset execution- Cascade changes to controlled climates when appropriate
- Maintain preset mode synchronization
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
custom_components/sat/minimum_setpoint.py (4)
28-34: Config comments are inverted relative to actual usage;low_load_maximum_duty_ratio_15mis unused.The comments on
increase_stepanddecrease_stepdescribe the opposite of what the code does:
increase_stepis applied for overshoot/short-cycling (to raise the minimum)decrease_stepis applied for underheat (to lower the minimum)Additionally,
low_load_maximum_duty_ratio_15mis defined but never used in_is_tunable_regime.
221-221: Return type annotation is incorrect.The method always returns a
RegimeState(creating one if needed), but is annotated asOptional[RegimeState].- def _regime_for(self, cycles: CycleStatistics, requested_setpoint: float, outside_temperature: Optional[float]) -> Optional[RegimeState]: + def _regime_for(self, cycles: CycleStatistics, requested_setpoint: float, outside_temperature: Optional[float]) -> RegimeState:
257-274: Bug: Incorrect index when extracting setpoint band from regime key.Given the key format
"{setpoint_band}:{temp_band}:{load_band}"(e.g.,"10:cold:normal"),split(":", 1)[1]returns"cold:normal", not the setpoint band. This causesint()to always fail, making the nearest-bucket lookup ineffective.if self._regimes: try: - target_bucket = int(regime_key.split(":", 1)[1]) + target_bucket = int(regime_key.split(":", 1)[0]) except (IndexError, ValueError): target_bucket = 0 def bucket_of(key: str) -> int: try: - return int(key.split(":", 1)[1]) + return int(key.split(":", 1)[0]) except (IndexError, ValueError): return 0
137-137: Malformed f-string: missing placeholders.The f-string uses C-style
%sand%.1fformat specifiers instead of f-string placeholders. This will log a literal%sinstead of the regime key.- _LOGGER.debug(f"Updated regime %s minimum_setpoint=%.1f after starvation.", regime_state.minimum_setpoint) + _LOGGER.debug("Updated regime %s minimum_setpoint=%.1f after starvation.", self._active_regime_key, regime_state.minimum_setpoint)Note: Using standard
%-style formatting with_LOGGER.debugis preferred for lazy evaluation.
🧹 Nitpick comments (3)
custom_components/sat/minimum_setpoint.py (3)
101-107: Consider simplifying withmax().The conditional assignment can be simplified.
# Additional guard: do not allow minimum to drift too far below recent bases. if self._last_requested_setpoint is not None: allowed_minimum = max(self._config.minimum_setpoint, self._last_requested_setpoint - self._config.max_deviation_from_recent_base) - if minimum_setpoint < allowed_minimum: - minimum_setpoint = allowed_minimum + minimum_setpoint = max(minimum_setpoint, allowed_minimum) return self._clamp_setpoint(minimum_setpoint)
165-167: Consider rejecting data on version mismatch or documenting migration strategy.When
version != STORAGE_VERSION, the code logs a debug message but continues loading the data anyway. If the storage format changes in a future version, this could lead to subtle bugs or incorrect state restoration.Consider either:
- Clearing regimes and starting fresh on version mismatch
- Implementing explicit migration logic for version upgrades
version = data.get("version") if version != STORAGE_VERSION: _LOGGER.debug("Unknown minimum setpoint storage version: %s", version) + return # Don't load incompatible data
115-117: Consider using or removing unusedboiler_stateparameter.The
boiler_stateparameter is unused inon_cycle_start. If this is intentional for API consistency withon_cycle_end, consider prefixing with underscore (_boiler_state) to signal intent.- def on_cycle_start(self, boiler_state: BoilerState, cycles: CycleStatistics, last_cycle: Optional[Cycle], requested_setpoint: Optional[float], outside_temperature: Optional[float]) -> None: + def on_cycle_start(self, _boiler_state: BoilerState, cycles: CycleStatistics, last_cycle: Optional[Cycle], requested_setpoint: Optional[float], outside_temperature: Optional[float]) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/minimum_setpoint.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/minimum_setpoint.py (4)
custom_components/sat/boiler.py (1)
BoilerState(21-34)custom_components/sat/const.py (2)
CycleClassification(188-194)CycleKind(181-185)custom_components/sat/cycles.py (2)
Cycle(40-63)reset(239-244)custom_components/sat/helpers.py (1)
clamp(90-95)
🪛 Pylint (4.0.3)
custom_components/sat/minimum_setpoint.py
[refactor] 23-23: Too many instance attributes (19/7)
(R0902)
[refactor] 104-105: Consider using 'minimum_setpoint = max(minimum_setpoint, allowed_minimum)' instead of unnecessary if block
(R1731)
[refactor] 115-115: Too many arguments (6/5)
(R0913)
[refactor] 115-115: Too many positional arguments (6/5)
(R0917)
[refactor] 139-139: Too many arguments (6/5)
(R0913)
[refactor] 139-139: Too many positional arguments (6/5)
(R0917)
[refactor] 276-276: Too many arguments (6/5)
(R0913)
[refactor] 276-276: Too many positional arguments (6/5)
(R0917)
[refactor] 276-276: Too many return statements (10/6)
(R0911)
[refactor] 276-276: Too many branches (13/12)
(R0912)
🪛 Ruff (0.14.5)
custom_components/sat/minimum_setpoint.py
115-115: Unused method argument: boiler_state
(ARG002)
137-137: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (9)
custom_components/sat/minimum_setpoint.py (9)
1-20: LGTM!Imports are well-organized with proper separation between standard library, Home Assistant, and local imports. Storage versioning is correctly initialized.
67-70: LGTM!Simple, focused dataclass for regime state tracking. Appropriate use of
slots=Truefor memory efficiency.
139-155: LGTM! Event-driven design addresses previous repeated-tuning concern.The refactor to
on_cycle_end(called once per cycle completion) properly addresses the previous concern about repeated tuning on the same cycle. The completed_cycles increment and tuning now happen exactly once per cycle.
201-219: LGTM!Clean serialization with proper null-check on store.
236-255: LGTM!Clear regime key construction with well-defined bands for setpoint, temperature, and load conditions.
276-368: Method is complex but logically sound.The tuning logic correctly handles each
CycleClassification:
GOOD/INSUFFICIENT_DATA: no change (stable)TOO_SHORT_UNDERHEAT: decrease minimum (with modulation guard)TOO_SHORT_OVERSHOOT/SHORT_CYCLING_OVERSHOOT: increase minimumUNCERTAIN: relax toward baseWhile static analysis flags complexity (too many returns/branches), the early-return pattern is appropriate for this decision tree.
370-384: LGTM!Tuning eligibility checks are appropriate. Note: The unused
low_load_maximum_duty_ratio_15mconfig parameter was flagged earlier.
386-401: LGTM!Proper exponential relaxation toward the base setpoint with appropriate factor validation.
403-425: LGTM!Jump detection prevents aggressive minimum application when transitioning between regimes. Clean delegation to the
clamphelper.
…ates, and improve sensor tracking logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
custom_components/sat/area.py (2)
92-92: Remove climate-specific enum from sensor state validation.Line 92 checks for
HVACMode.OFFwhen validating sensor states, butsensor_temperature_idreferences sensor entities (not climate entities). Sensors don't have HVAC modes. If you need to filter an "off" state from sensors, use the string literal"off"instead.🔎 Proposed fix
- if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]: + if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]:
203-203: Wrapupdate()call in event handler to match expected signature.
EVENT_HOMEASSISTANT_STARTEDhandlers receive anEventobject, butupdate()expectsOptional[datetime]. This signature mismatch will cause a runtime error.🔎 Proposed fix
- hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STARTED, self.update) + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STARTED, lambda _event: self.update())tests/test_pid.py (1)
85-105: Test validates buggy behavior:automatic_gain_valueparameter is ignored.The test passes
automatic_gain_value=2.0(line 90), but the PID implementation atcustom_components/sat/pid.pyline 51 ignores this parameter and hardcodes the value as 4 for underfloor heating. The test expectskp=20.0, which matches the buggy calculation:(2.0 * 40.0) / 4 = 20.0.When the bug in
pid.pyis fixed to use the constructor parameter, this test will fail. The expected value should then bekp=40.0(i.e.,(2.0 * 40.0) / 2.0 = 40.0).🔎 Expected fix (after pid.py is corrected)
pid.update(Error("sensor.test", 0.05), 5.0, heating_curve=40.0) - assert pid.kp == 20.0 - assert pid.ki == round(20.0 / 8400, 6) - assert pid.kd == round(0.07 * 8400 * 20.0, 6) + assert pid.kp == 40.0 + assert pid.ki == round(40.0 / 8400, 6) + assert pid.kd == round(0.07 * 8400 * 40.0, 6)custom_components/sat/pid.py (1)
26-52: Constructor parameterautomatic_gain_valueis stored but never used.Line 32 stores the
automatic_gain_valueparameter inself._automatic_gains_value, but line 51 hardcodes the value as4or3based on the heating system, completely ignoring the stored parameter. This means users configuringautomatic_gain_valuewill see no effect.Either use the stored parameter or remove it from the constructor signature.
🔎 Option 1: Use the constructor parameter
if self._heating_curve is None: return 0.0 - automatic_gain_value = 4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3 + # Use configured value, falling back to default based on heating system + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3) return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)🔎 Option 2: Remove the unused parameter
- def __init__(self, heating_system: str, automatic_gain_value: float, heating_curve_coefficient: float, kp: float, ki: float, kd: float, automatic_gains: bool = False) -> None: + def __init__(self, heating_system: str, heating_curve_coefficient: float, kp: float, ki: float, kd: float, automatic_gains: bool = False) -> None: self._kp: float = kp self._ki: float = ki self._kd: float = kd self._heating_system: str = heating_system self._automatic_gains: bool = automatic_gains - self._automatic_gains_value: float = automatic_gain_value self._heating_curve_coefficient: float = heating_curve_coefficientNote: This requires updating all call sites that pass
automatic_gain_value.
🧹 Nitpick comments (2)
custom_components/sat/area.py (2)
275-275: Add type annotation forareasparameter.The
areasparameter lacks a type annotation while other parameters are annotated. Based on usage, it should belist[Area].🔎 Proposed fix
- def __init__(self, areas, percentile: float = 0.75, headroom: float = 5.0): + def __init__(self, areas: list[Area], percentile: float = 0.75, headroom: float = 5.0):
290-294: Consider catching more specific exceptions in PID output aggregation.The broad
Exceptioncatch provides robustness but may hide unexpected bugs. Consider catching specific exceptions like(TypeError, ValueError, AttributeError)if you can anticipate the failure modes.🔎 Proposed refinement
- except Exception as exception: + except (TypeError, ValueError, AttributeError) as exception:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/sat/area.pycustom_components/sat/pid.pytests/test_pid.py
🧰 Additional context used
🧬 Code graph analysis (3)
custom_components/sat/pid.py (4)
custom_components/sat/state.py (1)
State(10-12)custom_components/sat/errors.py (1)
Error(37-40)custom_components/sat/helpers.py (1)
timestamp(12-14)custom_components/sat/climate.py (1)
error(430-438)
custom_components/sat/area.py (4)
custom_components/sat/heating_curve.py (3)
update(27-30)value(91-92)reset(21-25)custom_components/sat/helpers.py (4)
convert_time_str_to_seconds(40-49)is_state_stale(32-37)state_age_seconds(27-29)event_timestamp(17-19)custom_components/sat/pid.py (5)
PID(23-211)available(38-40)output(94-99)update(138-148)reset(101-115)custom_components/sat/climate.py (3)
current_temperature(411-419)target_temperature(406-408)error(430-438)
tests/test_pid.py (1)
custom_components/sat/pid.py (11)
PID(23-211)kp(43-52)ki(55-60)kd(63-68)integral(76-78)raw_derivative(89-91)derivative(81-86)output(94-99)update(138-148)proportional(71-73)restore(117-136)
🪛 Pylint (4.0.4)
custom_components/sat/pid.py
[refactor] 23-23: Too many instance attributes (15/7)
(R0902)
[refactor] 26-26: Too many arguments (8/5)
(R0913)
[refactor] 26-26: Too many positional arguments (8/5)
(R0917)
custom_components/sat/area.py
[refactor] 37-37: Too many instance attributes (8/7)
(R0902)
tests/test_pid.py
[refactor] 19-19: Too few public methods (1/2)
(R0903)
🪛 Ruff (0.14.10)
custom_components/sat/pid.py
8-8: from .const import * used; unable to detect undefined names
(F403)
51-51: HEATING_SYSTEM_UNDERFLOOR may be undefined, or defined from star imports
(F405)
154-154: DEADBAND may be undefined, or defined from star imports
(F405)
158-158: DEADBAND may be undefined, or defined from star imports
(F405)
186-186: DEADBAND may be undefined, or defined from star imports
(F405)
custom_components/sat/area.py
292-292: Do not catch blind exception: Exception
(BLE001)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
custom_components/sat/pid.py (1)
42-52: Constructor parameterautomatic_gain_valueis still being ignored.Line 51 hardcodes
automatic_gain_valuebased on heating system type instead of usingself._automatic_gains_value(stored at line 32). This issue was flagged in previous reviews but remains unresolved.When
automatic_gains=True, the configured value passed to the constructor is completely ignored, making the parameter effectively useless.🔎 Proposed fix
- automatic_gain_value = 4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3 + # Use configured value, falling back to default based on heating system + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3) return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)Alternatively, if the hardcoded logic is intentional, remove the unused constructor parameter and the stored attribute.
Based on learnings, this issue persists from previous reviews.
🧹 Nitpick comments (1)
custom_components/sat/pid.py (1)
150-175: Integral anti-windup logic is sound.The implementation correctly prevents integral windup by:
- Resetting the time base when entering the deadband (line 154)
- Zeroing integral when error exceeds deadband (lines 158-160)
- Clamping accumulated integral to curve bounds (lines 171-172)
This ensures integral action only occurs when close to the target, avoiding overshoot.
Minor: Redundant unary plus operator
Line 171 uses
float(+heating_curve_value)where the unary+is redundant:- self._integral = min(self._integral, float(+heating_curve_value)) + self._integral = min(self._integral, float(heating_curve_value))
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/pid.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/pid.py (2)
custom_components/sat/state.py (1)
State(10-12)custom_components/sat/helpers.py (1)
timestamp(12-14)
🪛 Pylint (4.0.4)
custom_components/sat/pid.py
[refactor] 23-23: Too many instance attributes (15/7)
(R0902)
[refactor] 26-26: Too many arguments (8/5)
(R0913)
[refactor] 26-26: Too many positional arguments (8/5)
(R0917)
🪛 Ruff (0.14.10)
custom_components/sat/pid.py
8-8: from .const import * used; unable to detect undefined names
(F403)
51-51: HEATING_SYSTEM_UNDERFLOOR may be undefined, or defined from star imports
(F405)
154-154: DEADBAND may be undefined, or defined from star imports
(F405)
158-158: DEADBAND may be undefined, or defined from star imports
(F405)
187-187: DEADBAND may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
custom_components/sat/pid.py (4)
101-116: LGTM!The reset logic correctly initializes all state variables, timing anchors, and accumulators to appropriate starting values.
117-137: LGTM!The restore logic correctly reconstructs PID state from saved attributes and resets timing anchors to the current moment, ensuring derivative calculations start fresh after restoration.
138-149: LGTM!The update flow correctly stores the heating curve, updates derivative and integral components, and maintains error history for the next iteration.
177-230: Sophisticated derivative computation with appropriate filtering.The implementation uses:
- Exponential smoothing of the error signal (line 185)
- Decay during deadband to prevent derivative kick (line 189)
- Change detection to skip updates when error is stable (line 184)
- Double cascaded low-pass filtering to reduce noise (lines 218, 221)
- Clamping to prevent excessive derivative action (line 222)
The detailed debug logging aligns with the commit message noting "detailed debug logging to PID."
One edge case to verify: when
time_elapsed <= 0(line 205), the derivative update is correctly skipped, but this could occur iftimestamp()returns identical values on consecutive calls or immediately afterrestore(). Verify this behavior is intentional and doesn't cause issues in practice, particularly after integration restarts.
…tegral, and other values are retained across restarts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
custom_components/sat/boiler.py (3)
256-257: PotentialTypeError: comparingNonewith float.If
state.max_modulation_levelisNone, the comparisonmax_modulation < BOILER_MODULATION_DELTA_THRESHOLDon line 256 will raise aTypeError. A past review comment indicated this was addressed in commit b239316, but themax_modulation is Nonecheck is still missing from the current code.🔎 Proposed fix
max_modulation = state.max_modulation_level current_modulation = state.relative_modulation_level - if current_modulation is None or max_modulation < BOILER_MODULATION_DELTA_THRESHOLD: + if current_modulation is None or max_modulation is None or max_modulation < BOILER_MODULATION_DELTA_THRESHOLD: return
267-271: Modulation reliability logic checks individual values instead of variation.The current logic counts how many modulation readings are ≥
BOILER_MODULATION_DELTA_THRESHOLD, but reliable modulation means the values vary by at least 3.0 percentage points over the window. A boiler stuck at 50% modulation would be incorrectly marked reliable even though it's not varying. This could contribute to incorrect boiler status detection mentioned in the PR feedback.🔎 Proposed fix to check modulation range instead
window = self._modulation_values_when_flame_on[-BOILER_MODULATION_RELIABILITY_MIN_SAMPLES:] - above_threshold = sum(1 for value in window if value >= BOILER_MODULATION_DELTA_THRESHOLD) - required_samples = max(2, int(len(window) * 0.4)) - - self._modulation_reliable = above_threshold >= required_samples + max_value = max(window) + min_value = min(window) + + if max_value - min_value < BOILER_MODULATION_DELTA_THRESHOLD: + self._modulation_reliable = False + else: + self._modulation_reliable = True
118-123: Type annotation mismatch persists: properties returnOptional[int]but storeOptional[float].The
flame_on_sinceandflame_off_sinceproperties are annotated to returnOptional[int], but_last_flame_on_atand_last_flame_off_atstorefloatvalues fromtimestamp()(line 156, which callsdt.utcnow().timestamp()). A past review comment indicated this was addressed in commit 554bdfc, but the type mismatch remains in the current code.🔎 Proposed fix
@property - def flame_on_since(self) -> Optional[int]: + def flame_on_since(self) -> Optional[float]: return self._last_flame_on_at @property - def flame_off_since(self) -> Optional[int]: + def flame_off_since(self) -> Optional[float]: return self._last_flame_off_atcustom_components/sat/minimum_setpoint.py (1)
368-370: Useasync_create_taskinstead ofcreate_taskfor Home Assistant compatibility.The code calls
self._hass.create_task(...)but Home Assistant's API usesasync_create_task()for scheduling coroutines from synchronous code.🔎 Proposed fix
if self._hass is not None: - self._hass.create_task(self.async_save_regimes()) + self._hass.async_create_task(self.async_save_regimes())custom_components/sat/area.py (2)
90-97: Remove climate-specificHVACMode.OFFfrom sensor state validation.Line 92 checks for
HVACMode.OFFwhen validating sensor state, butsensor_temperature_idreferences sensor entities (e.g.,sensor.room1_temp), which don't have HVAC modes.HVACMode.OFFis a climate-specific enum constant and shouldn't be used here.🔎 Proposed fix
# Check if there is an overridden sensor temperature if (sensor_temperature_id := climate_state.attributes.get(ATTR_SENSOR_TEMPERATURE_ID)) is not None: sensor_state = self._hass.states.get(sensor_temperature_id) - if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]: + if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]: if is_state_stale(sensor_state, self._sensor_max_value_age):If you need to filter an "off" state from sensors, use the string literal
"off"instead.
202-206: Fix event handler signature mismatch.Line 205 registers
self.updatedirectly as theEVENT_HOMEASSISTANT_STARTEDhandler, but event callbacks receive anEventobject whileupdate()expectsdatetime | None. This will cause aTypeErrorwhen the event fires.🔎 Proposed fix
if hass.state is CoreState.running: self.update() else: - hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STARTED, self.update) + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STARTED, lambda _event: self.update())tests/test_pid.py (1)
85-106: Test validates buggy PID behavior—constructor parameterautomatic_gain_valueis ignored.The test passes
automatic_gain_value=2.0to the PID constructor (line 91) and expectskp == 20.0(line 103). However, the PID implementation ignores the constructor parameter and uses hardcoded values instead:automatic_gain_value = 4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3 return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)Current behavior (using hardcoded 4 for underfloor):
kp = (2.0 * 40.0) / 4 = 20.0✓ (test passes)Expected behavior (using constructor parameter 2.0):
kp = (2.0 * 40.0) / 2.0 = 40.0When the PID implementation is fixed to use
self._automatic_gains_value, this test will need updated expectations.custom_components/sat/sensor.py (4)
88-91: Fix implicitOptionaltype annotation.Line 88 has
area_id: str = Nonewhich creates an implicit Optional prohibited by PEP 484. The parameter should be explicitly typed asOptional[str].🔎 Proposed fix
- def __init__(self, coordinator, config_entry: ConfigEntry, climate: SatClimate, area_id: str = None): + def __init__(self, coordinator, config_entry: ConfigEntry, climate: SatClimate, area_id: Optional[str] = None):
123-147: Guard againstNonePID innative_valueandextra_state_attributes.While the
availableproperty correctly checks forNone, Home Assistant may still accessnative_valueandextra_state_attributesbefore checking availability, causingAttributeErrorwhenself._pidisNone.🔎 Proposed fix
@property def native_value(self) -> float: + if self._pid is None: + return None return self._pid.output @property def extra_state_attributes(self) -> Optional[Mapping[str, Any]]: + if self._pid is None: + return None return { "current_kp": self._pid.kp, "current_ki": self._pid.ki, "current_kd": self._pid.kd, "proportional": self._pid.proportional, "integral": self._pid.integral, "derivative": self._pid.derivative, "error": self._pid.last_error, "output": self._pid.output, }Also update the return type of
native_valuetoOptional[float].
262-270: Align return type with nullable heating curve value.The
native_valueproperty returnsself._climate.heating_curve.valuewhich can beNone, but the return type is annotated asfloat.🔎 Proposed fix
@property - def native_value(self) -> float: + def native_value(self) -> Optional[float]: """Return the state of the device in native units.""" return self._climate.heating_curve.value
304-312: Guard againstNoneerror innative_value.Line 307 accesses
self._climate.error.valuewithout checking iferrorisNone. Even thoughavailablechecks this, Home Assistant may callnative_valuebefore checking availability.🔎 Proposed fix
@property - def native_value(self) -> float: + def native_value(self) -> Optional[float]: """Return the state of the device in native units.""" - return self._climate.error.value + return self._climate.error.value if self._climate.error is not None else None @property def available(self): """Return availability of the sensor.""" return self._climate.error is not Nonecustom_components/sat/climate.py (2)
877-892: Critical:HassJobcannot wrap async coroutines—will raiseValueErrorat runtime.Line 892 passes
self.async_control_heating_loop(an async method) directly toHassJob. Home Assistant'sHassJobraisesValueError: Coroutine not allowed to be passed to HassJobwhen given an async callable. The correct pattern is demonstrated at line 885, which usesasync_create_task.This issue was flagged in multiple previous reviews and reportedly addressed, but the problematic code remains.
🔎 Proposed fix
- self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop)) + self._control_heating_loop_unsub = async_call_later( + self.hass, + 5, + lambda _: self.hass.async_create_task(self.async_control_heating_loop()) + )
778-783: Undefined attribute_requested_setpoint_up_tickscauses potential runtime error.Line 781 assigns to
self._requested_setpoint_up_ticks, but this attribute is not initialized in__init__. While the assignment will create the attribute, reading it elsewhere before this code path executes would fail.🔎 Proposed fix
Add to
__init__around line 90:self._setpoint: Optional[float] = None self._requested_setpoint_down_ticks: int = 0 +self._requested_setpoint_up_ticks: int = 0 self._rooms: Optional[dict[str, float]] = Nonecustom_components/sat/pid.py (1)
35-48: Constructor storesautomatic_gain_valuebut never uses it.The
automatic_gain_valueparameter is passed to the constructor and stored inself._automatic_gains_value(line 41), but line 64 hardcodes the gain value instead of using the stored parameter. This means the configured value is ignored whenautomatic_gains=True.🔎 Proposed fix (use the stored parameter)
if self._heating_curve is None: return 0.0 - automatic_gain_value = 4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3 + # Use configured value, falling back to default based on heating system + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3) return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)Alternatively, if the hardcoded logic is intentional, remove the unused
automatic_gain_valueparameter from the constructor signature andcreate_pid_controllerinutil.py.
🧹 Nitpick comments (1)
custom_components/sat/pid.py (1)
172-173: Usingcreate_taskfor persistence is acceptable but consider error handling.The fire-and-forget
create_taskfor saving state is a reasonable pattern for non-critical persistence. However, if the save fails repeatedly, errors will be silently lost. Consider whether a warning log on save failure would be helpful for debugging.🔎 Optional: Add error handling to save
async def _async_save_state(self) -> None: if self._store is None: return data: dict[str, Any] = { STORAGE_KEY_INTEGRAL: self._integral, STORAGE_KEY_LAST_ERROR: self._last_error, STORAGE_KEY_PREVIOUS_ERROR: self._previous_error, STORAGE_KEY_FILTERED_ERROR: self._filtered_error, STORAGE_KEY_RAW_DERIVATIVE: self._raw_derivative, STORAGE_KEY_LAST_INTERVAL_UPDATED: self._last_interval_updated, STORAGE_KEY_LAST_DERIVATIVE_UPDATED: self._last_derivative_updated, } - await self._store.async_save(data) - _LOGGER.debug("Saved PID state to storage for entity=%s", self._entity_id) + try: + await self._store.async_save(data) + _LOGGER.debug("Saved PID state to storage for entity=%s", self._entity_id) + except Exception as exc: + _LOGGER.warning("Failed to save PID state for entity=%s: %s", self._entity_id, exc)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
custom_components/sat/area.pycustom_components/sat/boiler.pycustom_components/sat/climate.pycustom_components/sat/minimum_setpoint.pycustom_components/sat/pid.pycustom_components/sat/sensor.pycustom_components/sat/util.pytests/test_area.pytests/test_pid.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_area.py
🧰 Additional context used
🧬 Code graph analysis (4)
custom_components/sat/boiler.py (3)
custom_components/sat/helpers.py (1)
timestamp(12-14)custom_components/sat/types.py (1)
BoilerStatus(15-41)custom_components/sat/cycles.py (2)
Cycle(53-72)duration(70-72)
custom_components/sat/climate.py (10)
custom_components/sat/area.py (11)
Areas(228-346)get(249-255)update(215-225)items(261-263)async_added_to_hass(196-207)async_added_to_hass(265-268)ids(257-259)reset(342-346)pids(245-247)output(279-307)overshoot_cap(310-340)custom_components/sat/boiler.py (4)
BoilerControlIntent(37-39)update(150-164)async_added_to_hass(125-141)status(99-103)custom_components/sat/coordinator.py (18)
SatDataUpdateCoordinator(102-455)get(459-474)minimum_setpoint(308-310)update(50-55)async_added_to_hass(370-372)device_id(131-133)async_control_heating_loop(387-400)device_capabilities(147-152)cycles(174-176)setpoint(190-192)maximum_setpoint(313-316)device_status(142-144)hot_water_active(217-219)minimum_setpoint_value(252-254)flame_active(207-209)async_set_control_setpoint(406-409)device_state(155-171)async_set_heater_state(402-404)custom_components/sat/helpers.py (5)
is_state_stale(32-37)state_age_seconds(27-29)event_timestamp(17-19)clamp(108-113)timestamp(12-14)custom_components/sat/manufacturers/geminox.py (1)
Geminox(4-7)custom_components/sat/types.py (4)
RelativeModulationState(107-112)PWMStatus(79-83)PWMDecision(86-104)enabled(103-104)custom_components/sat/util.py (3)
create_pid_controller(20-41)create_heating_curve_controller(55-62)create_pwm_controller(65-75)custom_components/sat/pid.py (4)
update(155-173)async_added_to_hass(135-153)reset(119-133)output(112-117)custom_components/sat/pwm.py (6)
update(84-151)state(241-247)reset(66-76)enabled(233-234)status(237-238)disable(153-156)custom_components/sat/simulator/__init__.py (6)
device_id(29-30)async_control_heating_loop(85-111)setpoint(49-50)flame_active(61-62)async_set_control_setpoint(77-79)async_set_heater_state(72-75)
custom_components/sat/pid.py (4)
custom_components/sat/helpers.py (1)
timestamp(12-14)custom_components/sat/area.py (6)
output(279-307)async_added_to_hass(196-207)async_added_to_hass(265-268)get(249-255)update(215-225)error(145-153)custom_components/sat/boiler.py (2)
async_added_to_hass(125-141)update(150-164)custom_components/sat/climate.py (2)
async_added_to_hass(199-241)error(421-429)
tests/test_pid.py (2)
custom_components/sat/pid.py (11)
PID(32-268)kp(56-65)ki(68-73)kd(76-81)available(51-53)integral(89-91)raw_derivative(102-104)derivative(94-99)output(112-117)update(155-173)proportional(84-86)custom_components/sat/helpers.py (1)
timestamp(12-14)
🪛 Pylint (4.0.4)
custom_components/sat/minimum_setpoint.py
[refactor] 98-98: Too many instance attributes (8/7)
(R0902)
[refactor] 446-446: Too many local variables (16/15)
(R0914)
[refactor] 511-572: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 446-446: Too many return statements (12/6)
(R0911)
[refactor] 446-446: Too many branches (22/12)
(R0912)
[refactor] 446-446: Too many statements (58/50)
(R0915)
[refactor] 587-587: Too many local variables (18/15)
(R0914)
custom_components/sat/boiler.py
[refactor] 50-50: Too many instance attributes (11/7)
(R0902)
[refactor] 77-77: Too many instance attributes (12/7)
(R0902)
[refactor] 166-166: Too many return statements (18/6)
(R0911)
[refactor] 166-166: Too many branches (17/12)
(R0912)
[refactor] 276-276: Too many return statements (7/6)
(R0911)
[refactor] 390-390: Too many return statements (7/6)
(R0911)
custom_components/sat/climate.py
[refactor] 76-76: Too many statements (53/50)
(R0915)
[refactor] 291-291: Too many branches (16/12)
(R0912)
[refactor] 731-731: Too many branches (13/12)
(R0912)
custom_components/sat/pid.py
[refactor] 32-32: Too many instance attributes (18/7)
(R0902)
[refactor] 35-35: Too many arguments (9/5)
(R0913)
[refactor] 35-35: Too many positional arguments (9/5)
(R0917)
custom_components/sat/area.py
[refactor] 37-37: Too many instance attributes (8/7)
(R0902)
tests/test_pid.py
[refactor] 17-17: Too few public methods (1/2)
(R0903)
🪛 Ruff (0.14.10)
custom_components/sat/util.py
9-9: from .const import * used; unable to detect undefined names
(F403)
23-23: CONF_PROPORTIONAL may be undefined, or defined from star imports
(F405)
24-24: CONF_INTEGRAL may be undefined, or defined from star imports
(F405)
25-25: CONF_DERIVATIVE may be undefined, or defined from star imports
(F405)
27-27: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
28-28: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
29-29: CONF_AUTOMATIC_GAINS_VALUE may be undefined, or defined from star imports
(F405)
30-30: CONF_HEATING_CURVE_COEFFICIENT may be undefined, or defined from star imports
(F405)
49-49: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
50-50: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
68-68: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
69-69: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
custom_components/sat/climate.py
39-39: from .const import * used; unable to detect undefined names
(F403)
80-80: CONF_THERMOSTAT may be undefined, or defined from star imports
(F405)
81-81: CONF_INSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
82-82: CONF_HUMIDITY_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
83-83: CONF_OUTSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
104-104: Consider [PRESET_NONE, *list(self._presets.keys())] instead of concatenation
Replace with [PRESET_NONE, *list(self._presets.keys())]
(RUF005)
112-112: CONF_NAME may be undefined, or defined from star imports
(F405)
113-113: CONF_NAME may be undefined, or defined from star imports
(F405)
115-115: CONF_RADIATORS may be undefined, or defined from star imports
(F405)
116-116: CONF_WINDOW_SENSORS may be undefined, or defined from star imports
(F405)
118-118: CONF_SIMULATION may be undefined, or defined from star imports
(F405)
119-119: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
120-120: CONF_OVERSHOOT_PROTECTION may be undefined, or defined from star imports
(F405)
121-121: CONF_PUSH_SETPOINT_TO_THERMOSTAT may be undefined, or defined from star imports
(F405)
124-124: CONF_HEATING_MODE may be undefined, or defined from star imports
(F405)
125-125: CONF_THERMAL_COMFORT may be undefined, or defined from star imports
(F405)
126-126: CONF_CLIMATE_VALVE_OFFSET may be undefined, or defined from star imports
(F405)
127-127: CONF_TARGET_TEMPERATURE_STEP may be undefined, or defined from star imports
(F405)
128-128: CONF_DYNAMIC_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
129-129: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
130-130: CONF_SYNC_CLIMATES_WITH_PRESET may be undefined, or defined from star imports
(F405)
131-131: CONF_MAXIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
132-132: CONF_SENSOR_MAX_VALUE_AGE may be undefined, or defined from star imports
(F405)
133-133: CONF_WINDOW_MINIMUM_OPEN_TIME may be undefined, or defined from star imports
(F405)
134-134: CONF_MODE may be undefined, or defined from star imports
(F405)
134-134: MODE_SWITCH may be undefined, or defined from star imports
(F405)
134-134: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
160-160: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
225-225: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
233-233: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
243-243: Unused method argument: event
(ARG002)
353-353: DOMAIN may be undefined, or defined from star imports
(F405)
353-353: SERVICE_RESET_INTEGRAL may be undefined, or defined from star imports
(F405)
488-488: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
493-493: HEATING_MODE_ECO may be undefined, or defined from star imports
(F405)
506-506: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
506-506: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
593-593: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
597-597: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
610-610: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
616-616: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
620-620: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
631-631: MINIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
640-640: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
737-737: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
738-738: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
793-793: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
797-797: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
797-797: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
866-866: DEADBAND may be undefined, or defined from star imports
(F405)
866-866: DEADBAND may be undefined, or defined from star imports
(F405)
928-928: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/pid.py
9-9: from .const import * used; unable to detect undefined names
(F403)
64-64: HEATING_SYSTEM_UNDERFLOOR may be undefined, or defined from star imports
(F405)
179-179: DEADBAND may be undefined, or defined from star imports
(F405)
183-183: DEADBAND may be undefined, or defined from star imports
(F405)
211-211: DEADBAND may be undefined, or defined from star imports
(F405)
custom_components/sat/sensor.py
14-14: from .const import * used; unable to detect undefined names
(F403)
53-53: CONF_ROOMS may be undefined, or defined from star imports
(F405)
59-59: CONF_MINIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
59-59: CONF_MAXIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
67-67: CONF_NAME may be undefined, or defined from star imports
(F405)
84-84: CONF_NAME may be undefined, or defined from star imports
(F405)
88-88: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
106-106: CONF_NAME may be undefined, or defined from star imports
(F405)
108-108: CONF_NAME may be undefined, or defined from star imports
(F405)
130-130: CONF_NAME may be undefined, or defined from star imports
(F405)
132-132: CONF_NAME may be undefined, or defined from star imports
(F405)
323-323: CONF_NAME may be undefined, or defined from star imports
(F405)
342-342: CONF_NAME may be undefined, or defined from star imports
(F405)
347-347: CycleClassification may be undefined, or defined from star imports
(F405)
353-353: CONF_NAME may be undefined, or defined from star imports
(F405)
359-359: CONF_NAME may be undefined, or defined from star imports
(F405)
custom_components/sat/area.py
290-290: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (19)
custom_components/sat/boiler.py (1)
273-274: No changes needed —create_taskis the correct API here.The method
_update_modulation_reliabilityis synchronous (not async), so usingself._hass.create_task()is the correct Home Assistant pattern for scheduling tasks from sync contexts. Theasync_create_task()method is for async contexts only. The codebase correctly uses this pattern consistently in other sync methods (pid.py, minimum_setpoint.py).custom_components/sat/util.py (1)
4-95: LGTM! Factory functions properly updated for new architecture.The factory functions have been correctly refactored to support the new cycle-centric architecture. The changes include:
- Added proper imports for new controller types (DynamicMinimumSetpoint, PWMConfig)
- Updated PID factory to accept
entity_idparameter for per-area instances- Created DynamicMinimumSetpoint factory with MinimumSetpointConfig
- Simplified PWM factory to use PWMConfig dataclass
All previously flagged issues (TYPE_CHECKING import, CONF_MAXIMUM_SETPOINT default) have been addressed in prior commits.
custom_components/sat/climate.py (10)
56-59: Good addition of explicit threshold constants.Extracting these magic numbers into named constants improves readability and maintainability of the setpoint control logic.
146-168: Clean static helper extraction.The
_ensure_list,_build_config_options, and_build_presetsstatic methods improve code organization by extracting configuration logic from__init__.
170-180: Good defensive feature detection.Using
hasattrto check forTURN_ON/TURN_OFFfeatures ensures backward compatibility with older Home Assistant versions.
182-197: Well-structured sensor state retrieval with staleness check.The
_get_entity_state_floathelper centralizes entity state retrieval with proper staleness validation and invalid state handling.
224-241: Event listeners properly useasync_on_removefor cleanup.The cycle event listeners are correctly registered with
async_on_remove, ensuring they are unsubscribed when the entity is removed.
551-566: Clean PWM decision tracking with change detection logging.The refactored
pulse_width_modulation_enabledproperty with decision tracking and change-based logging provides good observability without excessive log spam.
568-623: Well-structured PWM decision methods.Breaking the PWM enable logic into
_pwm_decision,_pwm_static_decision, and_pwm_dynamic_decisionimproves readability and makes the decision tree easier to understand and maintain.
731-797: Solid setpoint control logic with flame stability protection.The near-target downward lock logic (lines 757-765) prevents premature flame-off by holding the setpoint when the boiler is actively heating near target. The persistent decrease tracking (lines 748-754, 773-775) adds hysteresis to avoid oscillation. Good logging throughout aids debugging.
894-930: Well-structured heating control loop with clean coordinator delegation.The control loop properly:
- Guards against missing required values (lines 900-908)
- Updates PWM state before coordinator interaction (lines 915-918)
- Passes control context and intent to coordinator (lines 921-923)
- Applies computed controls (lines 926-928)
484-506: LGTM:requested_setpointnow returns a computed value, not a method reference.The
requested_setpointproperty correctly accessesself.pid.outputandself.areas.pids.outputas properties (not method calls), which aligns with the PID class'soutputproperty definition. The clamping logic ensures the result stays within valid bounds.custom_components/sat/pid.py (7)
14-20: Good extraction of derivative filtering constants.Named constants for the derivative filtering pipeline make the algorithm more maintainable and the tuning parameters explicit.
22-29: Storage key constants improve maintainability.Using named constants for storage keys prevents typos and makes the persistence interface self-documenting.
50-53: Reasonable availability check.The
availableproperty correctly requires both a last error value and a heating curve to be set before the PID is considered usable.
111-117: Output computation is straightforward.The
outputproperty correctly combines the heating curve with P, I, and D terms. Returning 0.0 when the heating curve is None is a safe fallback.
175-200: Solid integral term implementation with anti-windup.The integral update correctly:
- Resets the time base when entering the deadband (lines 179-180)
- Disables accumulation outside the deadband (lines 183-185)
- Clamps the integral to prevent windup (lines 196-197)
202-241: Well-designed derivative filtering pipeline.The two-stage low-pass filtering approach effectively reduces noise while maintaining responsiveness. Key features:
- Error smoothing with
DERIVATIVE_ERROR_ALPHA(line 209)- Decay within deadband prevents stale derivative influence (line 213)
- Dual alpha filters (lines 229, 232) for progressive smoothing
- Capping to
±DERIVATIVE_RAW_CAPprevents extreme values (line 233)
243-251: Good defensive parsing with_maybe_float.The static helper safely converts stored values to floats, returning
Noneon failure rather than raising exceptions during state restoration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
custom_components/sat/climate.py (3)
253-289: Critical: Missing state change listener for inside temperature sensor.The
current_temperatureproperty (lines 402-410) reads frominside_sensor_entity_id, but there is no state change listener registered for this sensor in_register_event_listeners. State listeners exist for thermostat, climates, and window sensors, but not for the primary inside temperature sensor.This means the climate entity won't update its state when the inside sensor changes, which directly correlates with the user's reported issue: "SAT climate 'current temperature' did not update correctly (only updated once on Home Assistant restart)."
The entity relies solely on periodic
async_control_pidcalls (every 30 seconds), which have multiple early-return paths that can skipasync_write_ha_state().🔎 Proposed fix
Add this listener registration in
_register_event_listeners:self.async_on_remove( async_track_state_change_event( self.hass, self.areas.ids(), self._async_climate_changed ) ) + + self.async_on_remove( + async_track_state_change_event( + self.hass, self.inside_sensor_entity_id, self._async_inside_sensor_changed + ) + ) if len(self._window_sensors) > 0:And add the handler method:
async def _async_inside_sensor_changed(self, event: Event[EventStateChangedData]) -> None: """Handle inside temperature sensor state changes.""" new_state = event.data.get("new_state") if new_state is None or new_state.state in (STATE_UNAVAILABLE, STATE_UNKNOWN): return _LOGGER.debug("Inside sensor changed to %.2f°C", float(new_state.state)) self.async_write_ha_state()
877-892: Critical:HassJobwith async callable requires proper scheduling.Line 892 passes the async method
self.async_control_heating_looptoHassJob. Whenasync_call_laterinvokes this, it will call the function and receive a coroutine object, but Home Assistant's scheduler won't automatically await it. The coroutine will be created but never executed.Line 885 demonstrates the correct pattern using
async_create_task.🔎 Proposed fix
- self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop)) + def _schedule_heating_loop() -> None: + self.hass.async_create_task(self.async_control_heating_loop()) + + self._control_heating_loop_unsub = async_call_later(self.hass, 5, _schedule_heating_loop)Alternatively, use a lambda:
- self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop)) + self._control_heating_loop_unsub = async_call_later( + self.hass, 5, lambda _: self.hass.async_create_task(self.async_control_heating_loop()) + )
731-797: Minor: Uninitialized attribute_requested_setpoint_up_ticks.Line 781 assigns to
self._requested_setpoint_up_ticks, but this attribute is never initialized in__init__(line 90 only initializes_requested_setpoint_down_ticks). While the assignment will create the attribute, it's inconsistent with the established pattern of initializing all instance variables.🔎 Proposed fix
Add initialization in
__init__around line 90:self._setpoint: Optional[float] = None self._requested_setpoint_down_ticks: int = 0 + self._requested_setpoint_up_ticks: int = 0 self._rooms: Optional[dict[str, float]] = Nonecustom_components/sat/pid.py (1)
56-65: Major issue remains: Constructor parameterautomatic_gain_valueis still ignored.Line 64 hardcodes
automatic_gain_valueas4or3based on heating system, completely ignoring theself._automatic_gains_valueparameter stored from the constructor (line 41). This was flagged in previous reviews but remains unresolved.Either use the stored parameter when
automatic_gains=True, or remove the unused constructor parameter and stored attribute.Based on learnings, this issue was previously flagged and marked unresolved.
🔎 Proposed fix (use stored parameter)
if self._heating_curve is None: return 0.0 - automatic_gain_value = 4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3 + # Use configured value, falling back to heating-system default + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HEATING_SYSTEM_UNDERFLOOR else 3) return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)
🧹 Nitpick comments (1)
custom_components/sat/pid.py (1)
161-179: Useasync_create_taskinstead ofcreate_task.Line 179 uses
self._hass.create_task(self._async_save_state())but Home Assistant's recommended pattern is to useasync_create_taskfor background tasks. The deprecatedcreate_taskmay be removed in future versions.🔎 Proposed fix
_LOGGER.debug( "PID update for %s (error=%.3f curve=%.3f proportional=%.3f integral=%.3f derivative=%.3f output=%.3f)", self._entity_id, error, heating_curve, self.proportional, self.integral, self.derivative, self.output ) if self._hass is not None and self._store is not None: - self._hass.create_task(self._async_save_state()) + self._hass.async_create_task(self._async_save_state())
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/sat/climate.pycustom_components/sat/pid.pycustom_components/sat/util.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/util.py (5)
custom_components/sat/helpers.py (1)
convert_time_str_to_seconds(40-49)custom_components/sat/minimum_setpoint.py (2)
DynamicMinimumSetpoint(269-792)MinimumSetpointConfig(92-94)custom_components/sat/pid.py (4)
PID(32-273)kp(56-65)ki(68-73)kd(76-81)custom_components/sat/pwm.py (2)
PWM(38-247)PWMConfig(30-35)custom_components/sat/climate.py (1)
SatClimate(73-1062)
🪛 Pylint (4.0.4)
custom_components/sat/climate.py
[refactor] 76-76: Too many statements (53/50)
(R0915)
[refactor] 291-291: Too many branches (16/12)
(R0912)
[refactor] 731-731: Too many branches (13/12)
(R0912)
custom_components/sat/pid.py
[refactor] 32-32: Too many instance attributes (18/7)
(R0902)
[refactor] 35-35: Too many arguments (9/5)
(R0913)
[refactor] 35-35: Too many positional arguments (9/5)
(R0917)
🪛 Ruff (0.14.10)
custom_components/sat/util.py
9-9: from .const import * used; unable to detect undefined names
(F403)
23-23: CONF_PROPORTIONAL may be undefined, or defined from star imports
(F405)
24-24: CONF_INTEGRAL may be undefined, or defined from star imports
(F405)
25-25: CONF_DERIVATIVE may be undefined, or defined from star imports
(F405)
27-27: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
28-28: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
29-29: CONF_AUTOMATIC_GAINS_VALUE may be undefined, or defined from star imports
(F405)
30-30: CONF_HEATING_CURVE_COEFFICIENT may be undefined, or defined from star imports
(F405)
49-49: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
50-50: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
68-68: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
69-69: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
custom_components/sat/climate.py
39-39: from .const import * used; unable to detect undefined names
(F403)
80-80: CONF_THERMOSTAT may be undefined, or defined from star imports
(F405)
81-81: CONF_INSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
82-82: CONF_HUMIDITY_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
83-83: CONF_OUTSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
104-104: Consider [PRESET_NONE, *list(self._presets.keys())] instead of concatenation
Replace with [PRESET_NONE, *list(self._presets.keys())]
(RUF005)
112-112: CONF_NAME may be undefined, or defined from star imports
(F405)
113-113: CONF_NAME may be undefined, or defined from star imports
(F405)
115-115: CONF_RADIATORS may be undefined, or defined from star imports
(F405)
116-116: CONF_WINDOW_SENSORS may be undefined, or defined from star imports
(F405)
118-118: CONF_SIMULATION may be undefined, or defined from star imports
(F405)
119-119: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
120-120: CONF_OVERSHOOT_PROTECTION may be undefined, or defined from star imports
(F405)
121-121: CONF_PUSH_SETPOINT_TO_THERMOSTAT may be undefined, or defined from star imports
(F405)
124-124: CONF_HEATING_MODE may be undefined, or defined from star imports
(F405)
125-125: CONF_THERMAL_COMFORT may be undefined, or defined from star imports
(F405)
126-126: CONF_CLIMATE_VALVE_OFFSET may be undefined, or defined from star imports
(F405)
127-127: CONF_TARGET_TEMPERATURE_STEP may be undefined, or defined from star imports
(F405)
128-128: CONF_DYNAMIC_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
129-129: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
130-130: CONF_SYNC_CLIMATES_WITH_PRESET may be undefined, or defined from star imports
(F405)
131-131: CONF_MAXIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
132-132: CONF_SENSOR_MAX_VALUE_AGE may be undefined, or defined from star imports
(F405)
133-133: CONF_WINDOW_MINIMUM_OPEN_TIME may be undefined, or defined from star imports
(F405)
134-134: CONF_MODE may be undefined, or defined from star imports
(F405)
134-134: MODE_SWITCH may be undefined, or defined from star imports
(F405)
134-134: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
160-160: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
225-225: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
233-233: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
243-243: Unused method argument: event
(ARG002)
353-353: DOMAIN may be undefined, or defined from star imports
(F405)
353-353: SERVICE_RESET_INTEGRAL may be undefined, or defined from star imports
(F405)
488-488: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
493-493: HEATING_MODE_ECO may be undefined, or defined from star imports
(F405)
506-506: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
506-506: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
593-593: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
597-597: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
610-610: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
616-616: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
620-620: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
631-631: MINIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
640-640: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
737-737: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
738-738: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
793-793: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
797-797: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
797-797: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
866-866: DEADBAND may be undefined, or defined from star imports
(F405)
866-866: DEADBAND may be undefined, or defined from star imports
(F405)
928-928: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/pid.py
9-9: from .const import * used; unable to detect undefined names
(F403)
64-64: HEATING_SYSTEM_UNDERFLOOR may be undefined, or defined from star imports
(F405)
185-185: DEADBAND may be undefined, or defined from star imports
(F405)
189-189: DEADBAND may be undefined, or defined from star imports
(F405)
217-217: DEADBAND may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
custom_components/sat/util.py (2)
20-41: LGTM! entity_id parameter properly added.The addition of the
entity_idparameter tocreate_pid_controllercorrectly forwards it to the PID constructor, enabling storage-backed state persistence for the PID controller. The parameter is appropriately optional with a default ofNone.
65-75: LGTM! PWM controller factory properly updated.The refactored
create_pwm_controllercorrectly accepts theheating_curveparameter and constructs aPWMConfigobject before passing both to the PWM constructor. This aligns with the new cycle-centric architecture.custom_components/sat/pid.py (1)
50-117: LGTM! PID properties properly expose controller state.The new properties (
available,kp,ki,kd,proportional,integral,derivative,raw_derivative,last_error,output) correctly handle None values and provide appropriate read-only access to the PID controller's internal state for observability.custom_components/sat/climate.py (1)
552-623: LGTM! PWM decision logic is well-structured.The refactored PWM enablement logic cleanly separates static and dynamic decision paths, uses descriptive enums for logging, and implements appropriate hysteresis bands to prevent oscillation. The separation of concerns between configuration checks, static minimum logic, and dynamic minimum logic improves maintainability.
…arity. Refactored configuration management using `SatConfig` to simplify data flow and reduce redundancy. Additionally, updated tests and logic to align with the new setup.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_config_flow.py (1)
5-13: Test may fail due to missing hass context.The
hassparameter is declared but never used. TheSatFlowHandlerandasync_create_coordinator()likely require a Home Assistant instance to function properly. Consider passinghassto the flow handler or initializing it withflow_handler.hass = hass.🔎 Proposed fix
async def test_create_coordinator(hass): flow_handler = SatFlowHandler() + flow_handler.hass = hass flow_handler.data = { "name": "Test", "mode": SatMode.FAKE, "device": "test_device", } await flow_handler.async_create_coordinator()custom_components/sat/mqtt/__init__.py (1)
34-46: Useself.hassconsistently throughout the method.The method mixes usage of the
hassparameter (line 35:mqtt.async_wait_for_mqtt_client(hass)) withself.hass(line 39:mqtt.async_subscribe(self.hass, ...)). Sinceself.hassis already initialized in__init__via the parent class before this lifecycle method is called, useself.hassin both places for consistency:await mqtt.async_wait_for_mqtt_client(self.hass)The
hassparameter is required by the parent class signature but doesn't need to be used whenself.hassis available.
♻️ Duplicate comments (11)
tests/test_climate.py (1)
218-222: Use publicpwm.enabledproperty instead of private_enabledattribute.Lines 218, 221, and 253 directly access
climate.pwm._enabled. The PWM class exposes a publicenabledproperty that should be used instead to respect encapsulation.Also applies to: 253-253
custom_components/sat/pid.py (1)
63-64: Constructor parameterautomatic_gain_valueis still ignored (duplicate issue).Line 63 hardcodes the automatic gain calculation using 4 or 3, completely ignoring
self._automatic_gains_valuewhich was passed to the constructor and stored at line 40. This issue was flagged in previous reviews but remains unresolved.Based on learnings, this was flagged in previous reviews.
🔎 Proposed fix
if self._heating_curve is None: return 0.0 - automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3 + # Use configured value, falling back to default based on heating system + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3) return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)Alternatively, if the hardcoded logic is intentional, remove the unused
automatic_gain_valueparameter from the constructor signature.tests/test_pid.py (1)
97-118: Test validates buggy behavior (duplicate issue).This test passes
automatic_gain_value=2.0but expectskp == 20.0, which only matches the current buggy implementation that ignores the parameter and uses hardcoded values (4 for underfloor, 3 for radiators). When the PID bug is fixed to honorself._automatic_gains_value, the expectedkpshould be40.0(since(2.0 * 40.0) / 2.0 = 40.0), andkiandkdmust also be recalculated.Based on learnings, this was flagged in previous reviews.
🔎 Proposed fix (after PID bug is resolved)
pid.update(state, heating_curve=40.0) - assert pid.kp == 20.0 - assert pid.ki == round(20.0 / 8400, 6) - assert pid.kd == round(0.07 * 8400 * 20.0, 6) + assert pid.kp == 40.0 + assert pid.ki == round(40.0 / 8400, 6) + assert pid.kd == round(0.07 * 8400 * 40.0, 6)custom_components/sat/simulator/__init__.py (1)
46-47: Fix return type annotation mismatch.The method is annotated to return
Optional[float]but returnsTrue(a bool). Based on the base class interface incoordinator.py, this property should returnbool.🔎 Proposed fix
- def supports_relative_modulation(self) -> Optional[float]: + def supports_relative_modulation(self) -> bool: return Truecustom_components/sat/esphome/__init__.py (1)
1-1: Remove duplicate import identifier.The
annotationsidentifier is duplicated in the future import. While Python may tolerate this, it should be corrected.🔎 Proposed fix
-from __future__ import annotations, annotations +from __future__ import annotationscustom_components/sat/mqtt/__init__.py (1)
20-25: Past review concern about storage key migration remains unaddressed.The storage key format change from the previous snake_case-based dynamic key to
f"sat.mqtt.{self._device_id}"will cause data loss for existing users, as previously noted. No migration logic has been added.custom_components/sat/coordinator.py (1)
117-117: Critical: Wrong debouncer callback method name.Line 117 references
self.async_update_listeners, which does not exist. The correct method isasync_notify_listeners(defined at line 448). This will cause anAttributeErrorwhen the debouncer attempts to call the callback.🔎 Proposed fix
- self._hass_notify_debouncer = Debouncer(hass=self.hass, logger=_LOGGER, cooldown=0.2, immediate=False, function=self.async_update_listeners) + self._hass_notify_debouncer = Debouncer(hass=self.hass, logger=_LOGGER, cooldown=0.2, immediate=False, function=self.async_notify_listeners)Based on past review comments, this issue was previously identified but remains unresolved.
custom_components/sat/sensor.py (1)
88-91: Use a stable identifier (entry_id) forSatRequestedSetpoint.unique_id.Deriving
unique_idfromname_lowermakes it change when the user renames the integration, breaking entity history and automations.Prefer the config entry id (or another immutable key) instead, e.g.:
Proposed fix
- def unique_id(self) -> str: - return f"{self._config.name_lower}-requested-setpoint" + def unique_id(self) -> str: + return f"{self._config.entry_id}-requested-setpoint"custom_components/sat/minimum_setpoint.py (1)
368-371: Useasync_create_taskinstead ofcreate_taskto scheduleasync_save_regimes.
HomeAssistantexposesasync_create_task, notcreate_task. This call will fail at runtime and regimes will not be persisted.Replace the scheduling with the supported API:
Proposed fix
- if self._hass is not None: - self._hass.create_task(self.async_save_regimes()) + if self._hass is not None: + self._hass.async_create_task(self.async_save_regimes())custom_components/sat/area.py (1)
88-97: RemoveHVACMode.OFFfrom sensor state validation intemperature_state.This filter mixes climate modes into generic sensor validation:
if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]:
- For real sensors,
HVACMode.OFFis meaningless.- For climates used as sensors, the temperature remains valid even when HVAC is off.
Treat only unavailable/unknown as invalid, or (if required) compare against the literal
"off"and only when the override entity is a climate.Proposed change
- if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]: + if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]:custom_components/sat/climate.py (1)
855-871: Do not pass async coroutine directly toHassJob/async_call_later.Here:
self._control_heating_loop_unsub = async_call_later( self.hass, 5, HassJob(self.async_control_heating_loop) )
HassJobcannot wrap async coroutines this way and will raiseValueError: Coroutine not allowed to be passed to HassJob, preventing the control loop from running.Schedule a regular callback that creates the task instead:
Proposed fix
- self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop)) + def _schedule_heating_loop(_time=None) -> None: + self.hass.async_create_task(self.async_control_heating_loop(_time)) + + self._control_heating_loop_unsub = async_call_later(self.hass, 5, _schedule_heating_loop)#!/bin/bash # Confirm other uses of HassJob do not wrap async coroutines directly. rg -n "HassJob\(" custom_components/sat -n -C2
🧹 Nitpick comments (8)
custom_components/sat/entry_data.py (1)
9-9: Consider replacing wildcard import with explicit imports.The
from .const import *pattern obscures dependencies and triggers static analysis warnings (F403, F405). While functional, explicit imports would improve code clarity and tooling support.custom_components/sat/fake/__init__.py (1)
66-68: Add return type annotation for consistency.The
relative_modulation_valueproperty lacks a return type annotation, unlike other similar properties in this class.🔎 Proposed fix
@property - def relative_modulation_value(self): + def relative_modulation_value(self) -> Optional[float]: return self._relative_modulation_valuecustom_components/sat/pid.py (1)
165-174: Verify background task completion on shutdown.The code schedules
_async_save_state()as a background task without awaiting it or tracking completion. If Home Assistant shuts down before the save completes, state may be lost. Consider usingasync_create_taskwith a task group or ensure shutdown sequences wait for pending saves.custom_components/sat/heating_curve.py (1)
78-80: Consider explicit import for better clarity.The enum usage is correct, but the static analysis warning indicates that star imports can obscure dependencies. While this is a false positive (since
HeatingSystemis defined inconst.py), an explicit import would improve code clarity and eliminate the warning.🔎 Proposed refactor
-from .const import * +from .const import ( + HeatingSystem, + MINIMUM_SETPOINT, + # Add other needed imports +)custom_components/sat/serial/sensor.py (1)
109-127: Consider refactoring to reduce constructor parameters.The constructor has 6 parameters, which exceeds the typical guideline of 5. While this refactor maintains the existing parameter count (previously had
config_entryinstead ofconfig), consider whetherinfo,key, andsourcecould be consolidated into a single data structure to improve maintainability.💡 Example refactor approach
@dataclass class SensorConfig: info: SatSensorInfo key: str source: str def __init__( self, coordinator: SatSerialCoordinator, config: SatConfig, sensor_config: SensorConfig ): ...This would reduce the parameter count and group related configuration together.
custom_components/sat/serial/binary_sensor.py (1)
94-109: Consider consolidating constructor parameters.The constructor accepts 6 parameters, slightly exceeding conventions. While all parameters are used, consider grouping
info,key, andsourceinto a single configuration object (e.g.,SensorConfig) in a future refactor to improve maintainability.custom_components/sat/__init__.py (2)
33-37: Uselogging.exceptionfor better error diagnostics.Replace
_LOGGER.errorwith_LOGGER.exceptionto automatically include the traceback, making Sentry initialization failures easier to diagnose.🔎 Proposed fix
try: if entry_data.config.error_monitoring_enabled: entry_data.sentry = initialize_sentry() except Exception as error: - _LOGGER.error("Error during Sentry initialization: %s", error) + _LOGGER.exception("Error during Sentry initialization")
59-65: Uselogging.exceptionfor better error diagnostics.Replace
_LOGGER.errorwith_LOGGER.exceptionto automatically include the traceback.🔎 Proposed fix
try: if entry_data.sentry is not None: entry_data.sentry.flush() entry_data.sentry.close() entry_data.sentry = None except Exception as error: - _LOGGER.error("Error during Sentry cleanup: %s", error) + _LOGGER.exception("Error during Sentry cleanup")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
custom_components/sat/__init__.pycustom_components/sat/area.pycustom_components/sat/binary_sensor.pycustom_components/sat/climate.pycustom_components/sat/config_flow.pycustom_components/sat/const.pycustom_components/sat/coordinator.pycustom_components/sat/entity.pycustom_components/sat/entry_data.pycustom_components/sat/errors.pycustom_components/sat/esphome/__init__.pycustom_components/sat/fake/__init__.pycustom_components/sat/heating_curve.pycustom_components/sat/helpers.pycustom_components/sat/minimum_setpoint.pycustom_components/sat/mqtt/__init__.pycustom_components/sat/mqtt/opentherm.pycustom_components/sat/number.pycustom_components/sat/pid.pycustom_components/sat/sensor.pycustom_components/sat/serial/__init__.pycustom_components/sat/serial/binary_sensor.pycustom_components/sat/serial/sensor.pycustom_components/sat/simulator/__init__.pycustom_components/sat/simulator/sensor.pycustom_components/sat/switch/__init__.pycustom_components/sat/temperature_state.pycustom_components/sat/util.pytests/conftest.pytests/const.pytests/test_area.pytests/test_climate.pytests/test_config_flow.pytests/test_overshoot_protection.pytests/test_pid.pytests/test_pwm.py
💤 Files with no reviewable changes (1)
- custom_components/sat/errors.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_area.py
- tests/const.py
🧰 Additional context used
🧬 Code graph analysis (23)
custom_components/sat/temperature_state.py (2)
custom_components/sat/area.py (2)
errors(245-248)error(127-145)custom_components/sat/climate.py (2)
error(380-399)setpoint(454-456)
tests/test_config_flow.py (2)
custom_components/sat/entry_data.py (1)
SatMode(19-26)custom_components/sat/config_flow.py (1)
SatFlowHandler(33-547)
custom_components/sat/entry_data.py (2)
custom_components/sat/helpers.py (2)
calculate_default_maximum_setpoint(65-67)convert_time_str_to_seconds(40-49)custom_components/sat/const.py (2)
HeatingMode(104-106)HeatingSystem(97-101)
custom_components/sat/__init__.py (2)
custom_components/sat/coordinator.py (4)
SatDataUpdateCoordinatorFactory(69-100)resolve(71-100)async_setup(371-373)get(464-474)custom_components/sat/entry_data.py (3)
SatConfig(87-249)SatEntryData(253-259)error_monitoring_enabled(97-98)
custom_components/sat/binary_sensor.py (13)
custom_components/sat/entity.py (2)
SatClimateEntity(42-56)async_added_to_hass(48-56)custom_components/sat/entry_data.py (6)
SatConfig(87-249)SatMode(19-26)mode(116-117)sensors(159-171)window_sensors(155-156)name_lower(127-128)custom_components/sat/helpers.py (2)
seconds_since(22-24)timestamp(12-14)custom_components/sat/__init__.py (1)
async_setup_entry(20-45)custom_components/sat/sensor.py (12)
async_setup_entry(23-66)name(72-73)name(126-130)name(178-179)name(219-220)name(261-262)name(293-294)name(327-328)name(354-355)name(371-372)async_added_to_hass(104-113)async_added_to_hass(345-351)custom_components/sat/serial/binary_sensor.py (2)
async_setup_entry(68-88)name(112-114)custom_components/sat/area.py (3)
get(255-261)async_added_to_hass(202-213)async_added_to_hass(271-274)custom_components/sat/coordinator.py (6)
get(464-474)supports_setpoint_management(324-330)supports_relative_modulation_management(354-360)setpoint(190-192)relative_modulation_value(247-249)async_added_to_hass(375-377)custom_components/sat/serial/__init__.py (4)
get(152-154)supports_setpoint_management(56-57)setpoint(72-76)relative_modulation_value(114-118)custom_components/sat/simulator/__init__.py (3)
supports_setpoint_management(38-39)setpoint(50-51)relative_modulation_value(66-67)custom_components/sat/pid.py (1)
async_added_to_hass(136-153)custom_components/sat/minimum_setpoint.py (1)
async_added_to_hass(371-423)custom_components/sat/boiler.py (1)
async_added_to_hass(125-141)
custom_components/sat/heating_curve.py (1)
custom_components/sat/const.py (1)
HeatingSystem(97-101)
custom_components/sat/simulator/__init__.py (4)
custom_components/sat/coordinator.py (6)
maximum_setpoint(313-321)supports_relative_modulation(342-351)setpoint(190-192)member_id(202-204)async_control_heating_loop(392-405)async_notify_listeners(448-455)custom_components/sat/entry_data.py (3)
SatConfig(87-249)simulation(242-249)limits(198-217)custom_components/sat/helpers.py (1)
seconds_since(22-24)custom_components/sat/types.py (1)
DeviceState(73-76)
custom_components/sat/entity.py (2)
custom_components/sat/entry_data.py (1)
SatConfig(87-249)custom_components/sat/esphome/__init__.py (1)
async_added_to_hass(139-164)
tests/test_overshoot_protection.py (3)
custom_components/sat/const.py (1)
HeatingSystem(97-101)custom_components/sat/overshoot_protection.py (6)
OvershootProtection(37-196)_record_sample(143-148)_sample_statistics(157-182)_is_stable(185-196)_calculate_overshoot_value(114-120)_get_setpoint(131-136)custom_components/sat/coordinator.py (1)
maximum_setpoint_value(257-259)
custom_components/sat/helpers.py (2)
custom_components/sat/state.py (1)
State(10-12)custom_components/sat/const.py (1)
HeatingSystem(97-101)
custom_components/sat/pid.py (3)
custom_components/sat/helpers.py (3)
clamp(108-113)timestamp(12-14)float_value(78-87)custom_components/sat/temperature_state.py (3)
TemperatureState(38-49)error(47-49)max(30-34)custom_components/sat/const.py (1)
HeatingSystem(97-101)
custom_components/sat/sensor.py (3)
custom_components/sat/coordinator.py (6)
SatDataUpdateCoordinator(103-460)get(464-474)supports_relative_modulation_management(354-360)relative_modulation_value(247-249)last_cycle(179-181)device_status(142-144)custom_components/sat/entry_data.py (9)
SatConfig(87-249)SatMode(19-26)mode(116-117)sensors(159-171)rooms(143-144)limits(198-217)name(123-124)name_lower(127-128)pid(174-183)custom_components/sat/types.py (2)
BoilerStatus(15-41)CycleClassification(54-70)
custom_components/sat/esphome/__init__.py (5)
custom_components/sat/coordinator.py (3)
SatDataUpdateCoordinator(103-460)get(464-474)async_added_to_hass(375-377)custom_components/sat/entry_data.py (2)
SatConfig(87-249)device(93-94)custom_components/sat/helpers.py (2)
float_value(78-87)int_value(96-99)custom_components/sat/types.py (1)
DeviceState(73-76)custom_components/sat/mqtt/__init__.py (1)
async_added_to_hass(34-46)
custom_components/sat/const.py (1)
custom_components/sat/types.py (1)
CycleClassification(54-70)
custom_components/sat/climate.py (4)
custom_components/sat/area.py (9)
Areas(234-352)get(255-261)async_added_to_hass(202-213)async_added_to_hass(271-274)target_temperature(119-124)items(267-269)ids(263-265)reset(348-352)pids(251-253)custom_components/sat/entry_data.py (9)
SatConfig(87-249)SatMode(19-26)presets(220-239)name_lower(127-128)pid(174-183)pwm(186-195)sensors(159-171)limits(198-217)is_mode(119-120)custom_components/sat/helpers.py (6)
float_value(78-87)is_state_stale(32-37)state_age_seconds(27-29)event_timestamp(17-19)clamp(108-113)timestamp(12-14)custom_components/sat/types.py (5)
BoilerStatus(15-41)RelativeModulationState(107-112)PWMStatus(79-83)PWMDecision(86-104)enabled(103-104)
custom_components/sat/minimum_setpoint.py (5)
custom_components/sat/boiler.py (4)
BoilerControlIntent(37-39)BoilerCapabilities(43-46)flow_setpoint_error(69-70)flow_return_delta(73-74)custom_components/sat/coordinator.py (5)
ControlLoopSample(33-39)cycles(174-176)minimum_setpoint(308-310)maximum_setpoint(313-321)setpoint(190-192)custom_components/sat/types.py (2)
CycleKind(44-51)CycleClassification(54-70)custom_components/sat/helpers.py (1)
clamp(108-113)custom_components/sat/cycles.py (3)
CycleStatistics(76-80)Cycle(53-72)duration(70-72)
custom_components/sat/serial/__init__.py (2)
custom_components/sat/coordinator.py (1)
member_id(202-204)custom_components/sat/entry_data.py (1)
device(93-94)
custom_components/sat/fake/__init__.py (3)
custom_components/sat/coordinator.py (5)
SatDataUpdateCoordinator(103-460)member_id(202-204)setpoint(190-192)device_active(196-198)relative_modulation_value(247-249)custom_components/sat/entry_data.py (1)
SatConfig(87-249)custom_components/sat/types.py (1)
DeviceState(73-76)
custom_components/sat/mqtt/__init__.py (2)
custom_components/sat/entry_data.py (3)
SatConfig(87-249)device(93-94)mqtt_topic(112-113)custom_components/sat/coordinator.py (1)
async_added_to_hass(375-377)
custom_components/sat/switch/__init__.py (2)
custom_components/sat/coordinator.py (1)
member_id(202-204)custom_components/sat/entry_data.py (1)
device(93-94)
custom_components/sat/serial/binary_sensor.py (2)
custom_components/sat/entry_data.py (3)
SatConfig(87-249)get_entry_data(262-263)name_lower(127-128)custom_components/sat/serial/__init__.py (1)
SatSerialCoordinator(27-208)
custom_components/sat/config_flow.py (2)
custom_components/sat/entry_data.py (3)
SatConfig(87-249)SatMode(19-26)mode(116-117)custom_components/sat/const.py (2)
HeatingSystem(97-101)HeatingMode(104-106)
tests/test_pwm.py (5)
custom_components/sat/boiler.py (6)
BoilerState(50-74)flame_on_since(118-119)flame_off_since(122-123)modulation_reliable(114-115)update(150-164)status(99-103)custom_components/sat/const.py (1)
HeatingSystem(97-101)custom_components/sat/heating_curve.py (3)
HeatingCurve(11-92)update(27-30)base_offset(78-80)custom_components/sat/pwm.py (9)
PWM(38-247)PWMConfig(30-35)update(84-151)enabled(233-234)status(237-238)state(241-247)restore(78-82)_calculate_duty_cycle(158-230)disable(153-156)custom_components/sat/types.py (2)
PWMStatus(79-83)enabled(103-104)
🪛 Pylint (4.0.4)
custom_components/sat/entry_data.py
[refactor] 87-87: Too many public methods (22/20)
(R0904)
custom_components/sat/binary_sensor.py
[refactor] 59-59: Too few public methods (1/2)
(R0903)
custom_components/sat/pid.py
[refactor] 31-31: Too many instance attributes (17/7)
(R0902)
[refactor] 34-34: Too many arguments (8/5)
(R0913)
[refactor] 34-34: Too many positional arguments (8/5)
(R0917)
custom_components/sat/climate.py
[refactor] 71-71: Too many instance attributes (28/7)
(R0902)
[refactor] 250-250: Too many branches (16/12)
(R0912)
[refactor] 71-71: Too many public methods (29/20)
(R0904)
[refactor] 707-707: Too many branches (13/12)
(R0912)
custom_components/sat/minimum_setpoint.py
[refactor] 98-98: Too many instance attributes (8/7)
(R0902)
[refactor] 446-446: Too many local variables (16/15)
(R0914)
[refactor] 511-572: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 446-446: Too many return statements (12/6)
(R0911)
[refactor] 446-446: Too many branches (22/12)
(R0912)
[refactor] 446-446: Too many statements (58/50)
(R0915)
[refactor] 587-587: Too many local variables (18/15)
(R0914)
custom_components/sat/serial/__init__.py
[error] 33-33: Undefined variable 'DEFAULT_STATUS'
(E0602)
custom_components/sat/serial/sensor.py
[refactor] 110-110: Too many arguments (6/5)
(R0913)
[refactor] 110-110: Too many positional arguments (6/5)
(R0917)
custom_components/sat/area.py
[refactor] 37-37: Too many instance attributes (8/7)
(R0902)
custom_components/sat/serial/binary_sensor.py
[refactor] 94-94: Too many arguments (6/5)
(R0913)
[refactor] 94-94: Too many positional arguments (6/5)
(R0917)
custom_components/sat/coordinator.py
[refactor] 71-71: Too many return statements (7/6)
(R0911)
[refactor] 103-103: Too many instance attributes (14/7)
(R0902)
[refactor] 103-103: Too many public methods (49/20)
(R0904)
[refactor] 463-463: Too few public methods (1/2)
(R0903)
tests/test_pid.py
[refactor] 18-18: Too few public methods (1/2)
(R0903)
tests/test_pwm.py
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
🪛 Ruff (0.14.10)
tests/test_config_flow.py
5-5: Unused function argument: hass
(ARG001)
custom_components/sat/entry_data.py
9-9: from .const import * used; unable to detect undefined names
(F403)
19-19: StrEnum may be undefined, or defined from star imports
(F405)
70-70: HeatingMode may be undefined, or defined from star imports
(F405)
94-94: CONF_DEVICE may be undefined, or defined from star imports
(F405)
98-98: CONF_ERROR_MONITORING may be undefined, or defined from star imports
(F405)
101-101: HeatingSystem may be undefined, or defined from star imports
(F405)
102-102: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
103-103: HeatingSystem may be undefined, or defined from star imports
(F405)
105-105: HeatingSystem may be undefined, or defined from star imports
(F405)
105-105: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
105-105: HeatingSystem may be undefined, or defined from star imports
(F405)
109-109: CONF_MANUFACTURER may be undefined, or defined from star imports
(F405)
113-113: CONF_MQTT_TOPIC may be undefined, or defined from star imports
(F405)
117-117: CONF_MODE may be undefined, or defined from star imports
(F405)
124-124: CONF_NAME may be undefined, or defined from star imports
(F405)
132-132: CONF_OVERSHOOT_PROTECTION may be undefined, or defined from star imports
(F405)
136-136: CONF_PUSH_SETPOINT_TO_THERMOSTAT may be undefined, or defined from star imports
(F405)
140-140: CONF_RADIATORS may be undefined, or defined from star imports
(F405)
144-144: CONF_ROOMS may be undefined, or defined from star imports
(F405)
148-148: CONF_SIMULATION may be undefined, or defined from star imports
(F405)
152-152: CONF_THERMOSTAT may be undefined, or defined from star imports
(F405)
156-156: CONF_WINDOW_SENSORS may be undefined, or defined from star imports
(F405)
161-161: CONF_INSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
162-162: CONF_HUMIDITY_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
163-163: CONF_OUTSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
166-166: CONF_WINDOW_MINIMUM_OPEN_TIME may be undefined, or defined from star imports
(F405)
169-169: CONF_SENSOR_MAX_VALUE_AGE may be undefined, or defined from star imports
(F405)
176-176: CONF_INTEGRAL may be undefined, or defined from star imports
(F405)
177-177: CONF_DERIVATIVE may be undefined, or defined from star imports
(F405)
178-178: CONF_PROPORTIONAL may be undefined, or defined from star imports
(F405)
180-180: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
181-181: CONF_AUTOMATIC_GAINS_VALUE may be undefined, or defined from star imports
(F405)
182-182: CONF_HEATING_CURVE_COEFFICIENT may be undefined, or defined from star imports
(F405)
188-188: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
189-189: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
191-191: CONF_DYNAMIC_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
192-192: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
193-193: CONF_MAXIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
194-194: CONF_MINIMUM_SETPOINT_ADJUSTMENT_FACTOR may be undefined, or defined from star imports
(F405)
199-199: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
199-199: HeatingSystem may be undefined, or defined from star imports
(F405)
200-200: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
204-204: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
209-209: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
209-209: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
212-212: CONF_MAXIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
213-213: CONF_MINIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
215-215: CONF_CLIMATE_VALVE_OFFSET may be undefined, or defined from star imports
(F405)
216-216: CONF_TARGET_TEMPERATURE_STEP may be undefined, or defined from star imports
(F405)
221-221: HeatingMode may be undefined, or defined from star imports
(F405)
221-221: CONF_HEATING_MODE may be undefined, or defined from star imports
(F405)
224-224: CONF_AWAY_TEMPERATURE may be undefined, or defined from star imports
(F405)
224-224: CONF_AWAY_TEMPERATURE may be undefined, or defined from star imports
(F405)
225-225: CONF_HOME_TEMPERATURE may be undefined, or defined from star imports
(F405)
225-225: CONF_HOME_TEMPERATURE may be undefined, or defined from star imports
(F405)
226-226: CONF_SLEEP_TEMPERATURE may be undefined, or defined from star imports
(F405)
226-226: CONF_SLEEP_TEMPERATURE may be undefined, or defined from star imports
(F405)
227-227: CONF_COMFORT_TEMPERATURE may be undefined, or defined from star imports
(F405)
227-227: CONF_COMFORT_TEMPERATURE may be undefined, or defined from star imports
(F405)
228-228: CONF_ACTIVITY_TEMPERATURE may be undefined, or defined from star imports
(F405)
228-228: CONF_ACTIVITY_TEMPERATURE may be undefined, or defined from star imports
(F405)
233-233: CONF_THERMAL_COMFORT may be undefined, or defined from star imports
(F405)
234-234: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
235-235: CONF_SYNC_CLIMATES_WITH_PRESET may be undefined, or defined from star imports
(F405)
238-238: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
244-244: CONF_SIMULATED_HEATING may be undefined, or defined from star imports
(F405)
244-244: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
244-244: CONF_SIMULATED_HEATING may be undefined, or defined from star imports
(F405)
245-245: CONF_SIMULATED_COOLING may be undefined, or defined from star imports
(F405)
245-245: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
245-245: CONF_SIMULATED_COOLING may be undefined, or defined from star imports
(F405)
247-247: CONF_SIMULATED_WARMING_UP may be undefined, or defined from star imports
(F405)
247-247: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
247-247: CONF_SIMULATED_WARMING_UP may be undefined, or defined from star imports
(F405)
263-263: DOMAIN may be undefined, or defined from star imports
(F405)
custom_components/sat/__init__.py
36-36: Do not catch blind exception: Exception
(BLE001)
37-37: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
64-64: Do not catch blind exception: Exception
(BLE001)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
custom_components/sat/binary_sensor.py
15-15: from .const import * used; unable to detect undefined names
(F403)
25-25: DOMAIN may be undefined, or defined from star imports
(F405)
220-220: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
239-239: CycleClassification may be undefined, or defined from star imports
(F405)
240-240: CycleClassification may be undefined, or defined from star imports
(F405)
241-241: CycleClassification may be undefined, or defined from star imports
(F405)
custom_components/sat/heating_curve.py
80-80: HeatingSystem may be undefined, or defined from star imports
(F405)
custom_components/sat/pid.py
10-10: from .const import * used; unable to detect undefined names
(F403)
63-63: HeatingSystem may be undefined, or defined from star imports
(F405)
174-174: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
182-182: DEADBAND may be undefined, or defined from star imports
(F405)
186-186: DEADBAND may be undefined, or defined from star imports
(F405)
215-215: DEADBAND may be undefined, or defined from star imports
(F405)
custom_components/sat/util.py
8-8: from .const import * used; unable to detect undefined names
(F403)
69-69: DOMAIN may be undefined, or defined from star imports
(F405)
custom_components/sat/sensor.py
14-14: from .const import * used; unable to detect undefined names
(F403)
25-25: DOMAIN may be undefined, or defined from star imports
(F405)
113-113: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
351-351: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
360-360: CycleClassification may be undefined, or defined from star imports
(F405)
custom_components/sat/climate.py
35-35: from .const import * used; unable to detect undefined names
(F403)
95-95: Consider [PRESET_NONE, *list(self._presets.keys())] instead of concatenation
Replace with [PRESET_NONE, *list(self._presets.keys())]
(RUF005)
185-185: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
193-193: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
203-203: Unused method argument: event
(ARG002)
242-242: DOMAIN may be undefined, or defined from star imports
(F405)
312-312: DOMAIN may be undefined, or defined from star imports
(F405)
312-312: SERVICE_RESET_INTEGRAL may be undefined, or defined from star imports
(F405)
462-462: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
467-467: HeatingMode may be undefined, or defined from star imports
(F405)
480-480: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
480-480: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
567-567: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
571-571: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
584-584: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
590-590: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
594-594: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
605-605: MINIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
614-614: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
713-713: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
714-714: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
769-769: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
773-773: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
773-773: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
844-844: DEADBAND may be undefined, or defined from star imports
(F405)
844-844: DEADBAND may be undefined, or defined from star imports
(F405)
906-906: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/serial/__init__.py
10-10: from pyotgw.vars import * used; unable to detect undefined names
(F403)
33-33: DEFAULT_STATUS may be undefined, or defined from star imports
(F405)
custom_components/sat/area.py
221-221: Unused method argument: time
(ARG002)
296-296: Do not catch blind exception: Exception
(BLE001)
tests/test_climate.py
74-74: Unused lambda argument: self
(ARG005)
75-75: Unused lambda argument: self
(ARG005)
76-76: Unused lambda argument: self
(ARG005)
85-85: Unused lambda argument: self
(ARG005)
86-86: Unused lambda argument: self
(ARG005)
87-87: Unused lambda argument: self
(ARG005)
119-119: Unused function argument: self
(ARG001)
148-148: Unused lambda argument: self
(ARG005)
160-160: Unused lambda argument: self
(ARG005)
175-175: Unused lambda argument: self
(ARG005)
custom_components/sat/config_flow.py
117-117: CONF_MODE may be undefined, or defined from star imports
(F405)
120-120: CONF_MODE may be undefined, or defined from star imports
(F405)
162-162: CONF_MODE may be undefined, or defined from star imports
(F405)
181-181: CONF_MODE may be undefined, or defined from star imports
(F405)
214-214: CONF_MODE may be undefined, or defined from star imports
(F405)
236-236: CONF_MODE may be undefined, or defined from star imports
(F405)
237-237: CONF_DEVICE may be undefined, or defined from star imports
(F405)
237-237: f-string without any placeholders
Remove extraneous f prefix
(F541)
237-237: CONF_NAME may be undefined, or defined from star imports
(F405)
277-277: CONF_MODE may be undefined, or defined from star imports
(F405)
310-310: HeatingSystem may be undefined, or defined from star imports
(F405)
311-311: HeatingSystem may be undefined, or defined from star imports
(F405)
312-312: HeatingSystem may be undefined, or defined from star imports
(F405)
350-350: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
353-353: CONF_MODE may be undefined, or defined from star imports
(F405)
439-439: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
442-442: CONF_MODE may be undefined, or defined from star imports
(F405)
458-458: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
463-463: CONF_MODE may be undefined, or defined from star imports
(F405)
523-523: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
560-560: CONF_ROOMS may be undefined, or defined from star imports
(F405)
586-586: HeatingMode may be undefined, or defined from star imports
(F405)
587-587: HeatingMode may be undefined, or defined from star imports
(F405)
599-599: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
657-657: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
659-659: CONF_ROOMS may be undefined, or defined from star imports
(F405)
675-675: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
697-697: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
697-697: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
700-700: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
700-700: HeatingSystem may be undefined, or defined from star imports
(F405)
701-701: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
701-701: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
708-708: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
708-708: HeatingSystem may be undefined, or defined from star imports
(F405)
737-737: CONF_MODE may be undefined, or defined from star imports
(F405)
custom_components/sat/coordinator.py
15-15: from .const import * used; unable to detect undefined names
(F403)
100-100: Create your own exception
(TRY002)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
106-106: DOMAIN may be undefined, or defined from star imports
(F405)
310-310: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
| async def async_control_pid(self, _time: Optional[datetime] = None) -> None: | ||
| """Control the PID controller.""" | ||
| sensor_max_value_age = self._config.sensors.sensor_max_value_age_seconds | ||
|
|
||
| Parameters: | ||
| entity_id (str): The entity id of the sensor to track. | ||
| if sensor_max_value_age > 0: | ||
| state = self.hass.states.get(self._config.sensors.inside_sensor_entity_id) | ||
| if is_state_stale(state, sensor_max_value_age): | ||
| _LOGGER.debug("Resetting PID for %s due to stale sensor %s (age=%.1fs > %.1fs)", self.entity_id, self._config.sensors.inside_sensor_entity_id, state_age_seconds(state), sensor_max_value_age) | ||
| self.pid.reset() | ||
| return |
There was a problem hiding this comment.
Guard stale-sensor check against missing inside sensor state.
async_control_pid can call is_state_stale with state=None:
state = self.hass.states.get(self._config.sensors.inside_sensor_entity_id)
if is_state_stale(state, sensor_max_value_age):
...If the inside sensor entity is removed or renamed, this will raise.
Add a None check before calling is_state_stale:
Proposed fix
- if sensor_max_value_age > 0:
- state = self.hass.states.get(self._config.sensors.inside_sensor_entity_id)
- if is_state_stale(state, sensor_max_value_age):
+ if sensor_max_value_age > 0:
+ state = self.hass.states.get(self._config.sensors.inside_sensor_entity_id)
+ if state is not None and is_state_stale(state, sensor_max_value_age):
_LOGGER.debug(
"Resetting PID for %s due to stale sensor %s (age=%.1fs > %.1fs)",
self.entity_id,
self._config.sensors.inside_sensor_entity_id,
state_age_seconds(state),
sensor_max_value_age,
)
self.pid.reset()
return🤖 Prompt for AI Agents
In custom_components/sat/climate.py around lines 820 to 829, the code calls
is_state_stale(state, ...) without checking if state is None which will raise if
the inside sensor entity is missing; update the logic to first check whether
state is None (e.g., if state is None: log a debug/warning about missing inside
sensor, reset the PID and return) and only call is_state_stale when state is not
None, ensuring the PID is reset/returned in both stale and missing-sensor cases.
| async def async_step_simulator(self, _user_input: Optional[dict[str, Any]] = None): | ||
| if _user_input is not None: | ||
| self.data.update(_user_input) | ||
| self.data[CONF_MODE] = MODE_SIMULATOR | ||
| self.data[CONF_DEVICE] = f"%s_%s".format(MODE_SIMULATOR, snake_case(_user_input.get(CONF_NAME))) | ||
| self.data[CONF_MODE] = SatMode.SIMULATOR | ||
| self.data[CONF_DEVICE] = f"%s_%s".format(SatMode.SIMULATOR, snake_case(_user_input.get(CONF_NAME))) | ||
|
|
There was a problem hiding this comment.
Fix simulator device id formatting (CONF_DEVICE).
The current line:
self.data[CONF_DEVICE] = f"%s_%s".format(SatMode.SIMULATOR, snake_case(_user_input.get(CONF_NAME)))does not interpolate anything—"%s_%s".format(...) with %-style placeholders just returns the literal "%s_%s".
Use a real f-string (or proper .format) so each simulator gets a unique, meaningful device id:
Proposed fix
- self.data[CONF_MODE] = SatMode.SIMULATOR
- self.data[CONF_DEVICE] = f"%s_%s".format(SatMode.SIMULATOR, snake_case(_user_input.get(CONF_NAME)))
+ self.data[CONF_MODE] = SatMode.SIMULATOR
+ self.data[CONF_DEVICE] = f"{SatMode.SIMULATOR}_{snake_case(_user_input.get(CONF_NAME))}"🧰 Tools
🪛 Ruff (0.14.10)
236-236: CONF_MODE may be undefined, or defined from star imports
(F405)
237-237: CONF_DEVICE may be undefined, or defined from star imports
(F405)
237-237: f-string without any placeholders
Remove extraneous f prefix
(F541)
237-237: CONF_NAME may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In custom_components/sat/config_flow.py around lines 233 to 238, the CONF_DEVICE
assignment uses "%s_%s".format(...) which does not interpolate and yields a
literal string; replace it with a proper string interpolation such as
f"{SatMode.SIMULATOR}_{snake_case(_user_input.get(CONF_NAME))}" (or use
"{}_{}".format(...)) so each simulator gets a unique device id; ensure you call
snake_case on a non-None name (e.g., _user_input.get(CONF_NAME) or a safe
default) before composing the device id.
| @@ -1,11 +1,13 @@ | |||
| from __future__ import annotations, annotations | |||
There was a problem hiding this comment.
Remove duplicate annotations import.
from __future__ import annotations, annotations imports annotations twice.
🔎 Proposed fix
-from __future__ import annotations, annotations
+from __future__ import annotations📝 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.
| from __future__ import annotations, annotations | |
| from __future__ import annotations |
🤖 Prompt for AI Agents
In custom_components/sat/fake/__init__.py around line 1, the future import lists
`annotations` twice; remove the duplicate so the line reads a single `from
__future__ import annotations` to eliminate the redundant import and keep the
same behavior.
| return 30 | ||
|
|
||
| @property | ||
| def native_max_value(self) -> float: | ||
| """Return the maximum accepted temperature.""" | ||
| return self._coordinator.maximum_hot_water_setpoint | ||
| return 60 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if coordinator exposes hot water temperature range properties
rg -n "hot_water.*min|hot_water.*max|dhw.*min|dhw.*max" custom_components/sat/coordinator.py -A 2 -B 2Repository: Alexwijn/SAT
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Also examine the full context of the number.py file around lines 64-69
cat -n custom_components/sat/number.py | sed -n '50,75p'Repository: Alexwijn/SAT
Length of output: 971
🏁 Script executed:
#!/bin/bash
# Search for any references to temperature ranges in the entire codebase
rg -n "native_min_value|native_max_value" custom_components/sat/ -A 1 -B 1Repository: Alexwijn/SAT
Length of output: 930
🏁 Script executed:
#!/bin/bash
# Check if there are multiple boiler model definitions or configurations
fd "model|config" custom_components/sat/ -type f | head -20Repository: Alexwijn/SAT
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Get full context of number.py to understand both temperature entities
wc -l custom_components/sat/number.pyRepository: Alexwijn/SAT
Length of output: 91
🏁 Script executed:
#!/bin/bash
# View more of the number.py file to see class names and both entities
head -130 custom_components/sat/number.py | tail -80Repository: Alexwijn/SAT
Length of output: 2526
🏁 Script executed:
#!/bin/bash
# Search for any documentation about temperature ranges or boiler models
fd -e md -e rst -e txt . custom_components/sat/ 2>/dev/null | head -10Repository: Alexwijn/SAT
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or docstrings mentioning temperature ranges
rg -n "temperature.*range|valid.*range|min.*max|boiler.*model" custom_components/sat/ -i -A 1Repository: Alexwijn/SAT
Length of output: 15218
🏁 Script executed:
#!/bin/bash
# Check git log or comments about when temperature limits were changed
git log -p --all -S "native_min_value\|native_max_value" -- custom_components/sat/number.py 2>/dev/null | head -100Repository: Alexwijn/SAT
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for boiler capabilities definitions and what ranges they support
rg -n "boiler_capabilities|BoilerCapabilities" custom_components/sat/ -A 3 -B 1Repository: Alexwijn/SAT
Length of output: 8301
🏁 Script executed:
#!/bin/bash
# Check what values are used in coordinator for actual control
rg -n "hot_water.*setpoint|async_set_control_hot_water" custom_components/sat/coordinator.py -A 2 -B 2Repository: Alexwijn/SAT
Length of output: 1090
🏁 Script executed:
#!/bin/bash
# Search for any validation or bounds checking related to setpoints
rg -n "clamp.*setpoint|setpoint.*30|setpoint.*60" custom_components/sat/ -iRepository: Alexwijn/SAT
Length of output: 1387
🏁 Script executed:
#!/bin/bash
# Check the full coordinator.py file around minimum/maximum_hot_water_setpoint properties
grep -n "class.*Coordinator" custom_components/sat/coordinator.py | head -5Repository: Alexwijn/SAT
Length of output: 211
🏁 Script executed:
#!/bin/bash
# See if these properties are implemented differently in subclasses
rg -n "minimum_hot_water_setpoint|maximum_hot_water_setpoint" custom_components/sat/ -B 2 -A 2Repository: Alexwijn/SAT
Length of output: 3939
🏁 Script executed:
#!/bin/bash
# Check boiler.py to understand BoilerCapabilities and if hot water should be included
cat -n custom_components/sat/boiler.py | head -60Repository: Alexwijn/SAT
Length of output: 2005
🏁 Script executed:
#!/bin/bash
# Check if hot water setpoint should use device_capabilities instead
grep -n "async_set_control_hot_water_setpoint\|supports_hot_water" custom_components/sat/number.py -B 5 -A 5Repository: Alexwijn/SAT
Length of output: 1038
Use coordinator properties instead of hard-coding hot water temperature limits.
The entity hard-codes min/max values (30°C, 60°C) instead of using the coordinator's minimum_hot_water_setpoint and maximum_hot_water_setpoint properties. These coordinator properties can be overridden by device-specific data from OpenTherm, Serial, and ESPHome protocols, allowing different boiler models to report their actual supported ranges. By hard-coding, the entity ignores potentially different limits reported by the device.
Replace the hard-coded returns with self._coordinator.minimum_hot_water_setpoint and self._coordinator.maximum_hot_water_setpoint respectively.
🤖 Prompt for AI Agents
In custom_components/sat/number.py around lines 64 to 69, the entity currently
hard-codes native_min_value and native_max_value to 30 and 60; replace those
returns to use the coordinator-provided ranges instead. Change native_min_value
to return self._coordinator.minimum_hot_water_setpoint and native_max_value to
return self._coordinator.maximum_hot_water_setpoint so device-specific limits
supplied by OpenTherm/Serial/ESPHome are respected.
| if not self._coordinator.device_active or not self._coordinator.flame_active: | ||
| return 0.0 | ||
|
|
||
| differential_gas_consumption = self._maximum_consumption - self._minimum_consumption | ||
| relative_modulation_value = self._coordinator.relative_modulation_value | ||
|
|
||
| return round(self._minimum_consumption + ((relative_modulation_value / 100) * differential_gas_consumption), 3) | ||
| modulation_fraction = relative_modulation_value / 100 | ||
| return round(self._minimum_consumption + (modulation_fraction * differential_gas_consumption), 3) | ||
|
|
There was a problem hiding this comment.
Defensively handle relative_modulation_value being None in SatCurrentConsumptionSensor.native_value.
available checks relative_modulation_value is not None, but HA may still call native_value when available is false. In that case, this line will fail:
modulation_fraction = relative_modulation_value / 100Add a simple None check:
Proposed fix
- relative_modulation_value = self._coordinator.relative_modulation_value
-
- modulation_fraction = relative_modulation_value / 100
+ relative_modulation_value = self._coordinator.relative_modulation_value
+ if relative_modulation_value is None:
+ return 0.0
+
+ modulation_fraction = relative_modulation_value / 100🤖 Prompt for AI Agents
In custom_components/sat/sensor.py around lines 243 to 251, native_value assumes
self._coordinator.relative_modulation_value is not None and will raise if HA
calls it while unavailable; add a defensive None check for
relative_modulation_value at the start of the calculation (e.g., if
relative_modulation_value is None: return 0.0 or another sensible fallback) so
the method returns a valid float instead of raising, then proceed with the
existing modulation_fraction calculation and rounding when the value is present.
| def test_setpoint_uses_default_when_maximum_missing(hass): | ||
| coordinator = _build_coordinator(hass) | ||
| protection = OvershootProtection(coordinator, HeatingSystem.RADIATORS) | ||
| assert protection._setpoint == float(coordinator.maximum_setpoint_value) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the _build_coordinator function in the test file
rg -n "_build_coordinator" tests/test_overshoot_protection.py -A 8Repository: Alexwijn/SAT
Length of output: 4387
🏁 Script executed:
# Find the OVERSHOOT_PROTECTION_SETPOINT definition
rg -n "OVERSHOOT_PROTECTION_SETPOINT" custom_components/sat/ -A 3Repository: Alexwijn/SAT
Length of output: 1091
🏁 Script executed:
# Find OvershootProtection.__init__ to see how _setpoint is assigned
rg -n "class OvershootProtection" custom_components/sat/ -A 30Repository: Alexwijn/SAT
Length of output: 3651
🏁 Script executed:
# Check if there's a fake coordinator and what maximum_setpoint_value returns
fd "fake" custom_components/sat/ -type f | head -20Repository: Alexwijn/SAT
Length of output: 226
🏁 Script executed:
# Find SatFakeCoordinator and its maximum_setpoint_value property/method
rg -n "class SatFakeCoordinator" custom_components/sat/ -A 50 | head -100Repository: Alexwijn/SAT
Length of output: 3566
🏁 Script executed:
# Search for maximum_setpoint_value property definition in the fake coordinator or base coordinator
rg -n "maximum_setpoint_value" custom_components/sat/ -B 2 -A 2Repository: Alexwijn/SAT
Length of output: 4173
🏁 Script executed:
# Search for the maximum_setpoint property in coordinator.py
rg -n "def maximum_setpoint" custom_components/sat/coordinator.py -B 2 -A 5Repository: Alexwijn/SAT
Length of output: 688
🏁 Script executed:
# Also check the SatFakeCoordinator to see if it overrides maximum_setpoint
rg -n "maximum_setpoint" custom_components/sat/fake/__init__.py -B 2 -A 5Repository: Alexwijn/SAT
Length of output: 1677
🏁 Script executed:
# Check what happens with the config - see if there's a default maximum_setpoint
rg -n "calculate_default_maximum_setpoint" custom_components/sat/ -B 2 -A 8Repository: Alexwijn/SAT
Length of output: 6347
🏁 Script executed:
# Get the full maximum_setpoint property implementation
rg -n "def maximum_setpoint" custom_components/sat/coordinator.py -A 10Repository: Alexwijn/SAT
Length of output: 941
🏁 Script executed:
# Check the test's DEFAULT_USER_DATA to understand the initial config
rg -n "DEFAULT_USER_DATA" tests/ -B 2 -A 5Repository: Alexwijn/SAT
Length of output: 3677
🏁 Script executed:
# Get the full DEFAULT_USER_DATA definition
rg -n "DEFAULT_USER_DATA" tests/const.py -A 10Repository: Alexwijn/SAT
Length of output: 611
🏁 Script executed:
# Check OPTIONS_DEFAULTS to see if CONF_MAXIMUM_SETPOINT is set there
rg -n "OPTIONS_DEFAULTS" tests/const.py -A 10Repository: Alexwijn/SAT
Length of output: 546
🏁 Script executed:
# Find OPTIONS_DEFAULTS definition
rg -n "OPTIONS_DEFAULTS" custom_components/sat/ -B 2 -A 15Repository: Alexwijn/SAT
Length of output: 14868
🏁 Script executed:
# Check if there are any other similar tests that show the correct pattern
rg -n "test_setpoint" tests/test_overshoot_protection.py -A 3Repository: Alexwijn/SAT
Length of output: 568
Assert against the correct default setpoint value.
The test name suggests it should verify _setpoint equals the OVERSHOOT_PROTECTION_SETPOINT default (62 for RADIATORS), but the assertion compares against coordinator.maximum_setpoint_value. The coordinator never returns None for maximum_setpoint_value; it calculates a default of 55 for RADIATORS when not explicitly configured. The test passes coincidentally because OvershootProtection clamps to min(62, 55) = 55, matching the coordinator's calculated value. To properly test the default behavior, assert against OVERSHOOT_PROTECTION_SETPOINT[HeatingSystem.RADIATORS] instead.
🤖 Prompt for AI Agents
In tests/test_overshoot_protection.py around lines 29-32, the assertion compares
protection._setpoint to coordinator.maximum_setpoint_value but should assert the
default overshoot setpoint for radiators; change the assertion to compare
protection._setpoint to OVERSHOOT_PROTECTION_SETPOINT[HeatingSystem.RADIATORS]
(and add or adjust the import for OVERSHOOT_PROTECTION_SETPOINT if not already
imported).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
custom_components/sat/pid.py (1)
61-65: Constructor parameterautomatic_gain_valueis still hardcoded and ignored.Despite past review comments indicating this was addressed in commits 716b320 to 28880d1, line 64 still hardcodes
automatic_gain_valueas 4 or 3 based on heating system, completely ignoringself._automatic_gains_valuefrom the constructor. This means the configured value is never used.🔎 Proposed fix
if self._heating_curve is None: return 0.0 - automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3 + # Use configured value, falling back to default based on heating system if not set + if self._automatic_gains_value > 0: + automatic_gain_value = self._automatic_gains_value + else: + automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3 return round((self._heating_curve_coefficient * self._heating_curve) / automatic_gain_value, 6)Based on learnings, this issue was previously flagged but remains unresolved despite being marked as addressed.
tests/test_pid.py (1)
73-92: Test validates buggy behavior and will fail when PID is fixed.This test passes
automatic_gain_value=2.0(line 76) but expectskp == 20.0(line 89), which is calculated using the hardcoded value of 4 instead of the configured 2.0. Once the bug inpid.pyline 64 is fixed to useself._automatic_gains_value, the expected value should be:kp = (heating_curve_coefficient * heating_curve) / automatic_gain_value = (2.0 * 40.0) / 2.0 = 40.0🔎 Proposed fix (after PID bug is resolved)
- assert pid.kp == 20.0 - assert pid.ki == round(20.0 / 8400, 6) - assert pid.kd == round(0.07 * 8400 * 20.0, 6) + assert pid.kp == 40.0 + assert pid.ki == round(40.0 / 8400, 6) + assert pid.kd == round(0.07 * 8400 * 40.0, 6)Based on learnings, this issue was previously flagged and remains dependent on fixing the PID implementation.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/sat/pid.pytests/test_pid.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/pid.py (4)
custom_components/sat/helpers.py (3)
clamp(108-113)float_value(78-87)timestamp(12-14)custom_components/sat/area.py (4)
temperature_state(83-102)get(255-261)update(221-231)error(127-145)custom_components/sat/temperature_state.py (3)
TemperatureState(38-49)error(47-49)max(30-34)custom_components/sat/const.py (1)
HeatingSystem(97-101)
🪛 Pylint (4.0.4)
custom_components/sat/pid.py
[refactor] 32-32: Too many instance attributes (17/7)
(R0902)
[refactor] 35-35: Too many arguments (8/5)
(R0913)
[refactor] 35-35: Too many positional arguments (8/5)
(R0917)
🪛 Ruff (0.14.10)
custom_components/sat/pid.py
10-10: from .const import * used; unable to detect undefined names
(F403)
64-64: HeatingSystem may be undefined, or defined from star imports
(F405)
177-177: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
185-185: DEADBAND may be undefined, or defined from star imports
(F405)
189-189: DEADBAND may be undefined, or defined from star imports
(F405)
222-222: DEADBAND may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
custom_components/sat/pid.py (3)
134-157: LGTM! Storage restoration is well-structured.The storage restoration logic properly handles optional fields, uses safe float conversion, and scopes storage per entity and device.
179-216: LGTM! Integral logic is well-designed.The integral implementation properly handles deadband transitions, prevents windup through clamping, and caps integration intervals to avoid over-accumulation from sensor gaps.
217-253: LGTM! Derivative filtering is robust.The two-stage low-pass filtering and capping logic effectively reduce noise while preventing extreme derivative values. The deadband freeze and sensor timing checks are appropriate safeguards.
tests/test_pid.py (1)
1-205: Excellent test coverage for PID controller.The test suite comprehensively covers initialization, manual/automatic gains, integral accumulation and clamping, derivative filtering and capping, deadband behavior, and timing considerations. The tests are well-structured and use appropriate assertions.
| def last_updated(self) -> float: | ||
| """Return the timestamp of the last update to the PID controller.""" | ||
| return self._last_updated | ||
| self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id) |
There was a problem hiding this comment.
Critical: async_dispatcher_send cannot be used with call_soon_threadsafe.
Line 177 passes async_dispatcher_send (a coroutine function) directly to call_soon_threadsafe, which expects a regular callable. This will fail because calling async_dispatcher_send(...) returns a coroutine object that needs to be scheduled, not a value.
This may be related to the TypeError reported in PR comments: "TypeError: a coroutine was expected, got ".
🔎 Proposed fix
- self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id)
+ async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id)Since update() is already called from an async context (the coordinator), you can call async_dispatcher_send directly without thread-safe wrapping. If update() is sometimes called from a sync context, use:
- self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id)
+ self._hass.async_create_task(async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
177-177: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In custom_components/sat/pid.py around line 177, calling
self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass,
SIGNAL_PID_UPDATED, self._entity_id) is wrong because async_dispatcher_send
should be invoked directly (or scheduled as a coroutine), not passed into
call_soon_threadsafe; replace that line with a direct call to
async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id) when
update() runs in async context, and if update() can be called from sync contexts
instead, schedule it via Home Assistant's async job scheduler (e.g.,
self._hass.async_add_job / self._hass.add_job or call_soon_threadsafe with a
lambda that creates an async task) so the dispatcher is properly scheduled
rather than passed as a coroutine object.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
custom_components/sat/pid.py (2)
35-65: Constructor parameterautomatic_gain_valueremains unused.Line 64 still hardcodes the gain value instead of using
self._automatic_gains_valuepassed to the constructor. This issue was flagged in previous reviews but remains unresolved.
177-177: Critical:async_dispatcher_sendmisuse withcall_soon_threadsafe.This issue was flagged in previous reviews and likely causes the TypeError mentioned in PR comments ("a coroutine was expected"). The
call_soon_threadsafepattern is incorrect forasync_dispatcher_send.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/sat/pid.pytests/test_pid.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_pid.py
🧰 Additional context used
🪛 Pylint (4.0.4)
custom_components/sat/pid.py
[refactor] 32-32: Too many instance attributes (17/7)
(R0902)
[refactor] 35-35: Too many arguments (8/5)
(R0913)
[refactor] 35-35: Too many positional arguments (8/5)
(R0917)
🪛 Ruff (0.14.10)
custom_components/sat/pid.py
10-10: from .const import * used; unable to detect undefined names
(F403)
64-64: HeatingSystem may be undefined, or defined from star imports
(F405)
177-177: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
185-185: DEADBAND may be undefined, or defined from star imports
(F405)
189-189: DEADBAND may be undefined, or defined from star imports
(F405)
222-222: DEADBAND may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
custom_components/sat/pid.py (4)
1-30: LGTM: Storage constants and imports are well-structured.The storage-backed state management constants and imports properly support the cycle-centric architecture refactor. The derivative filtering constants and storage keys are clearly named and appropriate.
67-120: LGTM: Property implementations are correct.The ki/kd automatic gain calculations, component properties (proportional/integral/derivative), and output computation all correctly implement PID logic with proper null checks and rounding.
122-156: LGTM: State management initialization is robust.The
reset()method properly initializes all state variables, andasync_added_to_hass()correctly restores PID state from storage with appropriate defaults and nullable field handling.
179-268: LGTM: PID component update logic is well-implemented.The integral update correctly handles deadband boundaries, time capping, and clamping to heating curve bounds. The derivative calculation uses appropriate low-pass filtering with magnitude capping. State persistence is straightforward and correct.
| """Return the previous error value used by the PID controller.""" | ||
| return self._previous_error | ||
| if self._store is not None: | ||
| self._hass.create_task(self._async_save_state()) |
There was a problem hiding this comment.
Fix: Use async_create_task instead of create_task.
Home Assistant's HomeAssistant object uses async_create_task, not create_task. This will raise an AttributeError.
🔎 Proposed fix
- self._hass.create_task(self._async_save_state())
+ self._hass.async_create_task(self._async_save_state())📝 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.
| self._hass.create_task(self._async_save_state()) | |
| self._hass.async_create_task(self._async_save_state()) |
🤖 Prompt for AI Agents
In custom_components/sat/pid.py around line 175, replace the incorrect call to
self._hass.create_task(self._async_save_state()) with Home Assistant's supported
async task helper; use self._hass.async_create_task(self._async_save_state())
instead so the coroutine is scheduled correctly and no AttributeError is raised.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
custom_components/sat/pid.py (2)
35-53: Constructor storesautomatic_gain_valuebut the value is ignored.The parameter
automatic_gain_valueis stored at line 41, but line 69 computes the gain using hardcoded values (4or3) instead of usingself._automatic_gains_value. This was previously flagged and marked as resolved, but the issue persists in the current code.Either use the stored value or remove the parameter from the constructor.
🔎 Proposed fix (use the stored parameter)
- automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3 + automatic_gain_value = self._automatic_gains_value if self._automatic_gains_value > 0 else (4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3)
173-176: Critical: Both task scheduling calls are incorrect and causing runtime errors.These bugs match the TypeError reported in PR comments ("a coroutine was expected, got <bound method>"):
Line 174:
self._hass.create_task()does not exist on HomeAssistant — useasync_create_task().Line 176:
call_soon_threadsafe(async_dispatcher_send, ...)is incorrect.async_dispatcher_sendreturns a coroutine when called, butcall_soon_threadsafeexpects a regular callable. The coroutine is never scheduled.Since
update()is called from an async context (the coordinator), thread-safe wrapping is unnecessary.🔎 Proposed fix
if self._store is not None: - self._hass.create_task(self._async_save_state()) + self._hass.async_create_task(self._async_save_state()) - self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id) + async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/pid.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/sat/pid.py (5)
custom_components/sat/helpers.py (2)
clamp(108-113)float_value(78-87)custom_components/sat/area.py (6)
temperature_state(83-102)async_added_to_hass(202-213)async_added_to_hass(271-274)get(255-261)update(221-231)error(127-145)custom_components/sat/temperature_state.py (3)
TemperatureState(38-49)error(47-49)max(30-34)custom_components/sat/climate.py (2)
async_added_to_hass(159-201)error(380-399)custom_components/sat/boiler.py (2)
async_added_to_hass(125-141)update(150-164)
🪛 Pylint (4.0.4)
custom_components/sat/pid.py
[refactor] 32-32: Too many instance attributes (17/7)
(R0902)
[refactor] 35-35: Too many arguments (8/5)
(R0913)
[refactor] 35-35: Too many positional arguments (8/5)
(R0917)
🪛 Ruff (0.14.10)
custom_components/sat/pid.py
10-10: from .const import * used; unable to detect undefined names
(F403)
69-69: HeatingSystem may be undefined, or defined from star imports
(F405)
176-176: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
184-184: DEADBAND may be undefined, or defined from star imports
(F405)
188-188: DEADBAND may be undefined, or defined from star imports
(F405)
221-221: DEADBAND may be undefined, or defined from star imports
(F405)
🔇 Additional comments (6)
custom_components/sat/pid.py (6)
17-29: LGTM!The constants are well-defined with clear naming conventions. The dual-stage low-pass filter coefficients (DERIVATIVE_ALPHA1/2), derivative cap, and storage keys are appropriately organized.
55-86: LGTM!The gain properties correctly handle the automatic gains calculation with proper null checks. The
availableproperty appropriately guards state access.
88-125: LGTM!The PID term properties are well-implemented with consistent null guards and appropriate rounding precision. The output calculation correctly combines all terms.
178-214: LGTM!The integral update logic is well-designed with:
- Anti-windup clamping to heating curve bounds
- Proper deadband handling (only accumulates when error is small)
- Time gap capping to prevent over-accumulation after sensor outages
216-252: LGTM!The derivative update uses a derivative-on-measurement approach (negative temperature delta) to avoid derivative kick on setpoint changes. The dual-stage low-pass filter effectively reduces noise while the magnitude cap prevents outlier spikes.
254-267: LGTM!The state persistence is clean and comprehensive, storing all necessary fields for proper restoration.
…ng curves, and area controllers.
There was a problem hiding this comment.
Actionable comments posted: 15
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/sensor.py (1)
237-255: Guard againstNonerelative modulation value in calculation.While
availablereturnsFalsewhenrelative_modulation_value is None, Home Assistant may still callnative_valuewhen unavailable. Line 249 will raiseTypeErrorwhen dividingNoneby 100.🔎 Proposed fix
@property def native_value(self) -> float: """Return the state of the device in native units.""" if not self._coordinator.device_active or not self._coordinator.flame_active: return 0.0 differential_gas_consumption = self._maximum_consumption - self._minimum_consumption relative_modulation_value = self._coordinator.relative_modulation_value + if relative_modulation_value is None: + return 0.0 modulation_fraction = relative_modulation_value / 100 return round(self._minimum_consumption + (modulation_fraction * differential_gas_consumption), 3)
🤖 Fix all issues with AI Agents
In @custom_components/sat/area.py:
- Around line 83-102: The sensor validation in temperature_state incorrectly
checks HVACMode.OFF (a climate enum) against sensor states; locate the
temperature_state method and the condition that compares sensor_state.state to
[STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF] (uses
ATTR_SENSOR_TEMPERATURE_ID and is_state_stale) and remove HVACMode.OFF or
replace it with the literal string "off" if you intend to filter sensors
reporting "off"; update the conditional to only use sensor-appropriate constants
(STATE_UNKNOWN, STATE_UNAVAILABLE, and optionally "off") and ensure logging and
stale checks remain unchanged.
In @custom_components/sat/binary_sensor.py:
- Around line 139-145: The is_on property can raise TypeError when calling int()
on None; update the is_on method (in binary_sensor.py) to first check
self._climate.relative_modulation_value and
self._coordinator.maximum_relative_modulation_value for None and only call int()
when both are not None; if either is None, return a safe boolean via
self.state_delayed(False) (or another explicit fallback) so the property never
attempts int(None) and always returns a bool.
In @custom_components/sat/climate.py:
- Around line 860-875: The code is passing an async coroutine method to HassJob
which raises ValueError; update the async_call_later call in
schedule_control_heating_loop so HassJob receives a synchronous callable that
schedules the coroutine instead. Replace
HassJob(self.async_control_heating_loop) with something that invokes
hass.async_create_task (e.g. HassJob(lambda _:
self.hass.async_create_task(self.async_control_heating_loop()))) so
async_control_heating_loop is scheduled correctly; keep the rest of the logic
and still cancel/clear _control_heating_loop_unsub as before.
- Around line 1047-1051: The _build_presets function builds conf_presets then
uses a dict comprehension that incorrectly checks "if key in conf_presets"
(always true) causing KeyError; change the comprehension to only include entries
whose mapped config key exists by replacing the condition with "if value in
config_options" (or use config_options.get(value) with a fallback) so it returns
a dict of preset_name: config_options[value] for available keys (conf_presets,
_build_presets, config_options,
PRESET_ACTIVITY/PRESET_AWAY/PRESET_HOME/PRESET_SLEEP/PRESET_COMFORT).
In @custom_components/sat/coordinator.py:
- Around line 143-149: The property maximum_setpoint_value is currently
annotated as returning Optional[float] but always forwards self.maximum_setpoint
(a float), causing a type mismatch; update the return type annotation of
maximum_setpoint_value from Optional[float] to float to match actual behavior
and keep type checkers happy, and ensure callers like device_capabilities (which
builds BoilerCapabilities with maximum_setpoint=maximum_setpoint_value) reflect
the non-optional float contract.
In @custom_components/sat/entry_data.py:
- Around line 171-181: The pid property builds a PidConfig but calls float(...)
and bool(...) directly on self.options.get(...) which will raise TypeError if
any option is missing; update the pid property to use safe conversions or
provide defaults to self.options.get(...) for each field (integral, derivative,
proportional, automatic_gains, automatic_gains_value, heating_curve_coefficient)
and coerce values only after defaulting (e.g. default numeric fields to 0.0 and
automatic_gains to False) so PidConfig(...) never receives None; ensure you
reference the PidConfig constructor and CONF_INTEGRAL / CONF_DERIVATIVE /
CONF_PROPORTIONAL / CONF_AUTOMATIC_GAINS / CONF_AUTOMATIC_GAINS_VALUE /
CONF_HEATING_CURVE_COEFFICIENT keys when making the changes.
- Around line 117-119: The property SatEntryData.mode currently calls
SatMode(self.data.get(CONF_MODE)) which will raise ValueError if CONF_MODE is
missing; update the mode property to first read value = self.data.get(CONF_MODE)
and if value is None return an appropriate fallback (e.g., None or a default
SatMode) or raise a clearer error, otherwise construct SatMode(value); reference
the mode property, SatMode, self.data, and CONF_MODE when making the change.
- Around line 217-219: The presets property calls
HeatingMode(self.options.get(CONF_HEATING_MODE)) which will raise ValueError if
CONF_HEATING_MODE is missing; change presets to first retrieve value =
self.options.get(CONF_HEATING_MODE) and only construct HeatingMode(value) when
value is not None (or wrap in try/except ValueError) and fall back to the same
default/None behavior used for SatMode so missing option does not raise.
In @custom_components/sat/mqtt/__init__.py:
- Around line 23-25: self._config.device is Optional[str] so calling
str(self._config.device) can produce "None"; update the initialization of
self._device_id (and the Store key f"sat.mqtt.{self._device_id}") to
validate/normalize the device value from SatConfig.device: if it's truthy and
matches expected format use it, otherwise generate a safe fallback (e.g.,
uuid.uuid4().hex) or raise a clear config error; modify the code that sets
self._device_id to use that validated/fallback value and add the necessary
import for uuid if you choose the generated-id approach.
In @custom_components/sat/pid.py:
- Around line 57-67: The kp property currently ignores
PidConfig.automatic_gains_value and hardcodes automatic_gain_value based on
HeatingSystem; update the logic in the kp getter to use
self._config.automatic_gains_value when automatic gains are enabled (falling
back to the heating-system-based default only if automatic_gains_value is None
or invalid), i.e., replace the hardcoded 4/3 selection with a value derived from
self._config.automatic_gains_value and then compute and return
round((self._config.heating_curve_coefficient * self._heating_curve.value) /
automatic_gain_value, 6) as before.
- Around line 139-142: Restoration uses inconsistent conversion: replace the
bare float() calls for self._integral and self._raw_derivative with the safe
conversion helper float_value (same used for self._last_error and
self._last_temperature) so corrupted or missing stored values don't raise
exceptions during state restoration; update the assignments that reference
STORAGE_KEY_INTEGRAL and STORAGE_KEY_RAW_DERIVATIVE to call float_value(...) and
supply the existing defaults (self._integral, self._raw_derivative) as before.
- Around line 154-156: The method set_heating_curve_value is dead because the
class uses self._heating_curve.value elsewhere; remove the method or change it
to update the real object (e.g., replace the body to set
self._heating_curve.value = heating_curve_value or call the actual heater curve
setter on the object) and ensure self._heating_curve exists (raise or initialize
if needed) instead of assigning to the unused self._heating_curve_value
attribute.
- Around line 176-179: The update() handler incorrectly uses
HomeAssistant.create_task and call_soon_threadsafe with async_dispatcher_send;
change self._hass.create_task(self._async_save_state()) to
self._hass.async_create_task(self._async_save_state()) and remove
call_soon_threadsafe: call async_dispatcher_send directly from the async context
as async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id) (i.e.,
dispatch directly in update()), ensuring the coroutine is scheduled/executed
properly.
In @custom_components/sat/serial/__init__.py:
- Around line 30-36: The constructor __init__ sets self._port =
str(self._config.device) which turns None into the string "None" and breaks
pyotgw; fix by checking self._config.device before coercion (e.g., if
self._config.device is not None assign its str() to self._port, otherwise set
self._port to None or an empty string) so the OpenThermGateway connection gets a
valid missing-port value; update the assignment near __init__ and ensure
downstream code using self._port can handle the chosen None/empty value.
🧹 Nitpick comments (6)
tests/test_pid.py (1)
32-48: Static analysis warning is a false positive.The Pylint warning about too many arguments (6/5) for
_pid_configis acceptable here. PID configuration naturally requires multiple parameters (proportional, integral, derivative, automatic gains settings, and heating curve coefficient), and consolidating them into a helper function improves test readability.tests/test_climate.py (2)
74-76: Static analysis warnings about unused lambda arguments are false positives.These lambdas are used with
monkeypatch.setattrto replace properties. Properties always receiveselfas the first argument, even though these particular lambdas return constant values and don't need to useself. This is the correct pattern for mocking properties.Also applies to: 85-87
220-224: Consider using a test helper or fixture to set PWM state.Lines 220, 223, and 255 directly set the private
_enabledattribute for test setup. While this may be necessary for testing hysteresis behavior, consider whether:
- A public setter method should be added to PWM for testing purposes
- A test fixture/helper should encapsulate this setup
- The test should use a different approach that doesn't require direct attribute access
The current approach works but may be fragile if the internal implementation changes.
Also applies to: 255-255
custom_components/sat/binary_sensor.py (1)
67-79: Wall-clock time is susceptible to clock adjustments.
timestamp()uses wall-clock time viadt.utcnow().timestamp(). System clock adjustments (NTP sync, manual changes) can cause the 60-second delay to behave unexpectedly. A backward clock jump prevents the condition from triggering; a forward jump causes premature alerts.Consider using
time.monotonic()for elapsed time measurement.🔎 Proposed fix
+import time + class SatSyncSensor: """Mixin to add delayed state change for binary sensors.""" def __init__(self, delay: int = 60): """Initialize the mixin with a delay.""" self._delay = delay self._last_mismatch: Optional[float] = None def state_delayed(self, condition: bool) -> bool: """Determine the delayed state based on a condition.""" if not condition: self._last_mismatch = None return False if self._last_mismatch is None: - self._last_mismatch = timestamp() + self._last_mismatch = time.monotonic() - if seconds_since(self._last_mismatch) >= self._delay: + if (time.monotonic() - self._last_mismatch) >= self._delay: return True return Falsecustom_components/sat/sensor.py (1)
69-91: Consider using stable identifier forunique_id.The
unique_idusesself._config.name_lowerwhich derives from the user-configurableCONF_NAME. If users rename the integration, entity tracking and automations will break.Consider using a stable identifier like
entry_idinstead:@property def unique_id(self) -> str: - return f"{self._config.name_lower}-requested-setpoint" + return f"{self._coordinator.device_id}-requested-setpoint"This pattern should be applied consistently across all sensor
unique_idproperties if the concern is valid. Based on learnings from past reviews, this was flagged but not yet addressed.custom_components/sat/area.py (1)
307-311: Consider logging specific exception type instead of catching broadException.Line 309 catches all exceptions when computing PID output. While the intent to log and continue is reasonable, catching
Exceptioncan mask unexpected errors.🔎 Proposed refinement
try: value = area.pid.output - except Exception as exception: + except (TypeError, ValueError, AttributeError) as exception: _LOGGER.warning("Failed to compute PID output for area %s: %s", area.id, exception) continueThis targets the expected failure modes while allowing truly unexpected exceptions to propagate.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
custom_components/sat/area.pycustom_components/sat/binary_sensor.pycustom_components/sat/climate.pycustom_components/sat/coordinator.pycustom_components/sat/entry_data.pycustom_components/sat/esphome/__init__.pycustom_components/sat/heating_curve.pycustom_components/sat/helpers.pycustom_components/sat/mqtt/__init__.pycustom_components/sat/pid.pycustom_components/sat/sensor.pycustom_components/sat/serial/__init__.pycustom_components/sat/switch/__init__.pycustom_components/sat/util.pytests/test_area.pytests/test_climate.pytests/test_pid.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_area.py
🧰 Additional context used
🧬 Code graph analysis (14)
custom_components/sat/binary_sensor.py (2)
custom_components/sat/entry_data.py (6)
SatConfig(89-246)SatMode(19-26)mode(118-119)sensors(157-169)name(125-126)name_lower(129-130)custom_components/sat/helpers.py (2)
seconds_since(22-24)timestamp(12-14)
tests/test_climate.py (6)
custom_components/sat/entry_data.py (3)
SatConfig(89-246)pid(172-181)pwm(184-193)custom_components/sat/const.py (2)
HeatingMode(104-106)HeatingSystem(97-101)custom_components/sat/heating_curve.py (5)
HeatingCurve(11-88)update(23-26)value(87-88)calculate(69-71)base_offset(74-76)custom_components/sat/pid.py (2)
PID(34-270)update(158-179)custom_components/sat/types.py (1)
DeviceState(73-76)custom_components/sat/minimum_setpoint.py (1)
value(286-287)
custom_components/sat/esphome/__init__.py (3)
custom_components/sat/coordinator.py (2)
SatDataUpdateCoordinator(103-457)get(461-471)custom_components/sat/entry_data.py (3)
SatConfig(89-246)device(95-96)simulation(240-246)custom_components/sat/types.py (2)
DeviceState(73-76)enabled(103-104)
custom_components/sat/heating_curve.py (3)
custom_components/sat/entry_data.py (1)
heating_system(103-107)custom_components/sat/const.py (1)
HeatingSystem(97-101)custom_components/sat/minimum_setpoint.py (1)
value(286-287)
custom_components/sat/coordinator.py (4)
custom_components/sat/boiler.py (8)
Boiler(77-439)BoilerState(50-74)BoilerCapabilities(43-46)BoilerControlIntent(37-39)status(99-103)flame_on_since(118-119)flame_off_since(122-123)update(150-164)custom_components/sat/cycles.py (6)
CycleTracker(251-503)CycleHistory(96-248)CycleStatistics(76-80)Cycle(53-72)statistics(181-199)update(278-309)custom_components/sat/entry_data.py (7)
SatConfig(89-246)SatMode(19-26)manufacturer(110-111)pwm(184-193)mode(118-119)limits(196-215)heating_system(103-107)custom_components/sat/types.py (2)
BoilerStatus(15-41)DeviceState(73-76)
custom_components/sat/mqtt/__init__.py (1)
custom_components/sat/entry_data.py (3)
SatConfig(89-246)device(95-96)mqtt_topic(114-115)
custom_components/sat/helpers.py (3)
custom_components/sat/state.py (1)
State(10-12)custom_components/sat/const.py (1)
HeatingSystem(97-101)custom_components/sat/entry_data.py (1)
heating_system(103-107)
custom_components/sat/serial/__init__.py (3)
custom_components/sat/coordinator.py (2)
SatDataUpdateCoordinator(103-457)member_id(199-201)custom_components/sat/entry_data.py (3)
SatConfig(89-246)device(95-96)simulation(240-246)custom_components/sat/types.py (2)
DeviceState(73-76)enabled(103-104)
custom_components/sat/switch/__init__.py (3)
custom_components/sat/coordinator.py (1)
SatDataUpdateCoordinator(103-457)custom_components/sat/entry_data.py (3)
SatConfig(89-246)device(95-96)simulation(240-246)custom_components/sat/types.py (2)
DeviceState(73-76)enabled(103-104)
custom_components/sat/entry_data.py (2)
custom_components/sat/helpers.py (2)
calculate_default_maximum_setpoint(65-67)convert_time_str_to_seconds(40-49)custom_components/sat/const.py (2)
HeatingMode(104-106)HeatingSystem(97-101)
custom_components/sat/climate.py (4)
custom_components/sat/entry_data.py (3)
presets(218-237)pwm(184-193)overshoot_protection(133-134)custom_components/sat/helpers.py (2)
clamp(108-113)ensure_list(126-134)custom_components/sat/types.py (2)
PWMDecision(86-104)enabled(103-104)custom_components/sat/const.py (1)
HeatingMode(104-106)
custom_components/sat/pid.py (5)
custom_components/sat/entry_data.py (2)
PidConfig(40-46)heating_system(103-107)custom_components/sat/heating_curve.py (4)
HeatingCurve(11-88)reset(17-21)value(87-88)update(23-26)custom_components/sat/helpers.py (3)
float_value(78-87)timestamp(12-14)clamp_to_range(116-118)custom_components/sat/temperature_state.py (3)
TemperatureState(38-49)error(47-49)max(30-34)custom_components/sat/const.py (1)
HeatingSystem(97-101)
custom_components/sat/sensor.py (3)
custom_components/sat/entity.py (1)
SatEntity(20-39)custom_components/sat/entry_data.py (4)
SatConfig(89-246)SatMode(19-26)mode(118-119)name(125-126)custom_components/sat/types.py (2)
BoilerStatus(15-41)CycleClassification(54-70)
tests/test_pid.py (3)
custom_components/sat/heating_curve.py (3)
HeatingCurve(11-88)value(87-88)update(23-26)custom_components/sat/pid.py (11)
PID(34-270)proportional(86-91)integral(94-96)derivative(99-104)available(53-55)raw_derivative(107-109)output(117-122)update(158-179)kp(58-67)ki(70-75)kd(78-83)custom_components/sat/temperature_state.py (2)
TemperatureState(38-49)error(47-49)
🪛 Pylint (4.0.4)
custom_components/sat/binary_sensor.py
[refactor] 59-59: Too few public methods (1/2)
(R0903)
custom_components/sat/coordinator.py
[refactor] 71-71: Too many return statements (7/6)
(R0911)
[refactor] 103-103: Too many instance attributes (12/7)
(R0902)
[refactor] 103-103: Too many public methods (49/20)
(R0904)
[refactor] 460-460: Too few public methods (1/2)
(R0903)
custom_components/sat/serial/__init__.py
[error] 33-33: Undefined variable 'DEFAULT_STATUS'
(E0602)
custom_components/sat/entry_data.py
[refactor] 89-89: Too many public methods (21/20)
(R0904)
custom_components/sat/climate.py
[refactor] 71-71: Too many instance attributes (28/7)
(R0902)
[refactor] 245-245: Too many branches (16/12)
(R0912)
[refactor] 71-71: Too many public methods (29/20)
(R0904)
[refactor] 716-716: Too many branches (13/12)
(R0912)
custom_components/sat/area.py
[refactor] 37-37: Too many instance attributes (8/7)
(R0902)
[refactor] 279-279: Too few public methods (1/2)
(R0903)
custom_components/sat/pid.py
[refactor] 34-34: Too many instance attributes (13/7)
(R0902)
tests/test_pid.py
[refactor] 32-32: Too many arguments (6/5)
(R0913)
🪛 Ruff (0.14.10)
custom_components/sat/binary_sensor.py
15-15: from .const import * used; unable to detect undefined names
(F403)
25-25: DOMAIN may be undefined, or defined from star imports
(F405)
220-220: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
239-239: CycleClassification may be undefined, or defined from star imports
(F405)
240-240: CycleClassification may be undefined, or defined from star imports
(F405)
241-241: CycleClassification may be undefined, or defined from star imports
(F405)
tests/test_climate.py
74-74: Unused lambda argument: self
(ARG005)
75-75: Unused lambda argument: self
(ARG005)
76-76: Unused lambda argument: self
(ARG005)
85-85: Unused lambda argument: self
(ARG005)
86-86: Unused lambda argument: self
(ARG005)
87-87: Unused lambda argument: self
(ARG005)
121-121: Unused function argument: self
(ARG001)
150-150: Unused lambda argument: self
(ARG005)
162-162: Unused lambda argument: self
(ARG005)
177-177: Unused lambda argument: self
(ARG005)
custom_components/sat/heating_curve.py
6-6: from .const import * used; unable to detect undefined names
(F403)
12-12: HeatingSystem may be undefined, or defined from star imports
(F405)
76-76: HeatingSystem may be undefined, or defined from star imports
(F405)
custom_components/sat/util.py
8-8: from .const import * used; unable to detect undefined names
(F403)
53-53: DOMAIN may be undefined, or defined from star imports
(F405)
custom_components/sat/coordinator.py
15-15: from .const import * used; unable to detect undefined names
(F403)
100-100: Create your own exception
(TRY002)
100-100: Avoid specifying long messages outside the exception class
(TRY003)
106-106: DOMAIN may be undefined, or defined from star imports
(F405)
307-307: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/serial/__init__.py
10-10: from pyotgw.vars import * used; unable to detect undefined names
(F403)
33-33: DEFAULT_STATUS may be undefined, or defined from star imports
(F405)
custom_components/sat/entry_data.py
9-9: from .const import * used; unable to detect undefined names
(F403)
19-19: StrEnum may be undefined, or defined from star imports
(F405)
71-71: HeatingMode may be undefined, or defined from star imports
(F405)
96-96: CONF_DEVICE may be undefined, or defined from star imports
(F405)
100-100: CONF_ERROR_MONITORING may be undefined, or defined from star imports
(F405)
103-103: HeatingSystem may be undefined, or defined from star imports
(F405)
104-104: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
105-105: HeatingSystem may be undefined, or defined from star imports
(F405)
107-107: HeatingSystem may be undefined, or defined from star imports
(F405)
107-107: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
107-107: HeatingSystem may be undefined, or defined from star imports
(F405)
111-111: CONF_MANUFACTURER may be undefined, or defined from star imports
(F405)
115-115: CONF_MQTT_TOPIC may be undefined, or defined from star imports
(F405)
119-119: CONF_MODE may be undefined, or defined from star imports
(F405)
126-126: CONF_NAME may be undefined, or defined from star imports
(F405)
134-134: CONF_OVERSHOOT_PROTECTION may be undefined, or defined from star imports
(F405)
138-138: CONF_PUSH_SETPOINT_TO_THERMOSTAT may be undefined, or defined from star imports
(F405)
142-142: CONF_RADIATORS may be undefined, or defined from star imports
(F405)
146-146: CONF_ROOMS may be undefined, or defined from star imports
(F405)
150-150: CONF_THERMOSTAT may be undefined, or defined from star imports
(F405)
154-154: CONF_WINDOW_SENSORS may be undefined, or defined from star imports
(F405)
159-159: CONF_INSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
160-160: CONF_HUMIDITY_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
161-161: CONF_OUTSIDE_SENSOR_ENTITY_ID may be undefined, or defined from star imports
(F405)
164-164: CONF_WINDOW_MINIMUM_OPEN_TIME may be undefined, or defined from star imports
(F405)
167-167: CONF_SENSOR_MAX_VALUE_AGE may be undefined, or defined from star imports
(F405)
174-174: CONF_INTEGRAL may be undefined, or defined from star imports
(F405)
175-175: CONF_DERIVATIVE may be undefined, or defined from star imports
(F405)
176-176: CONF_PROPORTIONAL may be undefined, or defined from star imports
(F405)
178-178: CONF_AUTOMATIC_GAINS may be undefined, or defined from star imports
(F405)
179-179: CONF_AUTOMATIC_GAINS_VALUE may be undefined, or defined from star imports
(F405)
180-180: CONF_HEATING_CURVE_COEFFICIENT may be undefined, or defined from star imports
(F405)
186-186: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
187-187: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
189-189: CONF_DYNAMIC_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
190-190: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
191-191: CONF_MAXIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
192-192: CONF_MINIMUM_SETPOINT_ADJUSTMENT_FACTOR may be undefined, or defined from star imports
(F405)
197-197: CONF_HEATING_SYSTEM may be undefined, or defined from star imports
(F405)
197-197: HeatingSystem may be undefined, or defined from star imports
(F405)
198-198: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
202-202: CONF_MAXIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
207-207: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
207-207: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
210-210: CONF_MAXIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
211-211: CONF_MINIMUM_CONSUMPTION may be undefined, or defined from star imports
(F405)
213-213: CONF_CLIMATE_VALVE_OFFSET may be undefined, or defined from star imports
(F405)
214-214: CONF_TARGET_TEMPERATURE_STEP may be undefined, or defined from star imports
(F405)
219-219: HeatingMode may be undefined, or defined from star imports
(F405)
219-219: CONF_HEATING_MODE may be undefined, or defined from star imports
(F405)
222-222: CONF_AWAY_TEMPERATURE may be undefined, or defined from star imports
(F405)
222-222: CONF_AWAY_TEMPERATURE may be undefined, or defined from star imports
(F405)
223-223: CONF_HOME_TEMPERATURE may be undefined, or defined from star imports
(F405)
223-223: CONF_HOME_TEMPERATURE may be undefined, or defined from star imports
(F405)
224-224: CONF_SLEEP_TEMPERATURE may be undefined, or defined from star imports
(F405)
224-224: CONF_SLEEP_TEMPERATURE may be undefined, or defined from star imports
(F405)
225-225: CONF_COMFORT_TEMPERATURE may be undefined, or defined from star imports
(F405)
225-225: CONF_COMFORT_TEMPERATURE may be undefined, or defined from star imports
(F405)
226-226: CONF_ACTIVITY_TEMPERATURE may be undefined, or defined from star imports
(F405)
226-226: CONF_ACTIVITY_TEMPERATURE may be undefined, or defined from star imports
(F405)
231-231: CONF_THERMAL_COMFORT may be undefined, or defined from star imports
(F405)
232-232: CONF_SYNC_CLIMATES_WITH_MODE may be undefined, or defined from star imports
(F405)
233-233: CONF_SYNC_CLIMATES_WITH_PRESET may be undefined, or defined from star imports
(F405)
236-236: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
242-242: CONF_SIMULATION may be undefined, or defined from star imports
(F405)
243-243: CONF_SIMULATED_HEATING may be undefined, or defined from star imports
(F405)
243-243: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
243-243: CONF_SIMULATED_HEATING may be undefined, or defined from star imports
(F405)
244-244: CONF_SIMULATED_COOLING may be undefined, or defined from star imports
(F405)
244-244: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
244-244: CONF_SIMULATED_COOLING may be undefined, or defined from star imports
(F405)
245-245: CONF_SIMULATED_WARMING_UP may be undefined, or defined from star imports
(F405)
245-245: OPTIONS_DEFAULTS may be undefined, or defined from star imports
(F405)
245-245: CONF_SIMULATED_WARMING_UP may be undefined, or defined from star imports
(F405)
260-260: DOMAIN may be undefined, or defined from star imports
(F405)
custom_components/sat/climate.py
35-35: from .const import * used; unable to detect undefined names
(F403)
95-95: Consider [PRESET_NONE, *list(self._presets.keys())] instead of concatenation
Replace with [PRESET_NONE, *list(self._presets.keys())]
(RUF005)
174-174: EVENT_SAT_CYCLE_STARTED may be undefined, or defined from star imports
(F405)
182-182: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
192-192: Unused method argument: event
(ARG002)
237-237: DOMAIN may be undefined, or defined from star imports
(F405)
307-307: DOMAIN may be undefined, or defined from star imports
(F405)
307-307: SERVICE_RESET_INTEGRAL may be undefined, or defined from star imports
(F405)
457-457: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
462-462: HeatingMode may be undefined, or defined from star imports
(F405)
475-475: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
475-475: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
562-562: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
566-566: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
579-579: UNHEALTHY_CYCLES may be undefined, or defined from star imports
(F405)
585-585: PWM_ENABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
589-589: PWM_DISABLE_MARGIN_CELSIUS may be undefined, or defined from star imports
(F405)
600-600: MINIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
609-609: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
722-722: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
723-723: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
778-778: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
782-782: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
782-782: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
848-848: DEADBAND may be undefined, or defined from star imports
(F405)
848-848: DEADBAND may be undefined, or defined from star imports
(F405)
911-911: COLD_SETPOINT may be undefined, or defined from star imports
(F405)
custom_components/sat/area.py
309-309: Do not catch blind exception: Exception
(BLE001)
custom_components/sat/pid.py
10-10: from .const import * used; unable to detect undefined names
(F403)
37-37: HeatingSystem may be undefined, or defined from star imports
(F405)
66-66: HeatingSystem may be undefined, or defined from star imports
(F405)
179-179: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
187-187: DEADBAND may be undefined, or defined from star imports
(F405)
191-191: DEADBAND may be undefined, or defined from star imports
(F405)
224-224: DEADBAND may be undefined, or defined from star imports
(F405)
custom_components/sat/sensor.py
14-14: from .const import * used; unable to detect undefined names
(F403)
25-25: DOMAIN may be undefined, or defined from star imports
(F405)
113-113: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
351-351: EVENT_SAT_CYCLE_ENDED may be undefined, or defined from star imports
(F405)
360-360: CycleClassification may be undefined, or defined from star imports
(F405)
🔇 Additional comments (38)
custom_components/sat/helpers.py (1)
12-171: LGTM! Well-implemented utility functions.The new timestamp, state management, and statistical utilities are well-designed with proper UTC semantics, None-safety, and error handling. The integration with the HeatingSystem enum (line 67) aligns correctly with the broader refactoring.
tests/test_pid.py (1)
105-123: Test correctly validates automatic gains calculation.The test validates that automatic gains are computed from heating_curve_value when
automatic_gains=True. The expected values (kp=20.0, ki=round(20.0/8400, 6), kd=round(0.07840020.0, 6)) align with the automatic gain formulas for UNDERFLOOR heating systems.Note: The past review comment about
automatic_gain_valuebeing ignored may refer to a different code version or may have been addressed.custom_components/sat/switch/__init__.py (1)
23-27: LGTM! Consistent refactoring to SatConfig architecture.The changes properly migrate from individual config parameters to the unified
SatConfigmodel. The updated type annotations (Optional[int] at line 53) and property access patterns (self._config.device, self._config.simulation.enabled) are consistent with the broader architectural refactoring across the codebase.Also applies to: 53-57
custom_components/sat/heating_curve.py (3)
12-15: LGTM! Constructor now uses strongly-typed HeatingSystem enum.The refactor from string-based heating system to
HeatingSystemenum improves type safety and aligns with the centralized enum definition inconst.py.
76-76: LGTM! Enum comparison is correct.The comparison
self._heating_system == HeatingSystem.UNDERFLOORproperly uses the enum instead of a string constant.
78-88: LGTM! Return types accurately reflect nullable properties.The
Optional[float]return types correctly indicate these properties can returnNonewhen no value has been computed yet.custom_components/sat/mqtt/__init__.py (2)
34-46: LGTM! Lifecycle method signature updated correctly.The
async_added_to_hassnow acceptshass: HomeAssistantparameter and properly awaits the parent method with the same parameter.
104-107: LGTM! Simulation check uses new config-driven approach.The simulation gating correctly accesses
self._config.simulation.enabledinstead of the legacyself._simulationattribute.custom_components/sat/entry_data.py (1)
249-256: LGTM! SatEntryData structure is well-designed.The dataclass properly groups per-entry runtime state with the immutable config. The
asyncio.Eventfactory is correctly usingfield(default_factory=asyncio.Event)to avoid shared state between instances.custom_components/sat/serial/__init__.py (1)
174-208: LGTM! Simulation gating consistently applied across all control methods.All
async_set_control_*methods properly checkself._config.simulation.enabledbefore issuing hardware commands, while still calling the parent method to update internal state.custom_components/sat/binary_sensor.py (2)
23-36: LGTM! Robust climate readiness check with timeout.The pattern of waiting for
climate_readyevent with a timeout and early returns on failure prevents race conditions during entity setup.
214-242: LGTM! Cycle health sensor with proper event-driven updates.The sensor correctly listens for
EVENT_SAT_CYCLE_ENDEDto trigger state updates, and theis_onlogic properly handles theNonecase forlast_cycle. The classification check excludesGOOD,UNCERTAIN, andINSUFFICIENT_DATAas non-problem states.custom_components/sat/util.py (2)
19-31: LGTM! Clean factory functions with SatConfig-driven construction.The factory functions now use a centralized
SatConfigfor all configuration access, providing a cleaner API and consistent configuration handling.
44-58: LGTM! Entity lookup with proper null checks.The function correctly validates registry entries and checks for
Noneclimate before appending to the result list.custom_components/sat/pid.py (2)
181-217: LGTM! Integral update logic is well-implemented.The integral term correctly:
- Resets outside the deadband
- Caps integration intervals to prevent over-accumulation after gaps
- Clamps to heating curve bounds
- Handles edge cases for forward timestamps
219-255: LGTM! Derivative update with dual low-pass filtering.The derivative calculation properly applies two-stage exponential smoothing and caps the magnitude to prevent noise amplification. The deadband check and forward-time validation are appropriate.
custom_components/sat/coordinator.py (6)
32-39: LGTM! Clean dataclass for control loop sampling.The
ControlLoopSampledataclass is well-designed with appropriate fields for cycle tracking. Usingfrozen=Trueandslots=Trueis good practice for immutable data containers.
69-100: LGTM! Factory pattern refactored to use SatConfig.The factory now cleanly resolves coordinator types based on
config.modeenum values. The pattern is consistent and readable.
103-124: LGTM! Coordinator initialization with new architecture.The initialization properly sets up the Boiler, CycleHistory, CycleTracker, and Debouncer instances. The debouncer function references (
self.async_update_listenersandself.async_update_control) appear correct based on the method definitions below.
389-402: LGTM! Safe timestamp handling withevent_timestamphelper.The
async_control_heating_loopnow uses theevent_timestamp()helper function which handles theNonecase safely, addressing the previous review concern aboutAttributeErrorwhentimeisNone.
444-457: LGTM! Debounced notification flow is well-structured.The
async_notify_listenersmethod properly coordinates control updates via debouncer and conditionally triggers listener updates based on dirty state. Theasync_set_updated_datamethod correctly updates data and triggers notifications withforce=False.
432-442: Guard condition correctly ensures cycle samples are only created with both control values present.The climate entity consistently sets both
set_control_context()andset_control_intent()before calling the coordinator'sasync_control_heating_loop()method (lines 904-905 in climate.py), so the debouncer will always have both values available when triggered from that path. The guard condition protects against edge cases whereasync_notify_listeners()might be called from other sources (simulator, esphome implementations) without the control values being set. This defensive check is appropriate and the implementation is sound.custom_components/sat/climate.py (8)
71-111: LGTM! Clean initialization with new controller architecture.The
__init__method properly initializes all controllers (Areas, heating_curve, pwm, pid, minimum_setpoint) using the factory functions. The preset building and feature detection are well-structured.
128-145: LGTM! Helper method for safe entity state retrieval.The
_get_entity_state_floatmethod properly handles None entity_id, missing state, stale state, and unavailable/unknown states before returning a float value.
173-190: LGTM! Cycle event listeners properly registered.The
EVENT_SAT_CYCLE_STARTEDandEVENT_SAT_CYCLE_ENDEDlisteners are correctly wrapped withasync_on_removefor cleanup. The lambda handlers appropriately pass event data to the minimum_setpoint controller.
190-200: LGTM! Shutdown handler signature fixed.The
async_will_remove_from_hassmethod now correctly accepts an optionalEventparameter, addressing the previous review concern about the HOMEASSISTANT_STOP listener callback signature.
374-394: LGTM! Error property now returns proper TemperatureState.The
errorproperty correctly handles None cases for inside sensor, state, target temperature, and current temperature before constructing theTemperatureStateobject.
537-592: LGTM! PWM decision logic is well-structured.The
_pwm_decision,_pwm_static_decision, and_pwm_dynamic_decisionmethods implement clear decision logic with appropriate logging. Each path returns a specificPWMDecisionenum value.
877-913: LGTM! Heating loop control flow is well-structured.The
async_control_heating_loopmethod properly:
- Clears the unsubscribe handle
- Validates required inputs before proceeding
- Updates PWM state based on device state
- Passes control context to the coordinator
- Applies setpoint and modulation controls
829-838: Theis_state_stale()function already handlesNonestate values gracefully. Its implementation explicitly checks forNoneand returnsFalseearly, so no guard is needed at line 835. The code is safe as written.Likely an incorrect or invalid review comment.
custom_components/sat/sensor.py (4)
23-66: LGTM! Robust async setup with timeout handling.The
async_setup_entryfunction properly:
- Checks for entry existence in
hass.data- Waits for climate readiness with a 10-second timeout
- Handles timeout gracefully by returning early
- Creates appropriate sensors based on coordinator capabilities
93-172: LGTM! SatPidSensor with proper None handling.The
SatPidSensorclass correctly:
- Handles optional
area_idparameter- Guards against None PID in
available,native_value, andextra_state_attributes- Uses dispatcher signal for state updates
- Generates unique IDs that include area context when applicable
344-366: LGTM! SatCycleSensor with event-driven updates.The
SatCycleSensorproperly:
- Registers for
EVENT_SAT_CYCLE_ENDEDevents- Returns
INSUFFICIENT_DATAwhen no cycle exists- Uses the cycle's classification for the native value
369-384: LGTM! SatBoilerSensor availability logic updated.The
availableproperty now correctly treatsINSUFFICIENT_DATAstatus as unavailable, which aligns with the user-reported issues about stale boiler status.custom_components/sat/area.py (4)
37-63: LGTM! Area initialization with room weight clamping.The
Areaclass properly initializes heating curve and PID controllers, and clamps the room weight to a safe range [0.1, 3.0] with appropriate error handling for invalid values.
289-326: LGTM! PID output aggregation with percentile-based selection.The
_PIDs.outputproperty implements a sophisticated aggregation strategy:
- Filters areas that require heat and have available PIDs
- Uses percentile-based selection for baseline
- Applies headroom limiting
- Handles empty outputs gracefully
328-359: LGTM! Overshoot cap calculation with proper error handling.The
overshoot_capproperty correctly:
- Uses
error.errorinstead of negating theErrorobject directly (addressing past review)- Applies cooling slope calculation
- Returns the strictest (minimum) cap
- Ensures the cap never falls below
COLD_SETPOINT
361-365: LGTM! PID reset with optional entity filtering.The
resetmethod correctly iterates through areas and resets PIDs either for all areas or for a specific entity whenentity_idis provided.
| def temperature_state(self) -> Optional[State]: | ||
| """Return the source state used to calculate the current temperature.""" | ||
| if (self._hass is None) or (climate_state := self.climate_state) is None: | ||
| return None | ||
|
|
||
| return float_value(state.attributes.get("temperature")) | ||
| # Check if there is an overridden sensor temperature | ||
| if (sensor_temperature_id := climate_state.attributes.get(ATTR_SENSOR_TEMPERATURE_ID)) is not None: | ||
| sensor_state = self._hass.states.get(sensor_temperature_id) | ||
| if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]: | ||
| if is_state_stale(sensor_state, self._sensor_max_value_age): | ||
| _LOGGER.debug("Area sensor %s stale for %s (age=%.1fs > %.1fs)", sensor_temperature_id, self._entity_id, state_age_seconds(sensor_state), self._sensor_max_value_age) | ||
| return None | ||
|
|
||
| return sensor_state | ||
|
|
||
| if is_state_stale(climate_state, self._sensor_max_value_age): | ||
| _LOGGER.debug("Area climate %s stale for %s (age=%.1fs > %.1fs)", self._entity_id, self._entity_id, state_age_seconds(climate_state), self._sensor_max_value_age) | ||
| return None | ||
|
|
||
| return climate_state |
There was a problem hiding this comment.
Remove HVACMode.OFF from sensor state validation.
Line 91 checks HVACMode.OFF alongside STATE_UNKNOWN and STATE_UNAVAILABLE for sensor temperature state validation. However, sensor_temperature_id references sensor entities (e.g., sensor.room1_temp), which don't use HVAC modes. HVACMode.OFF is a climate-specific constant.
🔎 Proposed fix
- if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]:
+ if sensor_state and sensor_state.state not in [STATE_UNKNOWN, STATE_UNAVAILABLE]:If filtering an "off" state from sensors is necessary, use the string "off" directly rather than the climate-specific enum.
This was flagged in a previous review but appears to remain unaddressed.
🤖 Prompt for AI Agents
In @custom_components/sat/area.py around lines 83 - 102, The sensor validation
in temperature_state incorrectly checks HVACMode.OFF (a climate enum) against
sensor states; locate the temperature_state method and the condition that
compares sensor_state.state to [STATE_UNKNOWN, STATE_UNAVAILABLE, HVACMode.OFF]
(uses ATTR_SENSOR_TEMPERATURE_ID and is_state_stale) and remove HVACMode.OFF or
replace it with the literal string "off" if you intend to filter sensors
reporting "off"; update the conditional to only use sensor-appropriate constants
(STATE_UNKNOWN, STATE_UNAVAILABLE, and optionally "off") and ensure logging and
stale checks remain unchanged.
| @property | ||
| def is_on(self): | ||
| def is_on(self) -> bool: | ||
| """Return the state of the sensor.""" | ||
| return self.state_delayed(int(self._climate.relative_modulation_value) != int(self._coordinator.maximum_relative_modulation_value)) | ||
| climate_modulation = int(self._climate.relative_modulation_value) | ||
| maximum_modulation = int(self._coordinator.maximum_relative_modulation_value) | ||
|
|
||
| return self.state_delayed(climate_modulation != maximum_modulation) |
There was a problem hiding this comment.
Potential TypeError: int(None) will fail.
If self._climate.relative_modulation_value or self._coordinator.maximum_relative_modulation_value is None, the int() conversion will raise TypeError. The available property guards against this, but if called directly, is_on could fail.
🔎 Proposed defensive fix
@property
def is_on(self) -> bool:
"""Return the state of the sensor."""
+ if not self.available:
+ return False
climate_modulation = int(self._climate.relative_modulation_value)
maximum_modulation = int(self._coordinator.maximum_relative_modulation_value)
return self.state_delayed(climate_modulation != maximum_modulation)📝 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.
| @property | |
| def is_on(self): | |
| def is_on(self) -> bool: | |
| """Return the state of the sensor.""" | |
| return self.state_delayed(int(self._climate.relative_modulation_value) != int(self._coordinator.maximum_relative_modulation_value)) | |
| climate_modulation = int(self._climate.relative_modulation_value) | |
| maximum_modulation = int(self._coordinator.maximum_relative_modulation_value) | |
| return self.state_delayed(climate_modulation != maximum_modulation) | |
| @property | |
| def is_on(self) -> bool: | |
| """Return the state of the sensor.""" | |
| if not self.available: | |
| return False | |
| climate_modulation = int(self._climate.relative_modulation_value) | |
| maximum_modulation = int(self._coordinator.maximum_relative_modulation_value) | |
| return self.state_delayed(climate_modulation != maximum_modulation) |
🤖 Prompt for AI Agents
In @custom_components/sat/binary_sensor.py around lines 139 - 145, The is_on
property can raise TypeError when calling int() on None; update the is_on method
(in binary_sensor.py) to first check self._climate.relative_modulation_value and
self._coordinator.maximum_relative_modulation_value for None and only call int()
when both are not None; if either is None, return a safe boolean via
self.state_delayed(False) (or another explicit fallback) so the property never
attempts int(None) and always returns a bool.
| def schedule_control_heating_loop(self, _time: Optional[datetime] = None, force: bool = False) -> None: | ||
| """Schedule a debounced execution of the heating control loop.""" | ||
| # Force immediate execution | ||
| if force: | ||
| # Cancel previous scheduled run, if any | ||
| if self._control_heating_loop_unsub is not None: | ||
| self._control_heating_loop_unsub() | ||
| self._control_heating_loop_unsub = None | ||
|
|
||
| self.hass.async_create_task(self.async_control_heating_loop()) | ||
| return | ||
| self.hass.async_create_task(self.async_control_heating_loop()) | ||
| return | ||
|
|
||
| # If a run is already scheduled, do nothing. | ||
| if self._control_heating_loop_unsub is not None: | ||
| return | ||
|
|
||
| self._control_heating_loop_unsub = async_call_later(self.hass, 10, HassJob(self.async_control_heating_loop)) | ||
| self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop)) |
There was a problem hiding this comment.
Critical: HassJob cannot accept async coroutines—will raise ValueError at runtime.
Line 875 passes self.async_control_heating_loop (an async method) directly to HassJob:
self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop))Home Assistant's HassJob raises ValueError: Coroutine not allowed to be passed to HassJob when given an async callable. This prevents the heating control loop from scheduling properly after the initial 5-second delay.
🔎 Proposed fix
- self._control_heating_loop_unsub = async_call_later(self.hass, 5, HassJob(self.async_control_heating_loop))
+ self._control_heating_loop_unsub = async_call_later(
+ self.hass,
+ 5,
+ lambda _: self.hass.async_create_task(self.async_control_heating_loop())
+ )This issue was flagged in previous reviews but appears to remain unresolved.
🤖 Prompt for AI Agents
In @custom_components/sat/climate.py around lines 860 - 875, The code is passing
an async coroutine method to HassJob which raises ValueError; update the
async_call_later call in schedule_control_heating_loop so HassJob receives a
synchronous callable that schedules the coroutine instead. Replace
HassJob(self.async_control_heating_loop) with something that invokes
hass.async_create_task (e.g. HassJob(lambda _:
self.hass.async_create_task(self.async_control_heating_loop()))) so
async_control_heating_loop is scheduled correctly; keep the rest of the logic
and still cancel/clear _control_heating_loop_unsub as before.
| @staticmethod | ||
| def _build_presets(config_options: Mapping[str, float]) -> dict[str, float]: | ||
| """Build preset temperature mapping from config options.""" | ||
| conf_presets = {p: f"{p}_temperature" for p in (PRESET_ACTIVITY, PRESET_AWAY, PRESET_HOME, PRESET_SLEEP, PRESET_COMFORT)} | ||
| return {key: config_options[value] for key, value in conf_presets.items() if key in conf_presets} |
There was a problem hiding this comment.
_build_presets always returns an empty dict due to logic error.
The condition if key in conf_presets is always True because key comes from iterating conf_presets.items(). The intended check was likely to verify the presence of value in config_options:
return {key: config_options[value] for key, value in conf_presets.items() if key in conf_presets}This results in a KeyError when config_options doesn't contain the expected keys.
🔎 Proposed fix
@staticmethod
def _build_presets(config_options: Mapping[str, float]) -> dict[str, float]:
"""Build preset temperature mapping from config options."""
conf_presets = {p: f"{p}_temperature" for p in (PRESET_ACTIVITY, PRESET_AWAY, PRESET_HOME, PRESET_SLEEP, PRESET_COMFORT)}
- return {key: config_options[value] for key, value in conf_presets.items() if key in conf_presets}
+ return {key: config_options[value] for key, value in conf_presets.items() if value in config_options}🤖 Prompt for AI Agents
In @custom_components/sat/climate.py around lines 1047 - 1051, The
_build_presets function builds conf_presets then uses a dict comprehension that
incorrectly checks "if key in conf_presets" (always true) causing KeyError;
change the comprehension to only include entries whose mapped config key exists
by replacing the condition with "if value in config_options" (or use
config_options.get(value) with a fallback) so it returns a dict of preset_name:
config_options[value] for available keys (conf_presets, _build_presets,
config_options,
PRESET_ACTIVITY/PRESET_AWAY/PRESET_HOME/PRESET_SLEEP/PRESET_COMFORT).
| @property | ||
| def device_capabilities(self) -> BoilerCapabilities: | ||
| """Describe setpoint capabilities for this device.""" | ||
| return BoilerCapabilities( | ||
| minimum_setpoint=self.minimum_setpoint, | ||
| maximum_setpoint=self.maximum_setpoint_value, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "def minimum_setpoint|def maximum_setpoint_value" custom_components/sat/coordinator.py -A 5Repository: Alexwijn/SAT
Length of output: 777
🏁 Script executed:
rg -n "def maximum_setpoint\b" custom_components/sat/coordinator.py -A 5Repository: Alexwijn/SAT
Length of output: 404
🏁 Script executed:
rg -n "class BoilerCapabilities" custom_components/sat/ -A 10Repository: Alexwijn/SAT
Length of output: 685
🏁 Script executed:
rg -n "def maximum_setpoint\b" custom_components/sat/coordinator.py -A 15Repository: Alexwijn/SAT
Length of output: 886
Type annotation mismatch in maximum_setpoint_value property.
While device_capabilities is safe at runtime, maximum_setpoint_value is declared as returning Optional[float] (line 254) but always returns a float via self.maximum_setpoint. Update the return type hint to float instead of Optional[float] to match the actual behavior and prevent type checking issues.
🤖 Prompt for AI Agents
In @custom_components/sat/coordinator.py around lines 143 - 149, The property
maximum_setpoint_value is currently annotated as returning Optional[float] but
always forwards self.maximum_setpoint (a float), causing a type mismatch; update
the return type annotation of maximum_setpoint_value from Optional[float] to
float to match actual behavior and keep type checkers happy, and ensure callers
like device_capabilities (which builds BoilerCapabilities with
maximum_setpoint=maximum_setpoint_value) reflect the non-optional float
contract.
| @property | ||
| def kp(self) -> Optional[float]: | ||
| """Return the value of kp based on the current configuration.""" | ||
| if not self._config.automatic_gains: | ||
| return float(self._config.proportional) | ||
|
|
||
| self._last_boiler_temperature = boiler_temperature | ||
| self._last_heating_curve_value = heating_curve_value | ||
| if self._heating_curve.value is None: | ||
| return 0.0 | ||
|
|
||
| def update_reset(self, error: Error, heating_curve_value: Optional[float]) -> None: | ||
| """ | ||
| Update the PID controller with resetting. | ||
| automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3 | ||
| return round((self._config.heating_curve_coefficient * self._heating_curve.value) / automatic_gain_value, 6) |
There was a problem hiding this comment.
Constructor parameter automatic_gains_value is still unused.
The PidConfig.automatic_gains_value is stored in config but line 66 hardcodes the gain value based on heating system instead of using self._config.automatic_gains_value. This was flagged in a previous review but remains unaddressed.
🔎 Proposed fix to use the configured value
if self._heating_curve.value is None:
return 0.0
- automatic_gain_value = 4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3
+ automatic_gain_value = self._config.automatic_gains_value if self._config.automatic_gains_value > 0 else (4 if self._heating_system == HeatingSystem.UNDERFLOOR else 3)
return round((self._config.heating_curve_coefficient * self._heating_curve.value) / automatic_gain_value, 6)🧰 Tools
🪛 Ruff (0.14.10)
66-66: HeatingSystem may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In @custom_components/sat/pid.py around lines 57 - 67, The kp property currently
ignores PidConfig.automatic_gains_value and hardcodes automatic_gain_value based
on HeatingSystem; update the logic in the kp getter to use
self._config.automatic_gains_value when automatic gains are enabled (falling
back to the heating-system-based default only if automatic_gains_value is None
or invalid), i.e., replace the hardcoded 4/3 selection with a value derived from
self._config.automatic_gains_value and then compute and return
round((self._config.heating_curve_coefficient * self._heating_curve.value) /
automatic_gain_value, 6) as before.
| self._last_error = float_value(data.get(STORAGE_KEY_LAST_ERROR)) | ||
| self._integral = float(data.get(STORAGE_KEY_INTEGRAL, self._integral)) | ||
| self._last_temperature = float_value(data.get(STORAGE_KEY_LAST_TEMPERATURE)) | ||
| self._raw_derivative = float(data.get(STORAGE_KEY_RAW_DERIVATIVE, self._raw_derivative)) |
There was a problem hiding this comment.
Inconsistent error handling during state restoration.
Lines 140 and 142 use bare float() which will raise exceptions on corrupted data, while lines 139 and 141 correctly use float_value() for safe conversion.
🔎 Proposed fix
self._last_error = float_value(data.get(STORAGE_KEY_LAST_ERROR))
- self._integral = float(data.get(STORAGE_KEY_INTEGRAL, self._integral))
+ self._integral = float_value(data.get(STORAGE_KEY_INTEGRAL)) or self._integral
self._last_temperature = float_value(data.get(STORAGE_KEY_LAST_TEMPERATURE))
- self._raw_derivative = float(data.get(STORAGE_KEY_RAW_DERIVATIVE, self._raw_derivative))
+ self._raw_derivative = float_value(data.get(STORAGE_KEY_RAW_DERIVATIVE)) or self._raw_derivative🤖 Prompt for AI Agents
In @custom_components/sat/pid.py around lines 139 - 142, Restoration uses
inconsistent conversion: replace the bare float() calls for self._integral and
self._raw_derivative with the safe conversion helper float_value (same used for
self._last_error and self._last_temperature) so corrupted or missing stored
values don't raise exceptions during state restoration; update the assignments
that reference STORAGE_KEY_INTEGRAL and STORAGE_KEY_RAW_DERIVATIVE to call
float_value(...) and supply the existing defaults (self._integral,
self._raw_derivative) as before.
| def set_heating_curve_value(self, heating_curve_value: float) -> None: | ||
| """Set the heating curve value.""" | ||
| self._heating_curve_value = heating_curve_value |
There was a problem hiding this comment.
Dead code: set_heating_curve_value sets unused attribute.
This method sets self._heating_curve_value, but the class uses self._heating_curve.value everywhere else. This appears to be dead code or an incomplete refactor.
🔎 Proposed fix - remove dead code
- def set_heating_curve_value(self, heating_curve_value: float) -> None:
- """Set the heating curve value."""
- self._heating_curve_value = heating_curve_valueOr if this should update the heating curve, use the proper interface:
def set_heating_curve_value(self, heating_curve_value: float) -> None:
"""Set the heating curve value."""
- self._heating_curve_value = heating_curve_value
+ self._heating_curve._last_heating_curve_value = heating_curve_value📝 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.
| def set_heating_curve_value(self, heating_curve_value: float) -> None: | |
| """Set the heating curve value.""" | |
| self._heating_curve_value = heating_curve_value |
🤖 Prompt for AI Agents
In @custom_components/sat/pid.py around lines 154 - 156, The method
set_heating_curve_value is dead because the class uses self._heating_curve.value
elsewhere; remove the method or change it to update the real object (e.g.,
replace the body to set self._heating_curve.value = heating_curve_value or call
the actual heater curve setter on the object) and ensure self._heating_curve
exists (raise or initialize if needed) instead of assigning to the unused
self._heating_curve_value attribute.
| if self._store is not None: | ||
| self._hass.create_task(self._async_save_state()) | ||
|
|
||
| @property | ||
| def last_updated(self) -> float: | ||
| """Return the timestamp of the last update to the PID controller.""" | ||
| return self._last_updated | ||
| self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id) |
There was a problem hiding this comment.
Critical: self._hass.create_task should be async_create_task, and call_soon_threadsafe misuses async_dispatcher_send.
Two issues here:
-
Line 177: Home Assistant's
HomeAssistantobject usesasync_create_task, notcreate_task. This will raiseAttributeError. -
Line 179:
async_dispatcher_sendis a coroutine function. Passing it tocall_soon_threadsafewill not schedule the coroutine properly - it will just call the function and discard the returned coroutine object.
This may be related to the TypeError reported in PR comments: "a coroutine was expected, got ".
🔎 Proposed fix
if self._store is not None:
- self._hass.create_task(self._async_save_state())
+ self._hass.async_create_task(self._async_save_state())
- self._hass.loop.call_soon_threadsafe(async_dispatcher_send, self._hass, SIGNAL_PID_UPDATED, self._entity_id)
+ async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id)Since update() is called from async context (the coordinator), direct dispatch is appropriate.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
179-179: SIGNAL_PID_UPDATED may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In @custom_components/sat/pid.py around lines 176 - 179, The update() handler
incorrectly uses HomeAssistant.create_task and call_soon_threadsafe with
async_dispatcher_send; change self._hass.create_task(self._async_save_state())
to self._hass.async_create_task(self._async_save_state()) and remove
call_soon_threadsafe: call async_dispatcher_send directly from the async context
as async_dispatcher_send(self._hass, SIGNAL_PID_UPDATED, self._entity_id) (i.e.,
dispatch directly in update()), ensuring the coroutine is scheduled/executed
properly.
| def __init__(self, hass: HomeAssistant, config: SatConfig) -> None: | ||
| """Initialize.""" | ||
| super().__init__(hass, config_data, options) | ||
| super().__init__(hass, config) | ||
| self.async_set_updated_data(DEFAULT_STATUS) | ||
|
|
||
| self._port: str = port | ||
| self._port: str = str(self._config.device) | ||
| self._api: OpenThermGateway = OpenThermGateway() |
There was a problem hiding this comment.
Same issue: str(None) produces "None" string for port.
Similar to the MQTT coordinator, if self._config.device is None, this produces a port value of "None" which will cause pyotgw connection to fail with a confusing error.
🔎 Proposed fix
def __init__(self, hass: HomeAssistant, config: SatConfig) -> None:
"""Initialize."""
super().__init__(hass, config)
self.async_set_updated_data(DEFAULT_STATUS)
- self._port: str = str(self._config.device)
+ if self._config.device is None:
+ raise ConfigEntryNotReady("Serial device configuration is missing")
+ self._port: str = self._config.device
self._api: OpenThermGateway = OpenThermGateway()🧰 Tools
🪛 Pylint (4.0.4)
[error] 33-33: Undefined variable 'DEFAULT_STATUS'
(E0602)
🪛 Ruff (0.14.10)
33-33: DEFAULT_STATUS may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In @custom_components/sat/serial/__init__.py around lines 30 - 36, The
constructor __init__ sets self._port = str(self._config.device) which turns None
into the string "None" and breaks pyotgw; fix by checking self._config.device
before coercion (e.g., if self._config.device is not None assign its str() to
self._port, otherwise set self._port to None or an empty string) so the
OpenThermGateway connection gets a valid missing-port value; update the
assignment near __init__ and ensure downstream code using self._port can handle
the chosen None/empty value.
2 participants
Summary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.