custom_components/sat/cycles.py (1)

228-228: Update type hint to explicit Optional.

The timestamp parameter should use explicit Optional or T | None syntax 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 robustness

The updated tests align with the new control flow (async_control_pid(reset=True) followed by async_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). Using template.DOMAIN / sensor.DOMAIN in 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.approx to 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 state

Switching the three properties to Optional[float] matches how the internal fields are initialized and used (reset to None until 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) as Optional[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 .const and drop unused local constant

Ruff’s F405 warning about HEATING_SYSTEM_UNDERFLOOR stems from the wildcard import; it also makes it harder to see which constants this module actually needs. Additionally, MAX_BOILER_TEMPERATURE_AGE = 300 is 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_AGE definition.

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 import

The new SatCycleSensor and the adjusted SatBoilerSensor.available logic look consistent with the cycle‑centric model:

• SatCycleSensor.native_value returns CycleClassification.INSUFFICIENT_DATA.name when there is no last_cycle, otherwise the classification name, which matches the new enum.
• SatBoilerSensor.available treating BoilerStatus.INSUFFICIENT_DATA as “unavailable” is reasonable as long as device_status is always a BoilerStatus member (never None).

Two follow‑ups worth considering:

1. Coordinator defaults: Double‑check that self._coordinator.device_status is initialized to BoilerStatus.INSUFFICIENT_DATA (or another BoilerStatus member) and never None, so native_value’s device_status.name access is always safe.
2. Imports: from .const import * is triggering Ruff F403/F405 and hides which names come from const. 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 simplified

The value property’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_minimum while preserving behavior.

---

203-214: Unnecessary int() around round() when computing the regime bucket

round(base_setpoint / width) already returns an int in Python 3 when called without ndigits, so the outer int(...) is redundant:

-        bucket = int(round(base_setpoint / width))
+        bucket = round(base_setpoint / width)

This addresses the Ruff RUF046 hint without changing semantics.

---

20-64: Config / helper fields appear unused; consider pruning or wiring through

MinimumSetpointConfig.target_min_on_time_seconds and minimum_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_base with minimum_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 explicit is None check for heating curve availability

In update you 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 for last_duty_cycle_percentage in PWMState

PWM.state currently 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_percentage is 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 .const obscures which symbols are required

from .const import * together with use of CONF_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 unused climate argument for async_control_heating_loop

async_control_heating_loop still takes a climate: Optional[SatClimate] = None argument 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_direction to reduce complexity

Both _derive_status and _modulation_direction have 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_reliable vs 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 mode

In extra_state_attributes you 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_setpoint

So when _dynamic_minimum_setpoint is False:

• PWM/control logic uses the coordinator’s static minimum_setpoint.
• The attribute minimum_setpoint still reports the dynamic controller’s learned value (which you continue to update inside async_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 flag

Inside 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 the minimum_setpoint attribute.

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.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

custom_components/sat/relative_modulation.py (1)

31-39: Relative modulation state and enabled are still inverted w.r.t. PWM.

Current logic:

if self._pulse_width_modulation_enabled:
    return RelativeModulationState.OFF
return RelativeModulationState.PWM_OFF

and:

return self.state != RelativeModulationState.OFF

means:

• PWM enabled → state = OFF → enabled == False
• PWM disabled → state = PWM_OFF → enabled == True

Downstream, SatClimate.relative_modulation_value uses enabled to decide whether to use MINIMUM_RELATIVE_MODULATION or _maximum_relative_modulation, so this inversion causes the system to clamp to the minimum modulation when PWM is actually active.

You likely want enabled to be True when 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.OFF

This aligns enabled with “relative modulation allowed / active” and matches the prior review feedback.

custom_components/sat/pwm.py (1)

17-24: PWM config/state refactor is solid; but maximum_cycles can still produce invalid (negative) OFF times.

The new PWMConfig/PWMState types and state exposure look good and make PWM much easier to introspect.

The earlier concern about maximum_cycles driving 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 * 2

• In 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_threshold

For sufficiently large maximum_cycles (e.g. > ~40), _on_time_maximum_threshold becomes smaller than _on_time_lower_threshold, making off_time negative. That then feeds into transitions using self._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_cycles at 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

custom_components/sat/services.py (1)

6-6: Import change is fine; consider tightening the service schema for entity_id.

The switch to only import SERVICE_RESET_INTEGRAL is consistent with dropping the PWM service. However, the service schema still uses list[str] as the validator:

schema=vol.Schema({vol.Required("entity_id"): list[str]})

In Voluptuous, you typically want something like [str] or Home Assistant's cv.entity_ids helper, not list[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 and UNHEALTHY_CYCLES look coherent; consider making BoilerStatus a str enum for consistency.

The new cycle‑related enums and UNHEALTHY_CYCLES tuple are consistent and easy to consume across the codebase.

One minor consistency nit: BoilerStatus is declared as a plain Enum, whereas CycleKind, CycleClassification, PWMStatus, and RelativeModulationState all inherit from str, Enum. If BoilerStatus values are ever exposed to Home Assistant as state/attributes or used in logging without .value, having it inherit from str, Enum would 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 and minimum_setpoint floor look good; climate arg in async_control_heating_loop is unused.

The move to a composed Boiler + CycleHistory/CycleTracker and the new state/cycles/last_cycle properties looks coherent, and tightening minimum_setpoint with:

return max(float(self._config_data.get(CONF_MINIMUM_SETPOINT)), MINIMUM_SETPOINT)

is a nice safety improvement.

One small cleanup: async_control_heating_loop now takes a climate: Optional[SatClimate] = None argument, and callers pass climate=self, but the parameter is not used anywhere in the method. This is causing the static analysis ARG002 warning and adds noise to the public API.

Unless you expect to use the climate parameter 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:

1. Line 231: PEP 484 prohibits implicit Optional. The parameter should be explicitly typed as Optional[float] = None.
2. Line 232: The timestamp or monotonic() pattern incorrectly treats 0.0 as falsy, which would use monotonic() even when a valid timestamp of 0.0 is provided. Use an explicit None check 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, and 0.1 should be defined as module-level constants (similar to OVERSHOOT_MARGIN_CELSIUS and 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 mode

Then 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.0 on 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/hour

Then 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
         )

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

custom_components/sat/minimum_setpoint.py (1)

66-70: Guard against re‑processing the same completed cycle in update / tuning

DynamicMinimumSetpoint.update increments regime_state.completed_cycles and calls _maybe_tune_minimum on every invocation where last_cycle is non‑None (Lines 134–136), but RegimeState has no notion of which cycle was last processed. If the caller passes the last completed Cycle repeatedly between new completions (a common pattern for “last completed cycle” APIs), this will:

• Inflate completed_cycles far beyond the true number of physical cycles.
• Apply the same classification adjustment multiple times for a single real cycle, quickly driving minimum_setpoint up 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_cycles aligned 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

custom_components/sat/minimum_setpoint.py (2)

88-104: Minor simplification of value clamping logic

The 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_regime for “low‑load” semantics

Current 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_15m here (see previous comment).
• Optionally rename or document low_load_minimum_cycles_per_hour to 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.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

custom_components/sat/climate.py (1)

211-252: Fix EVENT_HOMEASSISTANT_STOP listener to avoid unsafe hass.async_create_task usage.

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_task from 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_task usage 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).

custom_components/sat/cycles.py (2)

183-205: _current_time_hint choice 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_cycle type hints with actual Optional usage.

The implementation correctly handles missing data (average_setpoint is None, max_flow_temperature is None) and returns INSUFFICIENT_DATA / UNCERTAIN appropriately, but the signature still types these as float:

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 as Optional[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_enabled plus _is_pwm_forced, _should_enable_static_pwm, and _should_enable_dynamic_pwm helpers make the decision-making around PWM explicit and easier to reason about:

• Require a valid _calculated_setpoint before 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.value and recent last_cycle.classification via UNHEALTHY_CYCLES.

Behavior-wise this is sound. As a small polish, a brief comment on the intended hysteresis in _should_enable_static_pwm and on the “no history yet: keep current PWM state” strategy in _should_enable_dynamic_pwm would help future readers understand why PWM doesn’t flap at startup.

---

795-824: _async_control_setpoint matches PWM state model; first branch is currently unreachable.

The refactored _async_control_setpoint aligns 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_value when PWMStatus.ON, and MINIMUM_SETPOINT when PWM is effectively “off”.
• Always delegates the final setpoint application to the coordinator.

Given async_control_heating_loop already early-returns if hvac_mode != HVACMode.HEAT, the top branch that sets _setpoint = MINIMUM_SETPOINT for non-HEAT modes is currently unreachable. Not harmful, but you could simplify by either removing that branch or documenting that _async_control_setpoint is intended to be reusable from other call sites in the future.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

custom_components/sat/minimum_setpoint.py (2)

26-36: [Duplicate] Clarify inverted comments and use the defined low_load_maximum_duty_ratio_15m threshold.

As flagged in previous reviews, the comments for increase_step and decrease_step remain 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_15m is defined but never used. Add a check in _is_tunable_regime (lines 334–348) to gate tuning when statistics.duty_ratio_last_15m exceeds this threshold.

Based on past review comments.

---

112-140: [Duplicate] Prevent repeated tuning on the same completed cycle.

As flagged in previous reviews, the update method increments regime_state.completed_cycles and calls _maybe_tune_minimum every time last_cycle is non-None (lines 134–137), but there is no guard to ensure each physical cycle is processed only once.

Since last_cycle in CycleHistory is the last completed cycle and remains constant until a new one finishes, your heating loop will call update(...) many times for the same Cycle, re-applying the same classification adjustment on each call.

Consequences:

• For a single bad cycle (e.g., TOO_SHORT_UNDERHEAT), minimum_setpoint will 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.0 could move the learned minimum by many degrees based on a single cycle.

Fix: Track the last processed cycle identity (e.g., last_cycle.end or id(last_cycle)) in RegimeState or DynamicMinimumSetpoint, and skip _maybe_tune_minimum when 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.end

Also update storage load/save logic (lines 141–203) to persist/restore last_processed_cycle_end.

custom_components/sat/climate.py (1)

893-909: [Duplicate] Fix HassJob with coroutine function: wrap in callback that schedules the coroutine.

As flagged in previous reviews, line 909 passes self.async_control_pid (a coroutine function) to HassJob. When async_call_later calls it, it will return a coroutine object, which Home Assistant forbids passing to HassJob (raises ValueError).

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()))
+        )

