Merge pull request #4745 from springfall2008/fix/inverter-respect-configured-time-entity

Author: chalfontchubby

apps/predbat/inverter.py

@@ -19,6 +19,7 @@
 
 import math
 import os
+import re
 import time
 import pytz
 from datetime import datetime, timedelta
@@ -329,6 +330,40 @@ def create_missing_arg(self, arg, default):
             # then looked up as one.
             self.base.args[arg] = self.base.args[arg] + [None] * (self.id + 1 - len(self.base.args[arg]))
 
+    def user_configured_entity(self, arg):
+        """
+        The entity id the user gave this inverter for arg in apps.yaml, or None.
+
+        "Set in apps.yaml" is judged from args_from_apps_yaml, the snapshot taken before Predbat's
+        own defaulting touches args: apps.yaml must name an entity (or a re: pattern) for this
+        inverter. The entity itself is read from the live args, which by now has any re: pattern
+        resolved, and is never Predbat's own dummy for this slot - so a dummy written on an earlier
+        refresh never counts, even behind a pattern that matched nothing. A bare placeholder such as
+        '00:00:00', an unmatched pattern, or a malformed value gives None.
+        """
+        raw = (getattr(self.base, "args_from_apps_yaml", None) or {}).get(arg)
+        if isinstance(raw, list):
+            # Per inverter: a list naming inverter 0 only says nothing about inverter 1, whose live
+            # slot may by now hold the dummy an earlier refresh wrote there
+            raw = raw[self.id] if self.id < len(raw) else None
+        elif self.id > 0:
+            # A single value is inverter 0's, as create_missing_arg() has always treated it
+            raw = None
+        # Judged on what apps.yaml itself says, not just that it said something: a placeholder such as
+        # Sofar's '00:00:00' is set, but the live slot holds Predbat's own dummy from the first cycle on
+        if not (isinstance(raw, str) and (is_entity_id(raw) or raw.startswith("re:"))):
+            return None
+        value = self.base.args.get(arg)
+        if isinstance(value, list):
+            value = value[self.id] if self.id < len(value) else None
+        if not is_entity_id(value) or value.startswith("re:"):
+            return None
+        # A re: pattern that matched nothing leaves the slot empty, so from the second cycle on it holds
+        # the dummy Predbat wrote there - under whichever inverter type was current at the time
+        if re.fullmatch(r"sensor\.{}_.+_{}_{}".format(re.escape(self.base.prefix), self.id, re.escape(arg)), value):
+            return None
+        return value
+
     def __init__(self, base, id=0, quiet=False):
         """
         Inverter class
@@ -771,11 +806,19 @@ def refresh_config(self, quiet=False):
             self.base.args["inverter_mode"][self.id] = self.create_entity("inverter_mode", "Eco")
 
         if self.inv_charge_time_format != "HH:MM:SS":
+            # Some formats (H M) decompose the write into separate hour/minute entities and never
+            # expect the user to set this directly; others (no time window at all) ship a bare
+            # placeholder string. Either way Predbat needs somewhere to read/write for its own window
+            # bookkeeping. But if the user has genuinely pointed this at a real entity - a custom
+            # inverter definition using a non-HH:MM:SS format together with a real time window, e.g.
+            # #4738 - that's the one to use, not a self-created dummy that silently discards it.
             for x in ["charge", "discharge"]:
                 for y in ["start", "end"]:
                     entity_name = f"{x}_{y}_time"
+                    # Read before create_missing_arg(), which turns a single value into a list of defaults
+                    configured = self.user_configured_entity(entity_name)
                     self.create_missing_arg(entity_name, "23:59:00")
-                    self.base.args[entity_name][self.id] = self.create_entity(entity_name, "23:59:00")
+                    self.base.args[entity_name][self.id] = configured if configured else self.create_entity(entity_name, "23:59:00")
 
         # Create dummy idle time entities
         if not self.inv_has_idle_time:

apps/predbat/tests/test_inverter.py

@@ -118,6 +118,188 @@ def test_solis_energy_control(test_name, my_predbat, ha, inverter_type, switch_s
     return failed
 
 
+def test_custom_type_respects_configured_time_entity(my_predbat):
+    """
+    Regression test for issue #4738: for an inverter type whose charge_time_format is not
+    HH:MM:SS, a real time entity the user set in apps.yaml for charge/discharge start/end time must
+    be used, not silently replaced by a self-created dummy sensor.
+
+    "Set in apps.yaml" is judged from args_from_apps_yaml, the snapshot taken before Predbat's own
+    defaulting, so a dummy Predbat wrote into args on an earlier refresh never reads back as user
+    configuration. Since #5126 inverters persist and refresh_config() runs every plan cycle, so the
+    refresh path is tested on one object as well as fresh construction.
+    """
+    failed = False
+    print("Test: test_custom_type_respects_configured_time_entity")
+
+    # Restored in place: other objects hold a reference to this dict, so rebinding the attribute
+    # would leave them with the mutated one
+    saved_args = copy.deepcopy(my_predbat.args)
+    had_raw = hasattr(my_predbat, "args_from_apps_yaml")
+    saved_raw = copy.deepcopy(getattr(my_predbat, "args_from_apps_yaml", None))
+    # Each construction also writes its dummy entities into the shared HA mock
+    saved_items = dict(getattr(my_predbat.ha_interface, "dummy_items", {}))
+    test_types = ["TEST_TIME_ENTITY", "TEST_TIME_ENTITY_2"]
+    saved_defs = {name: copy.deepcopy(INVERTER_DEF.get(name)) for name in test_types}
+    time_args = ["charge_start_time", "charge_end_time", "discharge_start_time", "discharge_end_time"]
+
+    def configure(apps_yaml):
+        """Stand in for loading apps.yaml: args and its pre-defaulting snapshot both get the values."""
+        for arg in time_args:
+            my_predbat.args.pop(arg, None)
+        my_predbat.args.update(copy.deepcopy(apps_yaml))
+        my_predbat.args_from_apps_yaml = copy.deepcopy(apps_yaml)
+
+    def dummy(arg, inverter_type="TEST_TIME_ENTITY", inverter_id=0):
+        """The entity id create_entity() gives this inverter's dummy."""
+        return "sensor.{}_{}_{}_{}".format(my_predbat.prefix, inverter_type, inverter_id, arg)
+
+    def check(case, arg, expected, inverter_id=0):
+        """Report a mismatch in the live args slot."""
+        got = my_predbat.args[arg][inverter_id]
+        if got != expected:
+            print("ERROR: test_custom_type_respects_configured_time_entity: {}: {}[{}] should be {}, got {}".format(case, arg, inverter_id, expected, got))
+            return True
+        return False
+
+    try:
+        my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY"]
+        my_predbat.args["inverter"] = {"charge_time_format": "S", "has_time_window": True, "has_charge_enable_time": True, "has_discharge_enable_time": True}
+        my_predbat.args["givtcp_rest"] = None
+
+        # Case 1: real entities set in apps.yaml are kept, on construction and on every refresh
+        configure({arg: ["time.real_" + arg] for arg in time_args})
+        inv = Inverter(my_predbat, 0, quiet=True)
+        inv.refresh_config()
+        for arg in time_args:
+            failed |= check("apps.yaml list value", arg, "time.real_" + arg)
+
+        # Case 2: a single (non-list) value in apps.yaml is kept too - create_missing_arg() turns a
+        # single value into a list of defaults, so it has to be put back
+        configure({"discharge_start_time": "time.real_single"})
+        Inverter(my_predbat, 0, quiet=True)
+        failed |= check("apps.yaml single value", "discharge_start_time", "time.real_single")
+
+        # Case 3: nothing set - the dummy is created
+        configure({})
+        inv = Inverter(my_predbat, 0, quiet=True)
+        failed |= check("unset", "discharge_start_time", dummy("discharge_start_time"))
+
+        # Case 4: persisted object - refresh keeps its own dummy (it is not in apps.yaml, so it never
+        # reads back as configuration) and re-creates the state if HA has lost it
+        my_predbat.ha_interface.dummy_items.pop(dummy("discharge_start_time"), None)
+        inv.refresh_config()
+        failed |= check("refresh after HA lost the dummy", "discharge_start_time", dummy("discharge_start_time"))
+        if my_predbat.get_state_wrapper(dummy("discharge_start_time")) is None:
+            print("ERROR: test_custom_type_respects_configured_time_entity: refresh should re-create the dummy state HA lost")
+            failed = True
+
+        # Case 5: the inverter type changes on the persisted object, as it does in production (execute.py
+        # refreshes rather than rebuilds) - the old type's dummy is replaced by the new type's
+        my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY_2"]
+        inv.refresh_config()
+        failed |= check("type change on refresh", "discharge_start_time", dummy("discharge_start_time", "TEST_TIME_ENTITY_2"))
+        my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY"]
+
+        # Case 6: a bare placeholder set in apps.yaml (the shipped Sofar templates do this) is not an
+        # entity - the dummy replaces it. And it stays a dummy on refresh: apps.yaml did set the key,
+        # but not to an entity, so the dummy now in the live slot must not be trusted as configured
+        configure({"discharge_start_time": ["00:00:00"]})
+        inv = Inverter(my_predbat, 0, quiet=True)
+        failed |= check("bare placeholder", "discharge_start_time", dummy("discharge_start_time"))
+        my_predbat.ha_interface.dummy_items.pop(dummy("discharge_start_time"), None)
+        inv.refresh_config()
+        if my_predbat.get_state_wrapper(dummy("discharge_start_time")) is None:
+            print("ERROR: test_custom_type_respects_configured_time_entity: bare placeholder: the dummy was read back as configured on refresh and its lost state not re-created")
+            failed = True
+
+        # Case 7: an entity in args that apps.yaml never set is not user configuration
+        configure({})
+        my_predbat.args["discharge_start_time"] = ["time.not_from_apps_yaml"]
+        Inverter(my_predbat, 0, quiet=True)
+        failed |= check("entity not from apps.yaml", "discharge_start_time", dummy("discharge_start_time"))
+
+        # Case 8: apps.yaml names inverter 0 only. Inverter 1 gets its own dummy, and it is still
+        # treated as a dummy on refresh - its live slot then holds an entity id, but apps.yaml set
+        # nothing for it, so a lost state must be re-created rather than the slot trusted as configured
+        my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY", "TEST_TIME_ENTITY"]
+        configure({"discharge_start_time": ["time.real_inverter0"]})
+        Inverter(my_predbat, 0, quiet=True)
+        inv1 = Inverter(my_predbat, 1, quiet=True)
+        my_predbat.ha_interface.dummy_items.pop(dummy("discharge_start_time", inverter_id=1), None)
+        inv1.refresh_config()
+        failed |= check("multi-inverter, inverter 0", "discharge_start_time", "time.real_inverter0", 0)
+        failed |= check("multi-inverter, inverter 1", "discharge_start_time", dummy("discharge_start_time", inverter_id=1), 1)
+        if my_predbat.get_state_wrapper(dummy("discharge_start_time", inverter_id=1)) is None:
+            print("ERROR: test_custom_type_respects_configured_time_entity: multi-inverter: inverter 1's dummy was read back as configured and its lost state not re-created")
+            failed = True
+
+        # Case 8b: a single value in apps.yaml is inverter 0's - inverter 1 gets its own dummy and
+        # keeps treating it as one on refresh
+        configure({"discharge_start_time": "time.real_single"})
+        Inverter(my_predbat, 0, quiet=True)
+        inv1 = Inverter(my_predbat, 1, quiet=True)
+        my_predbat.ha_interface.dummy_items.pop(dummy("discharge_start_time", inverter_id=1), None)
+        inv1.refresh_config()
+        failed |= check("single value, inverter 0", "discharge_start_time", "time.real_single", 0)
+        failed |= check("single value, inverter 1", "discharge_start_time", dummy("discharge_start_time", inverter_id=1), 1)
+        if my_predbat.get_state_wrapper(dummy("discharge_start_time", inverter_id=1)) is None:
+            print("ERROR: test_custom_type_respects_configured_time_entity: single value: inverter 1's dummy was read back as configured and its lost state not re-created")
+            failed = True
+        my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY"]
+
+        # Case 9: a malformed value (a nested list) must not crash the refresh; the dummy is used
+        configure({"discharge_start_time": [["time.nested"]]})
+        Inverter(my_predbat, 0, quiet=True)
+        failed |= check("malformed value", "discharge_start_time", dummy("discharge_start_time"))
+
+        # Case 10: a re: pattern that matched nothing. auto_config() deletes a single unmatched key and
+        # leaves None in an unmatched list slot, so the first cycle writes the dummy there - and from
+        # then on the pattern must not make that dummy read back as the user's entity
+        for shape, apps_yaml_value, live_value in (("single", "re:(time\\.nothing_.*)", None), ("list", ["re:(time\\.nothing_.*)"], [None])):
+            configure({})
+            my_predbat.args_from_apps_yaml = {"discharge_start_time": apps_yaml_value}
+            if live_value is not None:
+                my_predbat.args["discharge_start_time"] = live_value
+            inv = Inverter(my_predbat, 0, quiet=True)
+            failed |= check("unmatched re: ({})".format(shape), "discharge_start_time", dummy("discharge_start_time"))
+            my_predbat.ha_interface.dummy_items.pop(dummy("discharge_start_time"), None)
+            inv.refresh_config()
+            if my_predbat.get_state_wrapper(dummy("discharge_start_time")) is None:
+                print("ERROR: test_custom_type_respects_configured_time_entity: unmatched re: ({}): the dummy was read back as configured and its lost state not re-created".format(shape))
+                failed = True
+            my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY_2"]
+            inv.refresh_config()
+            failed |= check("unmatched re: ({}), type change".format(shape), "discharge_start_time", dummy("discharge_start_time", "TEST_TIME_ENTITY_2"))
+            my_predbat.args["inverter_type"] = ["TEST_TIME_ENTITY"]
+
+        # Case 11: a re: pattern that did match is the user's entity, and stays so across refreshes
+        configure({})
+        my_predbat.args_from_apps_yaml = {"discharge_start_time": ["re:(time\\.matched_.*)"]}
+        my_predbat.args["discharge_start_time"] = ["time.matched_x"]
+        inv = Inverter(my_predbat, 0, quiet=True)
+        inv.refresh_config()
+        failed |= check("matched re:", "discharge_start_time", "time.matched_x")
+    finally:
+        my_predbat.args.clear()
+        my_predbat.args.update(saved_args)
+        if had_raw:
+            my_predbat.args_from_apps_yaml = saved_raw
+        elif hasattr(my_predbat, "args_from_apps_yaml"):
+            del my_predbat.args_from_apps_yaml
+        items = getattr(my_predbat.ha_interface, "dummy_items", None)
+        if items is not None:
+            items.clear()
+            items.update(saved_items)
+        for name, saved_def in saved_defs.items():
+            if saved_def is None:
+                INVERTER_DEF.pop(name, None)
+            else:
+                INVERTER_DEF[name] = saved_def
+
+    return failed
+
+
 def test_support_feedin_first_is_opt_in():
     """
     support_feedin_first says the inverter's Freeze Export really is a "Feed-in First" mode (load,
@@ -4950,6 +5132,7 @@ def run_inverter_tests(my_predbat_dummy):
     failed |= test_foxess_support_discharge_freeze_matches_foxcloud()
     failed |= test_support_feedin_first_is_opt_in()
     failed |= test_has_solis_energy_control_is_opt_in()
+    failed |= test_custom_type_respects_configured_time_entity(my_predbat)
     ha = my_predbat.ha_interface
 
     time_now = my_predbat.now_utc.strftime("%Y-%m-%dT%H:%M:%S%z")