Skip to content

Commit d46178f

Browse files
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 <noreply@anthropic.com>
1 parent 7ae4318 commit d46178f

2 files changed

Lines changed: 275 additions & 2 deletions

File tree

‎apps/predbat/inverter.py‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@
2929

3030
TIME_FORMAT_HMS = "%H:%M:%S"
3131

32+
# Give up enabling/writing the discharge target register after this many consecutive cycles where
33+
# rest_enableChargeTarget() itself fails - stops a genuinely unsupported inverter retrying forever,
34+
# every cycle, indefinitely (#4517). Small enough to bound the write storm quickly, but more than 1
35+
# so a single transient GivTCP/network hiccup doesn't permanently give up on a working register.
36+
DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES = 3
37+
3238

3339
class Inverter:
3440
"""Unified inverter control abstraction for multiple brands.
@@ -186,6 +192,9 @@ def __init__(self, base, id=0, quiet=False, rest_postCommand=None, rest_getData=
186192
self.rest_v3 = False
187193
self.serial_number = "Unknown"
188194
self.count_register_writes = 0
195+
self.discharge_target_enable_failures = 0
196+
self.discharge_target_enable_unsupported = False
197+
self.discharge_target_enable_ever_succeeded = False
189198
self.created_attributes = {}
190199
self.track_charge_start = "00:00:00"
191200
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
25272536
if current is None:
25282537
self.log("Inverter {} No current discharge target to read, export target not written".format(self.id))
25292538
elif current != target_soc:
2530-
self.rest_setDischargeTarget(target_soc)
2539+
# Without Enable_Charge_Target on, the inverter ignores a written discharge
2540+
# target - GivTCP still reports the write as successful, but it never sticks, so
2541+
# this repeats every cycle indefinitely (#4517). adjust_battery_target() already
2542+
# enables this before writing a charge target; mirror that here. No-ops if
2543+
# already enabled.
2544+
if self.discharge_target_enable_unsupported:
2545+
# Already gave up after DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES consecutive
2546+
# failures below, having never once succeeded - don't keep retrying either
2547+
# endpoint forever on hardware that has demonstrated it can't do this, or the
2548+
# fix just moves the write storm from setDischargeTarget onto
2549+
# enableChargeTarget instead of reducing it.
2550+
pass
2551+
elif self.rest_enableChargeTarget(True):
2552+
self.discharge_target_enable_failures = 0
2553+
self.discharge_target_enable_ever_succeeded = True
2554+
self.rest_setDischargeTarget(target_soc)
2555+
else:
2556+
self.discharge_target_enable_failures += 1
2557+
if self.discharge_target_enable_failures >= DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES and not self.discharge_target_enable_ever_succeeded:
2558+
# Never once worked since startup - a genuine capability gap, not a
2559+
# transient blip, so stop attempting it rather than retrying forever.
2560+
self.discharge_target_enable_unsupported = True
2561+
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)
2562+
self.log(message)
2563+
self.base.record_status(message, had_errors=True)
2564+
elif self.discharge_target_enable_failures >= DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES:
2565+
# Worked before, failing now - a different problem (regression, GivTCP
2566+
# restart, transient fault) to "never supported at all", so keep retrying
2567+
# rather than writing the hardware off permanently, but flag it loudly.
2568+
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(
2569+
self.id, self.discharge_target_enable_failures
2570+
)
2571+
self.log(message)
2572+
self.base.record_status(message, had_errors=True)
2573+
else:
2574+
self.log("Inverter {} Unable to enable charge target, discharge target not written".format(self.id))
25312575
else:
25322576
self.log("Inverter {} Current discharge target is already set to {}".format(self.id, current))
25332577
elif "discharge_target_soc" in self.base.args:

‎apps/predbat/tests/test_inverter.py‎

Lines changed: 230 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
from tests.test_infra import TestHAInterface
1818
from predbat import PredBat
1919
from const import MINUTE_WATT, INVERTER_MAX_RETRY_REST
20-
from inverter import Inverter
20+
from inverter import Inverter, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES
2121

2222

2323
def dummy_sleep(seconds):
@@ -344,6 +344,10 @@ def test_adjust_force_export(test_name, ha, inv, dummy_rest, prev_start, prev_en
344344
inv.rest_data["Control"] = {}
345345
inv.rest_data["Control"]["Enable_Discharge_Schedule"] = prev_force_export
346346
inv.rest_data["Control"]["Mode"] = prev_mode
347+
# Already enabled, matching a normal working inverter - keeps this helper's assertions about the
348+
# discharge target/slot/mode writes focused on those, rather than also exercising the
349+
# enableChargeTarget guard (#4517 follow-up), which has its own dedicated test.
350+
inv.rest_data["Control"]["Enable_Charge_Target"] = "enable"
347351
inv.rest_data["Timeslots"] = {}
348352
inv.rest_data["Timeslots"]["Discharge_start_time_slot_1"] = prev_start
349353
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):
19271931
return failed
19281932

19291933

1934+
def test_discharge_target_enables_before_write(test_name, ha, inv, dummy_rest):
1935+
"""
1936+
Regression test for issue #4517 (follow-up): on some inverters (confirmed against a real
1937+
reporter's REST_Response.txt - AC-coupled, GivTCP reports Control.Enable_Charge_Target as
1938+
"disable" and Control.Discharge_Target_SOC_1 is entirely absent), a written discharge target
1939+
never sticks while Enable_Charge_Target stays off - GivTCP's own log still reports each write as
1940+
a success, but it doesn't take, so adjust_force_export sees a permanent mismatch and rewrites
1941+
every cycle indefinitely.
1942+
1943+
adjust_battery_target() already calls rest_enableChargeTarget(True) before writing a charge
1944+
target for exactly this reason (the register is documented to be ignored otherwise) - this
1945+
mirrors that same call for the discharge target write. Not confirmed to be the true root cause
1946+
on real hardware yet (correlational, from one working and one broken sample), but low-risk: it's
1947+
a no-op wherever Enable_Charge_Target is already on, which is the common case exercised by every
1948+
other discharge-target test in this file.
1949+
"""
1950+
failed = False
1951+
print("Test: {}".format(test_name))
1952+
1953+
saved_rest_data = inv.rest_data
1954+
saved_rest_api = inv.rest_api
1955+
saved_rest_v3 = inv.rest_v3
1956+
1957+
try:
1958+
inv.rest_api = "dummy"
1959+
inv.rest_v3 = True
1960+
inv.reserve_percent = 20
1961+
1962+
start_time = "03:33:00"
1963+
end_time = "04:44:00"
1964+
ts = datetime.strptime(start_time, "%H:%M:%S")
1965+
te = datetime.strptime(end_time, "%H:%M:%S")
1966+
1967+
# Mirrors a reporter's real REST_Response.txt: Enable_Charge_Target off, no
1968+
# Discharge_Target_SOC_1 key in Control at all, raw.invertor reporting a plausible but wrong
1969+
# value (so rest_readDischargeTarget's fallback doesn't return None and skip the write).
1970+
# Timeslots/Enable_Discharge_Schedule/Mode already match the new values so only the discharge
1971+
# target write itself is exercised - the slot/mode writes have their own dedicated tests.
1972+
inv.rest_data = {
1973+
"Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"},
1974+
"Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time},
1975+
"raw": {"invertor": {"discharge_target_soc_1": "4"}},
1976+
}
1977+
dummy_rest.clear_queue()
1978+
dummy_rest.rest_data = copy.deepcopy(inv.rest_data)
1979+
1980+
# First queued response: consumed by rest_enableChargeTarget's own readback, showing GivTCP
1981+
# accepted the enable.
1982+
enabled = copy.deepcopy(inv.rest_data)
1983+
enabled["Control"]["Enable_Charge_Target"] = "enable"
1984+
dummy_rest.queue_rest_data(enabled)
1985+
1986+
# Second queued response: consumed by rest_setDischargeTarget's readback, now that the
1987+
# register is enabled the write finally sticks.
1988+
settled = copy.deepcopy(enabled)
1989+
settled["Control"]["Discharge_Target_SOC_1"] = "20"
1990+
dummy_rest.queue_rest_data(settled)
1991+
1992+
inv.adjust_force_export(True, ts, te)
1993+
1994+
commands = dummy_rest.get_commands()
1995+
expect_commands = [
1996+
["dummy/enableChargeTarget", {"state": "enable"}],
1997+
["dummy/setDischargeTarget", {"dischargeToPercent": 20, "slot": 1}],
1998+
]
1999+
if json.dumps(commands[:2]) != json.dumps(expect_commands):
2000+
print("ERROR: {}: expected enableChargeTarget before setDischargeTarget, got {}".format(test_name, commands))
2001+
failed = True
2002+
finally:
2003+
inv.rest_data = saved_rest_data
2004+
inv.rest_api = saved_rest_api
2005+
inv.rest_v3 = saved_rest_v3
2006+
2007+
return failed
2008+
2009+
2010+
def test_discharge_target_skips_write_when_enable_fails(test_name, ha, inv, dummy_rest):
2011+
"""
2012+
Regression test for issue #4517 (follow-up): if Enable_Charge_Target can't be turned on at all -
2013+
e.g. a genuinely unsupported register on this inverter, not just left off - don't also attempt a
2014+
discharge-target write we already know is doomed. Without this, a genuinely unsupported inverter
2015+
would retry *two* REST endpoints every cycle (enableChargeTarget then setDischargeTarget) instead
2016+
of the one it retried before this fix, which would make the write-storm this fix exists to
2017+
reduce worse, not better, for exactly the inverters it can't help.
2018+
"""
2019+
failed = False
2020+
print("Test: {}".format(test_name))
2021+
2022+
saved_rest_data = inv.rest_data
2023+
saved_rest_api = inv.rest_api
2024+
saved_rest_v3 = inv.rest_v3
2025+
2026+
try:
2027+
inv.rest_api = "dummy"
2028+
inv.rest_v3 = True
2029+
inv.reserve_percent = 20
2030+
2031+
start_time = "03:33:00"
2032+
end_time = "04:44:00"
2033+
ts = datetime.strptime(start_time, "%H:%M:%S")
2034+
te = datetime.strptime(end_time, "%H:%M:%S")
2035+
2036+
inv.rest_data = {
2037+
"Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"},
2038+
"Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time},
2039+
"raw": {"invertor": {"discharge_target_soc_1": "4"}},
2040+
}
2041+
dummy_rest.clear_queue()
2042+
dummy_rest.rest_data = copy.deepcopy(inv.rest_data)
2043+
# Enable_Charge_Target never flips to "enable" on any readback - simulates a register the
2044+
# inverter genuinely doesn't support, so rest_enableChargeTarget exhausts its retries and
2045+
# returns False.
2046+
2047+
inv.adjust_force_export(True, ts, te)
2048+
2049+
commands = dummy_rest.get_commands()
2050+
set_discharge_target_commands = [c for c in commands if c[0] == "dummy/setDischargeTarget"]
2051+
if set_discharge_target_commands:
2052+
print("ERROR: {}: setDischargeTarget should not be attempted when enabling fails, got {}".format(test_name, commands))
2053+
failed = True
2054+
enable_charge_target_commands = [c for c in commands if c[0] == "dummy/enableChargeTarget"]
2055+
if not enable_charge_target_commands:
2056+
print("ERROR: {}: expected at least one enableChargeTarget attempt, got {}".format(test_name, commands))
2057+
failed = True
2058+
finally:
2059+
inv.rest_data = saved_rest_data
2060+
inv.rest_api = saved_rest_api
2061+
inv.rest_v3 = saved_rest_v3
2062+
2063+
return failed
2064+
2065+
2066+
def test_discharge_target_gives_up_after_repeated_enable_failures(test_name, ha, inv, dummy_rest):
2067+
"""
2068+
Regression test for issue #4517 (follow-up): skipping the discharge-target write while still
2069+
retrying enableChargeTarget every single cycle forever just relocates the write storm this fix
2070+
exists to reduce from one REST endpoint to another - it doesn't reduce it. After
2071+
DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES consecutive cycle-level failures, Predbat should stop
2072+
attempting either endpoint on hardware that has demonstrated it can't do this, converging to
2073+
zero further REST commands rather than retrying indefinitely.
2074+
"""
2075+
failed = False
2076+
print("Test: {}".format(test_name))
2077+
2078+
saved_rest_data = inv.rest_data
2079+
saved_rest_api = inv.rest_api
2080+
saved_rest_v3 = inv.rest_v3
2081+
saved_failures = inv.discharge_target_enable_failures
2082+
saved_unsupported = inv.discharge_target_enable_unsupported
2083+
saved_ever_succeeded = inv.discharge_target_enable_ever_succeeded
2084+
2085+
try:
2086+
inv.rest_api = "dummy"
2087+
inv.rest_v3 = True
2088+
inv.reserve_percent = 20
2089+
inv.discharge_target_enable_failures = 0
2090+
inv.discharge_target_enable_unsupported = False
2091+
inv.discharge_target_enable_ever_succeeded = False
2092+
2093+
start_time = "03:33:00"
2094+
end_time = "04:44:00"
2095+
ts = datetime.strptime(start_time, "%H:%M:%S")
2096+
te = datetime.strptime(end_time, "%H:%M:%S")
2097+
2098+
inv.rest_data = {
2099+
"Control": {"Mode": "Timed Export", "Enable_Discharge_Schedule": "on", "Enable_Charge_Target": "disable"},
2100+
"Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time},
2101+
"raw": {"invertor": {"discharge_target_soc_1": "4"}},
2102+
}
2103+
# Enable_Charge_Target never flips to "enable" on any readback across every simulated cycle.
2104+
dummy_rest.rest_data = copy.deepcopy(inv.rest_data)
2105+
2106+
for cycle in range(DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES):
2107+
inv.rest_data = copy.deepcopy(inv.rest_data)
2108+
dummy_rest.clear_queue()
2109+
inv.adjust_force_export(True, ts, te)
2110+
commands = dummy_rest.get_commands()
2111+
if not any(c[0] == "dummy/enableChargeTarget" for c in commands):
2112+
print("ERROR: {}: cycle {} should still attempt enableChargeTarget (not yet given up), got {}".format(test_name, cycle, commands))
2113+
failed = True
2114+
is_last_cycle = cycle == DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES - 1
2115+
if inv.discharge_target_enable_unsupported and not is_last_cycle:
2116+
print("ERROR: {}: should not give up before {} failures, gave up after {}".format(test_name, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES, cycle + 1))
2117+
failed = True
2118+
2119+
if not inv.discharge_target_enable_unsupported:
2120+
print("ERROR: {}: should have given up after {} consecutive failures".format(test_name, DISCHARGE_TARGET_ENABLE_MAX_CYCLE_FAILURES))
2121+
failed = True
2122+
2123+
# One more cycle, now given up - no REST commands at all for either endpoint.
2124+
dummy_rest.clear_queue()
2125+
inv.adjust_force_export(True, ts, te)
2126+
commands = dummy_rest.get_commands()
2127+
if commands:
2128+
print("ERROR: {}: should attempt no REST commands once given up, got {}".format(test_name, commands))
2129+
failed = True
2130+
finally:
2131+
inv.rest_data = saved_rest_data
2132+
inv.rest_api = saved_rest_api
2133+
inv.rest_v3 = saved_rest_v3
2134+
inv.discharge_target_enable_failures = saved_failures
2135+
inv.discharge_target_enable_unsupported = saved_unsupported
2136+
inv.discharge_target_enable_ever_succeeded = saved_ever_succeeded
2137+
2138+
return failed
2139+
2140+
19302141
def test_discharge_target_read_prefers_control(test_name, ha, inv):
19312142
"""
19322143
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):
31733384
if failed:
31743385
return failed
31753386

