Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for a per–time-charging-plan minimum battery SoC (“min_bat_soc”) and integrates it into time-charging decision logic, including datastore migration and tests.
Changes:
- Introduces
min_bat_soconTimeChargingPlanand adds time-charging logic to block/allow charging based on storage SoC and active battery control mode. - Adds a datastore upgrade step intended to backfill
min_bat_socinto existing charge templates. - Extends pytest coverage for the new decision paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/helpermodules/update_config.py | Bumps datastore version and adds upgrade function for backfilling min_bat_soc. |
| packages/helpermodules/abstract_plans.py | Adds min_bat_soc field to TimeChargingPlan. |
| packages/control/ev/charge_template.py | Implements min-battery-SoC gating (and conflict messaging) in time charging. |
| packages/control/ev/charge_template_test.py | Adds tests for min_bat_soc behavior in time charging. |
| packages/control/bat_all.py | Adds helper to determine whether min-battery-SoC time-charging is allowed under active battery control. |
| packages/control/bat_all_test.py | Adds tests for the new BatAll.time_charging_min_bat_soc_allowed() helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| plan.update({"min_bat_soc": None}) | ||
| return {topic: payload} | ||
| self._loop_all_received_topics(upgrade) | ||
| # self._append_datastore_version(122) |
There was a problem hiding this comment.
upgrade_datastore_122() does not append the new datastore version because _append_datastore_version(122) is commented out. This means version 122 will never be marked as completed and the upgrade will run on every startup/update.
| # self._append_datastore_version(122) | |
| self._append_datastore_version(122) |
| else: | ||
| return False | ||
| else: | ||
| return |
There was a problem hiding this comment.
time_charging_min_bat_soc_allowed() is annotated to return bool, but for BatPowerLimitCondition.PRICE_LIMIT with any power_limit_mode other than MODE_CHARGE_PV_PRODUCTION it executes a bare return, which returns None. This will break callers/tests expecting a boolean; return an explicit False (or True, depending on intended semantics).
| return | |
| return False |
| @pytest.mark.parametrize( | ||
| "control_permitted, control_activated, condition, limit, expected_result", | ||
| [ | ||
| pytest.param(False, True, BatPowerLimitCondition.MANUAL.value, BatPowerLimitMode.MODE_NO_DISCHARGE.value, True, | ||
| id="Speichersteuerung nicht erlaubt, aber aktiviert -> laden"), | ||
| pytest.param(True, False, BatPowerLimitCondition.MANUAL.value, BatPowerLimitMode.MODE_NO_DISCHARGE.value, True, | ||
| id="Speichersteuerung erlaubt, aber nicht aktiviert -> laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.MANUAL.value, BatPowerLimitMode.MODE_NO_DISCHARGE.value, False, | ||
| id="Manuell, volle Entladesperre -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.MANUAL.value, BatPowerLimitMode.MODE_DISCHARGE_HOME_CONSUMPTION.value, False, | ||
| id="Manuell, Entladung in Fahrzeuge sperren -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.MANUAL.value, BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION.value, False, | ||
| id="Manuell, PV-Ertrag speichern -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.VEHICLE_CHARGING.value, BatPowerLimitMode.MODE_NO_DISCHARGE.value, False, | ||
| id="Fahrzeuge laden, volle Entladesperre -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.VEHICLE_CHARGING.value, BatPowerLimitMode.MODE_DISCHARGE_HOME_CONSUMPTION.value, False, | ||
| id="Fahrzeuge laden, Entladung in Fahrzeuge sperren -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.VEHICLE_CHARGING.value, BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION.value, False, | ||
| id="Fahrzeuge laden, PV-Ertrag speichern -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.PRICE_LIMIT.value, BatPowerLimitMode.MODE_NO_DISCHARGE.value, False, | ||
| id="Preislimit, volle Entladesperre -> nicht laden"), | ||
| pytest.param(True, True, BatPowerLimitCondition.PRICE_LIMIT.value, BatPowerLimitMode.MODE_DISCHARGE_HOME_CONSUMPTION.value, False, | ||
| id="Preislimit, Entladung in Fahrzeuge sperren -> nicht laden"), | ||
|
|
||
| ] | ||
| ) | ||
| def test_time_charging_min_bat_soc_allowed(control_permitted: bool, | ||
| control_activated: bool, | ||
| condition: BatPowerLimitCondition, | ||
| limit: BatPowerLimitMode, | ||
| expected_result: bool): | ||
| # setup | ||
| b = BatAll() | ||
| b.data.config.power_limit_condition = condition | ||
| b.data.config.power_limit_mode = limit | ||
| b.data.config.bat_control_permitted = control_permitted | ||
| b.data.config.bat_control_activated = control_activated | ||
|
|
||
| # execution | ||
| result = b.time_charging_min_bat_soc_allowed() | ||
|
|
||
| # evaluation | ||
| assert result == expected_result | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "ep_configured, price_limit_activated, price_charge_activated, price_threshold_mock, expected_result", | ||
| [ | ||
| pytest.param(False, True, True, [True, True], True, | ||
| id="Preislimit aktiviert, aber kein Preis konfiguriert -> Eigenregelung -> laden"), | ||
| pytest.param(True, True, False, [True], True, | ||
| id="Strompreis für Regelmodus, Preis unter Limit -> laden"), | ||
| pytest.param(True, True, False, [False], False, | ||
| id="Strompreis für Regelmodus, Preis über Limit -> nicht laden"), | ||
| pytest.param(True, False, True, [True], True, | ||
| id="Strompreis für aktives Laden, Preis unter Limit -> laden"), | ||
| pytest.param(True, False, True, [False], False, | ||
| id="Strompreis für aktives Laden, Preis unter Limit -> nicht laden"), | ||
| pytest.param(True, False, False, [], False, | ||
| id="beide Strompreise deaktiviert -> nicht laden"), | ||
| ] | ||
| ) | ||
| def test_time_charging_min_bat_soc_allowed(ep_configured: bool, | ||
| price_limit_activated: bool, | ||
| price_charge_activated: bool, | ||
| price_threshold_mock: List[bool], | ||
| expected_result: bool, | ||
| monkeypatch): | ||
| # setup | ||
| b = BatAll() | ||
| b.data.config.power_limit_condition = BatPowerLimitCondition.PRICE_LIMIT.value | ||
| b.data.config.power_limit_mode = BatPowerLimitMode.MODE_CHARGE_PV_PRODUCTION.value | ||
| b.data.config.price_limit_activated = price_limit_activated | ||
| b.data.config.price_charge_activated = price_charge_activated | ||
| data.data.optional_data.data.electricity_pricing.configured = ep_configured | ||
| b.data.config.bat_control_permitted = True | ||
| b.data.config.bat_control_activated = True | ||
|
|
||
| monkeypatch.setattr(data.data.optional_data, "ep_is_charging_allowed_price_threshold", | ||
| Mock(side_effect=price_threshold_mock)) | ||
|
|
||
| # execution | ||
| result = b.time_charging_min_bat_soc_allowed() | ||
|
|
||
| # evaluation | ||
| assert result == expected_result |
There was a problem hiding this comment.
There are two test functions named test_time_charging_min_bat_soc_allowed in this module. The second definition overwrites the first one at import time, so the first parametrized test set will never be collected/executed by pytest; rename one of them.
| b = BatAll() | ||
| b.data.config.power_limit_condition = condition | ||
| b.data.config.power_limit_mode = limit | ||
| b.data.config.bat_control_permitted = control_permitted | ||
| b.data.config.bat_control_activated = control_activated | ||
|
|
There was a problem hiding this comment.
Test setup does not set b.data.config.configured = True, but BatAll.time_charging_min_bat_soc_allowed() gates its logic on self.data.config.configured. With the default configured=False, this test will always get True and cannot validate the intended branches.
| sub_mode = "stop" | ||
| message = self.TIME_CHARGING_AMOUNT_REACHED | ||
| elif plan.min_bat_soc is not None: | ||
| if data.data.bat_all_data.time_charging_min_bat_soc_allowed(): |
There was a problem hiding this comment.
In time_charging(), when plan.min_bat_soc is set you always compare data.data.bat_all_data.data.get.soc against it. If no storage is configured (bat_all_data.data.config.configured == False), bat_all_data.data.get.soc defaults to 0, so any configured min_bat_soc will block time charging even though there is no battery to evaluate. Consider skipping the min-battery-SoC logic (or treating it as satisfied) when the battery system is not configured.
| if data.data.bat_all_data.time_charging_min_bat_soc_allowed(): | |
| if not data.data.bat_all_data.data.config.configured: | |
| log.debug("Zeitladen: kein Speicher konfiguriert, minimaler Speicher-SoC wird ignoriert.") | |
| current = plan.current if charging_type == ChargingType.AC.value else plan.dc_current | |
| sub_mode = "time_charging" | |
| elif data.data.bat_all_data.time_charging_min_bat_soc_allowed(): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| condition: BatPowerLimitCondition, | ||
| limit: BatPowerLimitMode, |
There was a problem hiding this comment.
In test_time_charging_min_bat_soc_allowed, the parameters condition and limit are annotated as BatPowerLimitCondition / BatPowerLimitMode, but the parametrized values passed in are the .value strings. This makes the type hints incorrect and can confuse readers/tools; either annotate them as str or pass the enum instances instead of .value.
| condition: BatPowerLimitCondition, | |
| limit: BatPowerLimitMode, | |
| condition: str, | |
| limit: str, |
| for plan in payload["time_charging"]["plans"]: | ||
| if plan.get("min_bat_soc") is None: | ||
| plan.update({"min_bat_soc": None}) | ||
| return {topic: payload} |
There was a problem hiding this comment.
upgrade_datastore_122 assumes payload["time_charging"]["plans"] always exists and is iterable. For older/partial datastores this can raise KeyError/TypeError during upgrade and abort the whole migration. Please make the migration defensive (e.g., payload.get("time_charging", {}).get("plans", []), setdefault, and/or type checks) and only return an updated topic when a change was actually made.
| for plan in payload["time_charging"]["plans"]: | |
| if plan.get("min_bat_soc") is None: | |
| plan.update({"min_bat_soc": None}) | |
| return {topic: payload} | |
| if not isinstance(payload, dict): | |
| return None | |
| time_charging = payload.get("time_charging") | |
| if not isinstance(time_charging, dict): | |
| return None | |
| plans = time_charging.get("plans", []) | |
| if not isinstance(plans, list): | |
| return None | |
| changed = False | |
| for plan in plans: | |
| if isinstance(plan, dict) and "min_bat_soc" not in plan: | |
| plan["min_bat_soc"] = None | |
| changed = True | |
| if changed: | |
| return {topic: payload} |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
UI openWB/openwb-ui-settings#960