custom_components/sat/minimum_setpoint.py (1)

350-364: Remove unused _relax_minimum_when_uncertain method.

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.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

custom_components/sat/pwm.py (1)

48-57: Negative off_time issue with high maximum_cycles already flagged.

The threshold calculation issue where high maximum_cycles can produce negative off_time has been identified in a previous review. The suggested fix to clamp _on_time_maximum_threshold and use max(0, ...) for off_time should be applied.

custom_components/sat/minimum_setpoint.py (2)

28-35: Config comments are inverted relative to actual behavior.

The comments on increase_step and decrease_step are 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_15m is defined but never used in tuning logic.

---

140-156: Tuning occurs on every on_cycle_end call without deduplication.

The on_cycle_end method increments completed_cycles and calls _maybe_tune_minimum every time it's invoked. If the same last_cycle object 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)

custom_components/sat/const.py (1)

197-217: Consider inheriting from str for consistency with other enums.

BoilerStatus inherits only from Enum, while CycleKind, CycleClassification, PWMStatus, and RelativeModulationState all 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 the UNHEALTHY_CYCLES constant for maintainability.

The health check logic manually excludes GOOD, UNCERTAIN, and INSUFFICIENT_DATA, which is the inverse of the UNHEALTHY_CYCLES tuple defined in const.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_CYCLES

Then update the is_on property:

     @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_CYCLES

custom_components/sat/pwm.py (1)

139-142: Redundant assignment in disable().

reset() already sets self._enabled = False at line 68, so the assignment at line 142 is redundant.

     def disable(self) -> None:
         """Disable the PWM control."""
         self.reset()
-        self._enabled = False

custom_components/sat/minimum_setpoint.py (1)

116-116: Unused boiler_state parameter.

The boiler_state parameter is accepted but never used in on_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: Unused climate parameter in async_control_heating_loop.

The climate parameter 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 implicit float = None per PEP 484:

timestamp: Optional[float] = None

---

119-120: Listener callbacks invoke properties that may trigger side effects.

The listeners call self.state and self.last_cycle which 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 deque with maxlen=50 would 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_start and on_cycle_end methods 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 explicit Optional type hint per PEP 484.

The timestamp parameter uses implicit Optional (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_state method 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
    }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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."""

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro