fix(inverter): don't overwrite a genuinely configured time entity with a dummy sensor

apps/predbat/inverter.py

@@ -301,6 +301,36 @@ 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 is_real_entity_configured(self, arg):
+        """
+        True if arg already resolves to a real HA entity id for this inverter (contains a domain,
+        e.g. 'time.foo'), as opposed to being unset, a bare placeholder value such as '23:59:00' or
+        '00:00:00' with no domain, or a dummy Predbat created for itself on an earlier
+        construction. Used to tell "this was configured deliberately" apart from "nothing real is
+        there yet" before create_missing_arg's own value-vs-list check would conflate the two - a
+        config item that's present but not a real entity is exactly the case a dummy entity still
+        needs to be created for.
+
+        "Deliberately" covers a component's automatic_config() as well as apps.yaml: fox.py and
+        gecloud.py both set_arg() real select.* time entities they discovered, which never appear
+        in args_from_apps_yaml, so presence in apps.yaml alone is not the question being asked.
+
+        The dummies are recognised from the registry create_entity() records them in rather than by
+        re-deriving their id format here: self.base.args lives for the whole process and is never
+        re-read from apps.yaml, and inverters are rebuilt from scratch on the balance path
+        (execute.py) as well as at startup, so from the second construction onwards args already
+        holds the dummy - which has a domain and would otherwise read back as genuine
+        configuration, skipping the recreation that re-registers created_attributes and restores
+        the state if it has gone away (#4745 review).
+        """
+        values = self.base.args.get(arg)
+        if not isinstance(values, list) or self.id >= len(values):
+            return False
+        value = values[self.id]
+        if value in getattr(self.base, "predbat_created_entities", ()):
+            return False
+        return is_entity_id(value)
+
     def __init__(self, base, id=0, quiet=False):
         """
         Inverter class
@@ -625,11 +655,18 @@ def __init__(self, base, id=0, quiet=False):
             self.base.args["inverter_mode"][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"
                     self.create_missing_arg(entity_name, "23:59:00")
-                    self.base.args[entity_name][id] = self.create_entity(entity_name, "23:59:00")
+                    if not self.is_real_entity_configured(entity_name):
+                        self.base.args[entity_name][id] = self.create_entity(entity_name, "23:59:00")
 
         # Create dummy idle time entities
         if not self.inv_has_idle_time:
@@ -1418,6 +1455,12 @@ def create_entity(self, entity_name, value, uom=None, device_class=None, icon=No
             attributes["icon"] = icon
 
         self.created_attributes[entity_id] = attributes
+        # Recorded on base, not self: created_attributes is reset per Inverter object, but args
+        # outlives every rebuild, so this is what lets a later construction tell its own dummy
+        # apart from real configuration (#4745 review).
+        if getattr(self.base, "predbat_created_entities", None) is None:
+            self.base.predbat_created_entities = set()
+        self.base.predbat_created_entities.add(entity_id)
 
         if self.base.get_state_wrapper(entity_id) is None:
             self.log("Inverter {} **** Creating dummy entity {} with value {} and attributes {}".format(self.id, entity_id, value, attributes))

apps/predbat/tests/test_inverter.py

@@ -41,6 +41,146 @@ def test_foxess_support_discharge_freeze_matches_foxcloud():
     return failed
 
 
+def test_custom_type_respects_configured_time_entity(my_predbat):
+    """
+    Regression test for issue #4738: a custom inverter definition using a non-HH:MM:SS
+    charge_time_format together with a real time window (has_time_window/has_discharge_enable_time
+    True) must not have its genuinely user-configured discharge_start_time/discharge_end_time/
+    charge_start_time/charge_end_time silently replaced by a self-created dummy sensor.
+
+    Two existing legitimate cases must be unaffected by the fix:
+      - GS_fb00 style (H M format): discharge_start_time is never user-configured at all - the real
+        writes go via separate hour/minute entities - so the dummy must still be created.
+      - SF style (has_time_window False): ships a bare placeholder string, not a real entity - the
+        dummy must still replace it, since there is nothing real to preserve.
+
+    And a dummy Predbat wrote into args itself must not read back as user configuration on a later
+    construction, since args is process-lifetime state and inverters are rebuilt from scratch on the
+    balance path: the dummy has a domain, so a plain "has a dot" test would skip recreating it.
+    """
+    failed = False
+    print("Test: test_custom_type_respects_configured_time_entity")
+
+    saved_args = copy.deepcopy(my_predbat.args)
+    saved_def = copy.deepcopy(INVERTER_DEF.get("TEST_CUSTOM_TIME_ENTITY"))
+    had_created_entities = hasattr(my_predbat, "predbat_created_entities")
+    saved_created_entities = copy.deepcopy(my_predbat.predbat_created_entities) if had_created_entities else None
+
+    try:
+        # Case 1: real entity configured - must be kept, not overwritten
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_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["discharge_start_time"] = ["time.real_discharge_start"]
+        my_predbat.args["discharge_end_time"] = ["time.real_discharge_end"]
+        my_predbat.args["charge_start_time"] = ["time.real_charge_start"]
+        my_predbat.args["charge_end_time"] = ["time.real_charge_end"]
+        my_predbat.args["givtcp_rest"] = None
+
+        Inverter(my_predbat, 0, quiet=True)
+
+        for arg, expected in [
+            ("discharge_start_time", "time.real_discharge_start"),
+            ("discharge_end_time", "time.real_discharge_end"),
+            ("charge_start_time", "time.real_charge_start"),
+            ("charge_end_time", "time.real_charge_end"),
+        ]:
+            got = my_predbat.args[arg][0]
+            if got != expected:
+                print(f"ERROR: test_custom_type_respects_configured_time_entity: {arg} should stay {expected}, got {got} (overwritten by a dummy entity)")
+                failed = True
+
+        # Case 2: nothing configured for this arg - dummy creation must still kick in
+        del my_predbat.args["discharge_start_time"]
+        Inverter(my_predbat, 0, quiet=True)
+        got = my_predbat.args["discharge_start_time"][0]
+        if not (isinstance(got, str) and got.startswith("sensor.")):
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: unconfigured discharge_start_time should get a dummy sensor, got {got}")
+            failed = True
+
+        # Case 3: SF-style bare placeholder (no domain) - must still be replaced by a dummy, not kept
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_TIME_ENTITY_SF"]
+        my_predbat.args["inverter"] = {
+            "charge_time_format": "S",
+            "has_time_window": False,
+            "has_charge_enable_time": False,
+            "has_discharge_enable_time": False,
+        }
+        my_predbat.args["discharge_start_time"] = ["00:00:00"]
+        Inverter(my_predbat, 0, quiet=True)
+        got = my_predbat.args["discharge_start_time"][0]
+        if not (isinstance(got, str) and got.startswith("sensor.")):
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: SF-style bare placeholder should still be replaced by a dummy sensor, got {got}")
+            failed = True
+
+        # Case 4: rebuild. self.base.args lives for the whole process and is never re-read from
+        # apps.yaml, and inverters are rebuilt from scratch on the balance path (execute.py), so by
+        # the second construction args already holds the dummy written by the first. That dummy has
+        # a domain, so a plain "has a dot" test reads it back as user-configured and skips creation
+        # - leaving created_attributes unpopulated on the new object and the state unrestored if it
+        # has gone away.
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_TIME_ENTITY_REBUILD"]
+        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.pop("discharge_start_time", None)
+
+        Inverter(my_predbat, 0, quiet=True)
+        dummy_id = my_predbat.args["discharge_start_time"][0]
+        rebuilt = Inverter(my_predbat, 0, quiet=True)
+
+        if my_predbat.args["discharge_start_time"][0] != dummy_id:
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: rebuild should keep the same dummy id {dummy_id}, got {my_predbat.args['discharge_start_time'][0]}")
+            failed = True
+        if dummy_id not in rebuilt.created_attributes:
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: rebuild should re-register {dummy_id} in created_attributes, got {sorted(rebuilt.created_attributes)}")
+            failed = True
+
+        # Case 5: the inverter type changes between constructions (discovery or an apps.yaml edit).
+        # The dummy id embeds the type, so the one left in args names the old type - it is still
+        # one Predbat created, so it must be replaced with the current type's dummy rather than
+        # preserved as though the user had chosen it.
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_TIME_ENTITY_REBUILD2"]
+        Inverter(my_predbat, 0, quiet=True)
+        got = my_predbat.args["discharge_start_time"][0]
+        if got != "sensor.{}_TEST_CUSTOM_TIME_ENTITY_REBUILD2_0_discharge_start_time".format(my_predbat.prefix):
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: a stale dummy from a previous inverter type should be replaced, got {got}")
+            failed = True
+
+        # Case 6: a real entity a component's automatic_config() discovered (fox.py/gecloud.py
+        # set_arg() select.* time entities) is deliberate configuration even though it never
+        # appears in apps.yaml, so it must be preserved exactly like a hand-written one.
+        my_predbat.args["discharge_start_time"] = ["select.predbat_fox_abc_battery_schedule_discharge_start_time"]
+        Inverter(my_predbat, 0, quiet=True)
+        got = my_predbat.args["discharge_start_time"][0]
+        if got != "select.predbat_fox_abc_battery_schedule_discharge_start_time":
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: an auto-discovered component entity should be preserved, got {got}")
+            failed = True
+    finally:
+        my_predbat.args = saved_args
+        if had_created_entities:
+            my_predbat.predbat_created_entities = saved_created_entities
+        else:
+            if hasattr(my_predbat, "predbat_created_entities"):
+                del my_predbat.predbat_created_entities
+        if saved_def is None:
+            INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY", None)
+        else:
+            INVERTER_DEF["TEST_CUSTOM_TIME_ENTITY"] = saved_def
+        INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_SF", None)
+        INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_REBUILD", None)
+        INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_REBUILD2", None)
+
+    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,
@@ -3283,6 +3423,7 @@ def run_inverter_tests(my_predbat_dummy):
     print("**** Running Inverter tests ****")
     failed |= test_foxess_support_discharge_freeze_matches_foxcloud()
     failed |= test_support_feedin_first_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")