From fb57b1f929d31ae8d135950a2079f0d8a5eea6c2 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Wed, 19 Aug 2026 09:21:19 +0100 Subject: [PATCH 1/2] fix(sunsynk): make the settings write actually work - group, sell flag, encodings First live write test against a real inverter. Control could never have worked before this: three separate defects each blocked it. 1. The write endpoint silently discards an oversized object. Posting all 350 settings keys returned {"code":0,"msg":"Success","success":true} and changed nothing, twice, including a probe altering a single field with every original type preserved. Posting only the System Mode group made the identical change persist at once. Predbat now sends that group. This is also safer: battery, grid and generator settings are never transmitted by a schedule write, so they cannot be disturbed. 2. The per-slot Sell flag, sellTime{n}En, was missing entirely. It is the third per-slot flag in the app alongside Grid Charge and Gen Charge, and it must be 1 for a forced export slot - so export windows could never have armed. Worse, its ABSENCE from the payload made the API silently drop time{n}on too: grid charge failed to write on six consecutive attempts across every encoding tried, while the rest of each write persisted. The API validates the per-slot field set as a whole. With the flag present the three are independent, proven by setting grid charge on a slot whose sell flag is 0 and vice versa in one write. 3. Boolean fields must be the strings "true"/"false", not bare JSON booleans - the opposite of what solarsynkv3's ReplaceTRUE() implied. sellTime{n}En is the exception, taking the numeric "1"/"0" the API returns for it. Also: time{n}On (capital O) is server-derived and must never be written. It went from '0' to '65' on a write that never mentioned it, and writing '1' produced '65' as well. Verified live with a combined charge and export plan: 33/33 owned fields written and read back exactly, 0 of the 350 keys lost, nothing outside the group changed, and the inverter restored to its original settings 33/33. Co-Authored-By: Claude Opus 5 (1M context) --- apps/predbat/sunsynk.py | 31 +++- apps/predbat/sunsynk_const.py | 69 +++++++-- apps/predbat/tests/test_sunsynk_const.py | 15 +- apps/predbat/tests/test_sunsynk_control.py | 159 +++++++++++++++++++-- 4 files changed, 237 insertions(+), 37 deletions(-) diff --git a/apps/predbat/sunsynk.py b/apps/predbat/sunsynk.py index c088a122d..93df70624 100644 --- a/apps/predbat/sunsynk.py +++ b/apps/predbat/sunsynk.py @@ -11,7 +11,8 @@ Registers each discovered Sunsynk inverter as a ``SunsynkCloud`` Predbat inverter, publishing monitoring sensors and DEYE-style schedule control entities. Predbat drives those entities through the generic Inverter class; this module derives the Sunsynk work -mode internally and applies it by read-modify-write of the whole settings object. +mode internally and applies it by read-modify-write of the System Mode settings group +(the write endpoint silently discards anything larger - see SUNSYNK_SYSTEM_MODE_FIELDS). Three auth methods: ``password`` (RSA-encrypted login, the default), ``password_legacy`` (the pre-2025 plaintext login, opt-in) and ``oauth`` (token injected by Predbat.com). @@ -57,6 +58,8 @@ SUNSYNK_SOLAR_SELL_FIELD, SUNSYNK_TOU_ENABLE_FIELD, SUNSYNK_SERIAL_FIELD, + SUNSYNK_SYSTEM_MODE_FIELDS, + SUNSYNK_DERIVED_SLOT_FIELDS, SUNSYNK_DAY_FIELDS, TOU_FIELD, TOU_SLOT_COUNT, @@ -527,7 +530,11 @@ async def fetch_device_data(self, sn): return values async def fetch_settings(self, sn): - """Read the whole settings object, which is both config and the write baseline.""" + """Read the whole settings object: config, plus the baseline the write group is built from. + + The read returns everything (350 keys on a real inverter); only the System Mode subset + of it is ever posted back. See SUNSYNK_SYSTEM_MODE_FIELDS. + """ data = await self._get("settings_read", sn=sn) if data: self.device_settings[sn] = data @@ -702,11 +709,11 @@ def _self_use_slot(self, start_time, reserve, self_use_power): the battery serving the house for the whole interval and push the load onto the grid. Self-use slots cover most of the day, so this is the default state. """ - return {"time": start_time, "power": int(self_use_power), "soc": int(reserve), "grid_charge": False} + return {"time": start_time, "power": int(self_use_power), "soc": int(reserve), "grid_charge": False, "sell": False} def _action_slot(self, start_time, state): """Build a slot realising a derived control state.""" - return {"time": start_time, "power": int(state["power"]), "soc": int(state["slot_soc"]), "grid_charge": bool(state["grid_charge"])} + return {"time": start_time, "power": int(state["power"]), "soc": int(state["slot_soc"]), "grid_charge": bool(state["grid_charge"]), "sell": bool(state.get("solar_sell"))} def build_tou_slots(self, schedule, current_soc, self_use_power): """Build exactly TOU_SLOT_COUNT ordered slots covering 24h from the schedule windows. @@ -848,13 +855,19 @@ def _owned_payload(self, sn, schedule, current_soc, now_minutes): payload[TOU_FIELD["power"].format(n=index)] = encode_setting(TOU_FIELD["power"].format(n=index), slot["power"]) payload[TOU_FIELD["soc"].format(n=index)] = encode_setting(TOU_FIELD["soc"].format(n=index), slot["soc"]) payload[TOU_FIELD["grid_charge"].format(n=index)] = encode_setting(TOU_FIELD["grid_charge"].format(n=index), slot["grid_charge"]) + # The per-slot Sell flag ("Sell" in the app). It MUST be 1 for a forced export + # slot, and every per-slot flag must be present in the payload or the API + # silently discards them all - see TOU_FIELD. + payload[TOU_FIELD["sell"].format(n=index)] = "1" if slot["sell"] else "0" return payload def build_settings_payload(self, sn, schedule, current_soc, now_minutes=None): """Build the full settings object to POST for one inverter. - Read-modify-write: start from the last-read settings so every field Predbat does - not own survives verbatim, then overwrite only the slots, mode and flags it does. + Read-modify-write over the System Mode group only: start from the last-read values + for those fields so the ones Predbat does not own survive verbatim, then overwrite the + slots, mode and flags it does. Fields outside the group are never sent - the endpoint + discards an oversized object entirely. Returns {} when self.device_settings holds no baseline for sn - a payload built from an empty baseline would contain only the owned keys, and posting it would drop every installer setting Predbat does not own. This is a public producer, not @@ -867,7 +880,11 @@ def build_settings_payload(self, sn, schedule, current_soc, now_minutes=None): return {} if now_minutes is None: now_minutes = self._now_minutes() - payload = dict(baseline) + # Only the System Mode group is sent. The endpoint accepts a larger object and then + # silently discards the whole write - see SUNSYNK_SYSTEM_MODE_FIELDS - so restricting + # this is what makes the write land at all. It also means a schedule write can never + # disturb the battery, grid or generator settings: they are simply never transmitted. + payload = {key: value for key, value in baseline.items() if key in SUNSYNK_SYSTEM_MODE_FIELDS and key not in SUNSYNK_DERIVED_SLOT_FIELDS} payload.update(self._owned_payload(sn, schedule, current_soc, now_minutes)) return payload diff --git a/apps/predbat/sunsynk_const.py b/apps/predbat/sunsynk_const.py index 172d8c81c..bba4879f1 100644 --- a/apps/predbat/sunsynk_const.py +++ b/apps/predbat/sunsynk_const.py @@ -94,25 +94,75 @@ } # Per-slot field name templates, rendered with n = 1..TOU_SLOT_COUNT. +# Per-slot fields Predbat writes. CONFIRMED live (2026-08-19) that all of "sell", +# "grid_charge" and their siblings must be present in the payload together: with +# sellTime{n}En absent, time{n}on was silently discarded on six consecutive writes across +# every encoding tried, while every other field of the same write persisted. Including it +# made time{n}on stick immediately. The API evidently validates the per-slot field set as a +# whole and drops the flags if it is incomplete. +# +# The three flags are INDEPENDENT once all are present - proven by setting grid charge on a +# slot whose sell flag is 0, and the sell flag on a slot whose grid charge is 0, in one +# write: both landed exactly as sent. TOU_FIELD = { "time": "sellTime{n}", "power": "sellTime{n}Pac", "soc": "cap{n}", "grid_charge": "time{n}on", + "sell": "sellTime{n}En", } +# NEVER write this. time{n}On (capital O) is server-derived: it changed from '0' to '65' on +# a write that did not mention it at all, and writing '1' to it also produced '65'. It is +# not the boolean it resembles, and the writable grid-charge flag is time{n}on (lower case). +SUNSYNK_DERIVED_SLOT_FIELDS = tuple(f"time{n}On" for n in range(1, TOU_SLOT_COUNT + 1)) + SUNSYNK_DAY_FIELDS = ["mondayOn", "tuesdayOn", "wednesdayOn", "thursdayOn", "fridayOn", "saturdayOn", "sundayOn"] +# The settings/set endpoint accepts ONLY the "System Mode" group of fields. CONFIRMED +# live (inverter 2405116013, 2026-08-19): posting the full 350-key object returned +# {"code":0,"msg":"Success","success":true} and changed NOTHING, twice, including a probe +# that altered a single field and preserved every original string type. Posting just these +# 53 keys with the same single change persisted immediately. +# +# So the whole-object read-modify-write this component originally used could never have +# worked - every write was silently accepted and discarded. Predbat now sends this group +# only, carrying through the fields inside it that it does not own (safetyType, battMode, +# energyMode, zeroExportPower, solarMaxSellPower, pvMaxLimit, sellTime{n}Volt, +# genTime{n}on). Everything outside the group - battery, grid, generator settings - is +# never transmitted at all, so it cannot be disturbed. +# +# Field list taken from solarsynkv3's DetermineSettingCategory, which posts the same group. +SUNSYNK_SYSTEM_MODE_FIELDS = ( + ["sn", "safetyType", "battMode", "solarSell", "pvMaxLimit", "energyMode", "peakAndVallery", "sysWorkMode", "zeroExportPower", "solarMaxSellPower"] + + [f"sellTime{n}" for n in range(1, TOU_SLOT_COUNT + 1)] + + [f"sellTime{n}Pac" for n in range(1, TOU_SLOT_COUNT + 1)] + + [f"sellTime{n}Volt" for n in range(1, TOU_SLOT_COUNT + 1)] + + [f"sellTime{n}En" for n in range(1, TOU_SLOT_COUNT + 1)] + + [f"cap{n}" for n in range(1, TOU_SLOT_COUNT + 1)] + + ["mondayOn", "tuesdayOn", "wednesdayOn", "thursdayOn", "fridayOn", "saturdayOn", "sundayOn"] + + [f"time{n}on" for n in range(1, TOU_SLOT_COUNT + 1)] + + [f"genTime{n}on" for n in range(1, TOU_SLOT_COUNT + 1)] +) + # Top-level settings keys Predbat owns. SUNSYNK_WORKMODE_FIELD = "sysWorkMode" SUNSYNK_SOLAR_SELL_FIELD = "solarSell" SUNSYNK_TOU_ENABLE_FIELD = "peakAndVallery" SUNSYNK_SERIAL_FIELD = "sn" -# VERIFY@SPIKE — solarsynkv3 carries a ReplaceTRUE() helper that rewrites the string -# "true" to a bare true before posting, which is strong evidence the API needs real -# JSON booleans for the per-slot and day flags while numeric fields stay quoted -# strings. Declared per field here rather than guessed at each call site. +# CONFIRMED live (inverter 2405116013, 2026-08-19) that these flags must be sent as the +# STRINGS "true"/"false", not as bare JSON booleans. +# +# A write carrying bare booleans was accepted and every other field of it persisted - slot +# times, target SoCs and powers all landed - while time1on and time2on alone were silently +# discarded and read back at their previous values. Sending them quoted, exactly as the read +# returns them, makes them stick. +# +# This is the opposite of what solarsynkv3's ReplaceTRUE() helper implied. That was the +# original basis for guessing bare booleans, and it was wrong. +# sellTime{n}En is deliberately NOT here: it is written as the numeric string "1"/"0", +# which is how the API returns it, unlike time{n}on which uses "true"/"false". SUNSYNK_BOOL_FIELDS = frozenset([TOU_FIELD["grid_charge"].format(n=n) for n in range(1, TOU_SLOT_COUNT + 1)] + SUNSYNK_DAY_FIELDS) # Values that mean False when Sunsynk hands a flag back as a string. @@ -122,13 +172,14 @@ def encode_setting(name, value): """Serialise one settings value the way Sunsynk expects it on the wire. - Boolean fields (per-slot grid charge, day-of-week enables) go bare; every other - field is quoted, because Sunsynk returns and accepts its numerics as strings. + Everything is quoted. The boolean fields (per-slot grid charge, day-of-week enables) + become the strings "true"/"false" rather than bare JSON booleans: the API silently + discards a bare boolean while accepting the rest of the same write. See + SUNSYNK_BOOL_FIELDS. """ if name in SUNSYNK_BOOL_FIELDS: - if isinstance(value, str): - return value.strip().lower() not in SUNSYNK_FALSE_STRINGS - return bool(value) + truthy = value.strip().lower() not in SUNSYNK_FALSE_STRINGS if isinstance(value, str) else bool(value) + return "true" if truthy else "false" return str(value) diff --git a/apps/predbat/tests/test_sunsynk_const.py b/apps/predbat/tests/test_sunsynk_const.py index f03d54d59..71383330b 100644 --- a/apps/predbat/tests/test_sunsynk_const.py +++ b/apps/predbat/tests/test_sunsynk_const.py @@ -152,14 +152,15 @@ def test_encode_setting_types(): if day not in SUNSYNK_BOOL_FIELDS: print(f"ERROR: day field {day} must be declared a boolean field") failed = True + # Booleans are sent QUOTED - a bare JSON boolean is silently discarded by the API. cases = [ - ("time1on", True, True), - ("time1on", "true", True), - ("time1on", 1, True), - ("time1on", False, False), - ("time1on", "false", False), - ("time1on", 0, False), - ("mondayOn", True, True), + ("time1on", True, "true"), + ("time1on", "true", "true"), + ("time1on", 1, "true"), + ("time1on", False, "false"), + ("time1on", "false", "false"), + ("time1on", 0, "false"), + ("mondayOn", True, "true"), ("cap1", 95, "95"), ("cap1", "95", "95"), ("sellTime1", "02:00", "02:00"), diff --git a/apps/predbat/tests/test_sunsynk_control.py b/apps/predbat/tests/test_sunsynk_control.py index 619b923e6..d25c737c3 100644 --- a/apps/predbat/tests/test_sunsynk_control.py +++ b/apps/predbat/tests/test_sunsynk_control.py @@ -10,6 +10,7 @@ from unittest.mock import patch from sunsynk_const import ( + SUNSYNK_SYSTEM_MODE_FIELDS, SUNSYNK_WORKMODE, SUNSYNK_WORKMODE_FIELD, SUNSYNK_SOLAR_SELL_FIELD, @@ -216,15 +217,15 @@ def test_payload_renders_indexed_fields_and_types(): continue value = payload[name] if concept == "grid_charge": - if not isinstance(value, bool): - print(f"ERROR: {name} = {value!r} should be a bare JSON boolean") + if value not in ("true", "false"): + print(f"ERROR: {name} = {value!r} should be the string 'true' or 'false' - a bare boolean is discarded by the API") failed = True elif not isinstance(value, str): print(f"ERROR: {name} = {value!r} should be a string") failed = True for day in SUNSYNK_DAY_FIELDS: - if payload.get(day) is not True: - print(f"ERROR: {day} should be True, got {payload.get(day)!r}") + if payload.get(day) != "true": + print(f"ERROR: {day} should be the string 'true', got {payload.get(day)!r}") failed = True if payload.get(SUNSYNK_TOU_ENABLE_FIELD) != "1": print(f"ERROR: TOU master enable should be '1', got {payload.get(SUNSYNK_TOU_ENABLE_FIELD)!r}") @@ -239,26 +240,42 @@ def test_payload_renders_indexed_fields_and_types(): def test_payload_preserves_unowned_settings(): - """Read-modify-write leaves every field Predbat does not own exactly as it was.""" + """Fields Predbat does not own survive - in-group verbatim, out-of-group by never being sent. + + Read-modify-write only covers the System Mode group, because the endpoint discards a + larger object outright. So an installer setting is protected two different ways + depending on which side of that boundary it sits, and both are worth pinning: a + zeroExportPower must come back unchanged, while a batteryShutdownCap must be absent + from the payload entirely rather than echoed. + """ failed = False s = MockSunsynk() s.device_rated_power["INV1"] = 8000.0 s.device_settings["INV1"] = { "sn": "INV1", - "batteryShutdownCap": "5", - "batteryLowCap": "10", + # inside the System Mode group - must be carried through untouched "safetyType": "3", + "battMode": "1", + "energyMode": "1", "zeroExportPower": "20", "solarMaxSellPower": "8000", - "genTime1on": False, + "pvMaxLimit": "7000", "sellTime1Volt": "49.0", + "genTime1on": "false", + # outside it - must never be transmitted by a schedule write + "batteryShutdownCap": "5", + "batteryLowCap": "10", "batteryMaxCurrentCharge": "100", } schedule = _schedule(reserve=10, charge={"enable": True, "soc": 95, "power": 3000, "start": "02:00:00", "end": "05:00:00"}) payload = s.build_settings_payload("INV1", schedule, current_soc=40, now_minutes=3 * 60) - for key, expect in (("batteryShutdownCap", "5"), ("safetyType", "3"), ("zeroExportPower", "20"), ("solarMaxSellPower", "8000"), ("batteryMaxCurrentCharge", "100"), ("genTime1on", False), ("sellTime1Volt", "49.0")): - if payload.get(key) != expect: - print(f"ERROR: unowned field {key} became {payload.get(key)!r}, expected {expect!r}") + for key, expect in (("safetyType", "3"), ("battMode", "1"), ("energyMode", "1"), ("zeroExportPower", "20"), ("solarMaxSellPower", "8000"), ("pvMaxLimit", "7000"), ("sellTime1Volt", "49.0"), ("genTime1on", "false")): + if str(payload.get(key)) != expect: + print(f"ERROR: in-group unowned field {key} became {payload.get(key)!r}, expected {expect!r}") + failed = True + for key in ("batteryShutdownCap", "batteryLowCap", "batteryMaxCurrentCharge"): + if key in payload: + print(f"ERROR: {key} is outside the System Mode group and must not be sent, got {payload[key]!r}") failed = True assert not failed, "test_payload_preserves_unowned_settings" @@ -886,12 +903,13 @@ def test_self_use_slots_are_never_zero_power(): payload = s.build_settings_payload("INV1", sched, current_soc=38, now_minutes=22 * 60) for n in range(1, TOU_SLOT_COUNT + 1): power = int(payload[TOU_FIELD["power"].format(n=n)]) - charging = payload[TOU_FIELD["grid_charge"].format(n=n)] + # The encoded flag is a STRING: "false" is truthy in Python, so compare it. + charging = payload[TOU_FIELD["grid_charge"].format(n=n)] == "true" if not charging and power <= 0: print(f"ERROR: self-use slot {n} at {payload[TOU_FIELD['time'].format(n=n)]} has power {power} - that freezes the battery") failed = True # The charge slot carries the requested charge power, not the self-use power. - charge_slots = [n for n in range(1, TOU_SLOT_COUNT + 1) if payload[TOU_FIELD["grid_charge"].format(n=n)]] + charge_slots = [n for n in range(1, TOU_SLOT_COUNT + 1) if payload[TOU_FIELD["grid_charge"].format(n=n)] == "true"] if not charge_slots or int(payload[TOU_FIELD["power"].format(n=charge_slots[0])]) != 3000: print(f"ERROR: charge slot should carry 3000W, got {[payload[TOU_FIELD['power'].format(n=n)] for n in charge_slots]}") failed = True @@ -949,7 +967,7 @@ def test_hold_charge_keeps_predbat_rate_without_charging(): failed = True else: n = hold[0] - if payload[TOU_FIELD["grid_charge"].format(n=n)]: + if payload[TOU_FIELD["grid_charge"].format(n=n)] == "true": print("ERROR: the hold-charge slot enabled grid charge") failed = True if int(payload[TOU_FIELD["power"].format(n=n)]) != 3000: @@ -958,6 +976,116 @@ def test_hold_charge_keeps_predbat_rate_without_charging(): assert not failed, "test_hold_charge_keeps_predbat_rate_without_charging" +def test_payload_is_limited_to_the_system_mode_group(): + """Only the System Mode group may be posted; a larger object is silently discarded. + + Confirmed live: posting the full 350-key settings object returned success and changed + nothing, twice, while the same change sent as the 53-key group persisted immediately. + Fields inside the group that Predbat does not own must still be carried through, and + fields outside it must never be sent at all. + """ + failed = False + s = MockSunsynk() + s.device_rated_power["INV1"] = 8000.0 + # A realistic blob: System Mode fields Predbat does not own, plus unrelated groups. + s.device_settings["INV1"] = { + "sn": "INV1", + "batteryLowCap": "20", + "sysWorkMode": "2", + "solarSell": "1", + "peakAndVallery": "1", + "safetyType": "3", + "battMode": "1", + "energyMode": "1", + "zeroExportPower": "0", + "solarMaxSellPower": "9200", + "pvMaxLimit": "7000", + "sellTime1Volt": "49.0", + "genTime1on": "false", + # outside the group - battery and grid settings that a schedule write must not touch + "batteryShutdownCap": "5", + "batteryMaxCurrentCharge": "100", + "importPower": "10350", + "acOutputPowerLimit": "14464", + "someUnknownKnob": "42", + } + sched = _schedule(reserve=20, charge={"enable": True, "soc": 90, "power": 3000, "start": "03:00:00", "end": "04:00:00"}) + payload = s.build_settings_payload("INV1", sched, current_soc=40, now_minutes=3 * 60 + 30) + outside = [k for k in payload if k not in SUNSYNK_SYSTEM_MODE_FIELDS] + if outside: + print(f"ERROR: payload carries fields outside the System Mode group: {sorted(outside)}") + failed = True + for k in ("batteryShutdownCap", "batteryMaxCurrentCharge", "importPower", "acOutputPowerLimit", "someUnknownKnob"): + if k in payload: + print(f"ERROR: {k} must never be sent by a schedule write") + failed = True + # Unowned fields INSIDE the group must survive verbatim. + for k, expect in (("safetyType", "3"), ("battMode", "1"), ("energyMode", "1"), ("zeroExportPower", "0"), ("solarMaxSellPower", "9200"), ("pvMaxLimit", "7000"), ("sellTime1Volt", "49.0"), ("genTime1on", "false")): + if str(payload.get(k)) != expect: + print(f"ERROR: in-group unowned field {k} became {payload.get(k)!r}, expected {expect!r}") + failed = True + assert not failed, "test_payload_is_limited_to_the_system_mode_group" + + +def test_export_slots_set_the_sell_flag(): + """A forced export slot must set sellTime{n}En, and it must be present on every slot. + + CONFIRMED live: with sellTime{n}En absent from the payload the API silently discarded + time{n}on on six consecutive writes across every encoding tried, while the rest of the + same write persisted. It validates the per-slot field set as a whole. The flags are + independent once all are present. + """ + failed = False + s = MockSunsynk() + s.device_rated_power["INV1"] = 8000.0 + s.device_settings["INV1"] = {"sn": "INV1", "batteryLowCap": "20"} + sched = _schedule(reserve=20, export={"enable": True, "soc": 20, "power": 2500, "start": "16:00:00", "end": "19:00:00"}) + payload = s.build_settings_payload("INV1", sched, current_soc=80, now_minutes=17 * 60) + # Present on every slot, or the API drops the flags. + for n in range(1, TOU_SLOT_COUNT + 1): + name = TOU_FIELD["sell"].format(n=n) + if name not in payload: + print(f"ERROR: {name} missing - the API needs the full per-slot set or it drops the flags") + failed = True + elif payload[name] not in ("0", "1"): + print(f"ERROR: {name} = {payload[name]!r}, expected the numeric string '0' or '1'") + failed = True + # The export slot sells; the self-use slots do not. + selling = [n for n in range(1, TOU_SLOT_COUNT + 1) if payload.get(TOU_FIELD["sell"].format(n=n)) == "1"] + if not selling: + print("ERROR: an export window produced no slot with the Sell flag set") + failed = True + for n in selling: + if payload[TOU_FIELD["time"].format(n=n)] != "16:00": + print(f"ERROR: Sell set on slot {n} at {payload[TOU_FIELD['time'].format(n=n)]}, expected the 16:00 export slot") + failed = True + # A charge-only schedule must not set Sell anywhere. + charge_sched = _schedule(reserve=20, charge={"enable": True, "soc": 90, "power": 3000, "start": "03:00:00", "end": "04:00:00"}) + charge_payload = s.build_settings_payload("INV1", charge_sched, current_soc=40, now_minutes=3 * 60 + 30) + if any(charge_payload.get(TOU_FIELD["sell"].format(n=n)) == "1" for n in range(1, TOU_SLOT_COUNT + 1)): + print("ERROR: a charge-only schedule set the Sell flag") + failed = True + assert not failed, "test_export_slots_set_the_sell_flag" + + +def test_server_derived_slot_fields_are_never_written(): + """time{n}On (capital O) is server-derived and must never be transmitted. + + It changed from '0' to '65' on a write that did not mention it, and writing '1' to it + also produced '65'. It is not the boolean it resembles. + """ + failed = False + s = MockSunsynk() + s.device_rated_power["INV1"] = 8000.0 + s.device_settings["INV1"] = {"sn": "INV1", "batteryLowCap": "20", **{f"time{n}On": "0" for n in range(1, TOU_SLOT_COUNT + 1)}} + payload = s.build_settings_payload("INV1", _schedule(reserve=20), current_soc=50, now_minutes=12 * 60) + leaked = [f"time{n}On" for n in range(1, TOU_SLOT_COUNT + 1) if f"time{n}On" in payload] + if leaked: + print(f"ERROR: server-derived fields transmitted: {leaked}") + failed = True + assert not failed, "test_server_derived_slot_fields_are_never_written" + + def run_sunsynk_control_tests(my_predbat): """Run all Sunsynk control-logic tests.""" failed = False @@ -976,6 +1104,9 @@ def run_sunsynk_control_tests(my_predbat): ("payload_field_types", test_payload_renders_indexed_fields_and_types), ("solar_export_never_off", test_solar_export_is_never_disabled), ("payload_preserves", test_payload_preserves_unowned_settings), + ("payload_system_mode_only", test_payload_is_limited_to_the_system_mode_group), + ("export_sell_flag", test_export_slots_set_the_sell_flag), + ("no_derived_fields", test_server_derived_slot_fields_are_never_written), ("payload_soc_floor", test_payload_clamps_to_the_inverter_soc_floor), ("payload_no_baseline_is_empty", test_build_settings_payload_returns_empty_without_a_baseline), ("payloads_equal", test_payloads_equal_ignores_nothing_material), From 8dd92f494873708bacf2d858ae64feec8033c8c3 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Wed, 19 Aug 2026 18:50:22 +0100 Subject: [PATCH 2/2] refactor(sunsynk): address PR review - centralise sell encoding, frozenset the group Both from Copilot's review of #4589. The per-slot Sell flag was encoded in-line rather than through encode_setting(), duplicating wire-encoding logic that every other owned field routes through one place. It now goes through it, passed as 1/0 rather than a bool: sellTime{n}En is deliberately not in SUNSYNK_BOOL_FIELDS, because the API returns it as "1"/"0" unlike time{n}on's "true"/"false", so it falls through to str() - and str(True) would be "True". SUNSYNK_SYSTEM_MODE_FIELDS is only ever used for membership tests, so it becomes a frozenset: O(1) lookups against a ~350-key baseline instead of a scan, and it cannot be mutated by accident. Payload output is unchanged - verified the emitted sellTime{n}En values and types are identical before and after. Co-Authored-By: Claude Opus 5 (1M context) --- apps/predbat/sunsynk.py | 6 +++++- apps/predbat/sunsynk_const.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/predbat/sunsynk.py b/apps/predbat/sunsynk.py index 93df70624..b5af13cbd 100644 --- a/apps/predbat/sunsynk.py +++ b/apps/predbat/sunsynk.py @@ -858,7 +858,11 @@ def _owned_payload(self, sn, schedule, current_soc, now_minutes): # The per-slot Sell flag ("Sell" in the app). It MUST be 1 for a forced export # slot, and every per-slot flag must be present in the payload or the API # silently discards them all - see TOU_FIELD. - payload[TOU_FIELD["sell"].format(n=index)] = "1" if slot["sell"] else "0" + # Through encode_setting like every other owned field, so wire encoding stays in + # one place. Passed as 1/0 rather than a bool: sellTime{n}En is deliberately NOT + # in SUNSYNK_BOOL_FIELDS (the API returns it as "1"/"0", unlike time{n}on's + # "true"/"false"), so it falls through to str() - and str(True) would be "True". + payload[TOU_FIELD["sell"].format(n=index)] = encode_setting(TOU_FIELD["sell"].format(n=index), 1 if slot["sell"] else 0) return payload def build_settings_payload(self, sn, schedule, current_soc, now_minutes=None): diff --git a/apps/predbat/sunsynk_const.py b/apps/predbat/sunsynk_const.py index bba4879f1..7859f1ce9 100644 --- a/apps/predbat/sunsynk_const.py +++ b/apps/predbat/sunsynk_const.py @@ -133,7 +133,7 @@ # never transmitted at all, so it cannot be disturbed. # # Field list taken from solarsynkv3's DetermineSettingCategory, which posts the same group. -SUNSYNK_SYSTEM_MODE_FIELDS = ( +SUNSYNK_SYSTEM_MODE_FIELDS = frozenset( ["sn", "safetyType", "battMode", "solarSell", "pvMaxLimit", "energyMode", "peakAndVallery", "sysWorkMode", "zeroExportPower", "solarMaxSellPower"] + [f"sellTime{n}" for n in range(1, TOU_SLOT_COUNT + 1)] + [f"sellTime{n}Pac" for n in range(1, TOU_SLOT_COUNT + 1)]