From c436e94a4894913452125732e7fbc93563427da6 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Sat, 1 Aug 2026 18:14:04 +0200 Subject: [PATCH] fix(inverter): stop spurious "REST failed to setExportTarget" warnings rest_setDischargeTarget verified the write with a raw string against an int - GivTCP reports the raw invertor registers as strings, so '4' == 4 was always False and the write never counted as successful. Every export slot burned all five retries (five redundant register writes) and then recorded an error status, even though the register had actually taken the value. rest_setReserve already coerces with int(float()); do the same here and report the value read back on failure. adjust_force_export also collapsed an unreadable target to 0 in both branches. That sentinel was harmless while the guard was "current > reserve_percent", but since it became "current != target_soc" an inverter that does not expose the register looks like a target parked below the reserve and gets written on every cycle. Keep it as None and skip the write instead, so the bidirectional tracking still applies to real values. Also raise INVERTER_REST_TIMEOUT from 5 to 10 seconds to give a busy GivTCP more room to respond. Fixes #4404 Co-Authored-By: Claude Opus 5 (1M context) --- apps/predbat/const.py | 2 +- apps/predbat/inverter.py | 28 +++++-- apps/predbat/tests/test_inverter.py | 126 ++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 8 deletions(-) diff --git a/apps/predbat/const.py b/apps/predbat/const.py index 1323e3bea..f49a7146b 100644 --- a/apps/predbat/const.py +++ b/apps/predbat/const.py @@ -33,7 +33,7 @@ CONFIG_REFRESH_PERIOD = 60 * 8 INVERTER_MAX_RETRY = 10 # Maximum number of retries for inverter commands INVERTER_MAX_RETRY_REST = 5 # Maximum number of retries for inverter REST commands -INVERTER_REST_TIMEOUT = 5 # Seconds to wait for a REST response before giving up (local network call, should be fast) +INVERTER_REST_TIMEOUT = 10 # Seconds to wait for a REST response before giving up (local network call, should be fast) INVERTER_QUICK_UPDATE_SECONDS = 120 # Minimum seconds between quick inverter data updates # 240v x 100 amps x 3 phases / 1000 to kW / 60 minutes in an hour is the maximum kWh in a 1 minute period diff --git a/apps/predbat/inverter.py b/apps/predbat/inverter.py index 106cb178a..9fbddf5e1 100644 --- a/apps/predbat/inverter.py +++ b/apps/predbat/inverter.py @@ -2494,6 +2494,8 @@ def adjust_force_export(self, force_export, new_start_time=None, new_end_time=No # Export target, always set to the minimum reserve. This must track the reserve in *both* # directions - a target left below the minimum reserve SoC (e.g. GE Cloud resets it to 4%) # lets the inverter drain the battery past the reserve between Predbat cycles. + # A target we can not read is left alone - an inverter that does not expose the register + # reads back as None, and writing to it every cycle just produces errors. if force_export: target_soc = int(self.reserve_percent) if self.rest_data and self.rest_v3: @@ -2502,9 +2504,11 @@ def adjust_force_export(self, force_export, new_start_time=None, new_end_time=No try: current = float(current) except (ValueError, TypeError) as e: - current = 0 + current = None - if current != target_soc: + 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) else: self.log("Inverter {} Current discharge target is already set to {}".format(self.id, current)) @@ -2513,8 +2517,10 @@ def adjust_force_export(self, force_export, new_start_time=None, new_end_time=No try: current = float(current) except (ValueError, TypeError) as e: - current = 0 - if current != target_soc: + current = None + 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.write_and_poll_value("discharge_target_soc", self.base.get_arg("discharge_target_soc", indirect=False, index=self.id, required_unit="%"), target_soc) else: self.log("Inverter {} Current discharge target is already set to {}".format(self.id, current)) @@ -3381,18 +3387,26 @@ def rest_setDischargeTarget(self, target): target = int(target) url = self.rest_api + "/setDischargeTarget" data = {"dischargeToPercent": target, "slot": 1} + result = None for retry in range(INVERTER_MAX_RETRY_REST): r = self.rest_postCommand(url, json=data) self.rest_data = self.rest_runAll(self.rest_data) - if self.rest_data["raw"]["invertor"]["discharge_target_soc_1"] == target: + # GivTCP reports the raw registers as strings, so coerce before comparing or a + # successful write reads back as '4' and never matches the int target + result = self.rest_data.get("raw", {}).get("invertor", {}).get("discharge_target_soc_1", None) + try: + result = int(float(result)) + except (ValueError, TypeError): + result = None + if result == target: self.count_register_writes += 1 self.base.log("Inverter {} Set export target slot 1 {} via REST successful after retry {}".format(self.id, data, retry)) return True self.sleep(2) - self.base.log("Warn: Inverter {} Set export target slot 1 {} via REST failed".format(self.id, data)) - self.base.record_status("Warn: Inverter {} REST failed to setExportTarget".format(self.id), had_errors=True) + self.base.log("Warn: Inverter {} Set export target slot 1 {} via REST failed got {}".format(self.id, data, result)) + self.base.record_status("Warn: Inverter {} REST failed to setExportTarget got {}".format(self.id, result), had_errors=True) return False def rest_setDischargeSlot1(self, start, finish): diff --git a/apps/predbat/tests/test_inverter.py b/apps/predbat/tests/test_inverter.py index cd6f21bad..b97fb7862 100644 --- a/apps/predbat/tests/test_inverter.py +++ b/apps/predbat/tests/test_inverter.py @@ -1632,6 +1632,127 @@ def setup_entity_case(current_target, reserve_percent): return failed +def test_discharge_target_read_back(test_name, ha, inv, dummy_rest): + """ + Regression test for issue #4404: spurious "REST failed to setExportTarget" warnings. + + GivTCP reports the raw invertor registers as strings, so rest_setDischargeTarget's read back + check compared '20' against the int 20 and never matched. Every export slot burned all five + retries (five redundant register writes) and then recorded an error status, even though the + write had actually landed. + + The same cycle also has to cope with an inverter that does not expose the export target at + all - reading it back as None must not be treated as a real target of 0 that needs raising to + the reserve, or Predbat writes to an entity that isn't there on every cycle. + """ + failed = False + print("Test: {}".format(test_name)) + + saved_reserve_percent = inv.reserve_percent + saved_rest_data = inv.rest_data + saved_rest_api = inv.rest_api + saved_rest_v3 = inv.rest_v3 + saved_had_errors = inv.base.had_errors + saved_record_status = inv.base.record_status + saved_target_soc = ha.dummy_items.get("number.discharge_target_soc") + + # Capture the error statuses so the unrelated fixture warnings (the H M format time entities + # this inverter has no entity for) don't mask what the export target write itself recorded + errors = [] + + def capture_record_status(message, debug="", had_errors=False, notify=False, extra=""): + """Record error statuses so the test can assert on the export target ones only.""" + if had_errors: + errors.append(message) + return saved_record_status(message, debug=debug, had_errors=had_errors, notify=notify, extra=extra) + + def target_errors(): + """Return the captured error statuses that relate to the export target.""" + return [message for message in errors if "ExportTarget" in message or "discharge_target_soc" in message] + + inv.base.record_status = capture_record_status + + 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") + + try: + # Case 1: GivTCP reports the register back as a string - the write must be seen as successful + inv.rest_api = "dummy" + inv.rest_v3 = True + inv.rest_data = {"Control": {"Mode": "Timed Export"}, "raw": {"invertor": {"discharge_target_soc_1": "4"}}} + dummy_rest.clear_queue() + dummy_rest.rest_data = copy.deepcopy(inv.rest_data) + polled = copy.deepcopy(inv.rest_data) + polled["raw"]["invertor"]["discharge_target_soc_1"] = "20" + dummy_rest.queue_rest_data(polled) + dummy_rest.get_commands() + del errors[:] + + if not inv.rest_setDischargeTarget(20): + print("ERROR: {}: string read back of the export target should count as success".format(test_name)) + failed = True + if target_errors(): + print("ERROR: {}: string read back of the export target should not record an error, got {}".format(test_name, target_errors())) + failed = True + commands = dummy_rest.get_commands() + if len(commands) != 1: + print("ERROR: {}: export target should be written once, got {} writes".format(test_name, len(commands))) + failed = True + + # Case 2: an export target that can not be read must not be written to, on the REST path + inv.reserve_percent = 20 + inv.rest_data = { + "Control": {"Enable_Discharge_Schedule": "on", "Mode": "Timed Export"}, + "Timeslots": {"Discharge_start_time_slot_1": start_time, "Discharge_end_time_slot_1": end_time}, + "raw": {"invertor": {"discharge_target_soc_1": None}}, + } + dummy_rest.clear_queue() + dummy_rest.rest_data = copy.deepcopy(inv.rest_data) + dummy_rest.get_commands() + del errors[:] + + inv.adjust_force_export(True, ts, te) + if [command for command in dummy_rest.get_commands() if "setDischargeTarget" in command[0]]: + print("ERROR: {}: unreadable REST export target should not be written".format(test_name)) + failed = True + if target_errors(): + print("ERROR: {}: unreadable REST export target should not record an error, got {}".format(test_name, target_errors())) + failed = True + + # Case 3: the same on the entity path, where the configured entity has no state + inv.rest_data = None + inv.rest_api = None + inv.reserve_percent = 20 + ha.dummy_items["select.discharge_start_time"] = start_time + ha.dummy_items["select.discharge_end_time"] = end_time + ha.dummy_items["switch.scheduled_discharge_enable"] = "on" + ha.dummy_items["sensor.predbat_GE_0_scheduled_discharge_enable"] = "on" + ha.dummy_items["number.discharge_target_soc"] = None + ha.dummy_items["select.inverter_mode"] = "Timed Export" + ha.dummy_items["switch.inverter_button"] = "off" + del errors[:] + + inv.adjust_force_export(True, ts, te) + if ha.get_state("number.discharge_target_soc") is not None: + print("ERROR: {}: unreadable export target entity should not be written, got {}".format(test_name, ha.get_state("number.discharge_target_soc"))) + failed = True + if target_errors(): + print("ERROR: {}: unreadable export target entity should not record an error, got {}".format(test_name, target_errors())) + failed = True + finally: + inv.reserve_percent = saved_reserve_percent + inv.rest_data = saved_rest_data + inv.rest_api = saved_rest_api + inv.rest_v3 = saved_rest_v3 + inv.base.record_status = saved_record_status + inv.base.had_errors = saved_had_errors + ha.dummy_items["number.discharge_target_soc"] = saved_target_soc + + return failed + + def test_force_export_unchanged_times_HM_format(test_name, ha, inv): """ Regression test for GS_fb00 (Solis) 'count register writes 0' bug. @@ -2757,5 +2878,10 @@ def run_inverter_tests(my_predbat_dummy): if failed: return failed + # Regression test for issue #4404: export target read back must cope with string and missing values + failed |= test_discharge_target_read_back("discharge_target_read_back", ha, inv, dummy_rest) + if failed: + return failed + failed |= test_inverter_self_test("self_test1", my_predbat) return failed