3387+
# Regression test for issue #4517 (follow-up): enable Enable_Charge_Target before writing a
3388+
# discharge target, mirroring adjust_battery_target()'s existing precedent for the charge side
3389+
failed |= test_discharge_target_enables_before_write("discharge_target_enables_before_write", ha, inv, dummy_rest)
3390+
if failed:
3391+
return failed
3392+
3393+
# Regression test for issue #4517 (follow-up): if enabling itself fails, skip the discharge
3394+
# target write rather than retrying two doomed REST endpoints instead of one
3395+
failed |= test_discharge_target_skips_write_when_enable_fails("discharge_target_skips_write_when_enable_fails", ha, inv, dummy_rest)
3396+
if failed:
3397+
return failed
3398+
3399+
# Regression test for issue #4517 (follow-up): stop attempting either endpoint after repeated
3400+
# cycle-level failures, rather than retrying enableChargeTarget forever
3401+
failed |= test_discharge_target_gives_up_after_repeated_enable_failures("discharge_target_gives_up_after_repeated_enable_failures", ha, inv, dummy_rest)
3402+
if failed:
3403+
return failed
3404+
31763405
# Regression test for issue #4517: the caller deciding whether to write at all must also prefer
31773406
# Control.Discharge_Target_SOC_1 over the slow raw.invertor fallback, or it re-writes every cycle
31783407
failed |= test_discharge_target_read_prefers_control("discharge_target_read_prefers_control", ha, inv)

0 commit comments

Comments
 (0)