Skip to content

fix(inverter): enable Enable_Charge_Target before writing a GivTCP discharge target #4542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chalfontchubby wants to merge 1 commit into from
Closed

fix(inverter): enable Enable_Charge_Target before writing a GivTCP discharge target #4542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion apps/predbat/inverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down
231 changes: 230 additions & 1 deletion apps/predbat/tests/test_inverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading