From 641a960c1c1090146e8d92a44c659b978ab7e82c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 15:36:38 +0000 Subject: [PATCH 1/3] Initial plan From 3dbe023dc0c6bcda8b6ec5d54d3d6ede8e4a6bda Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 15:42:19 +0000 Subject: [PATCH 2/3] fix: Address PR review feedback - clean up imports, restore tests, fix bounds logic, remove retain Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com> --- src/batcontrol/core.py | 31 ++++++++++++++--- src/batcontrol/mqtt_api.py | 3 +- tests/batcontrol/test_core.py | 40 +++++++++++++++++----- tests/batcontrol/test_production_offset.py | 31 +++++++++++++++++ 4 files changed, 90 insertions(+), 15 deletions(-) diff --git a/src/batcontrol/core.py b/src/batcontrol/core.py index ec9fe18d..e27e73c4 100644 --- a/src/batcontrol/core.py +++ b/src/batcontrol/core.py @@ -579,12 +579,35 @@ def limit_battery_charge_rate(self, limit_charge_rate: int = 0): self.allow_discharging() return - # Apply bounds from config - effective_limit = limit_charge_rate + # Always enforce a non-negative limit + effective_limit = max(0, limit_charge_rate) + if self.max_pv_charge_rate > 0: + # Guard against misconfigured min/max values + if ( + self.min_pv_charge_rate > 0 + and self.min_pv_charge_rate > self.max_pv_charge_rate + ): + logger.warning( + 'Configured min_pv_charge_rate (%d W) is greater than ' + 'max_pv_charge_rate (%d W). Adjusting minimum to max.', + self.min_pv_charge_rate, + self.max_pv_charge_rate, + ) + min_bound = self.max_pv_charge_rate + else: + min_bound = self.min_pv_charge_rate + + # First cap to the configured maximum effective_limit = min(effective_limit, self.max_pv_charge_rate) - if self.min_pv_charge_rate > 0 and limit_charge_rate > 0: - effective_limit = max(effective_limit, self.min_pv_charge_rate) + + # Then enforce the (possibly adjusted) minimum, without exceeding max + if min_bound > 0 and effective_limit > 0: + effective_limit = min(max(effective_limit, min_bound), self.max_pv_charge_rate) + else: + # No max configured (<= 0): only enforce minimum if both are positive + if self.min_pv_charge_rate > 0 and effective_limit > 0: + effective_limit = max(effective_limit, self.min_pv_charge_rate) logger.info('Mode: Limit Battery Charge Rate to %d W, discharge allowed', effective_limit) self.inverter.set_mode_limit_battery_charge(effective_limit) diff --git a/src/batcontrol/mqtt_api.py b/src/batcontrol/mqtt_api.py index 4ae754a4..67e7749d 100644 --- a/src/batcontrol/mqtt_api.py +++ b/src/batcontrol/mqtt_api.py @@ -200,8 +200,7 @@ def publish_limit_battery_charge_rate(self, limit: int) -> None: if self.client.is_connected(): self.client.publish( self.base_topic + '/limit_battery_charge_rate', - limit, - retain=True + limit ) def publish_production( diff --git a/tests/batcontrol/test_core.py b/tests/batcontrol/test_core.py index 1391e4fb..4c9bedc8 100644 --- a/tests/batcontrol/test_core.py +++ b/tests/batcontrol/test_core.py @@ -1,19 +1,10 @@ """Tests for core batcontrol functionality including MODE_LIMIT_BATTERY_CHARGE_RATE""" import pytest -import sys -import os from unittest.mock import MagicMock, patch -# Add the src directory to Python path for testing -sys.path.insert(0, os.path.join( - os.path.dirname(__file__), '..', '..', 'src')) - from batcontrol.core import ( Batcontrol, - MODE_ALLOW_DISCHARGING, - MODE_AVOID_DISCHARGING, MODE_LIMIT_BATTERY_CHARGE_RATE, - MODE_FORCE_CHARGING ) @@ -170,6 +161,37 @@ def test_limit_battery_charge_rate_zero_allowed( # Verify it was set to 0 (charging blocked) mock_inverter.set_mode_limit_battery_charge.assert_called_once_with(0) + @patch('batcontrol.core.tariff_factory.create_tarif_provider') + @patch('batcontrol.core.inverter_factory.create_inverter') + @patch('batcontrol.core.solar_factory.create_solar_provider') + @patch('batcontrol.core.consumption_factory.create_consumption') + def test_limit_battery_charge_rate_min_exceeds_max( + self, mock_consumption, mock_solar, mock_inverter_factory, mock_tariff, + mock_config): + """Test that when min_pv_charge_rate > max_pv_charge_rate, limit is clamped to max""" + # Misconfigured: min (500) > max (3000) - wait, let's use min>max: min=4000, max=3000 + mock_config['inverter']['min_pv_charge_rate'] = 4000 + + # Setup mocks + mock_inverter = MagicMock() + mock_inverter.max_pv_charge_rate = 3000 + mock_inverter.set_mode_limit_battery_charge = MagicMock() + mock_inverter.get_max_capacity = MagicMock(return_value=10000) + mock_inverter_factory.return_value = mock_inverter + + mock_tariff.return_value = MagicMock() + mock_solar.return_value = MagicMock() + mock_consumption.return_value = MagicMock() + + # Create Batcontrol instance + bc = Batcontrol(mock_config) + + # Set any positive limit - should be clamped to max_pv_charge_rate (3000) + bc.limit_battery_charge_rate(1000) + + # Verify effective limit does not exceed max_pv_charge_rate + mock_inverter.set_mode_limit_battery_charge.assert_called_once_with(3000) + @patch('batcontrol.core.tariff_factory.create_tarif_provider') @patch('batcontrol.core.inverter_factory.create_inverter') @patch('batcontrol.core.solar_factory.create_solar_provider') diff --git a/tests/batcontrol/test_production_offset.py b/tests/batcontrol/test_production_offset.py index 3b8fe969..b79fa6e7 100644 --- a/tests/batcontrol/test_production_offset.py +++ b/tests/batcontrol/test_production_offset.py @@ -165,6 +165,37 @@ def test_production_offset_applied_to_forecast(self, mock_config): assert batcontrol.last_production[1] == pytest.approx(1000.0) assert batcontrol.last_production[2] == pytest.approx(1500.0) + def test_production_offset_api_set_valid(self, mock_config): + """Test setting production offset via API with a valid mid-range value""" + with patch('batcontrol.core.tariff_factory'), \ + patch('batcontrol.core.inverter_factory'), \ + patch('batcontrol.core.solar_factory'), \ + patch('batcontrol.core.consumption_factory'): + + batcontrol = Batcontrol(mock_config) + + # Set a typical valid value (70% of production) + batcontrol.api_set_production_offset(0.7) + + # Should be updated + assert batcontrol.production_offset_percent == pytest.approx(0.7) + + def test_production_offset_api_set_invalid_negative(self, mock_config): + """Test setting production offset via API with an invalid negative value""" + with patch('batcontrol.core.tariff_factory'), \ + patch('batcontrol.core.inverter_factory'), \ + patch('batcontrol.core.solar_factory'), \ + patch('batcontrol.core.consumption_factory'): + + batcontrol = Batcontrol(mock_config) + original_value = batcontrol.production_offset_percent + + # Try to set invalid negative value + batcontrol.api_set_production_offset(-0.5) + + # Should not be updated + assert batcontrol.production_offset_percent == original_value + def test_production_offset_api_set_invalid_too_high(self, mock_config): """Test setting production offset via API with invalid high value""" with patch('batcontrol.core.tariff_factory'), \ From 274fa8e45cd36438829786903a8775e579a68202 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 15:48:58 +0000 Subject: [PATCH 3/3] refactor: Move min/max PV charge rate guard to initialization Co-authored-by: MaStr <1036501+MaStr@users.noreply.github.com> --- src/batcontrol/core.py | 46 ++++++++++++++++++----------------- tests/batcontrol/test_core.py | 8 +++--- 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/batcontrol/core.py b/src/batcontrol/core.py index e27e73c4..a21b0592 100644 --- a/src/batcontrol/core.py +++ b/src/batcontrol/core.py @@ -149,10 +149,28 @@ def __init__(self, configdict: dict): self.inverter = inverter_factory.create_inverter( config['inverter']) - # Get PV charge rate limits from inverter config (with defaults) - self.max_pv_charge_rate = getattr(self.inverter, 'max_pv_charge_rate', 0) + # Get PV charge rate limits from inverter config (with defaults), + # falling back to inverter attribute for backward compatibility + self.max_pv_charge_rate = config['inverter'].get( + 'max_pv_charge_rate', + getattr(self.inverter, 'max_pv_charge_rate', 0), + ) self.min_pv_charge_rate = config['inverter'].get('min_pv_charge_rate', 0) + # Validate min/max PV charge rate configuration at startup + if ( + self.max_pv_charge_rate > 0 + and self.min_pv_charge_rate > 0 + and self.min_pv_charge_rate > self.max_pv_charge_rate + ): + logger.warning( + 'Configured min_pv_charge_rate (%d W) is greater than ' + 'max_pv_charge_rate (%d W). Adjusting minimum to max.', + self.min_pv_charge_rate, + self.max_pv_charge_rate, + ) + self.min_pv_charge_rate = self.max_pv_charge_rate + self.pvsettings = config['pvinstallations'] self.fc_solar = solar_factory.create_solar_provider( self.pvsettings, @@ -583,27 +601,11 @@ def limit_battery_charge_rate(self, limit_charge_rate: int = 0): effective_limit = max(0, limit_charge_rate) if self.max_pv_charge_rate > 0: - # Guard against misconfigured min/max values - if ( - self.min_pv_charge_rate > 0 - and self.min_pv_charge_rate > self.max_pv_charge_rate - ): - logger.warning( - 'Configured min_pv_charge_rate (%d W) is greater than ' - 'max_pv_charge_rate (%d W). Adjusting minimum to max.', - self.min_pv_charge_rate, - self.max_pv_charge_rate, - ) - min_bound = self.max_pv_charge_rate - else: - min_bound = self.min_pv_charge_rate - - # First cap to the configured maximum + # Cap to the configured maximum effective_limit = min(effective_limit, self.max_pv_charge_rate) - - # Then enforce the (possibly adjusted) minimum, without exceeding max - if min_bound > 0 and effective_limit > 0: - effective_limit = min(max(effective_limit, min_bound), self.max_pv_charge_rate) + # Enforce minimum (guaranteed <= max_pv_charge_rate from init validation) + if self.min_pv_charge_rate > 0 and effective_limit > 0: + effective_limit = max(effective_limit, self.min_pv_charge_rate) else: # No max configured (<= 0): only enforce minimum if both are positive if self.min_pv_charge_rate > 0 and effective_limit > 0: diff --git a/tests/batcontrol/test_core.py b/tests/batcontrol/test_core.py index 4c9bedc8..4482b8b4 100644 --- a/tests/batcontrol/test_core.py +++ b/tests/batcontrol/test_core.py @@ -168,8 +168,7 @@ def test_limit_battery_charge_rate_zero_allowed( def test_limit_battery_charge_rate_min_exceeds_max( self, mock_consumption, mock_solar, mock_inverter_factory, mock_tariff, mock_config): - """Test that when min_pv_charge_rate > max_pv_charge_rate, limit is clamped to max""" - # Misconfigured: min (500) > max (3000) - wait, let's use min>max: min=4000, max=3000 + """Test that when min_pv_charge_rate > max_pv_charge_rate, min is clamped to max at init""" mock_config['inverter']['min_pv_charge_rate'] = 4000 # Setup mocks @@ -183,9 +182,12 @@ def test_limit_battery_charge_rate_min_exceeds_max( mock_solar.return_value = MagicMock() mock_consumption.return_value = MagicMock() - # Create Batcontrol instance + # Create Batcontrol instance — misconfiguration is corrected at init bc = Batcontrol(mock_config) + # min_pv_charge_rate should have been clamped to max_pv_charge_rate at init + assert bc.min_pv_charge_rate == 3000 + # Set any positive limit - should be clamped to max_pv_charge_rate (3000) bc.limit_battery_charge_rate(1000)