From bb08548c6740cad382f0cb8cfd904eeabfd3a8e7 Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Sat, 22 Aug 2026 08:18:49 +0100 Subject: [PATCH] fix(inverter): auto-create charge_rate entity for script-driven "power" inverters (#3311) Solax's low-power-mode charge rate is driven via charge_start_service (a plain HA script call), not a REST/cloud API, but output_charge_control: "power" meant the dummy charge_rate/discharge_rate entities were only ever auto-created for "current" mode. get_current_charge_rate() and adjust_charge_rate() both fall back to self.base.args["charge_rate"] regardless of output_charge_control unless the inverter is genuinely REST-driven - without the entity, the computed rate had nowhere to be stored and read back as battery_rate_max_raw, sending full power to the script regardless of what was planned. Only auto-create when nothing is already configured - unlike "current" mode (where charge_rate is always a synthetic mirror), a "power" inverter's charge_rate can be a real user-configured entity (e.g. GE's own "if not using REST" apps.yaml template) that must not be overwritten. Co-Authored-By: Claude Sonnet 5 --- apps/predbat/inverter.py | 22 +++++ apps/predbat/tests/test_inverter.py | 133 ++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/apps/predbat/inverter.py b/apps/predbat/inverter.py index b3ae9ebff..2fcd3f1d2 100644 --- a/apps/predbat/inverter.py +++ b/apps/predbat/inverter.py @@ -548,12 +548,34 @@ def __init__(self, base, id=0, quiet=False, rest_postCommand=None, rest_getData= self.base.args["charge_limit"][id] = self.create_entity("charge_limit", 100, device_class=None, uom="%", icon="mdi:target") if self.inv_output_charge_control != "power": + # "current" (and "none") mode: charge_rate is always a synthetic mirror of the real + # current-based control, not something a user configures directly, so it's always + # (re)created here regardless of whatever - if anything - is already in args. max_charge = self.battery_rate_max_charge * MINUTE_WATT max_discharge = self.battery_rate_max_discharge * MINUTE_WATT self.create_missing_arg("charge_rate", max_charge) self.create_missing_arg("discharge_rate", max_discharge) self.base.args["charge_rate"][id] = self.create_entity("charge_rate", max_charge, uom="W", device_class="power") self.base.args["discharge_rate"][id] = self.create_entity("discharge_rate", max_discharge, uom="W", device_class="power") + elif not self.rest_data: + # "power" mode inverters normally write the rate straight to the inverter (REST/cloud + # API) with no HA entity involved, so charge_rate is usually left for the user to + # configure only if they want one (e.g. GE's "if not using REST" apps.yaml comment) - + # unlike "current" mode above, an already-configured value here is real, not a + # placeholder, so it must not be overwritten. But a REST-less "power" inverter that's + # driven by a script rather than a native register (Solax, #3311) still needs + # get_current_charge_rate()/adjust_charge_rate() to have *something* to read/write - + # without an entity the computed rate has nowhere to be stored and reads back as + # battery_rate_max_raw instead, sending full power to the script regardless of what + # was actually planned. Only fill in the gap when nothing is configured at all. + max_charge = self.battery_rate_max_charge * MINUTE_WATT + max_discharge = self.battery_rate_max_discharge * MINUTE_WATT + if "charge_rate" not in self.base.args: + self.create_missing_arg("charge_rate", max_charge) + self.base.args["charge_rate"][id] = self.create_entity("charge_rate", max_charge, uom="W", device_class="power") + if "discharge_rate" not in self.base.args: + self.create_missing_arg("discharge_rate", max_discharge) + self.base.args["discharge_rate"][id] = self.create_entity("discharge_rate", max_discharge, uom="W", device_class="power") if not self.inv_has_ge_inverter_mode and not self.inv_has_fox_inverter_mode and not self.inv_has_ge_eco_toggle: self.create_missing_arg("inverter_mode", "Eco") diff --git a/apps/predbat/tests/test_inverter.py b/apps/predbat/tests/test_inverter.py index b32b58ea8..8e1b6e9a4 100644 --- a/apps/predbat/tests/test_inverter.py +++ b/apps/predbat/tests/test_inverter.py @@ -526,6 +526,131 @@ def test_current_reasserted_on_unchanged_rate(test_name, ha, inv, prev_current, return failed +def test_low_power_mode_entity_created_for_script_driven_power_inverter(test_name, my_predbat): + """ + #3311: a "power" output_charge_control inverter normally writes its rate straight to the + inverter (REST/cloud API) with no HA entity involved, so the dummy charge_rate/discharge_rate + entities are usually skipped for it. But a REST-less "power" inverter (Solax, driven via + charge_start_service/a script rather than a REST API) still reads/writes the rate through + self.base.args["charge_rate"] exactly like "current" mode does - without the entity, the + computed low-power-mode rate has nowhere to be stored, so get_current_charge_rate() falls back + to battery_rate_max_raw and the script is sent full power regardless of what was planned. + """ + print("**** Running Test: {} ****".format(test_name)) + failed = False + + saved_args = {key: my_predbat.args.get(key) for key in ["inverter_type", "givtcp_rest", "charge_rate", "discharge_rate", "charge_rate_percent", "discharge_rate_percent"]} + try: + my_predbat.args["inverter_type"] = ["GE"] # GE's output_charge_control is "power" + my_predbat.args["givtcp_rest"] = None # no REST configured - script/service driven + for key in ["charge_rate", "discharge_rate", "charge_rate_percent", "discharge_rate_percent"]: + my_predbat.args.pop(key, None) + + inv = Inverter(my_predbat, 0) + + if inv.inv_output_charge_control != "power": + print("ERROR: {} test fixture assumption broken - GE output_charge_control is no longer 'power'".format(test_name)) + failed = True + if inv.rest_data: + print("ERROR: {} test fixture assumption broken - rest_data unexpectedly populated with no REST configured".format(test_name)) + failed = True + + if "charge_rate" not in my_predbat.args: + print("ERROR: {} charge_rate entity was not auto-created for a REST-less 'power' inverter".format(test_name)) + failed = True + if "discharge_rate" not in my_predbat.args: + print("ERROR: {} discharge_rate entity was not auto-created for a REST-less 'power' inverter".format(test_name)) + failed = True + finally: + for key, value in saved_args.items(): + if value is None: + my_predbat.args.pop(key, None) + else: + my_predbat.args[key] = value + + return failed + + +def test_low_power_mode_entity_not_clobbered_when_already_configured(test_name, my_predbat): + """ + Guards the actual bug hit while building the #3311 fix: a non-REST "power" inverter that + already has a real charge_rate/discharge_rate configured (e.g. GE's own coverage/apps.yaml, + which sets charge_rate for the "if not using REST" case) must keep it - the entity-creation + block must only fill genuine gaps, not overwrite an already-configured real entity with a + fresh dummy one. + """ + print("**** Running Test: {} ****".format(test_name)) + failed = False + + saved_args = {key: my_predbat.args.get(key) for key in ["inverter_type", "givtcp_rest", "charge_rate", "discharge_rate", "charge_rate_percent", "discharge_rate_percent"]} + try: + my_predbat.args["inverter_type"] = ["GE"] + my_predbat.args["givtcp_rest"] = None + my_predbat.args["charge_rate"] = ["number.real_charge_rate", "number.real_charge_rate", "number.real_charge_rate", "number.real_charge_rate"] + my_predbat.args["discharge_rate"] = ["number.real_discharge_rate", "number.real_discharge_rate", "number.real_discharge_rate", "number.real_discharge_rate"] + for key in ["charge_rate_percent", "discharge_rate_percent"]: + my_predbat.args.pop(key, None) + + Inverter(my_predbat, 0) + + if my_predbat.args["charge_rate"][0] != "number.real_charge_rate": + print("ERROR: {} pre-configured charge_rate was clobbered by auto-creation, now {}".format(test_name, my_predbat.args["charge_rate"][0])) + failed = True + if my_predbat.args["discharge_rate"][0] != "number.real_discharge_rate": + print("ERROR: {} pre-configured discharge_rate was clobbered by auto-creation, now {}".format(test_name, my_predbat.args["discharge_rate"][0])) + failed = True + finally: + for key, value in saved_args.items(): + if value is None: + my_predbat.args.pop(key, None) + else: + my_predbat.args[key] = value + + return failed + + +def test_low_power_mode_entity_not_created_for_rest_driven_power_inverter(test_name, my_predbat): + """ + Companion to test_low_power_mode_entity_created_for_script_driven_power_inverter - a genuinely + REST-driven "power" inverter (GE with givtcp_rest configured) reads/writes its rate directly via + the REST API (get_current_charge_rate()'s self.rest_data branch), so it still doesn't need the + dummy entity. Guards against the #3311 fix over-widening and creating unused entities for the + inverters the original behaviour was correct for. + """ + print("**** Running Test: {} ****".format(test_name)) + failed = False + + saved_args = {key: my_predbat.args.get(key) for key in ["inverter_type", "givtcp_rest", "charge_rate", "discharge_rate", "charge_rate_percent", "discharge_rate_percent"]} + try: + my_predbat.args["inverter_type"] = ["GE"] + my_predbat.args["givtcp_rest"] = "dummy" + for key in ["charge_rate", "discharge_rate", "charge_rate_percent", "discharge_rate_percent"]: + my_predbat.args.pop(key, None) + + dummy_rest = DummyRestAPI() + dummy_rest.rest_data = {"Control": {}, "Stats": {}, "raw": {"invertor": {}}, "Invertor_Details": {}} + inv = Inverter(my_predbat, 0, rest_postCommand=dummy_rest.dummy_rest_postCommand, rest_getData=dummy_rest.dummy_rest_getData) + + if not inv.rest_data: + print("ERROR: {} test fixture assumption broken - rest_data not populated with REST configured".format(test_name)) + failed = True + + if "charge_rate" in my_predbat.args: + print("ERROR: {} charge_rate entity was auto-created for a REST-driven 'power' inverter - should read/write via REST directly".format(test_name)) + failed = True + if "discharge_rate" in my_predbat.args: + print("ERROR: {} discharge_rate entity was auto-created for a REST-driven 'power' inverter - should read/write via REST directly".format(test_name)) + failed = True + finally: + for key, value in saved_args.items(): + if value is None: + my_predbat.args.pop(key, None) + else: + my_predbat.args[key] = value + + return failed + + def test_adjust_inverter_mode(test_name, ha, inv, dummy_rest, prev_mode, mode, expect_mode=None): """ Test the adjust_inverter_mode function @@ -2933,6 +3058,14 @@ def run_inverter_tests(my_predbat_dummy): if failed: return failed + # #3311: script-driven "power" inverters (Solax) still need the charge_rate/discharge_rate + # dummy entity, unlike genuinely REST-driven "power" inverters (GE with givtcp_rest set) + failed |= test_low_power_mode_entity_created_for_script_driven_power_inverter("low_power_entity_created_script_driven", my_predbat) + failed |= test_low_power_mode_entity_not_clobbered_when_already_configured("low_power_entity_not_clobbered", my_predbat) + failed |= test_low_power_mode_entity_not_created_for_rest_driven_power_inverter("low_power_entity_not_created_rest_driven", my_predbat) + if failed: + return failed + failed |= test_adjust_reserve("adjust_reserve1", ha, inv, dummy_rest, 4, 50, reserve_max=100) failed |= test_adjust_reserve("adjust_reserve2", ha, inv, dummy_rest, 50, 0, 4, reserve_max=100) failed |= test_adjust_reserve("adjust_reserve3", ha, inv, dummy_rest, 20, 100, reserve_max=100)