diff --git a/src/interfaces/pv_interface.py b/src/interfaces/pv_interface.py index 744a2dc..a336361 100644 --- a/src/interfaces/pv_interface.py +++ b/src/interfaces/pv_interface.py @@ -86,7 +86,9 @@ def __init__( # Cache mechanism for fallback on API failures (similar to PriceInterface) # When Akkudoktor is unavailable, reuse last successful forecast self.last_successful_pv_forecast = [] + self.last_successful_temp_forecast = [] self.consecutive_failures = 0 + self.consecutive_temp_failures = 0 self.max_failures = 24 # Max consecutive failures before using defaults self._update_thread = None @@ -199,7 +201,9 @@ def reload_config( } # Reset cache when configuration changes (source switch, etc.) self.last_successful_pv_forecast = [] + self.last_successful_temp_forecast = [] self.consecutive_failures = 0 + self.consecutive_temp_failures = 0 try: self.__configure_update_interval() @@ -666,7 +670,10 @@ def __update_pv_state_loop(self): temp_result = self.__get_pv_forecast_akkudoktor_api( tgt_value="temperature", pv_config_entry=temp_config ) - if not temp_result: # If empty array or None due to API error + # Reject empty/None results and physically implausible values + # (e.g. PV Watts leaking into the temperature array) as a + # fail-safe on top of the target-aware cache/counter below. + if not temp_result or any(v > 60 or v < -60 for v in temp_result): logger.warning( "[PV-IF] Temperature forecast API failed - using default" + " temperature forecast (15°C)" @@ -1476,6 +1483,7 @@ def __get_pv_forecast_akkudoktor_api( f"No PV config entry provided for target: {tgt_value}", {}, "akkudoktor", + target=tgt_value, ) # Use standard request format for both PV and temperature @@ -1490,15 +1498,29 @@ def request_func(): day_values = response.json() return day_values["values"] + # Tracked locally (not via self.pv_forcast_request_error, which can + # still hold a stale error from an earlier unrelated call) so we know + # whether day_values below is raw API JSON or an already-final + # fallback array from _handle_interface_error. + failure = {"occurred": False} + def error_handler(error_type, exception): + failure["occurred"] = True return self._handle_interface_error( error_type, f"Akkudoktor API error for {tgt_value}: {exception}", pv_config_entry, "akkudoktor", + target=tgt_value, ) day_values = self._retry_request(request_func, error_handler, 5, 3) + if failure["occurred"] and day_values: + # day_values is a non-empty cache _handle_interface_error chose as + # fallback; feeding it into the raw-JSON processing below would + # corrupt it. An empty fallback ([] - no cache available) is safe + # to fall through: the processing below pads it to tgt_duration. + return day_values # Data processing try: @@ -1571,14 +1593,20 @@ def error_handler(error_type, exception): ) if self.time_frame_base == 900 and tgt_value == "power": - return self._convert_hourly_to_15min(forecast_values) - # all value have to be repeated 4 times for 15min base for temperature - if self.time_frame_base == 900 and tgt_value == "temperature": - extended_values = [] + result = self._convert_hourly_to_15min(forecast_values) + elif self.time_frame_base == 900 and tgt_value == "temperature": + # all values have to be repeated 4 times for 15min base for temperature + result = [] for val in forecast_values: - extended_values.extend([val] * 4) - return extended_values - return forecast_values + result.extend([val] * 4) + else: + result = forecast_values + + if tgt_value == "temperature" and result: + self.last_successful_temp_forecast = list(result) + self.consecutive_temp_failures = 0 + + return result except (ValueError, TypeError, AttributeError, KeyError) as e: return self._handle_interface_error( @@ -1586,6 +1614,7 @@ def error_handler(error_type, exception): f"Error processing {tgt_value} forecast data: {e}", pv_config_entry, "akkudoktor", + target=tgt_value, ) def __get_horizon_elevation(self, sun_azimuth, horizon_for_elev): @@ -2766,12 +2795,16 @@ def _retry_request(self, request_func, error_handler, max_retries=3, delay=1): time.sleep(delay) def _handle_interface_error( - self, error_type, message, pv_config_entry, source="unknown" + self, error_type, message, pv_config_entry, source="unknown", target="power" ): """ Centralized error handling for all API errors. Uses last successful forecast as fallback if available. Similar to PriceInterface.last_successful_prices mechanism. + + `target` selects which cache/counter pair to use ("power" or + "temperature") so a failed temperature request never falls back to + the PV power cache (and vice versa). """ logger.error("[PV-IF] %s", message) self.pv_forcast_request_error.update( @@ -2783,34 +2816,42 @@ def _handle_interface_error( "source": source, } ) - self.consecutive_failures += 1 + + if target == "temperature": + self.consecutive_temp_failures += 1 + failures = self.consecutive_temp_failures + last_successful = self.last_successful_temp_forecast + else: + self.consecutive_failures += 1 + failures = self.consecutive_failures + last_successful = self.last_successful_pv_forecast # Fallback strategy: Use last successful forecast if available # and within failure threshold - if ( - self.consecutive_failures <= self.max_failures - and len(self.last_successful_pv_forecast) > 0 - ): + if failures <= self.max_failures and len(last_successful) > 0: logger.warning( - "[PV-IF] No forecast retrieved (failure %d/%d). Using last successful forecast.", - self.consecutive_failures, + "[PV-IF] No %s forecast retrieved (failure %d/%d)." + " Using last successful forecast.", + target, + failures, self.max_failures, ) - return self.last_successful_pv_forecast + return last_successful # If max failures exceeded or no cache available, return empty array # (let caller handle default generation) - if len(self.last_successful_pv_forecast) == 0: + if len(last_successful) == 0: logger.warning( - "[PV-IF] No forecast available and no cache - returning empty array" + "[PV-IF] No %s forecast available and no cache - returning empty array", + target, ) # Log detailed recovery diagnostics for troubleshooting - self._log_error_diagnostics(error_type, source) + self._log_error_diagnostics(error_type, source, target) return [] - def _log_error_diagnostics(self, error_type, source): + def _log_error_diagnostics(self, error_type, source, target="power"): """ Log detailed error diagnostics including available sources and recovery hints. Helps users troubleshoot and fix configuration issues faster. @@ -2828,11 +2869,17 @@ def _log_error_diagnostics(self, error_type, source): ] current_source = self.config_source.get("source", "unknown") - if self.consecutive_failures >= self.max_failures: + failures = ( + self.consecutive_temp_failures + if target == "temperature" + else self.consecutive_failures + ) + if failures >= self.max_failures: logger.error( - "[PV-IF] Maximum failures reached (%d) - " + "[PV-IF] Maximum %s failures reached (%d) - " "please check configuration in Settings > PV Forecast", - self.consecutive_failures, + target, + failures, ) if source == "timeseries": diff --git a/tests/interfaces/test_pv_interface.py b/tests/interfaces/test_pv_interface.py index bec8395..07381b4 100644 --- a/tests/interfaces/test_pv_interface.py +++ b/tests/interfaces/test_pv_interface.py @@ -280,6 +280,63 @@ def test_handle_interface_error_with_empty_config(): assert pv.pv_forcast_request_error["timestamp"] is not None +def test_handle_interface_error_temperature_target_ignores_pv_cache(): + """ + Regression test for issue #276: a failed temperature request must not + fall back to the PV power cache, even when it holds unrelated Watt + values and the temperature cache is still empty. + """ + pv = PvInterface({}, [], time_frame_base, {}, timezone="UTC") + pv.last_successful_pv_forecast = [2090.0, 1500.0, 800.0] + pv.last_successful_temp_forecast = [] + + result = pv._handle_interface_error( + "timeout", "temp failed", {}, "akkudoktor", target="temperature" + ) + + assert result == [] + assert pv.consecutive_temp_failures == 1 + assert pv.consecutive_failures == 0 + + +def test_handle_interface_error_temperature_target_uses_own_cache(): + """ + Test that a failed temperature request falls back to its own cache when + available, not the PV power cache. + """ + pv = PvInterface({}, [], time_frame_base, {}, timezone="UTC") + pv.last_successful_pv_forecast = [2090.0, 1500.0, 800.0] + pv.last_successful_temp_forecast = [15.0, 16.0, 17.0] + + result = pv._handle_interface_error( + "timeout", "temp failed", {}, "akkudoktor", target="temperature" + ) + + assert result == [15.0, 16.0, 17.0] + + +def test_consecutive_temp_failures_independent_of_pv_success(): + """ + Temperature failures must accumulate toward max_failures even while PV + power keeps succeeding and resetting its own counter every cycle - + reproduces the 0/1 oscillation from issue #276 that kept the temperature + failure count from ever reaching the threshold. + """ + pv = PvInterface({}, [], time_frame_base, {}, timezone="UTC") + for _ in range(30): + # PV power fetch succeeds and resets only the power counter. + pv.last_successful_pv_forecast = [100.0] + pv.consecutive_failures = 0 + # Temperature fetch fails every cycle. + pv._handle_interface_error( + "timeout", "temp failed", {}, "akkudoktor", target="temperature" + ) + + assert pv.consecutive_temp_failures == 30 + assert pv.consecutive_temp_failures >= pv.max_failures + assert pv.consecutive_failures == 0 + + def test_default_pv_forecast_length_and_values(): """ Test that the default PV forecast returns 48 values of type int or float. @@ -422,6 +479,134 @@ def test_api_error_triggers_fallback(monkeypatch): assert pv.pv_forcast_request_error["error"] in (None, "api_error") +def test_temperature_api_error_never_returns_pv_watts(monkeypatch): + """ + Regression test for issue #276: a total temperature-fetch failure must + never return data derived from the cached PV power (Watts) array, even + though both forecasts are fetched through this same shared method. + """ + pv = PvInterface({}, [], time_frame_base, {}, timezone="UTC") + pv.last_successful_pv_forecast = [2090.0, 1500.0, 800.0] + pv._retry_request = lambda req, err, *args, **kwargs: err( + "timeout", Exception("fail") + ) + + result = pv._PvInterface__get_pv_forecast_akkudoktor_api( + tgt_value="temperature", + pv_config_entry={ + "lat": 50, + "lon": 8, + "azimuth": 180, + "tilt": 30, + "power": 100, + "powerInverter": 800, + "inverterEfficiency": 0.95, + "horizon": "0", + }, + ) + + # No temperature cache exists yet, so this falls through to the same + # zero-padding default the power path uses (see test_api_error_triggers_fallback). + assert result == [0] * 48 + assert 2090.0 not in result + + +def test_temperature_api_error_falls_back_to_temp_cache(monkeypatch): + """ + Once a temperature forecast has succeeded at least once, a later + failure must reuse the temperature cache, not the PV power cache. + """ + pv = PvInterface({}, [], time_frame_base, {}, timezone="UTC") + pv.last_successful_pv_forecast = [2090.0, 1500.0, 800.0] + pv.last_successful_temp_forecast = [15.0, 16.0, 17.0] + pv._retry_request = lambda req, err, *args, **kwargs: err( + "timeout", Exception("fail") + ) + + result = pv._PvInterface__get_pv_forecast_akkudoktor_api( + tgt_value="temperature", + pv_config_entry={ + "lat": 50, + "lon": 8, + "azimuth": 180, + "tilt": 30, + "power": 100, + "powerInverter": 800, + "inverterEfficiency": 0.95, + "horizon": "0", + }, + ) + + assert result == [15.0, 16.0, 17.0] + + +def _run_one_update_loop_iteration(pv): + """ + Helper to run exactly one iteration of the background update loop. + threading.Thread is stubbed by the autouse patch_thread fixture, so the + loop was never actually started - it is safe to invoke directly here. + """ + calls = {"n": 0} + + def is_set_once(): + calls["n"] += 1 + return calls["n"] > 1 + + pv._stop_event.is_set = is_set_once + pv._PvInterface__update_pv_state_loop() + + +def test_update_loop_temperature_failure_never_uses_pv_cache(monkeypatch): + """ + End-to-end regression test for issue #276: with PV power succeeding and + caching Watt values, and the temperature fetch exhausting its own retries + with no temperature cache yet, the update loop must fall back to the + default 15C forecast - never to the cached PV power array. + """ + config = [{"name": "roof", "lat": 50, "lon": 8, "power": 5000}] + pv = PvInterface( + {}, config, time_frame_base, {}, temperature_forecast_enabled=True, + timezone="UTC", + ) + pv.last_successful_pv_forecast = [2090.0, 1500.0, 800.0] + monkeypatch.setattr(pv, "get_summarized_pv_forecast", lambda scale=False: [100.0]) + monkeypatch.setattr(pv, "apply_autoscaling", lambda values: values) + monkeypatch.setattr( + pv, + "_PvInterface__get_pv_forecast_akkudoktor_api", + lambda tgt_value, pv_config_entry: [], + ) + + _run_one_update_loop_iteration(pv) + + assert pv.temp_forecast_array == pv._PvInterface__get_default_temperature_forecast() + assert pv.temp_forecast_array != pv.last_successful_pv_forecast + + +def test_update_loop_rejects_implausible_temperature_values(monkeypatch): + """ + Defense-in-depth: even if a mislabeled PV-Watts array slipped past the + target-aware cache fix, physically implausible values must still be + rejected in favor of the default 15C forecast. + """ + config = [{"name": "roof", "lat": 50, "lon": 8, "power": 5000}] + pv = PvInterface( + {}, config, time_frame_base, {}, temperature_forecast_enabled=True, + timezone="UTC", + ) + monkeypatch.setattr(pv, "get_summarized_pv_forecast", lambda scale=False: [100.0]) + monkeypatch.setattr(pv, "apply_autoscaling", lambda values: values) + monkeypatch.setattr( + pv, + "_PvInterface__get_pv_forecast_akkudoktor_api", + lambda tgt_value, pv_config_entry: [2090.0, 1500.0, 800.0], + ) + + _run_one_update_loop_iteration(pv) + + assert pv.temp_forecast_array == pv._PvInterface__get_default_temperature_forecast() + + def test_get_current_pv_forecast_returns_array(): """ Test that get_current_pv_forecast returns the correct array.