From d46178fc8f7f3aeaa51343ee60386329f2e0b44f Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Sat, 15 Aug 2026 23:04:17 +0100 Subject: [PATCH] fix(inverter): enable Enable_Charge_Target before writing a GivTCP discharge target (#4517) adjust_force_export() decided a discharge-target write was needed based on rest_readDischargeTarget() alone, but on some inverters GivTCP reports each write as successful while it never actually sticks - Control.Enable_Charge_Target stays off, so the value is written but the inverter doesn't treat it as authoritative, and Predbat sees a permanent mismatch and rewrites every cycle indefinitely. adjust_battery_target() already calls rest_enableChargeTarget(True) before writing a charge target for exactly this reason (documented: without it the inverter ignores the target). Mirror that call for the discharge target too. No-ops wherever it's already enabled, which is the common case. Whether every rest_v3 inverter actually has this register at all is unconfirmed, so this is deliberately hardened for the case it's wrong: - If enabling itself fails, skip the discharge-target write rather than also attempting one already known to be doomed - a genuinely unsupported inverter retries one REST endpoint per cycle instead of two. - After DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES consecutive cycles where enabling has never once succeeded, stop attempting either endpoint entirely rather than retrying forever, and surface it via record_status so the reduced safety margin (no inverter-side reserve backstop during force export) is visible, not just logged. Resets naturally on restart and on any genuine success, so a working register never gets written off, and a regression (worked before, now failing) is flagged distinctly rather than silently treated the same as never-supported. Co-Authored-By: Claude Sonnet 5 --- apps/predbat/inverter.py | 46 +++++- apps/predbat/tests/test_inverter.py | 231 +++++++++++++++++++++++++++- 2 files changed, 275 insertions(+), 2 deletions(-) diff --git a/apps/predbat/inverter.py b/apps/predbat/inverter.py index cce72b408..9befef8f3 100644 --- a/apps/predbat/inverter.py +++ b/apps/predbat/inverter.py @@ -29,6 +29,12 @@ TIME_FORMAT_HMS = "%H:%M:%S" +# Give up enabling/writing the discharge target register after this many consecutive cycles where +# rest_enableChargeTarget() itself fails - stops a genuinely unsupported inverter retrying forever, +# every cycle, indefinitely (#4517). Small enough to bound the write storm quickly, but more than 1 +# so a single transient GivTCP/network hiccup doesn't permanently give up on a working register. +DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES = 3 + class Inverter: """Unified inverter control abstraction for multiple brands. @@ -186,6 +192,9 @@ def __init__(self, base, id=0, quiet=False, rest_postCommand=None, rest_getData= self.rest_v3 = False self.serial_number = "Unknown" self.count_register_writes = 0 + self.discharge_target_enable_failures = 0 + self.discharge_target_enable_unsupported = False + self.discharge_target_enable_ever_succeeded = False self.created_attributes = {} self.track_charge_start = "00:00:00" self.track_charge_end = "00:00:00" @@ -2527,7 +2536,42 @@ def adjust_force_export(self, force_export, new_start_time=None, new_end_time=No if current is None: self.log("Inverter {} No current discharge target to read, export target not written".format(self.id)) elif current != target_soc: - self.rest_setDischargeTarget(target_soc) + # Without Enable_Charge_Target on, the inverter ignores a written discharge + # target - GivTCP still reports the write as successful, but it never sticks, so + # this repeats every cycle indefinitely (#4517). adjust_battery_target() already + # enables this before writing a charge target; mirror that here. No-ops if + # already enabled. + if self.discharge_target_enable_unsupported: + # Already gave up after DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES consecutive + # failures below, having never once succeeded - don't keep retrying either + # endpoint forever on hardware that has demonstrated it can't do this, or the + # fix just moves the write storm from setDischargeTarget onto + # enableChargeTarget instead of reducing it. + pass + elif self.rest_enableChargeTarget(True): + self.discharge_target_enable_failures = 0 + self.discharge_target_enable_ever_succeeded = True + self.rest_setDischargeTarget(target_soc) + else: + self.discharge_target_enable_failures += 1 + if self.discharge_target_enable_failures >= DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES and not self.discharge_target_enable_ever_succeeded: + # Never once worked since startup - a genuine capability gap, not a + # transient blip, so stop attempting it rather than retrying forever. + self.discharge_target_enable_unsupported = True + message = "Warn: Inverter {} Enable_Charge_Target failed {} cycles in a row and has never succeeded, giving up on the discharge target register as unsupported".format(self.id, self.discharge_target_enable_failures) + self.log(message) + self.base.record_status(message, had_errors=True) + elif self.discharge_target_enable_failures >= DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES: + # Worked before, failing now - a different problem (regression, GivTCP + # restart, transient fault) to "never supported at all", so keep retrying + # rather than writing the hardware off permanently, but flag it loudly. + message = "Warn: Inverter {} Enable_Charge_Target has failed {} cycles in a row after previously working - not giving up, but this looks like a regression rather than an unsupported inverter".format( + self.id, self.discharge_target_enable_failures + ) + self.log(message) + self.base.record_status(message, had_errors=True) + else: + self.log("Inverter {} Unable to enable charge target, discharge target not written".format(self.id)) else: self.log("Inverter {} Current discharge target is already set to {}".format(self.id, current)) elif "discharge_target_soc" in self.base.args: diff --git a/apps/predbat/tests/test_inverter.py b/apps/predbat/tests/test_inverter.py index 511f425d3..33b7af7a2 100644 --- a/apps/predbat/tests/test_inverter.py +++ b/apps/predbat/tests/test_inverter.py @@ -17,7 +17,7 @@ from tests.test_infra import TestHAInterface from predbat import PredBat from const import MINUTE_WATT, INVERTER_MAX_RETRY_REST -from inverter import Inverter +from inverter import Inverter, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES def dummy_sleep(seconds): @@ -344,6 +344,10 @@ def test_adjust_force_export(test_name, ha, inv, dummy_rest, prev_start, prev_en inv.rest_data["Control"] = {} inv.rest_data["Control"]["Enable_Discharge_Schedule"] = prev_force_export inv.rest_data["Control"]["Mode"] = prev_mode + # Already enabled, matching a normal working inverter - keeps this helper's assertions about the + # discharge target/slot/mode writes focused on those, rather than also exercising the + # enableChargeTarget guard (#4517 follow-up), which has its own dedicated test. + inv.rest_data["Control"]["Enable_Charge_Target"] = "enable" inv.rest_data["Timeslots"] = {} inv.rest_data["Timeslots"]["Discharge_start_time_slot_1"] = prev_start inv.rest_data["Timeslots"]["Discharge_end_time_slot_1"] = prev_end @@ -1927,6 +1931,213 @@ def test_discharge_target_control_signal(test_name, ha, inv, dummy_rest): return failed +def test_discharge_target_enables_before_write(test_name, ha, inv, dummy_rest): + """ + Regression test for issue #4517 (follow-up): on some inverters (confirmed against a real + reporter's REST_Response.txt - AC-coupled, GivTCP reports Control.Enable_Charge_Target as + "disable" and Control.Discharge_Target_SOC_1 is entirely absent), a written discharge target + never sticks while Enable_Charge_Target stays off - GivTCP's own log still reports each write as + a success, but it doesn't take, so adjust_force_export sees a permanent mismatch and rewrites + every cycle indefinitely. + + adjust_battery_target() already calls rest_enableChargeTarget(True) before writing a charge + target for exactly this reason (the register is documented to be ignored otherwise) - this + mirrors that same call for the discharge target write. Not confirmed to be the true root cause + on real hardware yet (correlational, from one working and one broken sample), but low-risk: it's + a no-op wherever Enable_Charge_Target is already on, which is the common case exercised by every + other discharge-target test in this file. + """ + failed = False + print("Test: {}".format(test_name)) + + saved_rest_data = inv.rest_data + saved_rest_api = inv.rest_api + saved_rest_v3 = inv.rest_v3 + + try: + inv.rest_api = "dummy" + inv.rest_v3 = True + inv.reserve_percent = 20 + + start_time = "03:33:00" + end_time = "04:44:00" + ts = datetime.strptime(start_time, "%H:%M:%S") + te = datetime.strptime(end_time, "%H:%M:%S") + + # Mirrors a reporter's real REST_Response.txt: Enable_Charge_Target off, no + # Discharge_Target_SOC_1 key in Control at all, raw.invertor reporting a plausible but wrong + # value (so rest_readDischargeTarget's fallback doesn't return None and skip the write). + # Timeslots/Enable_Discharge_Schedule/Mode already match the new values so only the discharge + # target write itself is exercised - the slot/mode writes have their own dedicated tests. + inv.rest_data = { + "Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"}, + "Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time}, + "raw": {"invertor": {"discharge_target_soc_1": "4"}}, + } + dummy_rest.clear_queue() + dummy_rest.rest_data = copy.deepcopy(inv.rest_data) + + # First queued response: consumed by rest_enableChargeTarget's own readback, showing GivTCP + # accepted the enable. + enabled = copy.deepcopy(inv.rest_data) + enabled["Control"]["Enable_Charge_Target"] = "enable" + dummy_rest.queue_rest_data(enabled) + + # Second queued response: consumed by rest_setDischargeTarget's readback, now that the + # register is enabled the write finally sticks. + settled = copy.deepcopy(enabled) + settled["Control"]["Discharge_Target_SOC_1"] = "20" + dummy_rest.queue_rest_data(settled) + + inv.adjust_force_export(True, ts, te) + + commands = dummy_rest.get_commands() + expect_commands = [ + ["dummy/enableChargeTarget", {"state": "enable"}], + ["dummy/setDischargeTarget", {"dischargeToPercent": 20, "slot": 1}], + ] + if json.dumps(commands[:2]) != json.dumps(expect_commands): + print("ERROR: {}: expected enableChargeTarget before setDischargeTarget, got {}".format(test_name, commands)) + failed = True + finally: + inv.rest_data = saved_rest_data + inv.rest_api = saved_rest_api + inv.rest_v3 = saved_rest_v3 + + return failed + + +def test_discharge_target_skips_write_when_enable_fails(test_name, ha, inv, dummy_rest): + """ + Regression test for issue #4517 (follow-up): if Enable_Charge_Target can't be turned on at all - + e.g. a genuinely unsupported register on this inverter, not just left off - don't also attempt a + discharge-target write we already know is doomed. Without this, a genuinely unsupported inverter + would retry *two* REST endpoints every cycle (enableChargeTarget then setDischargeTarget) instead + of the one it retried before this fix, which would make the write-storm this fix exists to + reduce worse, not better, for exactly the inverters it can't help. + """ + failed = False + print("Test: {}".format(test_name)) + + saved_rest_data = inv.rest_data + saved_rest_api = inv.rest_api + saved_rest_v3 = inv.rest_v3 + + try: + inv.rest_api = "dummy" + inv.rest_v3 = True + inv.reserve_percent = 20 + + start_time = "03:33:00" + end_time = "04:44:00" + ts = datetime.strptime(start_time, "%H:%M:%S") + te = datetime.strptime(end_time, "%H:%M:%S") + + inv.rest_data = { + "Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"}, + "Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time}, + "raw": {"invertor": {"discharge_target_soc_1": "4"}}, + } + dummy_rest.clear_queue() + dummy_rest.rest_data = copy.deepcopy(inv.rest_data) + # Enable_Charge_Target never flips to "enable" on any readback - simulates a register the + # inverter genuinely doesn't support, so rest_enableChargeTarget exhausts its retries and + # returns False. + + inv.adjust_force_export(True, ts, te) + + commands = dummy_rest.get_commands() + set_discharge_target_commands = [c for c in commands if c[0] == "dummy/setDischargeTarget"] + if set_discharge_target_commands: + print("ERROR: {}: setDischargeTarget should not be attempted when enabling fails, got {}".format(test_name, commands)) + failed = True + enable_charge_target_commands = [c for c in commands if c[0] == "dummy/enableChargeTarget"] + if not enable_charge_target_commands: + print("ERROR: {}: expected at least one enableChargeTarget attempt, got {}".format(test_name, commands)) + failed = True + finally: + inv.rest_data = saved_rest_data + inv.rest_api = saved_rest_api + inv.rest_v3 = saved_rest_v3 + + return failed + + +def test_discharge_target_gives_up_after_repeated_enable_failures(test_name, ha, inv, dummy_rest): + """ + Regression test for issue #4517 (follow-up): skipping the discharge-target write while still + retrying enableChargeTarget every single cycle forever just relocates the write storm this fix + exists to reduce from one REST endpoint to another - it doesn't reduce it. After + DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES consecutive cycle-level failures, Predbat should stop + attempting either endpoint on hardware that has demonstrated it can't do this, converging to + zero further REST commands rather than retrying indefinitely. + """ + failed = False + print("Test: {}".format(test_name)) + + saved_rest_data = inv.rest_data + saved_rest_api = inv.rest_api + saved_rest_v3 = inv.rest_v3 + saved_failures = inv.discharge_target_enable_failures + saved_unsupported = inv.discharge_target_enable_unsupported + saved_ever_succeeded = inv.discharge_target_enable_ever_succeeded + + try: + inv.rest_api = "dummy" + inv.rest_v3 = True + inv.reserve_percent = 20 + inv.discharge_target_enable_failures = 0 + inv.discharge_target_enable_unsupported = False + inv.discharge_target_enable_ever_succeeded = False + + start_time = "03:33:00" + end_time = "04:44:00" + ts = datetime.strptime(start_time, "%H:%M:%S") + te = datetime.strptime(end_time, "%H:%M:%S") + + inv.rest_data = { + "Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"}, + "Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time}, + "raw": {"invertor": {"discharge_target_soc_1": "4"}}, + } + # Enable_Charge_Target never flips to "enable" on any readback across every simulated cycle. + dummy_rest.rest_data = copy.deepcopy(inv.rest_data) + + for cycle in range(DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES): + inv.rest_data = copy.deepcopy(inv.rest_data) + dummy_rest.clear_queue() + inv.adjust_force_export(True, ts, te) + commands = dummy_rest.get_commands() + if not any(c[0] == "dummy/enableChargeTarget" for c in commands): + print("ERROR: {}: cycle {} should still attempt enableChargeTarget (not yet given up), got {}".format(test_name, cycle, commands)) + failed = True + is_last_cycle = cycle == DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES - 1 + if inv.discharge_target_enable_unsupported and not is_last_cycle: + print("ERROR: {}: should not give up before {} failures, gave up after {}".format(test_name, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES, cycle + 1)) + failed = True + + if not inv.discharge_target_enable_unsupported: + print("ERROR: {}: should have given up after {} consecutive failures".format(test_name, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES)) + failed = True + + # One more cycle, now given up - no REST commands at all for either endpoint. + dummy_rest.clear_queue() + inv.adjust_force_export(True, ts, te) + commands = dummy_rest.get_commands() + if commands: + print("ERROR: {}: should attempt no REST commands once given up, got {}".format(test_name, commands)) + failed = True + finally: + inv.rest_data = saved_rest_data + inv.rest_api = saved_rest_api + inv.rest_v3 = saved_rest_v3 + inv.discharge_target_enable_failures = saved_failures + inv.discharge_target_enable_unsupported = saved_unsupported + inv.discharge_target_enable_ever_succeeded = saved_ever_succeeded + + return failed + + def test_discharge_target_read_prefers_control(test_name, ha, inv): """ Regression test for issue #4517: a discharge target write kept firing every cycle even when @@ -3173,6 +3384,24 @@ def run_inverter_tests(my_predbat_dummy): if failed: return failed + # Regression test for issue #4517 (follow-up): enable Enable_Charge_Target before writing a + # discharge target, mirroring adjust_battery_target()'s existing precedent for the charge side + failed |= test_discharge_target_enables_before_write("discharge_target_enables_before_write", ha, inv, dummy_rest) + if failed: + return failed + + # Regression test for issue #4517 (follow-up): if enabling itself fails, skip the discharge + # target write rather than retrying two doomed REST endpoints instead of one + failed |= test_discharge_target_skips_write_when_enable_fails("discharge_target_skips_write_when_enable_fails", ha, inv, dummy_rest) + if failed: + return failed + + # Regression test for issue #4517 (follow-up): stop attempting either endpoint after repeated + # cycle-level failures, rather than retrying enableChargeTarget forever + failed |= test_discharge_target_gives_up_after_repeated_enable_failures("discharge_target_gives_up_after_repeated_enable_failures", ha, inv, dummy_rest) + if failed: + return failed + # Regression test for issue #4517: the caller deciding whether to write at all must also prefer # Control.Discharge_Target_SOC_1 over the slow raw.invertor fallback, or it re-writes every cycle failed |= test_discharge_target_read_prefers_control("discharge_target_read_prefers_control", ha, inv)